8000 Nrf i2s buglets by jepler · Pull Request #2321 · adafruit/circuitpython · GitHub
[go: up one dir, main page]
Skip to content

Nrf i2s buglets #2321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Nov 26, 2019
Merged

Nrf i2s buglets #2321

merged 3 commits into from
Nov 26, 2019

Conversation

jepler
Copy link
@jepler jepler commented Nov 25, 2019 •
edited
Loading

These changes address two i2s buglets:

  • Ensure the i2s HW is deinitialized when the i2sout object is
  • Add a finali(s/z)er to the shared-bindings i2s object so it's deinitted on del
  • fix a problem where pausing would cause avoidable pops

These changes partially address #2255

@jepler
nrf: i2sout: Only update hold_value when appropriate
aabb56c 
If we put no samples into the buffer, then there is no last
sample to fill out hold_value with.  (and, in fact, the expression such
as *(uint32_t*)(buffer-4) is outside an allocated region)

Detect this condition, and leave the prior value in place.

This improves clicks heard when pausing and resuming a waveform.
@jepler
nrf: i2sout: Ensure hardware I2S is stopped at deinit
1c6c9a3 
.. otherwise, it may be possible under some scenario, for the background
task to continue and overwrite unrelated memory.
@jepler
shared-bindings: I2SOut: Ensure object is deinitialised
5115fda 
(or deinitialized, for those of us on this side of the pond)

Otherwise, a sequence like
```
audio = audiobusio.I2SOut(bit_clock=board.D6, word_select=board.D9, data=board.D10)
sine_wave_sample = audiocore.RawSample(sine_wave)
audio.play(sine_wave_sample, loop=True)
del audio
```
could free the memory associated with audio without stopping the
related background task.  Later, when fresh objects are allocated within
a now-freed memory region, they can get overwritten in the background
task, leading to a hard crash.

This presumably can affect multiple I2S implementations, but it was
reported against the nRF one.
@jepler
Copy link
Author
jepler commented Nov 25, 2019

Testing performed: on an nrf52840, ran the following test which crashed reliably before the finaliser change:

import array
import audiobusio
import audiocore
import board
import time

sine_wave = array.array("H", [0] * 24)
audio = audiobusio.I2SOut(bit_clock=board.D6, word_select=board.D9, data=board.D10)
sine_wave_sample = audiocore.RawSample(sine_wave)

audio.play(sine_wave_sample, loop=True)
time.sleep(1)
del audio
time.sleep(1)
import gc
time.sleep(1)
gc.collect()
time.sleep(1)
x
time.sleep(1)

@tannewt tannewt self-requested a review November 25, 2019 17:55
tannewt
tannewt approved these changes Nov 25, 2019
View reviewed changes
Copy link
Member
@tannewt tannewt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Thanks for squashing bugs!

@tannewt tannewt merged commit 2653455 into adafruit:master Nov 26, 2019
@jepler jepler deleted the nrf-i2s-buglets branch November 3, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tannewt tannewt tannewt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jepler @tannewt
0