The thing I'm concerned about here is that this is an API change at the RMW layer. While we can fix all of the in-tree rmws, this still is a hard break for all of the out-of-tree ones. I'd be much happier with this change if we deprecated the old signature, added a new signature, and had a shim to convert between them. Then we can remove the old signature for H-Turtle. Thoughts?

Is this what you're suggesting?

Galactic

rmw_wait(const rmw_time_t * wait_timeout) DEPRECATED { 
    rmw_wait2(time_to_duration(wait_timeout));
}

rmw_wait2(rmw_duration_t duration);

H-Turtle

rmw_wait(rmw_duration_t duration);

rmw_wait2(rmw_duration_t duration) DEPRECATED {
    rmw_wait(duration);
}

I-Turtle

rmw_wait(rmw_duration_t duration);

Or more along the lines of just introducing a new name entirely (can't think what it should be called... rmw_wait_on_set?), then just deprecate rmw_wait forever in I-turtle.

Or, do you think this change is not warranted and that we should maintain the rmw_time_t type?

Oh - another note: changing rmw_qos_profile_t to use rmw_duration_t instead of rmw_time_t is also is a breaking change for anything interacting with them directly - e.g. in https://github.com/ros2/rmw_cyclonedds/pull/283/files the .sec and .nsec usages have to be fixed. How would we handle this?

Or, do you think this change is not warranted and that we should maintain the rmw_time_t type?

That's hard for me to say. If I take a look at how the const rmw_time_t * wait_timeout parameter to rmw_wait is used by the various RMWs, it looks like this:

• Fast-DDS - maps it to a Duration_t, which uses an int32_t/uint32_t to represent the same data. There's clearly an impedance mis-match there (and the source of the clamp_rmw_time_to_dds_time method in rmw_dds_common).
• CycloneDDS - maps it to an internal dds_time_t. There is some possibility of impedance mismatch there (since in theory you could have very large numbers in both sec and nsec, and hence overflow the int64_t that dds_time_t is), but in real life that would be a pathological case. (Incidentally, we should probably check for that pathological case before doing the conversion at https://github.com/ros2/rmw_cyclonedds/blob/d024823043504ea40af24bf22365a21cd203df55/rmw_cyclonedds_cpp/src/rmw_node.cpp#L3384 , but that is orthogonal to this) (Also, it is kind of weird that rmw_cyclonedds maps this to a dds_time_t and not a dds_duration_t. Practically it doesn't matter since they are both int64_t, but that seems more natural).
• Connext - maps it to an internal DDS::Duration_t. Internally this seems to be an int32/uint32 sec/nsec in Connext: https://community.rti.com/rti-doc/500/ndds.5.0.0/doc/html/api_cpp/structDDS__Duration__t.html
• (extra credit) eCal maps it to std::duration here . Implicitly, the way that they are using it as waiting on a condition variable seems to be using it as an int64.
• (extra credit) iceoryx maps it to a struct timespec here.
• (extra credit) dps maps it to a std::duration here
• (extra credit) gurumdds maps it to an internal dds_Duration_t. That uses an int32/uint32 to represent sec/nsec, much like Fast-DDS.

So, in short, all of the RMWs do it differently. No matter what we do here, we're going to have some impedance mismatch with one of them. From that perspective, rmw_duration_t doesn't seem substantially better than rmw_time_t, just different.

However, it may make sense to look at it from the point of view of the users of the RMW. Do you think it makes more sense for a caller of rmw_wait to use an rmw_duration_t instead of an rmw_time_t? If so, why?

Or more along the lines of just introducing a new name entirely (can't think what it should be called... rmw_wait_on_set?), then just deprecate rmw_wait forever in I-turtle.

Even simpler:

For Galactic:

• add in rmw_wait_on_set(rmw_duration_t duration);
• mark rmw_wait(rmw_time_t *) deprecated
• Update all internal users to use rmw_wait_on_set

For H-Turtle:

• Remove rmw_wait(rmw_time_t *)

Oh - another note: changing rmw_qos_profile_t to use rmw_duration_t instead of rmw_time_t is also is a breaking change for anything interacting with them directly - e.g. in https://github.com/ros2/rmw_cyclonedds/pull/283/files the .sec and .nsec usages have to be fixed. How would we handle this?

Ug. In an ideal world, we would add another structure and do the deprecation dance there as well, but that is starting to get a bit ridiculous. I'm curious to hear your thoughts above before we discuss the finer point here.

Well, at the end of the day, my real goal is to solve #210, which is an actual behavioral problem that is not handled in RMW. Based on this conversation, I'm inclined to lean back on an implementation that doesn't require changing rmw_time_t to achieve the goal.

Edit: my inclination especially is based on the fact that there's no across-the-board benefit for RMW implementations.
The original reason to change this was to make time representation more uniform across the ROS 2 core, since rcl, rclcpp, rclpy all represent times/durations as a single int64 nanoseconds. Perhaps we can gain the necessary benefit with just a few convenience functions/macros for converting between

Edit: my inclination especially is based on the fact that there's no across-the-board benefit for RMW implementations.
The original reason to change this was to make time representation more uniform across the ROS 2 core, since rcl, rclcpp, rclpy all represent times/durations as a single int64 nanoseconds. Perhaps we can gain the necessary benefit with just a few convenience functions/macros for converting between

I see. So that would argue for making the switch to int64. Making that more consistent does seem worthwhile, but I also think that it shouldn't block your other work on #210. So maybe the thing to do here is to put all of this on hold, concentrate on #210, and return to this after Galactic.

#301