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

MC rate controller: add low-pass filter for D-term #8920

Merged
merged 6 commits into from
Feb 19, 2018
Merged

Conversation

bkueng
Copy link
Member

@bkueng bkueng commented Feb 19, 2018

Simpler version of #7698.

This adds a butterworth low-pass filter to the D-term of the rate controller.
The idea behind this is: The D-term uses the derivative of the rate and thus is the most susceptible to noise. Therefore, using a D-term filter allows to decrease the driver-level filtering, which leads to reduced control latency and permits to increase the P gains.

Output Noise

I was using IMU_GYRO_CUTOFF=90 for my testing. I think I could go even higher, but I should also reduce the on-chip filtering (which I did not do for these tests).

With IMU_GYRO_CUTOFF=90, I have visible noise on the outputs, but the quad still flies. I tested different values for the D-term cutoff filter frequency, and it reduces the noise on the output indeed. I found for my quad a cutoff frequency of 70 removes most of the noise.
This is the FFT for actuator outputs without D-term filter (first 3 motors):
fft_actuator_outputs_mc_dterm_cutoff 0

This is using D-term filter frequency = 30:
fft_actuator_outputs_mc_dterm_cutoff 30

Tuning Gains

I was flying with a P gain of 0.18 before, and was able to increase it to 0.28 now. I tested the same gains against the state of master (IMU_GYRO_CUTOFF=30, MC_DTERM_CUTOFF=0), and it has strong, visible oscillations, meaning it doesn't fly:

The filter is disabled by default, thus it should not be problematic to merge. We should evaluate the whole filtering pipeline and default values once we have #8731.
In future, we can also experiment with different lp filters.

@AndreasAntener @bresch @MaEtUgR @RomanBapst @Stifael

- use the same value for both
- lower control latency allows increasing these gains
…he D-term

The filter is disabled by default, thus the behavior is unchanged.
_lp_filters_d[0].set_cutoff_frequency(_loop_update_rate_hz, _params.d_term_cutoff_freq);
_lp_filters_d[1].set_cutoff_frequency(_loop_update_rate_hz, _params.d_term_cutoff_freq);
_lp_filters_d[2].set_cutoff_frequency(_loop_update_rate_hz, _params.d_term_cutoff_freq);
_lp_filters_d[0].reset(_rates_prev(0));
Copy link
Member

Choose a reason for hiding this comment

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

Reset on any param change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, I changed it to update only when the frequency changes.

_rates_int +
rates_d_scaled.emult(_rates_prev - rates) / dt +
_rates_int -
rates_d_scaled.emult(rates_filtered - _rates_prev_filtered) / dt +
_params.rate_ff.emult(_rates_sp);

_rates_sp_prev = _rates_sp;
Copy link
Member

Choose a reason for hiding this comment

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

I don't think _rates_sp_prev is used anywhere?

Copy link
Member Author

Choose a reason for hiding this comment

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

yep, removed.

@dagar
Copy link
Member

dagar commented Feb 19, 2018

Asking out of curiosity, did you consider BlockDerivative?
https://github.com/PX4/Firmware/blob/master/src/lib/controllib/BlockDerivative.hpp

math::Vector<3> _rates_prev; /**< angular rates on previous step */
math::Vector<3> _rates_sp_prev; /**< previous rates setpoint */
math::LowPassFilter2p _lp_filters_d[3]; /**< low-pass filters for D-term (roll, pitch & yaw) */
static constexpr const float initial_update_rate_hz = 250.f; /**< loop update rate used for initialization */
Copy link
Member

Choose a reason for hiding this comment

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

I think we're soon going to need a mechanism to set this system wide (single param or programmatically).

Copy link
Member Author

Choose a reason for hiding this comment

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

This is only for initialization. A module needs to be able to determine this on its own and at runtime.

@bkueng
Copy link
Member Author

bkueng commented Feb 19, 2018

Asking out of curiosity, did you consider BlockDerivative?

I've seen it before, but forgot about it. It's using a different filter, and comes with the typical Block* overhead. I think it can be useful to have a separate derivative class, but since the derivative is only used in one place, I don't see it being useful in that case.

LorenzMeier
LorenzMeier previously approved these changes Feb 19, 2018
@LorenzMeier LorenzMeier merged commit 7b5b1c4 into master Feb 19, 2018
@LorenzMeier LorenzMeier deleted the d-term_filter branch February 19, 2018 16:02
@mhkabir
Copy link
Member

mhkabir commented Feb 19, 2018

I'm seeing an incredible performance increase on my racer after applying this PR and adjusting cutoff frequencies! Thanks @bkueng!

@bkueng
Copy link
Member Author

bkueng commented Feb 19, 2018

Awesome! Do you have a video & log? 😬

@hamishwillee
Copy link
Contributor

@bkueng Is this something we could usefully add docs for in devguide? Perhaps part of a rate controller diagram?

@bkueng
Copy link
Member Author

bkueng commented Feb 20, 2018

@hamishwillee yes it should be part of the diagram. But do we already have one for the attitude & rate controller? I only see the one for position.

@hamishwillee
Copy link
Contributor

@bkueng No, not yet (FYI we do have an FW attitude controller going in soon). Perhaps worth talking to @bresch about whether you can work together on that?

@MaEtUgR
Copy link
Member

MaEtUgR commented Feb 21, 2018

@bkueng You're the man making it happen! This filtering process is super important, thanks a lot!

This pull request was closed.
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.

6 participants