Support pre-set and post-set parameter callbacks in addition to on-set-parameter-callback.

[deepanshubansal01]

This PR allows supporting add_pre_set_parameter_callback and add_post_set_parameter_callback in addition to the existing add_on_set_parameter_callback for Node API.

Design document: https://github.com/ros2/rclcpp/blob/deepanshu/local-param-changed-callback-support/rclcpp/doc/proposed_node_parameter_callbacks.md
Demo node: ros2/demos#565
rclpy ticket: ros2/rclpy#966
Related discussion: #1190, #609

cc: @adityapande-1995 @jacobperron

Signed-off-by: deepanshu deepanshubansal01@gmail.com

[adityapande-1995]

Is this what happens currently when you chain parameters like this ? (Should behave like this ideally I believe)

param A (set successfully) --> param B (fails to set) --> param C (should not be triggered) 

[deepanshubansal01]

Is this what happens currently when you chain parameters like this ? (Should behave like this ideally I believe)

param A (set successfully) --> param B (fails to set) --> param C (should not be triggered) 

Is param B and param C being set inside post callback?

Yup, So I meant A's callback sets B, B's callback sets C.

I will check this to confirm the behaviour.

I checked, this is indeed the behaviour right now, so should be good.

[adityapande-1995]

Some related discussion here : #1190, #609

[jacobperron]

To recap from our discussion at the ROS 2 meeting:

• Our current plan is to introduce a "pre set parameters" callback, that can return a modified list of parameters that is then forwarded to the current "on set parameters" callback for validation. This should satisfy the use-case of updating additional parameters conditioned on changes to a parameter.
• The post callback is still valuable as a place to cause side-effects based on parameter changes, but we will avoid subverting the parameter recursion lock (which we no longer need since we can use the pre-callback to request changes to parameters based on another).

[deepanshubansal01]

Not sure but should we do something about add_pre_set_parameters_callback, add_on_set_parameters_callback, add_post_set_parameters_callback having almost similar code except that they are using different callback containers.

[deepanshubansal01]

One more thing which we should probably address is that since the pre_set_parameter_callback will be mainly used to modify the rclcpp::Parameter list, this might result in a different behaviour where the set_parameters_atomically should have been called non-atomically but is being called atomically now.

We can probably do something about this by not calling the registered pre_set_parameter_callback inside the set_parameters_atomically but outside it.

[deepanshubansal01]

I have made changes to the code based on review suggestions. I have also updated documentation for existing Node API's based on pre and post callbacks and can use a review for that specific commit d0bc26b

Thanks!

[jacobperron]

A few aesthetic things, otherwise LGTM!

[jacobperron]

Please trigger a CI job from ci.ros2.org and make sure it is green before merging.

[adityapande-1995]

CI :
build : --packages-above-and-dependencies rclcpp
test : --packages-above rclcpp

• Linux
• Linux-aarch64
• Windows

[jacobperron]

The same Windows test failure occurred on the most recent nightly: https://ci.ros2.org/view/nightly/job/nightly_win_rep/lastBuild/testReport/junit/launch_testing_examples.launch_testing_examples/check_multiple_nodes_launch_test/launch_testing_examples_check_multiple_nodes_launch_test/

[adityapande-1995]

Looks good !