-
Notifications
You must be signed in to change notification settings - Fork 537
Document design decisions that were made for statically typed parameters #1568
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
Changes from 2 commits
2987e63
80624f0
f245d42
c789228
4167e5c
75691e5
f78327a
13fc4c8
46a773e
2576bc0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| # Notes on statically typed parameters | ||
|
|
||
| ## Introduction | ||
|
|
||
| Up to ROS 2 Foxy, all parameters could be changed of type anytime, except the user installed a parameter callback to reject that change. | ||
| This could generate confusing errors, for example: | ||
|
|
||
| ``` | ||
| $ ros2 run demo_nodes_py listener & | ||
| $ ros2 param set /listener use_sim_time not_a_boolean | ||
| [ERROR] [1614712713.233733147] [listener]: use_sim_time parameter set to something besides a bool | ||
| Set parameter successful | ||
| $ ros2 param get /listener use_sim_time | ||
| String value is: not_a_boolean | ||
| ``` | ||
|
|
||
| For most use cases, having a static parameter types is enough. | ||
|
ivanpauno marked this conversation as resolved.
Outdated
|
||
| This notes document some of the decisions that were made when implementing static parameter types enforcement in: | ||
|
ivanpauno marked this conversation as resolved.
Outdated
|
||
|
|
||
| * https://github.com/ros2/rclcpp/pull/1522 | ||
| * https://github.com/ros2/rclpy/pull/683 | ||
|
|
||
| ## Allowing dynamically typed parameters | ||
|
|
||
| There might be some scenarios were dynamic typing is desired, so a `dynamic_typing` field was added to the [parameter descriptor](https://github.com/ros2/rcl_interfaces/blob/09b5ed93a733adb9deddc47f9a4a8c6e9f584667/rcl_interfaces/msg/ParameterDescriptor.msg#L25). | ||
|
ivanpauno marked this conversation as resolved.
Outdated
|
||
| It defaults to `false`. | ||
|
|
||
| For example: | ||
|
|
||
| ```cpp | ||
| rcl_interfaces::msg::ParameterDescriptor descriptor; | ||
| descriptor.dynamic_typing = true; | ||
|
|
||
| node->declare_parameter("dynamically_typed_parameter", rclcpp::ParameterValue{}, descriptor); | ||
| ``` | ||
|
|
||
| ```py | ||
| rcl_interfaces.msg.ParameterDescriptor descriptor; | ||
| descriptor.dynamic_typing = True; | ||
|
|
||
| node.declare_parameter("dynamically_typed_parameter", None, descriptor); | ||
| ``` | ||
|
|
||
| ## How is the parameter type specified? | ||
|
|
||
| The parameter type will be inferred from the default value provided when declaring it. | ||
|
|
||
| ## Statically typed parameters when allowing undeclared parameters | ||
|
|
||
| When undeclared parameters are allowed, the descriptor of undeclared parameters will have the dynamic typing field set. | ||
| That's because in that case there is no declaration, and it doesn't make much sense to enforce the type of the first value set. | ||
|
wjwwood marked this conversation as resolved.
Outdated
|
||
|
|
||
| TBD: Should parameters that were declared with the dynamic typing option off enforce that the parameter doesn't change the type even if allow undeclared parameters is set? | ||
| In the current implementation that isn't enforce, but it might make sense to do it for parameters that were declared. | ||
|
ivanpauno marked this conversation as resolved.
Outdated
|
||
|
|
||
| ## Declaring a parameter without a default value | ||
|
|
||
| There might be cases were a default value does not make sense and the user must always provide an override. | ||
| In those cases, the parameter type can be specified explicitly: | ||
|
|
||
| ```cpp | ||
| // method signature | ||
| template<typename T> | ||
| Node::declare_parameter<T>(std::string name, rcl_interfaces::msg::ParameterDescriptor = rcl_interfaces::msg::ParameterDescriptor{}); | ||
| // or alternatively | ||
| Node::declare_parameter(std::string name, rclcpp::ParameterType type, rcl_interfaces::msg::ParameterDescriptor = rcl_interfaces::msg::ParameterDescriptor{}); | ||
|
|
||
| // examples | ||
| node->declare_paramter<int64_t>("my_integer_parameter"); // declare an integer parameter | ||
| node->declare_paramter("another_integer_parameter", rclcpp::ParameterType::PARAMETER_INTEGER); // another way to do the same | ||
| ``` | ||
|
|
||
| ```py | ||
| # method signature | ||
| Node.declare_parameter(name: str, param_type: rclpy.Parameter.Type, descriptor: rcl_interfaces.msg.ParameterDescriptor = rcl_interfaces.msg.ParameterDescriptor()) | ||
|
|
||
| # example | ||
| node.declare_paramter('my_integer_parameter', rclpy.Parameter.Type.INTEGER); # declare an integer parameter | ||
| ``` | ||
|
|
||
| If the parameter is optional, e.g.: only needed depending on the value of another parameter, the recommended approach is to conditionally declare the parameter: | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this was discussed a lot already, and implemented, so I'm just weighing my opinion here I don't mean to say it needs to change, but I don't care for how this make "requires override" implicit. I would have much rather preferred an explicit setting for "requires override" and if no type is given making the parameter dynamic and initialized to In the case that the user provides a name and a type but no default, then I'd say it should either be an error or it should default to the equivalent of 0, e.g. What I don't care for is, if a user asks "how do I make a parameter require an external override?" and the answer is "declare it by name and type but don't give a default value" then my question as a user would be "why?" and the answer depends on you understand quite a lot about how the parameter system works. I just seems like neat trick rather than a more intentional answer like "mark it as requires override". Also when reading code, it's hard to pick out the declare call (potentially amongst many) that omits a default value, whereas an explicit setting could be seen more easily and searched for. That's just my personal prespective. But I trust you guys discussed it and had reason for coming to this solution. That being said, this workaround isn't bad.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also...
We should be careful, I think, to be specific in our terminology. The logical "If..." following this section (in my opinion) is "If the parameter override is optional ..." (because in the examples above we're making the parameter override required). However, this case you've described here also makes sense, I think, but I would more accurately describe it as "If the parameter may be unused, but when used requires a parameter override, then you could ..." It's a subtle difference, but I think saying that the "parameter is optional" is inaccurate, imo it is being conditionally used, which is different from being optional. Optional implies it could be required (the opposite) and parameters cannot be required, only parameter overrides can be.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I should have written this before working on the implementation, so I don't mind doing any modification.
Do you mean that I deprecated that one to force people to think "did I really want a dynamically typed parameter?", but maybe that was too much.
I'm not sure how to make this less confusing, maybe the problem is naming (?).
That suggestion sounds fine to me
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Addressed in f245d42.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that's what I meant, yes. I don't feel strongly about it. I think that our users will probably want something convenient, but I'm ok with them having to opt into dynamic typing personally. For the other half, i.e. being explicit when asking for a required override, I think that's more important, because it's hard to know, looking at the source whether or not a given parameter is
So are there two use cases here?
I guess it makes sense that if you require an override a default value is meaningless. So they are related, but I wonder if the first ever makes sense? Why would you do that unless you wanted to require an override? I guess you wouldn't which leads you to the current behavior which is to say "if you do the first item above, it implies the second". I dunno, I'm just typing-out-loud here :) Maybe having side by side examples of each case would help, do we have documentation like this or an example where it's like "and this is how you do X with parameters, and this is how you do Y with parameters, etc..." basically like a parameters howto list or "cookbook"?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
So the other option is to allow "uninitialized statically typed parameters" which weren't supported because I didn't see their usefulness, though I might have overlooked it. I only imagine two scenarios were having an "uninitialized statically typed parameter" would make sense:
If we want to support "uninitialized statically typed parameters", I'm not sure what the descriptor type field should be in the uninitialized state. IMO, it should be the "required type", even if the parameter is uninitialized at the moment.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't think we need that, but again, I don't feel strongly about it.
I agree with that. |
||
|
|
||
| ```cpp | ||
| auto mode = node->declare_parameter("mode", "modeA"); // "mode" parameter is an string | ||
| if (mode == "modeB") { | ||
| node->declare_parameter<int64_t>("param_needed_when_mode_b"); // when "modeB", the user must provide "param_needed_when_mode_b" | ||
| } | ||
| ``` | ||
|
|
||
| ## Other migration notes | ||
|
|
||
| Declaring a parameter with only a name is deprecated: | ||
|
|
||
| ```cpp | ||
| node->declare_parameter("my_param"); // this generates a build warning | ||
| ``` | ||
|
|
||
| ```py | ||
| node.declare_parameter("my_param"); # this generates a python user warning | ||
| ``` | ||
|
|
||
| Before, you could initialize a parameter with the "NOT SET" value (in cpp `rclcpp::ParameterValue{}`, in python `None`). | ||
| Now this will throw an exception in both cases: | ||
|
ivanpauno marked this conversation as resolved.
|
||
|
|
||
| ```cpp | ||
| node->declare_parameter("my_param", rclcpp::ParameterValue{}); // not valid, will throw exception | ||
| ``` | ||
|
|
||
| ```py | ||
| node.declare_parameter("my_param", None); # not valid, will raise error | ||
| ``` | ||
Uh oh!
There was an error while loading. Please reload this page.