-
Notifications
You must be signed in to change notification settings - Fork 412
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
Use rosgraph_msgs/Clock for /clock topic. #474
Conversation
@ernestmc Thanks for the patch! We assumed this is ready for review so we labeled it as is (please comment here if this is not the case) |
@mikaelarguedas this is ready for review. I've tested and it works. Awaiting you comments. |
@ernestmc It looks like this PR makes multiple rclcpp tests fail. |
Forgot to try the tests. I'll look at them and let you know. |
@mikaelarguedas I've updated the test cases and now they are passing.
|
thanks @ernestmc |
cdcd9a7
to
b8fe1e0
Compare
@ernestmc FYI: I rebased this branch on top of master and force pushed it The reason: In the future we recommend, when possible, opening PRs from branches hosted on the ros2 repositories this way CI will make sure to merge your branch into master before building. New round of CI on rebased branch: |
b8fe1e0
to
016f9c7
Compare
Thanks @mikaelarguedas . I've rebased my branch to keep it updated. I didn't know if I could push a new branch directly into the repo. |
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.
code change looks good to me.
I leave it up to @tfoote to give final review/approval
clock_pub->publish(msg); | ||
// std::cout << "Publishing: '" << msg->sec << ".000000" << msg->nanosec << "'" << std::endl; |
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.
@tfoote did you want to keep these for debugging?
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.
If we decide to keep those lines I need to update them to the new message type.
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.
No we don't need to keep them.
clock_pub->publish(msg); | ||
// std::cout << "Publishing: '" << msg->sec << ".000000" << msg->nanosec << "'" << std::endl; |
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.
No we don't need to keep them.
Addresses issue #473
Use
rosgraph_msgs/Clock
message for/clock
topic instead ofbuiltin_interfaces/Time
to be compatible with ROS1.This is needed for the
ros1_bridge
to effectively map/clock
message when using Gazebo.connects to #473