search

ros / urdfdom

URDF parser
http://ros.org/wiki/urdf
Other
96 stars 132 forks source link

fixed transmission parser to match specification #68

Closed rhaschke closed 8 years ago

rhaschke commented 9 years ago

The new transmission parser of urdf_parser_py didn't match the specification in the wiki.

Moved hardwareInterface from <actuator> to <joint> and made them an aggregate (one to many such elements are possible according to specification).

guihomework commented 8 years ago

Looks like this PR is related to #36 and hence should be tested against older distribution to verify the parsing still works with hardwareInterface removed from the <actuator> (which was required for hydro version of gazebo_ros_control apparently) Also the duck typing might break with two types of <joint> elements but same tag.

rhaschke commented 8 years ago

Indeed hardwareInterface cannot be parsed in <actuator> tags anymore with this patch. Maintainers should decide about the API that should be supported. IMHO it doesn't make sense to support a wrong (or deprecated) syntax. Rather the URDF XML should be fixed to match the specification. Duck typing isn't affected by the PR, because the different <joint> elements occur in different contexts.

rhaschke commented 8 years ago

Actually, PR #63 tried to fix that issue before too. However, IMHO #63 is a hack, simply declaring <hardwareInterface> as optional within an <actuator> element, thus avoiding the error message, but still not correctly parsing the content. The proper way is to parse <hardwareInterface> as part of <joint> elements as I did in my very first commit.

My recent commits

In order to facilitate the Ubuntu release process (#30), should I rebase those python-related changes onto some older branch? Which one (cf. comment in #30)?

rhaschke commented 8 years ago

@isucan Ioan, please can you have a look at this PR?

jacquelinekay commented 8 years ago

Hi @rhaschke,

thanks for your contribution!

We should target backwards-compatible bugfixes into branches 0.2 or 0.3 (0.2 targets trusty, 0.3 targets Utopic and Vivid).

@scpeters and I are planning to release urdfdom 0.4 today, which will allow API/ABI changes. We are trying to get this version released into Ubuntu X (Xenial Xerus).

I will take a look at this PR today and judge which parts can be backported into which versions. We will try to get it in for 0.4.0 at least (assuming it correctly implements the specification).

scpeters commented 8 years ago

The test passes for me.

jacquelinekay commented 8 years ago

I agree that this implementation is a correct interpretation of the URDF spec.

This should get into 0.4 (and Kinetic by extension).

I also think it should get backported into 0.2/0.3.

It is an API change, but it fixes a major inconsistency with the standard.

And since it only effects urdf_parser_py, which we release as a ROS package and not an Ubuntu system package, there are fewer restrictions of API fixes.

scpeters commented 8 years ago

Ok, I'm +1 on merging to master. A separate pull request or manual cherry-pick can be used for backporting to older release branches.

jacquelinekay commented 8 years ago

I will merge this then squash the commits (since I don't have permission to squash from @rhaschke's fork)

jacquelinekay commented 8 years ago

I've cherry-picked the squashed commit into 0.2 and 0.3.