Implement matched event by Barry-Xu-2018 · Pull Request #645 · ros2/rmw_fastrtps

Barry-Xu-2018 · 2022-11-28T12:36:05Z

Implement the feature about ros2/rmw#330

Barry-Xu-2018 · 2022-11-28T12:47:43Z

@gbiggs @sloretz
While I change code, I have a question about below codes

rmw_fastrtps/rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

Lines 191 to 205 in ccea7a3

    
           if (ReturnCode_t::RETCODE_OK == ret_code) { 
        
             eprosima::fastdds::dds::Entity * entity = status_condition.get_entity(); 
        
             eprosima::fastdds::dds::StatusMask changed_statuses = entity->get_status_changes(); 
        
             if (changed_statuses.is_active( 
        
                 rmw_fastrtps_shared_cpp::internal::rmw_event_to_dds_statusmask( 
        
                   event->event_type))) 
        
             { 
        
               active = true; 
        
             } 
        
             if (guard_condition.get_trigger_value()) { 
        
               active = true; 
        
               guard_condition.set_trigger_value(false); 
        
             } 
        
           }

Could you explain what cases need double-check for active ?
I want to modify it (Please refer to my change in rmw_wait.cpp) since 2 RMW events is corresponding to one DDS event for implementation.

ivanpauno

first review

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp

Barry-Xu-2018 · 2022-12-04T02:37:23Z

Updated code based on review comments.
Now remain one comments #645 (comment)

Barry-Xu-2018 · 2023-02-11T09:52:25Z

do rebase.

fujitatomoya

some minor comments need to be resolved.

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp

rmw_fastrtps_shared_cpp/src/custom_subscriber_info.cpp

fujitatomoya

lgtm

fujitatomoya · 2023-03-03T18:40:24Z

@iuhilnehc-ynos can you have another look on this?

iuhilnehc-ynos · 2023-03-06T05:00:24Z

rmw_fastrtps_shared_cpp/src/rmw_wait.cpp

          guard_condition.set_trigger_value(false);
+        } else {
+          switch (event->event_type) {
+            case RMW_EVENT_SUBSCRIPTION_MATCHED:


It's better to add a comment to explain why we add these codes.

Yes. I should add explanation for the change.

iuhilnehc-ynos

I don't know if it's the correct way to use them, but it looks good to me overall.

I just wondered something as below,
What if there is a requirement about adding matched status event based on the DDS feature in the future? Do we need to update the whole related PRs?

I mean if we don't add the one only matched status event, we should not use the name matched status in rmw.

Barry-Xu-2018 · 2023-03-06T07:08:48Z

I just wondered something as below, What if there is a requirement about adding matched status event based on the DDS feature in the future? Do we need to update the whole related PRs?

I mean if we didn't add the matched status event, we should not use the name matched status in rmw.

Current rmw matched status is redefined based on DDS matched status definition for the convenience of user understanding.
If introduce original DDS matched status, maybe we can use dds_matched_status.

iuhilnehc-ynos · 2023-03-06T07:32:20Z

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_publisher_info.hpp

+  eprosima::fastdds::dds::PublicationMatchedStatus matched_status_
+  RCPPUTILS_TSA_GUARDED_BY(on_new_event_m_);
+
+  eprosima::fastdds::dds::PublicationMatchedStatus unmatched_status_


It's weird for me to define a unmatched_status with a structure named PublicationMatchedStatus.
Can we use the structures defined in the rmw directly?

Currently, there are two separate calculations in both on_*_matched and take_event for unmatched_status.
Can we just update the final data in on_*_matched?

Can we use the structures defined in the rmw directly?

Yes. Need not use eprosima::fastdds::dds::PublicationMatchedStatus.

Currently, there are two separate calculations in both on_matched and take_event for unmatched_status.
Can we just update the final data in on_matched?

Yes. I will optimize code.

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp

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

Barry-Xu-2018 · 2023-03-16T09:42:31Z

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:58:38Z

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

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

Barry-Xu-2018 requested review from gbiggs and sloretz as code owners November 28, 2022 12:36

Barry-Xu-2018 marked this pull request as draft November 28, 2022 12:36

ivanpauno requested changes Nov 29, 2022

View reviewed changes

Barry-Xu-2018 mentioned this pull request Dec 15, 2022

Add matched event support ros2/rmw#331

Merged

Barry-Xu-2018 force-pushed the topic-add-matched-unmatched-event-support branch from fc7f082 to 5f9fc62 Compare February 11, 2023 09:51

Barry-Xu-2018 marked this pull request as ready for review February 15, 2023 04:52

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

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

Closed

11 tasks

fujitatomoya requested changes Feb 26, 2023

View reviewed changes

fujitatomoya mentioned this pull request Feb 27, 2023

Implement matched event ros2/rmw_cyclonedds#435

Merged

fujitatomoya approved these changes Mar 3, 2023

View reviewed changes

iuhilnehc-ynos reviewed Mar 6, 2023

View reviewed changes

fujitatomoya reviewed Mar 8, 2023

View reviewed changes

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved

Implement matched event

5f4d1bd

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

Barry-Xu-2018 force-pushed the topic-add-matched-unmatched-event-support branch from afa489b to 5f4d1bd Compare March 16, 2023 09:43

Barry-Xu-2018 changed the title ~~Support matched & unmatched event~~ Support matched event Mar 16, 2023

Barry-Xu-2018 changed the title ~~Support matched event~~ Implement matched event Mar 16, 2023

Remove code on unread matched event

cc293d4

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

mjcarroll approved these changes Mar 22, 2023

View reviewed changes

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

Conversation

Barry-Xu-2018 commented Nov 28, 2022

Uh oh!

Barry-Xu-2018 commented Nov 28, 2022

Uh oh!

ivanpauno left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Barry-Xu-2018 commented Dec 4, 2022

Uh oh!

Barry-Xu-2018 commented Feb 11, 2023

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

fujitatomoya left a comment

Choose a reason for hiding this comment

Uh oh!

fujitatomoya commented Mar 3, 2023

Uh oh!

iuhilnehc-ynos Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Barry-Xu-2018 Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

iuhilnehc-ynos left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Barry-Xu-2018 commented Mar 6, 2023

Uh oh!

iuhilnehc-ynos Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Barry-Xu-2018 Mar 6, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Barry-Xu-2018 commented Mar 16, 2023

Uh oh!

Barry-Xu-2018 commented Mar 16, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

iuhilnehc-ynos left a comment •

edited

Loading