Can't set initial parameters when allowing undeclared

[chapulina]

Bug report

Required Info:

• Operating System:
  • Ubunto Bionic
• Installation type:
  • dashing sources
• Version or commit hash:
  • master

Steps to reproduce issue

Setting to accept undeclared parameters and passing initial parameters:

  std::vector<rclcpp::Parameter> initial_parameters =
      {rclcpp::Parameter("my_string", std::string("str"))};
  rclcpp::NodeOptions node_options;
  node_options.allow_undeclared_parameters(true);
  node_options.initial_parameters(initial_parameters);
  auto node = std::make_shared<Node>("name", "ns", node_options);
  node->get_parameter(param);  // throws

Throws this:

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

On the other hand, setting the parameters after construction is happy:

  rclcpp::NodeOptions node_options;
  node_options.allow_undeclared_parameters(true);
  auto node = std::make_shared<Node>("name", "ns", node_options);
  node->set_parameters(initial_parameters);
  node->get_parameter(param); // happy

Expected behavior

No throw on both cases

Actual behavior

Throw on first case

[dirk-thomas]

What is initial_parameters / param in your example?

[chapulina]

What is initial_parameters / param in your example?

Added to the description

[ivanpauno]

I haven't been able to reproduce the bug you're reporting, using master.

Initial parameters aren't supposed to be automatically declared, except you use node_options.automatically_declare_initial_parameters(true):

rclcpp/rclcpp/include/rclcpp/node_options.hpp

Lines 260 to 272 in 30df5cd

	/// Set the automatically_declare_initial_parameters, return this.
	/**
	* If true, automatically iterate through the node's initial parameters and
	* implicitly declare any that have not already been declared.
	* Otherwise, parameters passed to the node's initial_parameters, and/or the
	* global initial parameter values, which are not explicitly declared will
	* not appear on the node at all.
	* Already declared parameters will not be re-declared, and parameters
	* declared in this way will use the default constructed ParameterDescriptor.
	*/
	RCLCPP_PUBLIC
	NodeOptions &
	automatically_declare_initial_parameters(bool automatically_declare_initial_parameters);

.

If you add a has_parameter call, you will see that the parameter hasn't been declared.

  std::vector<rclcpp::Parameter> initial_parameters =
      {rclcpp::Parameter("my_string", std::string("str"))};
  rclcpp::NodeOptions node_options;
  node_options.allow_undeclared_parameters(true);
  node_options.initial_parameters(initial_parameters);
  auto node = std::make_shared<Node>("name", "ns", node_options);
  printf("has_parameter: %d\n", node->has_parameter("my_string")); // prints 0

If you add the following line before creating the node:

  node_options.automatically_declare_initial_parameters(true);

You will see that has_parameter returns true and your problem disappears.

And actually, get_parameter never throws by design when allow_undeclared_parameters is true:

rclcpp/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

Lines 554 to 585 in 30df5cd

	rclcpp::Parameter
	NodeParameters::get_parameter(const std::string & name) const
	{
	rclcpp::Parameter parameter;
	if (get_parameter(name, parameter)) {
	return parameter;
	} else if (this->allow_undeclared_) {
	return parameter;
	} else {
	throw rclcpp::exceptions::ParameterNotDeclaredException(name);
	}
	}
	bool
	NodeParameters::get_parameter(
	const std::string & name,
	rclcpp::Parameter & parameter) const
	{
	std::lock_guard<std::mutex> lock(mutex_);
	auto param_iter = parameters_.find(name);
	if (
	parameters_.end() != param_iter &&
	param_iter->second.value.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET)
	{
	parameter = {name, param_iter->second.value};
	return true;
	} else {
	return false;
	}
	}

The exception you are seeing came from a call you are doing later to rclcpp::Parameter::get_value<std::string>(), something like:

  auto param = node->get_parameter("my_string");
  param.get_value<std::string>(); // throws if you haven't set automatically_declare_initial_parameters

If this not answer your question, please share the complete example. Thanks!

[Karsten1987]

But isn't this weird that even though we've set the initial parameters, the call to has_parameter return 0? I would expect the initial parameters to be set independently from allow_undeclared.

[ivanpauno]

But isn't this weird that even though we've set the initial parameters, the call to has_parameter fails?

Maybe, allow_undeclared_parameters should set automatically_declare_initial_parameters to true (or the node_parameters interface should ignore automatically_declare_initial_parameters value and automatically declare them when allow_undeclared_parameters is true).
I think it should be still false by default if allow_undeclared_parameters is false.

@wjwwood any opinions?

Edit:
I think this affects in the same way parameters loaded from a yaml file (haven't tested).

[chapulina]

I haven't been able to reproduce the bug you're reporting in master.

I can work on a better example soon.

Maybe, allow_undeclared_parameters should set automatically_declare_initial_parameters to true

+1 to this

The documentation says

   * If undeclared parameters are allowed, then the parameter is implicitly
   * declared with the default parameter meta data before being set.

So I would expect that to also happen to undeclared parameters set through the constructor without needing to also set automatically_declare_initial_parameters.

[wjwwood]

I cannot reproduce the issue in the original post, get_parameter doesn't throw for me (my demo below).

---

Either way, I'd recommend these as the "golden rules" for parameters in Dashing:

• Always at least use declare_parameter(s), and use it before using any has/get/set parameter methods.
• Avoid using allow_undeclared_parameters and automatically_declare_initial_parameters except in exceptional cases, you most likely should not be using them.

---

I think the fundamental misunderstanding is that NodeOptions::initial_parameter is not intended to seed the node with parameters, it is actually only for overriding parameter defaults (same goes for the YAML file). I think this is a reasonable thing to be confused about, given the name, so perhaps we should change it. Most places I refer to this as the "initial parameter values" rather than "initial parameters".

The automatically_declare_initial_parameters is essentially a helper which uses those overrides to declare the parameters for you.

The documentation says

That means when you, the user, set a value for a parameter (using set_parameter(s)) it gets automatically declared (as if you had declare_parameter), but it is undeclared until you set it. If you only ever use initial parameter values and get, then the parameter will never be declared.

So I would expect that to also happen to undeclared parameters set through the constructor without needing to also set automatically_declare_initial_parameters.

I think you're misunderstanding allow_undeclared_parameters, it only avoids two things. First, throwing when you 'get' a parameter that has not been declared explicitly, and instead returns a rclcpp::Parameter with the type set to PARAMETER_NOT_SET (essentially like the None type in Python). Second, it also will avoid throwing when you use set_parameter(s)*, instead it implicitly declares the parameter and then sets it.

It does not allow for parameters to be declared automatically just because they're passed as initial parameters or in the YAML file.

Maybe, allow_undeclared_parameters should set automatically_declare_initial_parameters to true

This came up in the original review and I don't think anything has changed since then. There are cases where both are enabled, one or the other is enabled, and when both are disabled that all make sense. I don't think we should couple their behavior. If you want both behaviors, then enable them both.

---

I believe I've covered this topic quite thoroughly in the migration docs:

https://index.ros.org/doc/ros2/Releases/Release-Dashing-Diademata/#declaring-parameters

If you guys are still confused by it then we definitely need to update it, but I honestly don't know how I could have made it clearer.

---

My example (it compiles and runs without error on master):

#include "rclcpp/rclcpp.hpp"

int main(int argc, char * argv[])
{
  rclcpp::init(argc, argv);

  rclcpp::NodeOptions options;
  options.allow_undeclared_parameters(true);
  options.initial_parameters({{"bar", "baz"}});
  auto node = std::make_shared<rclcpp::Node>("test_node", "", options);

  node->get_parameter("foo");

  return 0;
}

[wjwwood]

Furthermore, as @ivanpauno pointed out, the exception you're getting is not due to it not being declared first, it is due to you trying to coerce it into a string when it has no value.

[chapulina]

Failing test:

diff --git a/rclcpp/test/test_node.cpp b/rclcpp/test/test_node.cpp
index bb3ac35..95cddfe 100644
--- a/rclcpp/test/test_node.cpp
+++ b/rclcpp/test/test_node.cpp
@@ -722,6 +722,24 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_allowed) {
   }
 }
 
+TEST_F(TestNode, initial_parameters_undeclared_parameters_allowed) {
+  rclcpp::NodeOptions node_options;
+  node_options.allow_undeclared_parameters(true);
+  node_options.initial_parameters({rclcpp::Parameter("my_string", std::string("str"))});
+  auto node = std::make_shared<rclcpp::Node>("test_set_parameter_node"_unq, node_options);
+  auto value = node->get_parameter("my_string").get_value<std::string>();
+  EXPECT_EQ("str", value);
+}
+
+TEST_F(TestNode, set_parameters_undeclared_parameters_allowed_initial) {
+  rclcpp::NodeOptions node_options;
+  node_options.allow_undeclared_parameters(true);
+  auto node = std::make_shared<rclcpp::Node>("test_set_parameter_node"_unq, node_options);
+  node->set_parameters({rclcpp::Parameter("my_string", std::string("str"))});
+  auto value = node->get_parameter("my_string").get_value<std::string>();
+  EXPECT_EQ("str", value);
+}
+
 TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) {
   auto node = std::make_shared<rclcpp::Node>(
     "test_set_parameters_node"_unq,

6: [ RUN      ] TestNode.initial_parameters_undeclared_parameters_allowed                                                                                                       
6: unknown file: Failure                                                                                                                                                        
6: C++ exception with description "expected [string] got [not set]" thrown in the test body.                                                                                    
6: [  FAILED  ] TestNode.initial_parameters_undeclared_parameters_allowed (4 ms)
6: [ RUN      ] TestNode.set_parameters_undeclared_parameters_allowed_initial
6: [       OK ] TestNode.set_parameters_undeclared_parameters_allowed_initial (4 ms)

The reason why one test fails but the other passes is esoteric at best 🔮

---

The use case I have on gazebo_ros_pkgs is that users can pass parameters to nodes through SDF. With the change in Dashing, I'll have to either:

1. Use both allow_undeclared_parameters and automatically_declare_initial_parameters - which sound contradictory
2. Change the meaning of parameters passed by SDF to do "default setting" rather than "value setting" - matching rclcpp's semantics - and make sure all existing nodes declare their parameters
3. Declare all the arbitrary parameters I receive through SDF, essentially bypassing the rclcpp recommendations.

"golden rules" for parameters

Since this sounds like a strong recommendation and the first-class supported behaviour, I'll go for option 2.

If you only ever use initial parameter values and get, then the parameter will never be declared.

But if the user is choosing to work with undeclared parameters, then the user shouldn't need to declare them 🤷‍♀️

I've covered this topic quite thoroughly in the migration docs:

That's exactly where I learned about that option. And it says "you can get the old behavior by using the allow_undeclared_parameters option when creating your node. You might want to do this in order to avoid code changes for now". Unfortunately it didn't go quite how I expected 😬

I honestly don't know how I could have made it clearer

I think the whole confusion is coming from the fact that initial_parameters doesn't set or declare. Maybe it could be renamed to something like parameter_defaults?

With the info on this issue, now I understand when the current documentation says that "these initial parameter values are used to change the initial value of declared parameters within the node, overriding hard coded default values if necessary." I think that someone looking at this for the first time may not realize the importance of the word declared in that sentence. It may be worth elaborating a bit more.

I'm also strongly on the side of automatically declaring all parameters when allow_undeclared_parameters is set - which I believe would restore the legacy behaviour.

---

As a side note, I imagine this has been discussed at length, but throwing an exception to save users from a typo feels a bit too strict to me. My 2 cents 🙂

[wjwwood]

The reason why one test fails but the other passes is esoteric at best 🔮

The first test you added exposes a definite bug, imo.

It wasn't clear to me because the example from the original issue description didn't declare the type of param in the last statement node->get_parameter(param); (also does this kind of call compile? I don't think there's a signature like this??). I'm guessing the type of param was std::string, which implicitly is like this: std::string my_string = node->get_parameter("my_string").as_string() (where as_string() actually is the one that throws not get_parameter)

get_parameter is throwing an exception, but not because the parameter was not declared, but because it was getting the wrong value. Instead of getting "str" from the initial parameter values as it should, it returns an empty parameter of type "not set", and then it attempts to coerce it into a string, which fails because it's not a string type parameter.

@ivanpauno and I suggested as much, but I didn't understand until now that that was actually incorrect.

Your first new test should pass and doesn't. I'll work on a patch for that now.

My updated example does fail:

#include <string>

#include "rclcpp/rclcpp.hpp"

int main(int argc, char * argv[])
{
  rclcpp::init(argc, argv);

  rclcpp::NodeOptions options;
  options.allow_undeclared_parameters(true);
  options.initial_parameters({{"bar", "baz"}});
  auto node = std::make_shared<rclcpp::Node>("test_node", "", options);

  std::string foo = node->get_parameter("foo").as_string();

  return 0;
}

With:

libc++abi.dylib: terminating with uncaught exception of type rclcpp::ParameterTypeException: expected [string] got [not set]

Which I think is an reasonably clear error, especially if you include the backtrace from gdb:

...
    frame #8: 0x00007fff7af55f99 libc++abi.dylib`__cxa_throw + 113
    frame #9: 0x000000010032ba5b librclcpp.dylib`std::__1::enable_if<((rclcpp::ParameterType)4) == ((rclcpp::ParameterType)4), std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&>::type rclcpp::ParameterValue::get<(this=0x00007ffeefbfb1d0)4>() const at parameter_value.hpp:184:7
    frame #10: 0x000000010032a319 librclcpp.dylib`decltype(this=0x00007ffeefbfb1b8) rclcpp::Parameter::get_value<(rclcpp::ParameterType)4>() const at parameter.hpp:111:19
    frame #11: 0x000000010032a2f5 librclcpp.dylib`rclcpp::Parameter::as_string(this=0x00007ffeefbfb1b8) const at parameter.cpp:121:10
    frame #12: 0x0000000100012377 talker`main(argc=1, argv=0x00007ffeefbfb5c0) at talker.cpp:12:30
    frame #13: 0x00007fff7dcd63d5 libdyld.dylib`start + 1

Use both allow_undeclared_parameters and automatically_declare_initial_parameters - which sound contradictory

Why do you think they are contradictory? (honestly we should document/clarify their difference and interactions if the change log and/or docstrings don't)

From my perspective they do two separate things. One changes the behavior of get/set parameters and the other automatically declares any parameters given to the node, even if the user doesn't use them.

Since this sounds like a strong recommendation and the first-class supported behaviour, I'll go for option 2.

That's definitely what I recommend. I think it's a _good thing_™ if users state in their gazebo plugin which parameters they are expecting to have, and in the future hopefully set some constraints on those parameters, like ranges or choices or something (for which declare_parameter is required).

In most cases you should be able to replace a single get_parameter, or a single set_parameter_if_not_set/get_parameter pair, with declare_parameter (as they all can return the actual value).

... it says "you can get the old behavior by using the allow_undeclared_parameters option when creating your node. You might want to do this in order to avoid code changes for now". Unfortunately it didn't go quite how I expected 😬

You're right, after fixing this bug, however, I think that statement about "getting the old behavior" should be true.

Maybe it could be renamed to something like parameter_defaults?

Yeah, I think that is a good name. As I said, I used "initial parameter values" in the docs a lot, but somehow that didn't translate to the option.

I also thought about parameter_overrides, but I'm interested in what others think.

I'm also strongly on the side of automatically declaring all parameters when allow_undeclared_parameters is set - which I believe would restore the legacy behaviour.

I'm hoping that once we address this bug, allow_undeclared_parameters will be sufficient to support all the old use cases (as I suggested originally in the docs, as that was the intention).

Automatically declaring them would only change whether or not has_parameter returns true and what parameters are available when an external observer lists the parameters on a node.

As a side note, I imagine this has been discussed at length, but throwing an exception to save users from a typo feels a bit too strict to me. My 2 cents 🙂

Yeah, it wasn't meant to be that sharp of a corner, the idea was that changes to code for this break would either be "set allow_undeclared_parameters to true" or "replace things like get_parameter or get_parameter_or (now deprecated) calls with a single declare_parameter instead". Hopefully after this bug fix it will be closer to that intended impact.

[chapulina]

(also does this kind of call compile? I don't think there's a signature like this??

Yup, I think it's this one. I compiled those example tests in rclcpp and gazebo_ros_pkgs is using that too.

Why do you think they are contradictory?

Think of it as having 2 modes: declared and undeclared. When I'm in "undeclared parameters mode", I wouldn't expect needing to set a variable that declares parameters.

Automatically declaring them would only change whether or not has_parameter returns true and what parameters are available when an external observer lists the parameters on a node.

I think it would be great to add this to the documentation of has_parameter.

As for getting the list of parameters, I'll comment on your PR with some more specific questions.

[wjwwood]

I closed #739 because I think you guys are right (and I was wrong) about the interaction between get_parameter and allow_undeclared_parameters, i.e. if true it should prevent exceptions from get_parameter but nothing else, and get_parameter's results should agree with has_parameter and list_parameters.

---

I think I completely missed this use case (where you get without set and rely on externally defined parameters):

#include <string>

#include "rclcpp/rclcpp.hpp"

int main(int argc, char * argv[])
{
  rclcpp::init(argc, argv);

  rclcpp::NodeOptions options;
  options.allow_undeclared_parameters(true);
  options.initial_parameters({{"bar", "baz"}});
  auto node = std::make_shared<rclcpp::Node>("test_node", "", options);

  std::string foo = node->get_parameter("foo").as_string();
  //                                           ^ fails without automatically_declare_initial_parameters = true

  return 0;
}

The more common existing case I ran into was something like this:

#include <string>

#include "rclcpp/rclcpp.hpp"

int main(int argc, char * argv[])
{
  rclcpp::init(argc, argv);

  rclcpp::NodeOptions options;
  options.allow_undeclared_parameters(true);
  options.initial_parameters({{"foo", "baz"}});
  auto node = std::make_shared<rclcpp::Node>("test_node", "", options);

  node->set_parameter({"foo", "bar"});
  std::string foo = node->get_parameter("foo").as_string();  // foo would be "baz"

  return 0;
}

Which does work, because set_parameter will implicitly declare the parameter, due to allow_undeclared_parameters being true, and when declaring would look to the initial parameter values for an override.

---

So, if we want to fix this without any code changes we'd need to update the migration docs to say:

you can get the old behavior by setting the allow_undeclared_parameters and automatically_declare_initial_parameters options both to true when creating your node. You might want to do this in order to avoid code changes for now

Otherwise the first case I described above will not work as it did in Crystal.

I see this doc change as the minimum we should do, unless we opt to do more (which I'll talk about below).

---

Additionally, or instead of the doc change, we could change it so that when allow_undeclared_parameters is true then automatically_declare_initial_parameters is turned on too, as you both suggested.

I'm still not convinced of this, mostly because now I see that we would need to change automatically_declare_initial_parameters to be a tri-state option rather than a boolean, e.g. "explicitly on", "explicitly off", and the default "depends on allow_undeclared_parameters".

---

Another option is that we could remove automatically_declare_initial_parameters entirely and only have allow_undeclared_parameters, but again I'm not 100% convinced, since I do see cases where setting these to behaviors separately is valuable. And so I think we should keep them separately controllable.

---

At the same time, it's clear that some of our option names (e.g. NodeOptions::initial_parameters is misleading) and documentation could be clearer. And I do think it would be nice if the users that want the "Crystal behavior" could just change one thing rather than two things.

---

I'm going to propose some additional options in a follow up comment, maybe even make pr's for each option and only merge one of the two.

[wjwwood]

I opened two pr's to the dashing migration notes, each describing one of the two ways I think we could go:

• Minimal change (docs only):
  • clarify documentation of options related to parameter behavior changes ros2_documentation#235
• More complex change, changes how automatically_declare_initial_parameters works:
  • change docs to reflect proposed options to control behavior ros2_documentation#238

Please comment on those or here or propose yet other options as changes to those migration notes so we can discuss them.

Separately from that, I'd like to touch up some of the option names and terminology we use. But I'll propose those separately.

[ivanpauno]

• Minimal change (docs only):

  • ros2/ros2_documentation#235

Looks good. It's clear and minimal.

The more common existing case I ran into was something like this:

#include <string>

#include "rclcpp/rclcpp.hpp"

int main(int argc, char * argv[])
{
 rclcpp::init(argc, argv);

 rclcpp::NodeOptions options;
 options.allow_undeclared_parameters(true);
 options.initial_parameters({{"foo", "baz"}});
 auto node = std::make_shared<rclcpp::Node>("test_node", "", options);

 node->set_parameter({"foo", "bar"});
 std::string foo = node->get_parameter("foo").as_string();  // foo would be "baz"

 return 0;
}

In that case, foo would be "bar". I'm surprised to see that you actually get "baz". What is this case useful for?

Another option is that we could remove automatically_declare_initial_parameters entirely and only have allow_undeclared_parameters, but again I'm not 100% convinced, since I do see cases where setting these to behaviors separately is valuable

I see only three valuable cases, and one case which shouldn't be recommended. As I commented above, I don't know who would want to use automatically_declare_initial_parameters=false with allow_undeclared_parameters=true. It's not the legacy behavior, and I would not recommend someone to use it. Actually, I would ban it.

• More complex change, changes how automatically_declare_initial_parameters works:

  • ros2/ros2_documentation#238

If we don't want to allow the combination I commented above, I wouldn't go in this way.

Use both allow_undeclared_parameters and automatically_declare_initial_parameters - which sound contradictory

For me, it sounds a little contradictory too. But if the documentation is clear enough, I think it's ok.

---

For me, ignoring automatically_declare_initial_parameters when allow_undeclared_parameters is true, it's reasonable.
I would actually wrap both options together in only one. Name it parameters_configuration.
I would use this three options:

• ALWAYS_DECLARE_PARAMETERS
• AUTOMATICALLY_DECLARE_INITIAL_PARAMETERS
• ALLOW_UNDECLARED_PARAMETERS

But that's a bigger change ...

[wjwwood]

I would use this three options:

What about the forth case, allow undeclared parameters but do not automatically declare initial parameters?

[wjwwood]

I'm surprised to see that you actually get "baz". What is this case useful for?

This is the use case for parameters, the user says there's a parameter "foo" with a value "bar" unless otherwise overridden (by the initial_parameters option in the NodeOptions or via a YAML file), which in this case it is overridden to be "baz".

[wjwwood]

Actually, I would ban it.

I would not, for instance there could be a use case for a "parameter server" which allows external nodes to set any parameter they want (allow undeclared parameters), but specifically wants to be immune to parameters passed on the command line (maybe it shares a process with another node and the user is prone to pass initial parameter values that don't have a node name associated with them, so called "global initial parameter values").

Either way, I see no reason to prevent someone from having this behavior. It's not conceptually inconsistent, allowing it doesn't break anything. It's already part of a some niche settings that 99% of users will/should never use after Dashing.

[wjwwood]

Personally, I think the current implementation is sufficient, it allows for all combinations of behaviors we've been able to imagine, can emulate the old behavior (even if I didn't document it properly the first time), and deals with just bool values which is simpler than the multi-state options. But I'm obviously biased.

Still looking for a recommendation (or for consensus) on how to move forward.

[ivanpauno]

Considering that the four combinations of allow_undeclared_parameters and automatically_declare_initial_parameters have use cases, I would go ahead with:
ros2/ros2_documentation#235

---

but specifically wants to be immune to parameters passed on the command line (maybe it shares a process with another node and the user is prone to pass initial parameter values that don't have a node name associated with them, so called "global initial parameter values").

It's not completely "immune", because the first time it's set, it will get the default value passed from the command line (as the example above). I don't know if that behavior would be good in that example.

[wjwwood]

Ok, I'll close the other pr, fixup the comments on ros2/ros2_documentation#235 and merge that. I'll also open a pr to update the name of NodeOptions::initial_parameters to avoid confusion in a separate pr.

---

It's not completely "immune", because the first time it's set, it will get the default value passed from the command line (as the example above). I don't know if that behavior would be good in that example.

That's a fair point, I was specifically thinking immune to have parameters intended for another node implicitly declared on it. But I could see that perhaps we need another parameter which makes declare/set ignore initial parameter values, if we want to give complete control over the behavior, but we could do that later as it would be a completely new option.

[chapulina]

Just caught up with the whole discussion, thank you for offering two alternatives.

This is the use case for parameters, the user says there's a parameter "foo" with a value "bar" unless otherwise overridden (by the initial_parameters option in the NodeOptions or via a YAML file), which in this case it is overridden to be "baz".

The more I look into this the less I understand it 🙃 So initial_parameters can't be overridden by set_parameter? Sounds more like the final_definitive_parameters to me 😵

---

I feel like there are way more use cases being covered than I currently understand. I came into this from the standpoint of someone who's just looking at the rclcpp::Node API, and a lot is unclear. Updating some variable names and documentation may help, especially for the case when allow_undeclared_parameters == true. I know I've been convinced not to use that option though 🙂

[chapulina]

Which does work, because set_parameter will implicitly declare the parameter, due to allow_undeclared_parameters being true, and when declaring would look to the initial parameter values for an override.

I think I finally understood this comment. When declaring a parameter, the override is taken. And setting an undeclared parameter behaves like declaring.

But even though I now understand the underlying implementation, I don't think it makes sense for the end user to ever have a situation where set_parameter doesn't set the parameter.

[wjwwood]

But even though I now understand the underlying implementation, I don't think it makes sense for the end user to ever have a situation where set_parameter doesn't set the parameter.

That's a fair point.

Doing something different presents two problems, though, in my opinion:

• how do you override/influence a parameter that the user only ever sets?
  • (maybe you don't and that's ok?)
• how do you implement set parameter when allow undeclared parameters is enabled?
  • certainly possible, but maybe complicated

That being said, maybe we should change it... Definitely worth discussing with the rest of the team.

Maybe not for Dashing though, and that presents a different issue is how to change it in the future without breaking anything...

---

Further more, I realize now that there's now way to declare a parameter and not be influenced by the overrides (like an "non-overridable parameter"?), which is something we may want to have in the future (just a new option to declare_parameter).

In which case we could change the first set when allow undeclared is enable to use that. That would be a behavior change, but maybe worth doing. Or maybe we expose the same option in set parameter (ignore overrides or similar). Not sure.

[chapulina]

Addressed by renaming the parameter and updating the documentation.