added result codes for global planner

[jwallace42]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#3043
Primary OS tested on	Ubuntu
Robotic platform tested on	Gazebo

---

Description of contribution in a few bullet points

Description of documentation updates required from your changes

---

Future work that may be required in bullet points

For Maintainers:

• Check that any new parameters added are updated in navigation.ros.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[mergify]

@jwallace42, please properly fill in PR template in the future. @SteveMacenski, use this instead.

• Check that any new parameters added are updated in navigation.ros.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[SteveMacenski]

Generally speaking, LGTM with some refinement

[jwallace42]

Alright,

I cleaned it up a quite a bit. I haven't explicitly made a list for all the runtime errors that planners could through. I can try to work on that next. I was trying to get a rough structure out there :).

[jwallace42]

As for errors,

smac is currently the only planner that throws anything.

1. No costmap given
2. No valid start or goal
3. goal occupied with no tolerance
4. Start in lethal space

The other planners don't throw any runtime errors.

[mergify]

This pull request is in conflict. Could you fix it @jwallace42?

[SteveMacenski]

Sure, but there are other error conditions that might be helpful to know about, even if they don't exist as exceptions right now. For instance, NavFn has off map checks.

Also, lets do 1 subsystem at a time, so lets keep this with planning, open a different PR for controllers. This just makes reviewing less error prone

[jwallace42]

So,

the list is currently

int16 START_OCCUPIED=2
int16 GOAL_OCCUPIED=3
int16 NO_VALID_PATH=4
int16 TIMEOUT=5
int16 START_OUTSIDE_MAP=6
int16 GOAL_OUTSIDE_MAP=7
int16 START_IS_EQUAL_TO_GOAL=8
int16 TF_ERROR=9

I assume we will also want to add this to the compute path through poses action?

[codecov]

Codecov Report

Base: 82.85% // Head: 82.92% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (8dc55f2) compared to base (1a06b92).
Patch coverage: 22.03% of modified lines in pull request are covered.

❗ Current head 8dc55f2 differs from pull request most recent head ea41362. Consider uploading reports for the commit ea41362 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3146      +/-   ##
==========================================
+ Coverage   82.85%   82.92%   +0.07%     
==========================================
  Files         339      333       -6     
  Lines       15242    15113     -129     
==========================================
- Hits        12628    12533      -95     
+ Misses       2614     2580      -34     

Impacted Files	Coverage Δ
nav2_planner/src/planner_server.cpp	62.94% <0.00%> (-9.53%)	⬇️
..._smac_planner/include/nav2_smac_planner/a_star.hpp	50.00% <ø> (ø)
nav2_smac_planner/src/smac_planner_hybrid.cpp	94.73% <25.00%> (+1.05%)	⬆️
nav2_smac_planner/src/smac_planner_lattice.cpp	92.99% <25.00%> (+1.28%)	⬆️
nav2_smac_planner/src/smac_planner_2d.cpp	95.21% <40.00%> (+1.98%)	⬆️
nav2_core/include/nav2_core/exceptions.hpp	60.00% <66.66%> (+10.00%)	⬆️
...ree/plugins/action/compute_path_to_pose_action.cpp	84.00% <80.00%> (-2.96%)	⬇️
...ree/plugins/action/compute_path_to_pose_action.hpp	100.00% <100.00%> (ø)
nav2_smac_planner/src/a_star.cpp	96.79% <100.00%> (ø)
nav2_costmap_2d/src/footprint_subscriber.cpp	77.14% <0.00%> (-22.86%)	⬇️
... and 22 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[jwallace42]

@SteveMacenski let me know if there is anything else you might want me to do here(what do we want to do for code cov). I can also create a separate pr for the ComputePathThroughPoses error codes.

[SteveMacenski]

Generally speaking LGTM!

[jwallace42]

I think the order of throwing order should be

# Error codes
int16 TF_ERROR
int16 START_OCCUPIED
int16 START_OUTSIDE_MAP
int16 GOAL_OCCUPIED
int16 GOAL_OUTSIDE_MAP
int16 NO_VALID_PATH
int16 TIMEOUT

@doisyg any thoughts?

[jwallace42]

For what analogs exist here that require changes, if you make them to the controller PR, I can start reviewing that critically next week!

Once this gets in I will clean that pr up :).

[doisyg]

I think the order of throwing order should be

# Error codes
int16 TF_ERROR
int16 START_OCCUPIED
int16 START_OUTSIDE_MAP
int16 GOAL_OCCUPIED
int16 GOAL_OUTSIDE_MAP
int16 NO_VALID_PATH
int16 TIMEOUT

@doisyg any thoughts?

hm, I don't have a strong opinion on this. Maybe it is more logical to have the outside map test before the occupied one ? dk

[jwallace42]

Well, if you are outside the map, it can't be occupied and vis versa. So, the ordering of occupied vs outside doesn't matter because they mutually exclusive.

[doisyg]

Well, if you are outside the map, it can't be occupied and vis versa. So, the ordering of occupied vs outside doesn't matter because they mutually exclusive.

Agreed!

[SteveMacenski]

One comment however, it may be useful to clearly specify the priority of the error codes and to check their consistency. If both the start and goal are occupied, which code should be returned ? The order at which they are defined in the action message is probably a good implicit definition of their priority though.

Good question!

I propose:

# Error codes
int16 TF_ERROR #system failure
int16 START_OUTSIDE_MAP #request failure of application (semantic definition of points of interest failure)
int16 GOAL_OUTSIDE_MAP #request failure of application (semantic definition of points of interest failure)
int16 START_OCCUPIED #current state failure (run-time nature)
int16 GOAL_OCCUPIED #task failure if tol=0 (run-time nature)
int16 TIMEOUT # too difficult or impossible (run-time nature)
int16 NO_VALID_PATH #impossible (run-time nature)

As critical failures are highest priority (system cannot operate or request is totally invalid via occupation). Then we have the codes that relate to real run-time failures unable to find paths. Between invalid path and timeout, I don't have a strong opinion, but timeout seems "less serious" than total failure to me.

But the larger problem of how to do we take the "most serious one" isn't easy. A TF failure could happen in the planner after some start/goal outside map checks, but if we through from map checks, then we'd never know. I'd be interested to know what you would have in mind for this. Though, many of these are easier (e.g. do outside map checks before occupation checks).

[jwallace42]

But the larger problem of how to do we take the "most serious one" isn't easy. A TF failure could happen in the planner after some start/goal outside map checks, but if we through from map checks, then we'd never know. I'd be interested to know what you would have in mind for this. Though, many of these are easier (e.g. do outside map checks before occupation checks).

The planners now throw errors in the order stated above.

I think that order above should be a rough guideline. In the particular instance you mentioned I think even if we were able to collect all the exception at once and choose we would still need the handle the first exception thrown.

[SteveMacenski]

Got it - well if they're all in that order, then I think the last thing is to switch that order in the message definition + make a note in the message that that is the expected priority order