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

Support for `use_sim_time` = `false` #546

Closed reinzor closed 2 months ago

reinzor commented 4 months ago

What are your thought about running the bridge with use_sim_time false? Would you be open for a PR like this: https://github.com/gazebosim/ros_gz/pull/545

Please give me your thoughts.

Thanks!

azeey commented 4 months ago

Could you elaborate on the purpose for this? ros_gz is meant to be used with Gazebo, so I would think use_sim_time would always be true, but maybe there are other use cases?

reinzor commented 4 months ago

Sure, we have two reasons for this:

reinzor commented 4 months ago

Any thoughts @azeey ?

azeey commented 4 months ago

Those seem like good reasons to me. My only concern is that we don't break any existing users since use_sim_time is assumed to be true for ros_gz by default. Is there a way we can ensure that the parameter is true if not set explicitly by the user?

reinzor commented 4 months ago

Thanks for your reply @azeey , I don't think we can and I don't think we should modify the default node behavior:

https://github.com/ros2/rclcpp/blob/343b29b617b163ad72b9fe3f6441dd4ed3d3af09/rclcpp/src/rclcpp/time_source.cpp#L257

Here the parameter is set to false by default.

This line:

https://github.com/nobleo/ros_gz/blob/714660e86e9b4e7ff60c1ce46bbc43a54d8a6bd3/ros_gz_bridge/src/bridge_handle_gz_to_ros.cpp#L74

will simply ask the node whether the simulation time is active.

azeey commented 4 months ago

Well, that would be a blocker then since it will likely break existing users. What about using additional parameter or environment variable to enable checking use_sim_time?

reinzor commented 4 months ago

What about adding indeed an additional parameter, e.g. use_wall_time that defaults to false and log a warning when it is conflicting with node->ros_time_is_active()? In order to supress this warning, the user should align these two parameters, either:

And in the implementation, we use the value of the use_wall_time_parameter. What do you think?

Timple commented 4 months ago

Or make this new behavior default as per ROS2 Jazzy. As breaking changes are typically allowed between distros.

reinzor commented 3 months ago

ping @azeey

azeey commented 3 months ago

The job of the bridge is to convert messages from Gazebo to ROS. It shouldn't really have its own concept of time. The timestamps are provided by Gazebo. Overwriting this timestamp with some other value should be an opt-in, so I would not be in favor of changing the current behavior. I would support adding a use_wall_time parameter, better yet, a override_timestamps_with_wall_time parameter. But I don't think it should give a warning in the default case, but your suggestion seems like it would, i.e, the default is override_timestamps_with_wall_time=false, use_sim_time=false. Do we really need to align the parameters? Can we not simply check override_timestamps_with_wall_time?

reinzor commented 3 months ago

For me just adding the override_timestamps_with_wall_time parameter is fine. Thanks for your elaborate reply. I will update my PR accordingly and publish it.