Parameter callback corrections

[JMoore5353]

We currently use the add_on_set_parameter_callback API to create a parameter callback to update the values stored in the param_manager when ROS2 parameters are changed.

However, according to the ROS2 docs regarding parameter changes, it seems that a on_parameter_event callback would be better. This is because our callbacks definitely have side effects, and therefore run the risk of desyncing the internal stored parameters with the ROS2 parameters. This would happen if a callback rejected the update.

I think our code works as intended now because we usually only change one parameter at a time (since launch files don't trigger the callback created with the add_on_set_parameters call). Furthermore, our code only rejects changes when a parameter doesn't exist. We also don't currently chain callbacks, so there is no risk of a future parameter callback called down the line rejecting the parameter change already accepted by the callback up the line.

Despite this, however, it would probably be better to implement the on_set_parameter callback to avoid potential issues.

@iandareid @bsutherland333 Is this how you understand the ROS2 documentation about parameter callbacks as well?

[iandareid]

This is how my reading of it went, please let me know if you feel differently @JMoore5353.

I think we are setting the parameters correctly, what I think the on_parameter_event does is allows us to change things like a timer or controller or something else, based on the parameter's success or failure.

I think that the syncing becomes a problem when that parameter does more than just change a value used in a calculation. For example, when we have to reset the timer if the frequency parameter changes, we create a new timer object. If that was not in the add_on_set_parameters callback, we could reset the timer even if the parameter was rejected. In our case I do not think this would be catastrophic, but if we had a parameter that would switch the control type to total_energy and also updated the timer in the same call, but the switch to total_energy was rejected for some reason then the timer would be changed but that would be out of sync with the actual total_energy parameter that would still be in its original configuration.

That was my understanding. I think that if the params make changes to the class then we should consider how to use the add_on_set_parameters, but as we have been using it I think it is correct.

Let me know what you think.

[iandareid]

Essentially I think we only need to be concerned if we have conditional parameter changes, change parameter y if parameter x is changed.

[JMoore5353]

Here are some examples of the 3 different types of parameter callbacks.

https://github.com/ros2/demos/blob/jazzy/demo_nodes_cpp/src/parameters/set_parameters_callback.cpp