ros2 / launch

Tools for launching multiple processes and for writing tests involving multiple processes.
Apache License 2.0
124 stars 139 forks source link

launch_xml param tag, value attribute parsed as yaml #729

Closed tylerjw closed 10 months ago

tylerjw commented 1 year ago

My use-case is something that was normal for us in ROS 1 and also works currently in ROS 2 with python launch files. The use-case is putting the output of the xacro command into a string parameter. This output is an XML file. When using the param XML tag like this:

<param name="robot_description_semantic" value="$(command 'xacro $(find-pkg-share kinova_gen3_7dof_robotiq_2f_85_moveit_config)/config/gen3.srdf')" />

I get errors because it is trying to parse the value field as if it was yaml:

' using yaml rules: yaml.safe_load() failed
mapping values are not allowed here
  in "<unicode string>", line 11, column 13:
      <!--GROUPS: Representation of a set of joi ...
                ^

To get around this I've tried various methods of escaping the string (', ", |(newline), &#34;, &#39;). With the exception of single quote, none of these have an effect on the output error. Single-quote causes a new failure because my XML file contains single quotes.

Is there some work-around to this you could give me or could you point me to the code that is doing this so I could maybe propose a change to this behavior? This is probably a side-affect of the whole escaping lists of things. Maybe we should use a different attribute for when you want this yaml parsing vs when you don't?

tylerjw commented 1 year ago

A work-around I found was to remove : characters from my XML file as they were all in comments.

nbbrooks commented 12 months ago

I think this might also be breaking custom gz-sim sensors launched via xacro, as this requires using a ignition:type or gz:type attribute (per example here). I've adopted a custom sensor into a xacro, and the presence of that attribute causes a Unbounded prefix error

Interestingly, if I change

        <sensor name="an_odometer" type="custom" ignition:type="odometer">
        </sensor>

to

        <!-- ignition:type="odometer" -->
        <sensor name="an_odometer" type="custom">
        </sensor>

The xacro launch does not fail to parse. So the confusion does not happen in this case with a : in a comment.

gavanderhoorn commented 11 months ago

@tylerjw: I'm not completely sure it would fix your issue, but I just noticed that the param element supports a type attribute (as it did in ROS 1).

Something like this lets me load XML files (such as SRDFs) with embedded colons and other characters which YAML parsers typically don't like:

<param
  name="robot_description_semantic"
  type="str"
  value="$(command 'cat $(find-pkg-share my_moveit_cfg)/config/robot.srdf')"
/>

without the type="str", this fails with:

[ERROR] [launch]: Caught exception in launch (see debug for traceback): Failed to convert '<?xml version="1.0" encoding="UTF-8"?>
<!--This does not replace URDF, and is not an extension of URDF.
...
</robot>
' using yaml rules: yaml.safe_load() failed
mapping values are not allowed here
  in "<unicode string>", line 7, column 15:
        <!--GROUPS: Representation of a set of joi ... 
                  ^

The documentation I've been able to find on launch_xml does not mention type AFAICT.

(edit: it is mentioned in https://github.com/ros/robot_state_publisher/issues/155#issuecomment-1023601332 and https://github.com/ros2/launch_ros/issues/136#issuecomment-669304265. Perhaps it's in the release notes as well, I haven't checked)


Edit: I've tested this on Rolling. Don't know whether it's supported on anything else.

Also: before this I implemented a work-around with a small Python script (instead of cat):

#!/usr/bin/env python3
import sys
print(f"|")
with open(sys.argv[1], 'r') as f:
    for line in f.readlines():
      sys.stdout.write(f"  {line}")
print("")

this emits an anonymous block scalar which makes the parser just consume all lines verbatim.

It worked, but it wasn't very elegant ..

tylerjw commented 10 months ago

@gavanderhoorn wow, that fixed my problem! I'm sorry it took me so long to get back to testing this.

gavanderhoorn commented 10 months ago

Np.

Thank @ivanpauno for implementing the needed support.

Would be nice to document it somewhere though :)

Together with ros2/launch_ros#382, loading SRDFs was the last bit needed to be able to use XML launch files to load MoveIt configuration packages.

tylerjw commented 10 months ago

While that feature would be nice you don't need it if you do what I did here: https://github.com/tylerjw/tylerjw.dev/tree/main/content%2Fposts%2Fxml_launch%2Feasy_launch_demo

tylerjw commented 10 months ago

Are you going to be at roscon?

gavanderhoorn commented 10 months ago

While that feature would be nice you don't need it if you do what I did here: tylerjw/tylerjw.dev@main/content%2Fposts%2Fxml_launch%2Feasy_launch_demo

true, but I'd like to keep .yaml files separate, and I don't want to have to add the /** and ros_parameters 'everywhere'.

you ended up with a similar .launch.xml structure I have, so that's good to see.

What I was trying to do was see whether I could auto-convert ROS 1 MoveIt config packages. With a few tweaks to the .xml launch files, it's actually not that hard to do.

In the end what you have seems like a ROS-2-i-fied demo.launch -- or the moveit_planning_execution.launch we have/had in ROS-Industrial tutorials/MoveIt config packages.

Are you going to be at roscon?

Unfortunately not.

From your article:

For this talk I converted MoveIt's launch from hundreds of lines of Python to less than 50 lines of xml.

yes, that's also what I set out to do.

I like Python as much as the next guy, but I'm really not sure it should be the go-to for these sorts of launch files. It's just too verbose.

Which is really saying something, especially seeing as XML is less verbose. That's not something I ever expected to write about XML :)

tylerjw commented 10 months ago

I've been toying with a script to convert existing config packages into this one file. I don't find the extra bit of syntax all that bad and in the single file format it isn't that hard to read. If we made all the config one file the launching becomes much simpler.

I also like python as a language, but I don't think it is a good tool where you need a declarative syntax.

tylerjw commented 10 months ago

The extra syntax in the yaml file is no where near as bad as all that happens in the Python launch files.

That being said being able to just drop in new xml launch files and use existing configs would be really nice.

gavanderhoorn commented 10 months ago

I've been toying with a script to convert existing config packages into this one file.

thing is: at least things like joint limits and kinematic configurations fi are sometimes part of robot description/support packages.

MoveIt / ROS 2 are not the only consumers of those files.

So I a) don't want to have to modify those just for use with ROS 2, and b) I'd like to avoid duplication as much as possible.

The extra syntax in the yaml file is no where near as bad as all that happens in the Python launch files.

completely agree.

tylerjw commented 10 months ago

In the case where those files are part of the description package, do they not end up being arguments to the xacro or are they used in some other way? In the case where limits go into the xacro shouldn't they just be read by MoveIt from the srdf instead of a separate config? Or maybe the limits represent different limits? Where the one in the xacro defines properties of the hardware but you still want separate limits for moveit that are more restrictive for the sake of how planning works?

gavanderhoorn commented 10 months ago

Or maybe the limits represent different limits? Where the one in the xacro defines properties of the hardware but you still want separate limits for moveit that are more restrictive for the sake of how planning works?

yes, this one fi.

The UR description packages are one example: default limits from the description pkg .yamls, MoveIt cfg might have different limits.

And this is just one use-case/example.