gazebosim / ros_gz

Integration between ROS (1 and 2) and Gazebo simulation
https://gazebosim.org
Apache License 2.0
233 stars 135 forks source link

feat: `override_timestamps_with_wall_time` parameter #562

Closed reinzor closed 2 months ago

reinzor commented 3 months ago

🎉 New feature

Closes #546

Summary

Stamp all outgoing headers with the wall time if parameter override_timestamps_with_wall_time is set to true.

Test it

Start the bridge with the additional parameter override_timestamps_with_wall_time set to true. Note that the outgoing messages have the wall time stamp and not the gazebo time stamp.

Checklist

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

azeey commented 3 months ago

@Mergifyio update

mergify[bot] commented 3 months ago

update

☑️ Nothing to do

- [ ] `#commits-behind>0` [📌 update requirement] - [X] `-closed` [📌 update requirement] - [X] `-conflict` [📌 update requirement] - [X] `queue-position=-1` [📌 update requirement]
azeey commented 3 months ago

CI issues should hopefully be fixed after https://github.com/ros2/ros2/pull/1574

Timple commented 3 months ago

Let's re-run them then :slightly_smiling_face:

azeey commented 3 months ago

I just reran it, but I realized the branch is out-of-date. Do you mind merging from ros2?

reinzor commented 3 months ago

I just reran it, but I realized the branch is out-of-date. Do you mind merging from ros2?

done

azeey commented 2 months ago

It seems like there are some CI failures. Do the tests pass locally for you?

reinzor commented 2 months ago

It seems like there are some CI failures. Do the tests pass locally for you?

I am running the tests locally and they seem to fail as well. However, I don't see how this code change would affect the failing tests. Could you help me with this?

azeey commented 2 months ago

The error logs indicated that it was failing to create any of the GZ->ROS bridges. Inspecting the exception with a print statement shows that the problem is that we are redeclaring the override_timestamps_with_wall_time parameter. My suggestion is to declare the parameter in https://github.com/gazebosim/ros_gz/blob/4c64bbea8041131b36a84692af7064a4abbfddb4/ros_gz_bridge/src/ros_gz_bridge.cpp#L35 and use get_parameter to actually retrieve the value. I would also suggest adding a member variable to BridgeHandle that stores the value of the parameter and fetch the parameter in the constructor of BridgeHandle instead of modifying the function signature.

reinzor commented 2 months ago

Thanks for your help @azeey ; I updated the PR with the suggestions you mentioned. Not sure about not changing the function signature though. We need to pass this variable to the factory right?

ahcorde commented 2 months ago

Thanks for your help @azeey ; I updated the PR with the suggestions you mentioned. Not sure about not changing the function signature though. We need to pass this variable to the factory right?

You are targeting ros2, we can change the API's here, but if we need to backport this, we should do it in a different way

reinzor commented 2 months ago

Thanks for your help @azeey ; I updated the PR with the suggestions you mentioned. Not sure about not changing the function signature though. We need to pass this variable to the factory right?

You are targeting ros2, we can change the API's here, but if we need to backport this, we should do it in a different way

I will leave that up to you as maintainers. Feel free to move the logic to a different place.

reinzor commented 2 months ago

Great thanks! How does this backporting to jazzy work; should I just send another PR that cherry-picks this commit?

ahcorde commented 2 months ago

https://github.com/Mergifyio backport jazzy

mergify[bot] commented 2 months ago

backport jazzy

✅ Backports have been created

* [#584 feat: `override_timestamps_with_wall_time` parameter (backport #562)](https://github.com/gazebosim/ros_gz/pull/584) has been created for branch `jazzy` but encountered conflicts