FilterReview: features to be implemented

[IamPete1]

Somewhere to note down all the features that we would like:

• More speed! Faster filter calculation would allow auto-re plot.
• Show filter tracking on averaged FFT plots (FilterReview: add notch tracking to FFT plot #11)
• Zero pad FFT data to allow running on batch sizes that are not a power of 2. (also do some reading to check that this is mathematically valid). Some reading suggests that zero padding might be a bad idea Or make the batch length selection box move in powers of 2. (Done this bit) Make windows per batch option take steps such that the window size is a power of 2.
• Add plot with general flight data, (roll, pitch, throttle) to allow selection of "good" portion of flight for FFT (FilterReview: add flight data plot for setting time range of analysis #12)
• Add param descriptions and bitmask helper (possibly auto generated from param meta data) (Layout generation from param metadata #30)
• Add more text to page describing how it works in more detail. (FilterReview: add lots of tool tips #106)
• Add link to filter tool for more detailed look at filter bode plots (FilterReview: add button to open in filter tool #23)
• Support accels?
• Add loading spinner to stop dead page warnings, loading page added does not spin and will not stop dead page warnings. all: add loading overlay for logs #55
• User configurable window overlap? Currently hard coded to 50%
• Highlight the primary sensor.
• Prompt to select estimated post filter if not shown and filter param change is done.
• Option to show logged notch center frequencies (FilterReview: add the ability to plot logged notch frequency on spectogram #108)
• Double check ESC notch source correctly avoids dropouts. There seems to be a bug in AP's logging here, not sure we can tell dropout vs genuine 0 rpm.
• support 4th and 5th IMU
• Warn if lots of missing data

[jkordani]

ESC Telem with "MultiSource" deselected doesn't respect the scale value of the _REF field. I am working on a PR to fix that.

Additionally, I'd like to have the _REF field used as a scale for multisource as well, but that is not supported in AP firmware yet (and I'll probably make a PR for that too)

[IamPete1]

ESC Telem with "MultiSource" deselected doesn't respect the scale value of the _REF field. I am working on a PR to fix that.

I think that is what I would have expected. The way to test is to turn on notch logging and compared the estimated to the logged on the spectrograph.

[jkordani]

Not sure I understand your response, to be clear
https://github.com/ArduPilot/ardupilot/blob/15376a4908cc1a0019f692b495085688736f84e0/libraries/AP_Vehicle/AP_Vehicle.cpp#L866
If the filter has the option set for HarmonicNotchFilterParams::Options::DynamicHarmonic which in the filter review tool corresponds to "MultiSource", then individual filters are added per esc telem freqency. If not, then the average is targeted. In the AP code, the average is scaled by the _REF parameter, in the WebTools it is not.

Edit: I do intend to get some experimental data, but this particular craft's flight windows are hard to come by, so I was hoping to get some insight from the tool ahead of time.

Edit 2: In my fork, I added the ref scaling parameter to the identically shaped code in the trackers/EKF.js, That resulted in the notch filter centers shifting visually in the logging overlay, but not being used in the transfer functions. When I added the scaling to the smaller get_target function, then I get scaling also with MultiSource, which I don't think is the way AP currently works and would be an incorrect feature at the moment

[IamPete1]

Not sure I understand your response, to be clear
https://github.com/ArduPilot/ardupilot/blob/15376a4908cc1a0019f692b495085688736f84e0/libraries/AP_Vehicle/AP_Vehicle.cpp#L866

Hum, that is quite odd. As you say, it looks like were doing it wrong. I guess most everyone uses per-ESC if they have the feedback (that is what I would suggest you do too).

[jkordani]

I have counter-rotating motors, so I have my first noise peak is at 1/2 the motor RPM and hence need to scale the targeted frequencies down by 1/2 of the reported RPM. The AP doesn't support scaling on per-ESC tracking (which frankly looks like a simple oversight, I'm sure my prop style is uncommon), but it does support scaling on avg-ESC and RPM tracking. In the short term, I we'll see how far we get with avg-ESC with scaling, but I plan on submitting a PR to add scaling on per-ESC as well.

[IamPete1]

I have counter-rotating motors, so I have my first noise peak is at 1/2 the motor RPM and hence need to scale the targeted frequencies down by 1/2 of the reported RPM.

I don't understand how it would be half?

[jkordani]

I have a motor that has a pair of props on the top, and a pair on the bottom. They rotate in opposite directions, and there seems to be a noise source at 1/2 the motor RPM. The upper and lower blades pass each other at 1/2 motor revolution

[IamPete1]

The upper and lower blades pass each other at 1/2 motor revolution

Right, but half a revolution would be twice the frequency.

Double check your motor poles is set correctly. It could be the ESC RPM is reporting double and your really seeing the fundamental.

[jkordani]

Nope I'm dumb, it would be twice the frequency, good catch.

Edit: Yes, we have a known issue with our ESC config, I missed a memo

[amilcarlucas]

I'm using multisouce throttle notch. I'm not logging the throttle, but there are FCN and FTN messages in the .bin file. Can the tool fall back to those messages when throttle information is missing?

Does it make sense? Should I PR it?

[IamPete1]

I'm using multisouce throttle notch. I'm not logging the throttle, but there are FCN and FTN messages in the .bin file. Can the tool fall back to those messages when throttle information is missing?

Hum, it sounds quite complicated. So I understand, you would use the logged notch frequencies to reverse the scaling and get back a throttle value. Then once you have the throttle you could run the maths forwards again to test param changes. In theory it would work, but there is some information lost, we constrain up to min frequency for example. You also need to make sure the filter config in the log is also throttle based.

Currently we get throttle from RATE.AOut, I think you would be better to add support for getting it from CTUN on copter if RATE is not available.

[amilcarlucas]

@IamPete1 thanks for the suggestion, I implemented that in #293 and it works fine here!
It's a lot simpler than the solution I was previously using.