Proper way of implementing dynamic parameters ?

[doisyg]

Bug report

Required Info:
I am a bit confused about what is the best way of handling dynamic parameters. I just realized the way it is implemented for Smac2D, DWB plugins, NavFn, goal checker and and progress checker using rclcpp::Subscription<rcl_interfaces::msg::ParameterEvent>::SharedPtr on_parameter_event to register a rcl_interfaces::msg::ParameterEvent::SharedPtr callback is probably wrong.
The callback is being called for every parameter change on any node ! Performance aside, this could create the issue of setting parameters in multiple node when targeting a single node. I think nobody noticed it until now because we are using this method only in plugins which requires filtering the parameter name based on the plugin name (for instance if (name == name_ + ".use_astar")). (And also because we are very few using dyn params)
My bad, I am the one who started using on_parameter_event in the first place.
I suggest using OnSetParametersCallbackHandle::SharedPtr add_on_set_parameters_callback(OnParametersSetCallbackType callback) to register a std::vector<rclcpp::Parameter> callback instead. I made a quick experiment using this method on Smac2d and it is working as expected. @SteveMacenski what do you think ?
Once we agree on the proper way of handling dynamic parameters I am okay to make a PR changing it everywhere in nav2.

Steps to reproduce issue

Put an info message at the beginning of one of the on_parameter_event_callback in nav2, it will be displayed everytime a parameter is changed anywhere

Expected behavior

Efficient and error proof dynamic parameters handling

Actual behavior

on_parameter_event_callbackare called for every parameter change

[SteveMacenski]

Can we move this discussion to #956 to keep all of this topic in 1 thread for future onlookers (and maintainers' organization)?

If you could offer a code snippet of how one of the simple changes would look like, that would help. You don't have to do every parameter in some node, just a couple to get the idea of what you're proposing looks like. The changing parameters APIs of ROS 2 keeps changing so my guess is that wasn't on the mind when we started. I know there's an API for registering callbacks for individual parameters (which I think this is asking for) but there's also a parameter filter object floating around too https://docs.ros2.org/foxy/api/rclcpp/classrclcpp_1_1ParameterEventsFilter.html. There was also a design discussion around topic filters for parameters so that it only triggers callbacks for the node of interest instead of all nodes, but that's not tangible yet.

If you make these changes, may I suggest another change at the same time to make our lives easier in the future. It's not unusual for systems to have a separate parameters struct (and hpp/cpp file) of the parameters in a system and are gotten there. In this case, then we could move all the dynamic parameter callbacks to this file and out of the main algorithm file. Values from the configuration are then "gotten" from the struct param_struct.my_param that is stored as a class member instance. We could have a get() function to get the values or something. Internal to this structure, we could have the locking mechanisms for the parameter changes so that its not exposed to the larger object (e.g. plan() isn't blocked by a parameter callabck, but getting value my_param is blocked within plan() while my_param's callback function is executing or use atomics so that we need no locking)

[doisyg]

Can we move this discussion to #956 to keep all of this topic in 1 thread for future onlookers (and maintainers' organization)?

Moving and closing