Skip to content

Document alternative for declaring an optional parameter #1588

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

Document alternative for declaring an optional parameter #1588

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions rclcpp/doc/notes_on_statically_typed_parameters.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,16 @@ if (mode == "modeB") {
}
```

Or, wrap the declaration in a try-catch:

```cpp
try {
node->declare_parameter<int64_t>("param_that_is_optional");
} catch (const rclcpp::exceptions::NoParameterOverrideProvided &) {
// a parameter value was not provided
}
```
Comment on lines +88 to +96
Copy link
Member

@wjwwood wjwwood Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think to be equivalent to the above, wouldn't it need to be like this:

Suggested change
Or, wrap the declaration in a try-catch:
```cpp
try {
node->declare_parameter<int64_t>("param_that_is_optional");
} catch (const rclcpp::exceptions::NoParameterOverrideProvided &) {
// a parameter value was not provided
}
```
Or, wrap the declaration in a try-catch:
```cpp
auto mode = node->declare_parameter("mode", "modeA");
try {
node->declare_parameter<int64_t>("param_that_is_optional");
} catch (const rclcpp::exceptions::NoParameterOverrideProvided &) {
// a parameter value was not provided
if (mode == "modeB") {
// parameter and parameter override was required
throw;
}
}
```

What you've described here is more like "conditionally declare a parameter if an override is given", which is like a subset of

rclcpp/rclcpp/include/rclcpp/node_options.hpp

Lines 329 to 343 in 7bfda87

/// Set the automatically_declare_parameters_from_overrides, return this.
/**
* If true, automatically iterate through the node's parameter overrides and
* implicitly declare any that have not already been declared.
* Otherwise, parameters passed to the node's parameter_overrides, and/or the
* global arguments (e.g. parameter overrides from a YAML file), which are
* not explicitly declared will not appear on the node at all, even if
* `allow_undeclared_parameters` is true.
* Already declared parameters will not be re-declared, and parameters
* declared in this way will use the default constructed ParameterDescriptor.
*/
RCLCPP_PUBLIC
NodeOptions &
automatically_declare_parameters_from_overrides(
bool automatically_declare_parameters_from_overrides);
.

Also, I'm kind of concerned with the conditionally declared parameters. I'm not saying they shouldn't be allowed, but if I were making a system, I'd prefer the parameters to be the same no matter what, and if one was not provided (and that was ok) it should be "not set" or something. I'm thinking about "dumping" parameters from a node in various scenarios. It's maybe undesirable to have different number and order of parameters in the dump depending on the circumstances, because then if a parameter is missing from the dump, it's hard to know if it was not specified or if the dump maybe came from a difference version of the program that didn't have the parameter yet/any longer. But that's just a architectural decision for the developer of the node.

Copy link
Member Author

@jacobperron jacobperron Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What you've described here is more like "conditionally declare a parameter if an override is given"

Right, this is exactly what I was trying to do. For situations where we want an optional parameter, but perhaps there are no default values that make sense. I guess what you're suggesting is that the author should try to avoid putting themselves in this situation.

This came up because I've already hit several places in the wild where code is trying to declare optional parameters, for example: cra-ros-pkg/robot_localization#631 (comment)

It would be nice if declare_parameter wouldn't throw, but we made that compromise for technical reasons, IIRC.

Copy link
Member Author

@jacobperron jacobperron Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe what we need is a representation for a value that is "not set"; currently, we only allow the type to be "not set".

Copy link
Member

@wjwwood wjwwood Mar 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think this is essentially what @ivanpauno was talking about with "uninitialized parameters". It can have a type, say int64_t, but the value is either an int64_t or uninitialized.

Copy link
Member

@ivanpauno ivanpauno Apr 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like @wjwwood suggestion here.
If not, the try/catch block isn't completely equivalent.

Also, I'm kind of concerned with the conditionally declared parameters. I'm not saying they shouldn't be allowed, but if I were making a system, I'd prefer the parameters to be the same no matter what, and if one was not provided (and that was ok) it should be "not set" or something. I'm thinking about "dumping" parameters from a node in various scenarios.

That sounds fair.


## Other migration notes

Declaring a parameter with only a name is deprecated:
Expand Down