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

[SteveMacenski]

Bug report

Required Info:

• Operating System: Ubuntu
  
  
  
  
  
• Installation type: N/A
  
  
  
  
  
• Version or commit hash: Main
  
  
  
  
  
• DDS implementation: N/A
  
  
  
  
  
• Client library (if applicable): rclcpp
  
  
  
  
  

Feature request

Feature description

Add a sleep() method to Duration analog to Rate to sleep for a specified duration rather than at a given rate, when semantically more logical.

[clalancette]

Add a sleep() method to Duration analog to Rate to sleep for a specified duration rather than at a given rate, when semantically more logical.

I'm honestly not sure that this makes sense. Duration represents exactly that; the representation of the difference between one time and another. It makes sense to me to use Duration as an input to an action to sleep, but it doesn't make sense to me to perform an action directly on it. Also in order to implement sleep, you'd have to attach a clock to the Duration object, which again doesn't make much sense to me.

Could you maybe explain a little more about your use case and what your thinking is here? Thanks.

[SteveMacenski]

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().

[clalancette]

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.

[SteveMacenski]

Both of those options would totally work, I suppose I wasn't creative enough to come up with that :smile_cat:

I'd prefer the sleep_for of the two, but I think both would find use.

[hidmic]

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?

Both would be nice additions. Though unlike ros::Duration::sleep() , rclcpp::sleep_for() knows nothing about ROS clocks and so all sleeps are system time sleeps. Just bear that in mind if you expect this to work against a simulated clock.

[wjwwood]

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.

The only reason rclcpp::sleep_for exists, rather than using std::this_thread::sleep_for everywhere, is so that we can interrupt it when shutdown occurs, which is something we had in ROS 1 that we needed to emulate.

I don't think having sleep on a duration object makes sense personally. I'd prefer to have the rclcpp::sleep_for optionally take an rclcpp::Duration instead, as @clalancette said. Even if it means we have to have a paragraph in the ROS 1/ROS 2 migration guide about it.

[hidmic]

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.

A chrono-like API would be great. But whether it's sleep_for or sleep_until, I still think it'd be nice to have some sleep API that abides to ROS clocks.

I don't think having sleep on a duration object makes sense personally.

Agreed.

[SteveMacenski]

I still think it'd be nice to have some sleep API that abides to ROS clocks.

I agree, that would be the most ideal outcome, but I think everything brought up here are good suggestions.

[clalancette]

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.

[tfoote]

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.

[wjwwood]

Well then I think we shouldn't have a sleep_for and instead we should have a sleep_until that only takes a Time. Connecting sleep_for and/or duration to a clock isn't the right approach.

[SteveMacenski]

@tfoote, if I understand your suggestion, you're proposing we expose this function to the Node (analog to now()) but implemented in the clock taking in a Duration &. I agree that rate should also follow the current clock. I think if a user wants wall time regardless of simulation, they should just use chrono. That's more or less the pattern we use in Nav2 to track timing for navigation vs timing for watchdogs.

[sloretz]

It seems like this ticket has become a discussion of where a ROS time aware sleep() method should live. In that sense, it seems like a duplicate of #465. This ticket has more discussion so I'm inclined to close the older one. Anyone have thoughts on which should stay open (@tfoote especially yours since you opened #465)?

---

Personally I'm a fan of rclcpp::sleep_for(clock, duration) as long as it uses the clock but, doesn't need internal state.

[SteveMacenski]

I think the discussion has steered in that direction, but the intend of the 2 tickets are different. 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.

[wjwwood]

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.

[tfoote]

Also cross referencing this to longstanding feature request https://github.com/ros2/rclcpp/issues/465 which has high overlap.

[SteveMacenski]

We could close this ticket if you feel that the other one covers the scope