-
Notifications
You must be signed in to change notification settings - Fork 17.5k
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: LowPassFilter: split into two classes for constant and varable dt #27670
Conversation
float LowPassFilter<T>::get_cutoff_freq(void) const { | ||
return _cutoff_freq; | ||
T LowPassFilter_const_dt<T>::apply(const T &sample) { | ||
return this->_apply(sample, alpha); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
using this->
is a little odd, its due to class template inheritance. see: https://kernhanda.github.io/templates-inheritance-methods/
test.Sub.RngfndQuality passes locally for me on this PR. Might be flaky. I'll take a look. |
I have restarted it for now, Thanks for taking a look! |
libraries/Filter/LowPassFilter.h
Outdated
|
||
// 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 | ||
LowPassFilter_const_dt: providing a sample freq and cutoff_freq once at start |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm really not enamoured with the name!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm open to better suggestions, I just wanted to make it clear which should be used in each case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FixedLowPassFilter? StaticLowPassFilter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its not fixed in cutoff frequency, its fixed in sample rate. So i'm not sure fixed really works. Or at least it does not line up with what we would call a fixed notch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LowPassFilterConstDt would be my vote
structure good, just needs testing and/or unit tests |
…e cutoff frequency update
looks good! marked for flight testing on the weekend |
tested on a 5" quad and a plane on the weekend with no noticeable issues |
This splits our existing
LowPassFilter
class into a class which uses variable time step and passes the dt in the apply call and a class which uses constant dt. The variable dt class retains theLowPassFilter
name, the constant dt class isLowPassFilter_const_dt
.The key thing is to only provide methods that will work together.
This found two bugs, copters flow hold
set_cutoff_frequency
call would not work while in the mode, only on init, so a cuttoff frequency param change would not take effect in teal time.Secondly the SITL battery voltage filter is currently none functional because it was using the constant dt methods without setting the sample rate.
Where needed the class has been changed to the constant dt version.