completely shutdown dyn_params_handler_ (s)

[GoesM]

---

Basic Info

Info	Please fill out this column
Ticket(s) this addresses	#4496
Primary OS tested on	(Ubuntu, MacOS, Windows)
Robotic platform tested on	(Steve's Robot, gazebo simulation of Tally, hardware turtlebot)
Does this PR contain AI generated software?	(No; Yes and it is marked inline in the code)

---

Description of contribution in a few bullet points

result from experiments
In amcl node, the following code could completely shutdown the callback threads properly:

  // shutdown and reset dynamic parameter handler
  remove_on_set_parameters_callback(dyn_params_handler_.get());
  dyn_params_handler_.reset();

Future work that may be required in bullet points

Here's a todo list for my future work, I think I'd check out package by package:

• nav2_amcl (fixed)
• nav2_behavior_tree (no dynamic_param)
• nav2_behaviors (no dynamic_param)
• nav2_bringup (no dynamic_param)
• nav2_bt_navigator
• nav2_collision_monitor
• nav2_common
• nav2_constrained_smoother
• nav2_controller
• nav2_core
• nav2_costmap_2d
• nav2_docking
• nav2_dwb_controller
• nav2_graceful_controller
• nav2_lifecycle_manager
• nav2_map_server
• nav2_mppi_controller
• nav2_msgs (no dynamic param)
• nav2_navfn_planner
• nav2_planner
• nav2_regulated_pure_pursuit_controller
• nav2_rotation_shim_controller
• nav2_rviz_plugins
• nav2_simple_commander
• nav2_smac_planner
• nav2_smoother
• nav2_system_tests
• nav2_theta_star_planner
• nav2_util
• nav2_velocity_smoother
• nav2_voxel_grid
• nav2_waypoint_follower

This PR is opened as a demo. If there are no issues here, I will quickly check and correct all remaining packages, and then update this PR in one go.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Files	Coverage Δ
nav2_amcl/src/amcl_node.cpp	70.65% <100.00%> (+0.03%)	⬆️

... and 1 file with indirect coverage changes

[SteveMacenski]

LGTM, ping me back when all done and I can merge 🙂

[mergify]

@GoesM, 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]

@GoesM, 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).

[GoesM]

There'are still changes for plugins, I would add soon.

[mergify]

@GoesM, 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).

[SteveMacenski]

This seems to cause a number of crashes, according to the test results. I retriggered to see if it was a fluke, but it doesn't appear to be from my intuition

[GoesM]

It seems that in the test, some dyn_param_handler instances might not be registered, which can lead to related errors during resource release. I am currently making adjustments.

[mergify]

@GoesM, 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).

[GoesM]

The latest CI test failed in nav2_behavior_tree , which seems not related to my change (I think) ?

[SteveMacenski]

Lets try that once more then - retriggering. Can you also rebase? Maybe there's something weird going on with caching if this branch has an old base.

[mergify]

@GoesM, 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]

@GoesM, 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]

@GoesM, 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).

[GoesM]

opps .... So sorry for that many weird things happened after rebase, hoping it's the final change ....

[GoesM]

still failed in nav2_behavior_tree ,

I reviewed the dependencies of nav2_behavior_tree, and it appears that it does not depend on any of the packages I modified.
SO I feel this failure is likely unrelated to my changes.

Perhaps, to be more cautious, I could also submit new PR for additional try?

[SteveMacenski]

I triggered one more time... but a few issues to resolve here

[GoesM]

I triggered one more time

I notice that it still failed in nav2_behavior_tree. 😵

but a few issues to resolve here

Shall I create another fork and submit a new PR ? or I still push my lateset changes here ?

[SteveMacenski]

I notice that it still failed in nav2_behavior_tree. 😵

I just checked our nightlies, its failing there too for some reason. You can ignore that here, not your fault!

Shall I create another fork and submit a new PR ? or I still push my lateset changes here ?

You can just update this PR :-)

[SteveMacenski]

Great, thanks!! Merged :-) This was a good find!