Update Parameter validation features

[alsora]

I went through the Read Only Parameters PR #495 and I experimented a little with Parameters validation via set_on_parameters_set_callback.

I think that there are 2 important features missing:

• In some cases READ_ONLY is too restrictive. The following use-cases should be also possible: a parameter can be modified, but it can't be deleted; a parameter can be modified only by the node that declared it.
• Currently it's possible to change the type of a parameter. To me this looks something that should be enabled only in particular cases, while the default behavior is that if a node declares a parameter to be bool I can't set it to a string or something else.

Both these things can be already done with a custom callback in set_on_parameters_set_callback, but it's quite verbose and not well documented.

Do you agree that these features should be present?
In case, should they be part of the ParameterDescriptor? I was reading about creating a custom structure for parameter constraints, but I haven't found a conclusion.

[clalancette]

Do you agree that these features should be present?

Yes, absolutely, to both. We just ran out of time after getting the read-only parameters in.

In case, should they be part of the ParameterDescriptor? I was reading about creating a custom structure for parameter constraints, but I haven't found a conclusion.

I'm honestly not sure of the implementation, I'd have to look at it again. But generally these tests should happen and abort before the user callbacks are called.

[alsora]

But generally these tests should happen and abort before the user callbacks are called.

Yes, it makes sense.
NodeParameters::set_parameters_atomically seems the correct place where to place these checks.

Enforcing the parameter type for already declared parameters should be straight forward.

On the other hand, for what concerns READ_ONLY it should also be possible to allow the node that owns the parameter to make changes.

The distinction can be done depending on whether the parameters are set using
node->set_parameters() or parameter_client->set_parameters().
A parameter_client can also be created by a node for accessing its own parameters, but this behavior should be deprecated in my opinion (i.e. always requiring a node name to be specified and showing a warning when a node uses its own name).
Otherwise it would be necessary to keep track of the id of the node that registered a parameter.

If these options should be configurable on a per-parameter basis, this may require some work.
A reasonable alternative could be the following:

• Once a parameter is set for the first time, its type can never be changed. The only way is to un-set and set it again.
• The READ_ONLY constraint applies only to other nodes (i.e. the node that owns a parameter can still write.
• A parameter can be un-set only by the node that owns it.
• The effect of the allow_undeclared_parameters flag could be changed in order to transform the node into a ROS1 parameter server where everyone can set/unset parameters.

[clalancette]

• Once a parameter is set for the first time, its type can never be changed. The only way is to un-set and set it again.

Yeah, this makes sense to me.

• The READ_ONLY constraint applies only to other nodes (i.e. the node that owns a parameter can still write.

I'm not sure it is worth special casing this, honestly. It requires a bunch more infrastructure and complexity (as you outline in the prose above). What's the use case you have in mind?

• A parameter can be un-set only by the node that owns it.

This point somewhat breaks the idea of parameters being used as a blackboard...

• The effect of the allow_undeclared_parameters flag could be changed in order to transform the node into a ROS1 parameter server where everyone can set/unset parameters.

...though this point at least makes it configurable. What's the use case you are thinking of here?

[alsora]

• The READ_ONLY constraint applies only to other nodes (i.e. the node that owns a parameter can still write.
• A parameter can be un-set only by the node that owns it.

The idea is to have parameters ownership. Let's exclude for a moment the parameters blackboard case (which should be treated as a special case).
Node A declares a wheel-radius parameter. Other nodes can read it, but they can't change it.
After some time, a calibration step occurs and Node A computes an updated estimate of the wheel-radius parameter.
Another use-case could be a parameter denoting the mode-of-operation of a node. The node itself may want to change this from time to time, but the other nodes are only allowed to read it.

• The effect of the allow_undeclared_parameters flag could be changed in order to transform the node into a ROS1 parameter server where everyone can set/unset parameters.

The use-case is only to preserver the parameters blackboard functionality

[dirk-thomas]

I understand the current semantic of READ_ONLY to be a constant. Once set, never changed.

Having a parameter only read but not written using the service API and only it to be changed by the nodes C++/Python API should be something a node can already do. Maybe there are ways to make that easier / more convenient. But I don't think that should affect the existing concept of READ_ONLY parameters.

[alsora]

I understand the current semantic of READ_ONLY to be a constant. Once set, never changed.

Before carefully reading the documentation, I was pretty confident that the node that owns a READ_ONLY parameter would have been able to modify it.

But I don't think that should affect the existing concept of READ_ONLY parameters.

An issue that I see with the current concept of READ_ONLY is that, even if you know that a certain parameter will never be changed once you read it, you still don't know when it will be available or not.
For this reason a node interested in it will still require to register a callback to be notified when that parameter has been set.

Besides this, in order to maintain this concept and to add the possibility of having parameters that are modified only by the owner node, it will be necessary to add a new flag or to change the READ_ONLY to a 3-states variable.

[tfoote]

The READ_ONLY constraint applies only to other nodes (i.e. the node that owns a parameter can still write.

The concept we're using as READ_ONLY is a programming limitation that doesn't allow changes to the parameter even locally.

Besides this, in order to maintain this concept and to add the possibility of having parameters that are modified only by the owner node, it will be necessary to add a new flag or to change the READ_ONLY to a 3-states variable.

All parameters are effectively already only allowed to be changed by the owner node. If a remote node requests changes the local node has the option to validates the changes before they are applied. If the local node determines them to be invalid for any reason the remote parameter set operation will fail and report that back to the requester. This doesn't need a new state as it's already that way.

It would be possible to add this as another flag that would support declaring that there's no valid use case for external changes to a parameter and that they will all be rejected. But that's getting deep into the system deployment details and looks a lot more like the permissions provided by the security models rather than something that needs to be built into the parameter implementation themselves.

I think that there's definitely some tooling and improvements that we can make to make it easier for nodes to validate incoming parameter requests and implement policies for those incoming requests. But I would suggest making sure that we try not to start building another layer of "permissions" into the core API.

Node A declares a wheel-radius parameter. Other nodes can read it, but they can't change it.
After some time, a calibration step occurs and Node A computes an updated estimate of the wheel-radius parameter.

This use case could easily happen with an external calibration tool/node and the calibration would most optimally be updated directly by the external node. Similarly, the developer/user might want to manually update the calibration parameter and it would be perfectly valid to do so. The node would accept it and all nodes monitoring it would get the updates. If you don't want random nodes updating this parameter that's getting into security and access control for which there are other tools.

Another use-case could be a parameter denoting the mode-of-operation of a node. The node itself may want to change this from time to time, but the other nodes are only allowed to read it.

This is another case where the user might want to change the mode of operation. If it's a parameter but you can't set it remotely, you'd have to create a special standalone service "change mode parameter" that will then do an end run around the default "change mode parameter" option is disabled. And proper validation is still important. It may be invalid to requested a transition from mode A to mode C without going through mode B first.

Something like the requiring you to unset before you change types etc seems like a validation policy that could be somewhat generically applied with some convenience helper tools. But if that's clearly documented what value does it provide for that policy to be fully introspectable? And is that worth the extra complexity?

[alsora]

@tfoote You are right that access control allows to do that, however access control also requires an additional layer of authentication and moreover, as far as I know, SROS2 has specific requirements for the DDS implementation in order to be used.

I agree that instead of adding validation steps inside node_parameters, we could find a way for make easier to create validation callbacks.
This is what I had to do for setting a validation callback that does the following:

• the server has 2 parameters and they can't be deleted.
• the type of a parameter can't be changed (this has to be repeated for all params)
• a parameter is an array and its size can't be changed
• parameter 1 must be a multiple of 42
• parameter 2 (the array) must contain the value 42

The callback does not scale at all, assuming that I have more parameters.
In my opinion, the first 3 points should be super easy to enable.

As you say, it is already the case that parameters are effectively changed only by the owner node (eventually after receiving a service request).
The owner node is also the only one who can set validation callbacks and who can declare parameters.

In this scenario, I don't see the need for having some parameter constraints in the ParameterDescriptor message.
The only situation where you would need to "send" constraints is when you have a blackboard node. However, these applies only to Integer and Double parameters and moreover what's the point for saying "this parameter can be only between 1 and 10" when any other node can simply un-set it or change its type? In a blackboard node, the node itself will not be setting any validation on the specific parameters as they haven't been declared in advance.

You recently added support for multiple validation callbacks.
I could have updated the previous code by creating: 1 callback that prevents anyone from deleting parameters, 1 callback per parameter where I enforce that the type can't be changed, 1 callback per parameter that has other special requirements.
The main issue that I see is that in every callback I would have to add some boilerplate code as

result.successful = true;
for (auto parameter : parameters) {
    if (parameter.get_name().compare("alberto") == 0) {
        if (!validate_condition(parameter.as_int()) {
                result.successful = false;
                result.reason = "parameter \'"+parameter.get_name()+"\' bla bla bla";
        }
    }
}

Maybe we could enforce that every callback should check 1 and only 1 condition.
Moreover, when setting a callback you can pass a vector of parameter names.
In this way the callback will automatically apply only to these parameters.
A default value would apply to all parameters.

This would reduce the code to something like

auto no_delete_check = []() {
     return rclcpp::ParameterType::PARAMETER_NOT_SET != parameter.get_type();
}
auto no_delete_failure_reason = "bla bla bla";
// This applies to all parameters
set_parameters_validation_callback(no_delete_check, no_delete_failure_reason);

auto specific_callback = [](){
      return validate_condition(parameter.as_int());
}
auto specific_failure_reason = "bla bla bla";
// This applies to parameter "alberto"
set_parameters_validation_callback(specific_callback, specific_failure_reason, {"alberto"});

The lambda should be able to access the current ParameterValue and only has to return true or false. Then, how to return the result can be handled internally by parameters_node.

[dirk-thomas]

we could find a way for make easier to create validation callbacks.

Absolutely, and steps towards making it easier to configure / use / customize are highly appreciated.

The only situation where you would need to "send" constraints is when you have a blackboard node. However, these applies only to Integer and Double parameters and moreover what's the point for saying "this parameter can be only between 1 and 10" when any other node can simply un-set it or change its type?

These meta information are available to allow e.g. provide a user interface to change the parameter value. For numeric values the range enable to use a slider with known boundaries.

[bpwilcox]

To chime in, I believe much of the existing validation (type, ranges, read-only) that exists as fields within the ParameterDescriptor could be supported at launch in the yaml parameters file.

I already files an issue ros2/rcl#481 in the rcl repository requesting support in the yaml parser, so in rclcpp node_parameters these parsed descriptions would fill the ParameterDescriptor objects. In code, these parsed descriptions can be optionally ignored via the ignore_override flag.

For the case of a "parameter_blackboard", this feature would be crucial, so as to allow validation support and descriptions for parameters that are declared automatically. Moreover, additional validation criteria (like those mentioned in this ticket thread) added later into the ParameterDescriptor could be captured by the parser in the future.

[ivanpauno]

Currently it's possible to change the type of a parameter. To me this looks something that should be enabled only in particular cases, while the default behavior is that if a node declares a parameter to be bool I can't set it to a string or something else.

There's an ongoing discussion of parameter type enforcement in ros2/ros2#720.

Besides this, in order to maintain this concept and to add the possibility of having parameters that are modified only by the owner node, it will be necessary to add a new flag or to change the READ_ONLY to a 3-states variable.

I think that having both "READ_ONLY" and "CONSTANT" parameters make sense.

A parameter can be un-set only by the node that owns it.

I think that we should restrict when a parameter can be unset, I'm not sure if allowing only the parameter owner to do it is the right way.

The main issue that I see is that in every callback I would have to add some boilerplate code as

I don't understand why that needs to be added. Can you further explain?

To chime in, I believe much of the existing validation (type, ranges, read-only) that exists as fields within the ParameterDescriptor could be supported at launch in the yaml parameters file.

I think that being able to specify a descriptor from a parameters file is a good idea.