intel / dps-for-iot

Other
61 stars 23 forks source link

Fix node discovery with dps middleware in ROS2 #124

Open talih0 opened 4 years ago

talih0 commented 4 years ago

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 andriy.gelman@gmail.com

malsbat commented 4 years ago

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 commented 4 years ago

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: https://github.com/ros2/rmw_dps/issues/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 commented 4 years ago

@talih0, I believe https://github.com/intel/dps-for-iot/pull/125 is the root cause of the issue you're seeing.