ros2 / rmw

The ROS Middleware (rmw) Interface.
Apache License 2.0
95 stars 67 forks source link

Introduce RMW_DURATION_INFINITE constant and API return value promise #301

Closed emersonknapp closed 3 years ago

emersonknapp commented 3 years ago

Resolves #210 Alternative to #255

Introduce a new constant, RMW_DURATION_INFINITE. Specify in get_topic_endpoint_info that it is expected to be returned to represent infinite durations. Add some helper functions that enable better usage of this new value.

This change is non-breaking, all RMW implementations will continue to work correctly as-is. However, there are two expectations that they must eventually implement:

  1. If a user inputs RMW_DURATION_INFINITE for Deadline, Lifespan, or Liveliness Lease Duration QoS policy values, this should be translated into the implementation-specific infinity value.
  2. When returning values in get_topic_endpoint_info, RMW impementations must intercept their own internall-defined "infinity" definitions and replace those values with RMW_DURATION_INFINITE, so that they can be understood correctly.

Today, I will not be changing the default qos_profile values to RMW_DURATION_INFINITY, as this would break existing implementations that do not know how to handle this value correctly. I would like to create a "value deprecation" so that this default value could be switched over in time for H-Turtle, but I'm not sure how this should be done.

Dependent (but not linked, can be merged after) PRs:

emersonknapp commented 3 years ago

@jacobperron @clalancette @mm318 @hidmic @fujitatomoya @Barry-Xu-2018

Hey all, as participants in the #210 discussion, I would like your feedback on this. See discussion on #298 for why I'm not addressing #215 here.

I've only performed one RMW implementation update, to show the pattern, without doing too much extra work. After going too far on other iterations of this feature, I wanted to wait until we have some consensus on the API before opening more update PRs.

emersonknapp commented 3 years ago

@jacobperron @hidmic @fujitatomoya Thanks for the feedback - I've pushed a new version that I hope addresses all concerns:

emersonknapp commented 3 years ago
emersonknapp commented 3 years ago
emersonknapp commented 3 years ago

@jacobperron looks all good except the rcl_action tests on Windows - it looks like those same tests failed in last night's nightly https://ci.ros2.org/view/nightly/job/nightly_win_rel/1863/#showFailuresLink so I'm assuming it's a preexisting issue - should we block on merging?

jacobperron commented 3 years ago

@emersonknapp Looks like this is the related issue: https://github.com/ros2/rmw_connext/issues/472

I think it's okay to not block on the rcl_action graph test failures.

emersonknapp commented 3 years ago

Sounds good to me! Note: I don't have merge access here so a maintainer will need to merge

emersonknapp commented 3 years ago

@jacobperron @hidmic friendly ping: can you merge this or direct it to the person who can?

hidmic commented 3 years ago

Thanks for the bump @emersonknapp. Merging.