-
Notifications
You must be signed in to change notification settings - Fork 430
Add utility node for transform wrench messages for a list of frames #2021
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 utility node for transform wrench messages for a list of frames #2021
Conversation
594c6ce to
254c53e
Compare
254c53e to
ab7b06a
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2021 +/- ##
==========================================
+ Coverage 85.13% 85.20% +0.07%
==========================================
Files 144 148 +4
Lines 13968 14321 +353
Branches 1201 1225 +24
==========================================
+ Hits 11891 12202 +311
- Misses 1670 1694 +24
- Partials 407 425 +18
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
Thanks a lot for picking this.
Overall LGTM. I added some suggestions to fix deprecation warnings or sphinx issues.
The tests do not cover if the message is actually transformed, right? Maybe you could add one for that.
force_torque_sensor_broadcaster/include/force_torque_sensor_broadcaster/wrench_transformer.hpp
Outdated
Show resolved
Hide resolved
force_torque_sensor_broadcaster/test/test_wrench_transformer.cpp
Outdated
Show resolved
Hide resolved
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.
Thanks for adding more tests, but I think the one I asked for is still missing?
And please use the github UI directly for committing proposed changes (from the files tab you can commit batches at once), this saves some minutes for the next review round :)
force_torque_sensor_broadcaster/test/test_wrench_transformer.cpp
Outdated
Show resolved
Hide resolved
Thanks, will do. Was looking for a way for batch commit. |
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
…liaj/ros2_controllers into fts_broadercaster_utility_node_1842_2
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.
Thanks, that test covers what I was asking for. A small detail in the comment
Thanks, @christophfroehlich. Please let me know if any additional changes are needed. I've reviewed the comments but wasn't able to determine what specific changes are required. |
force_torque_sensor_broadcaster/test/test_wrench_transformer.cpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Christoph Fröhlich <christophfroehlich@users.noreply.github.com>
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.
LGTM, thanks again!
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.
Thanks for taking up this P
Pretty good testing 👌🏾
I've some nitpicks on the parameters etc, but these can be addressed in a different PR
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.
Oh, I forgot one thing: Please update the doc/release_notes with this new feature.
|
@Juliaj The nitpicks I mentioned earlier are the following:
What's your opinion on this? |
|
@saikishor, thanks a lot for your feedback on simplifying the parameter handling, ❤️ them .
Agreed,
Implemented as your suggested with a small add-on. If the user remaps the topic to wrench_filtered, the output topic will be a corresponding /ns/<frame_name>/wrench_filtered. For following,
I created a separate issue and will look into it in subsequent PR. |
Added. The release_notes.rst was empty , so I wasn't sure if it was the right file to edit. |
No this is fine, I just recently branched off for kilted. That's why the kilted-lyrical release notes are empty. |
Address #1842
This PR adds a new utility node
wrench_transformer_nodetoforce_torque_sensor_broadcasterto transform input Wrench messages to output Wrench messages for a given list of frames.Feedback on naming is welcome and appreciated.
Testing