Improvements to Audio DMA management #2043
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As identified in adafruit#1908, when both AudioOut and PDMIn are used, hard locks can occur. Because audio_dma_stop didn't clear audio_dma_state[], a future call to audio_dma_load_next_block could occur using a DMA object which belongs to PDMIn. I believe that this Closes: adafruit#1908 though perhaps it is still not the full story. Testing performed: Loaded a sketch similar to the one on adafruit#1908 that tends to reproduce the bug within ~30s. Ran for >300s without hard lock. HOWEVER, while my cpx is no longer hard locking, it occasionally (<1 / 200s) announces Code done running. Waiting for reload. (and does so), even though my main loop is surrounded by a 'while True:' condition, so there are still gremlins nearby.
It turns out the "disable" error will fire in practice, we'll fix that next.
audio_dma_stop can be reached twice in normal usage of AudioOut. This may bear further investigation, but stop it here, by making the function check for a previously freed channel number. This also prevents the event channel from being disabled twice. The first stop location is from audio_dma_get_playing, when the buffers are exhausted; the second is from common_hal_audioio_audioout_stop when checking the 'playing' flag.
|
@tannewt while I based this off master, I would be happy to roll a new request that would go against 4.x.x. Let me know. |
tannewt
requested changes
Aug 7, 2019
These were most useful debugging, but because this code can be reached "outside of the VM", it's not actually permitted to throw exceptions here.
Some ports which actually don't have audioio or audiobusio were still calling into audio_dma_background(). This wasn't an error until the assignment to audio_dma_state in audio_dma_stop was added, though it's not clear why.
|
The linker error that travis found is a little weird, because it concerns an undefined symbol that I didn't undefine or add references to, inside a function that I didn't modify. That said, the fix I found and which is now reflected in this PR is to change one |
|
@tannewt looks like travis is OK with the latest changes, so I'm ready for your review again. TIA! |
tannewt
approved these changes
Aug 12, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This includes a fix which directly addresses the original problem, as well as some further fixes for other weirdness I noticed along the way. In particular, while I saw some spurious soft-resets with only the first fix, I believe the change to
audio_dma_stopsomehow has the side effect of fixing some spurious soft resets I was seeing during testing. Or, at least, they changed from 1 per ~300s to <1 per 30,000s while running the #1908 reproducer.Testing performed: Ran the reproducer script overnight on a CPX, no hard lockup.
@c-k-dillard said the first commit in this PR fixed things for them too.