Fix for #523: global planner called much more times than expected #524
Conversation
…or time allowed by planner_patiente
|
@corot , would you mind explaining the fix? From the diff, it seems as some combination with locks, but I don't understand it completely. I tested the behavior, and it is as described: the move base is being called only 1+max_planning_retries for each recovery behavior. |
|
Hi @Gergia, sorry for not being clear. All the issue is better explained in issue #523 and this ROS answer. The change is quite minimal, once whitespaces are ignored: Without the change, note that there's nothing preventing to keep calling the planner even if we are not in PLANNING state. The change makes that planning thread only calls the planner while current state is PLANNING, up to max_planning_retries+1 times or during the time allowed by planner_patiente. If not, it is blocked in the wait_for_wake planner_cond_.wait(lock); (saving CPU) Hope is clearer now |
|
Hi all, anything preventing this PR to get merged? |
|
PR also on kinetic |
move_base/src/move_base.cpp
Outdated
| @@ -619,54 +619,59 @@ namespace move_base { | |||
| wait_for_wake = false; | |||
| } | |||
| ros::Time start_time = ros::Time::now(); | |||
|
|
|||
| //time to plan! get a copy of the goal and unlock the mutex | |||
| geometry_msgs::PoseStamped temp_goal = planner_goal_; | |||
| lock.unlock(); | |||
There was a problem hiding this comment.
This lock.unlock() call should really move with the temp_goal declaration. We should get the goal WHILE under lock. Of course, that means we need to add an "else{ lock.unlock() }" to the "if (state_==PLANNING)" below, which isn't ideal (it's convoluted) but it is at least thread-safe
move_base/src/move_base.cpp
Outdated
| } | ||
| lock.unlock(); | ||
| } |
There was a problem hiding this comment.
This is where we would add "else { lock.unlock() }".
I did test this lock change, and it looks all good (also tested the whole feature in general).
Like that? Sorry, I edited directly the PR; hope I broke nothing!
|
Thanks! LGTM -- I'll wait for CI to complete again, and then squash-merge for ease of cherry-picking into K+L. |
Same change as in ros-planning#524
|
I think this PR breaks the ability to plan with positive |
|
I think you are right 🙈 Will try to fix it asap; sorry for the gaffe |
…ning#539) * Implements issue ros-planning#496: add a max_planning_retries parameter as an alternative to planner_patience to limit the failed calls to global planner * Includes ros-planning#498 and ros-planning#524 for kinetic and above
…ning#539) * Implements issue ros-planning#496: add a max_planning_retries parameter as an alternative to planner_patience to limit the failed calls to global planner * Includes ros-planning#498 and ros-planning#524 for kinetic and above
* Update the default ports Related to ros-navigation/navigation2#4060 * fix tables
So now planner is called 1 + max_planning_retries in a first try, and then 1 + max_planning_retries after every recovery behavior. Or up to planner_patiente seconds after every recovery. I think this is the expected operation.
Seems to work fine now, but move_base.cpp code is quite messy, so probably needs at least one independent tester.