Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix routing loop when 2 Subscribers for a same topic exist across 2 bridges #41

Merged
merged 3 commits into from
Dec 28, 2023

Conversation

JEnoch
Copy link
Member

@JEnoch JEnoch commented Dec 28, 2023

In a setup with 2 interconnected bridges, with for instance one on domain 0 and the other one on domain 1:
if in domain 0 there are a Publisher and a Subscriber on a same topic T and in domain 1 a Subscriber on topic T, then the Subscriber in domain 1 receives twice each publication.

E.g. to reproduce on a single host:

  • ROS_DOMAIN_ID=0 ros2 run demo_nodes_cpp talker
  • ROS_DOMAIN_ID=0 ros2 run demo_nodes_cpp listener
  • ROS_DOMAIN_ID=0 zenoh-bridge-ros2dds
  • ROS_DOMAIN_ID=1 zenoh-bridge-ros2dds
  • ROS_DOMAIN_ID=1 ros2 run demo_nodes_cpp listener

The problem comes from bridge on domain 1 that re-publishes to DDS the data coming from bridge on domain 0, but also receives back this publication and route it back to the bridge on domain 0.
The cause is the DDS Writers (and DDS Readers) created by the bridge on a remote announcement of route copy the same declared QoS without adding the ignore_local(Participant) QoS. Adding this QoS will make the bridge to ignore its own publications over DDS, avoiding to route them back.

This PR fixes that.

@JEnoch
Copy link
Member Author

JEnoch commented Dec 28, 2023

Note: the 2nd commit adds some cosmetic improvements in logs and comments

Copy link
Member

@Charles-Schleich Charles-Schleich left a comment

Choose a reason for hiding this comment

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

Small nit:
do_route_message is an improvement over do_route_data
Not urgent or majorly important but clarity of direction of do_route_message functions could be improved.

Suggestion:
For route_subscriber::do_route_message and
route_publisher::do_route_message

It could improve clarity where as to direction to name the two functions differently.
But i suppose because they are in differently named source files, it should be fine.

Otherwise LGTM

@JEnoch
Copy link
Member Author

JEnoch commented Dec 28, 2023

Agreed. I'm renamed as such for clarity:

  • route_subscriber::do_route_message => route_subscriber::route_zenoh_message_to_dds
  • route_publisher::do_route_message => route_publisher::route_dds_message_to_zenoh

@JEnoch JEnoch merged commit 270f023 into main Dec 28, 2023
7 checks passed
@JEnoch JEnoch deleted the fix_route_loop branch December 29, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants