Skip to content

rp2040: Avoid screeches due to audio underflow during flash writes #8123

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.

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

Merged
merged 2 commits into from
Jun 27, 2023

Conversation

jepler
Copy link

@jepler jepler commented Jun 26, 2023

By pausing audio during flash writes, the worst screeching of #8121 is avoided. I don't consider this a full fix, but it greatly improves the by far most common scenario in which the problem occurs, and can't otherwise be worked around

Tested on rp2040 prop feather with a midi synth playing arpeggios. When writing to the flash e.g., with

dd bs=512 count=32 if=/dev/zero of=/media/jepler/CIRCUITPY/boop

the audio goes "tap tap tap tap" during the flash write instead of the squawking.

This isn't a 100% fix; it will still glitch out, including during USB enumeration which must be taking a long time without servicing background tasks. Adding a delay if not usb-connected at startup ameliorates this greatly.

to work around the screech at startup, I have found that this is adequate:

def maybe_wait():
    deadline = time.monotonic() + 1
    while time.monotonic() < deadline and not usb_cdc.console.connected:
        pass

@todbot I know this problem has vexed you so your testing would be appreciated. It's still not perfect but I think you'll be able to judge if it's an improvement on the status quo.

By pausing audio during flash writes, the worst screeching of adafruit#8121
is avoided. I don't consider this a full fix, but it greatly improves
the by far most common scenario in which the problem occurs.

Tested on rp2040 prop feather with a midi synth playing arpeggios. When
writing to the flash e.g., with
```
dd bs=512 count=32 if=/dev/zero of=/media/jepler/CIRCUITPY/boop
```
the audio goes "tap tap tap tap" during the flash write instead of the
squawking.

This isn't a 100% fix; it will still glitch out, including during USB
enumeration which must be taking a long time without servicing background
tasks. Add a delay if not usb-connected at startup ameliorates this
greatly.
@todbot
Copy link

todbot commented Jun 26, 2023

Just tested this on a QTPY RP2040 w/ I2S DAC (PCM5102A) and, yes, SO MUCH BETTER.
(in general, I've found I2S to be more screechy than PWM, with M4 built-in DAC being least screechy).

Now, on reset with USB connected to MacOS: it's mostly quiet with maybe a small bit of chugga-chugga noise (that hilariously to me, sounds reminiscent of GSM cell interference)

This is a great improvement. I may still tell people to include a 5-second delay at the top of their code.py so they don't get the start/stop/restart that always happens to devices, at least on MacOS.

@tannewt tannewt self-requested a review June 26, 2023 17:31
@jepler jepler marked this pull request as ready for review June 26, 2023 18:00
@jepler
Copy link
Author

jepler commented Jun 27, 2023

fwiw I briefly tried implementing the "DMA controller sets its own next-DMA address" scott suggested but it didn't work right off the bat as a simple change.

It seems like either you'd end up using 4 DMA channels (2 to actually do transfers + 2 to set transfer start addresses), or you would have to use a different method than checking which DMA interrupt fired to know which memory buffer to fill.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Code looks good to me. Suggestion of more descriptive name than cookie.

I would feel comfortable putting this in 8.2.0. How about you?

@jepler jepler force-pushed the issue8121-workaround branch from 29ce182 to 3853661 Compare June 27, 2023 14:59
@jepler
Copy link
Author

jepler commented Jun 27, 2023

I'd like for this to be in 8.2. Do I need to re-target the branch or is main still where 8.2 comes from?

I took the "cookie" -> "channel_mask" suggestion, just not via the "accept suggestion" button.

@jepler jepler requested a review from dhalbert June 27, 2023 15:01
@dhalbert
Copy link
Collaborator

I'd like for this to be in 8.2. Do I need to re-target the branch or is main still where 8.2 comes from?

No problem - it's still main. I will make a branch when I make 8.2.0 final.

Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

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

Thanks! This is a big improvement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants