Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EventsExecutor #286

Merged
merged 10 commits into from
Feb 24, 2022
Merged

Conversation

irobot-ros
Copy link
Contributor

@irobot-ros irobot-ros commented Oct 21, 2020

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 ros2/rclcpp#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:

@ros-discourse
Copy link

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

@ros-discourse
Copy link

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

Copy link
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.

Thanks for working on this!

I have left some minor comments about the API, but I think it doesn't make a big difference.

rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/types.h Outdated Show resolved Hide resolved
Copy link
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.

I think this looks good.

I still have a minimal preference for not having an rmw_listener_event_t argument in the listener callbacks and making that part of user_data (which will require registering not exactly the same function in all listeners of the event executor, but it can be an small wrapper of a common function).
Without the rmw_listener_event_t argument, the extra void * <type>_handle is also not required in the functions that register a listener.

Anyways, I think that adding this makes sense.
I would recommend ignoring my suggestions until @wjwwood also reviews (to avoid unnecessary iterations).

edit: I also think it would be fine to have two void * user data arguments (e.g.: void * arg1, void * arg2 and maybe, arg3), and in that case an extra wrapper is not needed.

rmw/include/rmw/types.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
@ivanpauno ivanpauno requested a review from hidmic January 20, 2021 20:11
rmw/include/rmw/listener_event_types.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
rmw/include/rmw/rmw.h Outdated Show resolved Hide resolved
@rpaaron
Copy link

rpaaron commented Mar 9, 2021

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?

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2021

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).

@wjwwood
Copy link
Member

wjwwood commented Mar 10, 2021

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.

rmw/include/rmw/event.h Outdated Show resolved Hide resolved
@wjwwood
Copy link
Member

wjwwood commented Apr 1, 2021

@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?

@irobot-ros
Copy link
Contributor Author

@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.

@wjwwood
Copy link
Member

wjwwood commented Apr 2, 2021

CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

@wjwwood
Copy link
Member

wjwwood commented Apr 2, 2021

@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.

@irobot-ros
Copy link
Contributor Author

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.

@mauropasse
Copy link
Contributor

@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 ros2/rmw_cyclonedds#256 (comment) (Issue 1).
A temporary workaround is done in this PR: eclipse-cyclonedds/cyclonedds#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.

Mauro added 5 commits April 5, 2021 11:08
add constness

Use or discard previous events: Guard conditions

Move parentheses

Rename Event_callback to ExecutorEventCallback

update name

Add events support

void return on set_events_executor_callback

Revert "void return on set_events_executor_callback"

Rename ExecutorEventCallback -> EventsExecutorCallback

Rename set_events_executor_callback->set_listener_callback

Use data types when setting callbacks

Move rcutils/executor_event_types.h to rmw/

Add executor_event_types.h

rename event types

Rename executor_context->callback_context

Add APIs documentation

Modify doc

Rename callback_context->user_data

Reorder APIs arguments

Add more info on set_listener_callback comments

rename rmw_listener_cb_t->rmw_listener_callback_t

Add missing comments

use void * to pass executor ptr

Rework executor callback data

Use RMW renamed file

Define publisher/subscription event types

Signed-off-by: Alberto Soragna <asoragna@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Mauro Passerino and others added 2 commits April 5, 2021 11:08
Signed-off-by: Mauro Passerino <mpasserino@irobot.com>
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood force-pushed the irobot/add-events-executor branch from 8bff3d0 to 9492336 Compare April 5, 2021 18:09
@wjwwood
Copy link
Member

wjwwood commented Apr 6, 2021

I'm rerunning CI for this without the rclcpp pull request (ros2/rclcpp#1579):

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

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.

@mauropasse
Copy link
Contributor

mauropasse commented Apr 8, 2021

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 #286 (comment)

…-executor

Signed-off-by: Alberto Soragna <alberto.soragna@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants