Fix issue when a callback returns false, other callbacks are not triggered

[SteveMacenski]

As brought up to me by a Nav2 contributor, when a dynamic parameter callback returns false because it can't / doesn't want to deal with the parameter, it prevents all other callbacks from triggering because any returned false. For example, we have nodes with multiple dynamic callbacks registered for different composed objects to adjust their respective parameters.

I think this is a bug because each of the callbacks should be independent and whos state call shouldn't be dependent on other callbacks, who has no method of reordering or setting priority.

Instead, if any return false, we store it and set it at the end, but give all callbacks a chance to trigger. This results in the same behavior, but all callbacks are processed.

[clalancette]

Instead, if any return false, we store it and set it at the end, but give all callbacks a chance to trigger. This results in the same behavior, but all callbacks are processed.

I will say that the current behavior is by design (though we can consider changing the design). The thinking here is that there really should be no state in these particular callbacks; they should really only validate whether the new parameter value is valid or not. That's why the return value is merely a boolean.

That said, maybe there is a use-case here. Can you explain more fully what your use is?

[SteveMacenski]

Its being reported to me because some algorithms are sensitively logging/success=false that when a dynamic parameter callback is triggered containing a parameter it doesn't know about. When multiple objects that contain dynamic parameter callbacks are in the same process, then this rejection by one object is preventing the actual object that the parameter is updating from triggering and leaves the application in an undefined configuration state. whereas the state of a shared parameter across the different composed objects is dependent on the ordering of the dynamic parameter callback registration.

This is a bug in the object (that I'm about to fix once I finish writing this up) since I agree with you that that object shouldn't be success=false because we know that multiple objects are going to have those callbacks triggered. However, it did spark me enough to find that bit of code and think it was odd that we don't attempt to trigger all callbacks registered, even if one doesn't approve of the parameter update due to the ordering issues that it can create.

My belief is that the state of one callback shouldn't impact the allowable triggering of other callbacks. Otherwise, the ordering of the registration of the dynamic parameter callbacks has a meaningful impact on the system's configurations in these situations (where some have been called and possibly stored the new value; others haven't been; and we don't know which is which). To resolve that, I think we'd need to set priority levels to trigger the calls or allow the user explicitly set the ordering, but I think its better to just call them all so that the state is always known and consistent.

[clalancette]

(where some have been called and possibly stored the new value; others haven't been; and we don't know which is which)

I think this is the major disconnect. These callbacks absolutely should not be changing state, for exactly the reason listed here.

To give a more concrete example, suppose you have callbacks A, B, and C (registered in that order). A parameter change comes in, which calls A. A accepts the change, and updates some internal state based on that. B accepts the change. Then C rejects the change, which causes the parameter database in the node to not be updated. At this point, you have a completely inconsistent system. Some internal state has been changed because A accepted the change, but a ros2 param get will still show the old value.

Even if we were to accept this change, the situation above will still arise, because the internal node database still won't take the change (since it has been rejected by at least one callback). So I'm not sure that this is a net win.

For what it is worth, if you do want to make internal state changes when parameters change, you can use the add_post_set_parameters_callback function to do that. By the time that is called, we know for sure nothing will reject the change, and thus it is safe to make internal state changes. This is documented in https://docs.ros.org/en/rolling/Concepts/Basic/About-Parameters.html#parameter-callbacks

[SteveMacenski]

These callbacks absolutely should not be changing state, for exactly the reason listed here.

Sure but there are alot of examples of this, especially for applications that are attempting to have ROS wrappers around core algorithms instead of letting ROS be down to the algorithm level. We want often for member variables for parameters to be updated here because we don't use get_parameter in the actively running code, we use the member variable.

Ex.

• https://github.com/ros-navigation/navigation2/blob/main/nav2_controller/src/controller_server.cpp#L811-L854
• https://github.com/moveit/moveit2/blob/99227040ebe4bac4cc1a95afbe44ee9c543e3c02/moveit_ros/planning/planning_scene_monitor/src/planning_scene_monitor.cpp#L328-L347

I understand that add_post_set_parameters_callback exists and would fit this role better, however there's alot of ROS 2 code around that pre-dates 2022 when that was introduced. Much of the design patterns are already done, at least in Nav2, well before Humble. But with that said, I'll file a ticket about that in Nav2 and seems like a nice contribution for a Jr developer. I take pride that Nav2 is a 'best practices' reference for ROS 2 usage, so now that this has been brought to my attention, I want to have that fixed up.

That would resolve the need for this PR, I agree.

Even if we were to accept this change, the situation above will still arise, because the internal node database still won't take the change (since it has been rejected by at least one callback). So I'm not sure that this is a net win.

With that said, I would consider this to be a net win even with this all considered because of historical legacy of people not making the changes / aware of the change with the pre- and post- callbacks (and that I could find a few examples in about 30 seconds of that in related projects). However, as written and intended today, I agree this should make zero difference.

I leave it to you about whether to move forward or close this PR - I appreciate the time 😄

[SteveMacenski]

Nav2 ticket for visibility: ros-navigation/navigation2#4907

[Yadunund]

Closing this one out as the changes will be applied downstream.

Edit: Forgot to add that we discussed this during our triage meeting and agreed that it's best to update nav2 to rely on post_set_parameters_callbacks.

[SteveMacenski]

Sure thing 🙂