-
Notifications
You must be signed in to change notification settings - Fork 98
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
Fix bug in rate_limiter filter and add more tests #237
base: ros2-master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## ros2-master #237 +/- ##
===============================================
+ Coverage 71.05% 74.69% +3.63%
===============================================
Files 20 22 +2
Lines 1040 1071 +31
Branches 84 85 +1
===============================================
+ Hits 739 800 +61
+ Misses 255 223 -32
- Partials 46 48 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
default_value: .NAN, | ||
description: "Maximum value, e.g. [m/s]", | ||
validation: { | ||
"control_filters::gt_eq_or_nan<>": [0.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.
Is infinite a better default value here? Or Nan is better?
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.
in principle you are right, and it would already work setting it + or -infinite. However, NaN is the default value of the diff_drive_controller. And the logic of "copying the positive value if negative is not given" would not work because infinite is a valid limit 🤔
I ran in the same issue as was solved with diff_drive_controller already ros-controls/ros2_controllers#252
I added some tests, and furthermore improved the generate_parameter_library parameter declaration file.