Implement matched event #435

Barry-Xu-2018 · 2023-02-15T04:55:23Z

fujitatomoya · 2023-02-26T23:53:41Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+  rmw_event_callback_t event_callback[RMW_EVENT_INVALID + 1] {nullptr};
+  const void * event_data[RMW_EVENT_INVALID + 1] {nullptr};
+  size_t event_unread_count[RMW_EVENT_INVALID + 1] {0};


these changes are required because there are two different events for matched and unmatched, right? (ros2/rmw_fastrtps#645 (comment))
I understand that different events can avoid user to do the further work to see which event comes in, but also we are providing the almost same event in the implementation...

these changes are required because there are two different events for matched and unmatched, right? (ros2/rmw_fastrtps#645 (comment))

Yes. Original code is based on DDS_STATUS_ID which only matched event.
So in order to support unmatched event, I changed it to use RMW_EVENT ID.

I understand that different events can avoid user to do the further work to see which event comes in, but also we are providing the almost same event in the implementation...

rmw_take_event() will take event status from last read.
Now we use 2 RMW events for one DDS event, so we have to save the status of DDS event for each RMW event.

I discussed with Chen-san. Now we only know one real case. While first matched event (for subscription or publisher) happen, start certain service. While the last subscription or publisher is disconnected, stop certain service.

For this case, user will do based on matched/unmatched event

While RMW_EVENT_PUBLICATION_MATCHED/RMW_EVENT_SUBSCRIPTION_MATCHED is triggered, user check service is started. If not, start this service. Or call rmw_take_event() to check if current_matched_count is 1 and start service if count is 1.

While RMW_EVENT_PUBLICATION_UNMATCHED/RMW_EVENT_SUBSCRIPTION_UNMATCHED is triggered, user need to call rmw_take_event() to check if current_matched_count is 0. If count is 0, stop service.

If only one matched event (Same as DDS), user do the same thing as above.
Using 2 events doesn't bring convenience for user.

If user only care matched and unmatched event, current 2 events implementation can bring convenience.
2 events implementation introduce extra logic to rmw layer. I think ros2/rmw_fastrtps#645 (comment) don't want to have extra logic in rmw layer.
I have no enough requirement from real cases. Which implementation (2 events or 1 event) is better one.

I have no enough requirement from real cases.

That makes two of us ...

I would even go further: if the only foreseen use is starting/stopping a service, you could also simply do that based on a single event that merely says something changed in the set of matched readers. You use that event as a trigger, and then check that discovery information/graph cache to see whether you just went from 0 to 1 or from 1 to 0.

At least in this RMW layer, a quick reading of the changes suggests the code (almost) duplication could be eliminated by adding a few functions with well-chosen parameters.

@eboasson thanks for the comment!
CC: @Barry-Xu-2018

yeah, that is correct. i mean, we can just generate the event that tells us something happened on either publisher or subscription, then we can internally calls graph interfaces to return the number of endpoints. I would think most likely that works for many use cases.

one thing that i want to hear is there will be time window between triggering event from implementation and getting the graph information. these cannot be synced, so there might be event but it does not show any difference the graph change? this cannot be supported in current interfaces w/o this one?

these cannot be synced, so there might be event but it does not show any difference the graph change? this cannot be supported in current interfaces w/o this one?

I would think so. If you need that information, then you do need something like this.

fujitatomoya · 2023-02-26T23:56:30Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

  RMW_CHECK_ARGUMENT_FOR_NULL(rmw_event, RMW_RET_INVALID_ARGUMENT);
+


unrelated change should be avoided?

Yes. Will remove.

fujitatomoya · 2023-02-27T00:08:20Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+static inline rmw_event_type_t dds_event_to_rmw_event(
+  dds_status_id_t rmw_event,
+  const void * status,
+  user_callback_data_t * data)


is this actually needed as argument?

Not used.
This is residual code.

fujitatomoya · 2023-02-27T01:22:19Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+
+  if (callback && data->event_unread_count[rmw_event->event_type]) {
+    // Push events happened before having assigned a callback
+    callback(user_data, data->event_unread_count[rmw_event->event_type]);


This will notify the user application matched/unmatched event that happened before? I am not sure if this is what user expects? probably user application is not interested in the events in the past for this case?

at least, we would want to have the same behavior for rmw implementation.

Yes.
But for fastrtps, only matched/unmatched event don't notify previoius status. Other event will do.
So for keeping the same behavior, we have to deal with matched/unmatched individually.

Need to add below codes only for matched/unmatched event

switch (rmw_event->event_type) { case RMW_EVENT_SUBSCRIPTION_MATCHED: data->event_unread_count[RMW_EVENT_SUBSCRIPTION_MATCHED] = 0; break; case RMW_EVENT_SUBSCRIPTION_UNMATCHED: data->event_unread_count[RMW_EVENT_SUBSCRIPTION_UNMATCHED] = 0; break; case RMW_EVENT_PUBLICATION_MATCHED: data->event_unread_count[RMW_EVENT_PUBLICATION_MATCHED] = 0; break; case RMW_EVENT_PUBLICATION_UNMATCHED: data->event_unread_count[RMW_EVENT_PUBLICATION_UNMATCHED] = 0; break; default: break; }

fujitatomoya · 2023-02-27T01:35:12Z

@eboasson we would like to request the review on this.

this is related to new feature, ros2/rmw#330 (comment)

eboasson

I think this'll do what it says on the tin 🙂

eboasson · 2023-03-01T11:56:28Z

rmw_cyclonedds_cpp/src/rmw_node.cpp

+  rmw_event_callback_t event_callback[RMW_EVENT_INVALID + 1] {nullptr};
+  const void * event_data[RMW_EVENT_INVALID + 1] {nullptr};
+  size_t event_unread_count[RMW_EVENT_INVALID + 1] {0};


I have no enough requirement from real cases.

That makes two of us ...

I would even go further: if the only foreseen use is starting/stopping a service, you could also simply do that based on a single event that merely says something changed in the set of matched readers. You use that event as a trigger, and then check that discovery information/graph cache to see whether you just went from 0 to 1 or from 1 to 0.

At least in this RMW layer, a quick reading of the changes suggests the code (almost) duplication could be eliminated by adding a few functions with well-chosen parameters.

fujitatomoya · 2023-03-03T18:43:19Z

lgtm on my side.

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

mjcarroll · 2023-03-15T13:35:26Z

@Barry-Xu-2018 this has some conflicts now

Barry-Xu-2018 · 2023-03-16T09:47:23Z

Now interface is changed. We only use matched event in rmw layer (ros2/rmw#331).
So code changes are quite significant.
Besides, merged implementation of inconsistent topic leads to many conflicts with old commits while doing rebase.
So I will merge all commits in this PR to one commit and then do rebase.

Barry-Xu-2018 · 2023-03-16T09:57:47Z

Check failure is because rmw package is old in CI environment.

rmw_cyclonedds_cpp/src/rmw_node.cpp

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

Barry-Xu-2018 requested review from eboasson and ivanpauno as code owners February 15, 2023 04:55

Barry-Xu-2018 mentioned this pull request Feb 15, 2023

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

Closed

11 tasks

Barry-Xu-2018 force-pushed the topic-add-match-unmatch-event-support branch from 984ed63 to 13eeda5 Compare February 15, 2023 06:28

fujitatomoya reviewed Feb 27, 2023

View reviewed changes

eboasson reviewed Mar 1, 2023

View reviewed changes

Support matched event

df5e2f0

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

Barry-Xu-2018 force-pushed the topic-add-match-unmatch-event-support branch from 054bca7 to df5e2f0 Compare March 16, 2023 09:48

Barry-Xu-2018 changed the title ~~Add matched/unmatched event support~~ Implement matched event Mar 16, 2023

fujitatomoya mentioned this pull request Mar 18, 2023

Add matched event support ros2/rmw#331

Merged

iuhilnehc-ynos reviewed Mar 19, 2023

View reviewed changes

rmw_cyclonedds_cpp/src/rmw_node.cpp Outdated Show resolved Hide resolved

Fix wrong static_cast

7ef3848

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

mjcarroll approved these changes Mar 20, 2023

View reviewed changes

mjcarroll merged commit f6059c3 into ros2:rolling Mar 22, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement matched event #435

Implement matched event #435

Barry-Xu-2018 commented Feb 15, 2023

fujitatomoya Feb 26, 2023

Barry-Xu-2018 Feb 27, 2023

eboasson Mar 1, 2023

fujitatomoya Mar 1, 2023

eboasson Mar 2, 2023

fujitatomoya Feb 26, 2023

Barry-Xu-2018 Feb 27, 2023

fujitatomoya Feb 27, 2023

Barry-Xu-2018 Feb 27, 2023

fujitatomoya Feb 27, 2023

Barry-Xu-2018 Feb 27, 2023

Barry-Xu-2018 Feb 28, 2023

fujitatomoya commented Feb 27, 2023

eboasson left a comment

eboasson Mar 1, 2023

fujitatomoya commented Mar 3, 2023

mjcarroll commented Mar 15, 2023

Barry-Xu-2018 commented Mar 16, 2023

Barry-Xu-2018 commented Mar 16, 2023

		RMW_CHECK_ARGUMENT_FOR_NULL(rmw_event, RMW_RET_INVALID_ARGUMENT);

Implement matched event #435

Implement matched event #435

Conversation

Barry-Xu-2018 commented Feb 15, 2023

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

fujitatomoya commented Feb 27, 2023

eboasson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fujitatomoya commented Mar 3, 2023

mjcarroll commented Mar 15, 2023

Barry-Xu-2018 commented Mar 16, 2023

Barry-Xu-2018 commented Mar 16, 2023