Connect/Disconnect notification/callback support

[fujitatomoya]

Feature request

Feature description

• RMW can notify the event that any subscriptions connect / disconnect on the topic to each publisher.
• Based on this capability, in rclcpp and rclpy, user application can register user callback that will be called when subscription connects or disconnects on the topic for publisher.

ROS 1 Similar function: https://docs.ros.org/en/noetic/api/roscpp/html/classros_1_1NodeHandle.html#ae4711ef282892176ba145d02f8f45f8d

Use case

On visualization node, to suppress unnecessary CPU utilization, enable its visualization process only when RViz is connected.

Related Information / Reference

• image_pipeline comment out subscription match for temporary
• ROS2 publisher callback on subscription match
• pointcloud_to_laserscan does polling to check on subscription match

Implementation considerations

Since all tier-1 rmw implementation is DDS implementation, we can support this feature with on_publication_matched of DataWriterListener Interface. (we confirmed the code of Fast-DDS.)

• rmw Add matched event support #331
• rmw_implementation Add tests for rmw matched event rmw_implementation#216
• rmw_fastrtps Implement matched event rmw_fastrtps#645
• rmw_connextdds Support matched/unmatched event rmw_connextdds#101
• rmw_cyclonedds Implement matched event rmw_cyclonedds#435
• rcl Implement matched event rcl#1033
• rclcpp Implement matched event rclcpp#2105
• rclpy Implement matched event rclpy#1083
• Add matched event demo for rclcpp and rclpy demos#607
• Topic add matched event description ros2_documentation#3406
• Matched events release note update. ros2_documentation#3420

[fujitatomoya]

@ivanpauno @wjwwood @asorbini @MiguelCompany @alsora @SteveMacenski @mikeferguson
CC: @ros2/middleware_working_group @Barry-Xu-2018

This is to open discussion, could you share your thoughts?

[ivanpauno]

Adding this like a new rmw_event_t seems reasonable to me

[mikeferguson]

I think this is super useful - at the moment, you can't easily build something like rgbd_launch in ROS2 (where all of the various pipeline elements are lazy publishers/subscribers) - I didn't even realize this wasn't already ticketed somewhere...

I had some notes about use cases (and my current lazy subscriber workaround with a timer that checks get_subscription_count once a second) here: https://www.robotandchisel.com/2020/06/16/ubr1-on-ros2/

EDIT: there was also additional information in my post on 5 things ROS2 needed in 2020: https://www.robotandchisel.com/2020/07/29/five-things-ros2/

[alsora]

I'm not familiar with the on_publication_matched API.
Would it be equivalent to checking get_subscription_count() on the publishers objects?
Obviously having a callback would be much better than continuously checking that API, but I just want to make sure that I understand how it would work.

In general this looks a great feature to add.

[fujitatomoya]

Would it be equivalent to checking get_subscription_count() on the publishers objects?

the main difference is, application can be notified with event on connect/disconnect.

[MiguelCompany]

This would be a nice addition, probably as a new rmw_event_t as @ivanpauno suggests. We could interpret it as having an event notifying of changes on get_subscription_count()

We could do the same for subscriptions, though we should probably have that as a separate issue.

[Barry-Xu-2018]

We all agreed adding new event.
I submitted a PR to add connect/disconnect event and define rmw_matched_status_t for publisher and subscription event callback.
Please review it.

[Barry-Xu-2018]

@fujitatomoya

We should remove rmw_implementation in change list since the rmw interface isn't changed in #331

[MiguelCompany]

We should remove rmw_implementation in change list since the rmw interface isn't changed in #331

@Barry-Xu-2018 I don't think so, since that repository is there the tests for this feature should be added.

[Barry-Xu-2018]

@MiguelCompany

I don't think so, since that repository is there the tests for this feature should be added.

Thank you for your kind reminder.
Yes. I will check and add tests.

[Barry-Xu-2018]

In rmw_implementation, there is no test related to event currently (Including existing events).
So I need to add events test file.
Besides, rmw_implementation test depend on rmw_xxxx (e.g. rmw_fastrtps).
So I will implement rmw_fastrtps, rmw_connextdds, rmw_cyclonedds at first and then add test to rmw_implementation.

[fujitatomoya]

@Barry-Xu-2018 thanks for checking, i was thinking that there would be some related code or test cases in there, so just make sure that everything we may change on that table at 1st. if we do not need to update, that is also fine, i will remove rmw_implementaion from the issue header.

[ros-discourse]

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-tsc-meeting-minutes-2023-01-19/29423/1

[Barry-Xu-2018]

Related PRs have been submitted.

• [rmw]
  Add matched event support #331
• [rmw_implementation]
  Add tests for rmw matched event rmw_implementation#216
• [rmw_fastrtps]
  Implement matched event rmw_fastrtps#645
• [rmw_cyclonedds]
  Implement matched event rmw_cyclonedds#435
• [rmw_connextdds]
  Support matched/unmatched event rmw_connextdds#101
• [rcl]
  Implement matched event rcl#1033
• [rclcpp]
  Implement matched event rclcpp#2105
• [rclpy]
  Implement matched event rclpy#1083

TODO

• [Demo]
• [ros2_documentation]

[fujitatomoya]

@Barry-Xu-2018 thanks, i will be reviewing for sure.

CC: @iuhilnehc-ynos

[fujitatomoya]

@Barry-Xu-2018 Can we start the CI with all dependent PRs to see all related tests complete w/o error? i will be working on review at the same time.

[Barry-Xu-2018]

@fujitatomoya

once everything is rebased, can you start the CI?

Yes. After all are rebased, I will ask @iuhilnehc-ynos help to run CI.

[Barry-Xu-2018]

@fujitatomoya

CI was done.
#331 (comment)

The failed test cases on Windows are unrelated to my change.

[mjcarroll]

Everything up to rclcpp/rclpy have been approved and merged. I'm going to leave this to you to close when demos/documentation are updated. Thanks for all the hard work and iteration!

[fujitatomoya]

@mjcarroll appreciate it for taking care of those PRs 👍 thanks a lot 🙇‍♂️ (ありがとう)

[fujitatomoya]

@Barry-Xu-2018

the rest will be #330 (comment), can you also address these?

[Barry-Xu-2018]

the rest will be #330 (comment), can you also address these?

Yeah, I am writing demo code and updating the document.

[Barry-Xu-2018]

Demo PR was submitted. ros2/demos#607

About the document, I didn't find a detailed description of the "event" in the document. Only a brief introduction is provided, and there are no examples given.
Based on this file, I add brief introduction for matched as below. (New added event Inconsistent topic isn't described. I add brief introduction)
Barry-Xu-2018/ros2_documentation@c183c93

[fujitatomoya]

Based on this file, I add brief introduction for matched as below. (New added event Inconsistent topic isn't described. I add brief introduction)

I think that would be better to add an introduction like this, i left some comments on it, can you make the PR?

and, probably doc for inconsistent topic, we would want to create another PR.

[Barry-Xu-2018]

I think that would be better to add an introduction like this, i left some comments on it, can you make the PR?

and, probably doc for inconsistent topic, we would want to create another PR.

Okay. I created a PR ros2/ros2_documentation#3406.

[fujitatomoya]

ros2/rmw_fastrtps#683 has been merged.

[fujitatomoya]

complete all related PRs, see https://github.com/ros2/rmw/issues/330#issue-1371797574#353

i will go ahead to close this issue.

[ros-discourse]

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/ros-2-alternative-middleware-report/33771/19

[Ryanf55]

Hello,

This is a great feature for testing in ROS 2 - you can verify all topics are matched before starting tests.

1. Is this feature possible to backport to humble?
2. If not, is there any other approach to verify all topics are matched before starting a test when using rclpy?

Thanks!

[fujitatomoya]

@Ryanf55

Is this feature possible to backport to humble?

unfortunately I do not think so, originally we did not consider the backport of this feature. this adds many APIs including RMW interfaces.

If not, is there any other approach to verify all topics are matched before starting a test when using rclpy?

we can check if corresponding publishers or subscribers on the topic, which uses NodeGraph.

https://github.com/ros2/rclpy/blob/f1873bfb1e3261e617cb0a4c9cfb6ddcf57b6747/rclpy/rclpy/node.py#L2124-L2152

i think this could be used to check if someone is on the other side, but downside is that this is not an event, so we need to do polling to ask if there is someone on the topic.

Note: with rclcpp we can have an event from NodeGraph guard condition update, but this does not tell you what exactly has been updated. after all, we need to check how graph is updated from application.

that is pretty much that i know, hopefully this helps.

[tonynajjar]

This is a really cool feature but it feels quite verbose for the very common use case: "publish only if there are subscribers". For that I still prefer something as simple as:

  if (publisher_->get_subscription_count() > 0) {
    /* heavy work for visualization
    */
    publisher_->publish(msg);
  }

It was mentioned that the drawback with this approach is the need to constantly poll with an external timer but in many cases this block of code would already be in a timer/subscriber callback (you can see many examples in Nav2). So apart from this drawback, is there anything else matched events are better with? Thanks

[clalancette]

  if (publisher_->get_subscription_count() > 0) {

For what it is worth, that already exists, though with a caveat. The caveat is that because of intra-process publishing, just getting the subscription count isn't enough; you also need to get the intra-process subscription count. With that in mind, you can do:

if ((publisher_->get_subscription_count() + publisher_->get_intra_process_subscription_count()) > 0) {
  publisher_->publish();
}

[tonynajjar]

Yes I know it exists, thanks for mentioning the caveat though. I was just asking if this method has any other known drawbacks compared to matched events + counting subscribers manually (which would be a much more verbose solution for this simple yet very common use case)

[Barry-Xu-2018]

For different use cases, you can choose different methods.
For instance, if you need to perform some actions when certain events occur (e.g. the first subscriber connects or the last one disconnects), using event-driven code would be simpler and more efficient compared to using timers and polling for checks.

[fujitatomoya]

@tonynajjar the original motivation is, we need to create the data pipeline dynamically only if it comes necessary for network and cpu efficiency as we do develop edge devices. having polling mechanism and calling API to query the network connectivity is not really cost effective cz we have to do that every single time and most likely the network connectivity has not changed yet... i think it all comes down to the use case. (we sometimes use get_count as well.)

something i want to mention about the drawback (maybe not drawback but more like feature) as comparison is, that the matched event is synchronized information, that guarantees how the mached endpoits changes.

for example, publisher online -> get_sub_count called and 0 returns -> subscriber online -> subscriber offline -> get_sub_count called and 0 returns, this case it appears that nothing has happened in the network connectivity. but it actually did. the same thing could happen that publisher can see the same 1 count of subscription, but this subscription could be a different one in between get_count API calls. with matched events, we can see those changes for sure, so that could be one of the difference too.

@Barry-Xu-2018 please correct me if i got anything wrong above.

[tonynajjar]

I see your point about not missing any information in between API calls. Thanks everyone for the clarifications, hopefully that helps more people in the future.