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

Fixed a bug on isGoalReached method #290

Open
wants to merge 4 commits into
base: melodic-devel
Choose a base branch
from

Conversation

LucasNolasco
Copy link

@LucasNolasco LucasNolasco commented Mar 19, 2021

There was a bug on goal_reached_ flag where the robot wouldn't realize that a goal is reached. It was caused by a new global plan arriving before the move_base check if the goal is reached.

How the bug happens: The computeVelocityCommands method set the flag to true, then a new plan arrives and the setPlan method is called. Then, on the setPlan, the flag is set back to false before it was actually checked. So when the isGoalReached method is called, it keeps returning false.

Proposed Solution: The solution proposed here is to check the goal using the global plan, so it doesn't depend on the goal_reached_ flag anymore and even with a new plan be capable to realize that the goal is reached.

@amakarow
Copy link
Contributor

amakarow commented May 2, 2021

Sounds good, hope to get some time to test it.

@RohanDwivedi
Copy link

Is there a particular reason why this is not merged yet? I have ran into the same issue.

@VicenteQueiroz
Copy link

VicenteQueiroz commented Feb 6, 2024

I only had the early goal reach problem when I had planner_frequency > 0
With this solution, I would get a continuous error saying I had an empty global plan. My workaround was to, instead of clearing the global plan, I would return false, and the next iteration would have the correct goal.

if (isGoalReached())
{
  return false; // global_plan_.clear();
}

@LucasNolasco
Copy link
Author

I haven't returned to this issue in a long time, but I'd be willing to try to replicate the problem mentioned by @VicenteQueiroz if there's still any interest in merging this PR

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.

4 participants