feat(stream.streamg3_on): Set filter parameters.

[tristpinsm]

I've yet to test this on a SMuRF system (working on getting that going), and that will involve confirming that I've correctly set the default filter parameters, but I figured it would be useful in the meantime to get feedback on how I've implemented this.

[jlashner]

Hi Tristan, thanks for this! It looks good for the most part, see comments below. Mainly I think we need to talk with Matthew / Max about whether its ok to start specifying filter behavior using filter_cutoff if that means the default filter parameters don't match exactly what's currently used.

We'll also want to investigate what filter cutoff was used to generate the default parameters that we currently use.

[tristpinsm]

Thanks for the comments!

Between the two options of specifying the filter parameters as coefficients or order/frequency I chose the latter because it seems more user-friendly, but I agree it's less obvious that it guarantees the standard behaviour. I have investigated how the filter parameters are generated, and the ones I put in the defaults reproduce the filter parameters from SmurfProcessor exactly. I would still like to confirm this on a live system though.

[tristpinsm]

But I'm also happy to store the filter coefficients in the device config and have a user pass order / cutoff to the function. I guess it isn't super clear to me if the device config is meant to be user-facing or not.

[jlashner]

Ah ok that's great! If this produces the exact same filter params then I don't see any problem with setting the default using the cutoff freq like you're doing here. Thanks for checking!

[tristpinsm]

I've confirmed that the filter parameters in the device config produce the default coefficients on a live SMuRF system now.

The coefficients queried before running S.set_downsample_filter(filter_params["order"], filter_params["cutoff_freq"])

[ 1.         -3.74145562  5.25726624 -3.28776591  0.77203984  0.
  0.          0.          0.          0.          0.          0.
  0.          0.          0.          0.        ]
[5.28396689e-06 2.11358676e-05 3.17038014e-05 2.11358676e-05
 5.28396689e-06 0.00000000e+00 0.00000000e+00 0.00000000e+00
 0.00000000e+00 0.00000000e+00 0.00000000e+00 0.00000000e+00
 0.00000000e+00 0.00000000e+00 0.00000000e+00 0.00000000e+00]

and after

[ 1.         -3.74145562  5.25726624 -3.28776591  0.77203984]
[5.28396689e-06 2.11358676e-05 3.17038014e-05 2.11358676e-05
 5.28396689e-06]

I also checked that they are different if I change the cutoff frequency :)

[tristpinsm]

@swh76 may have an opinion on this change?

[jlashner]

Great. We should also check with shawn / make sure we test this, but I believe its ok that we don't pad with zeros when we update the filter params. I think that is just done so the initial buffer is set to a large enough length.

[tristpinsm]

@swh76 @msilvafe Should we try to get this merged before we release a new software stack with the RSSI fix?

[msilvafe]

@swh76 @msilvafe Should we try to get this merged before we release a new software stack with the RSSI fix?

I'm happy with it, remind me how you've tested it so far?

[tristpinsm]

I'm happy with it, remind me how you've tested it so far?

A few comments upstream I checked that the parameters were exactly the same as before using the default values stored in the config. I haven't looked at any data, do you think that is necessary?

[msilvafe]

I'm happy with it, remind me how you've tested it so far?

A few comments upstream I checked that the parameters were exactly the same as before using the default values stored in the config. I haven't looked at any data, do you think that is necessary?

I think what you checked above seems good. But I have Jack's same uncertainty about the zero padding. Perhaps to stay consistent we zero pad the arrays?

[tristpinsm]

I'm happy with it, remind me how you've tested it so far?

A few comments upstream I checked that the parameters were exactly the same as before using the default values stored in the config. I haven't looked at any data, do you think that is necessary?

I think what you checked above seems good. But I have Jack's same uncertainty about the zero padding. Perhaps to stay consistent we zero pad the arrays?

I think this is just a quirk of EPICS (see here). The actual filter doesn't care about the length (here).

The fact I was able to set it and it didn't complain suggests that EPICS is happy without zero-padding.

Would still be nice to get @swh76 opinion.

out of date

[tristpinsm]

Should we go ahead and merge? In any case, one reviewer will need to approve :)

[msilvafe]

Sorry to hold ya up!