-
Couldn't load subscription status.
- Fork 475
Parameter service compliance #2515
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
base: rolling
Are you sure you want to change the base?
Conversation
Signed-off-by: Michael X. Grey <[email protected]>
|
I'll need to update the unit tests for the parameter services to match the new behavior. |
| auto default_description = rcl_interfaces::msg::ParameterDescriptor(); | ||
| default_description.name = name; | ||
| response->descriptors.push_back(default_description); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not move these lined in to catch statement scope? In that case, probably we do not need to have description_found stack variable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use description.at(0) and intentionally segfault when the returned vector is empty, but that will produce a different kind of exception than ParameterNotDeclaredException. So then we'd have to either duplicate code between multiple catch blocks or else make a catch block that covers all exception types, which might hide other kinds of exceptions that should be handled differently.
In general I think exceptions are a rather blunt instrument so I prefer to avoid using them whenever possible. We could avoid exceptions if we are willing to make one of the following tweaks to NodeParametersInterface:
(1) Add a new argument to describe_parameters to override the behavior when allow_undeclared_parameters is false:
std::vector<rcl_interfaces::msg::ParameterDescriptor>
describe_parameters(const std::vector<std::string> & names, bool override_allow_undeclared_false=false) const = 0;
or (2) have a function that gets parameter descriptions one at a time, returning std::nullopt when the parameter is undeclared:
std::optional<rcl_interfaces::msg::ParameterDescriptor>
describe_parameter(const std::string & name) const = 0;
I don't have a strong opinion about which is better...
Option (1) would make it easy to avoid any code duplication, but the argument is awkward, and I can't think of a name for it that wouldn't be confusing.
Option (2) is probably the cleanest from an API standpoint, but then we have to consider the mutex locking situation. The natural thing to do would be to reimplement NodeParameters::describe_parameters by iterating over the new NodeParameters::describe_parameter method, but that might not be great... Currently NodeParameters::describe_parameters locks the mutex once at the start of the function and then iterates through all the names while keeping the mutex locked. In theory this reduces thread contention and makes the overall procedure more efficient compared to relocking the mutex for each parameter that we want to describe. We could introduce a NodeParameters::describe_parameter_impl which doesn't lock the mutex and is used to implement both NodeParameters::describe_parameter and NodeParameters::describe_parameters.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess a third option would be to reimplement NodeParameters::describe_parameter to just not behave differently under allow_undeclared_parameters(false), but that wouldn't align with the other rclcpp parameter functions that use exceptions to enforce the parameter declaration constraint.
Signed-off-by: Michael X. Grey <[email protected]>
I'm opening this draft to help in the discussion of #2512 . I'll make it a full fledged PR if it gets buy-in from the maintainers.