Skip to content

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jul 22, 2019

Follow-up PR after the discussions that took place in #242.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jul 22, 2019

I think we can (and probably should) augment this document to encompass all ROS 2 nodes command line features. Also, I wonder now that we're introducing options, if we should move some features away from the double leading underscore syntax e.g. __log_level:=... to --log-level .... It's a cosmetic change though.

Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Parameter assignment may be achieved using the `--param`/`-p` option.
This option takes a single `name:=value` assignment statement, where `value` is in YAML format.

As an example, to assign an integer value of `2` to an `int_param` upon running `some_node`, one may execute:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if integer is a good example here. That also opens the question what makes this a parameter of type since there needs to be some magic to convert the type solely based on its value which has problems on its own.

Therefore I would suggest to simply use a string value here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I agree with using a string value to simplify the example but I also think that parameter value syntax cannot be left out of a command line specification. Here, just like when parameter files are parsed in rcl and when launch substitutions occur in Node parameters, we delegate on YAML format rules for type inference based on values. Whether that's a good call or not is likely worth a separate discussion.

---

- This will become a table of contents (this text will be scraped).
{:toc}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add a short section about the implementation. Basically where the extraction from argc / argv happens and that all remaining arguments are still available for the user code to process.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. It also raises an interesting question. Assuming extraction happens in rcl, should we support arguments specific to upper layers e.g. rclcpp or rclpy? Those would have to be available unparsed, separate from user defined arguments.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but they could live in between the --ros-args and --, and rcl could just ignore them and leave them in a "left over args" output. Then rclcpp and/or rclpy can use those arguments or make warnings/errors because there are unknown parameters left over.

@wjwwood wjwwood mentioned this pull request Jul 23, 2019
34 tasks
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Jul 24, 2019

@dirk-thomas PTAL

hidmic added 2 commits July 25, 2019 14:02
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
Signed-off-by: Michel Hidalgo <michel@ekumenlabs.com>
@hidmic
Copy link
Contributor Author

hidmic commented Sep 13, 2019

@dirk-thomas @wjwwood PTAL at c079dc7 so we can call it done.

@dirk-thomas dirk-thomas changed the title Add ROS 2 command line design document Add ROS 2 command line arguments design document Sep 13, 2019
Copy link
Member

@dirk-thomas dirk-thomas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@hidmic
Copy link
Contributor Author

hidmic commented Sep 13, 2019

Alright, going in!

@hidmic hidmic merged commit 977d2a4 into gh-pages Sep 13, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/cli-syntax branch September 13, 2019 18:05
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

late lgtm too :)

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

Successfully merging this pull request may close these issues.

4 participants