Redesign dynamic parameters patterns

[SteveMacenski]

It is brought to my attention dynamic parameter handling matured in Humble that allows us to use the add_post_set_parameters_callback instead of add_on_set_parameters_callback for processing parameter updates into member functions. This was brought up from a conversation started by @pepisg and myself on MPPI dynamic parameter handling issues and explained in ros2/rclcpp#2735 by Chris. We should not be using the on_set one anymore and should be using the post- condition once the parameter updates have been accepted.

The tl;dr minimum of this ticket is to: Update globally all of the add_on_set_parameters_callback to use add_post_set_parameters_callback instead and update the dynamic parameter callback if the signature changes. Update the on_set callback to contain any validity checking and post-set for actually making the state updates.

This is a really nice first contribution that touches alot of code!

Secondarily, many nodes are not as well organized with larger sets of parameters like MPPI and RPP with parameter handler objects. Along with the migration, we should consider making that also a global pattern of having all objects have parameter handlers to manage the parameter's state and dynamic callbacks to simplify the application code and have consistency architecturally.

Also a really nice first-contribution that gets folks into the architecture a little!

[Nils-ChristianIseke]

I will work on this.

[SteveMacenski]

Awesome, thanks @Nils-ChristianIseke !

[Sahas-Ananth]

Can I also help with this?

[Nils-ChristianIseke]

Can I also help with this?

@Sahas-Ananth Thanks for offering your help. I think at the moment i can handle it on my own. But i will reach out to you if i need support.

[Nils-ChristianIseke]

An example of which callbacks should be introduced and what their respective function is can be found here.

Parameter validations and auditing of the dynamic parameters should happen during adapting the architecture of the nodes, e.g:

• Are all parameters set as dynamic parameters? If not, should those that are missing be dynamic?
• Are there any other range checks that should be performed for parameters?

Preliminary list of all nodes that need to be adjusted ( I searched the repo for add_on_set_parameters_callback)
I will adapt this list

• nav2_regulated_pure_pursuit_controller/src/parameter_handler.cpp Dynamic param patterns #4971

• nav2_controller/plugins/pose_progress_checker.cpp

• nav2_mppi_controller/src/parameters_handler.cpp

• nav2_costmap_2d/src/costmap_2d_ros.cpp

• nav2_costmap_2d/plugins/plugin_container_layer.cpp

• nav2_docking/opennav_docking/src/docking_server.cpp

• nav2_dwb_controller/dwb_plugins/src/kinematic_parameters.cpp

• nav2_controller/src/controller_server.cpp

• nav2_costmap_2d/plugins/static_layer.cpp

• nav2_controller/plugins/simple_progress_checker.cpp

• nav2_rotation_shim_controller/src/nav2_rotation_shim_controller.cpp

• nav2_smac_planner/src/smac_planner_hybrid.cpp

• nav2_graceful_controller/src/parameter_handler.cpp

• nav2_controller/plugins/simple_goal_checker.cpp

• nav2_costmap_2d/plugins/inflation_layer.cpp

• nav2_smac_planner/src/smac_planner_2d.cpp

• nav2_mppi_controller/tools/parameters_handler.hpp

• nav2_docking/opennav_docking/src/controller.cpp

• nav2_collision_monitor/src/polygon.cpp

• nav2_amcl/src/amcl_node.cpp

• nav2_costmap_2d/plugins/obstacle_layer.cpp

• nav2_controller/plugins/stopped_goal_checker.cpp

• nav2_costmap_2d/plugins/voxel_layer.cpp

• nav2_planner/src/planner_server.cpp

• nav2_velocity_smoother/src/velocity_smoother.cpp

• nav2_planner/test/test_dynamic_parameters.cpp

• nav2_controller/test/test_dynamic_parameters.cpp

• nav2_waypoint_follower/src/waypoint_follower.cpp

• nav2_theta_star_planner/src/theta_star_planner.cpp

• nav2_collision_monitor/src/source.cpp

• nav2_smac_planner/src/smac_planner_lattice.cpp

• nav2_navfn_planner/src/navfn_planner.cpp

[SteveMacenski]

It might be nice to open PRs in 3-4 node segments so that the reviews can go faster and not block progress on all nodes for 1 node's small tweaks!

[Nils-ChristianIseke]

Continuing parts of the discussion from PR#4971 here.

FYI you might be interested in https://github.com/PickNikRobotics/generate_parameter_library. I'm not sure I love this method (what if we want more custom parts of our parameter handler, like wanting to mutex lock parameter access for multi-threaded situations where the dynamic parameter executor is different from the running executor) -- but maybe those are handled and/or you see value/inspiration here

I had completely forgotten that this existed. As far as I know, this is not currently supported, but there is a possibility that I overlooked it.
I remember enjoying working with the generate_parameter library. Maybe we should discuss whether we want to use this before I start making big changes.

[Nils-ChristianIseke]

@SteveMacenski , what are your thoughts on generate_parameter_library? I’d like to use this, but I’m not sure if your concern (what if we want more custom parts of our parameter handler, like wanting to mutex lock parameter access for multi-threaded situations where the dynamic parameter executor is different from the running executor)
prevents us from using it.

[SteveMacenski]

I think for now, we should update the stack with our gameplan and not use the generate parameter library. After doing a look over a few of our uses of dynamic parameters, there are times that updates to specific parameters (or classes of parameters) also need to trigger actions like reinitalizations of objects.

These also need to occur under the same mutex lock as changing the parameter so that other operations don't happen between when the parameter is updated and a potential post-change event callback is triggered that we could use to do so. Plus, I'm not sure if that library allows us to override dynamic parameter callbacks to check on values to do that process.

I think that framework works for some nodes but not all, so I'd rather keep all of Nav2 consistent in design than cherry pick some percentage to do in another way.

[Nils-ChristianIseke]

Great! Then I will continue to do that :) Thanks for writing down your reasoning!

[SteveMacenski]

Sorry for the delay, my github notifications tray has too many thought-inducing topics that I'm not quite able to get them all done every day :-) I appreciate the patience

[Nils-ChristianIseke]

@Sahas-Ananth After the first PR was merged (and can be used as template for future modifications) I would appreciate your help now :). You can have a look here to see how it should look at the end.
I recently started to work on nav2_controller.

[SteveMacenski]

Something brought up in #5100 would be to change all the plugins to skip looking at updates for parameters outside of their namespace. That way we only validity check parameters having directly to do with this algorithm rather than checking against all parameters in that node (of potentially many algorithms + the core server).

There may be some exceptions where a plugin looks at server-specific values like control frequency, but those are the exceptions we can put in each algorithm as-needed rather than the rule