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

Fix 3 related issues on move_base action recovering #343

Merged
merged 1 commit into from
Jun 3, 2024

Conversation

corot
Copy link
Collaborator

@corot corot commented Apr 26, 2024

  • move_base action get's stuck if recovery behavior patience is exceeded
  • If a recovery behavior fails, calling the next immediatly breaks simple action client
  • If the last recovery behavior fails, we return that as move_base error instead of the error that triggered recovering

I was debugging the first, but realized the 2 other on the process

The solution for the first is to treat patience exceeded the same way as behavior failure. Then I realized the other 2 errors.
The solution for the 2nd is rather hackish, but a proper solution would require to implement the move_base action, for which I don't have time. In any case the previous code was incorrect.

Comment on lines 480 to 484
// recovery failed but we still have behaviors; we cannot call the next here (simple action client
// will complain about transition to ACTIVE from DONE), so we try replanning again (wrong, but works)
case actionlib::SimpleClientGoalState::SUCCEEDED:
Copy link
Collaborator

@siferati siferati Apr 30, 2024

Choose a reason for hiding this comment

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

Suggested change
// recovery failed but we still have behaviors; we cannot call the next here (simple action client
// will complain about transition to ACTIVE from DONE), so we try replanning again (wrong, but works)
case actionlib::SimpleClientGoalState::SUCCEEDED:
// recovery failed but we still have behaviors; we cannot call the next here
// (simple action client will complain about transition to ACTIVE from DONE)
// so we try replanning again (wrong, but works)
[[fallthrough]];
case actionlib::SimpleClientGoalState::SUCCEEDED:

S: add [[fallthrough]] attribute to make it clear what the comment is referring to.
I was a bit confused at first until I noticed it was referring to the lack of a break statement 😅
I think this requires C++17 though, so if you don't want to change CMakeLists.txt then maybe add it commented out

S: better text line breaks

If a recovery behavior fails, calling the next immediatly breaks simple action client
If the last recovery behavior fails, we return that as move_base error instead of the error that triggered recovering
@corot corot merged commit e8c7718 into master Jun 3, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants