Skip to content
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

rp2040/PWMAudioOut: Fix the click at start / stop of playback. #7618

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andrewleech
Copy link

Addresses #5136 for rp2040 for me with the code & mp3 listed in #5136 (comment)

During the testing of this change, in the construct and stop functions I sprinked a bit of:

    printf("quiescent_value: %d\n", self->quiescent_value);
   ...
    printf("\nslice top / cc: %d / %d\n", self->left_pwm.top, (uint16_t)(pwm_hw->slice[self->left_pwm.slice].cc));

To see what values were actually set at the register level for the pwm top and current levels.

Also, in common_hal_audiopwmio_pwmaudioout_stop I added the following to print the "last" pwm value at the end of a track.:

    printf("final: %d\n", (uint16_t)pwm_hw->slice[self->left_pwm.slice].cc);

With this I've found my mp3 starts/ends with a value of ~512 with the pwm top value of 1023.

During construct, the pwm is created with a current pwm level of 0. It then has the top changed to 1023 and pwm set to quiescent_value. For quiescent_value to result in a raw pwm level of 512 I had to remove the >> SAMPLE_BITS_TO_DISCARD on quiescent_value. I'm not too sure why this was originally added and whether removing it will break other usages.

Separetely I added some delays between these steps in construct and the mp3 start. With this it was clear that the jump from 0 to quiescent_value causes a click. Removing the "set to quiescent_value" moves the click to the start of pwm.

I tried changing the initial pwm value to "half of top" in common_hal_pwmio_pwmout_construct() which moves the click to common_hal_pwmio_pwmout_construct but then there's no click for "set to quiescent_value" or for mp3 start.

Long story short, the best solution I've come up with is contruct pwm with 0 (no click), slowly ramp up to quiescent_value of TOP/2 (adding a ~0.5 second startup delay) then let the mp3 place.

I also removed the re-set of pwm levels in stop. With quiescent_value == TOP/2 this could be left in without adding a click on most tracks, however if you've got two tracks that are supposed to be gapless they might not transition at "mp3 zero level" so resetting to quiescent_value will add a click.
I'm not sure if there's any other value in having stop force the pwm back to quiescent_value.

Signed-off-by: Andrew Leech <andrew@alelec.net>
@dhalbert
Copy link
Collaborator

Thank you for doing this work! We haven't tested it yet, but we will.

There is some audio ramping code in https://github.com/adafruit/circuitpython/blob/main/ports/atmel-samd/common-hal/audioio/AudioOut.c for DAC audio output. It sounds like you developed this independently, but it might be worth taking a look at that.

@andrewleech
Copy link
Author

Ah yes, I hadn't seen the (samd specific) ramp functions in https://github.com/adafruit/circuitpython/blob/main/ports/atmel-samd/common-hal/audioio/AudioOut.c#L58 - they've got a bit more going on than the basic / naieve ramp I threw in here. I did very little tuning on my ramp, other than initial versions with ~10 steps over shorter timeframe being able to be audibly heard.
RUN_BACKGROUND_TASKS; certainly seems like a good idea, though shouldn't this be included in any delay functions?
Those delays are going via the DAC, though is there much value in this when it's delaying in between each value anyway?
Though I had thought that making the ramp as an audio sample that fed in the same way as normal audio samples might make it cleaner and easier to get the speed optimised - just make it a low enough frequency it's below hearing.

@dhalbert dhalbert added this to the 8.x.x milestone Feb 21, 2023
@dhalbert dhalbert modified the milestones: 8.x.x, Long term Jun 21, 2023
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.

None yet

2 participants