Stop planner if the goal is cancelled

[bektaskemal]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	4139
Primary OS tested on	Ubuntu
Robotic platform tested on	Gazebo TB3 simulation
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

• Stops the planner execution if the goal is cancelled

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

[SteveMacenski]

Overall, LGTM!

[mergify]

@bektaskemal, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[mergify]

@bektaskemal, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[codecov]

Codecov Report

Attention: Patch coverage is 90.54054% with 7 lines in your changes are missing coverage. Please review.

Project coverage is 90.13%. Comparing base (865fd32) to head (a13ab59).
Report is 4 commits behind head on main.

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

Files	Patch %	Lines
nav2_planner/src/planner_server.cpp	73.33%	4 Missing ⚠️
nav2_smac_planner/src/smac_planner_lattice.cpp	83.33%	2 Missing ⚠️
nav2_navfn_planner/src/navfn.cpp	90.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4148      +/-   ##
==========================================
+ Coverage   89.85%   90.13%   +0.27%     
==========================================
  Files         432      432              
  Lines       19406    19455      +49     
==========================================
+ Hits        17438    17535      +97     
+ Misses       1968     1920      -48     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[SteveMacenski]

Overall, looks very good thank you for this great contribution!

A couple of small things to finish it up

• Documentation updates for new parameter for all appropriate algorithms. Add to Readmes (if they have a param list) & navigation.ros.org's configuration guide entries for the algorithms to add their params
• Migration guide entry on navigation.ros.org on Planner plugin API + new param and how to use
• I'm noticing alot of code cov warnings on lack of test coverage on this PRs diffs. We need things to be 90% tested, it seems like the dynamic parameter changes weren't added to their tests to exercise those lines, nor is cancel actually called on any of the algorithms as part of test to make sure that it does actually return early -- thus also that exception itself / planner server handling isn't exercised either https://github.com/ros-planning/navigation2/pull/4148/checks?check_run_id=22087057374 (or report comment above)

[SteveMacenski]

LGTM, just one set of important lines that aren't covered, the actual catching of the exception and checking that the correct state happens in the planner server.

We have some tests that trigger the others in https://github.com/ros-planning/navigation2/tree/main/nav2_system_tests/src/error_codes. I think you can just add the new plugin for cancel and check that its terminated correctly as a cancel state

Sorry to be a stickler on this one - these are just important systems to make sure are tested and have near full coverage in case of regression.

[bektaskemal]

We have some tests that trigger the others in https://github.com/ros-planning/navigation2/tree/main/nav2_system_tests/src/error_codes. I think you can just add the new plugin for cancel and check that its terminated correctly as a cancel state

If planner returns PlannerCancelled without a cancel request, the result status was ABORTED since we call terminate_all. So I tested it with an actual cancel request.
https://github.com/ros-planning/navigation2/pull/4148/files#diff-7d5b834e9aba5d3d110b02c975dc4c997b854ce7e92bdeb03d68108fa3071df1R120-R124

[SteveMacenski]

Great - a couple of tests failed - I don't think they're related to you - but rerunning CI just to check. I expect to be able to merge this before the end of today