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

Convert launch files to new style #262

Merged
merged 2 commits into from Jun 26, 2018

Conversation

Projects
None yet
6 participants
@kev-the-dev
Copy link
Contributor

commented Jun 25, 2018

in dummy_robot_bringup and lifecycle (addreses #249)

@wjwwood
Copy link
Member

left a comment

It's just convention, but please put .launch.py at the end of the file names (so rename them). Otherwise the ros2 launch command will not tab complete them. This is done to differentiate them from normal python scripts.

launch.actions.ExecuteProcess(
cmd=[
ExecutableInPackage(package='robot_state_publisher', executable_name='robot_state_publisher'),
os.path.join(

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 25, 2018

Member

Can you put a TODO comment here to use a substitution to find this file rather than using ament_index_python directly. I haven't implemented the substitution yet, but it is planned.

def generate_launch_description():
return LaunchDescription([
launch.actions.ExecuteProcess(
cmd=[ExecutableInPackage(package='dummy_map_server', executable_name='dummy_map_server')],

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 25, 2018

Member

If these are ROS nodes (I think they are) then you should use launch_ros.actions.Node instead.

def generate_launch_description():
return LaunchDescription([
launch.actions.ExecuteProcess(
cmd=[ExecutableInPackage(package='lifecycle', executable='lifecycle_talker')],

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 25, 2018

Member

I think you can/should use the launch_ros.actions.LifecycleNode action for the lifecycle_talker and lifecycle_listener instead and the launch_ros.actions.Node for the lifecycle_service_client.

This comment has been minimized.

Copy link
@mikaelarguedas

mikaelarguedas Jun 26, 2018

Contributor

Can launch_ros.actions.LifecycleNode be used on regular rclcpp Nodes ?

If it doesn't make sense to do so, we should likely not use it for the listener as it is a "regular node" AFAICT:

class LifecycleListener : public rclcpp::Node

This comment has been minimized.

Copy link
@wjwwood

wjwwood Jun 26, 2018

Member

If it is not an actual lifecycle node then it will work, but it will have unnecessary overhead and if someone attempts to make launch change its state that will fail).

So I'd say you're right we should use Node on the confusingly name :p lifecycle_listener.

@kev-the-dev kev-the-dev force-pushed the kev-the-dev:fix-legacy-launch branch 2 times, most recently from 60f2f80 to deef9fc Jun 26, 2018

@kev-the-dev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Addressed comments (that's a lot cleaner!). Feel free to wait until after bouncy release as this is low priority

# limitations under the License.

from launch import LaunchDescription
from launch_ros.actions import Node, LifecycleNode

This comment has been minimized.

Copy link
@mikaelarguedas

mikaelarguedas Jun 26, 2018

Contributor

Nit: I don't know if these files are linted automatically but we usually import in alphabetical order

This comment has been minimized.

Copy link
@dirk-thomas

dirk-thomas Jun 26, 2018

Member

And one import per line. So this should be split into two lines.

kev-the-dev added some commits Jun 25, 2018

Convert launch files to new style
in dummy_robot_bringup and lifecycle

@kev-the-dev kev-the-dev force-pushed the kev-the-dev:fix-legacy-launch branch from deef9fc to adcf3a7 Jun 26, 2018

@kev-the-dev

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2018

Once this is merged, the dummy-robot-demo wiki page will need to be updated to refer to the new filename. The lifecycle launch is not currently referenced in the lifecycle wiki page, but probably should be.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Jun 26, 2018

Let's merge this and @ironmig can you deploy the wiki changes and then link them here?

I'll test them on linux, macOS, and Windows (at least from source and x86).

@wjwwood wjwwood merged commit 03ee0e0 into ros2:master Jun 26, 2018

@wjwwood wjwwood removed the in review label Jun 26, 2018

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2018

I think there was a reason for not mentioning the launchfiles in the wiki page. I think they were even considered for removal after the ardent release.
Maybe @Karsten1987 can pitch in on this.
We can update the wiki in the meantime and revert based on the feedback.

@wjwwood

This comment has been minimized.

Copy link
Member

commented Jun 27, 2018

I checked out the wiki changes, and they work on my machine(s), thanks!

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Follow-up PR at #277

@Myzhar

This comment has been minimized.

Copy link

commented Sep 26, 2018

What about the https://github.com/ros2/launch/blob/master/launch_ros/examples/lifecycle_pub_sub_launch.py
launch file?

I noticed that it is written in old style, but the wiki cites it as a valid example:
https://github.com/ros2/ros2/wiki/Launch-system

I'm trying to understand how to modify it, but without success

@dhood

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

Would you be able to rephrase your question? I'm not sure that I've understood it. The launch file you referenced is using the new-style launch, not the legacy launch.

@Myzhar

This comment has been minimized.

Copy link

commented Sep 26, 2018

If I try to adapt it to my lifecycle node I get an error about generate_launch_description not defined.
I thought it was why it is using the old style

@dhood

This comment has been minimized.

Copy link
Member

commented Sep 26, 2018

it's likely you're using an older version of launch (Ardent, perhaps) that doesn't include the new-style launch (it was released in Bouncy). If that's not the case, please ask a question on answers.ros.org and we can troubleshoot there rather than on this PR

@Myzhar

This comment has been minimized.

Copy link

commented Sep 26, 2018

I'm using the launch file available in the master branch and it is exactly the same as the file that I linked here

@mikaelarguedas

This comment has been minimized.

Copy link
Contributor

commented Sep 26, 2018

@Myzhar please ask your question on ROS answers using the "ros2" tag and providing a description containing:

  • what you are trying to accomplish
  • what is your setup (OS, version of ROS2 etc)
  • exact steps to reproduce your issue

Commenting on a closed PR with little context makes it hard for the community to see your question and help you solve your issue.

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.