Added extra documentation and clarifications.

[jrutgeer]

Closes ros2/rclcpp#2317

Some points that are not clear to me:

1. The example declares param1 and param2 as well as reads their values into member variables before the callbacks are registered.

   • This seems to be a logical error, as this implies that it is possible to load parameter values from the command line or launch file, that are incompatible with the criteria in the on_set_parameters_callback,
   • So: Should the callbacks be registered first, and only afterwards the parameters declared?

2. Is it mandatory to keep the shared_ptr to the callback handles for the callbacks to work?
   Or is this only necessary in case you need to call
   this->remove_on_set_parameters_callback(callback_handle_.get()); lateron?

3. The documentation states: "Since multiple 'set parameter' callbacks can be chained, there is no way for an individual callback to know if a later callback will reject the update."

   • It is not clear to me what is meant with "Since multiple 'set parameter' callbacks can be chained",
   • Based on above signature, where the callback handle is passed to remove_on_set_parameters(), I assume that it is possible to call add_X_set_parameters_callback() multiple times, and hence "chain" multiple callbacks. Correct?
   • If so: What is the order that the callbacks are called: in the order that they were added?
   • And: first all the pre_set callbacks, then all on_set callbacks and then all post_set callbacks, correct?

[jrutgeer]

Should the callbacks be registered first, and only afterwards the parameters declared?

I gave it a try and I think the answer is 'yes'.

In its current form, it is possible to set incompatible parameter values.

In case the callbacks are registered first, and incompatible values are specified, then an exception is thrown:

$ ros2 run demo_nodes_cpp set_parameters_callback --ros-args -p param1:=8.0 
terminate called after throwing an instance of 'rclcpp::exceptions::InvalidParameterValueException'
  what():  parameter 'param1' could not be set: cannot set 'param1' > 5.0
[ros2run]: Aborted
$

[clalancette]

• So: Should the callbacks be registered first, and only afterwards the parameters declared?

Yes, I think you are right, but with a caveat. In particular, the on_set_parameters_callback should definitely be registered before declaring the variables. The pre and post callbacks can also, in theory, be registered before, but I don't think they should be in this example. The pre one might try to update a parameter that hasn't yet been declared, and the post one might try to fetch the data out of a parameter that hasn't yet been declared. So I think the sequence should be:

add_on_set_parameter_callback(validation_callback);
declare_parameters;
add_pre_callback(pre_callback);
add_post_callback(post_callback);

• Is it mandatory to keep the shared_ptr to the callback handles for the callbacks to work?

Yes, otherwise they will be immediately unregistered. The documentation for it states as much: http://docs.ros.org/en/rolling/p/rclcpp/generated/classrclcpp_1_1Node.html#_CPPv4NK6rclcpp4Node15list_parametersERKNSt6vectorINSt6stringEEE8uint64_t (I can't link to it directly because of some bug in the generated API documentation, but it is the three functions following this).

It is not clear to me what is meant with "Since multiple 'set parameter' callbacks can be chained",
Based on above signature, where the callback handle is passed to remove_on_set_parameters(), I assume that it is possible to call add_X_set_parameters_callback() multiple times, and hence "chain" multiple callbacks. Correct?

Yes, correct. Maybe we should update the documentation for that.

• If so: What is the order that the callbacks are called: in the order that they were added?

This is also in the documentation for the function:

The registered callbacks are called when a parameter is set.
When a callback returns a not successful result, the remaining callbacks aren’t called.
The order of the callback is the reverse from the registration order.

However, I don't think users should rely on any particular ordering, and hence I don't think we should explicitly call it out in the demos. This is part of the reason why it is important not to change state during add_on_set_parameters_callback.

• And: first all the pre_set callbacks, then all on_set callbacks and then all post_set callbacks, correct?

Yes, correct.

[jrutgeer]

The documentation for it states as much:

Oh my! If there were a contest for 'most scattered and well-hidden documentation', then ROS 2 would be a worthy contender! 😜

No seriously: you can't expect the typical ROS 2 end users to read about callbacks here and realize that there are caveats that are only mentioned in the API docs. Not to mention: if they even know where to find the API docs.

API docs cannot be considered as documentation, unless the necessary effort is spent to educate the ROS 2 users about the API docs. Imo tutorials should be comprehensive. And at least if not they should mention that they are not and point to the relevant sources of information.

The pre one might try to update a parameter that hasn't yet been declared,

Well, this isn't strictly true as the pre_set and on_set callbacks are not allowed to touch the internal state (i.e. neither member variables nor parameters of the node ).

However, the pre_set and on_set callbacks could theoretically read the internal state, and in that case it is indeed the responsibility of the node author to ensure that these variables have been defined before the first callback call, or to use something like if(parameters_initialized){...} in the callback.

But apart from that: it turns out that the pre_set callback is not being called upon declaration of a parameter. At first I thought it was a bug, but it is intentional: it is mentioned in the API docs.

and the post one might try to fetch the data out of a parameter that hasn't yet been declared.

This is indeed a similar issue as reading node parameters from the pre_set and on_set.
Even calling get_parameter("param1") in the post_set called from declare_parameter("param1", 1.0); leads to a ParameterNotDeclaredException. Though I think this might be a bug (see below).

CASE 1:
Consider a post_callback that accesses the node parameter, e.g.:

 RCLCPP_INFO(get_logger(), "param1: %f.", get_parameter("param1").as_double());

And following order:

declare param1;
add_post_callback(post_callback); 

Then upon e.g.

ros2 service call /set_parameters_callback/set_parameters rcl_interfaces/srv/SetParameters "{parameters: [{name: "param1", value: {type: 3, double_value: 3.3}}]}"

The correct value 3.3 is displayed in the info log message.

In other words: before entry of the post_set callback, the node parameter has been updated.

However CASE 2:
Identical code with different order:

add_post_callback(post_callback); 
declare param1;

This throws the ParameterNotDeclaredException.

It is not consistent that in case 1 the node parameter is updated before the post_set whereas in case 2 the parameter would not be defined yet (i.e. cannot have been updated yet, i.e. is updated after the post_set as opposed to case 1).

Also, according to the API doc the post_set is called with a "reference to a const vector of parameters that have been set successfully." and param1 is in that list also in case 2.

So I assume that also in case 2 the parameter is available but the exception is thrown unintentionally.

[jrutgeer]

Is it mandatory to keep the shared_ptr to the callback handles for the callbacks to work?
Yes, otherwise they will be immediately unregistered.

Does that mean that calling

this->remove_on_set_parameters_callback(callback_handle_.get())

could be replaced by:

callback_handle_.reset() ?

[jrutgeer]

What could be a use case to add multiple pre/on/post callbacks?

[clalancette]

Oh my! If there were a contest for 'most scattered and well-hidden documentation', then ROS 2 would be a worthy contender! 😜

We are trying our best. Pull requests accepted.

API docs cannot be considered as documentation, unless the necessary effort is spent to educate the ROS 2 users about the API docs.

Yes, the API docs are there to be used. If you have specific suggestions on how to educate ROS 2 users about the docs, we are happy to listen.

Imo tutorials should be comprehensive. And at least if not they should mention that they are not and point to the relevant sources of information.

Hence this PR.

Well, this isn't strictly true as the pre_set and on_set callbacks are not allowed to touch the internal state (i.e. neither member variables nor parameters of the node ).

This isn't quite true. Because the pre-callback can modify the passed in std::vector<rclcpp::Parameter> & parameters, it can change what parameters will attempted to be set. So it can affect the internal state indirectly.

But apart from that: it turns out that the pre_set callback is not being called upon declaration of a parameter. At first I thought it was a bug, but it is intentional: it is mentioned in the API docs.

Ah, OK. Thanks, I missed that part when I read it yesterday.

CASE 1:

Yes, this one is operating exactly as I expect.

CASE 2:

Hm, I don't see that behavior. For me, this seems to be operating exactly the same as CASE 1. What version of ROS 2 are you testing with?

Does that mean that calling

this->remove_on_set_parameters_callback(callback_handle_.get())

could be replaced by:

callback_handle_.reset() ?

Possibly. I don't know if we properly do the weak_ptr thing with callbacks here. It's worth a test, though it is somewhat orthogonal to this PR.

What could be a use case to add multiple pre/on/post callbacks?

Right, it is a good question. For a single node, you really don't need to have multiple of them.

However, a common pattern in ROS 1 days was to create a single node handle, and then pass it around to multiple components. While that isn't quite our best practice in ROS 2, it is still commonly used, particularly in pieces of code that were ported from ROS 1 (and even in a few places in the core). So it is entirely possible that another piece of code adds a parameter callback handler that you are completely unaware of.

[jrutgeer]

What version of ROS 2 are you testing with?

I tried again with a fresh ROS 2 rolling install:

Source code here: set_parameters_callback.zip

The 'callback registration first' version leads to following output:

$ ros2 run demo_nodes_cpp set_parameters_callback 
[INFO] [1696254471.001921861] [set_parameters_callback]: Registering parameter callbacks.

[INFO] [1696254471.002114598] [set_parameters_callback]: Declaring 'param1'
[INFO] [1696254471.002159032] [set_parameters_callback]: on_set_parameter_callback
[INFO] [1696254471.002188258] [set_parameters_callback]: post_set_parameter_callback
[INFO] [1696254471.002197305] [set_parameters_callback]: Member variable 'value_1_' set to: 1.000000.
[INFO] [1696254471.002205901] [set_parameters_callback]: Getting 'param1' value.
terminate called after throwing an instance of 'rclcpp::exceptions::ParameterNotDeclaredException'
  what():  param1
[ros2run]: Aborted

If 'param1' is declared before registering the callbacks, then there is no exception:

$ ros2 run demo_nodes_cpp set_parameters_callback 
[INFO] [1696254186.026562216] [set_parameters_callback]: Declaring 'param1'
[INFO] [1696254186.026678016] [set_parameters_callback]: Declared 'param1'

[INFO] [1696254186.026689438] [set_parameters_callback]: Registering parameter callbacks.

[INFO] [1696254186.026829865] [set_parameters_callback]: Getting 'param1'
[INFO] [1696254186.026846076] [set_parameters_callback]: Declaring and getting 'param2'
[INFO] [1696254186.026882645] [set_parameters_callback]: on_set_parameter_callback
[INFO] [1696254186.026919375] [set_parameters_callback]: post_set_parameter_callback
[INFO] [1696254186.026929024] [set_parameters_callback]: Member variable 'value_2_' set to: 2.000000.
[INFO] [1696254186.026937289] [set_parameters_callback]: Getting 'param1' value.
[INFO] [1696254186.026946838] [set_parameters_callback]: 'param_value' is: 1.000000.
[INFO] [1696254186.026953570] [set_parameters_callback]: post_set_parameter_callback done
[INFO] [1696254186.026991953] [set_parameters_callback]: Node parameter 'param1' holds value: 1.000000.
[INFO] [1696254186.027007543] [set_parameters_callback]: Node parameter 'param2' holds value: 2.000000.

And upon:

ros2 service call /set_parameters_callback/set_parameters rcl_interfaces/srv/SetParameters "{parameters: [{name: "param1", value: {type: 3, double_value: 1.5}}]}"

the expected output:

[INFO] [1696254217.911002264] [set_parameters_callback]: pre_set_parameter_callback
[INFO] [1696254217.911051328] [set_parameters_callback]: Adding 'param2' to the parameters vector
[INFO] [1696254217.911148522] [set_parameters_callback]: on_set_parameter_callback
[INFO] [1696254217.911169873] [set_parameters_callback]: post_set_parameter_callback
[INFO] [1696254217.911176926] [set_parameters_callback]: Member variable 'value_1_' set to: 1.500000.
[INFO] [1696254217.911185242] [set_parameters_callback]: Member variable 'value_2_' set to: 4.000000.
[INFO] [1696254217.911191384] [set_parameters_callback]: Getting 'param1' value.
[INFO] [1696254217.911202074] [set_parameters_callback]: 'param_value' is: 1.500000.
[INFO] [1696254217.911208717] [set_parameters_callback]: post_set_parameter_callback done

[clalancette]

Source code here: set_parameters_callback.zip

Thanks for the examples. That helped a lot. I see the behavior you are seeing as well.

Looking closer at the code, it looks like I was mistaken; the "post" callback is called before the parameter database is updated by declare_parameter. So that's why trying to call get_parameter during the post callback throws the exception. I'm not entirely sure why it was done that way. It is arguably a bug in how "post" works, but it would be a bug in semantics.

Regardless, I think we should move forward with the rest of this. We can certainly make a note of the current state of affairs, and then (optionally) file another issue to see if we should change the "post" semantics.

Does that make sense? Is there anything else preventing progress on this?

[jrutgeer]

@clalancette I further cleaned up the code and comments, should be ok now.

[clalancette]

I've left one more really minor thing to change, then this looks good to me. Thank you for iterating on this.

[clalancette]

Looks good. I'll run full CI on it next.

[clalancette]

CI:

• Linux
• Linux-aarch64
• Windows