search

ros2 / rmw_dds_common

Apache License 2.0
11 stars 20 forks source link

Normalize rmw_time_t according to DDS spec #48

Closed asorbini closed 3 years ago

asorbini commented 3 years ago

This PR updates the implementation of rmw_dds_common::clamp_rmw_time_to_dds_time() to better comply with the DDS specification, which mandates that time values be normalized to have the "nanoseconds" component always < 1 second.

Quoting the relevant bit from the spec ([v1.4](), section 2.3.2):

The two types used to represent time: Duration_t and Time_t are mapped into structures that contain fields for the second and the nanosecond parts. These types are further constrained to always use a ‘normalized’ representation for the time, that is, the nanosec field must verify 0 <= nanosec < 1000000000.

The updates limit the values produced by rmw_dds_common::clamp_rmw_time_to_dds_time() to a maximum of {INT_MAXseconds,10^9 - 1` nanoseconds}.

I came up with these changes while working on rmw_connextdds#21, and even though rmw_connextdds will not directly use this function for WaitSet::wait() going forward, I'm still submitting the PR because the function will be used again in the future for other purposes (e,g, see rmw_connextdds#20), and this might be a slightly "safer" conversion than just truncating at [U]INT_MAX, because it guarantees that any compliant DDS implementation will be able to represent the output.

The implementation intentionally doesn't handle constant RMW_DURATION_INFINITE, because it expects the caller to handle that case explicitly. If desired, the function could be extended to handle this value (and maybe produce in output { 0x7FFFFFFF, 0x7FFFFFFF } which the DDS spec defines as DDS_DURATION_INFINITE).

While reviewing the value of RMW_DURATION_INFINITE I have also been brought to wonder why it wasn't defined to be { UINT64_MAX, UINT64_MAX } and it instead uses a special canary value (INT64_MAX split between the two fields). It's not so much a problem when it comes to DDS (since the value is beyond what can be represented), but API-wise, I would have expected this special value to fall on some boundary of the "natural domain" of the type, rather than somewhere in the middle.

emersonknapp commented 3 years ago

API-wise, I would have expected this special value to fall on some boundary of the "natural domain" of the type, rather than somewhere in the middle.

For context, the reasoning is that most of the ros2 codebase uses an int64_t nanoseconds representation for durations. Only the RMW interface uses a split seconds+nanoseconds struct. This is good because it's the maximum duration, e.g. rclcpp and rclpy can represent. Unfortunately the downside is that it looks a little awkward in DDS.

hidmic commented 3 years ago

CI up to rmw_dds_common and test_rmw_implementation:

hidmic commented 3 years ago

This could be backported to Galactic. It can probably wait till patch release 1.

hidmic commented 3 years ago

Alright. Moving forward. Thanks for the fix @asorbini.