Skip to content

feat: Added callback group events executor#3097

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

feat: Added callback group events executor#3097
jmachowinski wants to merge 16 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 5 times, most recently from 26fd9db to 430c2dd Compare March 23, 2026 14:15
@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

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?

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>();

@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/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
skyegalaxy
skyegalaxy approved these changes Apr 7, 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.

lgtm! can we rebase on latest rolling before running CI?

@jmachowinski
Copy link
Copy Markdown
Collaborator Author

jmachowinski commented Apr 8, 2026

This currently depends on ros2/ament_cmake_ros#62

@SteveMacenski
Copy link
Copy Markdown
Collaborator

SteveMacenski commented Apr 8, 2026

Per a discussion with @skyegalaxy today: would it be possible to get the component container + isolated variant of this? I can have this as a tested variant in Nav2 then

@skyegalaxy
Copy link
Copy Markdown
Member

skyegalaxy commented Apr 8, 2026

@SteveMacenski - I see that there are these two PRs #3055, #3080 to try and generalize the component container across executors, but I'm not sure if that'll get in before the feature freeze. Absent that, we could probably just create a dedicated one for now

@skyegalaxy skyegalaxy mentioned this pull request Apr 9, 2026
Draft
@skyegalaxy
Copy link
Copy Markdown
Member

skyegalaxy commented Apr 9, 2026

@SteveMacenski - draft PR here: #3123

Janosch Machowinski and others added 14 commits April 9, 2026 11:45
feat: Added callback group events executor
709d820 
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
3763772 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
chore: Moved out of experimental
1974520 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
chore: cleanup
10d3126 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
feat: Refactored and extended executor benchmarks
260910a 
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
tests: Added EventsCBGExecutor to executor tests
de0176a 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
fix: Call timer->call() from the execution context, not the timer man…
acac510 
…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
7a533bf 
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>
test: Added test if service works as expected with executor
2013219 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: cpplint fix
fffe8e6 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: fixed typos
33acd3e 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
fix: Removed hack for getting clock from timer
7721d58 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: more typos
15ab21e 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: Minor cleanups
26654ef 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Janosch Machowinski added 2 commits April 9, 2026 13:34
feat: Switch to c++20 and remove resulting compile warnings
5f34bf4 
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
chore: fix compiler warning
432ebe0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skyegalaxy skyegalaxy skyegalaxy approved these 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.

5 participants

@jmachowinski @alsora @skyegalaxy @SteveMacenski @mini-1235