Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat: override_timestamps_with_wall_time parameter #562

Merged
merged 11 commits into from
Jul 31, 2024
Merged

feat: override_timestamps_with_wall_time parameter #562

merged 11 commits into from
Jul 31, 2024

Conversation

reinzor
Copy link
Contributor

@reinzor reinzor commented Jun 19, 2024 •
edited
Loading

🎉 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

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

@reinzor reinzor requested a review from ahcorde as a code owner June 19, 2024 09:39
@reinzor reinzor force-pushed the feat/override_timestamps_with_wall_time branch from a656c0f to d348aa2 Compare June 19, 2024 09:39
ahcorde
ahcorde requested changes Jun 19, 2024
View reviewed changes
Copy link
Collaborator

@ahcorde ahcorde left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code is not compiling https://github.com/gazebosim/ros_gz/actions/runs/9579762345/job/26418501612?pr=562

@reinzor
feat: override_timestamps_with_wall_time parameter
489affd 
Stamp all outgoing headers with the wall time if parameter
`override_timestamps_with_wall_time` is set to true.

Signed-off-by: Rein Appeldoorn <[email protected]>
@reinzor reinzor force-pushed the feat/override_timestamps_with_wall_time branch from d348aa2 to 489affd Compare June 20, 2024 05:33
@reinzor
uncrustify
5b3fc44 
Signed-off-by: Rein Appeldoorn <[email protected]>
@reinzor reinzor force-pushed the feat/override_timestamps_with_wall_time branch from 0720be1 to 5b3fc44 Compare June 25, 2024 05:37
@azeey
Copy link
Contributor

azeey commented Jun 25, 2024

@Mergifyio update

@mergify Mergify
Copy link
Contributor

mergify bot commented Jun 25, 2024

update

☑️ Nothing to do

  • #commits-behind>0 [📌 update requirement]
  • -closed [📌 update requirement]
  • -conflict [📌 update requirement]
  • queue-position=-1 [📌 update requirement]

@azeey
Copy link
Contributor

azeey commented Jun 25, 2024

CI issues should hopefully be fixed after ros2/ros2#1574

@Timple
Copy link

Timple commented Jul 2, 2024

Let's re-run them then 🙂

@azeey
Copy link
Contributor

azeey commented Jul 2, 2024

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

@reinzor
Copy link
Contributor Author

reinzor commented Jul 3, 2024

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

done

@azeey
Copy link
Contributor

azeey commented Jul 8, 2024

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

@azeey azeey added the beta label Jul 29, 2024
@reinzor
Copy link
Contributor Author

reinzor commented Jul 30, 2024

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
Copy link
Contributor

azeey commented Jul 30, 2024

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

ros_gz/ros_gz_bridge/src/ros_gz_bridge.cpp

Line 35 in 4c64bbe

this->declare_parameter<bool>("expand_gz_topic_names", false);
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
declare parameter only once
9837d6a 
Signed-off-by: Rein Appeldoorn <[email protected]>
@reinzor
Merge branch 'feat/override_timestamps_with_wall_time' of github.com:…
0ae220c 
…nobleo/ros_gz into feat/override_timestamps_with_wall_time
@reinzor
Copy link
Contributor Author

reinzor commented Jul 31, 2024

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?

@reinzor
Merge branch 'feat/override_timestamps_with_wall_time' of github.com:…
a6ef7d0 
…nobleo/ros_gz into feat/override_timestamps_with_wall_time
@reinzor
Merge branch 'feat/override_timestamps_with_wall_time' of github.com:…
fec2c4d 
…nobleo/ros_gz into feat/override_timestamps_with_wall_time
ahcorde
ahcorde requested changes Jul 31, 2024
View reviewed changes
ros_gz_bridge/src/factory.hpp
@@ -28,6 +28,16 @@

#include "factory_interface.hpp"

template<class T, class = void>
struct has_header : std::false_type
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include <type_traits>

ros_gz_bridge/src/factory.hpp
{
ROS_T ros_msg;
convert_gz_to_ros(gz_msg, ros_msg);
if constexpr (has_header<ROS_T>::value) {
if (override_timestamps_with_wall_time) {
auto now = std::chrono::system_clock::now().time_since_epoch();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

include <chrono>

@ahcorde
Copy link
Collaborator

ahcorde commented Jul 31, 2024

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
Added missing includes
99b0da0 
Signed-off-by: Rein Appeldoorn <[email protected]>
@reinzor
Copy link
Contributor Author

reinzor commented Jul 31, 2024

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.

@ahcorde ahcorde requested a review from azeey July 31, 2024 13:01
azeey
azeey approved these changes Jul 31, 2024
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for iterating. I realized the API change issue is not a problem since none of the changed headers are considered public API. I think this could actually be backported if needed.

@azeey azeey merged commit 6daea2c into gazebosim:ros2 Jul 31, 2024
3 checks passed
@reinzor
Copy link
Contributor Author

reinzor commented Aug 1, 2024

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

@ahcorde
Copy link
Collaborator

ahcorde commented Aug 1, 2024

https://github.com/Mergifyio backport jazzy

@mergify Mergify
Copy link
Contributor

mergify bot commented Aug 1, 2024 •
edited
Loading

backport jazzy

✅ Backports have been created

mergify bot pushed a commit that referenced this pull request Aug 1, 2024
@reinzor @mergify
Stamp all outgoing headers with the wall time if parameter override_t…
e93fc7d 
…imestamps_with_wall_time is set to true (#562)

Signed-off-by: Rein Appeldoorn <[email protected]>
(cherry picked from commit 6daea2c)

# Conflicts:
#	ros_gz_bridge/src/factory.hpp
@mergify mergify bot mentioned this pull request Aug 1, 2024
Merged
9 tasks
ahcorde pushed a commit that referenced this pull request Aug 29, 2024
@mergify @reinzor
feat: override_timestamps_with_wall_time parameter (backport #562) (#…
7aab274 
…584)

Signed-off-by: Rein Appeldoorn <[email protected]>
Co-authored-by: Rein Appeldoorn <[email protected]>
Amronos pushed a commit to Amronos/ros_gz that referenced this pull request Sep 18, 2024
@reinzor @Amronos
Stamp all outgoing headers with the wall time if parameter override_t…
f21c707 
…imestamps_with_wall_time is set to true (gazebosim#562)

Signed-off-by: Rein Appeldoorn <[email protected]>
@azeey azeey mentioned this pull request Nov 7, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahcorde ahcorde ahcorde approved these changes

@azeey azeey azeey approved these changes

Assignees
No one assigned
Labels
beta
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support for use_sim_time = false
4 participants
@reinzor @azeey @Timple @ahcorde