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.

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

Support for use_sim_time = false #546

Closed
reinzor opened this issue May 7, 2024 · 11 comments · Fixed by #562
Closed

Support for use_sim_time = false #546

reinzor opened this issue May 7, 2024 · 11 comments · Fixed by #562
Assignees
Labels
enhancement New feature or request

Comments

@reinzor
Copy link
Contributor

reinzor commented May 7, 2024

What are your thought about running the bridge with use_sim_time false? Would you be open for a PR like this: #545

Please give me your thoughts.

Thanks!

@reinzor reinzor added the enhancement New feature or request label May 7, 2024
@azeey
Copy link
Contributor

azeey commented May 7, 2024

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

reinzor commented May 7, 2024

Sure, we have two reasons for this:

  • When running a gazebo instance in a hardware in the loop set-up, some other nodes in the ros2 graph require real timestamps in order to function
  • Subscribing to the /clock topic for each running node in our ros2 set-up, introduces a lot of CPU load, especially for the python processes. This is more related to the performance of ros2 but in some cases and on some platforms, we can only run proper simulations if we set use_sim_time to false.

@reinzor
Copy link
Contributor Author

reinzor commented May 13, 2024

Any thoughts @azeey ?

@azeey
Copy link
Contributor

azeey commented May 22, 2024

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

reinzor commented May 23, 2024

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

azeey commented May 23, 2024

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

reinzor commented May 24, 2024

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:

  • use_wall_time=true, use_sim_time=false
  • use_wall_time=false, use_sim_time=true

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

@Timple
Copy link

Timple commented May 24, 2024

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

@reinzor
Copy link
Contributor Author

reinzor commented Jun 5, 2024

ping @azeey

@azeey
Copy link
Contributor

azeey commented Jun 18, 2024

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

reinzor commented Jun 19, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants