-
Notifications
You must be signed in to change notification settings - Fork 118
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 matched event #645
Implement matched event #645
Conversation
@gbiggs @sloretz rmw_fastrtps/rmw_fastrtps_shared_cpp/src/rmw_wait.cpp Lines 191 to 205 in ccea7a3
Could you explain what cases need double-check for |
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.
first review
rmw_fastrtps_shared_cpp/include/rmw_fastrtps_shared_cpp/custom_subscriber_info.hpp
Outdated
Show resolved
Hide resolved
Updated code based on review comments. |
fc7f082
to
5f9fc62
Compare
do rebase. |
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.
some minor comments need to be resolved.
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_subscriber_info.hpp
Show resolved
Hide resolved
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_subscriber_info.hpp
Outdated
Show resolved
Hide resolved
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
@iuhilnehc-ynos can you have another look on this? |
@@ -201,6 +201,17 @@ __rmw_wait( | |||
if (guard_condition.get_trigger_value()) { | |||
active = true; | |||
guard_condition.set_trigger_value(false); | |||
} else { | |||
switch (event->event_type) { | |||
case RMW_EVENT_SUBSCRIPTION_MATCHED: |
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's better to add a comment to explain why we add these codes.
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.
Yes. I should add explanation for the change.
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.
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
.
Current rmw |
eprosima::fastdds::dds::PublicationMatchedStatus matched_status_ | ||
RCPPUTILS_TSA_GUARDED_BY(on_new_event_m_); | ||
|
||
eprosima::fastdds::dds::PublicationMatchedStatus unmatched_status_ |
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'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
?
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.
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.
Signed-off-by: Barry Xu <barry.xu@sony.com>
Now interface is changed. We only use matched event in rmw layer (ros2/rmw#331). |
afa489b
to
5f4d1bd
Compare
Check failure is because rmw package is old in CI environment. |
Signed-off-by: Barry Xu <barry.xu@sony.com>
Implement the feature about ros2/rmw#330