feat: Added callback group events executor#3097
Conversation
jmachowinski
force-pushed
the
add_cbg_executor
branch
from
273c322 to
66467b3
Compare
|
@jmachowinski can you add this to the executor unit-tests? |
jmachowinski
force-pushed
the
add_cbg_executor
branch
5 times, most recently
from
26fd9db to
430c2dd
Compare
|
Pulls: #3097
|
skyegalaxy
left a comment
skyegalaxy
left a comment
There was a problem hiding this comment.
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.
| } | ||
| } | ||
|
|
||
| // FIXME inform scheduler about remove cbgs |
There was a problem hiding this comment.
what are the side-effects of leaving this as is and not informing the scheduler here?
yes, that is the idea. |
|
|
||
| // 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 |
There was a problem hiding this comment.
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)
|
@jmachowinski we also need DCO for some of the newer commits |
jmachowinski
force-pushed
the
add_cbg_executor
branch
from
dab768c to
b7e95f7
Compare
skyegalaxy
left a comment
skyegalaxy
left a comment
There was a problem hiding this comment.
lgtm! can we rebase on latest rolling before running CI?
|
This currently depends on ros2/ament_cmake_ros#62 |
|
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 |
|
@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 |
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
…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>
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>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <J.Machowinski@cellumation.com>
jmachowinski
force-pushed
the
add_cbg_executor
branch
from
432ebe0 to
5c38948
Compare
|
Pulls: #3097
|
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
This introduced a subtle bug, were the guard condition of the callback group would not trigger a resync of the callback groups. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
This also fixes bugs were the executor would not wake up. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
|
Pulls: #3097
|
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
This is the same place as were they are set, so this makes the logic easier. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
This fixes a segfault, as the callbackgroup was deleted were the logic did not explect it. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
|
This fixes a performance regression as we would wake threads for no reason in single cbg processing. Signed-off-by: Janosch Machowinski <j.machowinski@cellumation.com>
|
5 participants
This commit adds the callback group events executor. It features:
Description
This moved the events cbg executor from cm_executors into rlcpp mainline.
Did you use Generative AI?
No