From 5ec4b2d28af2ab7da82b3b9da351227aec40a9d4 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 2 Mar 2021 14:20:51 -0300 Subject: [PATCH 1/2] * Fix when to throw the NoParameterOverrideProvided exception. * Throw an exception when declaring a parameter of a specific type and passing a descriptor with dynamic typing enabled. Signed-off-by: Ivan Santiago Paunovic --- .../rclcpp/node_interfaces/node_parameters.cpp | 17 ++++++++++++++--- rclcpp/test/rclcpp/test_node.cpp | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index fd6943a64a..6cf123758b 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -440,6 +440,14 @@ declare_parameter_helper( parameter_descriptor.type = static_cast(type); } + if ( + rclcpp::PARAMETER_NOT_SET == default_value.get_type() && + overrides.find(name) == overrides.end() && + parameter_descriptor.dynamic_typing == false) + { + throw rclcpp::exceptions::NoParameterOverrideProvided(name); + } + rcl_interfaces::msg::ParameterEvent parameter_event; auto result = __declare_parameter_common( name, @@ -461,9 +469,6 @@ declare_parameter_helper( { // TODO(ivanpauno): Refactor the logic so we don't need the above `strncmp` and we can // detect between both exceptions more elegantly. - if (rclcpp::PARAMETER_NOT_SET == default_value.get_type()) { - throw rclcpp::exceptions::NoParameterOverrideProvided(name); - } throw rclcpp::exceptions::InvalidParameterTypeException(name, result.reason); } throw rclcpp::exceptions::InvalidParameterValueException( @@ -528,6 +533,12 @@ NodeParameters::declare_parameter( "declare_parameter(): the provided parameter type cannot be rclcpp::PARAMETER_NOT_SET"}; } + if (parameter_descriptor.dynamic_typing == true) { + throw std::invalid_argument{ + "declare_parameter(): cannot declare parameter of specific type and pass descriptor" + "with `dynamic_typing=true`"}; + } + return declare_parameter_helper( name, type, diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 57748e35a2..1302b11bb6 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -620,6 +620,22 @@ TEST_F(TestNode, declare_parameter_with_overrides) { {node->declare_parameter("parameter_type_mismatch", 42);}, rclcpp::exceptions::InvalidParameterTypeException); } + { + // default type and expected type do not match + EXPECT_THROW( + {node->declare_parameter( + "parameter_type_mismatch", rclcpp::ParameterType::PARAMETER_INTEGER);}, + rclcpp::exceptions::InvalidParameterTypeException); + } + { + // cannot pass an expected type and a descriptor with dynamic_typing=True + rcl_interfaces::msg::ParameterDescriptor descriptor{}; + descriptor.dynamic_typing = true; + EXPECT_THROW( + {node->declare_parameter( + "invalid_argument", rclcpp::ParameterType::PARAMETER_INTEGER, descriptor);}, + std::invalid_argument); + } } TEST_F(TestNode, declare_parameters_with_no_initial_values) { From 2036857e92a1c0a96c4626272724be3e0f267964 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 2 Mar 2021 14:23:00 -0300 Subject: [PATCH 2/2] When a parameter change is rejected, the parameters map shouldn't be updated. Signed-off-by: Ivan Santiago Paunovic --- rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 6cf123758b..36abc8e163 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -389,6 +389,10 @@ __declare_parameter_common( callback_container, callback); + if (!result.successful) { + return result; + } + // Add declared parameters to storage. parameters_out[name] = parameter_infos.at(name);