Implement inconsistent topic event #654

clalancette · 2022-12-20T18:21:38Z

This PR implements the inconsistent topic event as part of ros2/ros2#1361 .

Note that as it stands, this is not hooked up inside of Fast-DDS, so this event will never fire. That said, this should be enough to make it work once it is hooked up inside of Fast-DDS.

To make it so that this could be hooked up, a bunch of internal refactoring had to be done. In particular, rmw_fastrtps conflated the events that happen from Fast-DDS (the "listeners" in DDS parlance), with the events that need to be handed to the rmw/rcl layer (the RMW events in ROS 2 parlance). Since inconsistent topic uses a different listener, that had to be broken apart. That's what the first part of this series does, and then the last patch actually does the inconsistent topic implementation.

Part of ros2/ros2#1361

Depends on ros2/rmw#339

fujitatomoya

lgtm

fujitatomoya · 2023-01-22T21:38:42Z

@MiguelCompany @iuhilnehc-ynos can you take a look at this ?

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

rmw_fastrtps_cpp/src/publisher.cpp

rmw_fastrtps_cpp/src/subscription.cpp

rmw_fastrtps_cpp/src/publisher.cpp

clalancette · 2023-02-28T15:47:41Z

@methylDragon @fujitatomoya @MiguelCompany I could use another review on this one, which has undergone significant changes since the last review and #669 . Thanks in advance!

methylDragon

No blocking comments, just a bunch of potential clarity changes.

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp

rmw_fastrtps_shared_cpp/src/custom_participant_info.cpp

rmw_fastrtps_shared_cpp/src/participant.cpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/utils.hpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_event_info.hpp

methylDragon · 2023-02-28T20:07:06Z

rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp

 {
  return publisher_info_->data_writer_->get_statuscondition();
 }

-bool PubListener::take_event(
+bool RMWPublisherEvent::take_event(


It might just be me but I find it confusing that RMWPublisherEvent would be able to take events (I think of the event as a struct of information that can be manipulated, as opposed to having any listening capabilities, which is what seems to be happening in the body of this method.)

I'm wondering if there's a better name for the class. Or it might just be a bad name for the method in the current version of the library, or I might just be confusing myself.

Perhaps update_event?

If changes are made addressing this comment, they should be rolled out to the subscriber and event info too.

So what the RMWPublisherEvent class does is to collect data about events that happened to the publisher, and make them available to the RMW layer. In this new refactor, this class doesn't directly register for any event callbacks; that is done via the CustomDataWriterListener and CustomTopicListener, and when the callbacks fire they call their saved handle to this class to update the data. Then the layers above the RMW get notified of the event, and can come fetch the event via take_event.

So to me, the take_event makes sense; the upper layers are taking the data for this event. The name of the class is more up-in-the-air; it really is a collection of publisher events collected for the RMW layer, hence the current name. But I'm open to naming it something different, if you have a better idea. Let me know what you think.

MiguelCompany · 2023-03-02T15:58:32Z

@clalancette I won't have time till next week, but I promise to give you a review

rmw_fastrtps_cpp/src/rmw_client.cpp

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp

rmw_fastrtps_shared_cpp/src/custom_participant_info.cpp

clalancette · 2023-03-07T18:55:28Z

OK, I think I've addressed all comments except for one here. @methylDragon and @MiguelCompany thanks for the reviews so far. When you get a chance, could you please take another look?

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp

rmw_fastrtps_cpp/src/rmw_client.cpp

iuhilnehc-ynos · 2023-03-09T07:57:14Z

Sorry for the late review and it's not a blocking comment/question as well.

Can we add a new struct derived from eprosima::fastdds::dds::Topic to keep the EventListenerInterface as a member inside, and then use two dynamic_cast operations for checking the new struct and Topic by order in remove_topic_and_type?

something might be done as below,

add overloaded functions for CustomParticipantInfo::find_or_create_topic and CustomParticipantInfo::delete_topic for publisher and subscription

so we can get the following benefit

the client and service can take the original source code if they don't care about the event_listener.
there is no `rmw_fastrtps_shared_cpp::remove_topic_and_type( 'info->publisher_event_ / nullptr' ) with an extra parameter unrelated to the method name called.

This reduces the amount of duplicated code. No functional change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This just reduces the duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This reduces the amount of duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

This reduces the duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

To make Windows happy. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Make it {track,untrack}_unique_{publisher,subscription}. This should make it a little easier to understand. Also add in docstrings to try and clarify the use case. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette · 2023-03-10T14:41:17Z

@iuhilnehc-ynos I partially understand what you are saying, but not completely.

That said, I'm inclined to go with this PR as-is, and leave that for follow-up work. In particular, until Fast-DDS can produce inconsistent topic events, it is somewhat hard to tell exactly how this will all shake out.

So what I'm going to do here is to run one more CI on the main issue, and then merge this as-is. I'll then open an issue for your follow-up thoughts. Thanks for taking a look.

clalancette · 2023-03-13T13:37:29Z

And I've opened up #675 to track the improvement.

clalancette requested review from gbiggs and sloretz as code owners December 20, 2022 18:21

clalancette self-assigned this Jan 5, 2023

clalancette force-pushed the clalancette/inconsistent-topic-event branch from 72bc62d to b6d99ba Compare January 17, 2023 15:55

fujitatomoya approved these changes Jan 22, 2023

View reviewed changes

MiguelCompany suggested changes Jan 23, 2023

View reviewed changes

clalancette force-pushed the clalancette/inconsistent-topic-event branch from b6d99ba to 5c5490a Compare January 24, 2023 18:57

clalancette mentioned this pull request Feb 8, 2023

Rewrite how Topics are tracked in rmw_fastrtps_cpp. #669

Merged

clalancette force-pushed the clalancette/inconsistent-topic-event branch from 5c5490a to 4dc8490 Compare February 24, 2023 21:07

methylDragon approved these changes Feb 28, 2023

View reviewed changes

MiguelCompany suggested changes Mar 6, 2023

View reviewed changes

MiguelCompany suggested changes Mar 8, 2023

View reviewed changes

rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp Show resolved Hide resolved

rmw_fastrtps_cpp/src/rmw_client.cpp Show resolved Hide resolved

MiguelCompany approved these changes Mar 8, 2023

View reviewed changes

clalancette and others added 12 commits March 10, 2023 14:33

Revamp PubListener::take_event.

a5f8fbf

This reduces the amount of duplicated code. No functional change. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Revamp setting status mask in PubListener::set_on_new_event_callback.

5860bb5

This just reduces the duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Revamp SubListener::take_event.

3ec4bdc

This reduces the amount of duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Revamp setting status mask in SubListener::set_on_new_event_callback

ca0fba1

This reduces the duplicated code. Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove unused arguments from rmw_fastrtps_cpp::create_publisher

7d758f8

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Remove unused arguments from rmw_fastrtps_dynamic_cpp::create_publisher

577ba37

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Revamp to split publisher data writer listener from RMW event holder.

59741e9

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Split the DataReaderListener and EventListener.

6935874

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Implemenation of incompatible type.

b82f534

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Rename mutex to publishers_mutex_.

ad3ee30

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>

Mark more things as PUBLIC.

3cd91e3

To make Windows happy. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Small documentation updates.

75f9ac7

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette added 5 commits March 10, 2023 14:33

Fixes for Miguel's review.

f72982f

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Update some documentation from review.

535ab3f

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Another documentation update.

0a8a4f7

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Rename {add,remove}_{publisher,subscription}.

266d6fd

Make it {track,untrack}_unique_{publisher,subscription}. This should make it a little easier to understand. Also add in docstrings to try and clarify the use case. Signed-off-by: Chris Lalancette <clalancette@gmail.com>

Add in RCPPUTILS_TSA_GUARDED_BY.

e9d5961

Signed-off-by: Chris Lalancette <clalancette@gmail.com>

clalancette force-pushed the clalancette/inconsistent-topic-event branch from 8e73176 to e9d5961 Compare March 10, 2023 14:53

clalancette merged commit 926b3a1 into rolling Mar 13, 2023

delete-merged-branch bot deleted the clalancette/inconsistent-topic-event branch March 13, 2023 13:32

clalancette mentioned this pull request Mar 13, 2023

Improve the implementation of incompatible type #675

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement inconsistent topic event #654

Implement inconsistent topic event #654

clalancette commented Dec 20, 2022 •

edited

Loading

fujitatomoya left a comment

fujitatomoya commented Jan 22, 2023

clalancette commented Feb 28, 2023

methylDragon left a comment

methylDragon Feb 28, 2023

clalancette Mar 7, 2023 •

edited

Loading

MiguelCompany commented Mar 2, 2023

clalancette commented Mar 7, 2023

iuhilnehc-ynos commented Mar 9, 2023

clalancette commented Mar 10, 2023

clalancette commented Mar 13, 2023

Implement inconsistent topic event #654

Implement inconsistent topic event #654

Conversation

clalancette commented Dec 20, 2022 • edited Loading

fujitatomoya left a comment

Choose a reason for hiding this comment

fujitatomoya commented Jan 22, 2023

clalancette commented Feb 28, 2023

methylDragon left a comment

Choose a reason for hiding this comment

methylDragon Feb 28, 2023

Choose a reason for hiding this comment

clalancette Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

MiguelCompany commented Mar 2, 2023

clalancette commented Mar 7, 2023

iuhilnehc-ynos commented Mar 9, 2023

clalancette commented Mar 10, 2023

clalancette commented Mar 13, 2023

clalancette commented Dec 20, 2022 •

edited

Loading

clalancette Mar 7, 2023 •

edited

Loading