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

Hook up the inconsistent topic event inside of rclcpp #2069

Merged
merged 6 commits into from
Mar 13, 2023

Conversation

clalancette
Copy link
Contributor

@clalancette clalancette commented Dec 20, 2022

This PR is part of ros2/ros2#1361 . In particular, what this does is take the inconsistent topic event as it comes from the rmw/rcl layer, and allows the user to register for a callback to find that out.

Along the way, some refactoring was in order. In particular, rclcpp assumed that all events were QoS events, which is not really the case. This PR does some renaming of things to make it clear that these events are not just for QoS, but can be for other things as well. With that done, the last patch in the series actually implements the inconsistent topic callback.

Part of ros2/ros2#1361

Depends on ros2/rmw#339
Depends on ros2/rcl#1024

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.

LGTM!

I have some minor comments

rclcpp/src/rclcpp/publisher_base.cpp Outdated Show resolved Hide resolved
rclcpp/src/rclcpp/publisher_base.cpp Outdated Show resolved Hide resolved
using QOSDeadlineOfferedInfo = rmw_offered_deadline_missed_status_t;
using QOSLivelinessChangedInfo = rmw_liveliness_changed_status_t;
using QOSLivelinessLostInfo = rmw_liveliness_lost_status_t;
using QOSMessageLostInfo = rmw_message_lost_status_t;
Copy link
Member

Choose a reason for hiding this comment

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

it may be worth to rename this one as MessageLostInfo, as it's not qos related as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but I think it is outside the scope of this PR. I can open an issue to track that in a separate PR, if we want.

@clalancette clalancette force-pushed the clalancette/inconsistent-topic-event branch from 0f21d43 to 1413c8b Compare January 17, 2023 15:56
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.

Implementation looks good to me, but i want to know the behavior of this event mechanism in actual use cases.

rclcpp/src/rclcpp/publisher_base.cpp Show resolved Hide resolved
@clalancette clalancette force-pushed the clalancette/inconsistent-topic-event branch from 1413c8b to d21e1ca Compare January 24, 2023 16:30
@clalancette clalancette force-pushed the clalancette/inconsistent-topic-event branch 2 times, most recently from 022162e to a0b7f6f Compare February 10, 2023 16:50
@clalancette clalancette force-pushed the clalancette/inconsistent-topic-event branch 2 times, most recently from ccc107e to be40ebf Compare March 1, 2023 19:19
@clalancette clalancette force-pushed the clalancette/inconsistent-topic-event branch from be40ebf to 9064da9 Compare March 7, 2023 18:56
We are going to be using it for more than just QOS events, so
rename it to just EventHandler and EventHandlerBase for now.
Leave the old names in place but deprecated.

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

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
@clalancette clalancette force-pushed the clalancette/inconsistent-topic-event branch from 9064da9 to 9e47b51 Compare March 10, 2023 14:52
@clalancette clalancette merged commit 6c4afb3 into rolling Mar 13, 2023
@delete-merged-branch delete-merged-branch bot deleted the clalancette/inconsistent-topic-event branch March 13, 2023 13:33
Barry-Xu-2018 pushed a commit to Barry-Xu-2018/rclcpp that referenced this pull request Jan 12, 2024
* Rename QOSEventHandler* to EventHandler.

We are going to be using it for more than just QOS events, so
rename it to just EventHandler and EventHandlerBase for now.
Leave the old names in place but deprecated.

* Rename qos_event.hpp -> event_handler.hpp
* Revamp incompatible_qos callback setting.
* Add in incompatible type implementation.

Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
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

3 participants