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.

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

Jump to bottom

Parameter service compliance #2515

Draft
wants to merge 4 commits into
base: rolling
Choose a base branch
from
Draft

Parameter service compliance #2515

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9bd5769
Comply with rcl_interfaces parameter services documentation
mxgrey Apr 26, 2024
dfd40a2
Merge remote-tracking branch 'origin/rolling' into parameter_service_…
mxgrey Apr 26, 2024
1f8620a
Merge remote-tracking branch 'origin/rolling' into parameter_service_…
mxgrey Apr 29, 2024
5db9ffa
Use a constant value that is set upstream
mxgrey Apr 29, 2024
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
57 changes: 33 additions & 24 deletions rclcpp/src/rclcpp/parameter_service.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,15 +41,11 @@ ParameterService::ParameterService(
const std::shared_ptr<rcl_interfaces::srv::GetParameters::Request> request,
std::shared_ptr<rcl_interfaces::srv::GetParameters::Response> response)
{
try {
auto parameters = node_params->get_parameters(request->names);
for (const auto & param : parameters) {
response->values.push_back(param.get_value_message());
}
} catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameters: %s", ex.what());
} catch (const rclcpp::exceptions::ParameterUninitializedException & ex) {
RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameters: %s", ex.what());
response->values.reserve(request->names.size());
for (const auto & name : request->names) {
rclcpp::Parameter value;
node_params->get_parameter(name, value);
response->values.push_back(value.get_value_message());
}
},
qos_profile, nullptr);
Expand All @@ -62,15 +58,11 @@ ParameterService::ParameterService(
const std::shared_ptr<rcl_interfaces::srv::GetParameterTypes::Request> request,
std::shared_ptr<rcl_interfaces::srv::GetParameterTypes::Response> response)
{
try {
auto types = node_params->get_parameter_types(request->names);
std::transform(
types.cbegin(), types.cend(),
std::back_inserter(response->types), [](const uint8_t & type) {
return static_cast<rclcpp::ParameterType>(type);
});
} catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to get parameter types: %s", ex.what());
response->types.reserve(request->names.size());
for (const auto & name : request->names) {
rclcpp::Parameter value;
node_params->get_parameter(name, value);
response->types.push_back(value.get_type());
}
},
qos_profile, nullptr);
Expand All @@ -93,7 +85,7 @@ ParameterService::ParameterService(
} catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to set parameter: %s", ex.what());
result.successful = false;
result.reason = ex.what();
result.reason = "Parameter is not declared by the node, which only allows declared parameters";
fujitatomoya marked this conversation as resolved.
Show resolved Hide resolved
}
response->results.push_back(result);
}
Expand Down Expand Up @@ -135,11 +127,28 @@ ParameterService::ParameterService(
const std::shared_ptr<rcl_interfaces::srv::DescribeParameters::Request> request,
std::shared_ptr<rcl_interfaces::srv::DescribeParameters::Response> response)
{
try {
auto descriptors = node_params->describe_parameters(request->names);
response->descriptors = descriptors;
} catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
RCLCPP_DEBUG(rclcpp::get_logger("rclcpp"), "Failed to describe parameters: %s", ex.what());
// TODO(@mxgrey): This could have fewer heap allocations if
// NodeParametersInterface had a singular describe_parameter function or
// if describe_parameters could accept an argument to behave as if
// allow_undeclared_ is true.
for (const auto & name : request->names) {
bool description_found = false;
try {
auto description = node_params->describe_parameters({name});
if (!description.empty()) {
description_found = true;
response->descriptors.push_back(std::move(description.front()));
}
} catch (const rclcpp::exceptions::ParameterNotDeclaredException & ex) {
// If the parameter was not declared then we will just push_back a
// default-initialized descriptor.
}

if (!description_found) {
auto default_description = rcl_interfaces::msg::ParameterDescriptor();
default_description.name = name;
response->descriptors.push_back(default_description);
Comment on lines +148 to +150
Copy link
Collaborator

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?

Copy link
Contributor Author

@mxgrey mxgrey Apr 29, 2024 •
edited
Loading

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.

Copy link
Contributor Author

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.

}
}
},
qos_profile, nullptr);
Expand Down