`add_on_set_parameters_callback` `reason` field is lost upon `successful=true` about rclcpp HOT 6 CLOSED

I've opened up ros2/rcl_interfaces#159 to fix this.

I actually don't think I'll add this to the Iron release notes; this behavior has actually always been the case (at least since we've had multiple callbacks), it just wasn't as clearly exposed as it is now. That said, if you find other places that reference setting the 'reason' code on success, I'm happy to update them as well (just let me know where they are).

from rclcpp.

Confirmed that this is a change in behavior, though it brings up an interesting question of policy.

The way that this works is that the add_on_set_parameter_callbacks are a vector of callbacks; you can install an arbitrary number of them. Each one that is installed is placed at the front, so the callbacks are called newest first.

Further, the way that this list of callbacks is called is here. Notice in particular that the same result variable is reused for each invocation of the callback.

Finally, what happened in Iron is that we added a callback into the core rclcpp::Node:

Putting this all together, we can see what happens. During the rclcpp::Node constructor (which happens before any user code), we install the first add_on_set_parameters_callback for the TimeSource. Later on, we run the user code which installs the second add_on_set_parameters_callback. Since the user one is installed second, it is actually called first. Which means we call it, get the result successful and reason back, and then go on to call the TimeSource one which overwrites it. So that explains what is happening here.

Fixing it is tricky. In the set_parameters case, we could actually fix it since we have a separate SetParametersResult for each parameter, and thus we could refactor this to save off the result from each call. However, we run into a problem for set_parameters_atomically, which only has a single result for all parameters, and there it is unclear which one we would store.

Because of that latter problem, my inclination is to call this the new behavior and add a release note about it. But I'm interested to hear opinions from others, particularly from @ros2/team

from rclcpp.

Sounds good. Probably should be mentioned in documentation that it is only a failure reason, not a general message.

from rclcpp.

as @clalancette mentioned, originally the problem has been there in Humble, but Iron change makes it easier to happen.

Probably should be mentioned in documentation that it is only a failure reason, not a general message.

i thought that too, but that is not a designed behavior. application can set the reason why is is accepted as well.

see https://github.com/ros2/rcl_interfaces/blob/f03ee90089b96e9d6aa699019f43f17e1b0ac291/rcl_interfaces/msg/SetParametersResult.msg#L5-L7

In the set_parameters case, we could actually fix it since we have a separate SetParametersResult for each parameter, and thus we could refactor this to save off the result from each call.

agree.

However, we run into a problem for set_parameters_atomically, which only has a single result for all parameters, and there it is unclear which one we would store.

precisely, i think the problem comes up for successful cases.
once it hits the failure, it can return the result(false) with reason set by the user application callback right away.
but if all parameters can be accepted, we cannot be sure which reason should be set in result message.

a few approaches that we could take,

• set_parameters_atomically succeeds with multiple parameters, reason payload will be empty. (cannot use reason for general message with acceptance.)
• it can print warning, that reason field will be empty, if that hits this case.
• reason can be applied only when it fails. (design change, probably not a good idea.)

any thoughts?

from rclcpp.

After discussing this, we decided that the new behavior pretty much has to be the behavior. So I'll update the comments in the SetParametersResult message, add an Iron release note about this, and update any other documentation that we have that references this.

from rclcpp.

Thanks!

from rclcpp.