search

ros2 / domain_bridge

Bridge communication across different ROS 2 domains.
Apache License 2.0
52 stars 12 forks source link

Domain bridge container #32

Closed rebecca-butler closed 3 years ago

rebecca-butler commented 3 years ago

This PR is related to ros2/rclcpp#1702.

Signed-off-by: Rebecca Butler rebecca@openrobotics.org

rebecca-butler commented 3 years ago

Instead of adding a new executable, what do you think about making the existing executable a "container"?

I agree, this makes sense for reducing code duplication.

jacobperron commented 3 years ago

The changes lgtm, pending the decision in https://github.com/ros2/rclcpp/pull/1702. We should document this feature (maybe a new section in the README) and some simple regression tests would be good too.

rebecca-butler commented 3 years ago

Following the discussion in ros2/rclcpp#1702, this PR now contains a component manager specific to the domain bridge.

jacobperron commented 3 years ago

We'll have to wait until the connected PR is merged and released before the CI and RPr jobs will pass. I don't think getting the upstream component manager change into Galactic is likely since it changes ABI (I think adding a new virtual function is not guaranteed to be ABI compatible). So, we'll have to diverge from Galactic (e.g. create a new galactic branch).

ivanpauno commented 3 years ago

I think adding a new virtual function is not guaranteed to be ABI compatible

:+1: Adding a new virtual function breaks ABI.

jacobperron commented 3 years ago

I've created a galactic branch and opened a rosdistro PR to point to it: https://github.com/ros/rosdistro/pull/30128

rebecca-butler commented 3 years ago

@ros-pull-request-builder retest this please

rebecca-butler commented 3 years ago

@ros-pull-request-builder retest this please

jacobperron commented 3 years ago

I manually retriggered the GitHub workflow too.

We can ignore the Gpr job since we've already updated rosdistro to point to the galactic branch.

jacobperron commented 3 years ago

I guess the workflow may not use our testing repo for fetching Debians (or it's caching an older version of the package). I think we can ignore it and rely on the RPr job.

rebecca-butler commented 3 years ago

There's an API change in ros2/rclcpp#1716 that affects this PR, so I think we should wait until that lands before merging.

rebecca-butler commented 3 years ago

I rebased and CI looks good now. @jacobperron could you give this one more look before we merge?

rebecca-butler commented 3 years ago

@ivanpauno I added a test for this, could you take a look again? Thanks!