Create a default warning for qos incompatibility

[emersonknapp]

Signed-off-by: Emerson Knapp [email protected]

[mm318]

This pull request has been updated to behave similarly to the rclcpp version (in ros2/rclcpp#1051).

[ivanpauno]

Just one comment that I think needs to be addressed.

[mm318]

@ros2/aws-oncall - please run this CI job
Gist: https://gist.githubusercontent.com/mm318/9ab95351a88ec18085660718f582912f/raw/779f53aa1e5ffcebc4c093d71f49bd5c7d2c97c4/ros2_incompatible_qos_callbacks.repos
BUILD args: --packages-up-to rclpy
TEST args: --packages-select rclpy
Job: ci_launcher

[ivanpauno]

This is fine, though I think that a bit unnecessary.

IMO, a nicer approach would have been the following:

callbacks = PublisherEventCallbacks()  # default callbacks is true, but they're not yet defined
# Default callbacks are added here, if `event_callbacks._use_default_callbacks` is true, and the user doesn't explicitly define that callback.
publisher = self.node.create_publisher(EmptyMsg, 'test_topic', 10, event_callbacks=callbacks)

In both cases the result the user sees is the same, so I won't block on the change.

[mm318]

I thought about doing the way you mentioned, but with event_callbacks.incompatible_qos no longer None, it's harder to determine later on if it's the default one or a user provided one, which is needed to handle the UnsupportedEventTypeError exception differently.

[ivanpauno]

I thought about doing the way you mentioned, but with event_callbacks.incompatible_qos no longer None, it's harder to determine later on if it's the default one or a user provided one, which is needed to handle the UnsupportedEventTypeError exception differently.

It could initially be None, except the user provided explicitly one.
If it's set to None and use default_callbacks is true, the event is registered with the default callback when the publisher is created.

I think the approach I'm mentioning is exactly the one used in rclcpp.

[piraka9011]

• Linux
• Linux-aarch64
• macOS
• Windows

[mm318]

• Windows

Looks to me like the build test failures on Windows are unrelated, @ivanpauno.

[jacobperron]

FYI, I think this change is going to cause a string of new test failures in ros2cli, as the warning appears when running various CLI tools (e.g. ros2 topic echo).

[dirk-thomas]

FYI, I think this change is going to cause a string of new test failures in ros2cli, as the warning appears when running various CLI tools (e.g. ros2 topic echo).

@emersonknapp @mm318 @ros2/aws-oncall FYI

[emersonknapp]

Thx for the heads up. Saw one such test failure in rosbag2, though that was resolvable by making the test less fragile. CLI is a bit different

[dirk-thomas]

See https://ci.ros2.org/job/ci_linux/10244/testReport/ for a list of test failures in the ros2cli repo.

[nuclearsandwich]

@emersonknapp since this PR will introduce test failures. I'm wondering if we should revert it before the nightlies and explore solutions in the morning?

[emersonknapp]

I'm personally fine with that, I believe in a revert-friendly engineering culture. I'm unfortunately not at my computer for the remainder of the evening, if you or @mm318 would be able to do it?