[Draft]Add structured parameter support#2944
Conversation
Signed-off-by: Rahul K.A <karahul209@gmail.com>
Signed-off-by: Rahul K.A <karahul209@gmail.com>
Signed-off-by: Rahul K.A <karahul209@gmail.com>
Signed-off-by: Rahul K.A <karahul209@gmail.com>
Signed-off-by: Rahul K.A <karahul209@gmail.com>
Signed-off-by: Rahul K.A <karahul209@gmail.com>
|
Tagging this as a draft, because Im yet to add in tests/ apply linter. Once done,will remove the draft tag. Maintainers are welcome to start reviewing it in the meantime |
|
Tagging this as a draft, because Im yet to add in tests/ apply linter. Once done,will remove the draft tag. Maintainers are welcome to start reviewing it in the meantime |
ahcorde
left a comment
There was a problem hiding this comment.
uncrustify is failing. You might need to run
ament_uncrustify --reformat <filenames or directory>| as_yaml() const; | ||
|
|
||
|
|
There was a problem hiding this comment.
| as_yaml() const; | |
| as_yaml() const; | |
| explicit ParameterValue(const std::vector<std::string> & string_array_value); | ||
|
|
||
|
|
||
| /// Generate a parameter value with type PARAMETER_YAML |
There was a problem hiding this comment.
| explicit ParameterValue(const std::vector<std::string> & string_array_value); | |
| /// Generate a parameter value with type PARAMETER_YAML | |
| explicit ParameterValue(const std::vector<std::string> & string_array_value); | |
| /// Generate a parameter value with type PARAMETER_YAML |
|
|
||
| template<ParameterType type> | ||
| constexpr | ||
| typename std::enable_if<type == ParameterType::PARAMETER_YAML, const YAML::Node>::type |
| <package format="2"> | ||
| <name>rclcpp</name> | ||
| <version>30.1.0</version> | ||
| <version>30.0.0</version> |
There was a problem hiding this comment.
don't change this number manually. Restore
| } | ||
|
|
||
|
|
| #include <string> | ||
| #include <utility> | ||
| #include <vector> | ||
| #include "yaml-cpp/yaml.h" |
There was a problem hiding this comment.
move above <rcl_yaml_param_parser/parser.h> and use <>
| } | ||
|
|
||
|
|
||
| RCLCPP_LOCAL |
There was a problem hiding this comment.
| } | |
| RCLCPP_LOCAL | |
| } | |
| RCLCPP_LOCAL |
|
|
||
| RCLCPP_LOCAL | ||
| /// Pulled from Question 78243576 from Stack Overflow | ||
| void __traverse_node_and_change_value(YAML::Node & root, const std::vector<std::string>& path, rclcpp::ParameterValue value) |
|
hi, exposing third party library API (yaml-cpp) as a part of ROS api is not a good idea. |
|
Hi @asherikov , yaml-cpp is used by ROS2 on all platforms, as per my knowledge, since it's a dependency of rclcpp itself (yaml_cpp_vendor is just a wrapper). In that sense I do not consider it foreign to the ROS ecoyststem |
|
API dependency has more implications, e.g., a change in yaml-cpp API would imply a change in ROS2 public API affecting all user code. |
| if (value_.type != rcl_interfaces::msg::ParameterType::PARAMETER_YAML) { | ||
| throw ParameterTypeException(ParameterType::PARAMETER_YAML, get_type()); | ||
| } | ||
| return YAML::Load(value_.yaml_value); |
There was a problem hiding this comment.
This step may throw an exception during YAML parsing, so it’s best to handle the exception here and output an error log.
| std::vector<std::string> __split_parameter_name(const std::string & name) | ||
| { | ||
| constexpr char delimiter = '.'; | ||
| std::vector<std::string> ret; | ||
| std::string out; | ||
| size_t position_found = -1; | ||
| std::string evaluation_string = name; | ||
| int counter = 0; | ||
| // Set an upper limit for the loop | ||
| // Anyone having more than 100 layers of namespacing should really evaluate their code - Rahul-K-A | ||
| while(counter++ < 100) | ||
| { | ||
| position_found = evaluation_string.find(delimiter); | ||
| if (std::string::npos == position_found) | ||
| { | ||
| break; | ||
| } | ||
| ret.push_back( evaluation_string.substr(0, position_found )); | ||
| evaluation_string = evaluation_string.substr(position_found + 1); | ||
| } | ||
| ret.push_back(evaluation_string); | ||
| return ret; | ||
| } |
There was a problem hiding this comment.
I don't think it's necessary to limit the number of layers.
In addition, you can use string_view to optimize the code.
namespace
{
std::vector<std::string> split_parameter_name(const std::string& name)
{
std::vector<std::string> result;
std::string_view str_v(name);
size_t pos = 0;
while ((pos = str_v.find('.')) != std::string_view::npos) {
result.emplace_back(str_v.substr(0, pos));
str_v.remove_prefix(pos + 1);
}
result.emplace_back(str_v);
return result;
}
}|
Cross posting here from #Lyrical Luth Release Working Group > Structured Parameters @ 💬 For visibility: There's definitely some challenges with the potential duality of this. The parameter types are very limited so we can represent any parameters as a structure, but going the other way we can't set any structure into a parameter. I don't think that we want to enable this sort of dual access. It especially creates a lot of complexity with respect to atomic updates and being able to accept or reject parameter value changes. At it's core parameters are key value pairs. We use a structure for the keys for convenience, but it's not structurally meaningful in the current implementation. Looking at the PR it's adding a new paraemter type which is YAML. This is another approach, it would be that there's a large blob in there which is a YAML blob. And we won't be able to provide general introspection into what's encoded in the YAML. We can probably provide a generic yaml markdown/rendering in a text box in most cases. But we can't have a tool plotting a value over time or provide a slider with bounds on it because it's arbitrary data of any type. An example of this that we've done in the past is embedding URDFs in the robot_description parameter. It's "convenient" but it's also opaque and doesn't really provide much benefit of being a parameter. It also doesn't get logged, and we do now have potential callbacks on parameter changes. You only get a global update callback not one per sub field. And for most dynamic cases we're generally moving to sending the robot description over a topic now. I think that there's some level that we can support better "native" structured supports for convenience of setting and gettting parameters from the CLI as called out. But I'm not sure that we want to enable adding arbitrary structures into parameters. We have a good way to transport structure data in messages on topics that gives us introspection etc. And we also can't do any prevalidation of arbitrary yaml code. It can only be checked as the right type at runtime by whatever is parsing it. The other proposal in the past has been to allow parameters to have Message Datatypes. Which is nice that we've already got a lot of capabilities for parsing, rendering and checking. But it also gets into why not just send it as a message on a topic? |
Description
Adds additional API that allows the user to access structured YAML parameters as a
YAML::Node(part of yaml-cpp inrclcpp. Original GSOC proposal can be found hereEssentially, given a parameter YAML file such as
In the code, the user can declare a structured yaml parameter as
And get access to all the elements under
passenger_sizeasThey can then access individual fields using the standard
dict()syntaxUsers can also now set yaml parameters through the
ros2CLI. The syntax is very similar to the one already used for setting messages. For example:will change passenger size as
Fixes # (issue)
Is this user-facing behavior change?
Yes, it adds an additional parameter type that they can use. None of the existing parameter types are affected. Support for existing namespaced parameters is not affected. If required users will still be able to access individual namespaced parameters such as
passenger_size.min.x,passenger_size.max.x, etc.Did you use Generative AI?
No.
Additional Information
rclpyPR: [Draft]Add structured parameter support rclpy#1494rclPR: [Draft]Add structured parameter support rcl#1254rcl_interfacesPR: Add structured parameter support rcl_interfaces#183