Deprecate rmw_time_t

[emersonknapp]

Resolves #215

Removes rmw_time_t from the RMW API in favor of an int-based nanosecond type.
This simplifies most time-based code in the core, since rcl, rclcpp, and rclpy (and cyclonedds) all use an int64-based nanosecond timestamp already - it's a net-negative code change.

This should make #255 a lot easier to define - no special allowances will be needed around conversions to and from duration representations, nor can there be non-normalized durations (nanoseconds > 1billion)

Linked PRs:

• Use new rmw_duration_t rmw_implementation#178
• Remove rmw_time_t utilities rmw_dds_common#43
• Use new rmw_duration_t rmw_cyclonedds#283
• Use new rmw_duration_t rmw_fastrtps#508
• Use new rmw_duration_t rmw_connext#485
• Use new rmw_duration_t rcl#889
• Use new rmw_duration_t rclcpp#1549
• Use new rmw_duration_t rclpy#679
• Use new rmw_duration_t rosbag2#646
• Use new rmw_duration_t demos#484

Gist: https://gist.githubusercontent.com/emersonknapp/32eb5733086c7f6876e4bf9ece5d5fef/raw/caa6d31a0a84866cbc4e51afb60ad8b551a05acd/ros2.repos

• Linux
• Linux-aarch64
• macOS
• Windows

[jacobperron]

A couple minor comments, otherwise LGTM.

Would be good to get a second approval before moving forward with the deprecation.

[emersonknapp]

A couple minor comments, otherwise LGTM.

Would be good to get a second approval before moving forward with the deprecation.

Thanks for the comments - I'll take care of them.

Do you think we should deprecate the existing rmw_wait signature (to be removed in H-turtle) and add this new signature as an override? Or, is replacing the existing function OK?

I'm not actually sure if RMW implementations are allowed to leave RMW APIs unimplemented. If it's an error to not implement the new signature, then I wouldn't see a reason to deprecate+overload considering they would have to be touched regardless. But otherwise if it's not an error, then maybe it makes sense in the tick-tock strategy to do it that way.

[jacobperron]

Do you think we should deprecate the existing rmw_wait signature (to be removed in H-turtle) and add this new signature as an override? Or, is replacing the existing function OK?

How do you propose to deprecate the signature? C doesn't support overloading, so I'm not sure how best to deprecate the existing signature short of introducing a new symbol (e.g. rmw_wait_v2).

[emersonknapp]

C doesn't support overloading,

Of course, I was still thinking in C++. I don't have an answer then - introducing a function with a different name is not a great alternative.

[ivanpauno]

Of course, I was still thinking in C++. I don't have an answer then - introducing a function with a different name is not a great alternative.

You can achieve "overloading" in C with the _Generic macro (part of the C11 standard).
See for example https://stackoverflow.com/a/25026358 or https://devblogs.microsoft.com/cppblog/c11-and-c17-standard-support-arriving-in-msvc/.

I'm not sure if we want to use that thingy or not (I'm pretty sure MSVC/gcc/clang support that, but probably not other compilers).

If we want to move forward with this change, I also don't mind about having a rcl_wait2 or rmw_wait2 (or if considered ok, breaking API).

[emersonknapp]

Would be good to get a second approval before moving forward with the deprecation.

@jacobperron who would be best to weigh in with a second opinion? I have the followup of #255, which I'd like to get these both done in time for Galactic API freeze - just want to make sure we have a good buffer, I am not a fan of the scramble at distro freeze time.

[clalancette]

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?

[emersonknapp]

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);

[emersonknapp]

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?

[emersonknapp]

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?

[clalancette]

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.

[emersonknapp]

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

[clalancette]

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.

[emersonknapp]

#301

[emersonknapp]

I am closing this for now as I have no specific plan to readdress it later. I'm fairly convinced that there is not much benefit, and the simple helper functions added in #301 meet current needs.