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

Update running controller goal also when we are preempting it #345

Merged
merged 1 commit into from
Aug 27, 2024

Conversation

corot
Copy link
Collaborator

@corot corot commented Aug 21, 2024

This PR partially addresses issue #323, for the case of preempting a controller execution with the same controller.
The problem is still there if the old and new goals have different controller.

I re-state here the problem and the solution

  1. force_stop_on_cancel is set to false (we let the controller to smoothly stop when canceled)
  2. goal running
  3. we cancel it; the controller slows down gently
  4. before stopping, a new controller goal arrives
  5. we cancel the previous execution and join the thread, waiting for the robot to stop
  6. we keep calling the canceled controller to get slowing down speeds
  7. BUT as we have blocked the main thread, the velocity we pass to it (provided by /odom topic) won't change
  8. If the controller happens to relay on this velocity to control it's slowing down and stopping, we can deadlock

So instead of canceling and joining, with this PR, things change from point 5

  1. we reuse the same slot and change the path of the current execution
  2. it's not canceled anymore, so it executes the new path

@corot corot force-pushed the js/update_path_on_preempting branch from fdaa96b to 4c79d68 Compare August 21, 2024 01:53
if ((slot_it->second.execution->getName() == goal_handle.getGoal()->controller ||
goal_handle.getGoal()->controller.empty()) &&
slot_it->second.goal_handle.getGoalStatus().status == actionlib_msgs::GoalStatus::ACTIVE)
(slot_status == actionlib_msgs::GoalStatus::ACTIVE || slot_status == actionlib_msgs::GoalStatus::PREEMPTING))
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -143,7 +143,7 @@ class AbstractActionBase
// if there is already a plugin running on the same slot, cancel it
slot_it->second.execution->cancel();

// TODO + WARNING: this will block the main thread for an arbitrary time during which we won't execute callbacks
// WARNING: this will block the main thread for an arbitrary time during which we won't execute callbacks
Copy link
Collaborator Author

@corot corot Aug 21, 2024

Choose a reason for hiding this comment

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

Still true, e.g. if the old and new goals have different controller.

Copy link
Collaborator

@siferati siferati left a comment

Choose a reason for hiding this comment

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

This PR only fixes the issue if the new goal has the same controller as the currently cancelling goal right? Maybe better to leave the issue open then even after merging this PR.

Glad to see the graceful cancel bug at least partially addressed though! 🥳

@corot
Copy link
Collaborator Author

corot commented Aug 22, 2024

This PR only fixes the issue if the new goal has the same controller as the currently cancelling goal right? Maybe better to leave the issue open then even after merging this PR.

correct
yes, it makes sense to keep the issue as open
thanks for the review!

@corot corot merged commit 367269c into master Aug 27, 2024
5 checks passed
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.

2 participants