Tympan / Tympan_Library

Arduino/Teensy Library for Tympan Open Source Hearing Aid
MIT License
116 stars 31 forks source link

Sample rates set below 9888hz cause incorrect MCLK generation that leads to sample rate problems #75

Closed coreysmccall closed 1 week ago

coreysmccall commented 2 weeks ago

In Tympan_Library/src/output_i2s_F32.cpp, the AudioOutputI2S_F32::config_i2s() function generates the MCLK signal by setting up PLL4 ("audio clock") and SAI1 dividers. This MCLK signal then determines the effective sampling rate of the codec.

Unfortunately, the minimum sample rate for the equations currently in this function is 9888Hz. If 9887Hz or lower is used, it causes the SAI1 post divider n2 to be > 64, which is too large for the 6-bit register that holds it (CCM_CS1CDR_SAI1_CLK_PODF). This is an issue for 8KHz (or below) audio applications.

You can observe the actual sample rate by probing WCLK (same frequency as sample rate) or checking the time between when audio blocks are ready. For example, if you set sample_rate_Hz = 8000.0f, the actual resulting sample rate is 40,000Hz because of the overflow.

To resolve: can n1 = 8 be used instead of n1 = 4 for the SAI1 pre-divider? This will allow for sample rates down to 4944Hz. It will also limit the max sample rate from 1,265,625Hz to 632,812Hz. At least on my Tympan Rev:E, I cannot sample above this new limit without problems anyways though, even with a large block size like 256.   I am not sure if changing this has any other implications. The PaulStoffregen/Audio library uses SAI1 pre divider = 4, but that library is not designed to easily change sample rates like this one is.

coreysmccall commented 2 weeks ago

I can submit a pull request if you don't think that changing the SAI1 pre-divider like above will have another consequence. It seems to work for me.

chipaudette commented 2 weeks ago

Thanks for finding this issue!

Let me take a look today and I'll let you know...

Chip

chipaudette commented 1 week ago

I have confirmed that this is an issue. On a Tympan RevE, using the example 02-Utility/OutputTone.ino:

coreysmccall commented 1 week ago

Thanks for looking into this @chipaudette. Let me know if you would like me to open a PR (If allowed, I have not contributed here before). On mine, I just changed this SAI1 pre-divider to 8 instead of 4 in config_i2s() so that < 9888 hz works.

chipaudette commented 1 week ago

I'm actively working it right. Using 8 seems to work, which is great...but it fails again if one tries a sample rate of 4943 or lower, as you said.

I'm trying to modify the library so that it dynamically adjusts n1 in order to keep n2 at a value of 64 or below. Unfortunately, the system doesn't seem to work if I keep increasing n1 above 8 (ie, a value of 10 and 12 don't seem to work). So, that's a bummer.

I'm still working it...

chipaudette commented 1 week ago

In testing the code by ear via my consumer-level earbuds, lowering the sample rate does allow background noise to appear. I'm hearing hiss as the sample rate is dropped below 8 kHz.

To my ear, the frequency content of the noise starts in the high frequencies and then adds more lower-frequency content as the sample rate is lowered.

IMO, what I'm hearing is probably best characterized as being "out of band" noise (ie, noise above Nyquist) that becomes audible only because the above-Nyquist frequencies finally get low enough to be easily heard once the sample rate drops below 8 kHz.

chipaudette commented 1 week ago

Well, I can't figure out why it's not working with n1 = 10 or 12 or 16. It seems stuck with a max of n1 = 8.

I've committed my changes to the repo. It'll now auto-select n1 depending upon what sample rate you request. As a result, it now works down to a sample rate of 4988 or so. I'd like to to work at even lower sample rates, but at least it's better than it was.

Thanks for finding this issue!

(This foreign-language page helped me a tiny bit: https://www.wpgdadatong.com.cn/blog/detail/41412)

coreysmccall commented 1 week ago

Sounds good. Yes, a more general fix would be better.

So, n1 (SAI1 pre-divider) is only 4 bits wide unfortunately, IMXRT1060 manual page 1056.

The other constraint I see is the minimum output of the MCLK generator since PLL4 needs to output between 650 and 1300MHz (IMXRT1060 manual page 1028). So, if MCLK must be 64x the actual sample rate, the minimum MCLK is

650MHz -> ANALOG_PLL_AUDIO_POST_DIV (/4) -> SAI1 PRE DIVIDER (/8) -> SA1 POST DIVIDER (/64) -> ACTUAL SAMPLE RATE (/64) = 4,959Hz.

I haven't listened to the 8KHz audio enough yet to know if it should sound better than an idea 8KHz audio source. We will test some more here.

chipaudette commented 1 week ago

Oh, man, that's a good find! 650 MHz minimum! Dang!

Also, do you mean that the SAI1 pre-divider is only 3 bits wide, not 4 bits? If it's 3 bits wide (as the table on page 1056 suggests), that explains why I can only use values up to 8.

coreysmccall commented 1 week ago

Yes, it is 3 bits sorry for the typo. Maybe there's an alternative way to get below 4,959Hz actual sample rate, but for now this should be low enough for most audio applications. 8KHz was just excluded before.

chipaudette commented 1 week ago

Can we close this issue?

coreysmccall commented 1 week ago

Thanks, I'm closing it since it works on 8khz now. Thank you for the quick fix!!

I see the commit- It fixes the original problem (letting the post divider overflow), but now the input divider (n1) is susceptible to the same issue if the sample rate is set lower than 4,959Hz. We gained a lower sample rate minimum now though which fixes this particular issue. I think it's the best we can do without heavy lifting because of the PLL constraints. Thanks again!