-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Improve and extend launch file tutorial #876
Improve and extend launch file tutorial #876
Conversation
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments here and there. Overall it makes sense to me.
|
||
Open the new file in your preferred text editor. | ||
Create a launch file named ``turtlesim_mimic_launch.xml`` or ``turtlesim_mimic_launch.py``, depending if you're using XML or Python launch files. | ||
Open it in your preferred text editor. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
meta: why remove existing tabs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMHO, there's no reason to explain how to create a file.
Just open your favorite text editor and click the new file
button.
There's also no reason to explain how to create a directory, because you don't need to create one to follow the tutorial (e.g.: I didn't).
IMO, those tabs add unnecessary "distractions", but maybe my opinion is exaggerated.
|
||
|
||
TBD: Conditions look too ugly in XML (those scape characters!), we should improve them. | ||
We could use https://github.com/ros2/launch/pull/457, and make that available in XML. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Though I will say that it's not conditions that look ugly, but Python code evaluation. For that, we could get rid of the outer single quotes and have eval
parsing function putting together all arguments again (so long as it only takes one argument).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Though I will say that it's not conditions that look ugly, but Python code evaluation. For that, we could get rid of the outer single quotes and have eval parsing function putting together all arguments again (so long as it only takes one argument).
IIUC, you're saying that $(eval arg1 arg2 ... argN)
should be possible and equivalent to:
$(eval 'arg')
with arg=' '.join(args)
right?
That sounds reasonable to me, I can open a patch in launch with that.
We could also have substitutions like: $(var_eq var_name value)
$(var_neq var_name value)
, but it seems a bit unnecessary (remembering those substitutions names would be hard).
|
||
# In another terminal | ||
ros2 launch turtlesim_mimic_launch.py turtlesim_ns1:=drawing_squares_1 turtlesim_ns2:=drawing_squares_2 mimic_name:=mimic_squares | ||
ros2 run turtlesim draw_square --ros-args -r __ns:=/drawing_squares_1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think much of these could be inside the launch file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that is covered in the next step.
My idea was to motivate why you want launch arguments and substitutions, without making the launch file to complex.
<arg name="turtlesim_ns2" default="turtlesim2" description="namespace of the other turtlesim to run"/> | ||
<arg name="mimic_name" default="mimic" description="name of the mimicking node"/> | ||
|
||
<include file="$(dirname)/turtlesim_mimic_launch.xml"/> <!-- arguments are passed automatically--> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is clever, but not particularly common. I wonder if we shouldn't use packages in here (which are the usual deposit for launch files).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next step would be exactly that, using packages.
I didn't want to introduce that together with the new launch concepts, and I prefer starting with launch files created without an associated package than the opposite.
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
@ivanpauno In the interest of cleaning up the PRs and the branches on this repository, could you move this branch to a fork and close this PR? Thanks. |
IIRC this was waiting on a review/refactor from @maryaB-osr, but I can close it and move the branch to a fork if that's preferred. |
Yeah, if you don't mind moving it, that would be great. I'm happy to do a review pass once it is reopened. |
Done, see #1109. |
To start with the new tutorials, I have migrated the existing one to a XML launch file.
The python examples are still there, in a different tab.
I have also added a few sections showing more advance launch features (in progress).
I have deleted the explanation of how to create a folder/file. I think that's a bit unnecessary, but I can add that again.