ros2 / domain_bridge

Bridge communication across different ROS 2 domains.
Apache License 2.0
55 stars 11 forks source link

Rti qos profiles patch #25

Closed ivanpauno closed 3 years ago

ivanpauno commented 3 years ago

This is a workaround to be able to use a different participant qos profile in each domain.

For example:

export RMW_IMPLEMENTATION=rmw_connextdds
export NDDS_QOS_PROFILES=/path/to/examples/EXAMPLE_ROS2TEST_QOS_PROFILES.xml
# The domain bridge executable will try to load a qos profile named "ParticipantProfile_{domain_id}" before creating a participant in each domain
export DOMAIN_BRIDGE_CONNEXT_DDS_QOS_PROFILE_PREFIX=ParticipantProfile
ros2 run domain_bridge domain_bridge src/domain_bridge/examples/example_bridge_config.yaml

We probably don't want to merge this in the main branch. We can keep this patch in a separate branch, or only merge the callback trick into 'main` and create a new executable using it in a separate package.

ivanpauno commented 3 years ago

@jacobperron FYI, if you have a different idea of how to do this that would be great.

ivanpauno commented 3 years ago

I did some testing with this, and it seems to work as expected (I disabled SHMEM transport in only some of the domains and checked that it was set correctly).

mallanmba commented 3 years ago

We probably don't want to merge this in the main branch. We can keep this patch in a separate branch, or only merge the callback trick into 'main` and create a new executable using it in a separate package.

Would it be reasonable to merge the callback into the main branch and keep an example of how to use it w/ connext in the repo? This might be something someone else out there would want to do at some point.

ivanpauno commented 3 years ago

Would it be reasonable to merge the callback into the main branch and keep an example of how to use it w/ connext in the repo?

:+1: to merge the callback patch in main.

I don't mind to also keep the example with the Connext qos profiles patch here. I would prefer to not build that example though, or to build it only if Connext was found. In that way, we avoid depending directly on Connext.

@jacobperron what do you think?

ivanpauno commented 3 years ago

FYI I noticed that the patch in https://github.com/ros2/domain_bridge/pull/25/commits/3e7faca7409dabe12a15e37fafbc5b9e2444b4b3 is not working correctly (it fails to reset the qos participant profile). I will look for a fix tomorrow.

ivanpauno commented 3 years ago

FYI I noticed that the patch in 3e7faca is not working correctly (it fails to reset the qos participant profile).

I forgot to set when testing DOMAIN_BRIDGE_CONNEXT_DDS_QOS_PROFILE_PREFIX, the only problem was that when that variable was not set the last call to DDS_DomainParticipantFactory_set_default_participant_qos() failed. https://github.com/ros2/domain_bridge/pull/25/commits/baa755096b1029ef1912de8550ebf55c1175c7b4 fixes the issue.

ivanpauno commented 3 years ago

The CI issue is unrelated, the problem is that the bridge is only discovering one of the publishers (the reliable one), and so it automatically uses a Reliable profile instead of a best effort one. The current test is racy, I think we can improve that in a new PR.

ivanpauno commented 3 years ago

Rebased, now this branch is incompatible with galactic, but it will throw an exception that will make that clear (same behavior as in main).

The error message is: "unexpected callback being called".

https://github.com/ros2/domain_bridge/pull/29 is an easy fix to reintroduce compatibility with galactic, https://github.com/ros2/domain_bridge/pull/30 is a refactor that fixes the issue in a more elegant fashion.

jacobperron commented 3 years ago

I'm okay with merging the new callback mechanism. For the Connext-specific example, I'd rather not add any dependencies to Connext to this repo. So we could either keep it as an non-compiled example, or move it to it's own package (like domain_bridge_examples). I don't have a strong preference.

ivanpauno commented 3 years ago

Rebased with master

ivanpauno commented 3 years ago

I'm okay with merging the new callback mechanism. For the Connext-specific example, I'd rather not add any dependencies to Connext to this repo.

After the last commits, I'm conditionally compiling the domain bridge executable with the custom patch to be able to configure a different domain participant qos for each domain (the new executable is call domain_bridge_rti_qos) I would like to add a note to the docs and better documentation, but I think we can do that in a follow up PR.

ivanpauno commented 3 years ago

Force-pushed to resolve more conflicts

ivanpauno commented 3 years ago

The flaky tests are annoying. I'll see if I can make some time to look into the failures (#22). Maybe as a short-term workaround we can add a retry flag.

Agreed, I will take a look to that one soon.

ivanpauno commented 3 years ago

Going in, thanks for the review @jacobperron !