declare_parameter<T>(param_name) fails

[m2-farzan]

Bug report

Required Info:

• Operating System:
  • Archlinux @ June, 9 [GCC 11.1.0]
• Installation type:
  • From source
• Version or commit hash:
  • master (7d8b269)
• DDS implementation:
  • rmw_fastrtps_cpp

Steps to reproduce issue

Try to run the following code:

#include "rclcpp/rclcpp.hpp"
#include <string>

int main(int argc, char *argv[])
{
    rclcpp::init(argc, argv);
    rclcpp::Node mynode("name");
    mynode.declare_parameter<std::string>("myparam");
    rclcpp::shutdown();
}

Expected behavior

Run and exit successfully.

Actual behavior

Compiles but in runtime exits due to an exception. Full output:

terminate called after throwing an instance of 'rclcpp::ParameterTypeException'
  what():  expected [string] got [not set]
Aborted (core dumped)

Additional information

This call fails:

mynode.declare_parameter<std::string>("myparam");

(This is not specific to std::string)

Here's my understanding of why this happens: Under the hood, rclcpp declares an uninitialized parameter and tries to return it as a typed object (code here). However, it seems that rclcpp keeps track of uninitialized parameters by assigning not set type to them (code here), even if they are typed. So this boils down to contradictory typing (string type implied by template argument, not set type implied by the node being uninitialized...)

Interestingly the mynode.declare_parameter<T>(param_name) function call is actually included in the tests (code here) and they are passing without errors. The reason that the error doesn't manifest there is that in the same test file, a default value is defined for the parameter (code here). This means the parameter gets initialized (and therefore loses the not set type) before mynode.declare_parameter<T>(param_name) finishes.

[ivanpauno]

This one is unfortunate, but I don't think we can solve it.
The error message should be clearer though.

When we decided to make parameters "strongly typed" by default (#1522), we didn't originally support to declare them uninitialized.
You either needed to pass a default value or the user must provide an override of the correct type (e.g. <param_name>:=<param_value>) when running the node.

We later relaxed that requirement (#1681), and it is now possible to do the following:

rclcpp::ParameterValue param = node->declare_parameter("integer_override_not_given", rclcpp::PARAMETER_INTEGER);

The return type here is a rclcpp::ParameterValue, which will be of type int64_t if the parameter is initialized or of type "PARAMETER_NOT_SET" when not.

The templated version returns directly the real parameter type and not an rclcpp::ParameterValue, so it's impossible to achieve similar behavior:

// What do we return if the parameter is uninitialized?
int64_t param = node->declare_parameter<int64_t>("integer_override_not_given");

So I don't think we can fix this for the templated version, and the non-template method should be used when you want to allow the parameter to be uninitialized.

---

TBH, I completely forgot about the templated overload when we introduced #1681.
I think that at least the error message should be better (and maybe also the docs).

@jacobperron what do you think?

[m2-farzan]

IMHO the clean way would be to avoid using 'type' to indicate whether a parameter is initialized or not. It makes sense to have a separate initialized flag in ParameterValue for this purpose. This is an addition but the logical correctness will make everything smoother.

Just my two cents...

[ivanpauno]

IMHO the clean way would be to avoid using 'type' to indicate whether a parameter is initialized or not

"type" doesn't indicate if a parameter is initialized or not.
@m2-farzan I don't understand what you exactly mean, could you provide a complete example?

[m2-farzan]

Sorry, let me put it another way.

You said it:

The return type here is a rclcpp::ParameterValue, which will be of type int64_t if the parameter is initialized or of type "PARAMETER_NOT_SET" when not.

What I'm suggesting here is to return a ParameterValue of type int64_t regardless of whether the parameter is initialized or not. The information about initialized/uninitialized state will be stored in a new boolean member of ParameterValue, potentially called initialized.

[ivanpauno]

What I'm suggesting here is to return a ParameterValue of type int64_t regardless of whether the parameter is initialized or not. The information about initialized/uninitialized state will be stored in a new boolean member of ParameterValue, potentially called initialized.

Thanks for clarifying!
That seems like a valid approach but I don't think it's worth breaking API as what you want to do is still possible now.

I would just focus in making the error message more clear, but I will wait for @jacobperron input first.

[jacobperron]

I also overlooked the template overload... we could certainly improve the error message.
It doesn't seem like something we can "fix" for Galactic, in order to keep API stability. We could introduce an additional "initialized" state variable, but I'm not sure if it's worth the churn. As @ivanpauno points out, we have a way to declare a parameter without a value. Also, if we changed the signature of the templated overload to return a rclcpp::ParameterValue, we can already check if the parameter is initialized by checking the value's type. For example, the following code works for me:

rclcpp::ParameterValue param = node->declare_parameter("foo", rclcpp::PARAMETER_INTEGER);
if (param.get_type() == rclcpp::PARAMETER_NOT_SET) {
  // Parameter is unset
}

So, I'm not sure if a new "initialized" member adds value.

[jacobperron]

It might be valuable to support returning a rclcpp::ParameterValue object from the templated method (I haven't thought about how technically feasible it is). I.e.

rclcpp::ParameterValue param = node->declare_parameter<int64_t>("foo");

Then we wouldn't get an exception, and could check if the parameter is set before actually accessing the value.

[ivanpauno]

It might be valuable to support returning a rclcpp::ParameterValue object from the templated method (I haven't thought about how technically feasible it is)

It's not possible to have two overloads with the same argument and different return value, so the only way to do that is to change the return value of the current method.

For the moment, I will improve the error message.
We can later decide if we want to modify the templated method signature or not.