More constant resonance over the frequency range for LowPassFilter

[tomcombriat]

Hei,

This is a first response to #144 : the calculation explained in the last comment of that issue is applied here.

Tests with high cutoff values (for instance 200 for the 8 bits version) of the initial filter indeed showed no resonance at all. This version keeps a more constant resonance over the whole range (slightly less at very high frequencies).

Note that this is not heavier on the MCU.

PS: I do not understand why is git showing three commits, with only one file changed… I tried to rebase my branch with no success… Sorry for that.

[tfry-git]

Don't worry about the three commits, we can just squash merge them (incidentally, squash merging is also what started the trouble, as it has caused your master branch and sensorium:master to diverge; after this is merged, use git reset --hard origin/master to get back in sync.)

As for the MR itself, I'll have to admit, I have no idea. Anybody else?

[tomcombriat]

When analyzed carefully, the original version has a really non-constant resonance (from +14dB at low frequency to as less as 0.1dB at high frequencies). It diverges with the frequency approaching 0.

Unfortunately, the version proposed here has an overflow in ucfxmul(q, SHIFTED_1 + cutoff); : SHIFTED_1 + cutoff do not fit into the su type anymore, which also does not give a correct resonance. In order to have a more constant Q with the calculation explained I see only three solutions:

• increase the container size for SHIFTED_1 + cutoff to the next type. Note that this calculation will only affects performances when cutoff or resonance are changed which happens way less than the actual filter calculation which is already done at the bigger size. As these parameters are rarely (never?) changed at audio_rate but usually at control_rate, the impact on performances is likely very small.
• reduce both cutoff frequency and resonance parameters range to 0-127, allowing SHIFTED_1 + cutoff to fit in a su container holding, for the 8bits version, up to 255.
• actually using the division in the original post hence calculating fb = q + q/(1-f) (which I am afraid will be slow and will also overflow at high frequency cutoff…).

Any idea?

I'll perform some tests and report.

EDIT: actually, the ucfxmul(q, SHIFTED_1 + cutoff); calculation is already done in a casted bigger container, hence probably just changing the second input type of this function, and a cast when computing SHIFTED_1 + cutoff will do the trick. In that case the overhead will only be on passing the arguments, not on computing and should be minimal. Will test that (and benchmark) to see.

Best,

[tfry-git]

Yeah, some benchmarking is a good idea, although I believe your reasoning is sound, and there should not be any overhead.

[tomcombriat]

In the end, I had to increase the cast in ucfxmul because, SHIFTED_1 + cutoff is now more than the size of su hence its multiplication with q (also of size su) potentially overflow the size of two su, especially at high resonance.

Hence, the multiplication is now happening to the next bigger size container, which I expect will affect the performances. How much I do not know yet, I have to benchmark it.

At least, the resonance as a function of the frequency is now flatter (not flat as the approximation becomes more and more invalid at high cutoff frequencies). As an example, a cutoff frequency of 200 now display a resonance of 4dB whereas it used to be less than a dB. The difference is quite hearable, especially at "high" frequencies: it sounds more like a resonant.

Depending on the results of the benchmark we can decide if it is worth or not. I'll keep it as draft for now.

[tomcombriat]

Ok, I am fairly satisfied now.

My previous version (commit d6dd48d) was a bit slow. At the cost of one extra byte of memory I am now very close to original performances, with a more consistent Q.

Benchmark for setCutoffFreqAndResonance (Arduino Mega, 8 bits version of the filter):

LPF version	Timing (micro seconds)	Ratio with original
original	4.92	1
d6dd48d	7.68	1.56
last version	5.48	1.12

The drop in performance for the fast version is only caused by a binary shift. This version reduces a bit the accuracy of the Q correction with frequency, but stays very close to the "slow" version. Weirdly, these numbers are less than the comments in the original code… Or the compiler had become very smart (I had to force it to get these numbers) or the Mega is way more powerful than an Uno (I do not really believe so…).

A few examples

High frequency

Original (cutoff: 200, resonance 255)

This PR (cutoff: 200, resonance 255)

The resonance is now present, even thought it is only +4dB

Low frequency

Original (cutoff: 20, resonance 200)

This PR (cutoff: 20, resonance 200)

(the high frequency noise is due to saturation).

The two versions are equivalent at very low frequency, as the frequency increases the original version gets its resonance tamed out very fast. This version also shows a decrease in the resonance, because the approximation of $1/(1-f) \approx 1+1$ does not hold off, but this decrease is less pronounced, still showing a small resonance at high frequencies.

Tested for both versions of the filter (8 & 16 bits) on both 8 bitter (Arduino mega) and 32 bitters (Teensy 3.2)

[tfry-git]

Looks good to me.

Could you insert a comment into the code explaining the idea / tradeoff behind the calculation?

[tfry-git]

Afterthought: @sensorium , would it make sense to add @tomcombriat as "Collaborator"?

[sensorium]

Great idea, Tom's certainly contributing a lot.

[tomcombriat]

Hi, Thanks very much @sensorium for the promotion and @tfry-git for suggesting it! Considering that my PR usually get substantially improved before merging from discussion with you, be assured that I'll use push rights responsively and probably single-handedly only for small fixes. Cheers,