Launch file migration guide #302

ivanpauno · 2019-07-23T12:58:19Z

Migration guide from ROS 1's roslaunch XML files to ROS 2's XML launch files.
This is a starting point, in order to discuss possible changes in the ROS 2's XML launch file format.

TODOs:

Add examples of each tag.
Add a toy example.
Add a cheatsheet.
Reference ROS 2's XML launch format design document (when it gets merged).

Signed-off-by: ivanpauno <[email protected]>

ivanpauno

I'm doing an auto-review, adding comments about things we may want to change and adding links to previous discussions.

ivanpauno · 2019-07-23T13:03:32Z

source/Tutorials/Launch-files-migration-guide.rst

+As in `ROS1 <http://wiki.ros.org/roslaunch/XML/node>`__, it allows launching a new node.
+The following summarize the differences:
+
+* ``pkg`` attribute is now ``package``.


I tried using the same names as in the constructor:
https://github.com/ros2/launch_ros/blob/68337f836b9dfbf4bed2b9c3b3ae03d21570cf13/launch_ros/launch_ros/actions/node.py#L54-L67

I actually sometimes break that rule, when it was to verbose. For example:

node_name, node_executable, node_namespace are name, executable and ns.

I think it's fine to change package to pkg too.

Fine by me as well.

ivanpauno · 2019-07-23T13:06:24Z

source/Tutorials/Launch-files-migration-guide.rst

+The following summarize the differences:
+
+* ``pkg`` attribute is now ``package``.
+* ``type`` attribute is now ``executable``.


In this case, I prefer not to change executable. But I don't mind if we want to change to type.

I always thought type was confusing, I think executable makes more sense. Unless someone can explain why it is that way.

source/Tutorials/Launch-files-migration-guide.rst

ivanpauno · 2019-07-23T13:14:13Z

source/Tutorials/Launch-files-migration-guide.rst

+^^^^^
+
+In ROS 2, there's no global parameters concept.
+Application using global parameters should be refactored.


I think we should create a tutorial for that (sth like, Idioms for replacing global parameters).

Yeah, in general we've moved away from that, but we could still have a facility like this, using something like this node: https://github.com/ros2/demos/blob/f78dcc981cc783aad162e2a8530a7aa23c4fda21/demo_nodes_cpp/src/parameters/parameter_blackboard.cpp#L21

I think a tutorial for this would be good. But it's fine to just say here we don't have that so these tags don't necessarily make sense any longer.

source/Tutorials/Launch-files-migration-guide.rst

ivanpauno · 2019-07-23T13:28:43Z

source/Tutorials/Launch-files-migration-guide.rst

+* There is a new ``exec-in-package`` substitution.
+  e.g.: ``$(exec-in-package <package_name> <exec_name>)``
+* There is a new ``find-exec`` substitution.
+* ``anon`` hasn't an alternative at the moment.


There is not implementation in launch.

Can you create an issue to add that to launch (no need to be ROS specific I think)?

Suggested change

* ``anon`` hasn't an alternative at the moment.

* ``anon`` has no alternative at the moment.

ros2/launch#289

source/Tutorials/Launch-files-migration-guide.rst

ivanpauno · 2019-07-23T13:44:03Z

source/Tutorials/Launch-files-migration-guide.rst

+Type inference rules
+--------------------
+
+The rules that were shown in ``Type inference rules`` subsection of ``param`` tag applies to any attribute.


We have some inconsistencies on how we are coercing types in different places of launch.
For example, Node action use yaml for converting parameter, xml launch frontend has its own rules, and other actions do other things (e.g.:
https://github.com/ros2/launch/blob/15af530bd2a1d207849659615fbbdbb5a4daf4a5/launch/launch/actions/timer_action.py#L69-L72, and we're actually losing precision).

One problem of this, is that setting a parameter from a substitution looks different at setting it directly (see https://github.com/ros2/launch_ros/blob/68337f836b9dfbf4bed2b9c3b3ae03d21570cf13/test_launch_ros/test/test_launch_ros/frontend/test_node_frontend.py#L29-L53).

I was thinking in adding support of typed substitutions (they aren't performed always to str). The benefits would be:

Standardize code handling substitutions that are coerced to another type (bool, float, etc).

In the cases were more than a type is possible (e.g.: parameters), if the user want to pass a parameter that is known to be an integer and the substitution is later coerced to a string, we could raise a substitution failure. Now, this is not possible. In that case, we will set a parameter of the wrong type, and the node that uses it may handle it correctly or not.

I would need more convincing that typed substitutions make sense. We'd need special logic to then convert those typed substitutions to strings in many cases, which is basically what we're doing now.

I don't understand what it buys us, maybe you could give a concrete example where it would be better.

I'm not a big fan of typed substitutions either, but I do strongly believe we have to standardize type inference rules everywhere -- even during substitution.

maryaB-osr · 2019-07-24T16:54:11Z

source/Tutorials/Launch-files-migration-guide.rst

@@ -0,0 +1,287 @@
+Migrating ROS 1 launchfiles


"Migrating ROS 1 launch files to ROS 2"
Possibly add to the title for better description (it's obvious to someone already on the ROS 2 docs site that we're talking about ROS 2, but it might benefit searches from external search engines)
I think launch files is two words? I can't find anywhere in existing docs where it's combined

maryaB-osr · 2019-07-24T17:01:17Z

source/Tutorials/Launch-files-migration-guide.rst

+----------
+
+A description of the ROS 2 launch system and its Python API can be found in `this tutorial<Launch-system>`.
+In this tutorial, will be described how to write XML launch files, which allow an easy migration from ROS 1.


There's variation among the existing tutorials, but I think only the first sentence here constitutes a "background". The second sentence is more of a summary/overview. I would rearrange (and rewrite for wordiness) like this (following the table of contents):

"This tutorial describes how to write XML launch files for an easy migration from ROS 1.

Background

A description of the ROS 2 launch system and its Python API can be found in this tutorial<Launch-system>."

maryaB-osr · 2019-07-24T17:40:22Z

source/Tutorials/Launch-files-migration-guide.rst

+They aren't supported at the moment.
+
+New tags
+^^^^^^^^


"New tags in ROS 2
-------------------------"
This should be at the same level as "Migrating tags from ROS 1 to ROS 2", because it's two different topics: old tags and new tags. Then every subtopic should be moved up a level

wjwwood · 2019-07-24T17:16:16Z

source/Tutorials/Launch-files-migration-guide.rst

+As in `ROS1 <http://wiki.ros.org/roslaunch/XML/node>`__, it allows launching a new node.
+The following summarize the differences:
+
+* ``pkg`` attribute is now ``package``.


Fine by me as well.

wjwwood · 2019-07-24T17:16:55Z

source/Tutorials/Launch-files-migration-guide.rst

+The following summarize the differences:
+
+* ``pkg`` attribute is now ``package``.
+* ``type`` attribute is now ``executable``.


I always thought type was confusing, I think executable makes more sense. Unless someone can explain why it is that way.

source/Tutorials/Launch-files-migration-guide.rst

wjwwood · 2019-07-24T17:23:18Z

source/Tutorials/Launch-files-migration-guide.rst

+^^^^^
+
+In ROS 2, there's no global parameters concept.
+Application using global parameters should be refactored.


Yeah, in general we've moved away from that, but we could still have a facility like this, using something like this node: https://github.com/ros2/demos/blob/f78dcc981cc783aad162e2a8530a7aa23c4fda21/demo_nodes_cpp/src/parameters/parameter_blackboard.cpp#L21

I think a tutorial for this would be good. But it's fine to just say here we don't have that so these tags don't necessarily make sense any longer.

wjwwood · 2019-07-24T17:25:27Z

source/Tutorials/Launch-files-migration-guide.rst

+This tag can only be used nested in a node tag.
+`ROS 1 reference <http://wiki.ros.org/roslaunch/XML/param>`__.
+
+* There is not ``type`` attribute. See type deduction rules below.


This might be a good feature to have in the future, since you might want to catch issues early by having launch evaluate the output of a command or something and assert it's an int or a float or something. It's usefulness is limited, however, as it doesn't scale to sets of parameters (e.g. yaml dictionary) very easily.

source/Tutorials/Launch-files-migration-guide.rst

wjwwood · 2019-07-24T17:51:32Z

source/Tutorials/Launch-files-migration-guide.rst

+* There is a new ``exec-in-package`` substitution.
+  e.g.: ``$(exec-in-package <package_name> <exec_name>)``
+* There is a new ``find-exec`` substitution.
+* ``anon`` hasn't an alternative at the moment.


Can you create an issue to add that to launch (no need to be ROS specific I think)?

wjwwood · 2019-07-24T17:51:55Z

source/Tutorials/Launch-files-migration-guide.rst

+* There is a new ``exec-in-package`` substitution.
+  e.g.: ``$(exec-in-package <package_name> <exec_name>)``
+* There is a new ``find-exec`` substitution.
+* ``anon`` hasn't an alternative at the moment.


Suggested change

* ``anon`` hasn't an alternative at the moment.

* ``anon`` has no alternative at the moment.

wjwwood · 2019-07-24T17:57:12Z

source/Tutorials/Launch-files-migration-guide.rst

+Type inference rules
+--------------------
+
+The rules that were shown in ``Type inference rules`` subsection of ``param`` tag applies to any attribute.


I would need more convincing that typed substitutions make sense. We'd need special logic to then convert those typed substitutions to strings in many cases, which is basically what we're doing now.

I don't understand what it buys us, maybe you could give a concrete example where it would be better.

wjwwood · 2019-07-24T18:00:19Z

source/Tutorials/Launch-files-migration-guide.rst

+.. code-block:: xml
+
+   <!--Setting a string value to an attribute expecting an int will raise an error.-->
+   <tag1 int-attr="'1'"/>


I understand these examples, but I think it's confusing when first reading it. When you're using pseudo syntax, better to make it painfully obvious. Like consider using <made_up_tag1 attr-expecting-an-int="'1'">.

source/Tutorials/Launch-files-migration-guide.rst

Signed-off-by: ivanpauno <[email protected]>

source/Tutorials/Launch-files-migration-guide.rst

Signed-off-by: ivanpauno <[email protected]>

ivanpauno · 2019-07-26T19:48:00Z

@wjwwood @maryaB-osr I think I've addressed all your comments.

ivanpauno · 2019-07-26T19:54:23Z

@wjwwood @hidmic This PR, together with ros2/launch#291, ros2/launch#288, ros2/launch_ros#47, ros2/launch_ros#40, ros2/design#247, are the last changes that I'm planning to do related with XML launch files.

I also opened a bunch issues in launch and launch_ros repos, describing possible future improvements (and I will probably open a couple more).

If you think something else have to be addressed now, please let me know.

ivanpauno · 2019-07-26T20:13:02Z

@wjwwood @maryaB-osr I think I've addressed all your comments.

I missed your last review @maryaB-osr, I will address those comments too.

maryaB-osr · 2019-07-26T20:15:43Z

I missed your last review @maryaB-osr, I will address those comments too.

Just a final proofread. I didn't realize you'd missed it either!

Signed-off-by: ivanpauno <[email protected]>

hidmic

First pass. Punctuation use is inconsistent. Also, header layout does not follow a common pattern.

hidmic · 2019-08-06T21:19:00Z

source/Tutorials/Launch-files-migration-guide.rst

+
+* `In ROS1 <http://wiki.ros.org/roslaunch/XML/param>`__
+* Used for passing a parameter to a node
+* There's no global parameter concept in ROS 2.


@ivanpauno nit: for some sentences with have trailing periods, for some not. Should we standardize it? @maryaB-osr for feedback.

I think that the idea is that list items shouldn't end with a period, but I'm not sure.

"Used for passing..." is not a complete sentence, so it shouldn't end with a period.
"There's no global..." is a complete sentence, so it should end with a period. Also it has a sentence following it, so they need to be separated.
I agree it looks odd though, having both in one list.
For now you can pick one standard, but make sure all the bullets follow the rule. So either:

all ending with a period

none ending with a period

only complete sentences ending with a period (incomplete sentences are the ones starting with verbs like used, allows, etc.)

The first two will look the best, the last is technically grammatically correct

I agree, added period in all the sentences.

Thanks for the thorough explanation @maryaB-osr !

hidmic · 2019-08-06T21:21:50Z

source/Tutorials/Launch-files-migration-guide.rst

+Parameter grouping
+~~~~~~~~~~~~~~~~~~
+
+In ROS 2, param tags are allowed to be nested.


@ivanpauno consider:

Suggested change

In ROS 2, param tags are allowed to be nested.

In ROS 2, `<param/>` tags are allowed to be nested.

instead.

I prefer ``param`` than ``<param/>``, but I can change the style if you think the second option is better.

source/Tutorials/Launch-files-migration-guide.rst

hidmic · 2019-08-06T21:32:52Z

source/Tutorials/Launch-files-migration-guide.rst

+let
+^^^
+
+It's a replacement of ``arg`` tag with a value attribute.


@ivanpauno is it just that?

Yes, I think it's exactly the same.

Hmm it doesn't seem like it. In roslaunch, <arg/> was used to declare an argument or to set it upon an <include/>. Here, <let/> allows setting launch configurations which are superset of launch arguments. So, though it can be used to set launch arguments, that's not the only thing it is capable of.

From http://wiki.ros.org/roslaunch/XML/arg.

<arg name="foo" value="bar" /> Declares foo with constant value. The value for foo cannot be overridden. This usage enables internal parameterization of a launch file without exposing that parameterization at higher levels.

The other two cases:

<arg name="foo" /> <arg name="foo" default="1" />

look exactly the same in ROS 1 and ROS 2.

Signed-off-by: ivanpauno <[email protected]>

hidmic

Final pass. Also, most tags have a bullet that says In ROS1. When rendered, that's all you're going to see. I'd rather change it to something like Available in ROS1, or anything else along those lines.

source/Tutorials/Launch-files-migration-guide.rst

hidmic · 2019-08-09T18:59:05Z

source/Tutorials/Launch-files-migration-guide.rst

+let
+^^^
+
+It's a replacement of ``arg`` tag with a value attribute.


Hmm it doesn't seem like it. In roslaunch, <arg/> was used to declare an argument or to set it upon an <include/>. Here, <let/> allows setting launch configurations which are superset of launch arguments. So, though it can be used to set launch arguments, that's not the only thing it is capable of.

source/Tutorials/Launch-files-migration-guide.rst

Signed-off-by: ivanpauno <[email protected]>

source/Tutorials/Launch-files-migration-guide.rst

hidmic · 2019-08-21T18:59:34Z

source/Tutorials/Launch-files-migration-guide.rst

+   * The following attributes aren't available: ``machine``, ``respawn``, ``respawn_delay``, ``clear_params``.
+
+Example
+~~~~~~~


@ivanpauno nit: when rendered, it's not obvious which headers belong to what section/subsection. Maybe lowering the header level?

Signed-off-by: ivanpauno <[email protected]>

hidmic

LGTM, but the comment about header levels still applies. Let's see how it renders at index.ros.org

ivanpauno · 2019-08-27T12:36:15Z

LGTM, but the comment about header levels still applies. Let's see how it renders at index.ros.org

https://index.ros.org/doc/ros2/Tutorials/Intra-Process-Communication/ use the same header levels.
I tried some other ways of organizing the sections, but I didn't like them much.

I will merge this as-is. If it looks confusing in index.ros.org, we can improve header style in a follow-up PR.

Signed-off-by: ivanpauno <[email protected]>

ivanpauno added 3 commits July 19, 2019 17:13

Add migration guide of launch files

61c569f

Signed-off-by: ivanpauno <[email protected]>

Correct error

b4428a6

Signed-off-by: ivanpauno <[email protected]>

Address Mich's feedback

e3f3b14

Signed-off-by: ivanpauno <[email protected]>

ivanpauno added enhancement New feature or request in progress labels Jul 23, 2019

ivanpauno self-assigned this Jul 23, 2019

ivanpauno added 3 commits July 23, 2019 09:59

Correct nit

22d6cfa

Signed-off-by: ivanpauno <[email protected]>

Correct nit

d16c944

Signed-off-by: ivanpauno <[email protected]>

Correct exec-in-package substitution

7a56dc3

Signed-off-by: ivanpauno <[email protected]>

ivanpauno commented Jul 23, 2019

View reviewed changes

ivanpauno added in review and removed in progress labels Jul 23, 2019

ivanpauno requested review from hidmic, wjwwood, dirk-thomas and maryaB-osr July 23, 2019 13:47

ivanpauno mentioned this pull request Jul 23, 2019

ROS 2 Launch System ros2/design#163

Merged

wjwwood mentioned this pull request Jul 23, 2019

ROS 2 Eloquent Elusor ros2/ros2#734

Closed

34 tasks

maryaB-osr reviewed Jul 24, 2019

View reviewed changes

wjwwood reviewed Jul 24, 2019

View reviewed changes

maryaB-osr reviewed Jul 24, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

This was referenced Jul 25, 2019

Support for respawn in ExecuteProcess ros2/launch#287

Closed

Add support for not optional environment variable substitution ros2/launch#288

Merged

Add a way of grouping remapping rules and parameters ros2/launch_ros#45

Closed

Address review comments, add more examples

511dc73

Signed-off-by: ivanpauno <[email protected]>

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

maryaB-osr reviewed Jul 26, 2019

View reviewed changes

source/Tutorials/Launch-files-migration-guide.rst Outdated Show resolved Hide resolved

Minimal renaming

909bffd

Signed-off-by: ivanpauno <[email protected]>

ivanpauno mentioned this pull request Jul 26, 2019

[launch frontend] Rename some tag attributes ros2/launch_ros#47

Merged

maryaB-osr closed this Jul 26, 2019

ivanpauno reopened this Jul 26, 2019

Address per review comments

7e1b95c

Signed-off-by: ivanpauno <[email protected]>

ivanpauno requested a review from wjwwood July 30, 2019 13:59

hidmic requested changes Aug 6, 2019

View reviewed changes

ivanpauno added 3 commits August 7, 2019 10:46

Address per review comments

ea2f779

Signed-off-by: ivanpauno <[email protected]>

Address per review comments

287be4d

Signed-off-by: ivanpauno <[email protected]>

Correct punctuation and subtitles style

aa40977

Signed-off-by: ivanpauno <[email protected]>

hidmic reviewed Aug 9, 2019

View reviewed changes

ivanpauno added 2 commits August 9, 2019 17:13

Address per review comments

3d13b61

Signed-off-by: ivanpauno <[email protected]>

Replaced 'In ROS 1' to 'Available in ROS 1'

8f3310b

Signed-off-by: ivanpauno <[email protected]>

hidmic reviewed Aug 21, 2019

View reviewed changes

Address per review comments

24b6092

Signed-off-by: ivanpauno <[email protected]>

hidmic approved these changes Aug 26, 2019

View reviewed changes

ivanpauno merged commit b5e3177 into master Aug 27, 2019

delete-merged-branch bot deleted the ivanpauno/roslaunch-migration branch August 27, 2019 12:37

ivanpauno mentioned this pull request Sep 19, 2019

Bad interaction between launch frontend and YAML parameters ros2/launch_ros#74

Closed

prajakta-gokhale pushed a commit to aws-ros-dev/ros2_documentation that referenced this pull request Oct 14, 2019

Launch file migration guide (ros2#302)

6c13a3b

Signed-off-by: ivanpauno <[email protected]>

	* ``anon`` hasn't an alternative at the moment.
	* ``anon`` has no alternative at the moment.

	In ROS 2, param tags are allowed to be nested.
	In ROS 2, `<param/>` tags are allowed to be nested.

Launch file migration guide #302

Launch file migration guide #302

Conversation

ivanpauno commented Jul 23, 2019

ivanpauno left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maryaB-osr Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

maryaB-osr Jul 24, 2019 • edited Loading

Choose a reason for hiding this comment

Background

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanpauno commented Jul 26, 2019

ivanpauno commented Jul 26, 2019

ivanpauno commented Jul 26, 2019

maryaB-osr commented Jul 26, 2019

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hidmic left a comment

Choose a reason for hiding this comment

ivanpauno commented Aug 27, 2019 • edited Loading

ivanpauno left a comment •

edited

Loading

maryaB-osr Jul 24, 2019 •

edited

Loading

maryaB-osr Jul 24, 2019 •

edited

Loading

ivanpauno commented Aug 27, 2019 •

edited

Loading