Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YAML parser: support mixed types #463

Open
christianrauch opened this issue Jun 21, 2019 · 17 comments
Open

YAML parser: support mixed types #463

christianrauch opened this issue Jun 21, 2019 · 17 comments
Labels
backlog enhancement New feature or request

Comments

@christianrauch
Copy link

Bug report

Required Info:

  • Operating System: Ubuntu 18.04
  • Installation type: binary
  • Version or commit hash: dashing
  • DDS implementation: Fast-RTPS
  • Client library: rclcpp

Steps to reproduce issue

  1. create a yaml configuration file containing:
my_param_lists:
  - frame: base
    id: 9
  - frame: object
    id: 14
  1. provide configuration file to node (e.g. via mapping __params)

Expected behavior

The yaml parser should parse the YAML structure.

Actual behavior

The parser fails with:
Failed to parse yaml params file '[configuration].yaml': Sequence should be of same type. Value type 'integer' do not belong at line_num [line with "id: 9"], at /tmp/binarydeb/ros-dashing-rcl-yaml-param-parser-0.7.5/src/parser.c:931
I.e. it won't accept that frame and id are of different type (string, int).

Additional information

This has been supported in ROS1 (e.g. in the configuration of apriltags_ros) and should be made available for ROS2.

@jacobperron jacobperron added the enhancement New feature or request label Jun 26, 2019
@nuclearsandwich nuclearsandwich self-assigned this Jul 11, 2019
@nuclearsandwich
Copy link
Member

The error message is somewhat misleading here and there's more preventing the above from working than just the YAML parsing.

There is not currently a supported parameter datatype for dictionaries/associative arrays. The supported datatypes are listed in the design doc describing parameters.
Heterogeneous arrays are also not supported currently.

As a start, improving the user experience so the error text points out the conceptual issue rather than a parser error would be most welcome. Is that something you would consider contributing @christianrauch?

If there are additional parameter datatypes that you would like to see, I think a PR to the design article linked above would be a a good start.

@christianrauch
Copy link
Author

christianrauch commented Jul 12, 2019

Is it a conceptual issue that this kind of parameter is not supported? I.e. is it possible to support this later or will this never happen?

These kinds of parameters definitely worked in ROS1. For example, this:

<rosparam param="tag_descriptions">[
    {id: 0, size: 0.163513},
    {id: 1, size: 0.163513, frame_id: a_frame},
    {id: 2, size: 0.163513, frame_id: tag_2},
    {id: 3, size: 0.163513},
    {id: 4, size: 0.163513},
    {id: 5, size: 0.163513}]
</rosparam>

could be added to a launch file and represented and used in the node as XmlRpc::XmlRpcValue: https://github.com/RIVeR-Lab/apriltags_ros/blob/62fbdc5797e67058a88a2495b19664a455903b30/apriltags_ros/src/apriltag_detector.cpp#L153-L180

I am fine with the error message per se, but I think that ROS2 needs an equivalent representation for this kind of heterogeneous key value maps.

@jacobperron
Copy link
Member

I don't believe there's a conceptual issue. As @nuclearsandwich points out, what needs to be done is to add support for a new datatype to handle dictionaries. In order to do this we should extend the rcl_variant_s struct. It might make sense to define a new array of variant type, rcl_variant_array_s, and add that to the rcl_variant_s struct.

A agree that a PR to the design article that @nuclearsandwich referenced is the place to start.

@felixdivo
Copy link

This would really make things easier, and people should rightfully be able to expect (at least that degree of) YAML support for something that is called a YAML file.

@wieset
Copy link

wieset commented Jan 24, 2020

I was surprised today to find that ros2 supports only basic yaml syntax and had to resort to parsing by myself to avoid changing the whole structure. In addition to complex and mixed type lists it would also be nice to see anchors / aliases supported.

Any reason ros2 is using its own parser instead of e.g. yaml-cpp?

@jacobperron
Copy link
Member

The ROS 2 parameter parser uses the third-party parser from libyaml. I think we are not using yaml-cpp specifically because rcl is written in C not C++.

If you'd also like to see other YAML features supported, feel free open a feature request, or even better a pull request :)

@wieset
Copy link

wieset commented Jan 30, 2020

That makes sense. I must have missed that, sorry. Will open a feature request.

@LANB0

This comment was marked as abuse.

@zkytony
Copy link

zkytony commented Mar 26, 2023

Just wanted to raise here that the issue still exists (in ROS2 Humble).

Below is a minimum example YAML file needed to reproduce the error message:

node_name:
  ros__parameters:
      color:
        - name: red
          code:
            r: 255
            g: 0
            b: 0
            hex: F00

I wanted to load this YAML file in ROS2 launch file using <param from="xxx.yaml">. When running the launch file, I get

Couldn't parse params file: '--params-file .path/to/xxx.yaml'. Erro
r: Sequence should be of same type. Value type 'integer' do not belong at line_num 6, at ./src/parse.c:337, at ./src/rcl/arguments.c:406

@jukindle
Copy link

As a consequence, creating laser filters from parameters seems to be unusable as configuring a filter chain is not possible...

@fujitatomoya
Copy link
Collaborator

This is not supported in rolling, https://docs.ros2.org/foxy/api/rcl_interfaces/msg/ParameterType.html.
Parameter type does not have List or Dictionary/Map type, but only array type.

btw, is that supposed to be the following, which can be parsed.

root@tomoyafujita:~/ros2_ws/colcon_ws# cat demo_params.yaml
parameter_blackboard:
  ros__parameters:
    color:
      name: red
      code:
        r: 255
        g: 0
        b: 0
        hex: F00

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 run demo_nodes_cpp parameter_blackboard --ros-args --params-file demo_params.yaml
[INFO] [1680299494.598974816] [parameter_blackboard]: Parameter blackboard node named '/parameter_blackboard' ready, and serving '10' parameters already!

root@tomoyafujita:~/ros2_ws/colcon_ws# ros2 param list
/parameter_blackboard:
  color.code.b
  color.code.g
  color.code.hex
  color.code.r
  color.name
  qos_overrides./parameter_events.publisher.depth
  qos_overrides./parameter_events.publisher.durability
  qos_overrides./parameter_events.publisher.history
  qos_overrides./parameter_events.publisher.reliability
  use_sim_time

@zkytony
Copy link

zkytony commented Mar 31, 2023

@fujitatomoya Does this work in humble? Also, the issue I saw was with tag in a launch file. I don’t know if there is any difference loading it via command line for a node.

@fujitatomoya
Copy link
Collaborator

yes it does.

@rcywongaa
Copy link

rcywongaa commented Jun 5, 2024

FWIW while a list of dicts is not allowed, a dict of dicts is OK

my_param_lists:
  0:
    frame: base
    id: 9
  1:
    frame: object
    id: 14

which seems rather counterintuitive as one would expect a dict of dicts to be more complicated than a list of dicts.
This does allow a simple hack to get around this issue though.

@asherikov
Copy link

I've implemented this workaroud in my serialization library if someone is interested -> https://github.com/asherikov/ariles/blob/head_2/tests/api_v2/demo_api_v2_ros2.yaml, https://github.com/asherikov/ariles/blob/head_2/tests/api_v2/demo_api_v2_ros2.cpp.
It sort of works, but there are issues with parameter declaration etc.

@YoshuaNava
Copy link

YoshuaNava commented Nov 21, 2024

Hi all,
We have stumbled into this issue, and are implementing workarounds, but I'm worried about how the absence of this feature can affect my organization's agility in moving to ROS2.

@jacobperron @anup-pem @fujitatomoya two questions:

  1. If support for YAML lists was implemented by the community, would you approve of it being adopted by RCL?
  2. Would you suggest any specific implementation path? I could give it a go, at least as a proof of concept, and report back on findings.

Thank you in advance 🙏

@asherikov
Copy link

Hi, filling the void:

  1. In order to address this in a general way it is necessary (roughly speaking) to create an array of ParameterValue inside ParameterValue message (https://github.com/ros2/rcl_interfaces/blob/rolling/rcl_interfaces/msg/ParameterValue.msg), but recursive message definitions are not supported.

  2. Personally I think the best option is to rewrite parameter framework from scratch, it has a few other annoying design flaws:

    • dynamic parameters are misleading since handling of their modification is up to a node developer, e.g., in order to know if modification takes effect user has to read the code;
    • dynamic parameters functionally overlap with services/topics and encourage poor application design (yes, I see them being used instead of services in practice);
    • no built-in mechanisms for handling global parameters, such as sim_time, which you have to take care of by yourself.

I have some ideas regarding (2), but community is highly unlikely to support such effort.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog enhancement New feature or request
Projects
None yet
Development

No branches or pull requests