rmw
rmw copied to clipboard
ros2
rmw_time_t is redundant with rcutils_duration_value_t and doesn't need to be a struct
rmw_time_t is defined as:
typedef struct RMW_PUBLIC_TYPE rmw_time_t
{
uint64_t sec;
uint64_t nsec;
} rmw_time_t;
This differs from rcutils_duration_value_t which admits a value as just a int64_t holding nanoseconds. It probably makes sense to replace rmw_time_t with a type alias for rcutils_duration_value_t, and to make clear that it is a duration, not a time point.
It occurs to me that there is (until #214) no rmw concept of absolute time. It may make sense in the future to think about adding a rmw-implemented timestamp and comparator function so the RMW can use a different time concept, like vector clocks.
Why do you think rmw_time_t is supposed to be a duration? Its uses of "uint64_t" seems to suggest it's a timestamp.
That means it would be redundant with rcutils_time_point_value_t, though.
Because every existing use of it (wait time, liveliness, lifespan, deadline) is a relative time and because https://github.com/ros2/rclcpp/blob/911291f8d32da8a9a70f69adc7bcf1e41477e017/rclcpp/include/rclcpp/duration.hpp#L111.
Both time_point and duration are int64_t. The distinction is pretty meaningless in C, but matters in C++ (where you can enforce timestamp + duration = timestamp)
BTW, I don’t think this issue is high priority. I’d like to see it eventually fixed.
I'm looking at working on this in parallel to #210
Right now, I think rmw_time_t is used in 11 packages of ros2.repos. Most of the use is in tests or demos, luckily most use is hidden behind language client facades (rclcpp::Duration, rclpy.duration.Duration).
It makes sense to make a pass through ros2.repos, removing use, but it seems aggressive to delete the rmw_time_t type immediately. Should we give the type a deprecation notice via RCUTILS_DEPRECATED_WITH_MSG for Galactic, waiting to remove the type in H-turtle?
Noting discussion from middleware WG meeting - no immediate concern about calling this type deprecated in Galactic to be removed in H-turtle. Expectation would be to remove the use of the types from ros2.repos in time for Galactic so that there are not compiler warnings in the core distro
I think certain time window should be given for deprecation, the reasons are
- All non-Tier1 rmw implementation will get build break because of this change.
- Possibly implementation uses
rmw_time_tinternally. - fix all of the related packages are kinda heavy.
I think certain time window should be given for deprecation, the reasons are
Agreed. We actually talked about this in more detail over at https://github.com/ros2/rmw/pull/298#discussion_r579420814 , and the consensus there was to revisit this after Galactic.
