Skip to content

add ability to define and pass launch arguments to launch files#123

Merged
wjwwood merged 26 commits intomasterfrom
feature/launch_arguments
Sep 4, 2018
Merged

add ability to define and pass launch arguments to launch files#123
wjwwood merged 26 commits intomasterfrom
feature/launch_arguments

Conversation

@wjwwood
Copy link
Copy Markdown
Member

@wjwwood wjwwood commented Aug 14, 2018 •
edited
Loading

This pull request is ready for review.

To try it, you can use the example files in ros2launch, e.g.:

% ros2 launch --show-args ./path/to/ros2launch/examples/example.launch.py
...

% ros2 launch ./path/to/ros2launch/examples/example.launch.py
...

% ros2 launch ./path/to/ros2launch/examples/example.launch.py node_prefix:=foo_
...

fixes #107

@wjwwood wjwwood self-assigned this Aug 14, 2018
@wjwwood wjwwood added the in progress Actively being worked on (Kanban column) label Aug 14, 2018
@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Aug 22, 2018

@nuclearsandwich c45a877 should fix Python 3.5 support. Why it fixes it 🤷‍♂️ .

@dhood
Copy link
Copy Markdown
Member

dhood commented Aug 22, 2018

Why it fixes it 🤷‍♂️

I tracked this down once in the past: 72ae169#r195963696

@dhood
Copy link
Copy Markdown
Member

dhood commented Aug 22, 2018

edit (bad link): 72ae169#r195963696

@dhood
Copy link
Copy Markdown
Member

dhood commented Aug 22, 2018

github is changing my links, sorry for spam. this should work

@mikaelarguedas
Copy link
Copy Markdown
Member

mikaelarguedas commented Aug 22, 2018

github is changing my links, sorry for spam. this should work

Yes github now perform differently if you get the comment from specific commit. Getting the comment link from the PR page should work though: #81 (comment)

@wjwwood wjwwood force-pushed the feature/launch_arguments branch from 3540e2a to e01ed64 Compare August 23, 2018 02:44
wjwwood added 22 commits August 22, 2018 19:59
@wjwwood
add self descriptions for substitutions
c9fb73f
@wjwwood
add tracebacks back to the output by default
2bb466e
@wjwwood
add new actions for declaring launch arguments
dc3eaa6
@wjwwood
new method on LaunchDescription which gets all declared arguments within
1af02d1
@wjwwood
add ability to pass arguments when including a launch description
6213656
@wjwwood
add description for local variables used in Node action
5cfd710
@wjwwood
fix bug in Node action
42bfcb9
@wjwwood
cleanup error reporting in Node action
4719cb4
@wjwwood
use launch arguments in examples
6a9cc14
@wjwwood
add ability to show and pass launch arguments on the command line
6b4927b
@wjwwood
fix python 3.5 support
2977c5b
@wjwwood
add accessor for the Condition of an Action
1f5bf87 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
do not automatically push/pop configs when including
c134906 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
small refactor to get the launch file location
d27ea82 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
improve ability to detect declared arguments across launch file includes
89eab55 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
add tests for the new DeclareLaunchArgument action
71a1dfb 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
test new features of IncludeLaunchDescription
8617c38 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
test new features of LaunchDescription class
c236b57 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
remove unused imports
f5ba658 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
improve output when showing arguments of a launch file
6ab4d88 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
fix the return type of LaunchService.run()
60b72cf 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
fix the checking for the asyncio event loop for the case where it is …
43cb119 
…not set

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood wjwwood force-pushed the feature/launch_arguments branch from e01ed64 to 43cb119 Compare August 23, 2018 02:59
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Aug 23, 2018
@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Aug 23, 2018

Ok, this one is ready for review, CI:

  • Linux Build Status
  • Linux-aarch64 Build Status
  • macOS Build Status
  • Windows Build Status

mjcarroll
mjcarroll requested changes Aug 30, 2018
View reviewed changes
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll left a comment

Choose a reason for hiding this comment

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

I mentioned this directly to you, but if you run your example launch file without the demo executables available (I removed via rm -rf), the launch executable will report an error, and then go into a state where you cannot stop it, even with multiple Ctrl-C's.

I have put the output here: https://gist.github.com/mjcarroll/65ee17100613d41ea503408ba86c183e

launch/launch/actions/declare_launch_argument.py Outdated
will be exposed as arguments when that launch description is included, e.g.
as additional parameters in the
:py:class:`launch.actions.IncludeLaunchDescription` action or as
command-line arguments when launch with ``ros2 launch ...``.
Copy link
Copy Markdown
Member

@mjcarroll mjcarroll Aug 29, 2018

Choose a reason for hiding this comment

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

launched

@wjwwood
typo
97edc9f 
Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Aug 30, 2018

So, I think that if you change the name of the executable that the example launch file is looking for or delete it (as you did), then you'll get the same behavior on master. So I don't really want to have to fix that on this pull request. It's already being tracked here: #112 I hope to have time to look into that tomorrow.

Can you re-review with the above in mind, only testing out the new features (passing of arguments, etc...).

@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Aug 30, 2018

The only difference with master is that I don't print out the traceback by default (only on debug), which I changed in this pr, but it still produces the same exception and runtime behavior.

wjwwood added 3 commits August 30, 2018 15:38
@wjwwood
restart event loop to allow proper shutdown when there's an unhandled…
b4969f4 
… exception

Signed-off-by: William Woodall <william@osrfoundation.org>
@wjwwood
ExecuteProcess: unregister event handlers if rest of setup fails
f077ab6 
The event handlers need to be setup before the other lines, but are invalid if setup does not complete successfully.
@wjwwood
only put traceback in debug logging
4dbd9bb
@wjwwood
Copy link
Copy Markdown
Member Author

wjwwood commented Aug 30, 2018

I looked into it and the fix was pretty simple, so I went ahead and committed it in this pr: b4969f4

Basically that just lets already running things shutdown when an unhandled exception is received. I was never able to reproduce the "cannot even ctrl-c it" scenario, even before.

I also committed f077ab6 to avoid invalid state in the execute process action if the exception originated there (basically it registers event handlers, and then an exception occurs, making the event handlers invalid).

Also, I'd appreciate feedback on 4dbd9bb which is restoring the state of master which is that exceptions are reported as a single line and the traceback is only in the debug output.

@wjwwood wjwwood merged commit aaacf5c into master Sep 4, 2018
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Sep 4, 2018
@wjwwood wjwwood deleted the feature/launch_arguments branch September 4, 2018 16:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjcarroll mjcarroll mjcarroll approved these changes

Assignees

@wjwwood wjwwood

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[launch] add new concept for LaunchArgument

4 participants

@wjwwood @dhood @mikaelarguedas @mjcarroll