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

add talker_listener.launch.py new-style launch file #244

Merged
merged 3 commits into from
Jun 15, 2018
Merged

Conversation

wjwwood
Copy link
Member

@wjwwood wjwwood commented Jun 14, 2018

Connects to ros2/launch#76

@wjwwood wjwwood added enhancement New feature or request in progress Actively being worked on (Kanban column) labels Jun 14, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 14, 2018

I'd like to expand this to include launch files for everything, but after we get closer to review.

@wjwwood wjwwood mentioned this pull request Jun 14, 2018
@wjwwood wjwwood added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jun 15, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

I expanded the launch files to include all the topics and service demos in demo_nodes_cpp. If anyone has time to review these quickly, I'd appreciate it!

@dirk-thomas
Copy link
Member

Works for me (the launch files themselves that is).

I wasn't able to Ctrl-C though:

[INFO] [launch]: process[talker-1]: started with pid [10880]
[INFO] [launch]: process[listener-2]: started with pid [10881]
[INFO] [talker]: Publishing: 'Hello World: 1'
[INFO] [listener]: I heard: [Hello World: 1]
[INFO] [talker]: Publishing: 'Hello World: 2'
[INFO] [listener]: I heard: [Hello World: 2]
[INFO] [talker]: Publishing: 'Hello World: 3'
[INFO] [listener]: I heard: [Hello World: 3]
^Csignal_handler(2)
signal_handler(2)
[INFO] [launch]: process[talker-1]: process has finished cleanly
[INFO] [launch]: process[listener-2]: process has finished cleanly

There is was hanging indefinitely. That should not block this PR though.

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.

👍 for the launch files in this PR since my other comment is not related these launch files but launch in general.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

I'll look into the hang.

@dhood
Copy link
Member

dhood commented Jun 15, 2018

think this should add a dependency on launch_ros in the package.xml yeah?

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

think this should add a dependency on launch_ros in the package.xml yeah?

Not sure, I guess you could, but a similar thing comes up where you wonder if every package in ROS 1 that has a launch file should depend on roslaunch.

If we decide to add a dependency, then we might as well add ros2launch in addition to (or instead of) launch_ros. Because those launch files cannot be run on their own.

@dhood
Copy link
Member

dhood commented Jun 15, 2018

I think adding a ros2launch dependency is a step further than necessary, because even though they're not usable on their own, it doesn't have to be ros2launch that uses them (could theoretically be anyone).

But I take your point about having all ROS 1 packages depend on roslaunch. In that sense I think it's reasonable that only packages that are actually launching launch files (e.g. in tests) depend on launch_ros themselves.

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

Anyone else have an opinion on that? Should this package (and others that contain launch files) depend on something (either launch_ros or ros2launch)?

@dirk-thomas
Copy link
Member

Since the launch files do import launch_ros I think the packages should have an exec_depend on launch_ros.

Regarding the dependency on ros2launch: while I can see the convenience if someone would only install that package how it would help I wouldn't add it since ros2launch is just one way these launch files can be used. As an analogy: if a package provides a component (a node compiled into a shared library) I wouldn't expect a dependency on something like ros2run-component (not that we have that - just as an example).

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

I added a dependency on launch_ros in demo_nodes_cpp, however I don't completely agree with it.

For me, adding launch_ros only doesn't make sense because though it imports launch_ros it will never actually need it unless launched and the only way to do that (currently) is with ros2launch.

So you end up either installing ros2launch (and therefore launch_ros implicitly) to launch it or you don't use the launch files in which case you have a dependency that you're not using.

I understand you could use the launch file without ros2launch some how, but the only case where I can imagine that is by including it from another launch file (in which case you'd need ros2launch again at some point).

Personally, I would have gone for just ros2launch, but I don't feel strongly about it. I think about it from the package developer's perspective when writing tutorials, they'd like to say apt install my-thing and then do ros2launch my_thing my_launch_file.launch.py. Then again I see the arguments not to have a dependency on the command line tool as well (maybe you want to launch it with some future gui based launch tool).

@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

I'm going to do CI and merge this while I continue looking into the reason ros2 launch doesn't exit on Linux. I'll make an issue for that.

@wjwwood wjwwood merged commit 3b42efb into master Jun 15, 2018
@wjwwood wjwwood deleted the ros2launch branch June 15, 2018 23:35
@wjwwood wjwwood removed the in review Waiting for review (Kanban column) label Jun 15, 2018
@wjwwood
Copy link
Member Author

wjwwood commented Jun 15, 2018

ros2/launch#80

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

Successfully merging this pull request may close these issues.

None yet

3 participants