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

Allow non-ros parameters in params file #1149

Open
davy-code opened this issue Apr 23, 2024 · 4 comments
Open

Allow non-ros parameters in params file #1149

davy-code opened this issue Apr 23, 2024 · 4 comments
Labels

Comments

@davy-code
Copy link

Feature request

Feature description

Allow non-ROS parameters to be disregarded by the YAML parser when running a node with --ros-args --params-file <config-name>.yaml
Currently a node e.g. turtlesim crashes with a YAML file looking like this:

/turtlesim:
  ros__parameters:
    background_b: 3
  other__parameters:
    param_1: 1

With the error:
Error: Cannot have a value before ros__parameters at line 5, at ./src/parse.c:793, at ./src/rcl/arguments.c:406
The parsing of the YAML file prevents easy integration in larger deployments where ros2 is only one of several middlewares being used. This implementation also prevents having one YAML file in larger deployments where a ros2 node is perhaps only a small part of the larger software stack.

Implementation considerations

  1. Allow a certain YAML key which will be ignored by the ROS2 YAML parser. Easy to implement but is more difficult for larger (non-ROS centric) software stacks.
  2. Disregard the full YAML apart from parameters specified if the key is ros__parameters. Might create uglier (less ROS-like) YAML files but allows for easy integration in larger software stacks.
@fujitatomoya
Copy link
Collaborator

I am not necessary against suggested approach. but i have a question.

/turtlesim:
  ros__parameters:
    background_b: 3
  other__parameters:
    param_1: 1

it can also expect that other__parameters could be the different node under the namespace turtlesim.
that case it can generate the error to let the user know that param_1 cannot be loaded? (we can print the warning, but there would be many warnings that say parameters will be ignored...) this is also better for the user to know instead of processing silently? the question is how we can decide if that is ignorable or not?

how about the following case? just ignore?

/**:
        wildcard: true

i think current behavior is not bad, it validate the parameter yaml file during purse.
in addition, why we would want to have unrelated elements in the yaml file loaded into ROS 2 system? i think it is easy to exclude the unrelated elements like,

tomoyafujita@~/DVT/work >cat test.yaml
/turtlesim:
  ros__parameters:
    background_b: 3
  other__parameters:
    param_1: 1

tomoyafujita@~/DVT/work >yq 'del(.. | .other__parameters?)' test.yaml
/turtlesim:
  ros__parameters:
    background_b: 3

@davy-code
Copy link
Author

Thank you for your response!
I fully agree that there are easy workarounds in most cases, especially in the above mentioned case. The workaround might however create confusion in situations where ROS2 nodes are deployed since the YAML file would be edited without a user knowing necessarily how or why.

  • Indeed the current parsing of YAML files by ROS2 allows parameters specified with the ros__parameters key that are never used, a warning in this case might be convenient.
  • The specific reason as to why I would like to see ROS2's YAML parser allowing non-ROS parameters is so it allows for easier integration in software stacks where other non-ROS2 nodes might also require a YAML file. A normal YAML file would only need to specify non-default parameters, so it can be pretty short. Perhaps this is a better real-life example
id: 1
/turtlesim:
  ros__parameters:
     background_b: 3
/non_ros2_node:
     param_1: 1
/turtlesim2:
  enable_ros2: true
  ros__parameters:
     background_r: 3

In this case there might be a really small number of non-ROS parameters in the non_ros2_node. So it might be less desirable to generate a separate YAML file, perhaps for the same reason several ROS2 nodes are allowed to be specified separately in one YAML file. Furthermore, the turtlesim2 node might only require ROS2 in some cases, so if enabled. Both of the above mentioned nodes will cause an error, but in my opinion both might be quite common in bigger software stacks. Similarly an id might be needed for identification of the deployed software stack, but it does not necessarily need to be a ROS2 parameter. The id could be handled by another YAML parser, it might also however be needed by a ROS2 node as well. Currently, this would thus also require two separate YAML files.

@gavanderhoorn
Copy link
Contributor

Related perhaps: ros2/launch_ros#382 (my own, I know, but still)?

@audrow audrow added the backlog label Jun 6, 2024
@audrow
Copy link
Member

audrow commented Jun 6, 2024

It's not clear to me what the best thing to do here is.

I'm going to backlog this issue for now. We will probably have discussions about how this should be handled during the ROS 2 meeting in the near future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants