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

Filter: LowPassFilter2p: constrain cuttoff to 40% of sample rate #25175

Merged
merged 1 commit into from Oct 9, 2023

Conversation

IamPete1
Copy link
Member

@IamPete1 IamPete1 commented Oct 3, 2023

Currently its possible to get stuck in a boot loop if the frequency is set too high. For example set INS_GYRO_FILTER to 1500 you will find the board will no longer boot all the way. The main goal of this PR is to fix that boot loop but I have also done a couple of other tidy ups.

The stops numerical errors by constraining the cutoff down, we could also just turn off the filter as we would for a cutoff of zero.

@IamPete1 IamPete1 requested a review from lthall October 3, 2023 18:24
@@ -17,13 +17,12 @@ DigitalBiquadFilter<T>::DigitalBiquadFilter() {

template <class T>
T DigitalBiquadFilter<T>::apply(const T &sample, const struct biquad_params &params) {
if(is_zero(params.cutoff_freq) || is_zero(params.sample_freq)) {
if(!is_positive(params.cutoff_freq) || !is_positive(params.sample_freq)) {
Copy link
Member Author

@IamPete1 IamPete1 Oct 3, 2023

Choose a reason for hiding this comment

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

This changes to !is_positive to keep the same check as we do here:

if (!is_positive(ret.cutoff_freq)) {
// zero cutoff means pass-thru
return;
}

For example if a user was to set the cutoff to -1 for disable the compute_params function would not update so you would be left with the configutation for the old cutoff and the filter would still work fine because -1 is not zero.

return sample;
}

if (!initialised) {
reset(sample, params);
initialised = true;
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't need to set initialised because the reset call does it for us.

void DigitalBiquadFilter<T>::reset(const T &value, const struct biquad_params &params) {
_delay_element_1 = _delay_element_2 = value * (1.0 / (1 + params.a1 + params.a2));
initialised = true;
}

@@ -37,7 +36,6 @@ T DigitalBiquadFilter<T>::apply(const T &sample, const struct biquad_params &par

template <class T>
void DigitalBiquadFilter<T>::reset() {
_delay_element_1 = _delay_element_2 = T();
Copy link
Member Author

Choose a reason for hiding this comment

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

No need to zero these setting initialised false will force a reset in the next apply call

@@ -49,14 +47,15 @@ void DigitalBiquadFilter<T>::reset(const T &value, const struct biquad_params &p

template <class T>
void DigitalBiquadFilter<T>::compute_params(float sample_freq, float cutoff_freq, biquad_params &ret) {
ret.cutoff_freq = cutoff_freq;
// Keep well under Nyquist limit
ret.cutoff_freq = MIN(cutoff_freq, sample_freq * 0.4);
Copy link
Member Author

Choose a reason for hiding this comment

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

Using exactly 0,5 still resulted in numerical issues. 0.45 would probably work.

Comment on lines -106 to -109
if (!is_positive(_params.cutoff_freq)) {
// zero cutoff means pass-thru
return sample;
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Don't need this check as we now check in the main apply function.

Copy link
Contributor

@lthall lthall left a comment

Choose a reason for hiding this comment

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

Looks good mate.

@rmackay9 rmackay9 added the BUG label Oct 9, 2023
@rmackay9 rmackay9 merged commit d8e18a0 into ArduPilot:master Oct 9, 2023
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants