Closed irobot-ros closed 2 years ago
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/ros2-middleware-change-proposal/15863/20
This pull request has been mentioned on ROS Discourse. There might be relevant details there:
https://discourse.ros.org/t/ros2-middleware-change-proposal/15863/29
I see there is some discussion over on https://github.com/ros2/design/pull/305 about executors being supported out of tree. Will this PR get merged even if the other is still being discussed?
I've been testing these changes with some rebasing. I think things are looking good. I still have one item to follow up on design wise, but in the mean time we need to clean up the commit history. Ideally, we could organize the commits (maybe squash them into one or at least fewer commits), but at the very least we need to have all the commits passing the DCO checker. This goes for the other pull requests too.
I would offer to do this for you, but I really need you guys to do the DCO signing.
I cannot test it until master is merged in or rebased against master (my preference).
I see there is some discussion over on ros2/design#305 about executors being supported out of tree. Will this PR get merged even if the other is still being discussed?
That's the plan. Merge the rmw/rcl changes related to "listener" style API, and then iterate on the c++ executor designs asynchronously.
@irobot-ros or @mauropasse, is it ok if I push directly (maybe force push to rebase) to this pull request branch? Or should I make my own?
@wjwwood yes sure, go ahead and push to this branch if you prefer.
FYI we just pushed a commit (here and in other PRs) that removes guard condition listeners as discussed in last Middleware WG.
@irobot-ros @mauropasse there are some test failures (I can fix the uncrustify one for rmw_fastrtps
), but the others need some investigation, so if you have any time that would be great:
https://ci.ros2.org/job/ci_linux/14198/testReport/
I know it must be getting close to you guy's weekend, so if not I'll try to investigate tomorrow some if I have time.
I fixed the fastrtps uncrustify errors. I had a quick look at the other failures, but I don't really know the internals of how QoS events work. @mauropasse should be able to address them, but he will be back on Tuesday.
@irobot-ros @mauropasse there are some test failures (I can fix the uncrustify one for rmw_fastrtps), but the others need some investigation, so if you have any time that would be great: https://ci.ros2.org/job/ci_linux/14198/testReport/
projectroot.test.rclcpp.test_qos_event
fails because assigning listener callbacks in CycloneDDS
breaks the wait_set based executors notify mechanisms: see https://github.com/ros2/rmw_cyclonedds/pull/256#issuecomment-791478380 (Issue 1).
A temporary workaround is done in this PR: https://github.com/eclipse-cyclonedds/cyclonedds/pull/699
The test_qos_event
is based on the SingleThreadedExecutor
so is never notified about a happened event.
I'll take a look to the other failing tests.
I'm rerunning CI for this without the rclcpp pull request (https://github.com/ros2/rclcpp/pull/1579):
If it looks good, then we can at least take those API's in, and maybe we can do ABI stable additions to rclcpp
after the release. I'm going to propose we extend our pimpl pattern usage before the release to make that possible.
I went through the failing tests
projectroot.test.test_graph__rmw_cyclonedds_cpp
projectroot.test.test_events__rmw_cyclonedds_cpp
rcl.NodeGraphMultiNodeFixture.test_node_info_subscriptions
rcl.NodeGraphMultiNodeFixture.test_node_info_publishers
rcl.NodeGraphMultiNodeFixture.test_node_info_services
rcl.NodeGraphMultiNodeFixture.test_node_info_clients
rcl.test_events__rmw_cyclonedds_cpp.gtest.missing_result
They are all fixed with my temporary workaround on CycloneDDS eclipse-cyclonedds/cyclonedds#699 , the issue is what I explained on https://github.com/ros2/rmw/pull/286#issuecomment-813443853
This PR introduces the changes required to implement the
EventsExecutor
design in rmw. See design and Discourse post.The new executor uses an events queue and a timers manager as opposed to waitsets, to efficiently execute entities with work to do.
This new executor greatly reduces CPU usage of a ROS 2 application. See the blog post for more details on the tests that we run.
The bulk of the changes for this implementation are in the rclcpp layer, with some minor changes in other repositories (rcl, rmw, rmw_implementation) for forwarding entities, the declaration of some data types in rcutils, and finally some additional changes in the vendor specific rmw implementations.. We currently implemented this only on top of the default ROS middleware fastrtps, while we provided stubs for other middlewares.
See the main PR to rclcpp https://github.com/ros2/rclcpp/pull/1416.
The current implementation does not support ROS 2 actions, which will be added in a follow up PR.
Developed by iRobot Mauro Passerino Lenny Story Alberto Soragna
Connects to: