Skip to content

Hook up the inconsistent topic event inside of rclcpp#2069

Merged
clalancette merged 6 commits intorollingfrom
clalancette/inconsistent-topic-event
Mar 13, 2023
Merged

Hook up the inconsistent topic event inside of rclcpp#2069
clalancette merged 6 commits intorollingfrom
clalancette/inconsistent-topic-event

Conversation

@clalancette
Copy link
Copy Markdown
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
Copy Markdown
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

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.

@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.

3 participants