From e31332b9eed9f563043f60116480782a90cddbb0 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 4 Nov 2020 15:49:46 -0300 Subject: [PATCH 01/28] Enforce parameter types Signed-off-by: Ivan Santiago Paunovic --- .../node_interfaces/node_parameters.cpp | 44 ++++++++++++++----- 1 file changed, 32 insertions(+), 12 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 7560e31e30..244de86330 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -165,14 +165,13 @@ __are_doubles_equal(double x, double y, double ulp = 100.0) return std::abs(x - y) <= std::numeric_limits::epsilon() * std::abs(x + y) * ulp; } -RCLCPP_LOCAL -inline -void -format_reason(std::string & reason, const std::string & name, const char * range_type) +static +std::string +format_range_reason(const std::string & name, const char * range_type) { std::ostringstream ss; ss << "Parameter {" << name << "} doesn't comply with " << range_type << " range."; - reason = ss.str(); + return ss.str(); } RCLCPP_LOCAL @@ -191,7 +190,7 @@ __check_parameter_value_in_range( } if ((v < integer_range.from_value) || (v > integer_range.to_value)) { result.successful = false; - format_reason(result.reason, descriptor.name, "integer"); + result.reason = format_range_reason(descriptor.name, "integer"); return result; } if (integer_range.step == 0) { @@ -201,7 +200,7 @@ __check_parameter_value_in_range( return result; } result.successful = false; - format_reason(result.reason, descriptor.name, "integer"); + result.reason = format_range_reason(descriptor.name, "integer"); return result; } @@ -213,7 +212,7 @@ __check_parameter_value_in_range( } if ((v < fp_range.from_value) || (v > fp_range.to_value)) { result.successful = false; - format_reason(result.reason, descriptor.name, "floating point"); + result.reason = format_range_reason(descriptor.name, "floating point"); return result; } if (fp_range.step == 0.0) { @@ -224,12 +223,23 @@ __check_parameter_value_in_range( return result; } result.successful = false; - format_reason(result.reason, descriptor.name, "floating point"); + result.reason = format_range_reason(descriptor.name, "floating point"); return result; } return result; } +static +std::string +format_type_reason( + const std::string & name, const std::string & old_type, const std::string & new_type) +{ + std::ostringstream ss; + ss << "Parameter {" << name << "} is of type {" << old_type << "}, setting it to {" << + new_type << "} is not allowed."; + return ss.str(); +} + // Return true if parameter values comply with the descriptors in parameter_infos. RCLCPP_LOCAL rcl_interfaces::msg::SetParametersResult @@ -240,13 +250,23 @@ __check_parameters( rcl_interfaces::msg::SetParametersResult result; result.successful = true; for (const rclcpp::Parameter & parameter : parameters) { - const rcl_interfaces::msg::ParameterDescriptor & descriptor = - parameter_infos[parameter.get_name()].descriptor; + std::string name = parameter.get_name(); + auto item = parameter_infos[name]; + const rcl_interfaces::msg::ParameterDescriptor & descriptor = item.descriptor; + const rclcpp::ParameterType type = item.value.get_type(); + const rclcpp::ParameterType old_type = parameter.get_type(); + result.successful = + descriptor.allow_duck_typing || rclcpp::PARAMETER_NOT_SET == old_type || old_type == type; + if (!result.successful) { + result.reason = format_type_reason( + name, rclcpp::to_string(old_type), rclcpp::to_string(type)); + return result; + } result = __check_parameter_value_in_range( descriptor, parameter.get_parameter_value()); if (!result.successful) { - break; + return result; } } return result; From d46f8d9582a7cf247344cb8c781a41090811f8fa Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 4 Nov 2020 17:54:20 -0300 Subject: [PATCH 02/28] fix Signed-off-by: Ivan Santiago Paunovic --- rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 244de86330..caa992b346 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -253,10 +253,10 @@ __check_parameters( std::string name = parameter.get_name(); auto item = parameter_infos[name]; const rcl_interfaces::msg::ParameterDescriptor & descriptor = item.descriptor; - const rclcpp::ParameterType type = item.value.get_type(); - const rclcpp::ParameterType old_type = parameter.get_type(); + const rclcpp::ParameterType old_type = item.value.get_type(); + const rclcpp::ParameterType type = parameter.get_type(); result.successful = - descriptor.allow_duck_typing || rclcpp::PARAMETER_NOT_SET == old_type || old_type == type; + !descriptor.static_typing || rclcpp::PARAMETER_NOT_SET == old_type || old_type == type; if (!result.successful) { result.reason = format_type_reason( name, rclcpp::to_string(old_type), rclcpp::to_string(type)); From 3f1b2b31ebeb0e8f90250c36d95622097d5124cf Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 4 Nov 2020 17:54:33 -0300 Subject: [PATCH 03/28] adapt old tests Signed-off-by: Ivan Santiago Paunovic --- rclcpp/test/rclcpp/test_node.cpp | 68 ++++++++++++++++++++++++++------ 1 file changed, 55 insertions(+), 13 deletions(-) diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 8e9764d9f2..36b1bda004 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -842,12 +842,23 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { EXPECT_EQ(node->get_parameter(name).get_value(), 43); } { - // normal use, change type + // normal use, change type not allowed auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); node->declare_parameter(name, 42); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(node->get_parameter(name).get_value(), 42); + EXPECT_FALSE(node->set_parameter(rclcpp::Parameter(name, "not an integer")).successful); + } + { + // normal use, change type + auto name = "parameter"_unq; + EXPECT_FALSE(node->has_parameter(name)); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.static_typing = false; + node->declare_parameter(name, 42, descriptor); + EXPECT_TRUE(node->has_parameter(name)); + EXPECT_EQ(node->get_parameter(name).get_value(), 42); EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name, "not an integer")).successful); EXPECT_EQ(node->get_parameter(name).get_value(), std::string("not an integer")); } @@ -864,8 +875,6 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { EXPECT_TRUE(node->has_parameter(name3)); EXPECT_EQ(node->get_parameter(name1).get_value(), 42); - EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name1, "not an integer")).successful); - EXPECT_EQ(node->get_parameter(name1).get_value(), std::string("not an integer")); EXPECT_EQ(node->get_parameter(name2).get_value(), true); EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name2, false)).successful); @@ -910,13 +919,26 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { - // setting type of rclcpp::PARAMETER_NOT_SET, when already to another type, will undeclare + // setting type of rclcpp::PARAMETER_NOT_SET, when already to another type, will fail auto name = "parameter"_unq; node->declare_parameter(name, 42); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); + EXPECT_FALSE(node->set_parameter(rclcpp::Parameter(name)).successful); + } + { + // setting type of rclcpp::PARAMETER_NOT_SET, + // when dynamic typing is allowing and already set to another type, will undeclare + auto name = "parameter"_unq; + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.static_typing = false; + node->declare_parameter(name, 42, descriptor); + EXPECT_TRUE(node->has_parameter(name)); + auto value = node->get_parameter(name); + EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); + EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name)).successful); EXPECT_FALSE(node->has_parameter(name)); @@ -1256,15 +1278,9 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { auto name = "parameter"_unq; rcl_interfaces::msg::ParameterDescriptor descriptor; descriptor.type = rclcpp::PARAMETER_INTEGER; - node->declare_parameter(name, 42, descriptor); + node->declare_parameter(name, "asd", descriptor); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); - EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); - EXPECT_EQ(value.get_value(), 42); - - EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name, "asd")).successful); - EXPECT_TRUE(node->has_parameter(name)); - value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_STRING); EXPECT_EQ(value.get_value(), "asd"); } @@ -1422,13 +1438,26 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { - // setting type of rclcpp::PARAMETER_NOT_SET, when already to another type, will undeclare + // setting type of rclcpp::PARAMETER_NOT_SET, when already to another type, will fail auto name = "parameter"_unq; node->declare_parameter(name, 42); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); + EXPECT_FALSE(node->set_parameters({rclcpp::Parameter(name)})[0].successful); + } + { + // setting type of rclcpp::PARAMETER_NOT_SET, + // when already to another type and dynamic typic allowed, will undeclare + auto name = "parameter"_unq; + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.static_typing = false; + node->declare_parameter(name, 42, descriptor); + EXPECT_TRUE(node->has_parameter(name)); + auto value = node->get_parameter(name); + EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); + EXPECT_TRUE(node->set_parameters({rclcpp::Parameter(name)})[0].successful); EXPECT_FALSE(node->has_parameter(name)); @@ -1596,13 +1625,26 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { - // setting type of rclcpp::PARAMETER_NOT_SET, when already to another type, will undeclare + // setting type of rclcpp::PARAMETER_NOT_SET, when already to another type, will fail auto name = "parameter"_unq; node->declare_parameter(name, 42); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); + EXPECT_FALSE(node->set_parameters_atomically({rclcpp::Parameter(name)}).successful); + } + { + // setting type of rclcpp::PARAMETER_NOT_SET, + // when dynamic typing is allowed and already declared to another type, will undeclare + auto name = "parameter"_unq; + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.static_typing = false; + node->declare_parameter(name, 42, descriptor); + EXPECT_TRUE(node->has_parameter(name)); + auto value = node->get_parameter(name); + EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); + EXPECT_TRUE(node->set_parameters_atomically({rclcpp::Parameter(name)}).successful); EXPECT_FALSE(node->has_parameter(name)); From 2576512f5dc2568edc27758c3705c4ff118c713f Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 18 Jan 2021 15:44:30 -0300 Subject: [PATCH 04/28] Allowed type alternative Signed-off-by: Ivan Santiago Paunovic --- .../node_interfaces/node_parameters.hpp | 4 +- .../node_parameters_interface.hpp | 5 +- .../include/rclcpp/parameter_descriptor.hpp | 69 +++++++++++++++++++ .../node_interfaces/node_parameters.cpp | 22 ++++-- rclcpp/test/rclcpp/test_node.cpp | 16 ++--- 5 files changed, 94 insertions(+), 22 deletions(-) create mode 100644 rclcpp/include/rclcpp/parameter_descriptor.hpp diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp index be23615310..cec8428469 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp @@ -108,8 +108,8 @@ class NodeParameters : public NodeParametersInterface declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, - bool ignore_override) override; + const rclcpp::ParameterDescriptor & parameter_descriptor, + bool ignore_override = false) override; RCLCPP_PUBLIC void diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index 5b73998dbd..6ffc1a1cc0 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -26,6 +26,7 @@ #include "rclcpp/macros.hpp" #include "rclcpp/parameter.hpp" +#include "rclcpp/parameter_descriptor.hpp" #include "rclcpp/visibility_control.hpp" namespace rclcpp @@ -65,8 +66,8 @@ class NodeParametersInterface declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = - rcl_interfaces::msg::ParameterDescriptor(), + const rclcpp::ParameterDescriptor & parameter_descriptor = + rclcpp::ParameterDescriptor(), bool ignore_override = false) = 0; /// Undeclare a parameter. diff --git a/rclcpp/include/rclcpp/parameter_descriptor.hpp b/rclcpp/include/rclcpp/parameter_descriptor.hpp new file mode 100644 index 0000000000..b61f5865c9 --- /dev/null +++ b/rclcpp/include/rclcpp/parameter_descriptor.hpp @@ -0,0 +1,69 @@ +// Copyright 2020 Open Source Robotics Foundation, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#ifndef RCLCPP__PARAMETER_DESCRIPTOR_HPP_ +#define RCLCPP__PARAMETER_DESCRIPTOR_HPP_ + +namespace rclcpp +{ + +/// rcl_interfaces::msg::ParameterDescriptor wrapper used to streamline parameter declarations. +/** + * Examples: + * ```cpp + * // declare statically typed parameter, type inferred from gived default value + * node.declare_parameter("my_int", 5); + * // declare statically typed parameter, no default value given. + * node.declare_parameter("my_int", rclcpp::ParameterType::INTEGER_PARAMETER); + * // declare statically typed parameter, no default value given. + * node.declare_parameter(my_int, rclcpp::ParameterValue(), rclcpp::ParameterType::INTEGER_PARAMETER); + */ +class ParameterDescriptor +{ +public: + /// Default constructor. + explicit ParameterDescriptor(bool static_typing = true) : static_typing_{static_typing} {} + + /// Implicit constructor from parameter descriptor message. + ParameterDescriptor( + const rcl_interfaces::msg::ParameterDescriptor & msg) + : impl_(msg) {} + + /// Implicit constructor from a parameter type. + ParameterDescriptor(rclcpp::ParameterType type) + : static_typing_{type != rclcpp::ParameterType::PARAMETER_NOT_SET} {impl_.allowed_type = type;} + + /// Getter of internal message representation. + rcl_interfaces::msg::ParameterDescriptor & + get_msg() + {return impl_;} + + /// Getter of internal message representation. + const rcl_interfaces::msg::ParameterDescriptor & + get_msg() const + {return impl_;}; + + /// Return true if the parameter cannot change its type, false otherwise. + bool + is_statically_typed() const + {return static_typing_ || impl_.allowed_type != rclcpp::ParameterType::PARAMETER_NOT_SET;} + +private: + rcl_interfaces::msg::ParameterDescriptor impl_; + bool static_typing_; +}; + +} // namespace rclcpp + +#endif // RCLCPP__PARAMETER_DESCRIPTOR_HPP_ diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index caa992b346..8e9fd53d92 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -138,7 +138,7 @@ NodeParameters::NodeParameters( this->declare_parameter( pair.first, pair.second, - rcl_interfaces::msg::ParameterDescriptor(), + rclcpp::ParameterDescriptor(false), true); } } @@ -256,7 +256,8 @@ __check_parameters( const rclcpp::ParameterType old_type = item.value.get_type(); const rclcpp::ParameterType type = parameter.get_type(); result.successful = - !descriptor.static_typing || rclcpp::PARAMETER_NOT_SET == old_type || old_type == type; + descriptor.allowed_type == rclcpp::PARAMETER_NOT_SET || + descriptor.allowed_type == type; if (!result.successful) { result.reason = format_type_reason( name, rclcpp::to_string(old_type), rclcpp::to_string(type)); @@ -345,7 +346,7 @@ rcl_interfaces::msg::SetParametersResult __declare_parameter_common( const std::string & name, const rclcpp::ParameterValue & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + const rclcpp::ParameterDescriptor & parameter_descriptor, std::map & parameters_out, const std::map & overrides, CallbacksContainerType & callback_container, @@ -355,7 +356,16 @@ __declare_parameter_common( { using rclcpp::node_interfaces::ParameterInfo; std::map parameter_infos {{name, ParameterInfo()}}; - parameter_infos.at(name).descriptor = parameter_descriptor; + auto & param_info = parameter_infos.at(name); + param_info.descriptor = parameter_descriptor.get_msg(); + if (parameter_descriptor.is_statically_typed()) { + if (rclcpp::ParameterType::PARAMETER_NOT_SET == default_value.get_type()) { + throw std::runtime_error{ + "If not passing a defaul value, a parameter type must be specified in the descriptor or" + "dynamic typing must be allowed"}; + } + param_info.descriptor.allowed_type = static_cast(default_value.get_type()); + } // Use the value from the overrides if available, otherwise use the default. const rclcpp::ParameterValue * initial_value = &default_value; @@ -388,7 +398,7 @@ const rclcpp::ParameterValue & NodeParameters::declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + const rclcpp::ParameterDescriptor & parameter_descriptor, bool ignore_override) { std::lock_guard lock(mutex_); @@ -550,7 +560,7 @@ NodeParameters::set_parameters_atomically(const std::vector & result = __declare_parameter_common( parameter_to_be_declared->get_name(), parameter_to_be_declared->get_parameter_value(), - rcl_interfaces::msg::ParameterDescriptor(), // Implicit declare uses default descriptor. + rclcpp::ParameterDescriptor(false), // Implicit declare uses default descriptor. staged_parameter_changes, parameter_overrides_, // Only call callbacks once below diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 36b1bda004..f694bc27a5 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -854,9 +854,7 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { // normal use, change type auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); - rcl_interfaces::msg::ParameterDescriptor descriptor; - descriptor.static_typing = false; - node->declare_parameter(name, 42, descriptor); + node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(node->get_parameter(name).get_value(), 42); EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name, "not an integer")).successful); @@ -932,9 +930,7 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, // when dynamic typing is allowing and already set to another type, will undeclare auto name = "parameter"_unq; - rcl_interfaces::msg::ParameterDescriptor descriptor; - descriptor.static_typing = false; - node->declare_parameter(name, 42, descriptor); + node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); @@ -1451,9 +1447,7 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, // when already to another type and dynamic typic allowed, will undeclare auto name = "parameter"_unq; - rcl_interfaces::msg::ParameterDescriptor descriptor; - descriptor.static_typing = false; - node->declare_parameter(name, 42, descriptor); + node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); @@ -1638,9 +1632,7 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, // when dynamic typing is allowed and already declared to another type, will undeclare auto name = "parameter"_unq; - rcl_interfaces::msg::ParameterDescriptor descriptor; - descriptor.static_typing = false; - node->declare_parameter(name, 42, descriptor); + node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); From 7c3e175c8a37d645b1d3ca9fc8c99384cd089dfd Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 18 Jan 2021 15:50:04 -0300 Subject: [PATCH 05/28] linters Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/parameter_descriptor.hpp | 6 +++--- rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/rclcpp/include/rclcpp/parameter_descriptor.hpp b/rclcpp/include/rclcpp/parameter_descriptor.hpp index b61f5865c9..d96178a88b 100644 --- a/rclcpp/include/rclcpp/parameter_descriptor.hpp +++ b/rclcpp/include/rclcpp/parameter_descriptor.hpp @@ -36,12 +36,12 @@ class ParameterDescriptor explicit ParameterDescriptor(bool static_typing = true) : static_typing_{static_typing} {} /// Implicit constructor from parameter descriptor message. - ParameterDescriptor( + ParameterDescriptor( // NOLINT: implicit conversion constructor const rcl_interfaces::msg::ParameterDescriptor & msg) : impl_(msg) {} /// Implicit constructor from a parameter type. - ParameterDescriptor(rclcpp::ParameterType type) + ParameterDescriptor(rclcpp::ParameterType type) // NOLINT: implicit conversion constructor : static_typing_{type != rclcpp::ParameterType::PARAMETER_NOT_SET} {impl_.allowed_type = type;} /// Getter of internal message representation. @@ -52,7 +52,7 @@ class ParameterDescriptor /// Getter of internal message representation. const rcl_interfaces::msg::ParameterDescriptor & get_msg() const - {return impl_;}; + {return impl_;} /// Return true if the parameter cannot change its type, false otherwise. bool diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 8e9fd53d92..8f20f715b0 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -361,8 +361,8 @@ __declare_parameter_common( if (parameter_descriptor.is_statically_typed()) { if (rclcpp::ParameterType::PARAMETER_NOT_SET == default_value.get_type()) { throw std::runtime_error{ - "If not passing a defaul value, a parameter type must be specified in the descriptor or" - "dynamic typing must be allowed"}; + "If not passing a defaul value, a parameter type must be specified in the descriptor " + "or dynamic typing must be allowed"}; } param_info.descriptor.allowed_type = static_cast(default_value.get_type()); } From 92f53d03cfd6efd3ef4357de0a42d74694efaab8 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 19 Jan 2021 11:31:32 -0300 Subject: [PATCH 06/28] Fix bugs, adapt test cases Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 10 +- rclcpp/include/rclcpp/node_impl.hpp | 6 +- .../include/rclcpp/parameter_descriptor.hpp | 49 ++++++--- rclcpp/src/rclcpp/node.cpp | 2 +- .../node_interfaces/node_parameters.cpp | 29 +++-- rclcpp/test/rclcpp/test_node.cpp | 100 ++++++++++++------ 6 files changed, 135 insertions(+), 61 deletions(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index f2247b59a1..df422e66db 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -311,8 +311,8 @@ class Node : public std::enable_shared_from_this declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = - rcl_interfaces::msg::ParameterDescriptor(), + const rclcpp::ParameterDescriptor & parameter_descriptor = + rclcpp::ParameterDescriptor(), bool ignore_override = false); /// Declare and initialize a parameter with a type. @@ -341,8 +341,8 @@ class Node : public std::enable_shared_from_this declare_parameter( const std::string & name, const ParameterT & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = - rcl_interfaces::msg::ParameterDescriptor(), + const rclcpp::ParameterDescriptor & parameter_descriptor = + rclcpp::ParameterDescriptor(), bool ignore_override = false); /// Declare and initialize several parameters with the same namespace and type. @@ -401,7 +401,7 @@ class Node : public std::enable_shared_from_this const std::string & namespace_, const std::map< std::string, - std::pair + std::pair > & parameters, bool ignore_overrides = false); diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp index 4a24d0d359..27cb15b101 100644 --- a/rclcpp/include/rclcpp/node_impl.hpp +++ b/rclcpp/include/rclcpp/node_impl.hpp @@ -157,7 +157,7 @@ auto Node::declare_parameter( const std::string & name, const ParameterT & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + const rclcpp::ParameterDescriptor & parameter_descriptor, bool ignore_override) { try { @@ -187,7 +187,7 @@ Node::declare_parameters( return this->declare_parameter( normalized_namespace + element.first, element.second, - rcl_interfaces::msg::ParameterDescriptor(), + rclcpp::ParameterDescriptor(), ignore_overrides); } ); @@ -200,7 +200,7 @@ Node::declare_parameters( const std::string & namespace_, const std::map< std::string, - std::pair + std::pair > & parameters, bool ignore_overrides) { diff --git a/rclcpp/include/rclcpp/parameter_descriptor.hpp b/rclcpp/include/rclcpp/parameter_descriptor.hpp index d96178a88b..35c74e9b75 100644 --- a/rclcpp/include/rclcpp/parameter_descriptor.hpp +++ b/rclcpp/include/rclcpp/parameter_descriptor.hpp @@ -24,25 +24,49 @@ namespace rclcpp * ```cpp * // declare statically typed parameter, type inferred from gived default value * node.declare_parameter("my_int", 5); + * * // declare statically typed parameter, no default value given. - * node.declare_parameter("my_int", rclcpp::ParameterType::INTEGER_PARAMETER); - * // declare statically typed parameter, no default value given. - * node.declare_parameter(my_int, rclcpp::ParameterValue(), rclcpp::ParameterType::INTEGER_PARAMETER); + * node.declare_parameter( + * "my_int", rclcpp::ParameterValue{}, rclcpp::ParameterType::INTEGER_PARAMETER); + * + * // declare statically typed parameter, type inferred from given default, with descriptor. + * rcl_interfaces::msg::ParameterDescriptor descriptor; + * //>> Set descriptor << + * node.declare_parameter("my_int", 5, descriptor); + * + * // declare statically typed parameter, type inferred from descriptor. + * rcl_interfaces::msg::ParameterDescriptor descriptor; + * descriptor.allowed_type = static_cast(rclcpp::ParameterType::INTEGER_PARAMETER); + * //>> Set other things in descriptor.<< + * node.declare_parameter("my_int", rclcpp::ParamterValue{}, {descriptor, false}); + * + * // declare parameter that can change of type + * node.declare_parameter("my_param", rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + * + * // declare parameter that can change of type with default value + * node.declare_parameter("my_param", 5, rclcpp::ParameterDescriptor{false}); + * + * // declare parameter that can change of type with default value and descriptor + * rcl_interfaces::msg::ParameterDescriptor descriptor; + * //>> Set descriptor << + * node.declare_parameter("my_param", 5, {descriptor, false}); */ class ParameterDescriptor { public: /// Default constructor. - explicit ParameterDescriptor(bool static_typing = true) : static_typing_{static_typing} {} + explicit ParameterDescriptor(bool infer_type_from_value = true) + : infer_type_from_value_{infer_type_from_value} {} /// Implicit constructor from parameter descriptor message. - ParameterDescriptor( // NOLINT: implicit conversion constructor - const rcl_interfaces::msg::ParameterDescriptor & msg) - : impl_(msg) {} + ParameterDescriptor( + const rcl_interfaces::msg::ParameterDescriptor & msg, bool infer_type_from_value = true) + // NOLINT: implicit conversion constructor + : impl_(msg), infer_type_from_value_{infer_type_from_value} {} /// Implicit constructor from a parameter type. ParameterDescriptor(rclcpp::ParameterType type) // NOLINT: implicit conversion constructor - : static_typing_{type != rclcpp::ParameterType::PARAMETER_NOT_SET} {impl_.allowed_type = type;} + : infer_type_from_value_{false} {impl_.allowed_type = type;} /// Getter of internal message representation. rcl_interfaces::msg::ParameterDescriptor & @@ -54,14 +78,15 @@ class ParameterDescriptor get_msg() const {return impl_;} - /// Return true if the parameter cannot change its type, false otherwise. + /// Return true if we have to ignore the allowed type provided in the descriptor and infer + /// it later from the default value. bool - is_statically_typed() const - {return static_typing_ || impl_.allowed_type != rclcpp::ParameterType::PARAMETER_NOT_SET;} + do_infer_type_from_value() const + {return infer_type_from_value_;} private: rcl_interfaces::msg::ParameterDescriptor impl_; - bool static_typing_; + bool infer_type_from_value_; }; } // namespace rclcpp diff --git a/rclcpp/src/rclcpp/node.cpp b/rclcpp/src/rclcpp/node.cpp index aa961ec462..8092cec9b7 100644 --- a/rclcpp/src/rclcpp/node.cpp +++ b/rclcpp/src/rclcpp/node.cpp @@ -223,7 +223,7 @@ const rclcpp::ParameterValue & Node::declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + const rclcpp::ParameterDescriptor & parameter_descriptor, bool ignore_override) { return this->node_parameters_->declare_parameter( diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 8f20f715b0..1fbf067065 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -18,6 +18,7 @@ #include #include +#include #include #include #include @@ -235,8 +236,10 @@ format_type_reason( const std::string & name, const std::string & old_type, const std::string & new_type) { std::ostringstream ss; - ss << "Parameter {" << name << "} is of type {" << old_type << "}, setting it to {" << - new_type << "} is not allowed."; + // WARN: A condition later depends on this message starting with "Wrong parameter type", + // check `declare_parameter` if you modify this! + ss << "Wrong parameter type, parameter {" << name << "} is of type {" << old_type << + "}, setting it to {" << new_type << "} is not allowed."; return ss.str(); } @@ -358,11 +361,12 @@ __declare_parameter_common( std::map parameter_infos {{name, ParameterInfo()}}; auto & param_info = parameter_infos.at(name); param_info.descriptor = parameter_descriptor.get_msg(); - if (parameter_descriptor.is_statically_typed()) { + if (parameter_descriptor.do_infer_type_from_value()) { if (rclcpp::ParameterType::PARAMETER_NOT_SET == default_value.get_type()) { - throw std::runtime_error{ - "If not passing a defaul value, a parameter type must be specified in the descriptor " - "or dynamic typing must be allowed"}; + throw rclcpp::exceptions::InvalidParameterTypeException{ + name, + "If not passing a default value, a parameter type must be specified in the " + "descriptor or dynamic typing must be allowed"}; } param_info.descriptor.allowed_type = static_cast(default_value.get_type()); } @@ -430,6 +434,15 @@ NodeParameters::declare_parameter( // If it failed to be set, then throw an exception. if (!result.successful) { + constexpr const char type_error_msg_start[] = "Wrong parameter type"; + if ( + 0u == std::strncmp( + result.reason.c_str(), type_error_msg_start, sizeof(type_error_msg_start) - 1)) + { + // TODO(ivanpauno): Refactor the logic so we don't need the above `strncmp` and we can + // detect between both exceptions more elegantly. + throw rclcpp::exceptions::InvalidParameterTypeException(name, result.reason); + } throw rclcpp::exceptions::InvalidParameterValueException( "parameter '" + name + "' could not be set: " + result.reason); } @@ -461,6 +474,10 @@ NodeParameters::undeclare_parameter(const std::string & name) throw rclcpp::exceptions::ParameterImmutableException( "cannot undeclare parameter '" + name + "' because it is read-only"); } + if (rclcpp::PARAMETER_NOT_SET != parameter_info->second.descriptor.allowed_type) { + throw rclcpp::exceptions::InvalidParameterTypeException{ + name, "cannot undeclare an statically typed parameter"}; + } parameters_.erase(parameter_info); } diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index f694bc27a5..b74cda42e6 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -326,7 +326,8 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { auto node = std::make_shared("test_declare_parameter_node"_unq); { // no default, no initial - rclcpp::ParameterValue value = node->declare_parameter("parameter"_unq); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter"_unq, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { @@ -358,9 +359,12 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { { // parameter already declared throws auto name = "parameter"_unq; - node->declare_parameter(name); + node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_THROW( - {node->declare_parameter(name);}, + { + node->declare_parameter( + name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } { @@ -492,13 +496,17 @@ TEST_F(TestNode, declare_parameter_with_overrides) { auto node = std::make_shared("test_declare_parameter_node"_unq, no); { // no default, with override - rclcpp::ParameterValue value = node->declare_parameter("parameter_no_default"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_no_default", rclcpp::ParameterValue{}, rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 42); } { // no default, with override, and set after - rclcpp::ParameterValue value = node->declare_parameter("parameter_no_default_set"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_no_default_set", + rclcpp::ParameterValue{}, + rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 42); // check that the value is changed after a set @@ -507,8 +515,10 @@ TEST_F(TestNode, declare_parameter_with_overrides) { } { // no default, with override - const rclcpp::ParameterValue & value = - node->declare_parameter("parameter_no_default_set_cvref"); + const rclcpp::ParameterValue & value = node->declare_parameter( + "parameter_no_default_set_cvref", + rclcpp::ParameterValue{}, + rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 42); // check that the value is changed after a set @@ -555,9 +565,12 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // parameter already declared throws auto name = "parameter_already_declared"; - node->declare_parameter(name); + node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_THROW( - {node->declare_parameter(name);}, + { + node->declare_parameter( + name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } { @@ -668,7 +681,7 @@ TEST_F(TestNode, declare_parameters_with_no_initial_values) { { // parameter already declared throws, even with not_set type auto name = "parameter"_unq; - node->declare_parameter(name); + node->declare_parameter(name, 42); EXPECT_THROW( {node->declare_parameters("", {{name, 42}});}, rclcpp::exceptions::ParameterAlreadyDeclaredException); @@ -727,45 +740,53 @@ TEST_F(TestNode, declare_parameter_with_cli_overrides) { // To match parameters YAML file content, use a well-known node name for this test only. auto node = std::make_shared("test_declare_parameter_node", no); { - rclcpp::ParameterValue value = node->declare_parameter("parameter_bool"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_bool", rclcpp::ParameterValue{}, rclcpp::PARAMETER_BOOL); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_BOOL); EXPECT_EQ(value.get(), true); } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_int"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_int", rclcpp::ParameterValue{}, rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 21); // set to 42 in CLI, overriden by file } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_double"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_double", rclcpp::ParameterValue{}, rclcpp::PARAMETER_DOUBLE); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_DOUBLE); EXPECT_EQ(value.get(), 0.42); } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_string"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_string", rclcpp::ParameterValue{}, rclcpp::PARAMETER_STRING); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_STRING); EXPECT_EQ(value.get(), "foo"); } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_bool_array"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_bool_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_BOOL_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_BOOL_ARRAY); std::vector expected_value{false, true}; EXPECT_EQ(value.get>(), expected_value); } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_int_array"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_int_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_INTEGER_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER_ARRAY); std::vector expected_value{-21, 42}; EXPECT_EQ(value.get>(), expected_value); } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_double_array"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_double_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_DOUBLE_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_DOUBLE_ARRAY); std::vector expected_value{-1.0, 0.42}; EXPECT_EQ(value.get>(), expected_value); } { - rclcpp::ParameterValue value = node->declare_parameter("parameter_string_array"); + rclcpp::ParameterValue value = node->declare_parameter( + "parameter_string_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_STRING_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_STRING_ARRAY); std::vector expected_value{"foo", "bar"}; // set to [baz, baz, baz] in file, overriden by CLI @@ -778,7 +799,7 @@ TEST_F(TestNode, undeclare_parameter) { { // normal use auto name = "parameter"_unq; - node->declare_parameter(name); + node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -791,6 +812,16 @@ TEST_F(TestNode, undeclare_parameter) { {node->undeclare_parameter(name);}, rclcpp::exceptions::ParameterNotDeclaredException); } + { + // statically typed parameter throws + auto name = "parameter"_unq; + node->declare_parameter(name, 42); + EXPECT_TRUE(node->has_parameter(name)); + EXPECT_THROW( + {node->undeclare_parameter(name);}, + rclcpp::exceptions::InvalidParameterTypeException); + EXPECT_TRUE(node->has_parameter(name)); + } { // read only parameter throws auto name = "parameter"_unq; @@ -810,7 +841,7 @@ TEST_F(TestNode, has_parameter) { // normal use auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); - node->declare_parameter(name); + node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -821,7 +852,7 @@ TEST_F(TestNode, list_parameters) { // normal use auto name = "parameter"_unq; const size_t before_size = node->list_parameters({}, 1u).names.size(); - node->declare_parameter(name); + node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_EQ(1u + before_size, node->list_parameters({}, 1u).names.size()); node->undeclare_parameter(name); EXPECT_EQ(before_size, node->list_parameters({}, 1u).names.size()); @@ -907,7 +938,8 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter(name); + auto value = node->declare_parameter( + name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1424,7 +1456,8 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter(name); + auto value = node->declare_parameter( + name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1609,7 +1642,8 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter(name); + auto value = node->declare_parameter( + name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1992,30 +2026,28 @@ TEST_F(TestNode, get_parameters_undeclared_parameters_not_allowed) { { // templated version with empty prefix will get all parameters auto node_local = std::make_shared("test_get_parameters_node"_unq); - auto name1 = "prefix1.parameter"_unq; - auto name2 = "prefix2.parameter"_unq; + auto name1 = "parameter"_unq; + auto name2 = "parameter"_unq; - node_local->declare_parameter(name1, 42); - node_local->declare_parameter(name2, 100); - // undeclare so that it doesn't interfere with the test - node_local->undeclare_parameter("use_sim_time"); + node_local->declare_parameter("prefix." + name1, 42); + node_local->declare_parameter("prefix." + name2, 100); { std::map actual; - EXPECT_TRUE(node_local->get_parameters("", actual)); + EXPECT_TRUE(node_local->get_parameters("prefix", actual)); EXPECT_NE(actual.find(name1), actual.end()); EXPECT_NE(actual.find(name2), actual.end()); } // will throw if set of parameters is non-homogeneous - auto name3 = "prefix2.parameter"_unq; - node_local->declare_parameter(name3, "not an int"); + auto name3 = "prefix1.parameter"_unq; + node_local->declare_parameter("prefix." + name3, "not an int"); { std::map actual; EXPECT_THROW( { - node_local->get_parameters("", actual); + node_local->get_parameters("prefix", actual); }, rclcpp::exceptions::InvalidParameterTypeException); } From 30916b67f2d70d4903246c0c4772c5ee2e9c99aa Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 21 Jan 2021 12:51:38 -0300 Subject: [PATCH 07/28] API changes Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 16 ++++++ .../node_interfaces/node_parameters.hpp | 9 ++++ .../node_parameters_interface.hpp | 14 ++++++ .../include/rclcpp/parameter_descriptor.hpp | 10 ++-- rclcpp/src/rclcpp/node.cpp | 14 ++++++ .../node_interfaces/node_parameters.cpp | 12 +++++ rclcpp/test/rclcpp/test_node.cpp | 49 ++++++++----------- 7 files changed, 89 insertions(+), 35 deletions(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index df422e66db..26138d7c99 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -315,6 +315,22 @@ class Node : public std::enable_shared_from_this rclcpp::ParameterDescriptor(), bool ignore_override = false); + /// Declare and initialize a parameter, return the effective value. + /** + * Same as: + * ```cpp + * node.declare_parameter(name, ParameterValue{}, descriptor, ignore_override); + * ``` + */ + RCLCPP_PUBLIC + const rclcpp::ParameterValue & + declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor{}, + bool ignore_override = false); + /// Declare and initialize a parameter with a type. /** * See the non-templated declare_parameter() on this class for details. diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp index cec8428469..193823dfa3 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp @@ -111,6 +111,15 @@ class NodeParameters : public NodeParametersInterface const rclcpp::ParameterDescriptor & parameter_descriptor, bool ignore_override = false) override; + RCLCPP_PUBLIC + const rclcpp::ParameterValue & + declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), + bool ignore_override = false) override; + RCLCPP_PUBLIC void undeclare_parameter(const std::string & name) override; diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index 6ffc1a1cc0..a501def8cf 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -70,6 +70,20 @@ class NodeParametersInterface rclcpp::ParameterDescriptor(), bool ignore_override = false) = 0; + /// Declare and initialize a parameter. + /** + * \sa rclcpp::Node::declare_parameter + */ + RCLCPP_PUBLIC + virtual + const rclcpp::ParameterValue & + declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), + bool ignore_override = false) = 0; + /// Undeclare a parameter. /** * \sa rclcpp::Node::undeclare_parameter diff --git a/rclcpp/include/rclcpp/parameter_descriptor.hpp b/rclcpp/include/rclcpp/parameter_descriptor.hpp index 35c74e9b75..b32c166715 100644 --- a/rclcpp/include/rclcpp/parameter_descriptor.hpp +++ b/rclcpp/include/rclcpp/parameter_descriptor.hpp @@ -27,7 +27,7 @@ namespace rclcpp * * // declare statically typed parameter, no default value given. * node.declare_parameter( - * "my_int", rclcpp::ParameterValue{}, rclcpp::ParameterType::INTEGER_PARAMETER); + * "my_int", rclcpp::ParameterType::INTEGER_PARAMETER); * * // declare statically typed parameter, type inferred from given default, with descriptor. * rcl_interfaces::msg::ParameterDescriptor descriptor; @@ -38,10 +38,10 @@ namespace rclcpp * rcl_interfaces::msg::ParameterDescriptor descriptor; * descriptor.allowed_type = static_cast(rclcpp::ParameterType::INTEGER_PARAMETER); * //>> Set other things in descriptor.<< - * node.declare_parameter("my_int", rclcpp::ParamterValue{}, {descriptor, false}); + * node.declare_parameter("my_int", {descriptor, false}); * * // declare parameter that can change of type - * node.declare_parameter("my_param", rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + * node.declare_parameter("my_param", rclcpp::ParameterType::PARAMETER_NOT_SET); * * // declare parameter that can change of type with default value * node.declare_parameter("my_param", 5, rclcpp::ParameterDescriptor{false}); @@ -64,10 +64,6 @@ class ParameterDescriptor // NOLINT: implicit conversion constructor : impl_(msg), infer_type_from_value_{infer_type_from_value} {} - /// Implicit constructor from a parameter type. - ParameterDescriptor(rclcpp::ParameterType type) // NOLINT: implicit conversion constructor - : infer_type_from_value_{false} {impl_.allowed_type = type;} - /// Getter of internal message representation. rcl_interfaces::msg::ParameterDescriptor & get_msg() diff --git a/rclcpp/src/rclcpp/node.cpp b/rclcpp/src/rclcpp/node.cpp index 8092cec9b7..999f11d378 100644 --- a/rclcpp/src/rclcpp/node.cpp +++ b/rclcpp/src/rclcpp/node.cpp @@ -233,6 +233,20 @@ Node::declare_parameter( ignore_override); } +const rclcpp::ParameterValue & +Node::declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) +{ + return this->node_parameters_->declare_parameter( + name, + type, + parameter_descriptor, + ignore_override); +} + void Node::undeclare_parameter(const std::string & name) { diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 1fbf067065..5bbfa9132d 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -457,6 +457,18 @@ NodeParameters::declare_parameter( return parameters_.at(name).value; } +const rclcpp::ParameterValue & +NodeParameters::declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + rcl_interfaces::msg::ParameterDescriptor parameter_descriptor, + bool ignore_override) +{ + parameter_descriptor.allowed_type = static_cast(type); + return this->declare_parameter( + name, rclcpp::ParameterValue{}, {parameter_descriptor, false}, ignore_override); +} + void NodeParameters::undeclare_parameter(const std::string & name) { diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index b74cda42e6..90cec5c582 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -327,7 +327,7 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { { // no default, no initial rclcpp::ParameterValue value = node->declare_parameter( - "parameter"_unq, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + "parameter"_unq, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { @@ -362,8 +362,7 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_THROW( { - node->declare_parameter( - name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } @@ -497,16 +496,14 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // no default, with override rclcpp::ParameterValue value = node->declare_parameter( - "parameter_no_default", rclcpp::ParameterValue{}, rclcpp::ParameterType::PARAMETER_INTEGER); + "parameter_no_default", rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 42); } { // no default, with override, and set after rclcpp::ParameterValue value = node->declare_parameter( - "parameter_no_default_set", - rclcpp::ParameterValue{}, - rclcpp::ParameterType::PARAMETER_INTEGER); + "parameter_no_default_set", rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 42); // check that the value is changed after a set @@ -516,9 +513,7 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // no default, with override const rclcpp::ParameterValue & value = node->declare_parameter( - "parameter_no_default_set_cvref", - rclcpp::ParameterValue{}, - rclcpp::ParameterType::PARAMETER_INTEGER); + "parameter_no_default_set_cvref", rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 42); // check that the value is changed after a set @@ -565,11 +560,10 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // parameter already declared throws auto name = "parameter_already_declared"; - node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_THROW( { - node->declare_parameter( - name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } @@ -741,52 +735,52 @@ TEST_F(TestNode, declare_parameter_with_cli_overrides) { auto node = std::make_shared("test_declare_parameter_node", no); { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_bool", rclcpp::ParameterValue{}, rclcpp::PARAMETER_BOOL); + "parameter_bool", rclcpp::ParameterType::PARAMETER_BOOL); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_BOOL); EXPECT_EQ(value.get(), true); } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_int", rclcpp::ParameterValue{}, rclcpp::PARAMETER_INTEGER); + "parameter_int", rclcpp::ParameterType::PARAMETER_INTEGER); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); EXPECT_EQ(value.get(), 21); // set to 42 in CLI, overriden by file } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_double", rclcpp::ParameterValue{}, rclcpp::PARAMETER_DOUBLE); + "parameter_double", rclcpp::ParameterType::PARAMETER_DOUBLE); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_DOUBLE); EXPECT_EQ(value.get(), 0.42); } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_string", rclcpp::ParameterValue{}, rclcpp::PARAMETER_STRING); + "parameter_string", rclcpp::ParameterType::PARAMETER_STRING); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_STRING); EXPECT_EQ(value.get(), "foo"); } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_bool_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_BOOL_ARRAY); + "parameter_bool_array", rclcpp::ParameterType::PARAMETER_BOOL_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_BOOL_ARRAY); std::vector expected_value{false, true}; EXPECT_EQ(value.get>(), expected_value); } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_int_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_INTEGER_ARRAY); + "parameter_int_array", rclcpp::ParameterType::PARAMETER_INTEGER_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER_ARRAY); std::vector expected_value{-21, 42}; EXPECT_EQ(value.get>(), expected_value); } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_double_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_DOUBLE_ARRAY); + "parameter_double_array", rclcpp::ParameterType::PARAMETER_DOUBLE_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_DOUBLE_ARRAY); std::vector expected_value{-1.0, 0.42}; EXPECT_EQ(value.get>(), expected_value); } { rclcpp::ParameterValue value = node->declare_parameter( - "parameter_string_array", rclcpp::ParameterValue{}, rclcpp::PARAMETER_STRING_ARRAY); + "parameter_string_array", rclcpp::ParameterType::PARAMETER_STRING_ARRAY); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_STRING_ARRAY); std::vector expected_value{"foo", "bar"}; // set to [baz, baz, baz] in file, overriden by CLI @@ -799,7 +793,7 @@ TEST_F(TestNode, undeclare_parameter) { { // normal use auto name = "parameter"_unq; - node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -841,7 +835,7 @@ TEST_F(TestNode, has_parameter) { // normal use auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); - node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -852,7 +846,7 @@ TEST_F(TestNode, list_parameters) { // normal use auto name = "parameter"_unq; const size_t before_size = node->list_parameters({}, 1u).names.size(); - node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_EQ(1u + before_size, node->list_parameters({}, 1u).names.size()); node->undeclare_parameter(name); EXPECT_EQ(before_size, node->list_parameters({}, 1u).names.size()); @@ -939,7 +933,7 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; auto value = node->declare_parameter( - name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1457,7 +1451,7 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; auto value = node->declare_parameter( - name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1642,8 +1636,7 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter( - name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + auto value = node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); From 2a514bdf7161dfbc26448ada9b769d536ce59857 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 22 Jan 2021 18:20:44 -0300 Subject: [PATCH 08/28] Add ParameterType::PARAMETER_DYNAMIC Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/parameter_value.hpp | 1 + .../node_interfaces/node_parameters.cpp | 9 +++++++-- rclcpp/test/rclcpp/test_node.cpp | 20 +++++++++---------- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/rclcpp/include/rclcpp/parameter_value.hpp b/rclcpp/include/rclcpp/parameter_value.hpp index 77bcdec51f..e779655447 100644 --- a/rclcpp/include/rclcpp/parameter_value.hpp +++ b/rclcpp/include/rclcpp/parameter_value.hpp @@ -32,6 +32,7 @@ namespace rclcpp enum ParameterType : uint8_t { PARAMETER_NOT_SET = rcl_interfaces::msg::ParameterType::PARAMETER_NOT_SET, + PARAMETER_DYNAMIC = rcl_interfaces::msg::ParameterType::PARAMETER_DYNAMIC, PARAMETER_BOOL = rcl_interfaces::msg::ParameterType::PARAMETER_BOOL, PARAMETER_INTEGER = rcl_interfaces::msg::ParameterType::PARAMETER_INTEGER, PARAMETER_DOUBLE = rcl_interfaces::msg::ParameterType::PARAMETER_DOUBLE, diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 5bbfa9132d..c818839068 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -259,7 +259,7 @@ __check_parameters( const rclcpp::ParameterType old_type = item.value.get_type(); const rclcpp::ParameterType type = parameter.get_type(); result.successful = - descriptor.allowed_type == rclcpp::PARAMETER_NOT_SET || + descriptor.allowed_type == rclcpp::PARAMETER_DYNAMIC || descriptor.allowed_type == type; if (!result.successful) { result.reason = format_type_reason( @@ -486,7 +486,7 @@ NodeParameters::undeclare_parameter(const std::string & name) throw rclcpp::exceptions::ParameterImmutableException( "cannot undeclare parameter '" + name + "' because it is read-only"); } - if (rclcpp::PARAMETER_NOT_SET != parameter_info->second.descriptor.allowed_type) { + if (rclcpp::PARAMETER_DYNAMIC != parameter_info->second.descriptor.allowed_type) { throw rclcpp::exceptions::InvalidParameterTypeException{ name, "cannot undeclare an statically typed parameter"}; } @@ -638,6 +638,11 @@ NodeParameters::set_parameters_atomically(const std::vector & if (rclcpp::PARAMETER_NOT_SET == parameter.get_type()) { auto it = parameters_.find(parameter.get_name()); if (it != parameters_.end() && rclcpp::PARAMETER_NOT_SET != it->second.value.get_type()) { + if (rclcpp::PARAMETER_DYNAMIC != it->second.descriptor.allowed_type) { + result.reason = "cannot undeclare an statically typed parameter"; + result.successful = false; + return result; + } parameters_to_be_undeclared.push_back(¶meter); } } diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 90cec5c582..7b936990c4 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -327,7 +327,7 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { { // no default, no initial rclcpp::ParameterValue value = node->declare_parameter( - "parameter"_unq, rclcpp::ParameterType::PARAMETER_NOT_SET); + "parameter"_unq, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { @@ -362,7 +362,7 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); EXPECT_THROW( { - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } @@ -560,10 +560,10 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // parameter already declared throws auto name = "parameter_already_declared"; - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_THROW( { - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } @@ -793,7 +793,7 @@ TEST_F(TestNode, undeclare_parameter) { { // normal use auto name = "parameter"_unq; - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -835,7 +835,7 @@ TEST_F(TestNode, has_parameter) { // normal use auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -846,7 +846,7 @@ TEST_F(TestNode, list_parameters) { // normal use auto name = "parameter"_unq; const size_t before_size = node->list_parameters({}, 1u).names.size(); - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_EQ(1u + before_size, node->list_parameters({}, 1u).names.size()); node->undeclare_parameter(name); EXPECT_EQ(before_size, node->list_parameters({}, 1u).names.size()); @@ -933,7 +933,7 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; auto value = node->declare_parameter( - name, rclcpp::ParameterType::PARAMETER_NOT_SET); + name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1451,7 +1451,7 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; auto value = node->declare_parameter( - name, rclcpp::ParameterType::PARAMETER_NOT_SET); + name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1636,7 +1636,7 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_NOT_SET); + auto value = node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); From 8871f97d8649cb713455baa25b89433c4dfe207c Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 26 Jan 2021 14:42:12 -0300 Subject: [PATCH 09/28] refactor Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 10 +-- rclcpp/include/rclcpp/node_impl.hpp | 6 +- .../node_interfaces/node_parameters.hpp | 3 +- .../node_parameters_interface.hpp | 5 +- .../include/rclcpp/parameter_descriptor.hpp | 90 ------------------- rclcpp/include/rclcpp/parameter_value.hpp | 3 +- rclcpp/src/rclcpp/node.cpp | 2 +- .../node_interfaces/node_parameters.cpp | 54 ++++++----- rclcpp/test/rclcpp/test_node.cpp | 62 +++++++++---- 9 files changed, 92 insertions(+), 143 deletions(-) delete mode 100644 rclcpp/include/rclcpp/parameter_descriptor.hpp diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index 26138d7c99..3443f54f50 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -311,8 +311,8 @@ class Node : public std::enable_shared_from_this declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), - const rclcpp::ParameterDescriptor & parameter_descriptor = - rclcpp::ParameterDescriptor(), + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false); /// Declare and initialize a parameter, return the effective value. @@ -357,8 +357,8 @@ class Node : public std::enable_shared_from_this declare_parameter( const std::string & name, const ParameterT & default_value, - const rclcpp::ParameterDescriptor & parameter_descriptor = - rclcpp::ParameterDescriptor(), + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false); /// Declare and initialize several parameters with the same namespace and type. @@ -417,7 +417,7 @@ class Node : public std::enable_shared_from_this const std::string & namespace_, const std::map< std::string, - std::pair + std::pair > & parameters, bool ignore_overrides = false); diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp index 27cb15b101..4a24d0d359 100644 --- a/rclcpp/include/rclcpp/node_impl.hpp +++ b/rclcpp/include/rclcpp/node_impl.hpp @@ -157,7 +157,7 @@ auto Node::declare_parameter( const std::string & name, const ParameterT & default_value, - const rclcpp::ParameterDescriptor & parameter_descriptor, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, bool ignore_override) { try { @@ -187,7 +187,7 @@ Node::declare_parameters( return this->declare_parameter( normalized_namespace + element.first, element.second, - rclcpp::ParameterDescriptor(), + rcl_interfaces::msg::ParameterDescriptor(), ignore_overrides); } ); @@ -200,7 +200,7 @@ Node::declare_parameters( const std::string & namespace_, const std::map< std::string, - std::pair + std::pair > & parameters, bool ignore_overrides) { diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp index 193823dfa3..d9f856da64 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp @@ -108,7 +108,8 @@ class NodeParameters : public NodeParametersInterface declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rclcpp::ParameterDescriptor & parameter_descriptor, + rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor{}, bool ignore_override = false) override; RCLCPP_PUBLIC diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index a501def8cf..364eb083c6 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -26,7 +26,6 @@ #include "rclcpp/macros.hpp" #include "rclcpp/parameter.hpp" -#include "rclcpp/parameter_descriptor.hpp" #include "rclcpp/visibility_control.hpp" namespace rclcpp @@ -66,8 +65,8 @@ class NodeParametersInterface declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), - const rclcpp::ParameterDescriptor & parameter_descriptor = - rclcpp::ParameterDescriptor(), + rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false) = 0; /// Declare and initialize a parameter. diff --git a/rclcpp/include/rclcpp/parameter_descriptor.hpp b/rclcpp/include/rclcpp/parameter_descriptor.hpp deleted file mode 100644 index b32c166715..0000000000 --- a/rclcpp/include/rclcpp/parameter_descriptor.hpp +++ /dev/null @@ -1,90 +0,0 @@ -// Copyright 2020 Open Source Robotics Foundation, Inc. -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - -#ifndef RCLCPP__PARAMETER_DESCRIPTOR_HPP_ -#define RCLCPP__PARAMETER_DESCRIPTOR_HPP_ - -namespace rclcpp -{ - -/// rcl_interfaces::msg::ParameterDescriptor wrapper used to streamline parameter declarations. -/** - * Examples: - * ```cpp - * // declare statically typed parameter, type inferred from gived default value - * node.declare_parameter("my_int", 5); - * - * // declare statically typed parameter, no default value given. - * node.declare_parameter( - * "my_int", rclcpp::ParameterType::INTEGER_PARAMETER); - * - * // declare statically typed parameter, type inferred from given default, with descriptor. - * rcl_interfaces::msg::ParameterDescriptor descriptor; - * //>> Set descriptor << - * node.declare_parameter("my_int", 5, descriptor); - * - * // declare statically typed parameter, type inferred from descriptor. - * rcl_interfaces::msg::ParameterDescriptor descriptor; - * descriptor.allowed_type = static_cast(rclcpp::ParameterType::INTEGER_PARAMETER); - * //>> Set other things in descriptor.<< - * node.declare_parameter("my_int", {descriptor, false}); - * - * // declare parameter that can change of type - * node.declare_parameter("my_param", rclcpp::ParameterType::PARAMETER_NOT_SET); - * - * // declare parameter that can change of type with default value - * node.declare_parameter("my_param", 5, rclcpp::ParameterDescriptor{false}); - * - * // declare parameter that can change of type with default value and descriptor - * rcl_interfaces::msg::ParameterDescriptor descriptor; - * //>> Set descriptor << - * node.declare_parameter("my_param", 5, {descriptor, false}); - */ -class ParameterDescriptor -{ -public: - /// Default constructor. - explicit ParameterDescriptor(bool infer_type_from_value = true) - : infer_type_from_value_{infer_type_from_value} {} - - /// Implicit constructor from parameter descriptor message. - ParameterDescriptor( - const rcl_interfaces::msg::ParameterDescriptor & msg, bool infer_type_from_value = true) - // NOLINT: implicit conversion constructor - : impl_(msg), infer_type_from_value_{infer_type_from_value} {} - - /// Getter of internal message representation. - rcl_interfaces::msg::ParameterDescriptor & - get_msg() - {return impl_;} - - /// Getter of internal message representation. - const rcl_interfaces::msg::ParameterDescriptor & - get_msg() const - {return impl_;} - - /// Return true if we have to ignore the allowed type provided in the descriptor and infer - /// it later from the default value. - bool - do_infer_type_from_value() const - {return infer_type_from_value_;} - -private: - rcl_interfaces::msg::ParameterDescriptor impl_; - bool infer_type_from_value_; -}; - -} // namespace rclcpp - -#endif // RCLCPP__PARAMETER_DESCRIPTOR_HPP_ diff --git a/rclcpp/include/rclcpp/parameter_value.hpp b/rclcpp/include/rclcpp/parameter_value.hpp index e779655447..7272337f6f 100644 --- a/rclcpp/include/rclcpp/parameter_value.hpp +++ b/rclcpp/include/rclcpp/parameter_value.hpp @@ -32,7 +32,8 @@ namespace rclcpp enum ParameterType : uint8_t { PARAMETER_NOT_SET = rcl_interfaces::msg::ParameterType::PARAMETER_NOT_SET, - PARAMETER_DYNAMIC = rcl_interfaces::msg::ParameterType::PARAMETER_DYNAMIC, + /// Special value, only used to declare a dynamically typed parameter. + /// Not valid as a parameter type. PARAMETER_BOOL = rcl_interfaces::msg::ParameterType::PARAMETER_BOOL, PARAMETER_INTEGER = rcl_interfaces::msg::ParameterType::PARAMETER_INTEGER, PARAMETER_DOUBLE = rcl_interfaces::msg::ParameterType::PARAMETER_DOUBLE, diff --git a/rclcpp/src/rclcpp/node.cpp b/rclcpp/src/rclcpp/node.cpp index 999f11d378..aac18ca8f1 100644 --- a/rclcpp/src/rclcpp/node.cpp +++ b/rclcpp/src/rclcpp/node.cpp @@ -223,7 +223,7 @@ const rclcpp::ParameterValue & Node::declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rclcpp::ParameterDescriptor & parameter_descriptor, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, bool ignore_override) { return this->node_parameters_->declare_parameter( diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index c818839068..4e351fe179 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -133,13 +133,15 @@ NodeParameters::NodeParameters( // If asked, initialize any parameters that ended up in the initial parameter values, // but did not get declared explcitily by this point. + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; if (automatically_declare_parameters_from_overrides) { for (const auto & pair : this->get_parameter_overrides()) { if (!this->has_parameter(pair.first)) { this->declare_parameter( pair.first, pair.second, - rclcpp::ParameterDescriptor(false), + descriptor, true); } } @@ -257,10 +259,10 @@ __check_parameters( auto item = parameter_infos[name]; const rcl_interfaces::msg::ParameterDescriptor & descriptor = item.descriptor; const rclcpp::ParameterType old_type = item.value.get_type(); - const rclcpp::ParameterType type = parameter.get_type(); + const auto type = static_cast(descriptor.type); result.successful = - descriptor.allowed_type == rclcpp::PARAMETER_DYNAMIC || - descriptor.allowed_type == type; + descriptor.dynamic_typing || + descriptor.type == type; if (!result.successful) { result.reason = format_type_reason( name, rclcpp::to_string(old_type), rclcpp::to_string(type)); @@ -349,7 +351,7 @@ rcl_interfaces::msg::SetParametersResult __declare_parameter_common( const std::string & name, const rclcpp::ParameterValue & default_value, - const rclcpp::ParameterDescriptor & parameter_descriptor, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, std::map & parameters_out, const std::map & overrides, CallbacksContainerType & callback_container, @@ -359,17 +361,7 @@ __declare_parameter_common( { using rclcpp::node_interfaces::ParameterInfo; std::map parameter_infos {{name, ParameterInfo()}}; - auto & param_info = parameter_infos.at(name); - param_info.descriptor = parameter_descriptor.get_msg(); - if (parameter_descriptor.do_infer_type_from_value()) { - if (rclcpp::ParameterType::PARAMETER_NOT_SET == default_value.get_type()) { - throw rclcpp::exceptions::InvalidParameterTypeException{ - name, - "If not passing a default value, a parameter type must be specified in the " - "descriptor or dynamic typing must be allowed"}; - } - param_info.descriptor.allowed_type = static_cast(default_value.get_type()); - } + parameter_infos.at(name).descriptor = parameter_descriptor; // Use the value from the overrides if available, otherwise use the default. const rclcpp::ParameterValue * initial_value = &default_value; @@ -402,7 +394,7 @@ const rclcpp::ParameterValue & NodeParameters::declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rclcpp::ParameterDescriptor & parameter_descriptor, + rcl_interfaces::msg::ParameterDescriptor parameter_descriptor, bool ignore_override) { std::lock_guard lock(mutex_); @@ -420,6 +412,20 @@ NodeParameters::declare_parameter( "parameter '" + name + "' has already been declared"); } + if (!parameter_descriptor.dynamic_typing) { + auto type = parameter_descriptor.type; + if (rclcpp::PARAMETER_NOT_SET == type) { + type = default_value.get_type(); + } + if (rclcpp::PARAMETER_NOT_SET == type) { + throw rclcpp::exceptions::InvalidParameterTypeException{ + name, + "cannot declare a statically typed parameter with an uninitialized value" + }; + } + parameter_descriptor.type = type; + } + rcl_interfaces::msg::ParameterEvent parameter_event; auto result = __declare_parameter_common( name, @@ -464,9 +470,9 @@ NodeParameters::declare_parameter( rcl_interfaces::msg::ParameterDescriptor parameter_descriptor, bool ignore_override) { - parameter_descriptor.allowed_type = static_cast(type); + parameter_descriptor.type = type; return this->declare_parameter( - name, rclcpp::ParameterValue{}, {parameter_descriptor, false}, ignore_override); + name, rclcpp::ParameterValue{}, parameter_descriptor, ignore_override); } void @@ -486,7 +492,7 @@ NodeParameters::undeclare_parameter(const std::string & name) throw rclcpp::exceptions::ParameterImmutableException( "cannot undeclare parameter '" + name + "' because it is read-only"); } - if (rclcpp::PARAMETER_DYNAMIC != parameter_info->second.descriptor.allowed_type) { + if (!parameter_info->second.descriptor.dynamic_typing) { throw rclcpp::exceptions::InvalidParameterTypeException{ name, "cannot undeclare an statically typed parameter"}; } @@ -583,13 +589,17 @@ NodeParameters::set_parameters_atomically(const std::vector & rcl_interfaces::msg::ParameterEvent parameter_event_msg; parameter_event_msg.node = combined_name_; CallbacksContainerType empty_callback_container; + + // Implicit declare uses dynamic type descriptor. + rcl_interfaces::msg::ParameterDescriptor descriptor{}; + descriptor.dynamic_typing = true; for (auto parameter_to_be_declared : parameters_to_be_declared) { // This should not throw, because we validated the name and checked that // the parameter was not already declared. result = __declare_parameter_common( parameter_to_be_declared->get_name(), parameter_to_be_declared->get_parameter_value(), - rclcpp::ParameterDescriptor(false), // Implicit declare uses default descriptor. + descriptor, staged_parameter_changes, parameter_overrides_, // Only call callbacks once below @@ -638,7 +648,7 @@ NodeParameters::set_parameters_atomically(const std::vector & if (rclcpp::PARAMETER_NOT_SET == parameter.get_type()) { auto it = parameters_.find(parameter.get_name()); if (it != parameters_.end() && rclcpp::PARAMETER_NOT_SET != it->second.value.get_type()) { - if (rclcpp::PARAMETER_DYNAMIC != it->second.descriptor.allowed_type) { + if (!it->second.descriptor.dynamic_typing) { result.reason = "cannot undeclare an statically typed parameter"; result.successful = false; return result; diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 7b936990c4..6aa52ddea9 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -325,9 +325,11 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { // test cases without initial values auto node = std::make_shared("test_declare_parameter_node"_unq); { + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; // no default, no initial rclcpp::ParameterValue value = node->declare_parameter( - "parameter"_unq, rclcpp::ParameterType::PARAMETER_DYNAMIC); + "parameter"_unq, rclcpp::ParameterValue{}, descriptor); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); } { @@ -359,10 +361,14 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { { // parameter already declared throws auto name = "parameter"_unq; - node->declare_parameter(name, rclcpp::ParameterValue{}, rclcpp::ParameterDescriptor{false}); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_THROW( { - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } @@ -560,10 +566,14 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // parameter already declared throws auto name = "parameter_already_declared"; - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_THROW( { - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); }, rclcpp::exceptions::ParameterAlreadyDeclaredException); } @@ -793,7 +803,9 @@ TEST_F(TestNode, undeclare_parameter) { { // normal use auto name = "parameter"_unq; - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -835,7 +847,9 @@ TEST_F(TestNode, has_parameter) { // normal use auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_TRUE(node->has_parameter(name)); node->undeclare_parameter(name); EXPECT_FALSE(node->has_parameter(name)); @@ -846,7 +860,9 @@ TEST_F(TestNode, list_parameters) { // normal use auto name = "parameter"_unq; const size_t before_size = node->list_parameters({}, 1u).names.size(); - node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_EQ(1u + before_size, node->list_parameters({}, 1u).names.size()); node->undeclare_parameter(name); EXPECT_EQ(before_size, node->list_parameters({}, 1u).names.size()); @@ -879,7 +895,9 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { // normal use, change type auto name = "parameter"_unq; EXPECT_FALSE(node->has_parameter(name)); - node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, 42, descriptor); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(node->get_parameter(name).get_value(), 42); EXPECT_TRUE(node->set_parameter(rclcpp::Parameter(name, "not an integer")).successful); @@ -932,8 +950,9 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter( - name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + auto value = node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -956,7 +975,9 @@ TEST_F(TestNode, set_parameter_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, // when dynamic typing is allowing and already set to another type, will undeclare auto name = "parameter"_unq; - node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, 42, descriptor); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); @@ -1450,8 +1471,9 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter( - name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + auto value = node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1474,7 +1496,9 @@ TEST_F(TestNode, set_parameters_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, // when already to another type and dynamic typic allowed, will undeclare auto name = "parameter"_unq; - node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, 42, descriptor); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); @@ -1636,7 +1660,9 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { { // setting type of rclcpp::PARAMETER_NOT_SET, when already not set, does not undeclare auto name = "parameter"_unq; - auto value = node->declare_parameter(name, rclcpp::ParameterType::PARAMETER_DYNAMIC); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + auto value = node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); EXPECT_TRUE(node->has_parameter(name)); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_NOT_SET); @@ -1659,7 +1685,9 @@ TEST_F(TestNode, set_parameters_atomically_undeclared_parameters_not_allowed) { // setting type of rclcpp::PARAMETER_NOT_SET, // when dynamic typing is allowed and already declared to another type, will undeclare auto name = "parameter"_unq; - node->declare_parameter(name, 42, rclcpp::ParameterDescriptor(false)); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + node->declare_parameter(name, 42, descriptor); EXPECT_TRUE(node->has_parameter(name)); auto value = node->get_parameter(name); EXPECT_EQ(value.get_type(), rclcpp::PARAMETER_INTEGER); From 702519f3bf9b700f704e0001380c1e9f014c6752 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Tue, 26 Jan 2021 15:09:59 -0300 Subject: [PATCH 10/28] fixup Signed-off-by: Ivan Santiago Paunovic --- .../node_interfaces/node_parameters.hpp | 4 +- .../node_parameters_interface.hpp | 4 +- .../node_interfaces/node_parameters.cpp | 90 ++++++++++++++----- 3 files changed, 72 insertions(+), 26 deletions(-) diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp index d9f856da64..6f0d5b0ed2 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp @@ -108,7 +108,7 @@ class NodeParameters : public NodeParametersInterface declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = rcl_interfaces::msg::ParameterDescriptor{}, bool ignore_override = false) override; @@ -117,7 +117,7 @@ class NodeParameters : public NodeParametersInterface declare_parameter( const std::string & name, rclcpp::ParameterType type, - rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false) override; diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index 364eb083c6..824473b95a 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -65,7 +65,7 @@ class NodeParametersInterface declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), - rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false) = 0; @@ -79,7 +79,7 @@ class NodeParametersInterface declare_parameter( const std::string & name, rclcpp::ParameterType type, - rcl_interfaces::msg::ParameterDescriptor parameter_descriptor = + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false) = 0; diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 4e351fe179..432ec070e2 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -390,30 +390,34 @@ __declare_parameter_common( return result; } +static const rclcpp::ParameterValue & -NodeParameters::declare_parameter( +declare_parameter_helper( const std::string & name, + rclcpp::ParameterType type, const rclcpp::ParameterValue & default_value, rcl_interfaces::msg::ParameterDescriptor parameter_descriptor, - bool ignore_override) + bool ignore_override, + std::map & parameters, + const std::map & overrides, + CallbacksContainerType & callback_container, + const OnParametersSetCallbackType & callback, + rclcpp::Publisher * events_publisher, + const std::string & combined_name, + rclcpp::node_interfaces::NodeClockInterface & node_clock) { - std::lock_guard lock(mutex_); - - ParameterMutationRecursionGuard guard(parameter_modification_enabled_); - // TODO(sloretz) parameter name validation if (name.empty()) { throw rclcpp::exceptions::InvalidParametersException("parameter name must not be empty"); } // Error if this parameter has already been declared and is different - if (__lockless_has_parameter(parameters_, name)) { + if (__lockless_has_parameter(parameters, name)) { throw rclcpp::exceptions::ParameterAlreadyDeclaredException( "parameter '" + name + "' has already been declared"); } if (!parameter_descriptor.dynamic_typing) { - auto type = parameter_descriptor.type; if (rclcpp::PARAMETER_NOT_SET == type) { type = default_value.get_type(); } @@ -423,7 +427,7 @@ NodeParameters::declare_parameter( "cannot declare a statically typed parameter with an uninitialized value" }; } - parameter_descriptor.type = type; + parameter_descriptor.type = static_cast(type); } rcl_interfaces::msg::ParameterEvent parameter_event; @@ -431,10 +435,10 @@ NodeParameters::declare_parameter( name, default_value, parameter_descriptor, - parameters_, - parameter_overrides_, - on_parameters_set_callback_container_, - on_parameters_set_callback_, + parameters, + overrides, + callback_container, + callback, ¶meter_event, ignore_override); @@ -454,25 +458,67 @@ NodeParameters::declare_parameter( } // Publish if events_publisher_ is not nullptr, which may be if disabled in the constructor. - if (nullptr != events_publisher_) { - parameter_event.node = combined_name_; - parameter_event.stamp = node_clock_->get_clock()->now(); - events_publisher_->publish(parameter_event); + if (nullptr != events_publisher) { + parameter_event.node = combined_name; + parameter_event.stamp = node_clock.get_clock()->now(); + events_publisher->publish(parameter_event); } - return parameters_.at(name).value; + return parameters.at(name).value; +} + +const rclcpp::ParameterValue & +NodeParameters::declare_parameter( + const std::string & name, + const rclcpp::ParameterValue & default_value, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) +{ + std::lock_guard lock(mutex_); + ParameterMutationRecursionGuard guard(parameter_modification_enabled_); + + return declare_parameter_helper( + name, + rclcpp::PARAMETER_NOT_SET, + default_value, + parameter_descriptor, + ignore_override, + parameters_, + parameter_overrides_, + on_parameters_set_callback_container_, + on_parameters_set_callback_, + events_publisher_.get(), + combined_name_, + *node_clock_); } const rclcpp::ParameterValue & NodeParameters::declare_parameter( const std::string & name, rclcpp::ParameterType type, - rcl_interfaces::msg::ParameterDescriptor parameter_descriptor, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, bool ignore_override) { - parameter_descriptor.type = type; - return this->declare_parameter( - name, rclcpp::ParameterValue{}, parameter_descriptor, ignore_override); + std::lock_guard lock(mutex_); + ParameterMutationRecursionGuard guard(parameter_modification_enabled_); + + if (rclcpp::PARAMETER_NOT_SET == type) { + throw std::invalid_argument + } + + return declare_parameter_helper( + name, + type, + rclcpp::ParameterValue{}, + parameter_descriptor, + ignore_override, + parameters_, + parameter_overrides_, + on_parameters_set_callback_container_, + on_parameters_set_callback_, + events_publisher_.get(), + combined_name_, + *node_clock_); } void From f7ba067895f695f08a1f9f844114c92813fe9f17 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 27 Jan 2021 10:45:38 -0300 Subject: [PATCH 11/28] fix behavior when allow_undeclared Signed-off-by: Ivan Santiago Paunovic --- .../node_interfaces/node_parameters.cpp | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 432ec070e2..904f0d4ea1 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -250,22 +250,31 @@ RCLCPP_LOCAL rcl_interfaces::msg::SetParametersResult __check_parameters( std::map & parameter_infos, - const std::vector & parameters) + const std::vector & parameters, + bool allow_undeclared) { rcl_interfaces::msg::SetParametersResult result; result.successful = true; for (const rclcpp::Parameter & parameter : parameters) { std::string name = parameter.get_name(); - auto item = parameter_infos[name]; - const rcl_interfaces::msg::ParameterDescriptor & descriptor = item.descriptor; - const rclcpp::ParameterType old_type = item.value.get_type(); - const auto type = static_cast(descriptor.type); - result.successful = - descriptor.dynamic_typing || - descriptor.type == type; + rcl_interfaces::msg::ParameterDescriptor descriptor; + if (allow_undeclared) { + auto it = parameter_infos.find(name); + if (it != parameter_infos.cend()) { + descriptor = it->second.descriptor; + } else { + // implicitly declared parameters are dinamically typed! + descriptor.dynamic_typing = true; + } + } else { + descriptor = parameter_infos[name].descriptor; + } + const auto new_type = parameter.get_type(); + const auto specified_type = static_cast(descriptor.type); + result.successful = descriptor.dynamic_typing || specified_type == new_type; if (!result.successful) { result.reason = format_type_reason( - name, rclcpp::to_string(old_type), rclcpp::to_string(type)); + name, rclcpp::to_string(specified_type), rclcpp::to_string(new_type)); return result; } result = __check_parameter_value_in_range( @@ -319,7 +328,8 @@ __set_parameters_atomically_common( const std::vector & parameters, std::map & parameter_infos, CallbacksContainerType & callback_container, - const OnParametersSetCallbackType & callback) + const OnParametersSetCallbackType & callback, + bool allow_undeclared = false) { // Call the user callback to see if the new value(s) are allowed. rcl_interfaces::msg::SetParametersResult result = @@ -328,7 +338,7 @@ __set_parameters_atomically_common( return result; } // Check if the value being set complies with the descriptor. - result = __check_parameters(parameter_infos, parameters); + result = __check_parameters(parameter_infos, parameters, allow_undeclared); if (!result.successful) { return result; } @@ -503,7 +513,8 @@ NodeParameters::declare_parameter( ParameterMutationRecursionGuard guard(parameter_modification_enabled_); if (rclcpp::PARAMETER_NOT_SET == type) { - throw std::invalid_argument + throw std::invalid_argument{ + "declare_parameter(): the provided parameter type cannot be rclcpp::PARAMETER_NOT_SET"}; } return declare_parameter_helper( @@ -637,7 +648,7 @@ NodeParameters::set_parameters_atomically(const std::vector & CallbacksContainerType empty_callback_container; // Implicit declare uses dynamic type descriptor. - rcl_interfaces::msg::ParameterDescriptor descriptor{}; + rcl_interfaces::msg::ParameterDescriptor descriptor; descriptor.dynamic_typing = true; for (auto parameter_to_be_declared : parameters_to_be_declared) { // This should not throw, because we validated the name and checked that @@ -714,7 +725,8 @@ NodeParameters::set_parameters_atomically(const std::vector & on_parameters_set_callback_container_, // These callbacks are called once. When a callback returns an unsuccessful result, // the remaining aren't called. - on_parameters_set_callback_); + on_parameters_set_callback_, + allow_undeclared_); // allow undeclared // If not successful, then stop here. if (!result.successful) { From a0979f8595ef1e8da44dab41ec2fb486951f5be6 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 27 Jan 2021 11:29:57 -0300 Subject: [PATCH 12/28] Test cases and docs Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 19 +++++++++--- rclcpp/test/rclcpp/test_node.cpp | 51 ++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 4 deletions(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index 3443f54f50..126d7cc130 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -305,6 +305,8 @@ class Node : public std::enable_shared_from_this * name is invalid. * \throws rclcpp::exceptions::InvalidParameterValueException if initial * value fails to be set. + * \throws rclcpp::exceptions::InvalidParameterTypeException + * if the type of the default value or override is wrong. */ RCLCPP_PUBLIC const rclcpp::ParameterValue & @@ -317,10 +319,19 @@ class Node : public std::enable_shared_from_this /// Declare and initialize a parameter, return the effective value. /** - * Same as: - * ```cpp - * node.declare_parameter(name, ParameterValue{}, descriptor, ignore_override); - * ``` + * Same as the previous one, but a default value is not provided and the user + * must provide a parameter override of the correct type. + * + * \param[in] name The name of the parameter. + * \param[in] type Desired type of the parameter, which will enforced at runtime. + * \param[in] parameter_descriptor An optional, custom description for + * the parameter. + * \param[in] ignore_override When `true`, the parameter override is ignored. + * Default to `false`. + * \return A const reference to the value of the parameter. + * \throws Same as the previous overload taking a default value. + * \throws rclcpp::exceptions::InvalidParameterTypeException + * if an override is not provided or the provided override is of the wrong type. */ RCLCPP_PUBLIC const rclcpp::ParameterValue & diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 6aa52ddea9..1661622ff3 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -2659,3 +2659,54 @@ TEST_F(TestNode, create_sub_node_rmw_validate_namespace_error) { rclcpp::exceptions::RCLError); } } + +TEST_F(TestNode, static_and_dynamic_typing) { + rclcpp::NodeOptions no; + no.parameter_overrides( + { + {"integer_parameter_override_ok", 43}, + {"string_parameter_override_should_throw", 43}, + {"integer_must_provide_override", 43}, + }); + auto node = std::make_shared("node", "ns", no); + { + auto param = node->declare_parameter("an_int", 42); + EXPECT_EQ(42, param); + auto result = node->set_parameter({"an_int", "string value"}); + EXPECT_FALSE(result.successful); + result = node->set_parameter({"an_int", 43}); + EXPECT_TRUE(result.successful); + EXPECT_TRUE(node->get_parameter("an_int", param)); + EXPECT_EQ(43, param); + } + { + auto param = node->declare_parameter("integer_parameter_override_ok", 42); + EXPECT_EQ(43, param); + } + { + EXPECT_THROW( + node->declare_parameter("string_parameter_override_should_throw", "a string"), + rclcpp::exceptions::InvalidParameterTypeException); + } + { + auto param = node->declare_parameter( + "integer_must_provide_override", rclcpp::PARAMETER_INTEGER); + EXPECT_EQ(43, param.get()); + } + { + EXPECT_THROW( + node->declare_parameter("integer_override_not_given", rclcpp::PARAMETER_INTEGER), + rclcpp::exceptions::InvalidParameterTypeException); + } + { + EXPECT_THROW( + node->declare_parameter("parameter_not_set_is_not_a_valid_type", rclcpp::PARAMETER_NOT_SET), + std::invalid_argument); + } + { + EXPECT_THROW( + node->declare_parameter( + "uninitialized_not_valid_except_dynamic_typing", rclcpp::ParameterValue{}), + rclcpp::exceptions::InvalidParameterTypeException); + } +} From 0195a97a1508ae962be7ae78f1b250fa9574c66a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 27 Jan 2021 11:44:07 -0300 Subject: [PATCH 13/28] cool templated overload Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 12 ++++++++++++ rclcpp/include/rclcpp/node_impl.hpp | 17 +++++++++++++++++ rclcpp/test/rclcpp/test_node.cpp | 6 ++++++ 3 files changed, 35 insertions(+) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index 126d7cc130..9cb1979736 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -372,6 +372,18 @@ class Node : public std::enable_shared_from_this rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false); + /// Declare and initialize a parameter with a type. + /** + * See the non-templated declare_parameter() on this class for details. + */ + template + auto + declare_parameter( + const std::string & name, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), + bool ignore_override = false); + /// Declare and initialize several parameters with the same namespace and type. /** * For each key in the map, a parameter with a name of "namespace.key" diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp index 4a24d0d359..8490b251a2 100644 --- a/rclcpp/include/rclcpp/node_impl.hpp +++ b/rclcpp/include/rclcpp/node_impl.hpp @@ -172,6 +172,23 @@ Node::declare_parameter( } } +template +auto +Node::declare_parameter( + const std::string & name, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) +{ + // get advantage of parameter value template magic to get + // the correct rclcpp::ParameterType from ParameterT + rclcpp::ParameterValue value{ParameterT{}}; + return this->declare_parameter( + name, + value.get_type(), + parameter_descriptor, + ignore_override).get(); +} + template std::vector Node::declare_parameters( diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 1661622ff3..08a3d0f6d0 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -2667,6 +2667,7 @@ TEST_F(TestNode, static_and_dynamic_typing) { {"integer_parameter_override_ok", 43}, {"string_parameter_override_should_throw", 43}, {"integer_must_provide_override", 43}, + {"cool_way_of_declaring_a_string_without_a_default", "hello!"} }); auto node = std::make_shared("node", "ns", no); { @@ -2693,6 +2694,11 @@ TEST_F(TestNode, static_and_dynamic_typing) { "integer_must_provide_override", rclcpp::PARAMETER_INTEGER); EXPECT_EQ(43, param.get()); } + { + auto param = node->declare_parameter( + "cool_way_of_declaring_a_string_without_a_default"); + EXPECT_EQ("hello!", param); + } { EXPECT_THROW( node->declare_parameter("integer_override_not_given", rclcpp::PARAMETER_INTEGER), From cc24d9b4fcaf86c75602805fb207a44b44f57273 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 27 Jan 2021 15:10:33 -0300 Subject: [PATCH 14/28] Deprecate declaring parameter with only a name Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 17 ++++++++++++- .../node_interfaces/node_parameters.hpp | 19 +++++++++++++++ .../node_parameters_interface.hpp | 24 +++++++++++++++++-- rclcpp/src/rclcpp/node.cpp | 18 ++++++++++++++ .../node_interfaces/node_parameters.cpp | 8 +++++++ .../benchmark_node_parameters_interface.cpp | 13 ++++++---- .../benchmark/benchmark_parameter_client.cpp | 4 +++- rclcpp/test/rclcpp/test_node.cpp | 20 ++++++++++++++-- 8 files changed, 113 insertions(+), 10 deletions(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index 9cb1979736..0f1ef0758f 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -312,7 +312,7 @@ class Node : public std::enable_shared_from_this const rclcpp::ParameterValue & declare_parameter( const std::string & name, - const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), + const rclcpp::ParameterValue & default_value, const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false); @@ -342,6 +342,21 @@ class Node : public std::enable_shared_from_this rcl_interfaces::msg::ParameterDescriptor{}, bool ignore_override = false); + [[deprecated( + "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ + "If you want to declare a parameter that won't change of type without a default value use:\n" \ + "`node->declare_parameter(name)`, where e.g. ParameterT=int64_t.\n\n" \ + "If you want to declare a parameter that can dynamically change of type use:\n" \ + "```\n" \ + "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ + "descriptor.dynamic_typing = true;\n" \ + "node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);" \ + "```" + )]] + RCLCPP_PUBLIC + const rclcpp::ParameterValue & + declare_parameter(const std::string & name); + /// Declare and initialize a parameter with a type. /** * See the non-templated declare_parameter() on this class for details. diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp index 6f0d5b0ed2..3452c44f26 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp @@ -103,6 +103,25 @@ class NodeParameters : public NodeParametersInterface virtual ~NodeParameters(); +// This is overriding a deprecated method, so we need to ignore the deprecation warning here. +// Users of the method will still get a warning! +#ifndef _WIN32 +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#else +# pragma warning(push) +# pragma warning(disable: 4996) +#endif + [[deprecated(RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPTRECATE_DECLARE)]] + RCLCPP_PUBLIC + const rclcpp::ParameterValue & + declare_parameter(const std::string & name) override; +#ifndef _WIN32 +# pragma GCC diagnostic pop +#else +# pragma warning(pop) +#endif + RCLCPP_PUBLIC const rclcpp::ParameterValue & declare_parameter( diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index 824473b95a..ebce3ecda9 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -45,6 +45,17 @@ struct OnSetParametersCallbackHandle OnParametersSetCallbackType callback; }; +#define RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPTRECATE_DECLARE \ + "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ + "If you want to declare a parameter that won't change of type without a default value use:\n" \ + "`node_params->declare_parameter(name, type)`, with e.g. type=rclcpp::PARAMETER_INTEGER.\n\n" \ + "If you want to declare a parameter that can dynamically change of type use:\n" \ + "```\n" \ + "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ + "descriptor.dynamic_typing = true;\n" \ + "node_params->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);" \ + "```" + /// Pure virtual interface class for the NodeParameters part of the Node API. class NodeParametersInterface { @@ -55,6 +66,15 @@ class NodeParametersInterface virtual ~NodeParametersInterface() = default; + /// Declare a parameter. + /** + * \sa rclcpp::Node::declare_parameter + */ + [[deprecated(RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPTRECATE_DECLARE)]] + virtual + const rclcpp::ParameterValue & + declare_parameter(const std::string & name) = 0; + /// Declare and initialize a parameter. /** * \sa rclcpp::Node::declare_parameter @@ -64,12 +84,12 @@ class NodeParametersInterface const rclcpp::ParameterValue & declare_parameter( const std::string & name, - const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), + const rclcpp::ParameterValue & default_value, const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = rcl_interfaces::msg::ParameterDescriptor(), bool ignore_override = false) = 0; - /// Declare and initialize a parameter. + /// Declare a parameter. /** * \sa rclcpp::Node::declare_parameter */ diff --git a/rclcpp/src/rclcpp/node.cpp b/rclcpp/src/rclcpp/node.cpp index aac18ca8f1..0ccec6ae34 100644 --- a/rclcpp/src/rclcpp/node.cpp +++ b/rclcpp/src/rclcpp/node.cpp @@ -219,6 +219,24 @@ Node::create_callback_group( return node_base_->create_callback_group(group_type, automatically_add_to_executor_with_node); } +const rclcpp::ParameterValue & +Node::declare_parameter(const std::string & name) +{ +#ifndef _WIN32 +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#else +# pragma warning(push) +# pragma warning(disable: 4996) +#endif + return this->node_parameters_->declare_parameter(name); +#ifndef _WIN32 +# pragma GCC diagnostic pop +#else +# pragma warning(pop) +#endif +} + const rclcpp::ParameterValue & Node::declare_parameter( const std::string & name, diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 904f0d4ea1..4161b960f6 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -477,6 +477,14 @@ declare_parameter_helper( return parameters.at(name).value; } +const rclcpp::ParameterValue & +NodeParameters::declare_parameter(const std::string & name) +{ + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + return this->declare_parameter(name, rclcpp::ParameterValue{}, descriptor, false); +} + const rclcpp::ParameterValue & NodeParameters::declare_parameter( const std::string & name, diff --git a/rclcpp/test/benchmark/benchmark_node_parameters_interface.cpp b/rclcpp/test/benchmark/benchmark_node_parameters_interface.cpp index 26c0bcbb94..be447b7190 100644 --- a/rclcpp/test/benchmark/benchmark_node_parameters_interface.cpp +++ b/rclcpp/test/benchmark/benchmark_node_parameters_interface.cpp @@ -30,6 +30,7 @@ class NodeParametersInterfaceTest : public performance_test_fixture::Performance param2_name(param_prefix + ".my_param_2"), param3_name(param_prefix + ".my_param_3") { + dynamically_typed_descriptor.dynamic_typing = true; } void SetUp(benchmark::State & state) @@ -37,9 +38,12 @@ class NodeParametersInterfaceTest : public performance_test_fixture::Performance rclcpp::init(0, nullptr); node = std::make_shared(node_name); - node->declare_parameter(param1_name); - node->declare_parameter(param2_name); - node->declare_parameter(param3_name); + node->declare_parameter( + param1_name, rclcpp::ParameterValue{}, dynamically_typed_descriptor); + node->declare_parameter( + param2_name, rclcpp::ParameterValue{}, dynamically_typed_descriptor); + node->declare_parameter( + param3_name, rclcpp::ParameterValue{}, dynamically_typed_descriptor); node->undeclare_parameter(param3_name); performance_test_fixture::PerformanceTest::SetUp(state); @@ -58,6 +62,7 @@ class NodeParametersInterfaceTest : public performance_test_fixture::Performance const std::string param1_name; const std::string param2_name; const std::string param3_name; + rcl_interfaces::msg::ParameterDescriptor dynamically_typed_descriptor; protected: rclcpp::Node::SharedPtr node; @@ -66,7 +71,7 @@ class NodeParametersInterfaceTest : public performance_test_fixture::Performance BENCHMARK_F(NodeParametersInterfaceTest, declare_undeclare)(benchmark::State & state) { for (auto _ : state) { - node->declare_parameter(param3_name); + node->declare_parameter(param3_name, rclcpp::ParameterValue{}, dynamically_typed_descriptor); node->undeclare_parameter(param3_name); } } diff --git a/rclcpp/test/benchmark/benchmark_parameter_client.cpp b/rclcpp/test/benchmark/benchmark_parameter_client.cpp index 7a4c3181b7..7d27107fab 100644 --- a/rclcpp/test/benchmark/benchmark_parameter_client.cpp +++ b/rclcpp/test/benchmark/benchmark_parameter_client.cpp @@ -95,7 +95,9 @@ class ParameterClientTest : public RemoteNodeTest param1_name, rclcpp::ParameterValue("param1_value")); remote_node->declare_parameter( param2_name, rclcpp::ParameterValue(std::vector {1, 2, 3})); - remote_node->declare_parameter(param3_name); + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; + remote_node->declare_parameter(param3_name, rclcpp::ParameterValue{}, descriptor); remote_node->undeclare_parameter(param3_name); params_client = std::make_shared(node, remote_node_name); diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 08a3d0f6d0..090bff3e64 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -375,7 +375,7 @@ TEST_F(TestNode, declare_parameter_with_no_initial_values) { { // parameter name invalid throws EXPECT_THROW( - {node->declare_parameter("");}, + {node->declare_parameter("", 5);}, rclcpp::exceptions::InvalidParametersException); } { @@ -580,7 +580,7 @@ TEST_F(TestNode, declare_parameter_with_overrides) { { // parameter name invalid throws EXPECT_THROW( - {node->declare_parameter("");}, + {node->declare_parameter("", 5);}, rclcpp::exceptions::InvalidParametersException); } { @@ -2715,4 +2715,20 @@ TEST_F(TestNode, static_and_dynamic_typing) { "uninitialized_not_valid_except_dynamic_typing", rclcpp::ParameterValue{}), rclcpp::exceptions::InvalidParameterTypeException); } + { +#ifndef _WIN32 +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#else +# pragma warning(push) +# pragma warning(disable: 4996) +#endif + auto param = node->declare_parameter("deprecated_way_dynamic_typing"); + EXPECT_EQ(param, rclcpp::ParameterValue{}); +#ifndef _WIN32 +# pragma GCC diagnostic pop +#else +# pragma warning(pop) +#endif + } } From ca9431fbefc73c30dec990e912293d7cdd1934f3 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 16:44:39 -0300 Subject: [PATCH 15/28] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/include/rclcpp/node.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index 0f1ef0758f..0e1320a9a0 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -344,7 +344,7 @@ class Node : public std::enable_shared_from_this [[deprecated( "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ - "If you want to declare a parameter that won't change of type without a default value use:\n" \ + "If you want to declare a parameter that won't change type without a default value use:\n" \ "`node->declare_parameter(name)`, where e.g. ParameterT=int64_t.\n\n" \ "If you want to declare a parameter that can dynamically change of type use:\n" \ "```\n" \ From 48fd6f2f1ba78367c30ffbbd7fa4ff337c8805d7 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 16:45:21 -0300 Subject: [PATCH 16/28] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/include/rclcpp/node.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index 0e1320a9a0..c8ecf313f3 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -346,7 +346,7 @@ class Node : public std::enable_shared_from_this "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ "If you want to declare a parameter that won't change type without a default value use:\n" \ "`node->declare_parameter(name)`, where e.g. ParameterT=int64_t.\n\n" \ - "If you want to declare a parameter that can dynamically change of type use:\n" \ + "If you want to declare a parameter that can dynamically change type use:\n" \ "```\n" \ "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ "descriptor.dynamic_typing = true;\n" \ From 81deafe4cb4a840bd8b7f3ca8262145af47c27b4 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 16:58:48 -0300 Subject: [PATCH 17/28] Fix misspelling Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp | 2 +- .../rclcpp/node_interfaces/node_parameters_interface.hpp | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp index 3452c44f26..552fbc6979 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters.hpp @@ -112,7 +112,7 @@ class NodeParameters : public NodeParametersInterface # pragma warning(push) # pragma warning(disable: 4996) #endif - [[deprecated(RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPTRECATE_DECLARE)]] + [[deprecated(RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPRECATE_DECLARE)]] RCLCPP_PUBLIC const rclcpp::ParameterValue & declare_parameter(const std::string & name) override; diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index ebce3ecda9..a68d81d283 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -45,7 +45,7 @@ struct OnSetParametersCallbackHandle OnParametersSetCallbackType callback; }; -#define RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPTRECATE_DECLARE \ +#define RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPRECATE_DECLARE \ "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ "If you want to declare a parameter that won't change of type without a default value use:\n" \ "`node_params->declare_parameter(name, type)`, with e.g. type=rclcpp::PARAMETER_INTEGER.\n\n" \ @@ -70,7 +70,7 @@ class NodeParametersInterface /** * \sa rclcpp::Node::declare_parameter */ - [[deprecated(RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPTRECATE_DECLARE)]] + [[deprecated(RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPRECATE_DECLARE)]] virtual const rclcpp::ParameterValue & declare_parameter(const std::string & name) = 0; From 22c836575828eeb190e816e95219980e02487f39 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 16:59:56 -0300 Subject: [PATCH 18/28] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- .../rclcpp/node_interfaces/node_parameters_interface.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index a68d81d283..5f045127eb 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -47,7 +47,7 @@ struct OnSetParametersCallbackHandle #define RCLCPP_INTERNAL_NODE_PARAMETERS_INTERFACE_DEPRECATE_DECLARE \ "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ - "If you want to declare a parameter that won't change of type without a default value use:\n" \ + "If you want to declare a parameter that won't change type without a default value use:\n" \ "`node_params->declare_parameter(name, type)`, with e.g. type=rclcpp::PARAMETER_INTEGER.\n\n" \ "If you want to declare a parameter that can dynamically change of type use:\n" \ "```\n" \ From 93099092bef4bf259930a1868f61ff5b7923bcca Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 17:00:26 -0300 Subject: [PATCH 19/28] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- .../rclcpp/node_interfaces/node_parameters_interface.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index 5f045127eb..f397583fd9 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -49,7 +49,7 @@ struct OnSetParametersCallbackHandle "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ "If you want to declare a parameter that won't change type without a default value use:\n" \ "`node_params->declare_parameter(name, type)`, with e.g. type=rclcpp::PARAMETER_INTEGER.\n\n" \ - "If you want to declare a parameter that can dynamically change of type use:\n" \ + "If you want to declare a parameter that can dynamically change type use:\n" \ "```\n" \ "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ "descriptor.dynamic_typing = true;\n" \ From 6f39e2009fc25598df51d437588655d805673197 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 17:01:20 -0300 Subject: [PATCH 20/28] Remove vestigial comment Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/parameter_value.hpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/rclcpp/include/rclcpp/parameter_value.hpp b/rclcpp/include/rclcpp/parameter_value.hpp index 7272337f6f..77bcdec51f 100644 --- a/rclcpp/include/rclcpp/parameter_value.hpp +++ b/rclcpp/include/rclcpp/parameter_value.hpp @@ -32,8 +32,6 @@ namespace rclcpp enum ParameterType : uint8_t { PARAMETER_NOT_SET = rcl_interfaces::msg::ParameterType::PARAMETER_NOT_SET, - /// Special value, only used to declare a dynamically typed parameter. - /// Not valid as a parameter type. PARAMETER_BOOL = rcl_interfaces::msg::ParameterType::PARAMETER_BOOL, PARAMETER_INTEGER = rcl_interfaces::msg::ParameterType::PARAMETER_INTEGER, PARAMETER_DOUBLE = rcl_interfaces::msg::ParameterType::PARAMETER_DOUBLE, From 565af13b87063c8f7b04b4636932b2913cbb7ddf Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 28 Jan 2021 17:02:51 -0300 Subject: [PATCH 21/28] Move variable into scope where it is used Signed-off-by: Ivan Santiago Paunovic --- rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 4161b960f6..73e2a1f237 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -133,9 +133,9 @@ NodeParameters::NodeParameters( // If asked, initialize any parameters that ended up in the initial parameter values, // but did not get declared explcitily by this point. - rcl_interfaces::msg::ParameterDescriptor descriptor; - descriptor.dynamic_typing = true; if (automatically_declare_parameters_from_overrides) { + rcl_interfaces::msg::ParameterDescriptor descriptor; + descriptor.dynamic_typing = true; for (const auto & pair : this->get_parameter_overrides()) { if (!this->has_parameter(pair.first)) { this->declare_parameter( From 0e9be57f3cb9941fc96a606f7ada9a0481ee0627 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 18 Feb 2021 14:16:49 -0300 Subject: [PATCH 22/28] Fix test failures in rclcpp_lifecycle Signed-off-by: Ivan Santiago Paunovic --- rclcpp_lifecycle/test/test_lifecycle_node.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/rclcpp_lifecycle/test/test_lifecycle_node.cpp b/rclcpp_lifecycle/test/test_lifecycle_node.cpp index ce3809c755..1bd3032999 100644 --- a/rclcpp_lifecycle/test/test_lifecycle_node.cpp +++ b/rclcpp_lifecycle/test/test_lifecycle_node.cpp @@ -425,7 +425,9 @@ TEST_F(TestDefaultStateMachine, check_parameters) { rclcpp::exceptions::ParameterNotDeclaredException); // Default descriptor overload - test_node->declare_parameter(bool_name, rclcpp::ParameterValue(true)); + rcl_interfaces::msg::ParameterDescriptor bool_descriptor; + bool_descriptor.dynamic_typing = true; + test_node->declare_parameter(bool_name, rclcpp::ParameterValue(true), bool_descriptor); // Explicit descriptor overload rcl_interfaces::msg::ParameterDescriptor int_descriptor; From 1fd6df68999fb8b13b284688c71ef8280942f928 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 19 Feb 2021 17:49:40 -0300 Subject: [PATCH 23/28] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: William Woodall --- rclcpp/include/rclcpp/node.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index c8ecf313f3..e570b88af3 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -350,7 +350,7 @@ class Node : public std::enable_shared_from_this "```\n" \ "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ "descriptor.dynamic_typing = true;\n" \ - "node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);" \ + "node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);\n" \ "```" )]] RCLCPP_PUBLIC From 6c9871837326e5b0b64bfd35d6d18df3073bdb14 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 19 Feb 2021 17:50:18 -0300 Subject: [PATCH 24/28] style Signed-off-by: Ivan Santiago Paunovic Co-authored-by: William Woodall --- rclcpp/include/rclcpp/node_impl.hpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node_impl.hpp b/rclcpp/include/rclcpp/node_impl.hpp index 8490b251a2..8ed078113f 100644 --- a/rclcpp/include/rclcpp/node_impl.hpp +++ b/rclcpp/include/rclcpp/node_impl.hpp @@ -186,7 +186,8 @@ Node::declare_parameter( name, value.get_type(), parameter_descriptor, - ignore_override).get(); + ignore_override + ).get(); } template From 3852954fa54e328f3a17e31bad1ca7f1e954c4f7 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 19 Feb 2021 17:50:32 -0300 Subject: [PATCH 25/28] nit Signed-off-by: Ivan Santiago Paunovic Co-authored-by: William Woodall --- .../rclcpp/node_interfaces/node_parameters_interface.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp index f397583fd9..a9c054ad7f 100644 --- a/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp +++ b/rclcpp/include/rclcpp/node_interfaces/node_parameters_interface.hpp @@ -53,7 +53,7 @@ struct OnSetParametersCallbackHandle "```\n" \ "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ "descriptor.dynamic_typing = true;\n" \ - "node_params->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);" \ + "node_params->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);\n" \ "```" /// Pure virtual interface class for the NodeParameters part of the Node API. From 89597aa9155733b38d25f278c31470af367cf3b7 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 19 Feb 2021 18:20:06 -0300 Subject: [PATCH 26/28] Update lifecycle node declare parameter methods Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/node.hpp | 1 + .../rclcpp_lifecycle/lifecycle_node.hpp | 49 +++++++++++++++++-- .../rclcpp_lifecycle/lifecycle_node_impl.hpp | 24 ++++++++- rclcpp_lifecycle/src/lifecycle_node.cpp | 38 +++++++++++++- 4 files changed, 105 insertions(+), 7 deletions(-) diff --git a/rclcpp/include/rclcpp/node.hpp b/rclcpp/include/rclcpp/node.hpp index e570b88af3..38feb77b07 100644 --- a/rclcpp/include/rclcpp/node.hpp +++ b/rclcpp/include/rclcpp/node.hpp @@ -342,6 +342,7 @@ class Node : public std::enable_shared_from_this rcl_interfaces::msg::ParameterDescriptor{}, bool ignore_override = false); + /// Declare a parameter [[deprecated( "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ "If you want to declare a parameter that won't change type without a default value use:\n" \ diff --git a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp index 9856ba4bd4..4abc19ee15 100644 --- a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp +++ b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp @@ -282,9 +282,39 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface, const rclcpp::ParameterValue & declare_parameter( const std::string & name, - const rclcpp::ParameterValue & default_value = rclcpp::ParameterValue(), + const rclcpp::ParameterValue & default_value, const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = - rcl_interfaces::msg::ParameterDescriptor()); + rcl_interfaces::msg::ParameterDescriptor(), + bool ignore_override = false); + + /// Declare and initialize a parameter, return the effective value. + /** + * \sa rclcpp::Node::declare_parameter + */ + RCLCPP_PUBLIC + const rclcpp::ParameterValue & + declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor{}, + bool ignore_override = false); + + /// Declare a parameter + [[deprecated( + "declare_parameter() with only a name is deprecated and will be deleted in the future.\n" \ + "If you want to declare a parameter that won't change type without a default value use:\n" \ + "`node->declare_parameter(name)`, where e.g. ParameterT=int64_t.\n\n" \ + "If you want to declare a parameter that can dynamically change type use:\n" \ + "```\n" \ + "rcl_interfaces::msg::ParameterDescriptor descriptor;\n" \ + "descriptor.dynamic_typing = true;\n" \ + "node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);\n" \ + "```" + )]] + RCLCPP_PUBLIC + const rclcpp::ParameterValue & + declare_parameter(const std::string & name); /// Declare and initialize a parameter with a type. /** @@ -296,7 +326,20 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface, const std::string & name, const ParameterT & default_value, const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = - rcl_interfaces::msg::ParameterDescriptor()); + rcl_interfaces::msg::ParameterDescriptor(), + bool ignore_override = false); + + /// Declare and initialize a parameter with a type. + /** + * See the non-templated declare_parameter() on this class for details. + */ + template + auto + declare_parameter( + const std::string & name, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor = + rcl_interfaces::msg::ParameterDescriptor(), + bool ignore_override = false); /// Declare and initialize several parameters with the same namespace and type. /** diff --git a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp index be79e7944a..5582d8aca6 100644 --- a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp +++ b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node_impl.hpp @@ -135,12 +135,32 @@ auto LifecycleNode::declare_parameter( const std::string & name, const ParameterT & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor) + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) { return this->declare_parameter( name, rclcpp::ParameterValue(default_value), - parameter_descriptor + parameter_descriptor, + ignore_override + ).get(); +} + +template +auto +LifecycleNode::declare_parameter( + const std::string & name, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) +{ + // get advantage of parameter value template magic to get + // the correct rclcpp::ParameterType from ParameterT + rclcpp::ParameterValue value{ParameterT{}}; + return this->declare_parameter( + name, + value.get_type(), + parameter_descriptor, + ignore_override ).get(); } diff --git a/rclcpp_lifecycle/src/lifecycle_node.cpp b/rclcpp_lifecycle/src/lifecycle_node.cpp index 4da9502669..a82878e8ca 100644 --- a/rclcpp_lifecycle/src/lifecycle_node.cpp +++ b/rclcpp_lifecycle/src/lifecycle_node.cpp @@ -159,9 +159,43 @@ const rclcpp::ParameterValue & LifecycleNode::declare_parameter( const std::string & name, const rclcpp::ParameterValue & default_value, - const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor) + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) { - return this->node_parameters_->declare_parameter(name, default_value, parameter_descriptor); + return this->node_parameters_->declare_parameter( + name, default_value, parameter_descriptor, ignore_override); +} + +const rclcpp::ParameterValue & +LifecycleNode::declare_parameter(const std::string & name) +{ +#ifndef _WIN32 +# pragma GCC diagnostic push +# pragma GCC diagnostic ignored "-Wdeprecated-declarations" +#else +# pragma warning(push) +# pragma warning(disable: 4996) +#endif + return this->node_parameters_->declare_parameter(name); +#ifndef _WIN32 +# pragma GCC diagnostic pop +#else +# pragma warning(pop) +#endif +} + +const rclcpp::ParameterValue & +LifecycleNode::declare_parameter( + const std::string & name, + rclcpp::ParameterType type, + const rcl_interfaces::msg::ParameterDescriptor & parameter_descriptor, + bool ignore_override) +{ + return this->node_parameters_->declare_parameter( + name, + type, + parameter_descriptor, + ignore_override); } void From 89a734e1d664ec0b2c5043fc997f126d43256acf Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 22 Feb 2021 10:20:41 -0300 Subject: [PATCH 27/28] Specific exception when no parameter override is provided and one was required Signed-off-by: Ivan Santiago Paunovic --- rclcpp/include/rclcpp/exceptions/exceptions.hpp | 14 ++++++++++++++ .../src/rclcpp/node_interfaces/node_parameters.cpp | 3 +++ rclcpp/test/rclcpp/test_node.cpp | 2 +- 3 files changed, 18 insertions(+), 1 deletion(-) diff --git a/rclcpp/include/rclcpp/exceptions/exceptions.hpp b/rclcpp/include/rclcpp/exceptions/exceptions.hpp index 630e2d846f..a0c711290c 100644 --- a/rclcpp/include/rclcpp/exceptions/exceptions.hpp +++ b/rclcpp/include/rclcpp/exceptions/exceptions.hpp @@ -282,6 +282,20 @@ class ParameterModifiedInCallbackException : public std::runtime_error using std::runtime_error::runtime_error; }; +/// Thrown when a parameter override wasn't provided and one was required. +class NoParameterOverrideProvided : public std::runtime_error +{ +public: + /// Construct an instance. + /** + * \param[in] name the name of the parameter. + * \param[in] message custom exception message. + */ + explicit NoParameterOverrideProvided(const std::string & name) + : std::runtime_error("parameter '" + name + "' requires an user provided parameter override") + {} +}; + /// Thrown if the QoS overrides provided aren't valid. class InvalidQosOverridesException : public std::runtime_error { diff --git a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp index 73e2a1f237..fd6943a64a 100644 --- a/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp +++ b/rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp @@ -461,6 +461,9 @@ 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( diff --git a/rclcpp/test/rclcpp/test_node.cpp b/rclcpp/test/rclcpp/test_node.cpp index 090bff3e64..b884181a3a 100644 --- a/rclcpp/test/rclcpp/test_node.cpp +++ b/rclcpp/test/rclcpp/test_node.cpp @@ -2702,7 +2702,7 @@ TEST_F(TestNode, static_and_dynamic_typing) { { EXPECT_THROW( node->declare_parameter("integer_override_not_given", rclcpp::PARAMETER_INTEGER), - rclcpp::exceptions::InvalidParameterTypeException); + rclcpp::exceptions::NoParameterOverrideProvided); } { EXPECT_THROW( From 8f0e6382ab1c0b58928072a6a08b841eaa41de85 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 22 Feb 2021 13:06:23 -0300 Subject: [PATCH 28/28] fix windows Signed-off-by: Ivan Santiago Paunovic --- rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp index 4abc19ee15..8289187cce 100644 --- a/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp +++ b/rclcpp_lifecycle/include/rclcpp_lifecycle/lifecycle_node.hpp @@ -291,7 +291,7 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface, /** * \sa rclcpp::Node::declare_parameter */ - RCLCPP_PUBLIC + RCLCPP_LIFECYCLE_PUBLIC const rclcpp::ParameterValue & declare_parameter( const std::string & name, @@ -312,7 +312,7 @@ class LifecycleNode : public node_interfaces::LifecycleNodeInterface, "node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor);\n" \ "```" )]] - RCLCPP_PUBLIC + RCLCPP_LIFECYCLE_PUBLIC const rclcpp::ParameterValue & declare_parameter(const std::string & name);