Repository navigation
Halt the running branch before starting another asynchronous action - #1217
Quchaosheng wants to merge 1 commit into
Conversation
ReactiveSequence and ReactiveFallback halt the siblings of a child only after that child returned RUNNING. When the new child starts an asynchronous action in its onStart(), the action of the branch that is about to be preempted is still alive, so for a short time two actions run concurrently. With the ROS action wrappers that means that a new goal is sent before the previous one is cancelled, which is the race described in issue BehaviorTree#1031. Let an asynchronous node stop the branches that are still RUNNING right before it transitions out of IDLE, walking up to the root. Parallel is the only control node designed to keep several children running at the same time, so it stops the walk. To know the ancestors, every node now stores the parent that the parser sets when the children are added to the tree.
|
A gentle first ping on this one — it has not had a review yet. The race is the one from #1031, and the diff is the direction that issue invited. Reproduced here with The test in the PR fails without the fix and passes with it; the full suite is 523/523. CI is green on the current head as well. Nothing here needs an answer from me — it needs a reviewer's time. If the parent-pointer approach is not the shape you want for this, say so and I will rework it. |
|
I reran the full suite from this branch on current master, not just the new test: cmake -S . -B build -DBTCPP_SQLITE_LOGGING=ON -DBUILD_TESTING=ON 523 tests from 61 test suites ran. PASSED 523. The branch is based on ca4edbf and still merges cleanly into today's master; nothing since then touched the eight files in the diff. @facontidavide the design question is whether a node may walk up to the root and halt siblings before its own onStart(). The alternative in #93 was to have every control node know about asynchronous children (propagateHalt); this keeps that knowledge in the action node instead. Parallel and ParallelAll opt out so their branches still run concurrently. If you prefer the other shape, or want this limited to StatefulActionNode instead of also ThreadedAction and CoroActionNode, tell me which and I will change it. If the direction is fine, a review is all it needs. |
ReactiveSequence and ReactiveFallback halt the siblings of a child only after that child returned RUNNING. When the new child starts an asynchronous action in its onStart(), the action of the branch that is about to be preempted is still alive, so for a short time two actions run concurrently. With the ROS action wrappers that means that a new goal is sent before the previous one is cancelled, which is the race described in issue #1031.
Let an asynchronous node stop the branches that are still RUNNING right before it transitions out of IDLE, walking up to the root. Parallel is the only control node designed to keep several children running at the same time, so it stops the walk.
To know the ancestors, every node now stores the parent that the parser sets when the children are added to the tree.
This follows the direction invited in #1031 for reworking #93, without the propagateHalt coupling: the async node asks its ancestors to release the other branches, instead of every control node having to know about asynchronous children.
Reproduced with ReactiveSequence[ ReactiveFallback[BatteryOk, GotoCharging], GotoKitchen ]:
before: GotoKitchen:onStart ... GotoCharging:onStart GotoKitchen:onHalted
max concurrent actions = 2
after: GotoKitchen:onHalted GotoCharging:onStart
max concurrent actions = 1
The new test fails without the fix (max concurrent actions 2, halt after start) and passes with it. All 523 tests pass.
Fixes #1031