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

Fix node discovery with dps middleware in ROS2 #124

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

talih0
Copy link

@talih0 talih0 commented Sep 8, 2020

Currently node discovery does not work with dps middleware in ROS2.

The issue is as follows: When a subscriber comes online it sends out a
multicast discovery message. All the subscriber topics (bloom filter
outputs) are merged in a union and sent out as an "interest" message.

When the multicast message is received by the publishing node, it checks
whether any topic is in the set of the received interest. If a match is
found a unicast connection is created and the published message is sent
to the subscriber.

However at the moment, the publisher's bloom filter output is the union
over all possible wildcards, whereas a reduced set is calculated on the subscriber.
Thus the publisher's bloom filter output will not be in the set of the
subscriber's interests. Hence, the unicast connection is not created.

This commit makes sure the subscriber's interests are created in a
symmetrical way to the publisher side which solves the discovery
problem.

Also when creating the publisher's discovery interest use the wildcard
option. This is primarily so that discovery information is sent out
on localhost given the above change.

TODO: dps-micro updates

Signed-off-by: Andriy Gelman [email protected]

Currently node discovery does not work with dps middleware in ROS2.

The issue is as follows: When a subscriber comes online it sends out a
multicast discovery message. All the subscriber topics (bloom filter
outputs) are merged in a union and sent out as an "interest" message.

When the multicast message is received by the publishing node, it checks
whether any topic is in the set of the received interest. If a match is
found a unicast connection is created and the published message is sent
to the subscriber.

However at the moment, the publisher's bloom filter output takes into
account wildcards whereas they are not considered in the subscriber.
Thus the  publishers bloom filter output will not be in the set of the
subscriber's interests.  Hence, the unicast connection is not created.

This commit makes sure the subscriber's interests are created in a
symmetrical way to the publisher side which solves the discovery
problem.

Also when creating the publisher's discovery interest use the wildcard
option. This is primarily so that discovery information is sent out
on localhost given the above change.

Signed-off-by: Andriy Gelman <[email protected]>
Signed-off-by: Andriy Gelman <[email protected]>
@malsbat
Copy link
Contributor

malsbat commented Sep 9, 2020

ros2 does not support wildcards in topics (unless something has changed recently). Assuming the analysis is correct, the more straightforward fix would be to change noWildCard to DPS_TRUE in the calls to DPS_InitPublication in rmw_client.cpp and rmw_publisher.cpp. This appears to be a typo in rmw_dps.

But, do you have a test case showing the topics for subscriber and publisher that are expected to match and do not? I'm a bit surprised that an issue with the pub/sub matching was not caught much earlier. Changes to DPS_AddTopic will require some thorough testing. The topic_match test will be useful for this.

@talih0
Copy link
Author

talih0 commented Sep 9, 2020

ros2 does not support wildcards in topics (unless something has changed recently). Assuming the analysis is correct, the more straightforward fix would be to change noWildCard to DPS_TRUE in the calls to DPS_InitPublication in rmw_client.cpp and rmw_publisher.cpp. This appears to be a typo in rmw_dps.

Yes, this approach works too. I'll do a few more tests and submit a patch to rmw_dps.

But, do you have a test case showing the topics for subscriber and publisher that are expected to match and do not? I'm a bit surprised that an issue with the pub/sub matching was not caught much earlier.

I'm testing the minimal examples from the ROS2 eloquent repo, which doesn't work for me without the above change (or my original patch).
examples_rclcpp_minimal_subscriber/subscriber_lambda
examples_rclcpp_minimal_publisher/publiser_lambda

It's probably the same issue as:
ros2/rmw_dps#43

Changes to DPS_AddTopic will require some thorough testing. The topic_match test will be useful for this.

I tested different combinations and didn't see regressions before/after the patch. It may be good to keep this PR open in case ROS2 supports wildcards in the future.

@malsbat
Copy link
Contributor

malsbat commented Sep 14, 2020

@talih0, I believe #125 is the root cause of the issue you're seeing.

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.

2 participants