-
Notifications
You must be signed in to change notification settings - Fork 938
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
add warnings to jog_arm low_pass filter #1872
add warnings to jog_arm low_pass filter #1872
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1872 +/- ##
==========================================
+ Coverage 50.15% 50.27% +0.11%
==========================================
Files 313 313
Lines 24641 24628 -13
==========================================
+ Hits 12359 12381 +22
+ Misses 12282 12247 -35
Continue to review full report at Codecov.
|
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.
Approved but see comments.
7b2e469
to
9608820
Compare
d23c333
to
c8b8edd
Compare
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 think the error message about an unstable coefficient is awesome, greatly needed.
The last change was to make it C++11 compatible (the way I used static_assert was from C++17). |
{ | ||
filter_coeff_ = low_pass_filter_coeff; | ||
// guarantee this doesn't change because the logic depends on this length implicity | ||
static_assert(LowPassFilter::FILTER_LENGTH == 2, "moveit_jog_arm::LowPassFilter::FILTER_LENGTH should be 2"); |
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.
Isn't that a little paranoid? Also, what if someone initializes previous_measurements_
with another constant... ;)
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 don't think it is too paranoid. The indexes of the array are explicitly used instead of loops below this and this check is just a compiler check. It won't add any code to the binary. This is just helping protecting against someone modifying this value.
I don't know why changing the values of previous_measurements_
with the reset
method would affect 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.
Sure, initialization using reset
is not an issue. This check here only warns other developers from changing the value of FILTER_LENGTH
in the header file. But you could just declare previous_measurement_
with another constant and this assertion will never be triggered, so it's not really a guarantee that logic doesn't break. I'm not concerned about performance, I just think that a simple comment in the header file is more effective than letting developers run into warnings at compile time.
Description
These warnings and errors handle warning the user of bad input parameter values for the low pass filter in jog arm. Here is the function of the feedback magnitude with respect to the
low_pass_filter_coeff
value: https://www.wolframalpha.com/input/?i=y+%3D+%281%2F%281%2Bx%29%29+*+%281-x%29Here is a small python script that shows why these values are problematic:
output:
Checklist