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

Add rclcpp::RosTime #2084

Closed
wants to merge 3 commits into from
Closed

Add rclcpp::RosTime #2084

wants to merge 3 commits into from

Conversation

sloretz
Copy link
Contributor

@sloretz sloretz commented Jan 18, 2023

This adds a class rclcpp::RosTime, which inherits from rclcpp::Time. The value it adds is that the clock type can only be RCL_ROS_TIME. This prevents exceptions due to comparisons with default constructed objects. It can also be used to make it clear an API only accepts RCL_ROS_TIME.

For motivation see:

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@sloretz
Copy link
Contributor Author

sloretz commented Jan 18, 2023

CI (build: --packages-up-to rclcpp test: --packages-select rclcpp)

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Windows Build Status

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

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

overall lgtm, but this leads me to think we also would want to SystemTime and SteadyTime accordingly?

rclcpp/include/rclcpp/time.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/time.hpp Outdated Show resolved Hide resolved
rclcpp/include/rclcpp/time.hpp Outdated Show resolved Hide resolved
@clalancette
Copy link
Contributor

I'll also point out the somewhat related #1373 , which aims to do a similar thing for the Rate class.

Signed-off-by: Shane Loretz <sloretz@osrfoundation.org>
@alsora
Copy link
Collaborator

alsora commented Jan 20, 2023

Should we use a templated class?
I assume we may want the same for all clock types.

@mjcarroll
Copy link
Member

@sloretz was there anything else needed here?

@sloretz
Copy link
Contributor Author

sloretz commented Mar 16, 2023

I don't have time to follow up on this one, so I'll close it for now.

@sloretz sloretz closed this Mar 16, 2023
@sloretz sloretz deleted the sloretz__RosTime branch March 16, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants