Skip to content

Added incompatible event support to ros2 topic echo and ros2 topic pub#410

Merged
ivanpauno merged 4 commits intoros2:masterfrom
aws-ros-dev:jaisontj/incompatible_qos
Apr 1, 2020
Merged

Added incompatible event support to ros2 topic echo and ros2 topic pub#410
ivanpauno merged 4 commits intoros2:masterfrom
aws-ros-dev:jaisontj/incompatible_qos

Conversation

@jaisontj
Copy link
Copy Markdown
Contributor

@jaisontj jaisontj commented Nov 27, 2019 •
edited
Loading

Depends on ros2/rclpy:459

Related to the feature discussed here. This PR handles the ON_REQUESTED_INCOMPATIBLE_QOS and ON_OFFERED_INCOMPATIBLE_QOS callback events on ros2 topic echo and ros2 topic pub commands respectively.

This way, users now will get a warning when they echo/pub to a topic with incompatible QoS Policies.

Signed-off-by: Jaison Titus jaisontj92@gmail.com

@jaisontj jaisontj mentioned this pull request Nov 27, 2019
Closed
14 tasks
@mm318 mm318 force-pushed the jaisontj/incompatible_qos branch from 6d2aa02 to addbb81 Compare February 24, 2020 19:38
@jaisontj @mm318
Added incompatible event support to ros2 topic echo and ros2 topic pub
2bf2cbf 
Signed-off-by: Jaison Titus <jaisontj92@gmail.com>
@mm318 mm318 force-pushed the jaisontj/incompatible_qos branch from addbb81 to 2bf2cbf Compare March 22, 2020 18:22
@mm318 mm318 mentioned this pull request Mar 24, 2020
Merged
@mm318
optionally register incompatible qos callbacks depending on support
b3cb137 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318
Copy link
Copy Markdown
Member

mm318 commented Mar 24, 2020

@jaisontj, @ivanpauno, @wjwwood, can any of you bring this pull request out of draft mode?

@ivanpauno ivanpauno marked this pull request as ready for review March 25, 2020 12:21
ivanpauno
ivanpauno approved these changes Mar 25, 2020
View reviewed changes
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.

Left some minor comments, LGTM!

@mm318
use updated exception name
eaaf294 
Signed-off-by: Miaofei <miaofei@amazon.com>
@mm318 mm318 mentioned this pull request Mar 30, 2020
Merged
@mm318
fix flake8 linter issues
e07cf22 
Signed-off-by: Miaofei <miaofei@amazon.com>
@ivanpauno ivanpauno merged commit 02ae480 into ros2:master Apr 1, 2020
@ivanpauno ivanpauno changed the title [WIP] Added incompatible event support to ros2 topic echo and ros2 topic pub Added incompatible event support to ros2 topic echo and ros2 topic pub Apr 1, 2020
@Karsten1987
Copy link
Copy Markdown
Contributor

Karsten1987 commented Apr 28, 2020 •
edited
Loading

This change calls rmw_create_publisher twice per topic on the same node. In the case of DDS (tested on FastRTPS only) this might work well, but other RMW implementations, i.e. iceoryx might not support this.

Is there a way that the publisher is called only once? Meaning, can the QoS event checked somewhat before creating a publisher?

This was referenced Apr 28, 2020
Closed
Closed
@mm318
Copy link
Copy Markdown
Member

mm318 commented Apr 28, 2020

We could add an option to rclcpp's and rclpy's create_publisher()/create_subscription() to ignore registering callbacks of unsupported event types, instead of throwing and retrying to create_publisher()/create_subscription() at the client level.

@mm318 mm318 mentioned this pull request Apr 29, 2020
Merged
mm318 added a commit to aws-ros-dev/ros2cli that referenced this pull request Apr 30, 2020
@mm318
Revert "Added incompatible event support to ros2 topic echo and ros2 …
9e8bb33 
…topic pub (ros2#410)"

This reverts commit 02ae480.

Signed-off-by: Miaofei <miaofei@amazon.com>
@nnmm nnmm mentioned this pull request Feb 1, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mm318 mm318 mm318 left review comments

@ivanpauno ivanpauno ivanpauno approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jaisontj @mm318 @Karsten1987 @ivanpauno