Avoid calling user parameter callbacks for "qos_overrides" parameter changes

[jacobperron]

Feature request

This is less of an explicit request, and more a question about how we should interact with user-defined parameter callbacks.

Feature description

If a user registers a callback with rclcpp::Node::add_on_set_parameters_callback(), should the callback be triggered for any qos_overrides* parameters that are internally created by the node (since #1465)?

For example, it is somewhat common to see user callbacks that look like this:

  auto result = rcl_interfaces::msg::SetParametersResult();
  result.successful = true;

  for (auto parameter : parameters) {
    if (parameter.get_name().compare("user_defined_param") == 0) {
      // do something with the value here ...
    } else {
      string_result << "The parameter " << parameter.get_name() << " is not reconfigurable.\n";
      result.successful = false;
    }
  }

Since #1465, user code doing something like the code above will fail (the node terminates the process) if QoS overrides are enabled, because the parameter validation callback is not aware of the qos_overrides parameters declared by the nodes implementation.

Possible fixes:

1. Document this pitfall well and provide an example of handling the new "qos_overrides" parameters like:

  auto result = rcl_interfaces::msg::SetParametersResult();
  result.successful = true;

  for (auto parameter : parameters) {
    if (parameter.get_name().compare("user_defined_param") == 0) {
      // do something with the value here ...
+   else if (parameter.name.rfind("qos_overrides", 0) == 0) {
+     // Ignore changes to QoS override parameters here.
+     // Optionally, we could validate QoS values here, but we also have a more convenient mechanism for that when enabling QoS overrides
    } else {
      string_result << "The parameter " << parameter.get_name() << " is not reconfigurable.\n";
      result.successful = false;
    }
  }

2. Document that rejecting unknown parameters is an anti-pattern
   Just because one user callback doesn't know how to deal with a parameter doesn't mean that it should reject it. It's possible that another registered callback is expecting said parameter. If one callback rejects a change, then it may prevent other callbacks from getting a chance to handle the change. We would then recommend that the original example above be changed to look like:

  auto result = rcl_interfaces::msg::SetParametersResult();
  result.successful = true;

  for (auto parameter : parameters) {
    if (parameter.get_name().compare("user_defined_param") == 0) {
      // do something with the value here ...
+  }
-   } else {
-     string_result << "The parameter " << parameter.get_name() << " is not reconfigurable.\n";
-     result.successful = false;
-   }
  }

3. Change the rclcpp implementation to avoid calling user-defined callbacks for qos_overrides parameters
   This would make all existing code work as-is, but it is a little odd that qos_overrides parameters are not passed to user callbacks when other default node params, like use_sim_time are.

Implementation considerations

Whatever we choose, it would be good to resolve this prior to the Galactic release so there is a clear path forward for users porting to Galactic.

[clalancette]

I think we should do 2 regardless of what we decide elsewhere. As you say, it is bad practice since we have the ability to have multiple callbacks.

For 3, I agree it is weird to pass some of the internal parameters and not others. I would say that either we go all the way (and pass all of the internal parameters to user callbacks, like we have now), or none of the way (don't pass any internal parameters to the user callbacks). I think the problem with not passing any internal parameters is that things like use_sim_time could be useful for a node to know about when it is changed.

So after working through that, my opinion is that we do 2 and patch things in repositories where we can.

[wjwwood]

I think 2 is the right path, honestly.

We could try to separate "user" and "internal" parameters, but I'd recommend something like what we did with leading _ in topic names, so that the creator of the parameter could decide if it is "hidden" by default or not just using the name, similar to .name in filesystems on Linux. Then we could not pass "hidden" parameters to these callbacks by default and have an option to include them, but I don't really like that since it makes the interface more complicated and creates a special case.

Another option is that we could change the add_on_set_parameters_callback() (or add a new method like it, but with an additional argument) so that it had a "filter" function or class in addition to the callback, which would be applied before passing parameters to the callback. By default this could point to a filter provided by rclcpp which we could keep updated with the "internal" parameters, but the user could remove this filter to get access to them or extend it to exclude additional items. This would really just be taking what is normally done within the callback and moving it out to a separate step that could be extended or composed. I'd prefer the "filter" concept to a single flag that excludes some internal parameters.

[jacobperron]

I've added to the rclcpp API docs to encourage users to avoid rejecting unknown parameters: #1590

I can do something similar to Python.

I don't think we have any tutorials to update, but I can also add a note to Galactic about it.

[jacobperron]

We have consensus that rejecting unknown parameters is bad practice. I've added documentation about this in the following places:

• Document misuse of parameters callback #1590
• Document misuse of parameter callbacks rclpy#734
• Add Galactic release note about parameter callbacks ros2_documentation#1267

I'm satisfied, so I'm closing this ticket. Feel free to continue commenting if you think there's more to be done.