Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Take and return new RMW_DURATION_INFINITE correctly #491

Merged

Conversation

emersonknapp
Copy link
Contributor

Depends on ros2/rmw#301

Meets the new API expectations introduced in ros2/rmw#301 - to accept and return RMW_DURATION_INFINITE for cross-implementation consistency.

@emersonknapp
Copy link
Contributor Author

@asorbini this is probably not worth merging here, right, and should instead go to rmw_connextdds?

@asorbini
Copy link
Contributor

@emersonknapp I can confirm I'd like support for this special value to be added to rmw_connextdds, but I can't really comment on whether it shouldn't be merged to rmw_connext_cpp too. I'm not sure what the plans are for EOL'ing the package, beside phasing it out of the source tree. I'm assuming development will eventually stop, but a maintainer should comment on that.

Feel free to open a PR for rmw_connextdds though, I'd be happy to review/merge.

Keep in mind that @ivanpauno is also working on some changes related to QoS configuration (see rticommunity/rmw_connextdds#7), so you might want to coordinate these additions with him.

@clalancette
Copy link
Contributor

My personal view on this is that the work here is done, so we should just go ahead and merge this. I wouldn't spend much additional time here, given that it is on its way out, but "on its way out" is not the same thing as "gone".

@emersonknapp
Copy link
Contributor Author

Gist: https://gist.githubusercontent.com/emersonknapp/08a0496aa3726c8053285e6def25ab07/raw/1550d9ed37d12a2824dfb876996e6b09bf6844ba/ros2.repos
BUILD args: --packages-up-to rmw_connext_cpp rcl
TEST args: --packages-select rmw_connext_cpp rcl
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/7857

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

@emersonknapp
Copy link
Contributor Author

^ the rmw_connext packages are being ignored by the build, I guess we've already disabled connext in CI?

@asorbini
Copy link
Contributor

^ the rmw_connext packages are being ignored by the build, I guess we've already disabled connext in CI?

My changes have only made it into a branch of ros2/ci.

I think rmw_connext_cpp was already disabled (as in, completely ignored) for linux-aarch64 and armhf. See this change from my commit for reference.

@emersonknapp
Copy link
Contributor Author

Oh! You're right it's only on ARM. Never mind then, we can just ignore that build

@clalancette
Copy link
Contributor

@ros-pull-request-builder retest this please

@emersonknapp
Copy link
Contributor Author

@ahcorde @mjcarroll as maintainers do you have any input on this?

@emersonknapp
Copy link
Contributor Author

@clalancette friendly ping - should we merge this?

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Sorry, I just noticed one issue with this PR. After that, I'm happy to approve.

rmw_connext_shared_cpp/src/qos.cpp Outdated Show resolved Hide resolved
Signed-off-by: Emerson Knapp <[email protected]>
Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

Looks good to me with green CI (which is probably redundant, but I'm being a little more conservative as we approach the Galactic deadline).

@emersonknapp
Copy link
Contributor Author

  • Linux Build Status
  • Linux-aarch64 (not supported by rmw_connext)
  • macOS Build Status
  • Windows Build Status

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants