-
Notifications
You must be signed in to change notification settings - Fork 117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement inconsistent topic event #654
Conversation
72bc62d
to
b6d99ba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@MiguelCompany @iuhilnehc-ynos can you take a look at this ? |
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp
Outdated
Show resolved
Hide resolved
b6d99ba
to
5c5490a
Compare
5c5490a
to
4dc8490
Compare
@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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No blocking comments, just a bunch of potential clarity changes.
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp
Outdated
Show resolved
Hide resolved
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_event_info.hpp
Show resolved
Hide resolved
{ | ||
return publisher_info_->data_writer_->get_statuscondition(); | ||
} | ||
|
||
bool PubListener::take_event( | ||
bool RMWPublisherEvent::take_event( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@clalancette I won't have time till next week, but I promise to give you a review |
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_participant_info.hpp
Outdated
Show resolved
Hide resolved
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
Show resolved
Hide resolved
Sorry for the late review and it's not a blocking comment/question as well. Can we add a new struct derived from something might be done as below, add overloaded functions for so we can get the following benefit
|
This reduces the amount of duplicated code. No functional change. Signed-off-by: Chris Lalancette <[email protected]>
This just reduces the duplicated code. Signed-off-by: Chris Lalancette <[email protected]>
This reduces the amount of duplicated code. Signed-off-by: Chris Lalancette <[email protected]>
This reduces the duplicated code. Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
To make Windows happy. Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
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 <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
@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. |
8e73176
to
e9d5961
Compare
And I've opened up #675 to track the improvement. |
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