From 2987e6363e1beec39c739ac80494dd7c67abd5f8 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Wed, 3 Mar 2021 14:04:20 -0300 Subject: [PATCH 01/10] Document design decisions that were made for statically typed parameters Signed-off-by: Ivan Santiago Paunovic --- .../notes_on_statically_typed_parameters.md | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 rclcpp/doc/notes_on_statically_typed_parameters.md diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md new file mode 100644 index 0000000000..fc976ab48d --- /dev/null +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -0,0 +1,111 @@ +# Notes on statically typed parameters + +## Introduction + +Up to ROS 2 Foxy, all parameters could be changed of type anytime, except the user installed a parameter callback to reject that change. +This could generate confusing errors, for example: + +``` +$ ros2 run demo_nodes_py listener & +$ ros2 param set /listener use_sim_time not_a_boolean +[ERROR] [1614712713.233733147] [listener]: use_sim_time parameter set to something besides a bool +Set parameter successful +$ ros2 param get /listener use_sim_time +String value is: not_a_boolean +``` + +For most use cases, having a static parameter types is enough. +This notes document some of the decisions that were made when implementing static parameter types enforcement in: + +* https://github.com/ros2/rclcpp/pull/1522 +* https://github.com/ros2/rclpy/pull/683 + +## Allowing dynamically typed parameters + +There might be some scenarios were dynamic typing is desired, so a `dynamic_typing` field was added to the [parameter descriptor](https://github.com/ros2/rcl_interfaces/blob/09b5ed93a733adb9deddc47f9a4a8c6e9f584667/rcl_interfaces/msg/ParameterDescriptor.msg#L25). +It defaults to `false`. + +For example: + +```cpp +rcl_interfaces::msg::ParameterDescriptor descriptor; +descriptor.dynamic_typing = true; + +node->declare_parameter("dynamically_typed_parameter", rclcpp::ParameterValue{}, descriptor); +``` + +```py +rcl_interfaces.msg.ParameterDescriptor descriptor; +descriptor.dynamic_typing = True; + +node.declare_parameter("dynamically_typed_parameter", None, descriptor); +``` + +## How is the parameter type specified? + +The parameter type will be inferred from the default value provided when declaring it. + +## Statically typed parameters when allowing undeclared parameters + +When undeclared parameters are allowed, the descriptor of undeclared parameters will have the dynamic typing field set. +That's because in that case there is no declaration, and it doesn't make much sense to enforce the type of the first value set. + +TBD: Should parameters that were declared with the dynamic typing option off enforce that the parameter doesn't change the type even if allow undeclared parameters is set? +In the current implementation that isn't enforce, but it might make sense to do it for parameters that were declared. + +## Declaring a parameter without a default value + +There might be cases were a default value does not make sense and the user must always provide an override. +In those cases, the parameter type can be specified explicetly: + +```cpp +// method signature +template +Node::declare_parameter(std::string name, rcl_interfaces::msg::ParameterDescriptor = rcl_interfaces::msg::ParameterDescriptor{}); +// or alternatively +Node::declare_parameter(std::string name, rclcpp::ParameterType type, rcl_interfaces::msg::ParameterDescriptor = rcl_interfaces::msg::ParameterDescriptor{}); + +// examples +node->declare_paramter("my_integer_parameter"); // declare an integer parameter +node->declare_paramter("another_integer_parameter", rclcpp::ParameterType::PARAMETER_INTEGER); // another way to do the same +``` + +```py +# method signature +Node.declare_parameter(name: str, param_type: rclpy.Parameter.Type, descriptor: rcl_interfaces.msg.ParameterDescriptor = rcl_interfaces.msg.ParameterDescriptor()) + +# example +node.declare_paramter('my_integer_parameter', rclpy.Parameter.Type.INTEGER); # declare an integer parameter +``` + +If the parameter is optional, e.g.: only needed depending on the value of another parameter, the recommended approach is to conditionally declare the parameter: + +```cpp +auto mode = node->declare_parameter("mode", "modeA"); // "mode" parameter is an string +if (mode == "modeB") { + node->declare_parameter("param_needed_when_mode_b"); // when "modeB", the user must provide "param_needed_when_mode_b" +} +``` + +## Other migration notes + +Declaring a parameter with only a name is deprecated: + +```cpp +node->declare_parameter("my_param"); // this generates a build warning +``` + +```py +node.declare_parameter("my_param"); # this generates a python user warning +``` + +Before, you could initialize a parameter with the "NOT SET" value (in cpp `rclcpp::ParameterValue{}`, in python `None`). +Now this will throw an exception in both cases: + +```cpp +node->declare_parameter("my_param", rclcpp::ParameterValue{}); // not valid, will throw exception +``` + +```py +node.declare_parameter("my_param", None); # not valid, will raise error +``` From 80624f061f4844ee304052bbc867cda5914c822a Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 5 Mar 2021 10:00:15 -0300 Subject: [PATCH 02/10] fix typo Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Tomoya Fujita --- rclcpp/doc/notes_on_statically_typed_parameters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index fc976ab48d..01d94c6ab3 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -56,7 +56,7 @@ In the current implementation that isn't enforce, but it might make sense to do ## Declaring a parameter without a default value There might be cases were a default value does not make sense and the user must always provide an override. -In those cases, the parameter type can be specified explicetly: +In those cases, the parameter type can be specified explicitly: ```cpp // method signature From f245d42a871e4f33bc075cfd9f7d8994c05476c8 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 8 Mar 2021 13:52:52 -0300 Subject: [PATCH 03/10] Address peer review comments Signed-off-by: Ivan Santiago Paunovic --- rclcpp/doc/notes_on_statically_typed_parameters.md | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index 01d94c6ab3..72314aa606 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -47,11 +47,8 @@ The parameter type will be inferred from the default value provided when declari ## Statically typed parameters when allowing undeclared parameters -When undeclared parameters are allowed, the descriptor of undeclared parameters will have the dynamic typing field set. -That's because in that case there is no declaration, and it doesn't make much sense to enforce the type of the first value set. - -TBD: Should parameters that were declared with the dynamic typing option off enforce that the parameter doesn't change the type even if allow undeclared parameters is set? -In the current implementation that isn't enforce, but it might make sense to do it for parameters that were declared. +When undeclared parameters are allowed and a parameter is set without a previous declaration, the parameter will be dynamically typed. This is consistent with other allowed behaviors when undeclared parameters are allowed, e.g. trying to get an undeclared parameter returns "NOT_SET". +Parameter declarations will remain special and dynamic or static typing will be used based on the parameter descriptor (default to static). ## Declaring a parameter without a default value @@ -78,7 +75,7 @@ Node.declare_parameter(name: str, param_type: rclpy.Parameter.Type, descriptor: node.declare_paramter('my_integer_parameter', rclpy.Parameter.Type.INTEGER); # declare an integer parameter ``` -If the parameter is optional, e.g.: only needed depending on the value of another parameter, the recommended approach is to conditionally declare the parameter: +If the parameter may be unused, but when used requires a parameter override, then you could conditionally declare it: ```cpp auto mode = node->declare_parameter("mode", "modeA"); // "mode" parameter is an string From c789228d4563406aa4400216fdbd9a44638e556f Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 8 Mar 2021 15:56:54 -0300 Subject: [PATCH 04/10] Improve sentence Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/doc/notes_on_statically_typed_parameters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index 72314aa606..f8539a9ce5 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -2,7 +2,7 @@ ## Introduction -Up to ROS 2 Foxy, all parameters could be changed of type anytime, except the user installed a parameter callback to reject that change. +Until ROS 2 Foxy, all parameters could change type anytime, except if the user installed a parameter callback to reject a change. This could generate confusing errors, for example: ``` From 4167e5c0e62745b23fbee83c9b10ddcdea6e7611 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 8 Mar 2021 15:57:09 -0300 Subject: [PATCH 05/10] grammar Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/doc/notes_on_statically_typed_parameters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index f8539a9ce5..baa2711080 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -14,7 +14,7 @@ $ ros2 param get /listener use_sim_time String value is: not_a_boolean ``` -For most use cases, having a static parameter types is enough. +For most use cases, having static parameter types is enough. This notes document some of the decisions that were made when implementing static parameter types enforcement in: * https://github.com/ros2/rclcpp/pull/1522 From 75691e5ea59433d332feedeb2cf3bb21aadedf60 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 8 Mar 2021 15:57:26 -0300 Subject: [PATCH 06/10] wording Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/doc/notes_on_statically_typed_parameters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index baa2711080..ea3d669307 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -15,7 +15,7 @@ String value is: not_a_boolean ``` For most use cases, having static parameter types is enough. -This notes document some of the decisions that were made when implementing static parameter types enforcement in: +This article documents some of the decisions that were made when implementing static parameter types enforcement in: * https://github.com/ros2/rclcpp/pull/1522 * https://github.com/ros2/rclpy/pull/683 From f78327aec399e0abbf6e01c491177532df478a5d Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Mon, 8 Mar 2021 15:57:39 -0300 Subject: [PATCH 07/10] typo Signed-off-by: Ivan Santiago Paunovic Co-authored-by: Jacob Perron --- rclcpp/doc/notes_on_statically_typed_parameters.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index ea3d669307..e6d5dbb509 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -22,7 +22,7 @@ This article documents some of the decisions that were made when implementing st ## Allowing dynamically typed parameters -There might be some scenarios were dynamic typing is desired, so a `dynamic_typing` field was added to the [parameter descriptor](https://github.com/ros2/rcl_interfaces/blob/09b5ed93a733adb9deddc47f9a4a8c6e9f584667/rcl_interfaces/msg/ParameterDescriptor.msg#L25). +There might be some scenarios where dynamic typing is desired, so a `dynamic_typing` field was added to the [parameter descriptor](https://github.com/ros2/rcl_interfaces/blob/09b5ed93a733adb9deddc47f9a4a8c6e9f584667/rcl_interfaces/msg/ParameterDescriptor.msg#L25). It defaults to `false`. For example: From 13fc4c8f8b4ace832b911a3f8d93fc6b884f7b4d Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 12 Mar 2021 16:40:11 -0300 Subject: [PATCH 08/10] Correct typo Signed-off-by: Ivan Santiago Paunovic --- rclcpp/doc/notes_on_statically_typed_parameters.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index e6d5dbb509..12edfebc98 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -63,8 +63,8 @@ Node::declare_parameter(std::string name, rcl_interfaces::msg::ParameterDescr Node::declare_parameter(std::string name, rclcpp::ParameterType type, rcl_interfaces::msg::ParameterDescriptor = rcl_interfaces::msg::ParameterDescriptor{}); // examples -node->declare_paramter("my_integer_parameter"); // declare an integer parameter -node->declare_paramter("another_integer_parameter", rclcpp::ParameterType::PARAMETER_INTEGER); // another way to do the same +node->declare_parameter("my_integer_parameter"); // declare an integer parameter +node->declare_parameter("another_integer_parameter", rclcpp::ParameterType::PARAMETER_INTEGER); // another way to do the same ``` ```py @@ -72,7 +72,7 @@ node->declare_paramter("another_integer_parameter", rclcpp::ParameterType::PARAM Node.declare_parameter(name: str, param_type: rclpy.Parameter.Type, descriptor: rcl_interfaces.msg.ParameterDescriptor = rcl_interfaces.msg.ParameterDescriptor()) # example -node.declare_paramter('my_integer_parameter', rclpy.Parameter.Type.INTEGER); # declare an integer parameter +node.declare_parameter('my_integer_parameter', rclpy.Parameter.Type.INTEGER); # declare an integer parameter ``` If the parameter may be unused, but when used requires a parameter override, then you could conditionally declare it: From 46a773e5640648b7ffae96824233ea6f1c13e415 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Fri, 12 Mar 2021 17:32:04 -0300 Subject: [PATCH 09/10] Document possible improvement Signed-off-by: Ivan Santiago Paunovic --- .../notes_on_statically_typed_parameters.md | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index 12edfebc98..db55615e5c 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -106,3 +106,35 @@ node->declare_parameter("my_param", rclcpp::ParameterValue{}); // not valid, wi ```py node.declare_parameter("my_param", None); # not valid, will raise error ``` + +## Possible improvements + +### Easier way to declare dynamically typed parameters + +Declaring a dynamically typed parameter in `rclcpp` could be considered to be a bit verbose: + +```cpp +rcl_interfaces::msg::ParameterDescriptor descriptor; +descriptor.dynamic_typing = true; + +node->declare_parameter(name, rclcpp::ParameterValue{}, descriptor); +``` + +the following ways could be supported to make it simpler: + +```cpp +node->declare_parameter(name, rclcpp::PARAMETER_DYNAMIC); +node->declare_parameter(name, default_value, rclcpp::PARAMETER_DYNAMIC); +``` + +or alternatively: + +```cpp +node->declare_parameter(name, default_value, rclcpp::ParameterDescriptor{}.dynamic_typing()); +``` + +For `rclpy`, there's already a short way to do it: + +```py +node.declare_parameter(name, default_value, rclpy.ParameterDescriptor(dynamic_typing=true)); +``` From 2576bc0b5a15e5547bb87152b6c2f115dfeccc43 Mon Sep 17 00:00:00 2001 From: Ivan Santiago Paunovic Date: Thu, 18 Mar 2021 10:56:41 -0300 Subject: [PATCH 10/10] One sentence per line Signed-off-by: Ivan Santiago Paunovic --- rclcpp/doc/notes_on_statically_typed_parameters.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rclcpp/doc/notes_on_statically_typed_parameters.md b/rclcpp/doc/notes_on_statically_typed_parameters.md index db55615e5c..b96866c639 100644 --- a/rclcpp/doc/notes_on_statically_typed_parameters.md +++ b/rclcpp/doc/notes_on_statically_typed_parameters.md @@ -47,7 +47,8 @@ The parameter type will be inferred from the default value provided when declari ## Statically typed parameters when allowing undeclared parameters -When undeclared parameters are allowed and a parameter is set without a previous declaration, the parameter will be dynamically typed. This is consistent with other allowed behaviors when undeclared parameters are allowed, e.g. trying to get an undeclared parameter returns "NOT_SET". +When undeclared parameters are allowed and a parameter is set without a previous declaration, the parameter will be dynamically typed. +This is consistent with other allowed behaviors when undeclared parameters are allowed, e.g. trying to get an undeclared parameter returns "NOT_SET". Parameter declarations will remain special and dynamic or static typing will be used based on the parameter descriptor (default to static). ## Declaring a parameter without a default value