Utilize rclcpp::WaitSet as part of the executors

[mjcarroll]

Improve number of rcl_wait_set creations and deletions by making the default Single/Multithreaded executors work like the static single threaded executor in terms of entity collection rebuilding.

TODOS

• The concepts of entity collector and notify waitable come from add events-executor and timers-manager in rclcpp #2140, they need to be rectified with that pull request and iRobot copyright needs to be added to appropriate files
• restore static-single threaded files (removed for ease of testing)
• documentation/test coverage on some of the new structures

[alsora]

@mjcarroll I looked into the new entities collection and collector classes and they look great.

I think that there are very few changes needed in order to use them with the events-executor:

1. add to ExecutorEntitiesCollection some public methods such as

rclcpp::ClientBase::WeakPtr get_client(const rcl_client_t * client_handle);

2. the ExecutorNotifyWaitable in the events-executor should have an execute function that refresh the events-executor collections. to do so, it will need to access some internal methods of the events-executor. this could be configurable through a lambda function passed during construction.

[dhood]

hey @wjwwood , I was just in a roscontrol WG meeting where @christophfroehlich mentioned a regression that they've seen only in rolling testing build. they're manually building a waitset around a subscription, which maybe is a bit of a niche case, but wanted to flag it for triaging before the next sync is planned - it might not be possible to have a subscription in two wait sets after the changes in this PR?

[christophfroehlich]

Thanks for pinging this here. It is already failing on main, I think this change here was released already.

[dhood]

ok - although I have a new thought - maybe now the intended behaviour is being enforced! The exception "subscription already associated with a wait set" might mean that it was never appropriate to have it in two wait sets 😅

[fujitatomoya]

it might not be possible to have a subscription in two wait sets after the changes in this PR?

No, we cannot add the subscription to different wait set after this change. there is a test case to expect the exception,

rclcpp/rclcpp/test/rclcpp/node_interfaces/test_node_topics.cpp

Lines 165 to 167 in de666d2

	EXPECT_THROW(
	node_topics->add_subscription(subscription, callback_group_in_different_node),
	std::runtime_error);

CC: @wjwwood @mjcarroll

[wjwwood]

maybe now the intended behaviour is being enforced! The exception "subscription already associated with a wait set" might mean that it was never appropriate to have it in two wait sets 😅

Sorry, just catching up on this, yes indeed that is the case. Technically the constraint is that you cannot wait on one subscription from two different wait sets simultaneously, but we enforce that by not allowing a single subscription to be contained in/associated with more than one wait set at a time.

[wjwwood]

they're manually building a waitset around a subscription, which maybe is a bit of a niche case, but wanted to flag it for triaging before the next sync is planned

If you want to use a subscription with a wait set manually (and that is totally ok to do), then you need to make sure the subscription is not part of a callback group that was added to an executor. You can do that by creating a callback group that has the automatically_add_to_executor_with_node flag set to false (

rclcpp/rclcpp/include/rclcpp/node.hpp

Line 151 in de666d2

bool automatically_add_to_executor_with_node = true);

) and use it when creating the subscription. That will ensure that callback group is never added to an executor (and therefore a wait set) when the node is added to an executor.

[dhood]

oo hadn't noticed that before; awesome! thanks for the pointers!