ros2/rclcpp#1436 implements the above proposal except this point:

Should it be possible to declare a parameter with static_typing=true and type=PARAMETER_NOT_SET?
I would say no,

Though adding that is easy.

My one hesitation here is that making this change means that Galactic will not be able to communicate with Foxy and earlier (at least, for ParameterDescriptor)

Yeah, wire compatibility is hard in this case.
I think that the only way to achieve that here would be to reuse the type field which is already part of the parameter description.

Should it be possible to declare a parameter with static_typing=true and type=PARAMETER_NOT_SET?
I would say no

I agree.

Should it be possible to undeclare a parameter when static_typing=true? IMO, no.

I agree with this, too.

Should it be possible to declare a parameter with static_typing=true and type=PARAMETER_NOT_SET?
I would say no

While I agree with this in principle, I think this will cause a lot of downstream headaches. For instance, I know that navigation2 does things like this: https://github.com/ros-planning/navigation2/blob/11b25a51981eb53db6a6a3601729ae85238d9241/nav2_dwb_controller/dwb_core/src/dwb_local_planner.cpp#L79 . That declares a parameter as NOT_SET by default, but expects the user to explicitly set this. I think that would run afoul of this limitation. So I think this is one we'll have to allow for pragmatic reasons.

Should it be possible to undeclare a parameter when static_typing=true? IMO, no.

I agree with this one.

While I agree with this in principle, I think this will cause a lot of downstream headaches. For instance, I know that navigation2 does things like this: https://github.com/ros-planning/navigation2/blob/11b25a51981eb53db6a6a3601729ae85238d9241/nav2_dwb_controller/dwb_core/src/dwb_local_planner.cpp#L79 .

That doesn't seem like a problem, as they are already "failing" when a default value is not provided.
So it seems to me that they are never declaring a parameter without a default value (i.e. type = NOT_SET).

The name of the parameter (I like my suggested name slightly better)

That sounds good to me.

Adding some documentation to the Galactic release notes about this semantic change.

Probably, it's worth creating a thread in ros discourse, though I would like first to have an implementation working with all the details sorted out.

@jacobperron friendly ping, what are your thoughts about this?

That doesn't seem like a problem, as they are already "failing" when a default value is not provided.
So it seems to me that they are never declaring a parameter without a default value (i.e. type = NOT_SET).

I'm a bit confused by this point. I think the concern is valid. The nav stack defaults several parameters to NOT_SET and then is checking that the use provides a value. So, for this type of use-case, users will not be able to use statically typed parameters (unless we allow having an initial NOT_SET state). If users were able to detect if a default value was overridden, then they could use that signal as a workaround.

Perhaps it's okay to disallow statically typed parameters that default to NOT_SET, but I think we want to be sure in that choice, since it looks like changing our minds later will require another change to ParameterDescriptor.msg.

The nav stack defaults several parameters to NOT_SET and then is checking that the use provides a value

I see, yes they seem to be sometimes not passing a default value.

IMHO, the use case we're missing is when you want a "statically typed" parameter with no default value.
In that case, I would expect to declare the parameter passing the parameter type, and if the user didn't provide an override the first "get" throws an exception (or maybe "declare" should throw (?)).

Something like:

// throws if an override is not provided, or maybe the first `get()` does and `declare` doesn't throw.
node->declare_parameter(param_name, descriptor, rclcpp::ParameterType::Integer);

or

// throws if an override is not provided, or maybe the first `get()` does and `declare` doesn't throw.
descriptor.type = ParameterType::Integer;
node->declare_parameter(param_name, descriptor);

We would need an allowed_types field in the descriptor, or reuse the existing type field differently as mentioned here.

I like the allowed_types field. We could use a bitmask. This acts as a replacement for the statically_typed field, right?

I like the allowed_types field. We could use a bitmask.

I was actually thinking of a single type (i.e. allowed_type and not allowed_types, sorry for the misspelling), but an allowed_types bitmask is also an option.

This acts as a replacement for the statically_typed field, right?

Yes, we wouldn't need both.

Maybe I'm wrong, but it seems like we would get a lot more flexibility for little effort in allowing multiple types. I think the API you described would look the same in the single allowed type case. E.g., only allow integers

node->declare_parameter(param_name, descriptor, rclcpp::ParameterType::Integer);

or allow integers and strings

node->declare_parameter(param_name, descriptor, rclcpp::ParameterType::Integer | rclcpp::ParameterType::String);

I'm okay if we don't support this, but seems like a nice way to further avoid user-defined parameter callbacks.

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/feedback-required-should-parameters-be-able-to-change-of-type/18319/1

I'm okay if we don't support this, but seems like a nice way to further avoid user-defined parameter callbacks.

Yes, I don't mind strongly.
IMO, it looks like a bit of an overkill because I don't think much people is going to use the feature, but it's also not hard to support it.

It also might help us avoid a future change to the message definition if we decided to support it later.

or allow integers and strings

What's the use case for a parameter that can have such vastly different types?
Can't think of one that wouldn't be bad code design but maybe I'm just not thinking hard enough. It's Friday after all :)

What's the use case for a parameter that can have such vastly different types?

Good point. I can't think of any specific cases. And I agree it would be bad design. I think I was under the wrong impression that people were currently using parameters with dynamic types (since that's the current default behavior).

I change my mind and think a single "allowed type" is a good option.

I change my mind and think a single "allowed type" is a good option.

@jacobperron do you have any preference between the currently proposed "static_typing" boolean and "allowed_type" field?
(in both cases you can know the actual type by checking the "type" field)

I would like:

node->declare_parameter("an_int", 5);  // parameter_descriptor argument takes default value

to declare an integer parameter that cannot change of type.
In the case of an "static_typing" boolean, the default can be true (like the proposed PR here), and you don't have to modify the default parameter descriptor.
In the case of an "allowed_type" field if this behavior is desired, "declare_parameter" would need to overwrite the "allowed_type" field of the provided descriptor and to avoid that behavior we would need an extra flag "dynamic_typing" argument in declare_parameter.

Moreover in this case:

rcl_interfaces::msg::ParameterDescriptor descriptor;
descriptor.integer_range = /*set integer range to something here*/;
node->declare_parameter("an_int", 5, descriptor);

do you need to write descriptor.allowed_type = rclcpp::PARAMETER_TYPE_INT or is that implicit?
In the case of static_typing, no issue here as it defaults to true.

The only problem is when no default value is provided, that as commented above that seems to be used currently.
I think the options are:

• In case we go ahead with the "static_typing" field, if the user didn't provide a default value we can either:
  • throw an exception
  • set the parameter to "PARAMETER_NOT_SET". "static_typing" starts being enforced when the first actual value is given (taking the type of the first value as the type to enforce).
• In the case of "allowed_type":
  • throw an exception
  • set the parameter to "PARAMETER_NOT_SET". The first value being set must satisfy the allowed type (and after that it cannot be undeclared).

The details are trivial to implement, but I'm not sure what is the preferred way forward ...

A static_typing field (defaulting to true) sounds okay to me.

In the case where no parameter default value is provided, I'd rather not throw an exception since it seems like a valid use-case to me i.e., enforce that the user make a decision on a value instead of letting them use a default. I could be persuaded otherwise though.

i.e., enforce that the user make a decision on a value instead of letting them use a default

I'm not sure what that exactly means.
The only possibility, if not throwing an exception, is to declare the parameter with type "PARAMETER_NOT_SET" and wait until it's set the first time to decide it's type (which is what ros2/rclcpp#1512 does).
But that might be confusing, because it will take the type of the provided override.

I'm working on an alternative of the currently opened PRs, in order to experiment what's the best way to modify the behavior without breaking current use cases. I think we can discuss the final details in the review process

i.e., enforce that the user make a decision on a value instead of letting them use a default

I'm not sure what that exactly means.

I mean, I think it's a reasonable use-case to have a "required" parameter. In this case the parameter is not set, requiring the user to set it before it is used. Whether the type is set by the node author or inferred by the first override is another design decision that needs to be made. It would be nice for the node author to be able to specify the allowed type for an unset parameter, but I'm also okay with just inferring the type from the override to keep things simple.

Closing in favor of #118.