We were investigating if we should use ROS 2 Time or std::chrono for our product. Note that our use case is a highly safety critical real-time system influencing the priority of the arguments.
Upon closer inspection, I found that ROS 2 Time is largely redundant to std::chrono.
All functionality of ROS 2 simulated time can also be achieved by providing a custom std::chrono clock encapsulating the rclcpp::Clock which would look like this (modulo some shared ptr shenanigans I omitted):
Is it important for your product to support being used by two nodes in the same process at the same time?
If rclcpp used std::chrono then there could only be one clock per clock type per process. If a process with two nodes starts and a user sets use_sim_time to true on one node and false on the other then what time should ros2_clock::now() return?
If I understand correctly, I think the issue is that the std::chrono clock requirements don’t allow now() to be a non-static member function. ros2_node_clock::now() has to be static and could not access m_clock.
A while ago I did try prototyping a clock that conformed to the std::chrono requirements except with a non-static member now(). Not all C++ API’s allow this. For example std::thread::sleep_until() tries to call clock::now() on the time point passed into it. I gave up because whether or not an API calls clock::now() in its implementation is undefined behavior.
@sloretz: Oh, that is a really good point! Thank you!
They should have designed std::chrono as they fixed std::allocators.
This has the interesting implication that ROS 2 time can never be converted to std::chrono time and used with the standard library as it is not determinable from which clock the time came.
I will check if it is possible to create a std::chrono clock which at least works on a global level such that an error is produces when someone tries to switch a node to a different time than some global setting.
Why? From my perspective the time point returned from ROS Time must be compatible with the system clock since that’s what is returned when no simulated time is used.
I don’t see any reason why we could not have a conversion function between a ROS Time instance and a chrono system_clock::time_point instance.
I don’t think that’s what @sloretz was after, it might be totally correct to have one node using sim time and the other not, I see no reason to couple it to a global state.
Also, you have to consider the case where two nodes exist and only one has the subscription to the /clock topic and then that node is destroyed. Now the second node still exists and is expecting sim time but has no clock subscription.
These cases are why we have a non-global and non-static clock class.
I checked the code more closely and it is actually encoded in rclcpp::Time which clock was used. This is great! But if it set to RCL_ROS_TIME I have no information about the state of the clock which was used to create this rclcpp::Time, right? Maybe it was simulated time, maybe something else. Should rclcpp::Time not have a pointer to the associated rclcpp::Clock?
Maybe I am missing a feature in ROS 2 time which solves this issue but I am unable to find it.
I am also unsure about the solution how would one implement the conversion from rclcpp::Time to std::time_point.
So this is where my expectation for ROS Time and what is actually implemented right now has diverged.
My expectation is that ROS Time would not be able to store or provide Steady time, and instead that would be a separate type or just be std::chrono::steady_clock instead.
I did bring up this issue at the time, but I think the refactor to address my concerns was tabled for later, but unfortunately I don’t see an issue on ros2/rclcpp tracking the problem either. @tfoote do you know if this is tracked somewhere? Perhaps on a meta issue or something?
Either way it is stated this way in the design document:
SteadyTime will be typed differently than the interchangable SystemTime and ROSTime . This is because SystemTime and ROSTime have a common base class with runtime checks that they are valid to compare against each other. However SteadyTime is never comparable to SystemTime or ROSTime so it would actually have a separate implementation of the same API. Thus you could get protection from misusing them at compile time (in compiled languages) instead of only catching it at runtime.
If you assume that this change is made, then ROS Time (i.e. rclcpp::Time) can only either be from the system time or from a custom time source. However, the custom time source must provide a time that is “compatible” with system time. For me that means it uses the same epoch (i.e. unix epoch) and it follows the same behaviors like handling leap seconds. Put another way a time stamp from system time and the same timestamp from a custom time source should match when converted to a date time (yyyy-mm-dd-…).
So with that assumption, the conversion function would look like this:
I don’t think that this ended up being tracked because there was a desire to not grow the backlog with enhancement tickets for known things to do. We specifically did leave it in the design document as a specific choice. And since it’s something open that might be able to be picked up by someone else it would probably be worth us opening an issue to track it.