rclcpp: rclcpp::Duration does not include a sleep() method

I would strongly recommend that if we make sure that the simplest way to sleep that is our recommended pattern will follow the ROS clock. If we make the bare function rclcpp::sleep_for the default pattern then many algorithms will fail in subtle ways when run on simulated time. These bugs will be very hard to debug and will cause future problems. I’d almost recommend that we make this bare function implementation only and provide the user aware sleep at the Node level recommended for the users which can be made ROS Timesource/Clock aware. And looking at this we should make the Rate sleep time source aware too.

Sometimes you also just want to sleep for a specific duration of time. If I wanted to sleep for 0.3463 seconds (context: scheduler, waiting for an event to occur of a known time, etc), sure I could invert it and throw it into rate, but semantically would be more readable to have Duration be able to sleep.

I guess that’s the part that doesn’t make much sense to me; a time interval (a Duration) doesn’t sleep. The thread it is on, or the Rate, does sleep, and can take a Duration as the amount of time to sleep. But I just don’t think it makes logical sense for the Duration itself to sleep. Making an analogy to the C++ std::chrono API, there is no sleep method on duration. However, sleep_for receives a duration, which makes sense to me.

Maybe your use case could be served by adding a new constructor overload on the Rate class to take a Duration, rather than only std::chrono::nanoseconds? Or maybe adding a new overload of sleep_for that takes a Duration?

pinging @ros2/team to get other opinions here.

My intent is somehow to get a Duration based sleep() somewhere. The discussion on ROS time aware sleep() I think has progressed as a “OK, if we’re going to do this, lets do that at the same time”, but isn’t a requirement for this ticket to be fulfilled.

I think @tfoote’s feeling (and my own) is that we should not provide a way to sleep that does not respect ROS Time. Adding sleep() to Duration would be doing that. So I think we’re saying that’s not a good idea.

We already have rclcpp::sleep_for(chrono duration), but perhaps we should deprecate that and/or put it into a detail namespace as well.

In the vein of avoiding subtle bugs, as @tfoote put it:

If we make the bare function rclcpp::sleep_for the default pattern then many algorithms will fail in subtle ways when run on simulated time. These bugs will be very hard to debug and will cause future problems.

Perhaps a compromise would be a Duration::wall_sleep() which is documented to specifically sleep according the system time and not adhere to sim time or shutdown. Then again that could be emulated easily with std::this_thread::sleep_for(rclcpp::Duration(1).nanoseconds()), so maybe it’s not that valuable.

Further more, now that chrono exists and Duration conceptually is not related to a clock at all, maybe we should not have a duration object in rclcpp, instead using std::chrono::duration and having free functions to convert those to and from our Duration.msg type where needed.

I think it makes sense for sleep_for to not consider clocks, take std::this_thread::sleep_for and std::this_thread::sleep_until for examples. The first takes a std::chrono::duration which has no clock associated, and the second takes a std::chrono::time_point which is templated on a clock type. We would probably want to emulate that pattern.

That’s fair. I’m just pointing out that as of today’s rclcpp, rclcpp::sleep_for is not a ros::Duration::sleep equivalent. I’ve had to workaround it before using timers bound to the node’s clock (which does pay attention to use_sim_time), and it’s a bit of hassle.

This discussion is orthogonal to the proposal here (though it is something I’d like to discuss in the future).

For this particular proposal, I think we have agreement to have both a new rclcpp::sleep_for(const Duration &) and a new Rate(const Duration &) method. @SteveMacenski if you’d like to propose a patch, I’d be happy to review.

An example would be if you wanted to throttle an update loop (which I know this example is internally implemented in Rate but could be simplified using a Duration sleep). You aim to throttle a loop to 10hz but the workers are only taking 20ms to compute a task. You take a time when you start the loop and after you finish the work. This Duration then has the amount of time taken. Subtracting that duration from the 10 hz would give you the sleep duration to then call sleep() on.

Sometimes you also just want to sleep for a specific duration of time. If I wanted to sleep for 0.3463 seconds (context: scheduler, waiting for an event to occur of a known time, etc), sure I could invert it and throw it into rate, but semantically would be more readable to have Duration be able to sleep.

Finally, ROS 1’s duration had a sleep, so it would be nice to just have feature parity. I don’t think it hurts anything to have a Duration sleep().