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: LowPass: use calc_lowpass_alpha_dt in apply and prefix all class varables with _ #27582

Closed
wants to merge 1 commit into from

Conversation

IamPete1
Copy link
Member

A small simplification to our base low pass filter. It now uses calc_lowpass_alpha_dt and apply so the existing code is reused rather than being re-implemented.

There are some changes in behavior in the invalid dt/cutoff_freq cases. Previously the local copy of alpha was not updated where as it now is. This should not matter because the two apply methods should not be used together.

Future work
I would like to split the filter in to two classes one for each of the methods outlined here:

1) providing dt on every sample, and calling apply like this:
// call once
filter.set_cutoff_frequency(frequency_hz);
// then on each sample
output = filter.apply(sample, dt);
2) providing a sample freq and cutoff_freq once at start
// call once
filter.set_cutoff_frequency(sample_freq, frequency_hz);
// then on each sample
output = filter.apply(sample);

The two methods should not be mixed, so separate classes will ensure that.

Then I would like to add a filter class which includes the cutoff frequency parameter since that is a very common usecase.

@IamPete1 IamPete1 requested review from lthall and bnsgeyer July 17, 2024 21:06
} else {
alpha = calc_lowpass_alpha_dt(1.0/sample_freq, cutoff_freq);
_alpha = calc_lowpass_alpha_dt(1.0/sample_freq, cutoff_freq);
Copy link
Member Author

Choose a reason for hiding this comment

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

note that we are already using the calc_lowpass_alpha_dt function in this case.

return _output;
}
float rc = 1.0f/(M_2PI*cutoff_freq);
alpha = constrain_float(dt/(dt+rc), 0.0f, 1.0f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should this constrain be added to calc_lowpass_alpha_dt?

Copy link
Member Author

@IamPete1 IamPete1 Jul 18, 2024

Choose a reason for hiding this comment

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

See: #20911

The constrain will never trigger because we have already checked both values are > 0.

Comment on lines +72 to +73
float _alpha = 1.0f;
bool _initialised;
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were trying to canonicalise the other way, to remove leading underscores over time

@tridge
Copy link
Contributor

tridge commented Jul 23, 2024

it would be good to be sure the two API approaches are not currently mixed

@IamPete1
Copy link
Member Author

replaced by #27932

@IamPete1 IamPete1 closed this Aug 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants