sensorium / Mozzi

sound synthesis library for Arduino
https://sensorium.github.io/Mozzi/
GNU Lesser General Public License v2.1
1.06k stars 185 forks source link

Fix frequency offset for AVR #202

Closed tomcombriat closed 10 months ago

tomcombriat commented 10 months ago

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!

tfry-git commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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.

tomcombriat commented 10 months ago

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.