rmw_connextdds
rmw_connextdds copied to clipboard
ros2
Add support for listener callbacks
This PR implements the new listener-related functions that were introduced by #44.
Signed-off-by: Andrea Sorbini [email protected]
Linux CI with only Connext enabled for initial validation:
I investigated the failures, I believe the issues lie either in some test problem or they are unrelated to this PR (and they manifested because I ran CI with just this RMW enabled, which usually doesn't happen).
-
rclcpp.TestSubscription.on_new_message_callback: in the last case, the test only waits while
c3 == 0while instead the loop should continue for as long asc3 < 3. - rclcpp.TestWaitForAllAckedWithParm/TestPublisherWaitForAllAcked.check_wait_for_all_acked_with_QosPolicy/0: unrelated issue. Problem is with the test which only waits for 500ms to receive an ACK. Connext's default settings use a 3s periodic heartbeat, which may cause the ACK not to be received within the timeout. Increasing the wait to 6s makes the test pass consistently.
-
rclpy.rclpy.test.test_client.TestClient.test_service_timestamps: unrelated problem. The issue lies with the test which doesn't take into account the possible return value
(None, None)which indicates a successfultake_request()with no request actually taken (i.e. an "empty polling" of the requests).. - launch_testing_examples.launch_testing_examples.check_node_launch_test and launch_testing_examples.launch_testing_examples.set_param_launch: unrelated issue. It seems like the failures can be avoided by increasing the wait timeout or by configuring Connext to more quickly repair samples on topic "ros_discovery_info".
- Errors in
rviz_renderingseem to be unrelated to the RMW layer because they occur also with other RMW implementations.
rclcpp.TestSubscription.on_new_message_callback: in the last case, the test only waits while c3 == 0 while instead the loop should continue for as long as c3 < 3.
Could we fix that test? It's directly related to the listener callbacks API, so it would be good to get it fixed to make sure that's the only issue.
Could we fix that test? It's directly related to the listener callbacks API, so it would be good to get it fixed to make sure that's the only issue.
I opened ros2/rclcpp#1900 to fix the unit tests.
Could we fix that test? It's directly related to the listener callbacks API, so it would be good to get it fixed to make sure that's the only issue.
I opened ros2/rclcpp#1900 to fix the unit tests.
Also opened ros2/rclcpp#1901 to fix the issue with the wait_for_acknowledgments() test.
Looks good to me, I just want to confirm how the notify_new_data function works exactly.
@clalancette @ivanpauno can this PR be merged now even with the freeze? It doesn't change API/ABI.
@clalancette @ivanpauno can this PR be merged now even with the freeze? It doesn't change API/ABI.
Hi @asorbini, I don't think we should merge this because it is a feature addition and the freeze is in place. If it doesn't break API/ABI, it should be able to be added in a patch release.
Did anyone ever test this PR with irobot's events executors? https://github.com/irobot-ros/events-executor If I run their tests with rmw_connextdds enabled and these changes here applied to the rmw_connextdds humble branch, I get these kinds of errors/warnings:
This PR is needed though, if one is trying to use events executors with connextdds. It would be nice if someone could have a look at this issue.
@asorbini FYI we recently opened the PR for the events-executor in rclcpp and we currently have disabled it from running with connext. To test this PR you can easily remove the "if connext -> skip" parts of the tests https://github.com/ros2/rclcpp/pull/2140
for example here (and in every test in this file https://github.com/ros2/rclcpp/pull/2140/files#diff-e1ac0eadd712d463d84ffc15b59e669fbf53f75c6f81e4c06f03f4d9c91b0ceaR47)
@alsora thank you for the heads up, that's very useful to validate these changes.
Do you have a .repos file that I can use to test that PR or is it just those rclcpp changes + current rollng?
rolling + that PR is sufficient to test, no other changes to other repos are needed.
In the last CI run, all the unit-tests passed https://github.com/ros2/rclcpp/pull/2140#issuecomment-1486605066.
Note that there is 1 flaky test that we are currently investigating (TEST_F(TestEventsExecutor, destroy_entities)).
Feel free to share the results if you need help debugging failures.
What's the status of this PR? Given that https://github.com/ros2/rclcpp/pull/2155 is merged now, it would be really greate to have this PR "officially" approved and merged in order to use rclcpp with ConnextDDS to it's full potential out of the box.
What's the status of this PR? Given that ros2/rclcpp#2155 is merged now, it would be really greate to have this PR "officially" approved and merged in order to use rclcpp with ConnextDDS to it's full potential out of the box.
At this point, it needs another approval (there have been changes since the previous ones), and CI run on it. I can do the latter, but I'd prefer @asorbini or @alsora did another review/approval first.
I'm not really familiar with connext, but overall the PR looks sane.
From my perspective I would suggest to create a rclcpp branch where the checks that disable events-executor with connext are removed, and then, if it passes all unit-tests, it should be good to go.
@alsora here is the PR to enable the callback groupd tests in rclcpp again: https://github.com/ros2/rclcpp/pull/2182
The tests reenabled in https://github.com/ros2/rclcpp/pull/2182 are now succeeding. Although they seem to be somewhat flakly, I'd suppose, that this PR should be mergable now?
Although they seem to be somewhat flakly, I'd suppose, that this PR should be mergable now?
We'd really prefer to not merge in "somewhat flaky" tests. We are just down to handful of those now, and I'd like not to increase that number.