Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add type enforcement of parameters #720

Closed
ivanpauno opened this issue Jun 3, 2019 · 8 comments
Closed

Add type enforcement of parameters #720

ivanpauno opened this issue Jun 3, 2019 · 8 comments
Labels
enhancement New feature or request

Comments

@ivanpauno
Copy link
Member

ivanpauno commented Jun 3, 2019

Feature request

Feature description

Meta ticket for discussing how to add type enforcement of parameters both in rclcpp and rclpy.
See discussion in range enforcement PR.

Summary of the previous discussion

Currently, the parameter descriptor message has a type field, which is updated each time the parameter is set. To add type enforcement, some different proposals have been discussed:

  1. Add a new field to the parameter descriptor, called allowed_types. The type field will continue working as before, and allowed_types shall be a vector with the admitted types. (api compatible but breaking abi)
  2. Similar to the above option, but the adding a member called allowed_type. Only one type can be specified. (api compatible but breaking abi)
  3. Change the type field meaning. It won't be updated anymore when a parameter is set, and it would be used for type enforcing. When the specified type is PARAMETER_NO_TYPE, type enforcement is not applied (this could be applied in the other options). (breaks abi and api)
  4. Do not apply type enforcement for undeclared parameters, and continue updating the type field in that case. Stop updating it with declared parameters, and use it for type enforcement. (breaks api)
  5. Add an is_type_enforced field to the parameter descriptor (default to false). When it is false, type enforcement is not applied, and type field is updated as before. When it is true, it's used for type enforcement (no need to update it, as the type is enforced). (api compatible, but breaking abi)

Implementation considerations

Any of the options should be easy to implement both in rclcpp and rclpy.

@ivanpauno ivanpauno added the enhancement New feature or request label Jun 3, 2019
@ivanpauno
Copy link
Member Author

cc @jacobperron @wjwwood @jubeira @hidmic @clalancette @tfoote.
If something is missing in the discussion summary, let me know and I will update it.

Although my original proposal was the third member of the list, I suggest going on with the last option (which does not break api).

@jubeira
Copy link

jubeira commented Jun 3, 2019

I'd propose to discuss this in our next meeting.
IMO:

  • For most use-cases, allowing a single type should do the job as @tfoote mentioned.
    • Having a way to bypass type enforcement explicitly would be a nice to have (shouldn't be the recommended option though). Then, a user would be able to enforce more than one type with the callback in case it's really required.
    • If we are enforcing the type defined in the descriptor, having a variable with the actual type and a variable with the enforced type feels a bit redundant, because the actual type and the enforced type should always be the same by design. I know this wouldn't be the case if we are bypassing type enforcement (e.g. using PARAMETER_NOT_SET), but since this shouldn't be the recommended case and that the type can be retrieved in a different way, I would be fine with changing the meaning of type for this particular case.

In other words, if a slight change in the meaning of type is acceptable, I would use the existing type inside ParameterDescriptor to enforce the parameter type to avoid adding more fields to the message.

  • When type is enforced, the enforced type will be the actual parameter type (which doesn't change the current meaning of the variable). This should be the most common use-case and the recommended one.
  • When the type is not enforced, type will not give any information about the actual type. This would change the meaning of the current type variable. The actual type information can be retrieved using GetParameterTypes service.

If that change of meaning for type is not acceptable, I'd be fine with either of the other options or new proposals.

@jacobperron
Copy link
Member

jacobperron commented Jun 4, 2019

  1. Add an is_type_enforced field to the parameter descriptor (default to false). When it is false, type enforcement is not applied, and type field is updated as before. When it is true, it's used for type enforcement (no need to update it, as the type is enforced). (api compatible, but breaking abi)

This option makes the most sense to me. We keep the same (or similar) intention of type and are backwards compatible. But, maybe it is better to go the recommended way and enforce the type by default. This would mean that we postpone releasing this until Eloquent (or just flip the switch after backporting to Dashing).

@ivanpauno
Copy link
Member Author

@wjwwood Friendly ping.

@alsora
Copy link

alsora commented Aug 16, 2019

Change the type field meaning. It won't be updated anymore when a parameter is set, and it would be used for type enforcing. When the specified type is PARAMETER_NO_TYPE, type enforcement is not applied (this could be applied in the other options).

I see this as the simplest and most effective way for enforcing the parameter type.

One of the concerns for this release was that it breaks APIs.
Now that this would go into Eloquent, is the option 3 more interesting for you?

In any case, regardless of the chosen option, it should be also added a new API for declaring parameters:

declare_parameter(name, description)

This allows to declare a parameter and to specify its description (with the type), while still keeping it UNSET

@clalancette
Copy link
Contributor

@ivanpauno Now that we are enforcing types, can we close this one out?

@jacobperron
Copy link
Member

I'm pretty sure we can close this.

@clalancette
Copy link
Contributor

All right, will do. Feel free to reopen if there is further work we need for this feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

5 participants