-
Notifications
You must be signed in to change notification settings - Fork 423
TF prefix helper for diff drive controller #1997
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
base: master
Are you sure you want to change the base?
Conversation
saikishor
left a comment
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 not sure if it makes sense to have it in the control_toolbox package. What about controllers_interfaces/helpers.hpp existing file?
What do you think @christophfroehlich ?
OK, maybe you are right and we should put it here. |
I will start working on it |
christophfroehlich
left a comment
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.
There is a behavior-breaking change now, right?
Can we do that gracefully? e.g.,
- Deprecate the
tf_frame_prefix_enableparameter in the yaml, and log a warning message if it is set to a value different than default. And we should maintain the behavior and still use the value. - Please update the description of the
tf_frame_prefixparameter
https://github.com/ros-controls/ros2_controllers/blob/master/diff_drive_controller/src/diff_drive_controller_parameter.yaml#L49-L53 - Please add release and migration notes.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1997 +/- ##
==========================================
- Coverage 85.32% 85.30% -0.03%
==========================================
Files 143 143
Lines 13936 13920 -16
Branches 1201 1198 -3
==========================================
- Hits 11891 11874 -17
- Misses 1638 1639 +1
Partials 407 407
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
- control toolbox pkg already linked
- this commit is to be deleted later
- enable flag removed from args - tilde substitution with ns added - tests updated - verbose comments
53851e8 to
0e25332
Compare
- log message added if tf_frame_prefix_enable is not set default - relevant info added to yaml description, migration and release notes
christophfroehlich
left a comment
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.
tf_frame_prefix_enable was not unused before? Instead, during the deprecation phase, we still should use this parameter before we remove it.
I'm not sure how we can gracefully transition the usage of node namespace? Currently, if tf_frame_prefix is not set, then automatically the node namespace is used.
Used new tf frame prefix helper in differential drive controller to test out & showcase the helper. This is the first part of the changes, the other controllers will come next:
Related issue