Skip to content
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

Merged

Conversation

Barry-Xu-2018
Copy link
Contributor

Implement the feature about ros2/rmw#330

@Barry-Xu-2018 Barry-Xu-2018 marked this pull request as draft November 28, 2022 12:36
@Barry-Xu-2018
Copy link
Contributor Author

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

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.

Copy link
Member

@ivanpauno ivanpauno left a 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/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved
rmw_fastrtps_shared_cpp/src/custom_publisher_info.cpp Outdated Show resolved Hide resolved
@Barry-Xu-2018
Copy link
Contributor Author

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

@Barry-Xu-2018
Copy link
Contributor Author

do rebase.

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

@fujitatomoya fujitatomoya left a 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.

Copy link
Collaborator

@fujitatomoya fujitatomoya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@fujitatomoya
Copy link
Collaborator

@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:
Copy link
Contributor

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.

Copy link
Contributor Author

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.

Copy link
Contributor

@iuhilnehc-ynos iuhilnehc-ynos left a 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.

@Barry-Xu-2018
Copy link
Contributor Author

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.

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

eprosima::fastdds::dds::PublicationMatchedStatus unmatched_status_
Copy link
Contributor

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?

Copy link
Contributor Author

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>
@Barry-Xu-2018
Copy link
Contributor Author

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 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
Copy link
Contributor Author

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

@Barry-Xu-2018 Barry-Xu-2018 changed the title Support matched & unmatched event Support matched event Mar 16, 2023
@Barry-Xu-2018 Barry-Xu-2018 changed the title Support matched event Implement matched event Mar 16, 2023
Signed-off-by: Barry Xu <barry.xu@sony.com>
@mjcarroll mjcarroll merged commit f13df4c into ros2:rolling Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants