Add new RCL_RAW_STEADY_TIME changes

[saikishor]

Description

Needs ros2/rcl#1248 and ros2/rcutils#507

Fixes ros2/rcutils#488

Is this user-facing behavior change?

NO

Did you use Generative AI?

NO

@fujitatomoya FYI

[jmachowinski]

I am confused by this commit. The code for steady and steady raw looks identical. @saikishor am I overlooking sometime ?

[saikishor]

I am confused by this commit. The code for steady and steady raw looks identical. @saikishor am I overlooking sometime ?

Yes, they are pretty much similar. What changes is that the STEADY_CLOCK uses CLOCK_MONOTONIC and the RAW_STEADY_CLOCK uses CLOCK_MONOTONIC_RAW. The main difference is here : ros2/rcutils#507

[jmachowinski]

As far as I can see the rclcpp implementation uses std::chrono::steady_clock for sleeping in both cases (steady and steady raw). This seems odd.

[saikishor]

As far as I can see the rclcpp implementation uses std::chrono::steady_clock for sleeping in both cases (steady and steady raw). This seems odd.

I agree, it is not ideal. The conditional variable impl_->cv_.wait_until still needs the std::chrono to sleep. I'm still looking into how to implement it differently. The ideal would be to use pthread_cond_t along with pthread_attr_setclock, maybe this is the way to go.

[fujitatomoya]

The ideal would be to use pthread_cond_t along with pthread_attr_setclock, maybe this is the way to go.

yeah, right. i think we are going to have to keep the platform dependent code to support sleep APIs with monotonic raw clock. otherwise, we can get the hardware slew-free clock via now, but sleep APIs are affected by system.

[fujitatomoya]

@jmachowinski how about we bring this topic to the PMC meeting before platform dependent implementation?

[fujitatomoya]

overall, lgtm except sleep APIs behavior.

[saikishor]

@jmachowinski how about we bring this topic to the PMC meeting before platform dependent implementation?

Yes, please let me know how you want me to proceed after the PMC meeting. Thank you for taking time to review @fujitatomoya

In the meantime, I'll implement things on rclpy side

[jmachowinski]

We discussed this shortly at the PMC meeting.
Some questions popped up:

• Is this portable ?
• What should be the behavior if the platform does not support monotonic raw ?

Currently this patch silently falls back to the non slew free clock if using timers etc. I guess in order to support this fully, one would need to implement a custom std::chrono::clock class using the now from rclutils and providing the specializations for wait_until for the std::conditional_variable.

[saikishor]

@jmachowinski Thank you for the summary

What should be the behavior if the platform does not support monotonic raw ?

If the platform doesn't support MONOTONIC_RAW, then the behavior will be similar to MONOTONIC or RCL_STEADY_TIME. There is nothing much we can do in that case.

Currently this patch silently falls back to the non slew free clock if using timers etc. I guess in order to support this fully, one would need to implement a custom std::chrono::clock class using the now from rclutils and providing the specializations for wait_until for the std::conditional_variable.

The problem is that we cannot directly use std::conditional_variable in these situations as it internally converts the clock to system_clock and then the behaviour would be similar.

The standard recommends that the clock tied to abs_time be used to measure time; that clock is not required to be a monotonic clock. There are no guarantees regarding the behavior of this function if the clock is adjusted discontinuously, but the existing implementations convert abs_time from Clock to [std::chrono::system_clock](https://en.cppreference.com/w/cpp/chrono/system_clock.html) and delegate to POSIX [pthread_cond_timedwait](https://pubs.opengroup.org/onlinepubs/9699919799/functions/pthread_cond_timedwait.html) so that the wait honors adjustments to the system clock, but not to the user-provided Clock. In any case, the function also may wait for longer than until after abs_time has been reached due to scheduling or resource contention delays.

Even if the clock in use is [std::chrono::steady_clock](https://en.cppreference.com/w/cpp/chrono/steady_clock.html) or another monotonic clock, a system clock adjustment may induce a spurious wakeup.

The effects of notify_one()/notify_all() and each of the three atomic parts of wait()/wait_for()/wait_until() (unlock+wait, wakeup, and lock) take place in a single total order that can be viewed as [modification order](https://en.cppreference.com/w/cpp/atomic/memory_order.html#Modification_order) of an atomic variable: the order is specific to this individual condition variable. This makes it impossible for notify_one() to, for example, be delayed and unblock a thread that started waiting just after the call to notify_one() was made.

https://en.cppreference.com/w/cpp/thread/condition_variable/wait_until.html

[jmachowinski]

What should be the behavior if the platform does not support monotonic raw ?

If the platform doesn't support MONOTONIC_RAW, then the behavior will be similar to MONOTONIC or RCL_STEADY_TIME. There is nothing much we can do in that case.

That's actually my biggest problem with the proposed patch. As enduser one would request a MONOTONIC_RAW clock and currently it silently falls back to MONOTONIC. The timers won't work on the requested time type, and the sleep functions won't work either as suggested by the API. Basically the only thing that might work as expected is rclcpp::Clock::now(). Looking at the implementation in rclutils, even this one has a silent fallback.

Note that the timer implementation in rcl_wait is flawed in the way that a relative wait time is computed from all the clocks and given to rmw_wait as a timeout. The underlying RWM implementation will treat this timeout most likely as MONOTONIC. Meaning that if there is a clock adjustment active, rmw_wait will either take to long, or wake up to early constantly, as it is affected by the clock adjustment.

On Friday there is a client working group meeting I think it would be beneficial if you could join the meeting.
The link can be found here : https://calendar.google.com/calendar/u/0/[email protected]&ctz=CET

[fujitatomoya]

@jmachowinski thank you so much for taking care of this. and sorry for missing last PMC meeting because of my relocation from US to Japan.

That's actually my biggest problem with the proposed patch. As enduser one would request a MONOTONIC_RAW clock and currently it silently falls back to MONOTONIC. The timers won't work on the requested time type, and the sleep functions won't work either as suggested by the API. Basically the only thing that might work as expected is rclcpp::Clock::now(). Looking at the implementation in rclutils, even this one has a silent fallback.

totally agree with you. this is surprise for user, and hard to find out.
IMO, if the platform does not support the MONOTONIC_RAW clock, it returns error to let the user know. at least that is much better than silently falling back to the MONOTONIC clock internally?

[jmachowinski]

@saikishor We had a lengthy discussion about this in the CWG and came to the conclusion that the rcutils PR looks useful to us, but the one for rcl and rclcpp seem flawed, or we are missing something.
The main argument was, that the wait operation in rcl / rclpp introduce so much random latency that we can't see a use case were you could notice a real difference between the monotonic and a monotonic_raw clock.

Would you mind sharing more details about your use case ?

[fujitatomoya]

our use case is that, sensing data coming from sensing framework use only monotonic raw clock time stamp. and of course we need to handle these sensing data with ROS 2 application, that requires ROS 2 application should be able to handle the compatible hardware clock with sensing data from the system.

[fujitatomoya]

@saikishor friendly ping.

[saikishor]

@saikishor friendly ping.

@fujitatomoya sorry I was super busy these days right before my vacation. I've checked the replies, but couldn't find time to reply properly. I'll surely get back to them ASAP. Sorry for the inconvenience 🙏🏽🙏🏽

[saikishor]

@saikishor We had a lengthy discussion about this in the CWG and came to the conclusion that the rcutils PR looks useful to us, but the one for rcl and rclcpp seem flawed, or we are missing something.
The main argument was, that the wait operation in rcl / rclpp introduce so much random latency that we can't see a use case were you could notice a real difference between the monotonic and a monotonic_raw clock.

The latency is coming due to the fact that wait_until from std::conditional_variable converts the clock to system_clock and this does affect right?

Would you mind sharing more details about your use case ?

I'll respond to your earlier message soon, in the meantime this is my usecase.

With the integration of more industrial grade motor control boards like ELMO on our robots, we have observed in ROS 1 with ros_control, that as the NTP clock adjustment affect the frequency at which the EtherCAT commands are send and thereby making the motors make rattling sound.

For critical applications, it is very important that the clock is consistent to trigger control. I want to improve this part in ros2_control, so everyone can make use of it.

I hope this gives you some context of my usecase.

[fujitatomoya]

@saikishor @jmachowinski thanks for taking care of this.

how about taking ros2/rcutils#507 for now? i think this is still useful for end user application to get the time with monotonic raw clock via ROS 2 API?

[saikishor]

That's actually my biggest problem with the proposed patch. As enduser one would request a MONOTONIC_RAW clock and currently it silently falls back to MONOTONIC. The timers won't work on the requested time type, and the sleep functions won't work either as suggested by the API. Basically the only thing that might work as expected is rclcpp::Clock::now(). Looking at the implementation in rclutils, even this one has a silent fallback.

If the platform doesn't support MONOTONIC_RAW, I think it is better to fallback to the MONOTONIC, because if not released packages would have to somehow add the logic based on the platform. Maybe we can add a WARN_ONCE print to mention that it is failing back to MONOTONIC.

On Friday there is a client working group meeting I think it would be beneficial if you could join the meeting. The link can be found here : https://calendar.google.com/calendar/u/0/[email protected]&ctz=CET

I'm sorry I missed the meeting that day.

IMO, if the platform does not support the MONOTONIC_RAW clock, it returns error to let the user know. at least that is much better than silently falling back to the MONOTONIC clock internally?

@fujitatomoya failing might not a good idea, then applications instead of running would simply crash on unsupported platforms, this means users will have to open PRs to add platform dependent logic in order to solve it. Instead a simple warning should work right?, this is all about performance rather than functionality. What do you think?

how about taking ros2/rcutils#507 for now? i think this is still useful for end user application to get the time with monotonic raw clock via ROS 2 API?

@fujitatomoya sure.

[ahcorde]

@fujitatomoya what's the status of this PR ?

[fujitatomoya]

because of the inconsistent behavior for user application explained in #2910 (comment), we are not inclined to take this API to the client layer yet.

i think that rcutils interfaces are fine as low level implementation.