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 frequency offset for AVR #202

Merged
merged 4 commits into from
Oct 24, 2023
Merged

Conversation

tomcombriat
Copy link
Collaborator

@tomcombriat tomcombriat commented Oct 11, 2023

Hi,

Fixes #201 by fine tuning the timer frequency. Basically, we get closer to the "ideal" timer frequency by this. Output frequency detuning shifts from 0.25% to 0.06%. To test this, I generated sinusoidal tones in Audacity and listen to the beating when superimposing them to the same tone generated by Mozzi. HiFi is 0.06% off in the native version so I did not touch that.

Untested with external audio, but that should behave the same. Of course, this changes the behavior of all sketches on AVR (but to be closer to reality which I think is fine!)

Would love to have confirmation from someone else owning an AVR to be sure I am not looking to fix problems with the particular board I am testing with!

@tomcombriat tomcombriat requested a review from sensorium October 11, 2023 22:03
@tfry-git
Copy link
Collaborator

I have not done any testing. I suppose this makes some sense, though: Timer1.initializeCPUCycles() actually just seems to set the timer compare register to that value. So if I understand that correct (which I am not sure about), a value of 0 means to trigger every timer tick, 1 every other timer tick, and so on.

What I find a bit unsettling, is that the formulas in AVRs own application note do not include the -1.

At any rate, for better readability, I suggest adding braces as (F_CPU/AUDIO_RATE)-1 (or at least use even spacing around "/" and "-").

--- Reference:

https://ww1.microchip.com/downloads/en/Appnotes/Atmel-2505-Setup-and-Use-of-AVR-Timers_ApplicationNote_AVR130.pdf

@tomcombriat
Copy link
Collaborator Author

Hi,

Thanks @tfry-git !

At any rate, for better readability, I suggest adding braces as (F_CPU/AUDIO_RATE)-1 (or at least use even spacing around "/" and "-").

Done.

For the rate, I do not really understand why the -1 is needed, but maybe the timer is losing one clock cycle somewhere? In any case, it seems to be quite closer to the ideal case (there is always a rounding), but I just want to be sure it is not a local defect on the board I am testing on.

@tomcombriat tomcombriat requested a review from tfry-git October 23, 2023 20:23
@tomcombriat
Copy link
Collaborator Author

Any comment/test for this?

I have drafted a new release which could (or not) include this. More importantly it is adding two new boards (Giga and Uno R4). It is one click away from being published, if you do not have concerns I'll do it in a few days!

@tfry-git
Copy link
Collaborator

tfry-git commented Oct 24, 2023

My first approach was to check what the CPU itself has to say about it: I added debug code inside the timer 1 ISR to measure the time spend for 10*AUDIO_RATE samples (based on micros()).

This yields 9994212 micros (with the patch) vs. 10014696 micros (without). Those numbers are fairly stable (with an occasional deviation of 4 micros), and are identical at AUDIO_RATE 16384 and 32768. -> Slightly better with the patch.

I then used a simple updateAudio like this:

AudioOutput_t updateAudio(){
  if ((audioTicks() / 256) %2) return 244;
  return -243;
}

For a scope-friendly rectangle wave at theoretically 32 Hz. On my crap-scope that yielded a stable 32.02 Hz with the patch and 31.95 Hz without. -> Again slightly better with the patch. Results were again remarkably similar for AUDIO_RATE 16384 or 32768 (adjusting the divisor, of course).

Tested using STANDARD_PLUS, only. Also all testing done on a low quality Uno clone, but in particular the experiment using on-board timing with micros() seems to provide fairly solid backing that this is indeed the correct thing to do.

Please do add a comment pointing to the issue / MR near that -1.

@tomcombriat
Copy link
Collaborator Author

tomcombriat commented Oct 24, 2023

Ok, great, thanks for testing! I do not have a digital scope at home, but listing to the "beating" with respect to a calibrated source also yielded to a slight improvement, so it sounds like the way to do it.

I'll add some comments to these tests and merge soon then.

Added documentation for tweak on AVR timer frequency
@tomcombriat
Copy link
Collaborator Author

Added some comments and again made a mess with git trying to squash my last two commits… Sorry for that. Will merge once tests are completed.

@tomcombriat tomcombriat merged commit cf9e173 into sensorium:master Oct 24, 2023
7 of 8 checks passed
@tomcombriat tomcombriat deleted the AVR_misFreq branch October 24, 2023 22:02
tfry-git pushed a commit that referenced this pull request Jan 2, 2024
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.

Output frequency 0.25% off on AVRs
2 participants