Rclcpp: Range support for parameters

[ivanpauno]

Feature request

Support enforcing parameter ranges according to ParameterDescriptor.

Feature description

See ros2/rcl_interfaces#72 for related discussion.

Implementation considerations

Current parameter API allows setting descriptors, but the ranges are not currently being enforced.

[ivanpauno]

Long question:

Now, we are not checking if the type of the parameter being set is correct (e.g.: you can override a bool parameter with an string).
I suppose that "type enforcement" is also desired as part of "range enforcement".

My idea is to reject a set_parameter call when the parameter type is not the same of the type in the descriptor, and ignore the checking when the type in the descriptor is PARAMETER_NOT_SET.
The only problem is that now, when a parameter is set we are updating the type in the descriptor. Is that the desired behavior?
If that the case, when allowing undeclared parameters, overriding an int parameter with a bool parameter will fail.

I asked @jubeira , and in rclpy the behavior is different: The descriptor is not updated when the parameter is set.

[jacobperron]

Now, we are not checking if the type of the parameter being set is correct (e.g.: you can override a bool parameter with an string).
I suppose that "type enforcement" is also desired as part of "range enforcement".

So then I guess that https://github.com/ros2/rclcpp/issues/732 is not necessarily a bug, but just a feature that is missing.

My idea is to reject a set_parameter call when the parameter type is not the same of the type in the descriptor, and ignore the checking when the type in the descriptor is PARAMETER_NOT_SET.

I think this behavior makes sense.

The only problem is that now, when a parameter is set we are updating the type in the descriptor. Is that the desired behavior?
If that the case, when allowing undeclared parameters, overriding an int parameter with a bool parameter will fail.

If we consider range constraints, I think that it is good to fail if a value violates an existing constraint. I think it makes sense to extend this to type constraints as well.

[wjwwood]

I think that if we want to enforce type checking, then we need a new constraint which is something like allowed_types. Currently there is no automatic type enforcement, other than if the node developer attempts to extract the type as one thing when it is stored as another incompatible thing, e.g. .get<int64_t> when it is a PARAMETER_TYPE_STRING or something.

What I was imagining for Dashing was that the range enforcement would be something like:

• if it is an integer:
  
  
  
  • if it is not in the range:
  
  
  • fail the setting
  
  
• allow it to be set

So basically the range would only apply if the new value is an integer. Separately the descriptor could say that the parameter must be an integer, but not because there is an integer range.

So:

• if the value was true and you're changing to 42
  
  
  
  • the range would apply here
  
  
• if the value was 42 and you're changing to true
  
  
  
  • the range would not apply
  
  
• if the value was 42 and you're changing to 100
  
  
  
  • the range would apply
  
  

I think that's what we'd like long term. In the short term things like use_sim_time can implement their own callbacks to enforce the type is correct.

This kind of unforeseen conceptual issue is why I was in favor of waiting until we get more constraints and the ability to have multiple callbacks sorted out.

[jubeira]

@wjwwood the descriptor currently has a type. Just to double check: shouldn't it be used to enforce that new values are of the same type? What's the point of specifying a type in the descriptor if the parameter can be set afterwards with any type?

What I was imagining for Dashing was that the range enforcement would be something like:

if it is an integer:
if it is not in the range:
fail the setting
allow it to be set

What I had in mind was:

• If the descriptor type is different from NOT_SET and the descriptor type doesn't match the parameter type: fail the setting.
• If the descriptor type is integer and we have an integer range, apply it (fail if the value does not comply). EDIT - optional: instead of checking the descriptor type, check the value type as a criteria to apply the range. This would allow a descriptor with type=NOT_SET and an integer range, so that the behavior described above is also taken into account.
• If the descriptor type is floating point and we have a floating point range, apply it (fail if the value does not comply).

IMHO it would be less error prone for a user to enforce the type if the descriptor specifies it so as not to overwrite a parameter with a different type. As a user, if I declare a parameter with a descriptor with a given type, I'd probably want it to stay like that unless I do something explicit to change it (not just setting a different one).
Changing the type when setting should still be possible if the descriptor doesn't specify a type. If the descriptor actually does specify it, the parameter can still be undeclared and declared with a different type / descriptor (or the descriptor can be modified to accept any type at least in rclpy with set_descriptor).

Do you think that makes sense, or should we go as described in https://github.com/ros2/rclcpp/issues/728#issuecomment-494919646? (edited: the behavior described above can still be achieved if the type is not set; maybe that's the way to go).

[wjwwood]

@wjwwood the descriptor currently has a type. Just to double check: shouldn't it be used to enforce that new values are of the same type? What's the point of specifying a type in the descriptor if the parameter can be set afterwards with any type?

Well, there may be more than one opinion about this, but my opinion is that the name and type in the descriptor are intended for the person who calls describe parameters, and not for a way for the developer to tell the node what the type should be.

In fact, in rclcpp the name and type fields of the descriptor are explicitly ignored when declaring (this was a compromise so we could reuse that type rather than having another type which contains only the meta data and not the name/type fields):

https://github.com/ros2/rclcpp/blob/30df5cdf36b56aa530e7cca6466d9df88cbf20b9/rclcpp/include/rclcpp/node.hpp#L368-L370

So maybe the type field in the descriptor should be current_type, and then separate from that there could be allowed_types which is a list of allowed types for the value, with length 0 meaning anytime and length 1 meaning only that one specified type. Or something like that.

So in my opinion, no the type field in the descriptor should not be used to enforce the type. And the point of it is to let external users know what the type is currently, when they are introspecting remotely.

I think the rest of your argument hinges on the idea that the type field in the descriptor is used only as a constraint and not as a way to communicate the current type of the value, which I don't think is right. So, what you've proposed, in that context, I don't think makes sense, and I stand by my suggestion.

One important use case to consider is if a user wants a parameter to be able to have any type over its lifetime, how would you support that if type checking is enforced always using the descriptor's type? And if the answer is, don't set the descriptor type if you don't want type checking, then I'd ask how then do you get the current type of the stored values externally, i.e. the describe_parameters ROS service (normally communicated with the descriptor, https://github.com/ros2/rcl_interfaces/blob/7e2e6a7578f588e775f4f1f3b5b8500e3fcbb4f0/rcl_interfaces/srv/DescribeParameters.srv#L9).

[hidmic]

So, IIUC while @ivanpauno and @jubeira think of type as a constraint, @wjwwood thinks of it as state. I can see the value of both, which makes me think that just having type is not enough and likely confusing. The same goes for constraints implicitly bound to the type they apply to.

For now, @wjwwood if we are moving towards parameter declaration everywhere (by setting allow_undeclared_parameters to False by default) and considering that, at least in ROS1, that I know of, variant type parameters weren't a thing, IMHO it'd make it less confusing to think of type as allowed_type. Eventually we'll have both state and constraints. Just my 2 cents, feel free to dismiss.

[ivanpauno]

One important use case to consider is if a user wants a parameter to be able to have any type over its lifetime, how would you support that if type checking is enforced always using the descriptor's type? And if the answer is, don't set the descriptor type if you don't want type checking, then I'd ask how then do you get the current type of the stored values externally, i.e. the describe_parameters ROS service (normally communicated with the descriptor, https://github.com/ros2/rcl_interfaces/blob/7e2e6a7578f588e775f4f1f3b5b8500e3fcbb4f0/rcl_interfaces/srv/DescribeParameters.srv#L9).

It wouldn't be possible with the describe_parameters service, but it would be still be possible calling get_parameter_types (or get_parameters):
https://github.com/ros2/rcl_interfaces/blob/7e2e6a7578f588e775f4f1f3b5b8500e3fcbb4f0/rcl_interfaces/srv/GetParameterTypes.srv
https://github.com/ros2/rcl_interfaces/blob/7e2e6a7578f588e775f4f1f3b5b8500e3fcbb4f0/rcl_interfaces/srv/GetParameters.srv

IMO, mixing both constraints and actual parameter information in the same place is confusing.

[wjwwood]

For now, @wjwwood if we are moving towards parameter declaration everywhere (by setting allow_undeclared_parameters to False by default) and considering that, at least in ROS1, that I know of, variant type parameters weren't a thing...

Actually I think in ROS 1 they are implicitly type-less:

% roscore &

% rosparam set foo 42

% rosparam list
/foo
...

% rosparam get /foo
42

% rosparam set foo true

% rosparam get /foo    
true

% rosparam set foo "taco"

% rosparam get /foo      
taco

If you're talking about dynamic reconfigure then you're right it's limited to four types:

paramtype - defines the type of value stored, and can be any of int_t, double_t, str_t, or bool_t

But for me, the parameters we have in ROS 2 are a mixture of both. So I don't see the fact that dynamic reconfigure didn't support it as a good reason to not allow it in ROS 2.

don't you think it'd make it less confusing to think of type as allowed_type?

Until we have both the type constraint field as well as the current type field, then no, I don't think it's less confusing to think of it as a constraint. I interpret each entry in the result of describe_parameters as "this is parameter 'name', with the current type 'type', that is described as 'description', and if changed may be constrained by these constraints: 'allowed_types', 'additional_constraints', 'interger_range', etc...".

---

It wouldn't be possible with the describe_parameters service, but it would be still be possible calling get_parameter_types (or get_parameters):

As far as I know, for external nodes which use the ROS interface, there is no get_parameter_types. So you'd be forced to retrieve the parameter value in order to know it's current type.

Even if we agreed to change the meaning of the type in the descriptor, it would require us to change the design document (which covers the ROS part of the interface):

• Describe Parameters
  
  
  
  • Given a list of parameter names, return their datatype.
  
  

(from https://design.ros2.org/articles/ros_parameters.html#required-functionality)

IMO, mixing both constraints and actual parameter information in the same place is confusing.

I agree, and I'd actually be in favor of splitting the ParameterDescriptor message up. I almost did that so that we had a message which could be passed to declare_parameter which didn't need to ignore certain fields (like the name and type) as we currently do in rclcpp.

---

Sorry guys, I remain unconvinced by the idea of redfining the meaning of the type field in the descriptor message.

For me, the two paths forward are:

• do not enforce type, and let the user do if they choose using a custom callback (and apply the ranges only when the new value is of that type), or
• introduce the allowed_types constraint to the descriptor now
  
  
  
  • later we could split ParameterDescriptor into two parts, current name/type and description/constraints, then update rclcpp::Node::declare_parameter* to take the latter part
  
  

Obviously others on @ros2/team can/should weigh in here, but I would go for the first option, since it's not intrusive and can most easily be improved in the future without deprecations or changes in behavior.

[wjwwood]

Actually, I was wrong there is a get_parameter_types ROS service:

https://github.com/ros2/rcl_interfaces/blob/master/rcl_interfaces/srv/GetParameterTypes.srv

So, I guess it's not _as_ important to have the information in the descriptor, but I still think it changes the intended meaning of the field, and should be reflected in the design document if we change it.

[clalancette]

Sorry guys, I remain unconvinced by the idea of redfining the meaning of the type field in the descriptor message.

For me, I'm generally of the opinion that types should be enforced on parameters by default (whether we do that for Dashing is a separate discussion). While I can imagine some scenarios where having a multitude of types could be useful, I still maintain that the vast majority of users will want each parameter to have a specific type, in a specific range. If you are using parameters as tunables, you definitely want to make sure that the type set from the outside is what you expect (and not that a bool got stuffed into a double). If you are using parameters as a blackboard, you want to make sure all getters and setters agree that the type is the same. What are other uses for parameters that we envision, and do we really think that they should be duck typed?

With the above in mind I would say that we should enforce types by default, with a way to opt out of it if you really want your parameter to be able to take on multiple types.

For the immediate problem, I agree that changing the design document and the implementation at this late stage is a problem. But I also wouldn't want to codify not doing type checking by default in the future.

[hidmic]

I think in ROS 1 they are implicitly type-less

That is true, my point being that I have yet to see a ROS1 package that isn't implicitly assuming the parameter will be of a given, fixed type (and documenting it as such). Of course, I can't say I've seen them all :)

[jubeira]

For the immediate problem, I agree that changing the design document and the implementation at this late stage is a problem. But I also wouldn't want to codify not doing type checking by default in the future.

It's probably not the right time of changing it, so moving forward with option 1 unless someone has anything else to say about it. One small note about it though: if the clients enforce the type, then getting the constrained type should actually be the type of the parameter itself, so the design document would still be _(almost)_ right -except in the case the type is NOT_SET, if that's interpreted as "no type constraints"-.

[wjwwood]

I also agree that we want type checking in the future, but by not doing it now we're just deferring the improvement, not ruling it out.

Honestly, this is why I was iffy on doing the range checking for Dashing (also since we're past the feature freeze...). The user can already make their own callback to do this type checking, and if we start doing type checking automatically in the future, it will not break them, instead it will continue to work and their callback that checks the type will become redundant and they can remove it at their leisure. That's why I liked the first of my two options above.

Either way more work is required here, we're just deciding what to do right now.

[tfoote]

We definitely want type checking. In ROS 1 the parameters were untyped. In the design of ROS 2 parameters we added the type to the parameter description to avoid the ambiguity of the types. One of the strengths of ROS 1 has been the strongly typed messages that then supported easy communications of the interfaces. But we lacked that ability in general parameters. Using untyped parameters requires much more defensive programming when using parameters.

Sometimes flexibility can be too much, explicitly supporting N types for a parameter doesn't really make sense. You could just as easily set those two parameters side by side with different names and have a precedence policy documented.

In the long run if we're not requiring parameter declarations we can update the type when set. But if we're enforcing that the parameters are declared that should also enforce the type is the same as the declaration.

[ivanpauno]

We definitely want type checking.

+1. The question if we want to land it for this release or for a later patch. IMHO, landing first with range enforcement and later with type enforcement sounds strange.

Sometimes flexibility can be too much, explicitly supporting N types for a parameter doesn't really make sense. You could just as easily set those two parameters side by side with different names and have a precedence policy documented.

I also prefer not supporting N types, since most cases are covered by only one. On the other hand, we should have an option in the descriptor for skipping type enforcement (our original ideas was using type=PARAMETER_NOT_SET for that). In that case, the user could write his own callback for rejecting it.

In the long run if we're not requiring parameter declarations we can update the type when set. But if we're enforcing that the parameters are declared that should also enforce the type is the same as the declaration.

The problem with that, is if we want to have an option in the description for skipping type enforcement. The idea I had, was using PARAMETER_NOT_SET as a type with that meaning. If we later update the type in the descriptor, we would lose that info. We can add a bool as a work-around.
The other problem, is that the default descriptor starts with type=PARAMETER_NOT_SET. So, what does that mean? If it means that we are not doing type checking, we shouldn't update it.
Another option is to take the default_value type in the declare_parameter method as the type, and then enforce it.

IMO, updating it in some cases and in others not is not a good idea. e.g.: If I ask for a parameter description and read type=PARAMETER_INTEGER, I will not know if it was declared as an integer and type is enforced or if the node was accepting undeclared parameters and the type is not enforced.

---

The option of having both in the descriptor: allowed_type and actual_type will clarify all the cases.
And maybe, type and is_type_enforced is a good option too.

[wjwwood]

Whether or not we support variable type, I agree it would be better to have type enforcement before or at least with range checking, but in my opinion this is all too rushed in order to check a box. In my opinion type checking has not been thought about enough to implement this close to a release. At least with ranges we have had a solid discussion about how it should be used and interpreted.

We should be able to add type checking in the future without breaking behavior of ranges. That's why I suggested to put in ranges as-is and only check them when the type is appropriate.