-
Notifications
You must be signed in to change notification settings - Fork 125
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
TF2 msgs not implemented in ros2 branch (#116) #117
Conversation
* add ign pose_v to ros tf2_message bridge * add tf2 msgs dependency Signed-off-by: 09ubberboy90 <2330834a@student.gla.ac.uk>
I'll fix the test tomorrow. Not too sure why the tf2 test doesn't pass as ignition correctly register and echo the topic fine in terminal. |
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'll fix the test tomorrow.
Thanks! There are some uncrustify (style) errors and also some test failures. I also think you should either remove the support for double
or complete the tests.
It looks like you're missing some lines on the launch files:
https://github.com/ignitionrobotics/ros_ign/tree/ros2/ros_ign_bridge/test/launch
ros_ign_bridge/package.xml
Outdated
@@ -19,6 +19,8 @@ | |||
<depend>rosgraph_msgs</depend> | |||
<depend>sensor_msgs</depend> | |||
<depend>std_msgs</depend> | |||
<depend>std_srvs</depend> |
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 std_srvs
needed?
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.
It was added in 7b06c21 but it wasn't needed for just that so I removed it. Good call
Signed-off-by: 09ubberboy90 <2330834a@student.gla.ac.uk>
Signed-off-by: 09ubberboy90 <2330834a@student.gla.ac.uk>
I updated the Readme too because the list was not reflecting the correct node name. If it should be in a separate issue/PR tell me and I'll revert it |
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.
CI is happy now, nice work! Just a couple more comments.
Signed-off-by: 09ubberboy90 <2330834a@student.gla.ac.uk>
add ign pose_v to ros tf2_message bridge
add tf2 msgs dependency