Support several Publishers/Subscribers per-Node on a same topic #29
Conversation
There was a problem hiding this comment.
A few General knowledge questions:
but otherwise LGTM.
Larger, more general discussion regarding project at
eclipse-zenoh/roadmap#107
| // List of DDS Readers declared by 1 Node for a same topic | ||
| // Issue #27: usually only 1 Reader, but may happen that 1 Node declares several Subscribers | ||
| // on the same topic. In this case `ros2 node info <node_id>` still shows only 1 Subscriber, | ||
| // but all the writers can be seen with `ros2 topic info <topic_name> -v`. | ||
| // Hence the choice here to aggregate all the Readers in 1 Subscriber representation. | ||
| // The Subscriber is declared undiscovered only when all its Readers are undiscovered. |
There was a problem hiding this comment.
General question,
What are the differences between Readers and Subscribers In this text ?
Are Subscribers a Zenoh concept, and Readers a ROS2 concept ?
There was a problem hiding this comment.
Answering on behalf of @JEnoch . Here Readers are related to DDS while Subscribers are related to Zenoh.
There was a problem hiding this comment.
To be more precise, in the context of this code:
- Reader means DDS DataReader
- Subscriber means ROS 2 Subscriber, which happens to map in this plugin to a Zenoh Subscriber.
| // List of DDS Writers declared by 1 Node for a same topic | ||
| // Issue #27: usually only 1 Writer, but may happen that 1 Node declares several Publishers | ||
| // on the same topic. In this case `ros2 node info <node_id>` still shows only 1 Publisher, | ||
| // but all the writers can be seen with `ros2 topic info <topic_name> -v`. | ||
| // Hence the choice here to aggregate all the Writers in 1 Publisher representation. | ||
| // The Publisher is declared undiscovered only when all its Writers are undiscovered. |
There was a problem hiding this comment.
General question,
What are the differences between Writers and Publishers In this text ?
Are Publishers a Zenoh concept, and Writers a ROS2 concept ?
There was a problem hiding this comment.
Answering on behalf of @JEnoch . Here Writers are related to DDS while Publisher are related to Zenoh.
There was a problem hiding this comment.
To be more precise, in the context of this code:
- Writer means DDS DataWriter
- Publisher means ROS 2 Publisher, which happens to map in this plugin to a Zenoh Publisher.
| return Some(UndiscoveredMsgPub( | ||
| node_fullname, | ||
| self.msg_pub.remove(&name.clone()).unwrap(), | ||
| self.msg_pub.remove(&name).unwrap(), |
There was a problem hiding this comment.
Based on eclipse-zenoh/roadmap#107, unwrap() could be avoided here. However, multiple unwrap() are already present in the code, which should be IMHO removed. I'd would leave that to a separate PR.
There was a problem hiding this comment.
For this specific unwrap(), the panic should never happen here. The name has just been found in self.msg_pub hashmap (see call to find_map()). And this code is not async.
Do you think .expect("cannot happen") better than unwrap() in such case ?
Closing #27