Create a default warning for qos incompatibility#536
Conversation
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Emerson Knapp <emerson.b.knapp@gmail.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
Signed-off-by: Miaofei <miaofei@amazon.com>
7e2be31 to
f56ff54
Compare
|
This pull request has been updated to behave similarly to the |
ivanpauno
left a comment
There was a problem hiding this comment.
Just one comment that I think needs to be addressed.
Signed-off-by: Miaofei <miaofei@amazon.com>
|
@ros2/aws-oncall - please run this CI job |
| const char * node_logger_name = rcl_node_get_logger_name(pub->node); | ||
| if (NULL == node_logger_name) { | ||
| Py_RETURN_NONE; | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Looks to me like the build test failures on Windows are unrelated, @ivanpauno. |
|
FYI, I think this change is going to cause a string of new test failures in |
@emersonknapp @mm318 @ros2/aws-oncall FYI |
|
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 |
|
See https://ci.ros2.org/job/ci_linux/10244/testReport/ for a list of test failures in the |
|
@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? |
|
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? |
This reverts commit d94960b.
Signed-off-by: Emerson Knapp emerson.b.knapp@gmail.com