Skip to content
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

Allow the controller to handle cancel if properly implemented #274

Merged
merged 1 commit into from
Oct 11, 2021

Conversation

corot
Copy link
Collaborator

@corot corot commented Jul 12, 2021

Possibly a much safer solution for #273. This allows smooth cancels with decreasing velocities without risking the deadlocks reported in #273. The controller becomes responsible of slowing down by returning decreasing speeds on computeVelocityCommands and SUCCESS outcome. When he considers that the robot can stop, returns zero velocity and CANCELED outcome.

Trivial example implementation for TEB:

image

If the controller doesn't implement the controller plugin API cancel method, or returns false, we use the normal stop method with an abrupt zero-velocity.

⚠️⚠️⚠️
One corner case is when we cancel the controller but it reaches the goal while slowing down. In this case, we will report success, which is a sensible result because we accomplish the requested goal despite the cancel, but one could argue that we should return preempted.
⚠️⚠️⚠️

Example:

  1. send goal to exe_path
  2. cancel it, slowing down
  3. send the same goal before it stops --> we preempt the canceling and start executing the new goal
cancel.online-video-cutter.com.mp4

@dorezyuk
Copy link
Contributor

@corot I had a look at the bug and it looks not good. I've dead locked the node with your code and generated a coredump. Interestingly in the coredump, I didn't find the thread execution the cancel method anymore - which explains why the cancel method never returns...

Since its related to the spinning (I'm not sure if spinning within the plugin is actually a good idea but this is not the point), I've created a dedicated callback queue just for our servers - and this seems to solve the problem.

Could you check out https://github.com/magazino/move_base_flex/tree/dima/callback_queue (without the here mentioned changed to TEB) to see if it works for you?

@corot
Copy link
Collaborator Author

corot commented Jul 13, 2021

@corot I had a look at the bug and it looks not good. I've dead locked the node with your code and generated a coredump. Interestingly in the coredump, I didn't find the thread execution the cancel method anymore - which explains why the cancel method never returns...

you mean, u had a deadlock with this PR? And which controller? default TEB or my example for slowing down?

@corot
Copy link
Collaborator Author

corot commented Jul 13, 2021

Interestingly in the coredump, I didn't find the thread execution the cancel method anymore - which explains why the cancel method never returns...

wooooo,,,, I knew something superfishy was happening,,, I though because of stopping the execution thread, but not
any idea why a thread can disappear like that?

@dorezyuk
Copy link
Contributor

you mean, u had a deadlock with this PR? And which controller? default TEB or my example for slowing down?

Your TEB example from #273

@corot
Copy link
Collaborator Author

corot commented Jul 13, 2021

Your TEB example from #273

Ah,,, yes, yes. I thought u meant this PR 😌

It's still interesting to investigate that strange problem, but this PR is a much cleaner solution to smooth cancels, imho

@corot
Copy link
Collaborator Author

corot commented Jul 15, 2021

Could you check out https://github.com/magazino/move_base_flex/tree/dima/callback_queue (without the here mentioned changed to TEB) to see if it works for you?

But wait,,, this can be much simpler, right? Not enough to run an async spinner instead of spin?

In any case, yes, sounds reasonable to run this multi-threaded. Did you try to reproduce the freezing with this change, but removing the ros::spinOnce();?

@dorezyuk
Copy link
Contributor

But wait,,, this can be much simpler, right? Not enough to run an async spinner instead of spin?

Also a good idea - to me both seems fine.

Did you try to reproduce the freezing with this change, but removing the ros::spinOnce();?

Not sure about which spinOnce we are talking now 😕 About the one in cancel or in the main? It worked for me with the spinOnce in the cancel

@corot
Copy link
Collaborator Author

corot commented Jul 15, 2021

Not sure about which spinOnce we are talking now About the one in cancel or in the main? It worked for me with the spinOnce in the cancel

Then, can u approve this PR? Also @spuetz, can you test and approve this?

As for the async spinner... I see the point, but is a completely different issue and so we shouldn't mix with this PR

@corot
Copy link
Collaborator Author

corot commented Jul 20, 2021

@dorezyuk , @spuetz , Description updated; take a look at the ⚠️ section and tell me if u ratify my choice

@@ -254,18 +254,24 @@ bool AbstractControllerExecution::reachedGoalCheck()

bool AbstractControllerExecution::cancel()
{
// request the controller to cancel; it returns false if cancel is not implemented or rejected by the plugin
if (!controller_->cancel())
// Request the controller to cancel; returns true if he takes care of stopping, returning CANCELED when done,
Copy link
Contributor

Choose a reason for hiding this comment

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

controller seem to be very male in this pr 😁

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mmm.... I think I refer always as "him"... 🤔
I'll start using it

@corot corot force-pushed the ctrl_handled_cancel branch 2 times, most recently from 0c8decd to 392c67c Compare July 21, 2021 02:10
@spuetz spuetz merged commit db86b77 into master Oct 11, 2021
@spuetz spuetz deleted the ctrl_handled_cancel branch October 26, 2021 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants