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

Clarify ros2 arguments precedence #289

Open
wants to merge 10 commits into
base: gh-pages
Choose a base branch
from

Conversation

ivanpauno
Copy link
Member

@ivanpauno ivanpauno commented Jun 26, 2020

Adds a section to the command line arguments article, clarifying how precedence currently works.
Adds a discussion section exposing inconsistencies and weird interaction with launch files.

The goal of this PR is to state how precedence currently works to kick off a discussion on how it should work.
After that discussion takes place, I will update the document with the conclusion.

Follow up of ros2/rclcpp#953.

Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno ivanpauno added the in review Waiting for review (Kanban column) label Jun 26, 2020
@ivanpauno ivanpauno requested a review from hidmic June 26, 2020 18:49
@ivanpauno ivanpauno self-assigned this Jun 26, 2020
@ivanpauno
Copy link
Member Author

@hidmic can you make a first pass?

articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Outdated Show resolved Hide resolved
articles/150_ros_command_line_arguments.md Show resolved Hide resolved
ivanpauno and others added 9 commits June 26, 2020 18:16
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>

Co-authored-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@clalancette
Copy link
Contributor

As the starting point of what rules are currently in place, this is great. Thanks @ivanpauno .

Where we want to go is harder to say. I would say that there are 2 main problems pointed out here:

  1. The rules for parameter overrides and remapping overrides are different.
  2. The rules for launch are inconsistent with the command-line.

I'm going to concentrate on the second one for now (the first one is unfortunate, but at least documented now). The problem as I understand it is that launch does not (and cannot) know the fully-qualified node name. That's because the node may have hard-coded node names and even namespaces internally to itself. Is that a good summary of the problem?

If that's the case, I'm wondering what would happen if we end up making launch a bit dumber. That is, launch would just pass along the arguments exactly as it had them. Then the precedence rules would be figured out by the rcl/rclcpp layer inside of the node, and the parameters applied. This takes launch completely out of the equation when doing overrides, and concentrates all of the logic of overrides in one place (it needs to be in rcl/rclcpp for the command-line case anyway).

I'm sure there are complications to the above, so I'm looking for feedback on why that would or wouldn't work.

@ivanpauno
Copy link
Member Author

If that's the case, I'm wondering what would happen if we end up making launch a bit dumber. That is, launch would just pass along the arguments exactly as it had them. Then the precedence rules would be figured out by the rcl/rclcpp layer inside of the node, and the parameters applied. This takes launch completely out of the equation when doing overrides, and concentrates all of the logic of overrides in one place (it needs to be in rcl/rclcpp for the command-line case anyway).

The current implementation tries to be as dumb as possible.
The problem is that Node action targets a single process with a single node, and rcl doesn't have any assumption about the number of nodes in a process. We could definitely have in launch a MultiNodeExecutable action, which would allow to set up a process with manually composed nodes (trying to use Node action for this doesn't work at all).

When launch generates a command from:

Node(
  package='my_pkg',
  executable='my_exec',
  name = 'my_node',
  parameters=['path/to/file', {'my_param': 'my_value}]
)

the result can be:

(op A)

/path/to/executable/in/package --ros-args -r __node:=my_node --params-file path/to/file -p my_param:=my_value

or:

(op B)

/path/to/executable/in/package --ros-args -r __node:=my_node --params-file path/to/file --params-file path/to/tmp/param/file

where:

# path/to/tmp/param/file contents
/**:
  ros__parameters:
    my_param: my_value

Currently, launch is doing op B, but op A would result in the same problem.

I'm not sure of what modifications in rcl and launch_ros could solve the problem without the user explicitly passing the namespace in the Node action.
i.e. I'm not sure if we can make launch more dumb of what currently is (I have tried to think of a solution, and I haven't come up with anything).


Support to wildcards in parameter files was added to solve the problem of not knowing the fully qualified node name at launch time (see ros2/launch_ros#35, ros2/launch_ros#4).

Considering that that was the main goal of wildcards, maybe we could make them have the same precedence than parameter assignments targeting a specific node. The problem is that that is a bit counter intuitive (I find it super counter intuitive, but it would solve the launch file use case).

@jacobperron
Copy link
Member

Reviewing proposed changes for additional wildcard support (ros2/rclcpp#1265) brought me here.
I realized that wildcard support in parameter YAML files was not documented anywhere and so it would be nice to update this design doc.

I've opened #303 to document how wildcards can be used in parameter files. I have not addressed precedence (like this PR aims to do).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants