Add matched event demo for rclcpp and rclpy #607

Barry-Xu-2018 · 2023-03-24T07:21:52Z

Signed-off-by: Barry Xu <barry.xu@sony.com>

iuhilnehc-ynos

a new test is required in https://github.com/ros2/demos/tree/9fbd3e840113e183b7ccc935e17fa275fdea6540/demo_nodes_cpp/test

iuhilnehc-ynos · 2023-03-24T08:04:51Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "rcl/event.h"


I think we should not use rcl header file in this application

I think it depends on implementation, but in this demo, we do not need to include this file i think.

iuhilnehc-ynos · 2023-03-24T08:05:14Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+  {
+    rclcpp::PublisherOptions pub_options;
+    pub_options.event_callbacks.matched_callback =
+      [this](rmw_matched_status_t & s) {


use rclcpp::MatchedInfo instead?

Barry-Xu-2018 · 2023-03-24T08:41:01Z

README.md need to be updated.
I will update it.

Barry-Xu-2018 · 2023-03-24T08:57:49Z

a new test is required in https://github.com/ros2/demos/tree/9fbd3e840113e183b7ccc935e17fa275fdea6540/demo_nodes_cpp/test

matched_event_detect like 'service'. What output depend on 'client' behavior.
This PR doesn't provide 'client'.
I want to use 'ros2 topic echo' and 'ros2 topic pub' command as 'client'.
Not sure whether current test framework can support it or not.

iuhilnehc-ynos · 2023-03-24T09:21:22Z

matched_event_detect like 'service'. What output depend on 'client' behavior.
This PR doesn't provide 'client'.
I want to use 'ros2 topic echo' and 'ros2 topic pub' command as 'client'.
Not sure whether current test framework can support it or not.

I see. Let's hear others' opinions.

fujitatomoya

implementation looks good to me, but some comments need to be addressed.

fujitatomoya · 2023-03-24T16:49:47Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#include "rcl/event.h"


I think it depends on implementation, but in this demo, we do not need to include this file i think.

fujitatomoya · 2023-03-24T16:56:21Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+  bool connect_subscription_{false};
+  bool connect_publisher_{false};


Suggested change

bool connect_subscription_{false};

bool connect_publisher_{false};

bool any_subscription_connected_{false};

bool any_publisher_connected_{false};

fujitatomoya · 2023-03-24T16:59:56Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+          } else {
+            RCLCPP_INFO(
+              this->get_logger(),
+              "Current number of connected publisher is %lu", s.current_count);


How about adding current_change to show the user application can see difference? I think total_xxx would be overkill.

fujitatomoya · 2023-03-24T17:00:31Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+namespace demo_nodes_cpp
+{
+
+// This demo program shows detected matched event.


Suggested change

// This demo program shows detected matched event.

// This demo program shows matched event works.

fujitatomoya · 2023-03-24T17:01:25Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+{
+
+// This demo program shows detected matched event.
+// Matched event occur while connection between publisher and subscription is done.


Suggested change

// Matched event occur while connection between publisher and subscription is done.

// Matched event occurs when publisher and subscription establishes the connection.

fujitatomoya · 2023-03-24T17:03:06Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+// For checking matched event of publisher
+// On another console, run below command
+// $ ros2 topic echo /pub_matched_event_detect
+// You can run above command many times on different consoles to check the number of connected
+// subscription by the output of demo program.


I think that it would be better to show the complete demonstration by itself. that said, what about having the client endpoint to fire the matched event callback in the demo code, instead of ros2cli?

Okay. I will update code to remove dependency of ros2cli.

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2023-03-25T12:26:50Z

matched_event_detect like 'service'. What output depend on 'client' behavior.
This PR doesn't provide 'client'.
I want to use 'ros2 topic echo' and 'ros2 topic pub' command as 'client'.
Not sure whether current test framework can support it or not.

I see. Let's hear others' opinions.

I change demo codes to remove dependency of roscli. So now I can add test. @iuhilnehc-ynos

fujitatomoya · 2023-03-26T00:11:34Z

lgtm, @iuhilnehc-ynos can you do another review on this? if it is good to go, please start the CI.

iuhilnehc-ynos

I just left some comments from my point of view.

iuhilnehc-ynos · 2023-03-26T11:05:12Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+class MatchedEventDetectNode : public rclcpp::Node
+{
+public:
+  DEMO_NODES_CPP_PUBLIC explicit MatchedEventDetectNode(


If using the main in this file, which means this file will not be built into a library, I think it's unnecessary to add DEMO_NODES_CPP_PUBLIC.

iuhilnehc-ynos · 2023-03-26T11:06:42Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+
+#include "std_msgs/msg/string.hpp"
+
+#include "demo_nodes_cpp/visibility_control.h"


Please remove it if not to build the following classes into a library.

Yes. Should remove it.
Thanks.

iuhilnehc-ynos · 2023-03-26T11:12:29Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+      topic_name_,
+      10,
+      [](std_msgs::msg::String::ConstSharedPtr) {});
+    subs_.emplace_back(sub);


It's better to output an info message that there is a subscription coming, which will make the whole output a little more clear. (destroy_one_sub as well)

iuhilnehc-ynos · 2023-03-26T11:16:32Z

demo_nodes_cpp/src/events/matched_event_detect.cpp

+    subs_.emplace_back(sub);
+  }
+
+  DEMO_NODES_CPP_PUBLIC void destroy_one_sub(void)


Even if it's a demo that we can control the whole process, I think we should design a better behavior in this method. It's just a suggestion,

rclcpp::Subscription<std_msgs::msg::String>::WeakPtr create_one_sub(void); void destroy_one_sub(rclcpp::Subscription<std_msgs::msg::String>::WeakPtr sub);

Do you means test as the below

auto sub1 = multi_sub_node->create_one_sub(); executor.spin_some(200ms); auto sub2 = multi_sub_node->create_one_sub(); executor.spin_some(200ms); multi_sub_node->destroy_one_sub(sub1); executor.spin_some(200ms); multi_sub_node->destroy_one_sub(sub2); executor.spin_some(200ms);

iuhilnehc-ynos · 2023-03-26T11:30:33Z

demo_nodes_py/demo_nodes_py/events/matched_event_detect.py

+'''
+
+
+class MatchedEventDetectNode(Node):


The behavior for the matched event detect in demo_nodes_cpp is different from demo_nodes_py, which might seem not good.

Okay. I use the same logic as demo_nodes_cpp.

iuhilnehc-ynos · 2023-03-27T02:41:02Z

demo_nodes_cpp/test/matched_event_detect.txt

+The changed number of connected subscription is -1 and current number of connected subscription is 1.
+Last subscription is disconnected.
+First publisher is connected.
+The changed number of connected publisher is 1 and  Current number of connected publisher is 2.


Suggested change

The changed number of connected publisher is 1 and Current number of connected publisher is 2.

The changed number of connected publisher is 1 and current number of connected publisher is 2.

please update README.md and matched_event_detect.cpp as well.

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2023-03-27T06:25:32Z

I updated your comments @fujitatomoya @iuhilnehc-ynos
Please check again

iuhilnehc-ynos · 2023-03-27T06:33:26Z

demo_nodes_py/demo_nodes_py/events/matched_event_detect.py

+            self.destroy_subscription(sub)
+
+    def __sub_callback(self, msg):
+        True


Generally, I think we will use pass instead of True here.
a better way to use an empty callback for the subscription might use

sub = self.create_subscription(String, self.__topic_name, lambda msg: ..., 10)

and then remove __sub_callback. (please update other places as well)

After the above update, I'll trigger the CI.

Yes. Revised it.

Signed-off-by: Barry Xu <barry.xu@sony.com>

iuhilnehc-ynos · 2023-03-27T06:57:58Z

CI:

Linux
Linux-aarch64
Windows

fujitatomoya · 2023-03-27T18:15:40Z

@Barry-Xu-2018 test with connextdds is failing, could you check the failure?

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2023-03-28T03:01:41Z

I cannot reproduce this problem in my local environment.
I guess waiting time is too short for test.
So I extend waiting time.
Please help to run CI again @iuhilnehc-ynos

iuhilnehc-ynos · 2023-03-28T03:02:05Z

CI:

Linux
Linux-aarch64
Windows

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2023-03-28T05:54:10Z

@iuhilnehc-ynos Help to run CI again.

iuhilnehc-ynos · 2023-03-28T05:57:30Z

CI:

Linux
Linux-aarch64
Windows

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 · 2023-03-28T08:35:22Z

Update to codes to ensure the program does spin until the matched event occurs.

Barry-Xu-2018 · 2023-03-28T08:35:58Z

@iuhilnehc-ynos Please help to run CI again

iuhilnehc-ynos · 2023-03-28T08:38:29Z

@iuhilnehc-ynos Please help to run CI again

CI:

Linux
Linux-aarch64
Windows

Barry-Xu-2018 · 2023-03-29T02:31:29Z

CI failure is related to a bug in Fastdds.
eProsima/Fast-DDS#3395
The fix was submitted. eProsima/Fast-DDS#3396
We have to wait for this patch merged.

fujitatomoya · 2023-03-29T18:18:49Z

CI failure is related to a bug in Fastdds. eProsima/Fast-DDS#3395 The fix was submitted. eProsima/Fast-DDS#3396 We have to wait for this patch merged.

that makes sense, to avoid double triggered, update_publication_matched_status does only updating the cached publication matched event only.

iuhilnehc-ynos · 2023-03-30T06:22:37Z

CI:

Linux
Linux-aarch64
Windows

Barry-Xu-2018 · 2023-03-30T09:36:18Z

CI for Linux didn't run.
Please help to run Linux again @iuhilnehc-ynos

iuhilnehc-ynos · 2023-03-30T09:38:04Z

CI for Linux didn't run. Please help to run Linux again @iuhilnehc-ynos

CI is busy building some nighty jobs. Please be patient.

fujitatomoya · 2023-03-31T04:14:13Z

@clalancette @audrow can you do us a favor? we do not have merge permission to this repo.

after either of you merged this PR, i will merge the following accordingly. (or if you can do that, that would be nice!!!)

iuhilnehc-ynos · 2023-03-31T13:23:36Z

Based on eProsima/Fast-DDS#3395 (comment), we found Fast-DDS is updated again, just in case, I need to re-trigger CI:

Linux
Linux-aarch64
Windows

fujitatomoya · 2023-03-31T15:41:54Z

it looks like eProsima/Fast-DDS#3418 brings CI failure once again?

Barry-Xu-2018 · 2023-04-01T03:16:03Z

it looks like eProsima/Fast-DDS#3418 brings CI failure once again?
Yes. @MiguelCompany explain this change in eProsima/Fast-DDS#3395 (comment).

I will consider how to resolve it.

Barry-Xu-2018 · 2023-04-04T02:32:51Z

Now depend on the fix issue ros2/rmw_fastrtps#681

fujitatomoya · 2023-04-12T19:40:26Z

CI:

Linux
Linux-aarch64
Windows

iuhilnehc-ynos · 2023-04-13T01:34:59Z

As ros2 repos changed a lot, I need to re-run CI with a new custom repos file.

CI:

Linux
Linux-aarch64
Windows

fujitatomoya · 2023-04-13T03:53:38Z

@clalancette @mjcarroll can you merge this to rolling?

Add matched event demo for rclcpp and rclpy

fbe800e

Signed-off-by: Barry Xu <barry.xu@sony.com>

Barry-Xu-2018 requested review from adityapande-1995, audrow and mjeronimo as code owners March 24, 2023 07:21

Barry-Xu-2018 mentioned this pull request Mar 24, 2023

Connect/Disconnect notification/callback support ros2/rmw#330

Closed

11 tasks

iuhilnehc-ynos reviewed Mar 24, 2023

View reviewed changes

fujitatomoya reviewed Mar 24, 2023

View reviewed changes

Address comments

ea480d8

Signed-off-by: Barry Xu <barry.xu@sony.com>

fujitatomoya approved these changes Mar 26, 2023

View reviewed changes

iuhilnehc-ynos suggested changes Mar 26, 2023

View reviewed changes

iuhilnehc-ynos reviewed Mar 27, 2023

View reviewed changes

Barry-Xu-2018 added 2 commits March 27, 2023 11:56

Address review comments for cpp

334a2ca

Signed-off-by: Barry Xu <barry.xu@sony.com>

Update demo codes for python

b93de93

Signed-off-by: Barry Xu <barry.xu@sony.com>

iuhilnehc-ynos reviewed Mar 27, 2023

View reviewed changes

Fix python codes

47d293f

Signed-off-by: Barry Xu <barry.xu@sony.com>

iuhilnehc-ynos approved these changes Mar 27, 2023

View reviewed changes

Extend waiting time for test

9a9aa4e

Signed-off-by: Barry Xu <barry.xu@sony.com>

Use spin_all to replace spin_some

1f97ec7

Signed-off-by: Barry Xu <barry.xu@sony.com>

Make sure executor spin till matched event occur

21b3bf1

Signed-off-by: Barry Xu <barry.xu@sony.com>

fujitatomoya mentioned this pull request Mar 29, 2023

Fix repeated matched event notification eProsima/Fast-DDS#3396

Merged

13 tasks

fujitatomoya mentioned this pull request Mar 29, 2023

Matched events release note update. ros2/ros2_documentation#3420

Merged

methylDragon merged commit a2fb913 into ros2:rolling Apr 13, 2023

		bool connect_subscription_{false};
		bool connect_publisher_{false};

	// This demo program shows detected matched event.
	// This demo program shows matched event works.

	// Matched event occur while connection between publisher and subscription is done.
	// Matched event occurs when publisher and subscription establishes the connection.


		#include "std_msgs/msg/string.hpp"

		#include "demo_nodes_cpp/visibility_control.h"

	The changed number of connected publisher is 1 and Current number of connected publisher is 2.
	The changed number of connected publisher is 1 and current number of connected publisher is 2.

		'''


		class MatchedEventDetectNode(Node):

Add matched event demo for rclcpp and rclpy #607

Add matched event demo for rclcpp and rclpy #607

Conversation

Barry-Xu-2018 commented Mar 24, 2023

iuhilnehc-ynos left a comment • edited Loading

Choose a reason for hiding this comment

iuhilnehc-ynos Mar 24, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Mar 24, 2023

Barry-Xu-2018 commented Mar 24, 2023

iuhilnehc-ynos commented Mar 24, 2023

fujitatomoya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Mar 25, 2023

fujitatomoya commented Mar 26, 2023

iuhilnehc-ynos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Barry-Xu-2018 commented Mar 27, 2023 • edited Loading

iuhilnehc-ynos Mar 27, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iuhilnehc-ynos commented Mar 27, 2023

fujitatomoya commented Mar 27, 2023

Barry-Xu-2018 commented Mar 28, 2023

iuhilnehc-ynos commented Mar 28, 2023

Barry-Xu-2018 commented Mar 28, 2023

iuhilnehc-ynos commented Mar 28, 2023

Barry-Xu-2018 commented Mar 28, 2023

Barry-Xu-2018 commented Mar 28, 2023

iuhilnehc-ynos commented Mar 28, 2023 • edited Loading

Barry-Xu-2018 commented Mar 29, 2023

fujitatomoya commented Mar 29, 2023

iuhilnehc-ynos commented Mar 30, 2023

Barry-Xu-2018 commented Mar 30, 2023

iuhilnehc-ynos commented Mar 30, 2023

fujitatomoya commented Mar 31, 2023

iuhilnehc-ynos commented Mar 31, 2023

fujitatomoya commented Mar 31, 2023

Barry-Xu-2018 commented Apr 1, 2023

Barry-Xu-2018 commented Apr 4, 2023

fujitatomoya commented Apr 12, 2023

iuhilnehc-ynos commented Apr 13, 2023

fujitatomoya commented Apr 13, 2023

iuhilnehc-ynos left a comment •

edited

Loading

iuhilnehc-ynos Mar 24, 2023 •

edited

Loading

Barry-Xu-2018 commented Mar 27, 2023 •

edited

Loading

iuhilnehc-ynos Mar 27, 2023 •

edited

Loading

iuhilnehc-ynos commented Mar 28, 2023 •

edited

Loading