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

Adds ROS 2 launch XML format design document #207

Merged
merged 11 commits into from
Jul 16, 2019
Merged

Conversation

hidmic
Copy link
Contributor

@hidmic hidmic commented Jan 22, 2019

This pull request is the v0.1.0 draft of an XML format for the ROS 2 launch system.

Currently just mostly keeping portability with ROS 1. It includes all relevant features for the first milestone on ROS 2 launch front-ends.

@tfoote tfoote added the in progress Actively being worked on (Kanban column) label Jan 22, 2019
@dirk-thomas dirk-thomas mentioned this pull request Jan 22, 2019
20 tasks
@hidmic hidmic self-assigned this Jan 23, 2019
- Adds <executable> tag to launch non ROS nodes executables.
- Modifies <param> and <params> tags for better usability.
- Adds optional default values for $(env ...) substitutions.
- Fully documents the XML schema.
- Fixes bad Markdown headers layout.

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic hidmic changed the title [WIP] Adds ROS 2 launch XML format design document Adds ROS 2 launch XML format design document Jan 24, 2019
@hidmic hidmic added in review Waiting for review (Kanban column) and removed in progress Actively being worked on (Kanban column) labels Jan 24, 2019
@hidmic hidmic requested review from mjcarroll and wjwwood January 24, 2019 16:52
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
- Adds a <let> tag to set launch configuration variables.
- Drops $(arg ...) substitution in favor of $(var ...).
- Addresses peer review comments.

Signed-off-by: Michel Hidalgo <[email protected]>
Signed-off-by: Michel Hidalgo <[email protected]>
- Adds version attribute for <launch> tag
- Moves launch XML schema to its own .xsd file

Signed-off-by: Michel Hidalgo <[email protected]>
@hidmic
Copy link
Contributor Author

hidmic commented Feb 5, 2019

FYI I just collapsed the <params> tag into the <param> to make things slightly more homogeneous and thus easier to deal with in the implementation (which I'm thinking of and drafting some considerations and hints for).

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

@ivanpauno ivanpauno left a comment

Choose a reason for hiding this comment

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

I did some minimal changes in 96307f7 and 44b9054.

I also left some comment about namespaces. Basically, include and group action are part of launch package, which shouldn't include ros related code.
One possible workaround is always doing the following:

<group>
    <let name="ros_namespace" value="$(var ros_namespace)/my_namespace"/>
    ... further include or node actions
</group>

That doesn't look quite nice.

The other workaround for this is adding alternative versions of IncludeLaunchDescription and GroupAction in launch_ros which handle this. We should also add in launch.frontend a way of allowing overriding exposed actions and substitutions (which is a little tricky).

@wjwwood @hidmic any opinions?

I think that we can delete it from the design document at the moment, and address it in a follow up.

articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
@hidmic
Copy link
Contributor Author

hidmic commented Jul 12, 2019

We should also add in launch.frontend a way of allowing overriding exposed actions and substitutions (which is a little tricky).

Hmm, I see why, but I wonder if what this is actually telling us is that we need namespaced tags e.g. <ros::group/> vs. <group/> in XML.

I think that we can delete it from the design document at the moment, and address it in a follow up.

I'm also inclined to defer this till the next iteration, along with event handlers.

@ivanpauno
Copy link
Member

Hmm, I see why, but I wonder if what this is actually telling us is that we need namespaced tags e.g. <ros::group/> vs. <group/> in XML.

That sounds like a great idea. I would like to see a using tag too (<using name="ros"/>). Handling overrides will be easy.

I'm also inclined to defer this till the next iteration, along with event handlers.

Sure, I will delete event handlers and ros-namespaces from the document. We can add them in the next iteration.

@wjwwood
Copy link
Member

wjwwood commented Jul 12, 2019

So it just boil down to this:

<group ns="my_ns">
</group>

Versus this:

<group>
  <let name="ros_namespace" value="$(var ros_namespace)/my_namespace"/>
</group>

I think we could have something like this:

<group>
  <push_ros_namespace namespace="my_namespace"/>
  ...
</group>

And that would be sufficient.

If we really want to have it as an attribute of <group> then I think you guys have identified one way which is to have <group> and <ros::group> where the latter can override the first if you do something like <using name="ros::group"/> or <using namespace="ros"/>. Another option would be to extend <group> directly, so that if you have launch_ros_xml, it adds acceptable attributes to <group>, like the ns one, through some extensibility interface.

@ivanpauno
Copy link
Member

I think we could have something like this:

<group>
  <push_ros_namespace namespace="my_namespace"/>
  ...
</group>

Yes, that one would be easy to implement, and less verbose than my option.

If we really want to have it as an attribute of <group> then I think you guys have identified one way which is to have <group> and <ros::group> where the latter can override the first if you do something like <using name="ros::group"/> or <using namespace="ros"/>. Another option would be to extend <group> directly, so that if you have launch_ros_xml, it adds acceptable attributes to <group>, like the ns one, through some extensibility interface.

I think the namespace option would be really nice, because it avoids the possibility of name collisions. But, it's harder to implement.

For the moment I will add a push_ros_namespace action. I think we can merge the document as-is and update it later.

@hidmic
Copy link
Contributor Author

hidmic commented Jul 12, 2019

+1 to push_ros_namespace for now and deferring namespace/extension discussions to the next iteration.

@ivanpauno ivanpauno requested a review from wjwwood July 12, 2019 19:52
Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, with a few comments and one requested style change.

articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
articles/151_roslaunch_xml.md Outdated Show resolved Hide resolved
@ivanpauno ivanpauno requested a review from wjwwood July 16, 2019 14:04
@ivanpauno ivanpauno assigned ivanpauno and unassigned hidmic Jul 16, 2019
@ivanpauno
Copy link
Member

ivanpauno commented Jul 16, 2019

With ros2/launch#272 and ros2/launch#273, we will have exactly the functionality described in this document.

Copy link
Member

@wjwwood wjwwood left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@ivanpauno ivanpauno merged commit 9d902de into roslaunch Jul 16, 2019
@delete-merged-branch delete-merged-branch bot deleted the hidmic/launch-xml branch July 16, 2019 18:24
@wjwwood wjwwood mentioned this pull request Jul 23, 2019
34 tasks
wjwwood added a commit that referenced this pull request Sep 18, 2019
* wip

* more work in progress

* more work done on the calling conventions section

* added context section, still a WIP

* updated event sections

* event filters

* fixup even handler subsection

* typos

* add subsections about the system description (language agnostic)

* Proposal for launching dynamically composable nodes (#206)

* Proposal for dynamically composed nodes

* allow multiple extra_arguments

* Allow node_name and namespace to be empty

* Human readable error message

* Update articles/150_roslaunch.md

Co-Authored-By: sloretz <[email protected]>

* Assign nodes unique ids, but still forbid duplicates

* Update articles/150_roslaunch.md

Co-Authored-By: sloretz <[email protected]>

* Update articles/150_roslaunch.md

Co-Authored-By: sloretz <[email protected]>

* Section to list

* More generic wording about container processes

* namespace -> node_namespace

* _launch/ -> ~/_container/

Signed-off-by: Shane Loretz <[email protected]>

* Propose ROS 2 launch front-end design hints. (#208)

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

* Adds ROS 2 launch XML format design document (#207)

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

* Update ROS launch documentation (frontend documents) (#247)

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

* review fixup

Signed-off-by: William Woodall <[email protected]>

* clarify lifecycle transition due to review comment

Signed-off-by: William Woodall <[email protected]>

* provide rationale for shutdown procedure

Signed-off-by: William Woodall <[email protected]>

* Add launch XML substitution for a packages share directory (#254)

* Add launch XML substitution for a packages share directory

Rename find-pkg to find-pkg-prefix.
Add find-pkg-share substitution for the share directory.

Signed-off-by: Jacob Perron <[email protected]>

* remove some old rfc's and general cleanup for first merge

Signed-off-by: William Woodall <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in review Waiting for review (Kanban column)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants