Repository: quartiq/stabilizer (language: rust)

Issue:
IIR filter coefficients set incorrectly
Hello,

Thanks for keeping up the development of the stabilizer. I've just tried to set a low-pass filter using your script at https://github.com/quartiq/stabilizer/blob/main/py/stabilizer/iir_coefficients.py, but I think the IIR BA coefficients are returned with the wrong sign. Specifically, lines 59-61 say
```
#     Returns:
#       [b0, b1, b2, -a1, -a2] IIR coefficients to be programmed into a
#       Stabilizer IIR filter configuration.

```
However, in line 144, it's actually +a1, not -a1 being returned. Therefore, this implements the wrong filter.

It seems to me that this is a problem for all filters (+a_i returned instead of -a_i), but it seems OK for the PID (I haven't tested yet).

I'm not a 100% sure why -a_i needs to be sent to the stabilizer, but when I change a1 to -a1 in line 144 I do get the expected, correct behaviour - so it's probably just a matter of updating the Python script.


Discussion / hints:
iir_coefficients is out of date w.r.t. recent changes in `idsp`.
Is there any chance you could release a temporary fix for this coefficient sign issue while presumably a more general rewrite lands? 
Further to the above, I think the PID coefficients are also returned with the wrong sign, and the b0 coefficient of the high-pass filter is wrong (it has an extra factor of f0_bar in the numerator).

Furthermore, I'd like to get the PID setpoint (the x offset) working, which it seem to me is not implemented in the this python file - I wonder if there is any description somewhere regarding how that could be done?
Some background on PID/IIRs is here: https://hackmd.io/IACbwcOTSt6Adj3_F9bKuw
A proven implementation is here: https://github.com/quartiq/idsp/blob/main/src/iir/pid.rs
while the underlying `Biquad` impl that `dual-iir` uses is this: https://github.com/quartiq/idsp/blob/main/src/iir/biquad.rs
It's on my list to get iir_coefficients back up to speed (i.e. to the state that `idsp` is in).
Thanks very much! I've already read the PID primer which was very useful. I'm less familiar with Rust, hence the need for the python interface.

Fix patch:
diff --git a/py/stabilizer/iir_coefficients.py b/py/stabilizer/iir_coefficients.py
index 5675d2a64..b85d43a15 100644
--- a/py/stabilizer/iir_coefficients.py
+++ b/py/stabilizer/iir_coefficients.py
@@ -141,7 +141,7 @@ def lowpass_coefficients(args):
     b0 = args.K * (f0_bar / (1 + f0_bar))
     b1 = args.K * f0_bar / (1 + f0_bar)
 
-    return [b0, b1, 0, a1, 0]
+    return [b0, b1, 0, -a1, 0]
 
 
 def highpass_coefficients(args):
@@ -149,10 +149,10 @@ def highpass_coefficients(args):
     f0_bar = pi * args.f0 * args.sample_period
 
     a1 = (1 - f0_bar) / (1 + f0_bar)
-    b0 = args.K * (f0_bar / (1 + f0_bar))
+    b0 = args.K / (1 + f0_bar)
     b1 = - args.K / (1 + f0_bar)
 
-    return [b0, b1, 0, a1, 0]
+    return [b0, b1, 0, -a1, 0]
 
 
 def allpass_coefficients(args):
@@ -164,7 +164,7 @@ def allpass_coefficients(args):
     b0 = args.K * (1 - f0_bar) / (1 + f0_bar)
     b1 = - args.K
 
-    return [b0, b1, 0, a1, 0]
+    return [b0, b1, 0, -a1, 0]
 
 
 def notch_coefficients(args):
@@ -179,7 +179,7 @@ def notch_coefficients(args):
     b1 = - (2 * args.K * (1 - f0_bar ** 2)) / denominator
     b2 = args.K * (1 + f0_bar ** 2) / denominator
 
-    return [b0, b1, b2, a1, a2]
+    return [b0, b1, b2, -a1, -a2]
 
 
 def pid_coefficients(args):
@@ -216,7 +216,7 @@ def pid_coefficients(args):
     b = [i/a[0] for i in b]
     a = [i/a[0] for i in a]
     assert a[0] == 1
-    return b + [-ai for ai in a[1:]]
+    return b + a[1:]
 
 
 def _main():


Test patch: