-
Notifications
You must be signed in to change notification settings - Fork 152
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix early preempt when planning #597
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #597 +/- ##
==========================================
- Coverage 58.82% 56.44% -2.37%
==========================================
Files 91 131 +40
Lines 8623 10684 +2061
Branches 0 951 +951
==========================================
+ Hits 5072 6030 +958
- Misses 3551 4607 +1056
- Partials 0 47 +47 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When preempting early, this does not work because the preempt flag is reseted inside the plan() method.
I don't yet understand the race condition. Do you mean, preempt_requested_
may be set before it is initialized in plan()
and then the request is lost?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like that the user needs to remember to reset the preempt_requested_ flag.
What about resetting the flag when leaving Task::plan()?
moveit::core::MoveItErrorCode plan(size_t max_solutions = 0); | ||
/// interrupt current planning (or execution) | ||
void preempt(); | ||
/// interrupt current planning (or execution) or reset the preempt flag (false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Execution cannot be preempted with this call.
/// interrupt current planning (or execution) or reset the preempt flag (false) | |
/// interrupt current planning or reset the preempt flag (false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would be more clean. We would need a try catch block and catch everything in the plan() to reset the value.
Yes exactly. |
@rhaschke This would work in most of the cases (see last commit). But consider the following case:
So I don't think it can be done it automatically... EDIT Unless we always need to reset the task between 2 consecutive plan(), we could reset the |
6bbd283
to
2b82592
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious how you manager to call preempt() before plan() actually started its work.
I do see the theoretical possibility, but practically that shouldn't occur.
core/test/test_container.cpp
Outdated
t.add(std::make_unique<GeneratorMockup>(PredefinedCosts::constant(0.0))); | ||
t.add(std::make_unique<TimedForwardMockup>(timeout)); | ||
|
||
// preempt before any stage computation is done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// preempt before any stage computation is done | |
// preempt before preempt_request_ is reset in plan() |
core/test/test_container.cpp
Outdated
|
||
// preempt before any stage computation is done | ||
{ | ||
std::thread thread{ [&ec, &t] { ec = t.plan(1); } }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about introducing a small sleep before t.plan() in order to emphasize the issue even more?
core/src/task.cpp
Outdated
pimpl()->preempt_requested_ = false; | ||
throw; // rethrow the original exception |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The try-catch-block is the wrong tool to cleanup before leaving the function scope: it is only considered if an exception is thrown, not when leaving the function via return. Please revert this.
What you need is a scope guard:
https://stackoverflow.com/questions/50182244/simple-way-to-execute-code-at-the-end-of-function-scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would using this external library be acceptable ? At a first glance it seems well tested and easy to use.
2b82592
to
2da7ff8
Compare
2da7ff8
to
38a4801
Compare
A custom CMakeLists.txt is used to create the scope_guard libray. The original cmake libray file is only used to setup tests. See ricab/scope_guard#3.
38a4801
to
69b7f22
Compare
Remove the initialization of preemt_requested_ in Task::plan(). Make preempted request atomic. Added method to reset the preempt request.
69b7f22
to
6761c68
Compare
We started using BehaviorTree.CPP to create MTC tasks. I was adding a test to test the preempting of a BT node when planning. Here is the sample. <root BTCPP_format="4" >
<BehaviorTree ID="MainTree">
<Sequence>
<Script code="test := true"/>
<ReactiveSequence name="root">
<ForceFailure _skipIf="test">
<Script code=" result:='error' " />
</ForceFailure>
<Script code=" result:='error' " _onSuccess="test = false" />
<PlanMTCTask task="{mtc_task}" max_solutions="1000" />
</ReactiveSequence>
</Sequence>
</BehaviorTree>
</root> So planning is first started in another thread. Then the main thread will check if the BT node |
@rhaschke Ready for another review with all changes requested. This adds an external dependency to the scope_guard library. |
If you agree to my simplification, this is ready to merge. |
LGTM and thanks for the review. |
We have a usecase in which we need to plan the task in another thread and sometimes preempt it. When preempting early, this does not work because the preempt flag is reseted inside the
plan()
method.This fix makes the preempt flag changeable from the user. So for the first planning nothing changes. But users must reset the preempt flag before each subsequent planning (if the preempt method is used).
Even with this fix, preempting a task can take some time. The preempt flag is only checked at the start of the plan compute loop. #312 seems far away, but I think we could reduce the time it takes to preempt a task without too much changes ?