fix behavior of get_parameter when used with allow_undeclared_parameter

[wjwwood]

Should fix #730.

I think I missed this case when testing because I tested declare_parameter in combination with allow_undeclared_parameters AND rclcpp::NodeOptions::initial_parameters, but for get_parameter I only tested with and without allow_undeclared_parameters but not in conjunction with rclcpp::NodeOptions::initial_parameters.

@chapulina I don't know how easy it is for you to try this pr out, but hopefully it should make your porting only require switching the allows_undeclared_parameter flag to true, even if you decide to do more changes and use declare_parameters.

[ivanpauno]

With this as-is, get_parameter returns the value loaded with initial_parameters. But calling has_parameter returns false, which is quite strange.

I don't see the point of adding extra logic in all the methods instead of enforcing automatically_declare_parameters when allow_undeclared_parameters is true.

My idea is to change L164 here:

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

Lines 164 to 173 in 131a11b

	if (automatically_declare_initial_parameters) {
	for (const auto & pair : this->get_initial_parameter_values()) {
	if (!this->has_parameter(pair.first)) {
	this->declare_parameter(
	pair.first,
	pair.second,
	rcl_interfaces::msg::ParameterDescriptor());
	}
	}
	}

to:

  if (automatically_declare_initial_parameters || allow_undeclared_) {

With that change, we also fix the calls to:

• get_parameter, get_parameters, etc
• has_parameter
• describe_parameters (consistent behavior with allow_undeclared_parameters and setting it with set_parameter)
• list_parameters

And I think nothing more changes.

With the change in get_parameter as proposed in this PR, I would expect the other calls to have the result of the fix I'm proposing. e.g.: If I can get the parameter, I expect it to be a member of the result of list_parameters.

[chapulina]

@ivanpauno brings up several good questions about how the rest of the API should work in "undeclared parameter mode". I wrote a couple of tests to illustrate that and also to point out how passing parameters in the constructor still works differently from setting them afterwards.

All these tests pass with this branch, see questions inline:

diff --git a/rclcpp/test/test_node.cpp b/rclcpp/test/test_node.cpp
index 456a3f2..e088b5d 100644
--- a/rclcpp/test/test_node.cpp
+++ b/rclcpp/test/test_node.cpp
@@ -697,6 +697,70 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) {
   }
 }
 
+TEST_F(TestNode, initial_parameters_undeclared_parameters_allowed) {
+  // Parameter at construction
+  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);
+
+  // Check parameter
+  auto value = node->get_parameter("my_string").get_value<std::string>();
+  EXPECT_EQ("str", value);
+  EXPECT_FALSE(node->has_parameter("my_string"));
+
+  // undeclare so that it doesn't interfere with the test
+  node->undeclare_parameter("use_sim_time");
+
+  // QUESTION Is it expected that the undeclared parameter comes up with `get_parameter` but not
+  // with `get_parameters` and `list_parameters`? This can be confusing.
+  {
+    std::map<std::string, std::string> values;
+    EXPECT_FALSE(node->get_parameters("", values));
+    EXPECT_EQ(0u, values.size());
+  }
+
+  {
+    auto values = node->list_parameters({""}, 1);
+    EXPECT_EQ(0u, values.names.size());
+    EXPECT_EQ(0u, values.prefixes.size());
+  }
+}
+
+TEST_F(TestNode, set_parameters_undeclared_parameters_allowed_initial) {
+  // Parameter after construction
+  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"))});
+
+  // Check parameter
+  auto value = node->get_parameter("my_string").get_value<std::string>();
+  EXPECT_EQ("str", value);
+
+  // undeclare so that it doesn't interfere with the test
+  node->undeclare_parameter("use_sim_time");
+
+  // QUESTION Should we expect `has_parameter` and `get_parameters` to have different results from
+  // the test above?
+  EXPECT_TRUE(node->has_parameter("my_string"));
+
+  {
+    std::map<std::string, std::string> values;
+    EXPECT_TRUE(node->get_parameters("", values));
+    ASSERT_EQ(1u, values.size());
+    EXPECT_EQ("my_string", values.begin()->first);
+    EXPECT_EQ("str", values.begin()->second);
+  }
+
+  // QUESTION is it expected that `list_parameters` has a different result from `get_parameters`?
+  {
+    auto values = node->list_parameters({""}, 1);
+    EXPECT_EQ(0u, values.names.size());
+    EXPECT_EQ(0u, values.prefixes.size());
+  }
+}
+
 TEST_F(TestNode, set_parameter_undeclared_parameters_allowed) {
   auto node = std::make_shared<rclcpp::Node>(
     "test_set_parameter_node"_unq,

[wjwwood]

With this as-is, get_parameter returns the value loaded with initial_parameters. But calling has_parameter returns false, which is quite strange.

I agree, in retrospect maybe what's on master is what I intended all along. I think instead, my mistake was in the documentation that allow_undeclared_parameter was the only option needed to get back to the "crystal behavior". Which makes some sense because the automatically_declare_initial_parameters option was a late addition to the pull request.

I'll close this and continue the discussion on the other issue.