Adding parameter util to node utils

[MarcoMatteoBassa]

Basic Info

Info	Please fill out this column
Primary OS tested on	Ubuntu
Robotic platform tested on	Independent
Does this PR contain AI generated software?	No

---

Description of contribution in a few bullet points

The added utility tackles 2 issues:

• Reduces the amount of boilerplate code required to declare and get parameters, and consequently makes it less likely to introduce typos: it combines declare_parameter_if_not_declared and get_parameter in a single instruction.

Example:

nav2_util::declare_parameter_if_not_declared(
    node,
    "max_rotational_vel", rclcpp::ParameterValue(1.0));
  node->get_parameter("max_rotational_vel", max_rotational_vel_);

Can be rewritten as

max_rotational_vel_ = nav2_util::declare_or_get_param(node, "max_rotational_vel", 1.0);

• Helps tracking typos / mismatching namespaces in the parameters overrides: if a parameter is not found in the overrides, resulting in the default value being used, the utility can optionally print a WARN message. This can often help the developers catch configuration mistakes, and is particularly useful when relying on the default value likely results in degraded performance.

The utility can be used either through a generic node pointer, or (useful when using derived nodes) using the internal node interfaces.

Description of how this change was tested

• Added a unit test.
• As I didn't add the utilty to any node yet, it won't affect any existing behavior.

---

Future work that may be required in bullet points

• The utility can be used for refactoring purposes or for the development of new modules.

For Maintainers:

• Check that any new parameters added are updated in docs.nav2.org
• Check that any significant change is added to the migration guide
• Check that any new features OR changes to existing behaviors are reflected in the tuning guide
• Check that any new functions have Doxygen added
• Check that any new features have test coverage
• Check that any new plugins is added to the plugins page
• If BT Node, Additionally: add to BT's XML index of nodes for groot, BT package's readme table, and BT library lists

[codecov]

Codecov Report

Attention: Patch coverage is 96.00000% with 1 line in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
nav2_util/include/nav2_util/node_utils.hpp	96.00%	1 Missing ⚠️

Files with missing lines	Coverage Δ
nav2_util/include/nav2_util/node_utils.hpp	91.66% <96.00%> (+5.00%)	⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[SteveMacenski]

As a potential suggestion, this is declare_parameter_if_not_declared

template<typename NodeT>
void declare_parameter_if_not_declared(
  NodeT node,
  const std::string & param_name,
  const rclcpp::ParameterValue & param_value,
  const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor =
  rcl_interfaces::msg::ParameterDescriptor())
{
  if (!node->has_parameter(param_name)) {
    node->declare_parameter(param_name, param_type, parameter_descriptor);
  }
}

The declare_parameter bit can/will return a ParameterValue that we can work with if its not been declared, and otherwise we can add a get_parameter as an else condition to return the value.

We could actually just adapt the declare_parameter_if_not_declared to have a return type instead and use that as a single-call. Possibly even rename at that point to declare_or_get_parameter and use that globally, except the one situation below which is only in a handful of places.

Note: We also have a version for parameter types rather than values for the case in which we want to declare a parameter without a default value but only declare its type. We'd have to keep that in place with the current workflow of declare & get I think.

What do you think?

[MarcoMatteoBassa]

Hi, yes the util is basically doing that, but in addition it also removes the need to explicitally transform the parameters with rclcpp::ParameterValue() when calling it by using the type template.
The WARN part is also quite handy, but if you feel that would be too spammy, we could default it to false.

I didn't want to modify declare_parameter_if_not_declared to avoid intrusion in the code base and possibly generate unused-return-value warnings, but using an unified strategy later would for sure be beneficial :)

Yes, the Type-Only version of declare_parameter_if_not_declared should stay there. As in this case the parameter might not have a value, I would leave it as void; alternatively we could add a version of declare_or_get returning an optional rclcpp::ParameterValue if an rclcpp::ParameterType is used.

Let me know what you prefer :)

[SteveMacenski]

I suppose I'm less tied to a particular implementation and more interested in just removing redundancy as much as possible. Maybe we don't need to do everything at once :-) It would be nice as you say to have a version that doesn't produce a unused return value warning --

Yes, the Type-Only version of declare_parameter_if_not_declared should stay there. As in this case the parameter might not have a value, I would leave it as void; alternatively we could add a version of declare_or_get returning an optional rclcpp::ParameterValue if an rclcpp::ParameterType is used.

We could add in the version but allow get_parameter throw in the case it was not declared (as it does). As long as the doxygen mentions this behavior, I think that's OK. So it would return a value if one is given, else throws. That should be a known behavior in that case and I believe all uses in Nav2 wraps those calls in try/catch anyway to capture it and allow for a 1:1 replacement.

The WARN part is also quite handy, but if you feel that would be too spammy, we could default it to false.

I do like default false personally. I'm curious to know why its a warning to use a default value - is your expectation that all nodes are full configured in YAML files? I suppose that is good practice, but would spam the terminal quite alot for existing users that are less disciplined (including nav2_bringup currently 🥲 )

[MarcoMatteoBassa]

We could add in the version but allow get_parameter throw in the case it was not declared (as it does). As long as the doxygen mentions this behavior, I think that's OK. So it would return a value if one is given, else throws. That should be a known behavior in that case and I believe all uses in Nav2 wraps those calls in try/catch anyway to capture it and allow for a 1:1 replacement.

I added a version that handles the declaration by type, it will return a parameter_not_set type if the param is not there. The exception will eventually come when trying to cast if that is done without checking the type.

I do like default false personally. I'm curious to know why its a warning to use a default value - is your expectation that all nodes are full configured in YAML files? I suppose that is good practice, but would spam the terminal quite alot for existing users that are less disciplined (including nav2_bringup currently 🥲 )

Yes, having the warning combined with fully configured YAML files has a lot of advantages:

• You always know if the param defined in the yaml takes effect -> It's easy to spot typos in the parameter names or when these are defined in the wrong namespace
• Makes it easier to maintain compatibility between the configuration files and the source code (which is particularly challenging when maintaining multiple robots/environments that require the yamls to be defined in separate packages)
• Makes it easier for the user to understand what can be configured in the node without having to call param_list at runtime -> the YAML files can become part of the documentation

But I don't want to enforce this on everyone, so I defaulted to false now :)

[SteveMacenski]

Honestly I totally agree. If I was building applications I would be fully describing my configuration files. However, I don't want to overwhelm people reading the config file for the first few times (its already alot) so that's why nav2_bringup isn't as complete

[SteveMacenski]

LGTM, ready to merge on your front?

[SteveMacenski]

Just to trigger that warning message to complete coverage metrics

[SteveMacenski]

Mhm, there are some tests that are failing, can you rebase? We did make some changes in the default bringup recently related to the failing tests and I'm wondering if there's a caching issue.

[mergify]

@MarcoMatteoBassa, your PR has failed to build. Please check CI outputs and resolve issues.
You may need to rebase or pull in main due to API changes (or your contribution genuinely fails).

[MarcoMatteoBassa]

I realized that subsequent calls of declare_or_get by type would result in an exception instead of an uninitialized parameter, so I changed it to consistently return an uninitialized parameter.

PS, a possible way to allow the user to globally choose for a node if he wants warnings for missing overrides, could be to enable it through an optional dedicated boolean parameter, ex by changing it to:

template<typename ParamType, typename NodeT>
ParamType declare_or_get_parameter(
  NodeT node, const std::string & parameter_name,
  const ParamType & default_value, const ParameterDescriptor & parameter_descriptor = ParameterDescriptor())
{
  const auto ParamBool = rclcpp::ParameterType::PARAMETER_BOOL;
  auto warn_value = declare_or_get_parameter(node, "warn_missing_parameter_overrides", ParamBool);
  const bool warn = warn_value.get_type() == ParamBool && warn_value.template get<bool>();
  return declare_or_get_parameter(node->get_logger(), node->get_node_parameters_interface(),
      parameter_name, default_value, warn, parameter_descriptor);
}

In this way there wouldn't be a need to hard-code a choice in the function call, and by default there would be no warning.

[SteveMacenski]

Love it, anything else or can I merge?

Sorry for the back and forth, I think this is better though because of it!

[MarcoMatteoBassa]

All good from my side :) Maybe I'll do a follow-up proposal for enabling the warning through a parameter, but that might not be very "elegant".