Graceful cancel (preempt)

[Timple]

When preempting the "exe_path" action we would like to come to a graceful stop and then say we succeeded in cancelling.

Current implementation:

sets the cancel_ flag op true before we can handle any actions in our overloaded cancel function.

This results in an immediate zero publish on the cmd_vel topic.

Desired behavior:

Allow for a custom implementation upon receiving a preempt on an "exe_path" action. For example: We want to adhere to deceleration limits and still follow the current active path. So it could also take some time before the cancel is acknowledged.

[corot]

Sounds reasonable; did you try to move the
cancel_ = true;
to lines 242 and 244 just brefore both returns? Then the cancel call to your plugin will activate your graceful stop mode, during which you provide slowing down velocities until you are ready to stop. Then you return the cancel call, and MBF will stop the robot and terminate the exe_path action. All this should be possible because the cancel runs in a separated thread from the main execution.

[Timple]

I tried to move the line and that seemed to work. I was surprised they run in a separated thread.

However still a goal of 0.0 is send at the end. Which might or might not be what somebody wants.
We could also leave this up to the user (and maybe default it if no cancel implementation is available).

[corot]

Can make this line parametrizable, yes. We added on request by another user to ensure that the robot doesn't keep moving at a fixed speed if the controller dies

[Timple]

Would it be okay if this becomes the default if controller_->cancel() fails?
If it succeeds it would be up to the user to publishZeroVelocity() if they desire so.

Or perhaps always leave it up to the user to implement this.

[corot]

Would it be okay if this becomes the default if controller_->cancel() fails?
If it succeeds it would be up to the user to publishZeroVelocity() if they desire so.

Or perhaps always leave it up to the user to implement this.

I would say the last option is the simplest and more predictable, defaulting to pub the zero vel. @spuetz , your thoughts?
For para name: force_stop_on_cancel = true sounds good?

[spuetz]

Solved with #172

[spuetz]

@corot I ran into an error, since the default value of force_stop_on_cancel is false. It should be true, right? So this could cause some robots crashing into walls or something else.

[Timple]

I think false is fine. Because preempts also occur when new goals are send. This leads to a short 0.0 publish in between. See comment here: e4cdbb9#commitcomment-37274956

I can also remember a discussion where 'doing nothing should be the default': #173 (comment)

I think 'doing nothing by default' (so not publish 0.0 unless told to do so) is nicer. It also has been the default on every buildfarm release until now. But I don't have a very strong opinion about it.

[spuetz]

But, cancelling without sending a new goal will just tun off the controller and the last cmd_vel will be valid and the robot just moves on without stopping. (In the default case)

[corot]

Well, if the executive cancels the navigation, makes sense that it also stops the robot if needed. Moreover, the controller cancel method gets called, so he too has the opportunity to stop the robot.

…

On Fri, Apr 24, 2020 at 5:39 PM Sebastian Pütz ***@***.***> wrote: But, cancelling without sending a new goal will just tun off the controller and the last cmd_vel will be valid and the robot just moves on without stopping. (In the default case) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#171 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACOYMS3T7YLIEUJLBJ44YTROFF4RANCNFSM4KUTTBDA> .

[spuetz]

Yes, of course. I'm just saying we changed the behaviour. I had the problem: I updated the robot, which also installed the new version of MBF, and after that the robots crashed into walls. That should not happen.

[spuetz]

Therefore I suggested to set the default value of force_stop_on_cancel back to true.

[corot]

To me, now we have the right behavior. Your client code relies on a "wrong" (too strong word, for sure) behavior introduced on 16 October 2019
(see #173 (comment)) and fixed on March 24 2020. So I think you should change your client code instead.

[corot]

Can we close this?

[corot]

Hey @Timple, not sure if it was you who wanted to slow down during cancels; We allow exactly that with #274. Please tell me if you want more information.

[Timple]

We already do successfully for a year now.
So I have no idea why #274 is required.

I can compare our implementation with that one later today. Thanks for the ping!

[corot]

Is required if these 3 conditions meet

• you don't return fast on cancel method in your plugin
• you need to call spinOnce inside cancel (e.g. to get odom msgs)
• you get new goals for exe_path while cancel is still running

[Timple]

• We might, but I don't expect that often
• We don't? See below
• We don't do that, but good to know that is currently not supported!

This is the content of our cancel method:

bool TrackingPidLocalPlanner::cancel()
{
  // This function runs in a separate thread
  cancel_requested_ = true;
  cancel_in_progress_ = true;
  ros::Rate r(10);
  ROS_INFO("Cancel requested, waiting in loop for cancel to finish");
  while (active_goal_)
  {
      r.sleep();
  }
  ROS_INFO("Finished waiting loop, done cancelling");
  return true;
}

Maybe the rate sleep is identical to a spin()?

[corot]

Maybe the rate sleep is identical to a spin()?

No, afaik. But during this time your controller is not getting any callbacks, I guess;
In any case, the solution on #274 is much cleaner, as blocking the call to cancel doesn't sound right (yes,,, I know I suggested you to do so! 🙊).
Are you using master? Your current code will work even if we merge #274, but should be trivial to adapt for not blocking the cancel, and I can help if u want.

[Timple]

It could be that odom is not coming in. It might be something that got unnoticed (we decelerate open-loop, but the a curve won't be followed correctly then).

We'll migrate to the solution in #274. Thanks for the clear example in there!

We're using the apt version currently. Happy to evaluate master if the merge is completed.

[corot]

Welcome! Feedback will be highly appreciated!