add_on_set_parameters_callback callback takes vector of parameters but only returns a single SetParametersResult

[tylerjw]

https://github.com/ros2/rclcpp/blob/d9377dc740d4424d0740131e79cfdb2a31dcf87c/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp#L40-L46

SetParameters

# A list of parameters to set.
Parameter[] parameters

---
# Indicates whether setting each parameter succeeded or not and why.
SetParametersResult[] results

SetParametersResult

# A true value of the same index indicates that the parameter was set
# successfully. A false value indicates the change was rejected.
bool successful

# Reason why the setting was either successful or a failure. This should only be
# used for logging and user interfaces.
string reason

This is probably already a known issue. It seems like there is a mismatch between this interface and the message/service interface in the rcl_interfaces repo. In my testing on foxy it seems that when you send it a vector of parameters to set it calls the callback once for each one (each time with the vector containing only one parameter).

Because this is an interface issue it'll probably have to be fixed over major releases to avoid breaking the most code. What I think should change is the callback interface should return a vector of SetParametsResult to match the service interface.

[wjwwood]

I don't think this is actually a problem. The reason that the OnParametersSetCallbackType takes a vector of parameters to be set but returns only one result is that it could either be called as a result of setting one parameter or several parameters atomically.

The Service interface chooses to call set parameter for each parameter, getting a separate result for each one. We could have another service like SetParametersAtomically which looks like this:

# A list of parameters to set.
Parameter[] parameters

---
# Indicates whether setting the parameters succeeded or not and why.
SetParametersResult result

---

Btw, I think you got your labels backwards on the service/msg definitions.

[fujitatomoya]

btw, I think SetParametersResult and SetParameters are the other way around in your description. (SetParametersResult.msg, SetParameters.srv)

It seems like there is a mismatch between this interface and the message/service interface in the rcl_interfaces repo.

I don't see in that way. it is really clear to me that server takes Parameter[] parameters and return SetParametersResult[] results.

In my testing on foxy it seems that when you send it a vector of parameters to set it calls the callback once for each one (each time with the vector containing only one parameter).

i think this is only because of atomic operation for services. (e.g) NodeParameters::set_parameters_atomically and https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/srv/SetParametersAtomically.srv)

[fujitatomoya]

i was one step behind... :cry: after all, i agree with https://github.com/ros2/rclcpp/issues/1550#issuecomment-778047330

[wjwwood]

We could have another service like SetParametersAtomically which looks like this

i think this is only because of atomic operation for services. (e.g) NodeParameters::set_parameters_atomically and https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/srv/SetParametersAtomically.srv)

Annnnd we do 🙃

i was one step behind... 😢 after all, i agree with #1550 (comment)

At least we agree 🤷

[tylerjw]

result of setting one parameter or several parameters atomically

I don't understand what this means with respect to how I should implement the callback. Should I expect to validate all parameters together as a group and only change the internal state if all parameters that I receive in the vector if all of them are valid to handle if the "automatic" functionality is turned on for that node?

If that's so should some documentation about this be updated to give advice on how to implement the callback? Where would that documentation be? I'd be happy to submit a PR for this.

I only found how to do this by finding an example of someone using it and reading the source for it, I completely missed the nuance of automatic parameter setting and was confused why the interface was the way it was and that when I called the service with an array of parameters it called the method once for each.

meta

Thank you for the quick responses. I updated the labels on the messages. :smile:

[tylerjw]

Point me to where I can submit a PR for documenting advice on implementing the callback and close this issue. Now that I understand the concept of automatic parameters the interface makes sense the way it is.

[fujitatomoya]

I don't understand what this means with respect to how I should implement the callback. Should I expect to validate all parameters together as a group and only change the internal state if all parameters that I receive in the vector if all of them are valid to handle if the "automatic" functionality is turned on for that node?

as a service client of application, you can choose either

https://github.com/ros2/rclcpp/blob/18b112fcce90ef41fb2a312dda59508f2164dc5e/rclcpp/include/rclcpp/parameter_client.hpp#L141-L147

or

https://github.com/ros2/rclcpp/blob/18b112fcce90ef41fb2a312dda59508f2164dc5e/rclcpp/include/rclcpp/parameter_client.hpp#L149-L155

you can also refer to the following services provided by service server,

root@a6cfc30c581b:~/docker_ws/ros2_colcon# ros2 service list
/talker/describe_parameters
/talker/get_parameter_types
/talker/get_parameters
/talker/list_parameters
/talker/set_parameters !!! HERE
/talker/set_parameters_atomically !!! HERE

and i don't think this concerns callback implementation since these are interfaces. i think we can just implement callback based on how we want to manage each parameter.

If that's so should some documentation about this be updated to give advice on how to implement the callback? Where would that documentation be? I'd be happy to submit a PR for this.

i am not sure articles/ros_parameters can answer this, but at least it mentions about atomic service operation.

when I called the service with an array of parameters it called the method once for each.

i guess you are saying that we can optimize it?

i hope i am not mistaken :smiley:

[clalancette]

Point me to where I can submit a PR for documenting advice on implementing the callback and close this issue. Now that I understand the concept of automatic parameters the interface makes sense the way it is.

There are 2 places we can think about adding some documentation:

1. In the doxygen header block for add_on_set_parameters_callback.
2. In a new page/tutorial at https://index.ros.org/doc/ros2/

Which one we choose I think depends on how much content we are going to add. If it is just a couple of extra explanatory sentences, then I'd choose the doxygen header block. If it is going to be a lot more elaborate than that, I'd go for a new tutorial on index.ros.org.