Address review comments. (#4704)

* remove comments. * Use RCLCPP_DEBUG instead of INFO for low level messages. * Add test for trying to access parameters that are not declared. Signed-off-by: Mike Wake <[email protected]>
ros-navigation · Oct 8, 2024 · 7cb8ef5 · 7cb8ef5
1 parent 7e9304b
commit 7cb8ef5
Show file tree

Hide file tree

Showing 3 changed files with 34 additions and 5 deletions.
diff --git a/nav2_mppi_controller/include/nav2_mppi_controller/tools/parameters_handler.hpp b/nav2_mppi_controller/include/nav2_mppi_controller/tools/parameters_handler.hpp
@@ -211,7 +211,7 @@ void ParametersHandler::getParam(
 
   if (param_type == ParameterType::Dynamic) {
     if (verbose_) {
-      RCLCPP_INFO(node->get_logger(), "setDynamicParamCallback for %s", name.c_str());
+      RCLCPP_DEBUG(node->get_logger(), "setDynamicParamCallback for %s", name.c_str());
     }
     setDynamicParamCallback(setting, name);
   } else {

diff --git a/nav2_mppi_controller/src/parameters_handler.cpp b/nav2_mppi_controller/src/parameters_handler.cpp
@@ -39,7 +39,6 @@ void ParametersHandler::start()
 {
   auto node = node_.lock();
 
-  // Register the special case "verbose" parameter before registering dynamicParamsCallback
   auto get_param = getParamGetter(node_name_);
   get_param(verbose_, "verbose", false);
 
@@ -70,11 +69,8 @@ ParametersHandler::dynamicParamsCallback(
       callback->second(param);
     } else {
       if (verbose_) {
-        // Expected if static parameter, ie one with no registered callback is attempted
-        // to be changed
         RCLCPP_WARN(logger_, "Parameter callback func for '%s' not found", param_name.c_str());
       }
-      // success = true; // Decision ??? Still return true to avoid exception ???
       success = false;
     }
   }

diff --git a/nav2_mppi_controller/test/parameter_handler_test.cpp b/nav2_mppi_controller/test/parameter_handler_test.cpp
@@ -220,3 +220,36 @@ TEST(ParameterHandlerTest, DynamicAndStaticParametersNotVerboseTest)
   EXPECT_EQ(p1, 10);
   EXPECT_EQ(p2, 7);
 }
+
+TEST(ParameterHandlerTest, DynamicAndStaticParametersNotDeclaredTest)
+{
+  auto node = std::make_shared<rclcpp_lifecycle::LifecycleNode>("my_node");
+
+  node->declare_parameter("dynamic_int", rclcpp::ParameterValue(7));
+  node->declare_parameter("static_int", rclcpp::ParameterValue(7));
+  ParametersHandlerWrapper handler(node);
+  handler.start();
+
+  // Set verbose true to get more information about bad parameter usage
+  auto getParamer = handler.getParamGetter("");
+  auto rec_param = std::make_shared<rclcpp::AsyncParametersClient>(
+    node->get_node_base_interface(), node->get_node_topics_interface(),
+    node->get_node_graph_interface(),
+    node->get_node_services_interface());
+
+  auto results = rec_param->set_parameters_atomically(
+  {
+    rclcpp::Parameter("my_node.verbose", true),
+  });
+
+  rclcpp::spin_until_future_complete(
+    node->get_node_base_interface(),
+    results);
+
+  // Try to access some parameters that have not been declared
+  int p1 = 0, p2 = 0;
+  EXPECT_THROW(getParamer(p1, "not_declared", 8, ParameterType::Dynamic),
+               rclcpp::exceptions::InvalidParameterValueException);
+  EXPECT_THROW(getParamer(p2, "not_declared2", 9, ParameterType::Static),
+               rclcpp::exceptions::InvalidParameterValueException);
+}