Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Normalize rmw_time_t according to DDS spec #48

Merged
merged 4 commits into from
May 13, 2021

Conversation

asorbini
Copy link
Contributor

@asorbini asorbini commented Apr 3, 2021

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
Copy link
Contributor

emersonknapp commented Apr 3, 2021

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.

Signed-off-by: Andrea Sorbini <[email protected]>
@clalancette clalancette requested a review from hidmic April 22, 2021 13:35
rmw_dds_common/src/time_utils.cpp Outdated Show resolved Hide resolved
rmw_dds_common/src/time_utils.cpp Outdated Show resolved Hide resolved
rmw_dds_common/src/time_utils.cpp Outdated Show resolved Hide resolved
rmw_dds_common/test/test_time_utils.cpp Show resolved Hide resolved
* Use constexpr instead of static const.
* Make variable declaration more readable.
* Simplify test for overflow detection.
* Introduce additional tests for overflow and normalization.

Signed-off-by: Andrea Sorbini <[email protected]>
Copy link
Contributor

@hidmic hidmic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending green CI

rmw_dds_common/src/time_utils.cpp Show resolved Hide resolved
@hidmic
Copy link
Contributor

hidmic commented May 13, 2021

CI up to rmw_dds_common and test_rmw_implementation:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@hidmic
Copy link
Contributor

hidmic commented May 13, 2021

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

@hidmic
Copy link
Contributor

hidmic commented May 13, 2021

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

@hidmic hidmic merged commit eeb04c3 into ros2:master May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants