-
Notifications
You must be signed in to change notification settings - Fork 406
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
Low bitrate whine fix (DMA pacing timer) #1996
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks very much! I have a couple suggestions to make it better.
I haven't had time to try it out yet. Is there anything you could give as an example code/sample to show the difference in sound quality?
This will also fail the astyle
formatting check. If you're under Linux/Windows WSL you can run tests/restyle.sh
to clean it up.
libraries/PWMAudio/src/PWMAudio.cpp
Outdated
uint16_t bestDenom = 1; | ||
|
||
for (uint16_t denom = 1; denom < max; denom++) { | ||
Serial.println(denom); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the print statement here, please?
|
||
void PWMAudio::find_pacer_fraction(int target, uint16_t *numerator, uint16_t *denominator) { | ||
const uint16_t max = 0xFFFF; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does look like it could take a little bit to find the right numerator/denominator. It's the same problem as the SPI clock change (and others), and I think your method is the cleanest way.
That said. can we cache the last result for numerator/denomiator? Maybe make something like
void PWMAudio::find_pacer_fraction(int target, uint16_t *numerator, uint16_t *denominator) {
const uint16_t max = 0xFFFF;
static int lastTarget;
static uint16_t lastNum, lastDen;
if (target==lastTarget) {
*numerator = lastNum;
*denominator = lastDen;
return;
}
....
<before exiting the function...>
lastTarget = target;
lastNum = bestNum;
lastDen = bestDenom;
*numerator = bestNum;
*denominator = bestDenom;
}
I'm thinking that most use cases will have the same frequency if they do multiple PWMAudio samples. So no need to recalc. And, worst case, we use 64 bits of RAM and a negligible amount of CPU cycles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put 4 samples together in a video(Because i cant upload audio here). First two are after fix and last two before fix.
https://github.com/earlephilhower/arduino-pico/assets/9679272/4b2914d6-5eee-4e04-abee-97fcbb8285c5
Here, two spectrum graphs if it weren't obvious already (Or some got bad hearing)
1 is 8000hz samplerate before fix and 2 is 8000hz after.
Also thank you for the feedback! I will fix those right away and see what i can do.
I added caching with statics as requested, this will skip floating math when its not needed. I removed the debugging print. And i ran the restyle in WSL. Hope these changes are passing the tests! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work, thanks!
When using low bitrates like 8000hz or 11025hz as example, PWMAudio will produce a high pitched whine(same as sample rate hz), that sometimes can overwhelm the audio.
This is because the DMA transfers (sample rate) are synced to the PWM wave generation (pwm frequency).
In the RP2040 datasheet under DMA 2.5.5.1, there is a section about pacing timers.
This is a timer that triggers the DREQ every n clock_sys ticks.
Now this is replacing the PWM synced method, which allows the PWM wave generation to have a different frequency than the sample rate.
There is one downside;
The begin() function CAN take quite a few milliseconds worst case to find the perfect fraction for the pacing timer. As the pacer has to be configured with a numerator and a denominator.
It has to find the right fraction based on CPU frequency and bitrate target.
For backwards compatibility and to prevent confusion, I kept the old functions.
setFrequency is still bit rate and setPWMFrequency is to set the PWM.
Now begin has an override so you can select either the usual begin(bitrate) OR begin(bitrate, pwmfreq).
If there is anything to be changed, let me know. (This is my first contribution too!)
Thanks!