-
Notifications
You must be signed in to change notification settings - Fork 955
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
Change 2nd order Butterworth low pass filter to 1st order #1483
Conversation
Thanks for helping in improving MoveIt! |
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'd like to check this equation in Matlab before it gets merged.
double previous_measurements_[3] = { 0., 0., 0. }; | ||
double previous_filtered_measurements_[2] = { 0., 0. }; | ||
double previous_measurements_[2] = { 0., 0. }; | ||
double previous_filtered_measurement = 0.; |
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.
Should be previous_filtered_measurement_ (the underscore indicates it's a member of the class)
Once that is done, I can also change the comment in the header file to say that it is indeed a first order Butterworth filter. |
Why was this originally a 2nd order filter? Has anyone looked into the history and can explain? |
It was just re-used code from an older project. I haven't forgotten to test this... |
(previous_measurements_[2] + 2. * previous_measurements_[1] + previous_measurements_[0] - | ||
(filter_coeff_ * filter_coeff_ - 1.414 * filter_coeff_ + 1.) * previous_filtered_measurements_[1] - | ||
(-2. * filter_coeff_ * filter_coeff_ + 2.) * previous_filtered_measurements_[0]); | ||
(1. / (1. + filter_coeff_)) * (previous_measurements_[1] + previous_measurements_[0] - |
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.
Should be (Matlab syntax):
new_filtered_msrmt = (1 + filter_coeff_ / (2pi))^-1 * (previous_measurements_[1] + previous_measurements_[0] - (1 - filter_coeff_/(2pi)) * previous_filtered_measurement_);
Would it perhaps be an idea to use the filters package here? |
I see that a TransferFunctionFilter is available. That could work. Making it configurable to different cutoff frequencies might be tricky. |
@SansoneG don't forget to do this:
|
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.
Tests well with the equation in my comment. I'll approve if you make those small changes to the equation and update the description in the header. Thanks!
(filter_coeff_ * filter_coeff_ - 1.414 * filter_coeff_ + 1.) * previous_filtered_measurements_[1] - | ||
(-2. * filter_coeff_ * filter_coeff_ + 2.) * previous_filtered_measurements_[0]); | ||
(1. / (1. + filter_coeff_ / (2 * M_PI))) * (previous_measurements_[1] + previous_measurements_[0] - | ||
(-1. * filter_coeff_ / (2 * M_PI) + 1.) * previous_filtered_measurement_); |
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.
No need to multiply by negative 1, just put the negative sign:
(1. / (1. + filter_coeff_ / (2 * M_PI))) * (previous_measurements_[1] + previous_measurements_[0] -
(-filter_coeff_ / (2 * M_PI) + 1.) * previous_filtered_measurement_);
I guess we might as well combine filter_coeff_ with the 2*M_PI constant, right? Save a couple extra computations (like you had it originally - my bad):
(1. / (1. + filter_coeff_)) * (previous_measurements_[1] + previous_measurements_[0] -
(-filter_coeff_ + 1.) * previous_filtered_measurement_);
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 tested carefully with this equation. Did not see any issues:
double new_filtered_msrmt =
(1. / (1. + filter_coeff_)) * (previous_measurements_[1] + previous_measurements_[0] -
(-filter_coeff_ + 1.) * previous_filtered_measurement_);
Top graph is the input to the jogger from my joystick, bottom is the outgoing command to the robot.
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.
^Note, different timescales on the 2 plots
I was not sure if you meant i should move the constant into the constructor (so the computation is done only once instead of each iteration) or remove it completely (maybe it serves the purpose of giving the filter_coeff_ a certain unit). For now I decided to remove it completely. |
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 tested it locally again, seems fine. Thanks a lot!
(I'm just a "core contributor" though, a maintainer needs to approve it before merging.)
@SansoneG Can you assign a few code reviewers so we can finally get this merged? It's a button in the top right that looks like a gear. Just pick a few MoveIt maintainers at random - I'm not aware of any who have experience with digital filtering. Here's a list to pick from. To summarize, the benefits of this PR are:
|
Unfortunately that button is not present on my end. I think the reason for that is stated in this StackOverflow answer. |
@BryceStevenWilley, @felixvd awaits2ndreview please. I'd appreciate, if this finally gets merged. |
Congrats on getting your first MoveIt pull request merged and improving open source robotics! |
I still get asked about this from time to time. Here's the MATLAB code for the filter:
|
Description
Fixes Issue #1471 about filter overshooting by changing the 2nd order Butterworth low pass filter to 1st order.