-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 for #517: create a getRobotPose method on move_base #520
Conversation
…nstead of using that on the costmaps
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this PR -- but I don't like the breaking of ABI in the Costmap2dROS class (which will cause all sorts of hell on people with out-of-sync plugins, which is tough to track down). Here's my proposal:
-
I will cherry-pick this as-is into Lunar. There are far fewer users and we've still been fixing build issues over there, so stability should not yet be expected.
-
We update this PR to hard-code name+tolerance as parameters to MoveBase::getRobotPose() since ABI of MoveBase class does not matter.
-
Once we merge this updated version into indigo, I will cherry-pick to Kinetic
@@ -1181,4 +1181,46 @@ namespace move_base { | |||
controller_costmap_ros_->stop(); | |||
} | |||
} | |||
|
|||
bool MoveBase::getRobotPose(tf::Stamped<tf::Pose>& global_pose, costmap_2d::Costmap2DROS* costmap) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ABI in MoveBase doesn't matter, for Indigo+Kinetic -- let's add the name & transform_tolerance to this function signature, and then hard code them above (not great, but avoids ABI break). For Lunar -- let's go ahead and put this PR just this way (with the Costmap2DROS ABI change)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Btw, I think I understand better now the problem I was solving with this PR: As we look the global costmap before calling the global planner (
navigation/move_base/src/move_base.cpp
Line 450 in ada542f
boost::unique_lock<costmap_2d::Costmap2D::mutex_t> lock(*(planner_costmap_ros_->getCostmap()->getMutex())); |
As I mention on #441, I think this costmap locking should be responsibility of the planners, and last for the shorter possible time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw @mikeferguson, do you want me to apply the requested changes? Or will you do by yourself?
#520 for lunar [ABI Breaking]
This was merged into lunar in #601 and is a part of melodic and noetic as well. |
Signed-off-by: ARK3r <kermani.areza@gmail.com>
instead of using that on the costmaps.
Solves (I would say surprisingly) the
problem.
Change is very simple and hardly harmful. But I cannot tell what is exactly fixing nor why...