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

Fix audio sample rate on rp2040 #5218

Merged
merged 2 commits into from Aug 25, 2021
Merged

Fix audio sample rate on rp2040 #5218

merged 2 commits into from Aug 25, 2021

Conversation

jepler
Copy link
Member

@jepler jepler commented Aug 24, 2021

There's a simple fix, that uses 64 bit arithmetic where necessary. But I chose to make a more complex fix, because I also noticed that it could take up to 100ms for the sample rate to be found! The new algorithm is faster (I think it runs in log(max_denominator) time, sort of like gcd) and can also give slightly more accurate results due to comparing the frequencies as floating point instead of integers—it only makes a difference in cases where the sample rate was already being met to within 1Hz, so not a big difference.

Closes: #5092

I noticed that the loop over 65535 possible denominators took a long time,
causing up to 100ms wait for a sound sample to start playing!

This algorithm, adapted from an algorithm shown in Python's fractions.py,
is guaranteed to find the best denominator in a small number of steps
(I think log2-many steps but I'm not sure). In practice, it means the time
between samples playing is just 10ms, and some of that is recreating the
sine wave sample in Python each time.

It often finds the same solution as the old code, but sometimes it finds
one a bit better since it compares the ratios using float point instead
of integer arithmetic.
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.

I compared limit_denominator() with the Python implementation. I did not try to understand the algorithm, but it looks like an accurate transcription. Does it sound better?

I misunderstood #5092 and was looking at how PWMOut calculates the frequency divisor and top value. It's not that accurate at low frequencies, but I'm not sure it's any worse than other PWMOut impls.

@microdev1 microdev1 merged commit cd912e1 into adafruit:main Aug 25, 2021
@microdev1 microdev1 added audio bug rp2 Raspberry Pi RP2 Micros labels Aug 25, 2021
@dhalbert
Copy link
Collaborator

I misunderstood #5092 and was looking at how PWMOut calculates the frequency divisor and top value. It's not that accurate at low frequencies, but I'm not sure it's any worse than other PWMOut impls.

Using PWMOut to make musical notes is kind of an edge case anyway, since there are other ways to do it, and is a trade-off of frequency accuracy vs duty-cycle accuracy; the latter is what the current impl favors.

@jepler jepler deleted the issue-5092 branch November 3, 2021 21:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audio bug rp2 Raspberry Pi RP2 Micros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PWM-generated tones may be off-frequency at higher frequencies
3 participants