Implement the new move base flex interface for the global planner #774

vsteinb · 2018-07-31T07:53:24Z

Added a second inheritance to GlobalPlanner from the mbf_costmap_core::CostmapPlanner interface to be able to use move_base_flex while backwards compatible to move_base.

mikeferguson · 2018-07-31T13:09:15Z

So, this really isn't going to work -- we can't depend on things from the MBF repo, since the MBF repo has packages that depend on at least costmap_2d, base_local_planner, and nav_core.

spuetz · 2018-08-01T13:49:37Z

Aaa circular dependencies. :/ @corot How could we handle that?

corot · 2018-08-01T14:24:42Z

I think build just fails because mbf_msgs dependency is missing; same for the local planner.

I don't see any circular dependency; true mbf_costmap_core depends on costmap_2d, but also does nav_core

mikaelarguedas · 2018-08-01T14:45:24Z

I believe what @mikeferguson means is that the ROS release infrastructure works on a per repository basis.
So circular dependencies should not only be avoided at the package level but also at the repository level. And this PR will introduce a repository level circular dependency between navigation and move_base_flex that will prevent them from being released

spuetz

There are some things to change. And some things also missing. mbf_msgs dependency, and cost is not computed or set and message is not set. The rest looks fine to me.

spuetz · 2018-08-01T14:32:03Z

global_planner/src/planner_core.cpp

@@ -271,7 +286,7 @@ bool GlobalPlanner::makePlan(const geometry_msgs::PoseStamped& start, const geom
    if (!costmap_->worldToMap(wx, wy, goal_x_i, goal_y_i)) {
        ROS_WARN_THROTTLE(1.0,
                "The goal sent to the global planner is off the global costmap. Planning will always fail to this goal.");
-        return false;
+        return mbf_msgs::GetPathResult::INTERNAL_ERROR;


INVALID_GOAL would be the right outcome number code here.

spuetz · 2018-08-01T14:34:06Z

global_planner/src/planner_core.cpp

@@ -256,7 +271,7 @@ bool GlobalPlanner::makePlan(const geometry_msgs::PoseStamped& start, const geom
    if (!costmap_->worldToMap(wx, wy, start_x_i, start_y_i)) {
        ROS_WARN(
                "The robot's start position is off the global costmap. Planning will always fail, are you sure the robot has been properly localized?");
-        return false;
+        return mbf_msgs::GetPathResult::INTERNAL_ERROR;


INVALID_START would be the right number code here.

spuetz · 2018-08-01T14:35:47Z

global_planner/src/planner_core.cpp

+    }
+
+    if (canceled_) {
+        ROS_ERROR("This planner was canceled!");


The error msg is a bit unspecific.

spuetz · 2018-08-01T14:40:46Z

global_planner/src/planner_core.cpp

+}
+
+uint32_t GlobalPlanner::makePlan(const geometry_msgs::PoseStamped &start, const geometry_msgs::PoseStamped &goal,
+                  double tolerance, std::vector<geometry_msgs::PoseStamped> &plan, double &cost, std::string &/*message*/) {


/message/ ?

where is the cost computed

where is the message set? -> The message should be defined following to the error meaning.

spuetz · 2018-08-01T14:41:19Z

global_planner/package.xml

@@ -24,6 +24,7 @@
  <build_depend>pluginlib</build_depend>
  <build_depend>roscpp</build_depend>
  <build_depend>tf</build_depend>
+  <build_depend>mbf_costmap_core</build_depend>



mbf_msgs is missing here

spuetz · 2018-08-01T14:41:36Z

global_planner/package.xml

@@ -34,8 +35,10 @@
  <run_depend>pluginlib</run_depend>
  <run_depend>roscpp</run_depend>
  <run_depend>tf</run_depend>
+  <run_depend>mbf_costmap_core</run_depend>
  <export>


spuetz · 2018-08-01T14:42:12Z

global_planner/CMakeLists.txt

@@ -8,6 +8,7 @@ find_package(catkin REQUIRED
    dynamic_reconfigure
    geometry_msgs
    nav_core
+    mbf_costmap_core
    navfn


mbf_msgs is missing here

mikeferguson · 2018-08-02T01:56:56Z

Yes, what @mikaelarguedas mentioned is the problem -- since we certainly want to release debians.

I think the proper way to get this out would be to include a minimal set of headers in nav_core (Ala this PR: #605). Since we haven't sync'd the first round of debians yet -- it is definitely still early enough to get this into Melodic.

Or split the core stuff out of move_base_flex into a new repo (but before I would support navigation depending on said code, I'd suggest that move_base_flex_core be part of ors-planning, so that if in the future you guys decide to stop maintaining it, we can still release it and the navigation stack).

jspricke · 2018-08-02T12:35:55Z

I would propose to not change navigation and handle the extended plugins at runtime in mbf.

corot · 2018-08-02T13:01:56Z

That's what we currently do; when dealing with "old-stile" API (nav_core) plugins, MBF wraps them so they can be used indistinctly from "new-stile" API (mbf_core) plugins. Problem is that we miss the richer interface, as nav_core plugins don't provide error code or message, just success/failure, nor can be canceled (plus other new fancy stuff).

corot · 2018-08-02T13:06:19Z

So as I see, there are several possibilities (all for melodic and later, kinetic will just remain as it is):

add mbf_core and mbf_msgs to ROS navigation repo, and upgrade the plugins to the new interface (backward compatible!). Main problem is the increased complexity (possibly confusing for newcomers) of ROS navigation stack
move the plugins to a separated repo, so the can freely depend on both ROS nav (core, msgs, costmaps...) and MBF (mbf_core, mbf_msgs). Probably too much work for the maintainers.
duplicate the plugins and re-release with a mbf_ prefix. Duplicate == 👿
let things as they are and make a common interface (both C++ plugin and ROS API) for ROS 2.0 navigation.

mikeferguson · 2020-03-03T18:37:24Z

Going to close this since we never resolved the circular dependency - and we certainly aren't going to make this change in Kinetic

Implement the new move base flex interface for the global planner

b5a16da

spuetz mentioned this pull request Aug 1, 2018

Dwa planner mbf #772

Closed

spuetz suggested changes Aug 1, 2018

View reviewed changes

DLu added planners kinetic PRs for kinetic-devel labels Feb 26, 2019

amrp1 mentioned this pull request May 27, 2019

No costmaps published with new plugin naturerobots/move_base_flex#129

Closed

mikeferguson closed this Mar 3, 2020

corot mentioned this pull request Mar 26, 2021

How to get different outcome codes in GetPath result? naturerobots/move_base_flex#253

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the new move base flex interface for the global planner #774

Implement the new move base flex interface for the global planner #774

vsteinb commented Jul 31, 2018

mikeferguson commented Jul 31, 2018

spuetz commented Aug 1, 2018

corot commented Aug 1, 2018

mikaelarguedas commented Aug 1, 2018

spuetz left a comment

spuetz Aug 1, 2018

spuetz Aug 1, 2018

spuetz Aug 1, 2018

spuetz Aug 1, 2018

spuetz Aug 1, 2018

spuetz Aug 1, 2018

spuetz Aug 1, 2018

mikeferguson commented Aug 2, 2018

jspricke commented Aug 2, 2018

corot commented Aug 2, 2018

corot commented Aug 2, 2018

mikeferguson commented Mar 3, 2020

Implement the new move base flex interface for the global planner #774

Implement the new move base flex interface for the global planner #774

Conversation

vsteinb commented Jul 31, 2018

mikeferguson commented Jul 31, 2018

spuetz commented Aug 1, 2018

corot commented Aug 1, 2018

mikaelarguedas commented Aug 1, 2018

spuetz left a comment

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

spuetz Aug 1, 2018

Choose a reason for hiding this comment

mikeferguson commented Aug 2, 2018

jspricke commented Aug 2, 2018

corot commented Aug 2, 2018

corot commented Aug 2, 2018

mikeferguson commented Mar 3, 2020