Skip to content

feat: Added callback group events executor#3097

Open
jmachowinski wants to merge 13 commits intoros2:rollingfrom
cellumation:add_cbg_executor
Open

feat: Added callback group events executor#3097
jmachowinski wants to merge 13 commits intoros2:rollingfrom
cellumation:add_cbg_executor

Conversation

@jmachowinski
Copy link
Copy Markdown
Collaborator

@jmachowinski jmachowinski commented Mar 13, 2026

This commit adds the callback group events executor. It features:

  • multithreading support
  • correct handling of sim time
  • usage of the events subsystem

Description

This moved the events cbg executor from cm_executors into rlcpp mainline.

Did you use Generative AI?

No

@alsora
Copy link
Copy Markdown
Collaborator

alsora commented Mar 15, 2026

@jmachowinski can you add this to the executor unit-tests?

@jmachowinski jmachowinski force-pushed the add_cbg_executor branch 3 times, most recently from f7b3021 to 7eb2fca Compare March 18, 2026 12:20
Janosch Machowinski and others added 5 commits March 20, 2026 13:30
feat: Added callback group events executor
5dc4cfc 
This commit adds the callback group events executor.
It features:
 - multithreading support
 - correct handling of sim time
 - usage of the events subsystem

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
fix: Moved implementation into own namespace cbg_executor
19d754c 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
chore: Moved out of experimental
ad080e4 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
chore: cleanup
b311afb 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
feat: Refactored and extended executor benchmarks
e7e65d9 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
Janosch Machowinski and others added 3 commits March 23, 2026 15:14
tests: Added EventsCBGExecutor to executor tests
11d733c 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
fix: Call timer->call() from the execution context, not the timer man…
80cc919 
…ager

This code was copied straight from the executor and seems to be a
workaround for the multithreaded executor, that breaks in this use case.

The correct solution for us is to do the timer->call() from within the
worker thread. This fixes a deadlock due to double acquisition of an
internal lock within the timer manager.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
perf: Don't wake the timer thread if not needed
430c2dd 
In case the next timer wakeup time is not changed by an insertion of a
timer, don't wake up the timer thread.

Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Mar 23, 2026

Pulls: #3097
Gist: https://gist.githubusercontent.com/jmachowinski/f9f71b19f46697be097713df6f9f6016/raw/f0f3fa2b988318178c70a0f1c9f90dbc50321ee8/ros2.repos
BUILD args:
TEST args:
ROS Distro: rolling
Job: ci_launcher
ci_launcher ran: https://ci.ros2.org/job/ci_launcher/18616

  • Linux Build Status
  • Linux-aarch64 Build Status
  • Linux-rhel Build Status
  • Windows Build Status

Janosch Machowinski added 2 commits March 23, 2026 15:39
test: Added test if service works as expected with executor
cf11ea0 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: cpplint fix
0cae831 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
skyegalaxy
skyegalaxy requested changes Mar 23, 2026
View reviewed changes
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aside from some spelling nits and a handful of small implementation questions, this is looking great! Very excited to get this over the finish line soon.

rclcpp/src/rclcpp/executors/events_cbg_executor/events_cbg_executor.cpp
added_nodes_cpy = added_nodes;
}

// *3 is a rough estimate of how many callback_group a node may have
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What would happen in the case where a node might have more than 3 callback groups?

Copy link
Copy Markdown
Collaborator Author

@jmachowinski jmachowinski Mar 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you get slight runtime overhead, as the vector needs to be resized.

rclcpp/src/rclcpp/executors/events_cbg_executor/events_cbg_executor.cpp
}
}

// FIXME inform scheduler about remove cbgs
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are the side-effects of leaving this as is and not informing the scheduler here?

rclcpp/src/rclcpp/executors/events_cbg_executor/timer_manager.hpp
* This is needed, as clocks may be deleted during normal operation,
* and be don't have a way to create a permanent ros time clock.
*/
class ClockConditionalVariable
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it could be worth eventually merging the functionality of this ClockConditionalVariable variant with the existing one and cutting down on the duplicate code if it's not too complex to do.

mini-1235
mini-1235 reviewed Mar 27, 2026
View reviewed changes
Copy link
Copy Markdown
Contributor

@mini-1235 mini-1235 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will/should this replace the experimental event executor here?

rclcpp/rclcpp_components/src/component_container_event.cpp

Line 26 in 1e07045

auto exec = std::make_shared<rclcpp::experimental::executors::EventsExecutor>();

chore: fixed typos
1e526d2 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Janosch Machowinski added 2 commits March 27, 2026 19:14
fix: Removed hack for getting clock from timer
5082992 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: more typos
dab768c
@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Mar 27, 2026

Will/should this replace the experimental event executor here?

rclcpp/rclcpp_components/src/component_container_event.cpp

Line 26 in 1e07045

auto exec = std::make_shared<rclcpp::experimental::executors::EventsExecutor>();

yes, that is the idea.

skyegalaxy
skyegalaxy requested changes Apr 1, 2026
View reviewed changes
rclcpp/include/rclcpp/executors/events_cbg_executor/events_cbg_executor.hpp
* \param options common options for all executors
* \param number_of_threads number of threads to have in the thread pool,
* the default 0 will use the number of cpu cores found (minimum of 2)
* \param yield_before_execute if true std::this_thread::yield() is called
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this parameter yield_before_execute isn't present in this constructor

rclcpp/include/rclcpp/executors/events_cbg_executor/events_cbg_executor.hpp

std::unique_ptr<cbg_executor::CBGScheduler> scheduler;

std::thread rcl_polling_thread;
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this thread doesn't appear to be used anywhere

rclcpp/src/rclcpp/executors/events_cbg_executor/timer_manager.hpp

class TimerManager
{
std::array<TimerQueue, 3> timer_queues;
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::array<TimerQueue, 3> timer_queues;
constexpr int NUM_TYPES_OF_TIMERS = 3;
std::array<TimerQueue, NUM_TYPES_OF_TIMERS> timer_queues;

rclcpp/src/rclcpp/executors/events_cbg_executor/first_in_first_out_scheduler.cpp

// as, we remove an reappend ready callback_groups during execution,
// the first ready cbg may not contain the lowest id. Therefore we
// need to search the whole deque
Copy link
Copy Markdown
Member

@skyegalaxy skyegalaxy Apr 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a blocker for this PR, but I think it would be interesting to see if we could find a way to efficiently index ready_callback_groups by ID as well as the order, so that instead of searching through the entire deque, we could do something like ready_callback_groups.pop_by_id(max_id)

@skyegalaxy
Copy link
Copy Markdown
Member

skyegalaxy commented Apr 1, 2026

@jmachowinski we also need DCO for some of the newer commits

@skyegalaxy skyegalaxy requested review from alsora and mjcarroll April 1, 2026 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skyegalaxy skyegalaxy skyegalaxy requested changes

@alsora alsora Awaiting requested review from alsora

@mjcarroll mjcarroll Awaiting requested review from mjcarroll

+1 more reviewer

@mini-1235 mini-1235 mini-1235 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jmachowinski @alsora @skyegalaxy @mini-1235