Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Notch filter #13549

Merged
merged 6 commits into from
Jan 6, 2020
Merged

Notch filter #13549

merged 6 commits into from
Jan 6, 2020

Conversation

bresch
Copy link
Member

@bresch bresch commented Nov 21, 2019

When vibrations are coming from structural resonances, they are usually at low frequencies and setting a lowpass filter to avoid feedback amplification often ruins the phase margin of the system. Reducing the phase margin limits the loop gains and leads to poor rate tracking and disturbance rejection.
Structural resonances have usually a very narrow frequency amplification band and depending on the material, only lightly damped (carbon fiber plates have really little damping).

The notch filter is a good solution to this problem as the phase loss before the cutoff frequency is much less than on a 2nd order Butterworth low-pass filter as shown on the plot below:

Notch_vs_Butter_Group_delay

As an example, take a drone that has a resonance at 20Hz and more noise at 80Hz. Currently, a low-pass filter has to be set below 20Hz to effectively reduce the amplitude of the resonance. With this PR, a notch filter can be placed at 20Hz (with the required bandwidth) and a low-pass Butterworth between 50 and 70Hz (depending on the amplitude of the noise at 80Hz). Then gains robustness and can be returned to gain more performance if required.

Also, since most of the noise comes from the blades passing over the arms, we can in the future create a bank of notch filters that tracks the frequency of each rotor is an RPM feedback signal is available (DShot, UAVCAN for example).

Derivation of the discrete time notch filter (bilinear transform) taken from :
Rusty Allred, Digital Filters for Everyone - 3rd edition, 2015

Edit: add group delay graph

@bresch bresch added this to the Release v1.11.0 milestone Nov 21, 2019
@bresch bresch requested review from dagar and bkueng November 21, 2019 10:17
@bresch bresch self-assigned this Nov 21, 2019
@LorenzMeier
Copy link
Member

Nice!

@dagar
Copy link
Member

dagar commented Nov 21, 2019

Looks good. If we want to get fancy later we might want to consider unifying with LowPassFilter2p and have a generic biquad cascade IIR base or template.

Do you want to take a pass in this PR to start using the filter?

Add it here:
https://github.com/PX4/Firmware/blob/1371887578af6af9fbd3775104c505ce1fc69e92/src/lib/drivers/gyroscope/PX4Gyroscope.hpp#L75

Parameters:
https://github.com/PX4/Firmware/blob/1371887578af6af9fbd3775104c505ce1fc69e92/src/lib/drivers/gyroscope/PX4Gyroscope.hpp#L88

Then actually filter the data here.
https://github.com/PX4/Firmware/blob/1371887578af6af9fbd3775104c505ce1fc69e92/src/lib/drivers/gyroscope/PX4Gyroscope.cpp#L123-L124

I was also considering doing all the filtering downstream on only the primary (#12730). I'll revisit that idea later after killing off DriverFramework. Once it's out of the way it'll be easier to change things in the sensor pipeline.

@bresch bresch marked this pull request as ready for review November 25, 2019 09:02
* @reboot_required true
* @group Sensors
*/
PARAM_DEFINE_FLOAT(IMU_GYRO_NOTCH_W, 20.0f);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably overthinking this, but another option could be to use NF for "notch filter" and then have room to be a bit more descriptive with frequency and bandwidth.

IMU_GYRO_NOTCH_F -> IMU_GYRO_NF_FREQ
IMU_GYRO_NOTCH_W -> IMU_GYRO_NF_BW

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea!

@bresch
Copy link
Member Author

bresch commented Dec 12, 2019

@dagar I rebased, changed the names of the parameters and implemented the "array" version of NotchFilter for the new updateFIFO function. Can you review again please?

@@ -54,7 +54,7 @@ class LowPassFilter2pArray : public LowPassFilter2p
*
* @return retrieve the filtered result
*/
inline float apply(const int16_t samples[], uint8_t num_samples)
inline float apply(const float samples[], uint8_t num_samples)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is ugly, but it was done to avoid copying all the data again. Let me see if it matters (we're barely using the FIFOs currently).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to do that because I wanted the NotchFilterArray template to output a float array, so then the lowpass needs to accept a float array as well

@dagar
Copy link
Member

dagar commented Dec 12, 2019

I'll find some flash to free up to get this in.

@bresch
Copy link
Member Author

bresch commented Dec 18, 2019

@dagar What would be the next step here? Is the array implementation too heavy?

@dagar
Copy link
Member

dagar commented Jan 6, 2020

@dagar What would be the next step here? Is the array implementation too heavy?

The extra stack is a bit of a concern. The IMU drivers using FIFOs aren't really used yet. Let's bring in the core change and readd the filter array (FIFO data) once we're able to test easily.

@dagar dagar merged commit 5adf23a into master Jan 6, 2020
@dagar dagar deleted the dev-notch-filter-upstream branch January 6, 2020 21:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants