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

Move specification from ROS wiki to the urdfdom repo #183

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

traversaro
Copy link
Contributor

@traversaro traversaro commented May 9, 2023

Fix ros/urdfdom_headers#62 . See also discussion in https://discourse.ros.org/t/urdf-improvements/30520/25 .

Currently in draft, however early comments on the parts that are already converted are welcome. PR is now ready for review.

The converted specification is visible at https://github.com/traversaro/urdfdom/tree/movespecification/specification .

@traversaro traversaro marked this pull request as draft May 9, 2023 21:08
@ros-discourse
Copy link

This pull request has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/short-term-wg-about-urdf-description-formats/30843/17

@traversaro traversaro changed the title [Draft] Move specification from ROS wiki to the urdfdom repo Move specification from ROS wiki to the urdfdom repo Feb 15, 2024
@traversaro
Copy link
Contributor Author

Thanks to @harleylara that completed the work in traversaro#1, I think we are not ready for review in the PR. There are still some DCO problems that we need to fix, but anyhow I think this is ready for a first review. @harleylara feel free to report here the question you made in traversaro#1 .

@traversaro traversaro marked this pull request as ready for review February 15, 2024 14:22
nickswalker and others added 9 commits February 16, 2024 00:10
Makes it easier to pipe results from xacro, and matches urdfdom_py's behavior
Closes ros#170

Signed-off-by: harleylara <[email protected]>
* upgrade from tinyxml to tinyxml2

* Remove 'using tinyxml2'

* Update github actions.

* Add in FindTinyXML2 for systems that don't have it.

* Make sure that tinyxml2::tinyxml2 is available in downstream packages

* Update urdfdom-config.cmake.in

* Replace InsertNewChildElement with GetDocument()->NewElement.

The former doesn't exist in older TinyXML2, and is just
a convenience function anyway.

* Fix generation of CMake config files.

Use the provided CMake primitives for this, which should
make it work in all situations.

* Install the FindTinyXML2.cmake file.

This is so downstream projects can always find it.

* Backup and restore the CMAKE_MODULE_PATH.

So we don't permanently manipulate it.

Signed-off-by: Chris Lalancette <[email protected]>
Co-authored-by: Silvio Traversaro <[email protected]>
Signed-off-by: harleylara <[email protected]>
That way, we don't have to export the tinyxml2 dependencies
to downstream consumers.  It is just a private dependency
at that point.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: harleylara <[email protected]>
There are some absolute path in the generation of the .cmake files
which ruin the creation of relocatable packages.
Fixing:
 * CMAKE_MODULE_PATH append instead of replacement
 * INTERFACE use relative path instead of ${CMAKE_INSTALL_INCLUDEDIR}

See also

https://cmake.org/cmake/help/latest/guide/importing-exporting/index.html#creating-relocatable-packages

for reference.

Signed-off-by: Matthias Schoepfer <[email protected]>
Co-authored-by: Matthias Schoepfer <[email protected]>
Signed-off-by: harleylara <[email protected]>
* Deprecate the APIs that we think are unused.

In an earlier commit, we changed from tinyxml -> tinyxml2
in the public API because we thought that there were no
users of these APIs.  Codify that here by marking these
APIs as deprecated; if a user comes along and says that
they are actually using it, we can undeprecated it.

Note that in order to avoid deprecations from within the
library, I had to add a bit of additional indirection here.
If we remove the APIs in the future, we can also remove this
indirection.

* Properly export parsePoseInternal to make Windows happy.

Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: harleylara <[email protected]>
Signed-off-by: Chris Lalancette <[email protected]>
Signed-off-by: harleylara <[email protected]>
Signed-off-by: harleylara <[email protected]>
@harleylara
Copy link

Trying to add the DCO to the commits is modifying the commit history heavily (I'm not a big fan of it)...Any strategy or recommendation?

@traversaro
Copy link
Contributor Author

I see a few spurious commits to the history, I can look in cleaning up the history and leave your commits ready for adding DCO, is that ok?

Copy link

@peci1 peci1 left a comment

Choose a reason for hiding this comment

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

I'm sending a batch of mostly stylistic/grammar changes.

@@ -0,0 +1,47 @@
# XML Robot Description Format (URDF)
Copy link

Choose a reason for hiding this comment

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

Suggested change
# XML Robot Description Format (URDF)
# Unified Robot Description Format (URDF)

?

Choose a reason for hiding this comment

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

Definitely it is Unified 😂... For some reason in the wiki is written as "XML ..."

Copy link

Choose a reason for hiding this comment

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

I guess keeping consistency would be prefered ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is one please where I think we should make a change from the wiki, and call it "Unified" as is appropriate.

@@ -0,0 +1,27 @@
# `<robot>` element

The root element in a robot description file must be a `<robot>` element, with all other elements must be encapsulated within.
Copy link

Choose a reason for hiding this comment

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

Suggested change
The root element in a robot description file must be a `<robot>` element, with all other elements must be encapsulated within.
The root element in a robot description file must be a `<robot>` element. All other elements must be encapsulated within it.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is another place where I think we can make the update to the language in this PR.


| `<robot>` attr | type | use | default value | description |
| -------------- | ------ | -------- | ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `name` | string | required | NA | The master file must have a *name* attribute. The *name* attribute is optional in included files. If the attribute *name* is specified in an additional included file, it must have the same value as in the master file. |
Copy link

Choose a reason for hiding this comment

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

What is a master file? Also, the term "master" is a candidate for eviction sooner or later... Also, what are included files? URDF itself doesn't support includes, or does it?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is another place where I think we should update the language. As @peci1 points out, this doesn't actually make too much sense currently.

Choose a reason for hiding this comment

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

I agree

The visual properties of the link. This element specifies the shape of the object (box, cylinder, etc.) for visualization purposes.

> [!NOTE]
> multiple instances of `<visual>` tags can exist for the same link. The union of the geometry they define forms the visual representation of the link.
Copy link

Choose a reason for hiding this comment

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

Suggested change
> multiple instances of `<visual>` tags can exist for the same link. The union of the geometry they define forms the visual representation of the link.
> Multiple instances of `<visual>` tags can exist for the same link. The union of the geometry they define forms the visual representation of the link.


| `<visual>` attr | type | use | default value | description |
| --------------- | -------- | -------- | ------------- | -------------------------------------------------------------------------------------------------------------------------------- |
| `name` | `string` | optional | none | Specifies a name for a part of a link's geometry. This is useful to be able to refer to specific bits of the geometry of a link. |
Copy link

Choose a reason for hiding this comment

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

Suggested change
| `name` | `string` | optional | none | Specifies a name for a part of a link's geometry. This is useful to be able to refer to specific bits of the geometry of a link. |
| `name` | `string` | optional | none | Specifies a name for a part of a link's geometry. This is useful to be able to refer to specific parts of the geometry of a link. |

| element | use | description |
| ---------------------- | -------- | -------------------------------------------------------------------------------------------- |
| [`<parent`](#parent) | required | Defines the parent link this sensor is attached to. |
| [`<origin>`](#origin)` | optional | This is the pose of the sensor optical frame, relative to the sensor parent reference frame. |
Copy link

Choose a reason for hiding this comment

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

Suggested change
| [`<origin>`](#origin)` | optional | This is the pose of the sensor optical frame, relative to the sensor parent reference frame. |
| [`<origin>`](#origin) | optional | This is the pose of the sensor optical frame, relative to the sensor parent reference frame. |


| element | use | description |
| ---------------------- | -------- | -------------------------------------------------------------------------------------------- |
| [`<parent`](#parent) | required | Defines the parent link this sensor is attached to. |
Copy link

Choose a reason for hiding this comment

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

Suggested change
| [`<parent`](#parent) | required | Defines the parent link this sensor is attached to. |
| [`<parent>`](#parent) | required | Defines the parent link this sensor is attached to. |


### `<origin>`

| attribute | type | use | default value | description |
Copy link

Choose a reason for hiding this comment

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

The table doesn't render correctly.

| `xyz` | `string` | optional | zero vector | Represents the offset with respect to the parent frame. |
| `rpy` | `string` | optional | zero vector | Represents the fixed axis roll, pitch and yaw angles in radians. |

### `<camaera>`
Copy link

Choose a reason for hiding this comment

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

Suggested change
### `<camaera>`
### `<camera>`

| --------- | -------------- | -------- | ------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `width` | `unsigned int` | required | NA | Width of the camera in `pixels`. |
| `height` | `unsigned int` | required | NA | Height of the camera in `pixels`. |
| `format` | `string` | required | NA | Can be any of the strings defined in [image_encodings.h insdie sensor_msgs](https://code.ros.org/trac/ros-pkg/browser/stacks/common_msgs/trunk/sensor_msgs/include/sensor_msgs/image_encodings.h). |
Copy link

Choose a reason for hiding this comment

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

Suggested change
| `format` | `string` | required | NA | Can be any of the strings defined in [image_encodings.h insdie sensor_msgs](https://code.ros.org/trac/ros-pkg/browser/stacks/common_msgs/trunk/sensor_msgs/include/sensor_msgs/image_encodings.h). |
| `format` | `string` | required | NA | Can be any of the strings defined in [image_encodings.h inside sensor_msgs](https://github.com/ros/common_msgs/blob/noetic-devel/sensor_msgs/include/sensor_msgs/image_encodings.h). |

@harleylara
Copy link

@peci1 thanks for taking the time to review and improve it, fresh eyes are always a great help 😁

@traversaro
Copy link
Contributor Author

Thanks @peci1 , I totally agree on the changes but I am not sure if we want to conflate in the same PR the migration to markdown/GitHub and changing the text that was in the wiki. Leaving the text as in the wiki probably would simplify the life of the reviewers of the PR. Perhaps @clalancette can comment on this. My hunch is that we can keep improvements in the text to future PRs. Related to this @harleylara do you think we could spin off the XSD changes in a separate PR, as it is kind of independent from the spec migration?

@peci1
Copy link

peci1 commented Feb 16, 2024

Okay, feel free to do it as you like/need. I think it'd be relatively easy to repeat the changes in a new PR.

@clalancette
Copy link
Contributor

Thanks @peci1 , I totally agree on the changes but I am not sure if we want to conflate in the same PR the migration to markdown/GitHub and changing the text that was in the wiki. Leaving the text as in the wiki probably would simplify the life of the reviewers of the PR. Perhaps @clalancette can comment on this.

In general, yeah, I think that would be better. I think we should do some light editing here, but anything more major should go into a separate PR.

With that in mind, I'm going to do a review, mostly thinking about where this should live in the directory hierarchy and making sure that this mostly matches what was in the wiki.

Copy link
Contributor

@clalancette clalancette left a comment

Choose a reason for hiding this comment

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

I've left some cleanups, and commented on the ones from @peci1 that I think we should keep.

Besides that, I have two broader things to talk about:

  1. There is no mention of model_state in here. Again, while I understand this is not heavily used (or used at all), it is part of the specification and I think we should have a page for it.
  2. While the tables describing the XML elements and attributes is kind of nice, it does seem somewhat harder to read than the original, which was nicely indented as the XML would be. I'm curious what others think.

@@ -0,0 +1,47 @@
# XML Robot Description Format (URDF)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is one please where I think we should make a change from the wiki, and call it "Unified" as is appropriate.

@@ -3,7 +3,8 @@ urdfdom

The URDF (U-Robot Description Format) library provides core data structures and a simple XML parsers for populating the class data structures from an URDF file.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
The URDF (U-Robot Description Format) library provides core data structures and a simple XML parsers for populating the class data structures from an URDF file.
The URDF (Unified Robot Description Format) library provides core data structures and a simple XML parsers for populating the class data structures from an URDF file.

@@ -0,0 +1,47 @@
# XML Robot Description Format (URDF)

The Unified Robot Description Format (URDF) is an XML specification to describe a robot. We attempt to keep this specification as general as possible, but obviously the specification cannot describe all robots. The main limitation at this point is that only tree structures can be represented, ruling out all parallel robots. Also, the specification assumes the robot consists of rigid links connected by joints; flexible elements are not supported. The specification covers:
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, nowadays we prefer one-sentence-per-line. That's so that review on particular sentences are easier in the future. So I'll suggest doing that here, as well as in all places it makes sense below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to suggest that we do all updates to the XSD in a separate PR. While I think the updates are important, I don't think they are directly tied to moving the specification over (but correct me if I'm wrong).

Choose a reason for hiding this comment

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

I completely agree with you on this... the main reason they are in the PR is for convenience (my own convenience 😅), as I was moving things from the wiki to the spec I was checking that the XSD reflected the wiki information and vice versa.

@@ -0,0 +1,27 @@
# `<robot>` element

The root element in a robot description file must be a `<robot>` element, with all other elements must be encapsulated within.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is another place where I think we can make the update to the language in this PR.


| attribute | type | use | default value | description |
| ------------------ | -------- | -------- | ------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `k_velocity` | `double` | required | NA | An attribute specifying the relation between effort and velocity limits. See See safety limits for more details. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed we should take this fix now.

# `<sensor>` element

>[!WARNING]
> The sensor element has been implemented in the URDF Dom but has never really been used in application. This is a project that was dropped anyone is encouraged to pick it up and extend it to sensor hardware applications. Please contribute!
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this warning is actually something we should have in the specification. That is, we have support for sensors, so we should just say that. The fact that no downstreams that we know of use it is a deployment/implementation detail, not a specification detail.

Comment on lines +109 to +111
## Proposal for New Type of Sensor

TBD
Copy link
Contributor

Choose a reason for hiding this comment

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

Just remove this, it only belongs in the specification once it has been implemented.

Comment on lines +8 to +30
Here is an example of a camera sensor element:
```xml
<sensor name="my_camera_sensor" update_rate="20">
<parent link="optical_frame_link_name"/>
<origin xyz="0 0 0" rpy="0 0 0"/>
<camera>
<image width="640" height="480" hfov="1.5708" format="RGB8" near="0.01" far="50.0"/>
</camera>
</sensor>
```

And below is an example of a laser scan (ray) sensor element:

```xml
<sensor name="my_ray_sensor" update_rate="20">
<parent link="optical_frame_link_name"/>
<origin xyz="0 0 0" rpy="0 0 0"/>
<ray>
<horizontal samples="100" resolution="1" min_angle="-1.5708" max_angle="1.5708"/>
<vertical samples="1" resolution="1" min_angle="0" max_angle="0"/>
</ray>
</sensor>
```
Copy link
Contributor

Choose a reason for hiding this comment

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

In most of the other files, the Examples are after the rest of the specification. I think that makes sense; talk about the individual items, and then show how to put it together. I'll suggest the same here.

| ------------------------------------- | -------------------------------------------------------------------- |
| [`<link>`](./link.md) | defines a link with its own frame. |
| [`<joint>`](./joint.md) | mandatory joint frame definition. |
| [`<transmission>`](./transmission.md) | (PR2 specific). |
Copy link
Contributor

Choose a reason for hiding this comment

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

This references transmission.md, but there is no file here with that name.

Choose a reason for hiding this comment

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

Do you think it makes more sense to remove it? <transmission> is not part of the parser and is specific to PR2 and ros_control so it doesn't seem to be part of the specification. Or what do you recommend?

Copy link

@harleylara harleylara left a comment

Choose a reason for hiding this comment

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

Just some comments.

Comment on lines +33 to +47
## `<link>` element

For details on the `<link>` description format, check out the [link element](./link.md) page.

## `<joint>` element

For details on the `<joint>` description format, check out the [joint element](./joint.md) page.

## `<gazebo>` element

For details on the `<gazebo>` description format, check out the [gazebo element](./gazebo.md) page.

## `<sensor>` element

For details on the `<senosr>` description format, check out the [sensor element](./sensor.md) page.

Choose a reason for hiding this comment

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

I agree... the specification/README.md we can try to keep it with general information about the specification, such as the spec target, current limitations, current version, among others matters.


| `<robot>` attr | type | use | default value | description |
| -------------- | ------ | -------- | ------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| `name` | string | required | NA | The master file must have a *name* attribute. The *name* attribute is optional in included files. If the attribute *name* is specified in an additional included file, it must have the same value as in the master file. |

Choose a reason for hiding this comment

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

I agree

| ------------------------------------- | -------------------------------------------------------------------- |
| [`<link>`](./link.md) | defines a link with its own frame. |
| [`<joint>`](./joint.md) | mandatory joint frame definition. |
| [`<transmission>`](./transmission.md) | (PR2 specific). |

Choose a reason for hiding this comment

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

Do you think it makes more sense to remove it? <transmission> is not part of the parser and is specific to PR2 and ros_control so it doesn't seem to be part of the specification. Or what do you recommend?

@@ -13,6 +13,18 @@
xmlns="http://www.ros.org"
elementFormDefault="qualified">

<!-- data type definitions -->
Copy link

@aminya aminya Apr 16, 2024

Choose a reason for hiding this comment

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

I tried the xsd specification. It's better than what's on Master, however, I got these errors:

There are '3' errors in 'https://github.com/traversaro/urdfdom/raw/movespecification/xsd/urdf.xsd'.xml
urdf.xsd(341, 69): s4s-att-invalid-value: Invalid attribute value for 'processContents' in element 'any'. Recorded reason: cvc-enumeration-valid: Value 'gazebo' is not facet-valid with respect to enumeration '(lax | skip | strict)'. It must be a value from the enumeration.
urdf.xsd(166, 43): cos-all-limited.2: The {max occurs} of an element in an 'all' model group must be 0 or 1. The value 'unbounded' for element 'visual' is invalid.
urdf.xsd(168, 46): cos-all-limited.2: The {max occurs} of an element in an 'all' model group must be 0 or 1. The value 'unbounded' for element 'collision' is invalid.

Maybe, the xsd can be changed in a separate pull request to speed up the review.

Copy link

Choose a reason for hiding this comment

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

I separated the XSD changes with my suggestions/fixes in #200.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, thanks!

Choose a reason for hiding this comment

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

Thanks @aminya 😃

Copy link

@aminya aminya May 28, 2024

Choose a reason for hiding this comment

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

You're welcome!

I do not know how the review process in this repo works, but it would be helpful if you approved the other pull request so that the maintainers can merge it faster.

#200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move URDF specification to a Github repo?
9 participants