Add ParameterDescriptor override from yaml #909

bpwilcox · 2019-10-30T00:55:42Z

This PR (built on top of required PR ros2/rcl#533) allows for parameter descriptions to be parsed from the yaml parameter file (proposed syntax described in above PR link) into the parameter_overrides when declaring a parameter via declare_parameter. This enables users to describe the parameter constraints at run-time to fill a ParameterDescriptor.msg for parameter validation and documentation. As mentioned in #807 (comment), this feature may be particularly useful for a "parameter blackboard" or for dynamic parameter tuning.

Matching modifications can be made to enable this feature for rclpy.

wjwwood · 2019-11-15T23:08:37Z

@hidmic can you please take a look at these pr's?

hidmic

First pass, though most comments that apply to ros2/rcl#533 (review) will likely apply to this patch.

Also, rclpy must be updated as well (but hold on until we've settled on these PRs first).

hidmic · 2019-11-21T15:20:00Z

rclcpp/include/rclcpp/parameter_map.hpp

 /// A map of fully qualified node names to a list of parameters
-using ParameterMap = std::unordered_map<std::string, std::vector<Parameter>>;
+using rcl_interfaces::msg::ParameterDescriptor;
+using ParameterAndDescriptor = std::unordered_map<std::string, std::pair<Parameter,


@bpwilcox std::pair is convenient but obscure when reading code (e.g. what is param.second.second??). Consider defining a struct instead.

I think it may be even easier to directly create the ParameterInfo struct here.

rclcpp/src/rclcpp/parameter_map.cpp

hidmic · 2019-11-21T15:33:22Z

rclcpp/src/rclcpp/parameter_map.cpp

+      const char * const c_param_name = c_param_descriptors_node->parameter_names[p];
+      if (NULL == c_param_name) {
+        std::string message(
+          "At node " + std::to_string(n) + " parameter " + std::to_string(p) + " name is NULL");


@bpwilcox do we need an auxiliary variable?

The message string? Probably not needed, this was left from the previous code, not my additions.

bpwilcox · 2019-11-23T00:18:59Z

@hidmic Thanks for the first pass reviews! In regard to changes to rclpy, I do have have a local branch with modifications that I can update for a PR soon after this one.

hidmic · 2021-02-12T21:36:18Z

@bpwilcox do you still plan on submitting changes? Or shall I close this patch?

bpwilcox · 2021-02-19T21:47:15Z

@bpwilcox do you still plan on submitting changes? Or shall I close this patch?

Yes, I believe I should have some more cycles to work on this in the near future.

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

minor cleanup and lint fix Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

Signed-off-by: Brian Wilcox <[email protected]>

… unit test Signed-off-by: Brian Wilcox <[email protected]>

alsora · 2021-08-09T13:07:18Z

FYI this PR has been rebased to work with Galactic and it's ready for review, together with ros2/rcl#533

hidmic · 2021-08-18T16:03:51Z

rclcpp/include/rclcpp/parameter_map.hpp

 ParameterMap
 parameter_map_from_yaml_file(const std::string & yaml_filename);

+


@bpwilcox nit: drop extra blank line

hidmic · 2021-08-18T16:06:15Z

rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

  // but did not get declared explcitily by this point.
  if (automatically_declare_parameters_from_overrides) {
-    rcl_interfaces::msg::ParameterDescriptor descriptor;
-    descriptor.dynamic_typing = true;


@bpwilcox why the change?

I've reverted this change, though I am still open to discussing the most appropriate behavior for dynamic_tuning field being set from a yaml file. For example, if you have a blackboard node and wanted to have a parameter become static typing, this change would overwrite. Before allowing yaml descriptor overrides, I'd agree it makes more sense to have parameters be dynamic typing when automatically declared, but now with this feature, I'm unsure if this should be the case.

hidmic · 2021-08-18T16:17:01Z

rclcpp/src/rclcpp/node_interfaces/node_parameters.cpp

+    auto has_parameter_override = false;
+    auto has_descriptor_override = false;
+
+    if (overrides_it->second.value.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET) {
+      has_parameter_override = true;
+    }
+    if (overrides_it->second.descriptor.name != "") {
+      has_descriptor_override = true;
+    }
+
+    if (has_parameter_override && has_descriptor_override) {
+      initial_value = &overrides_it->second.value;
+      parameter_infos.at(name).descriptor = overrides_it->second.descriptor;
+    } else if (has_parameter_override) {
+      initial_value = &overrides_it->second.value;
+    } else if (has_descriptor_override) {
+      parameter_infos.at(name).descriptor = overrides_it->second.descriptor;
+    }


@bpwilcox isn't this equivalent to:

Suggested change

auto has_parameter_override = false;

auto has_descriptor_override = false;

if (overrides_it->second.value.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET) {

has_parameter_override = true;

}

if (overrides_it->second.descriptor.name != "") {

has_descriptor_override = true;

}

if (has_parameter_override && has_descriptor_override) {

initial_value = &overrides_it->second.value;

parameter_infos.at(name).descriptor = overrides_it->second.descriptor;

} else if (has_parameter_override) {

initial_value = &overrides_it->second.value;

} else if (has_descriptor_override) {

parameter_infos.at(name).descriptor = overrides_it->second.descriptor;

}

if (overrides_it->second.value.get_type() != rclcpp::ParameterType::PARAMETER_NOT_SET) {

initial_value = &overrides_it->second.value;

}

if (overrides_it->second.descriptor.name != "") {

parameter_infos[name].descriptor = overrides_it->second.descriptor;

}

hidmic · 2021-08-18T16:20:46Z

rclcpp/test/rclcpp/test_parameter_map.cpp

+  }
+  // zero init parameter value
+  c_node_param_descriptors->parameter_descriptors =
+    static_cast<rcl_param_descriptor_t *>(alloc.allocate(


@bpwilcox nit: using zero_allocate() would have the same net effect.

I agree but is there any benefit of changing?

hidmic · 2021-08-18T16:22:45Z

rclcpp/test/rclcpp/test_parameter_map.cpp

+  EXPECT_TRUE(params.count("bar"));
+  EXPECT_STREQ("Integer Range Descriptor", params.at("bar").descriptor.description.c_str());
+  EXPECT_STREQ("Even numbers only", params.at("bar").descriptor.additional_constraints.c_str());
+  EXPECT_EQ(-1234, params.at("bar").descriptor.integer_range[0].from_value);


@bpwilcox can't we use min_value in place for the literal? Same in all other cases below.

hidmic · 2021-08-18T16:24:50Z

rclcpp/include/rclcpp/parameter.hpp

+
+  /// A description of the parameter
+  rcl_interfaces::msg::ParameterDescriptor descriptor;
+};


@bpwilcox meta: hmm, why not pushing this descriptor attribute into rclcpp::Parameter? Then we can use rclcpp::Parameter everywhere.

I do agree that you could put the descriptor in the rclcpp::Parameter object, however that seems to be a more far-reaching change than this PR, especially since many user-code uses rclcpp::Parameter. I think it is a good candidate for a follow-up PR.

Signed-off-by: Brian <[email protected]>

bpwilcox · 2022-01-31T20:40:32Z

@hidmic Could you take another pass through this review since latest changes?

hidmic · 2022-01-31T23:25:57Z

@bpwilcox yes! I'll take a look at these tomorrow morning first thing. Sorry for the insane delay, I've been too busy.

) Signed-off-by: Simon Honigmann <[email protected]> Co-authored-by: Simon Honigmann <[email protected]>

bpwilcox force-pushed the add_parameter_descriptors_from_yaml branch from fbde81f to 532f273 Compare October 30, 2019 00:57

wjwwood added the enhancement New feature or request label Nov 14, 2019

wjwwood requested a review from hidmic November 15, 2019 23:08

wjwwood assigned hidmic Nov 15, 2019

hidmic reviewed Nov 21, 2019

View reviewed changes

bpwilcox mentioned this pull request Mar 29, 2021

Parse parameter descriptors in yaml ros2/rcl#533

Open

bpwilcox force-pushed the add_parameter_descriptors_from_yaml branch from 532f273 to b8d89d3 Compare June 29, 2021 16:44

bpwilcox and others added 9 commits June 29, 2021 16:46

convert c struct parameter descriptor to ParamDescriptor message

702cf7f

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

return map of parameter and parameter descriptors

22b897a

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

declare parameter descriptors from overrides

aaa8f73

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

only add integer range or floating point range

8965a81

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

default max/min range if either not provided

2727fdc

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

throw error if mixed int/double descriptor range types

e59690b

minor cleanup and lint fix Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

modify parameter map tests for descriptors and add test cases

f66c785

Signed-off-by: bpwilcox <[email protected]> Signed-off-by: Brian Wilcox <[email protected]>

fix rebase compile issues

e6546c8

Signed-off-by: Brian Wilcox <[email protected]>

move ParameterInfo to parameters.hpp and use in parameter map, update…

070abdd

… unit test Signed-off-by: Brian Wilcox <[email protected]>

bpwilcox force-pushed the add_parameter_descriptors_from_yaml branch from b8d89d3 to 070abdd Compare June 29, 2021 16:47

hidmic self-requested a review August 9, 2021 13:17

hidmic reviewed Aug 18, 2021

View reviewed changes

address feedback and update for rcl yaml param parser changes

ff1b8e5

Signed-off-by: Brian <[email protected]>

bpwilcox force-pushed the add_parameter_descriptors_from_yaml branch from 29be629 to ff1b8e5 Compare December 12, 2021 12:59

hidmic self-requested a review January 31, 2022 23:25

clalancette changed the base branch from master to rolling June 28, 2022 14:28

nnmm pushed a commit to ApexAI/rclcpp that referenced this pull request Jul 9, 2022

updating quality declaration links (re: ros2/docs.ros2.org#52) (ros2#909

f2e8d1c

) Signed-off-by: Simon Honigmann <[email protected]> Co-authored-by: Simon Honigmann <[email protected]>

		ParameterMap
		parameter_map_from_yaml_file(const std::string & yaml_filename);

Add ParameterDescriptor override from yaml #909

Are you sure you want to change the base?

Add ParameterDescriptor override from yaml #909

Uh oh!

Conversation

bpwilcox commented Oct 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wjwwood commented Nov 15, 2019

Uh oh!

hidmic left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hidmic Nov 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpwilcox commented Nov 23, 2019

Uh oh!

hidmic commented Feb 12, 2021

Uh oh!

bpwilcox commented Feb 19, 2021

Uh oh!

alsora commented Aug 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bpwilcox commented Jan 31, 2022

Uh oh!

hidmic commented Jan 31, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bpwilcox commented Oct 30, 2019 •

edited

Loading

hidmic left a comment •

edited

Loading

hidmic Nov 21, 2019 •

edited

Loading