Create common structures for executors to use #2143

mjcarroll · 2023-03-29T15:09:15Z

Broken off of #2142 so that it can be used as a base for #2140.

This introduces a few main structures:

ExecutorNotifyWaitable - an object used to collect node and callback group guard conditions from things associated with an executor.
ExecutorEntitiesCollector - an object to collect entities from executor-associated nodes and callback groups
ExecutorEntitiesCollection - a collection of entities that are associated with an executor via callback groups and nodes.

CC: @alsora @mauropasse

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2023-03-29T17:40:43Z

Linux
Linux-aarch64
Windows

rclcpp/src/rclcpp/executors/executor_entities_collector.cpp

rclcpp/test/rclcpp/executors/test_executor_notify_waitable.cpp

Signed-off-by: Michael Carroll <[email protected]>

* Add callback to EntitiesCollector constructor * Make function to check automatically added callback groups take a list Signed-off-by: Michael Carroll <[email protected]>

Signed-off-by: Michael Carroll <[email protected]>

rclcpp/src/rclcpp/executors/executor_entities_collector.cpp

rclcpp/include/rclcpp/executors/executor_entities_collector.hpp

rclcpp/include/rclcpp/executors/executor_entities_collection.hpp

rclcpp/src/rclcpp/executors/executor_entities_collector.cpp

Signed-off-by: Michael Carroll <[email protected]>

rclcpp/src/rclcpp/executors/executor_notify_waitable.cpp

Signed-off-by: Michael Carroll <[email protected]>

Also change node's get_guard_condition to return shared_ptr Signed-off-by: Michael Carroll <[email protected]>

rclcpp/src/rclcpp/executors/executor_notify_waitable.cpp

Check if guard condition is valid before adding it to the waitable Signed-off-by: Michael Carroll <[email protected]>

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2023-04-03T18:57:19Z

Linux
Linux-aarch64
Windows

mjcarroll · 2023-04-11T15:23:40Z

Linux
Linux-aarch64
Windows

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2023-04-12T23:18:36Z

Linux
Linux-aarch64
Windows

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll · 2023-04-13T00:25:50Z

Linux
Linux-aarch64
Windows

mjcarroll · 2023-04-13T03:07:44Z

aarch flaked: :(

wjwwood

I did a pretty thorough review and I didn't really see any issues. I assume a lot of them have been ironed out by the tests and existing applications which exercise it well.

lgtm

wjwwood · 2023-04-13T06:14:14Z

rclcpp/test/rclcpp/executors/test_entities_collector.cpp

+  /**
+   * TODO(mjcarroll): Assert this when we are enforcing that nodes must be destroyed
+   * after their created callback groups.
+  RCLCPP_EXPECT_THROW_EQ(
+    entities_collector.remove_callback_group(cb_group),
+    std::runtime_error("Node must not be deleted before its callback group(s)."));
+  */


How do we enforce this? Or when do you think this TODO could be resolved?

Maybe my understanding is incorrect, but should a callback group be allowed to exist without being created by a node?

If callback groups can exist independently, and outlive their creating node, then this todo is resolved by simply never doing this check, because it is based on an invalid assumption.

If callback groups should not outlive their creating node, then maybe the API should be updated to not return SharedPtr and instead use WeakPtr, which we could then check here.

clalancette · 2023-04-13T14:35:56Z

Windows unfortunately has a bunch of new failures that probably need to be looked into. They were not in the nightlies.

(for that matter, we should also probably run CI on Windows Debug to see what it has to say here)

mjcarroll · 2023-04-13T14:51:15Z

Windows unfortunately has a bunch of new failures that probably need to be looked into. They were not in the nightlies.

Yeah, I'm looking into them.

mjcarroll · 2023-04-13T15:07:24Z

Windows debug for rclcpp, lifecycle demo, and robot state publisher:

mjcarroll · 2023-04-13T17:14:00Z

Windows release again:

mjcarroll · 2023-04-13T19:18:44Z

Try again as None, so I don't hit path length limits:

clalancette · 2023-04-14T13:35:42Z

@mjcarroll Given our long discussion yesterday, where are we with this PR? I think we came to the conclusion that we have a flaky test in test_rclcpp, is there anything else that we need here?

mjcarroll · 2023-04-14T13:43:20Z

@mjcarroll Given our long discussion yesterday, where are we with this PR? I think we came to the conclusion that we have a flaky test in test_rclcpp, is there anything else that we need here?

The test in test_rclcpp is unrelated this pull request, it's the pull request using the rclcpp::WaitSet is where that test failure came in.

clalancette · 2023-04-14T13:52:55Z

@ros-pull-request-builder retest this please

clalancette · 2023-04-14T14:28:43Z

Rpr job is happy. One last CI on Linuxen, since there has been a merge since the last one, then I'll go ahead and merge this:

Linux
Linux-aarch64
RHEL

* Deprecate callback_group call taking context * Add base executor objects that can be used by implementors * Template common operations * Add callback to EntitiesCollector constructor * Make function to check automatically added callback groups take a list * Make executor own the notify waitable * Add pending queue to collector, remove from waitable Also change node's get_guard_condition to return shared_ptr * Change interrupt guard condition to shared_ptr Check if guard condition is valid before adding it to the waitable * Make get_notify_guard_condition follow API tick-tock * Improve callback group tick-tocking * Add thread safety annotations and make locks consistent * Remove the "add_valid_node" API * Only notify if the trigger condition is valid * Only trigger if valid and needed Signed-off-by: Michael Carroll <[email protected]>

mjcarroll force-pushed the mjcarroll/executor_structures branch from d77bac1 to d53349a Compare March 29, 2023 17:05

mjcarroll added 2 commits March 29, 2023 17:37

Deprecate callback_group call taking context

2bf88de

Signed-off-by: Michael Carroll <[email protected]>

Add base executor objects that can be used by implementors

9099635

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll force-pushed the mjcarroll/executor_structures branch from d53349a to 9099635 Compare March 29, 2023 17:37

mjcarroll marked this pull request as ready for review March 29, 2023 17:38

mjcarroll requested review from ivanpauno, hidmic and wjwwood as code owners March 29, 2023 17:38

alsora reviewed Mar 29, 2023

View reviewed changes

rclcpp/src/rclcpp/executors/executor_entities_collector.cpp Outdated Show resolved Hide resolved

alsora reviewed Mar 29, 2023

View reviewed changes

rclcpp/test/rclcpp/executors/test_executor_notify_waitable.cpp Show resolved Hide resolved

mjcarroll added 3 commits March 29, 2023 19:42

Template common operations

2426056

Signed-off-by: Michael Carroll <[email protected]>

Address reviewer feedback:

173ffd6

* Add callback to EntitiesCollector constructor * Make function to check automatically added callback groups take a list Signed-off-by: Michael Carroll <[email protected]>

Lint

a524bf0

Signed-off-by: Michael Carroll <[email protected]>

alsora reviewed Mar 29, 2023

View reviewed changes

rclcpp/src/rclcpp/executors/executor_entities_collector.cpp Outdated Show resolved Hide resolved

alsora approved these changes Mar 29, 2023

View reviewed changes

alsora reviewed Mar 30, 2023

View reviewed changes

rclcpp/include/rclcpp/executors/executor_entities_collector.hpp Outdated Show resolved Hide resolved

alsora reviewed Mar 30, 2023

View reviewed changes

rclcpp/include/rclcpp/executors/executor_entities_collection.hpp Outdated Show resolved Hide resolved

alsora reviewed Mar 30, 2023

View reviewed changes

rclcpp/src/rclcpp/executors/executor_entities_collector.cpp Outdated Show resolved Hide resolved

mjcarroll added 3 commits March 30, 2023 19:20

Address reviewer feedback and fix templates

89f2106

Signed-off-by: Michael Carroll <[email protected]>

Merge branch 'rolling' into mjcarroll/executor_structures

9695eaa

Lint and docs

e173e5a

Signed-off-by: Michael Carroll <[email protected]>

alsora reviewed Mar 30, 2023

View reviewed changes

rclcpp/src/rclcpp/executors/executor_notify_waitable.cpp Show resolved Hide resolved

mjcarroll added 2 commits March 31, 2023 01:45

Make executor own the notify waitable

653d1a3

Signed-off-by: Michael Carroll <[email protected]>

Add pending queue to collector, remove from waitable

a6c4c1b

Also change node's get_guard_condition to return shared_ptr Signed-off-by: Michael Carroll <[email protected]>

mjcarroll force-pushed the mjcarroll/executor_structures branch from 3b7266c to a6c4c1b Compare April 3, 2023 17:09

alsora reviewed Apr 3, 2023

View reviewed changes

rclcpp/src/rclcpp/executors/executor_notify_waitable.cpp Outdated Show resolved Hide resolved

mjcarroll added 2 commits April 3, 2023 17:55

Change interrupt guard condition to shared_ptr

9dd48ce

Check if guard condition is valid before adding it to the waitable Signed-off-by: Michael Carroll <[email protected]>

Lint and docs

6267741

Signed-off-by: Michael Carroll <[email protected]>

alsora approved these changes Apr 12, 2023

View reviewed changes

Remove the "add_valid_node" API

6379f0c

Signed-off-by: Michael Carroll <[email protected]>

mjcarroll added 2 commits April 12, 2023 18:51

Only notify if the trigger condition is valid

855c64d

Signed-off-by: Michael Carroll <[email protected]>

Only trigger if valid and needed

d9a9206

Signed-off-by: Michael Carroll <[email protected]>

wjwwood approved these changes Apr 13, 2023

View reviewed changes

Merge branch 'rolling' into mjcarroll/executor_structures

838d1ae

Merge branch 'rolling' into mjcarroll/executor_structures

ab3bbf4

clalancette approved these changes Apr 14, 2023

View reviewed changes

clalancette merged commit e3d9d81 into rolling Apr 14, 2023

delete-merged-branch bot deleted the mjcarroll/executor_structures branch April 14, 2023 18:56

Barry-Xu-2018 mentioned this pull request Jun 4, 2024

Regression in Rolling: error "cannot publish data, at ./src/rmw_publish.cpp:66" during shutdown of binary ros2/rmw_fastrtps#761

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create common structures for executors to use #2143

Create common structures for executors to use #2143

mjcarroll commented Mar 29, 2023

mjcarroll commented Mar 29, 2023

mjcarroll commented Apr 3, 2023

mjcarroll commented Apr 11, 2023

mjcarroll commented Apr 12, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

wjwwood left a comment

wjwwood Apr 13, 2023

mjcarroll Apr 13, 2023

clalancette commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

clalancette commented Apr 14, 2023

mjcarroll commented Apr 14, 2023

clalancette commented Apr 14, 2023

clalancette commented Apr 14, 2023

Create common structures for executors to use #2143

Create common structures for executors to use #2143

Conversation

mjcarroll commented Mar 29, 2023

mjcarroll commented Mar 29, 2023

mjcarroll commented Apr 3, 2023

mjcarroll commented Apr 11, 2023

mjcarroll commented Apr 12, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

wjwwood left a comment

Choose a reason for hiding this comment

wjwwood Apr 13, 2023

Choose a reason for hiding this comment

mjcarroll Apr 13, 2023

Choose a reason for hiding this comment

clalancette commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

mjcarroll commented Apr 13, 2023

clalancette commented Apr 14, 2023

mjcarroll commented Apr 14, 2023

clalancette commented Apr 14, 2023

clalancette commented Apr 14, 2023