Urdf with args and yaml configuration #371

dhled · 2018-07-26T13:55:16Z

Move the duplicates parameters into common file and uses yaml file and xacro macro to configure urdf.

This PR includes:

Refactoring the common ur macro in a common files
Parse the parameters from yaml files
Put every ur specific urdf into a common one
Modify launch files and bringups according to the new parameters

Modifications:

3 new yaml files to configure the robots parameters one for each robot model / urdf (joints_limits, physical, visual)
ur.common.xacro contains the parsing of the yaml files
ur.geometry contains the geometry of the robot
DOESN'T CONTAINS JOINT LIMITED ANYMORE
Bringups launch file and tests changes accordingly.

This PR doesn't apply to ur-e series (WIP in another PR)

gavanderhoorn · 2018-07-26T14:07:44Z

Nice to see some work in this direction.

Some first suggestions / comments:

give the .yaml files a default location, and use default args in the xacro macro to load the files from there. Only if users want to use custom settings they'd have to provide an alternative location for the .yaml file. That would remove the need for the urN_robot_yaml.urdf.xacro files completely, which would add yet another (confusing) set of xacros to the package.
(this is a suggestion / debatable): do away with the in-xacro constants completely: so all D-H derived constants, all limits, mesh locations, link lengths, everything. Place all of them in .yaml files, one for each robot variant. Rationale: preparation for when we can use the calibration data from each individual UR in the xacro (How to add calibration.conf data from controller to URDF for maximum accuracy #357) and we essentially end up with a single macro for all robots that gets parameterised by the data in the .yaml files. This could allow us to simplify things significantly.
if we do this: also remove the limit args on the urN_robot macros: all limits can be set using the .yaml files, no need for the args
if we don't migrate all constants to the .yaml file, let's not make the mistake of including the world frame in the main xacro files agian (Rename 'world' frame to 'robot_base' in URDF #284).
iirc, we don't need to limit all joints to [-pi, pi], just one (the elbow_joint). See Fix elbow joint limits #268.

gavanderhoorn · 2018-07-29T13:18:07Z

@ipa-led: as I used a squash-and-merge of #356 the first few commits of your PR now conflict with kinetic-devel. Sorry about that.

Could you rebase and the fix merge conflicts? That would also give you an opportunity to drop all the merge commits that Github introduced when updating your branch.

dhled · 2018-09-06T14:50:54Z

Hey @gavanderhoorn

Sorry for the delay on this one. I've made the requested changes. Please tell what you think.

If possible @ipa-nhg or @ipa-jfh can review it too.

gavanderhoorn · 2018-09-06T15:04:28Z

Starting to get there @ipa-led :)

Any reason you did not add the mesh URLs to the yaml files?

That would allow ppl to use custom meshes, which would help with #267.

gavanderhoorn · 2018-09-06T15:08:31Z

Similar question on the inertia parameters and origins.

dhled · 2018-09-06T15:45:43Z

No particular reason. It looks, indeed, that we can add more parameters (we also discussed this with Nadia). I will try to update it next week. Thank you.

v4hn · 2018-09-07T08:29:27Z

This looks like a nice change w.r.t. urdf-calibration support.

I'm not sure whether it makes sense to export everything to yaml, because you will eventually end up with a general-purpose xacro forward to yaml... unless this is exactly what you want.
Might make it harder to debug things though when setting up new systems.

But I do see the advantage to overload the specific mesh of wrist3...

nitpick: typo compute_lenght

gavanderhoorn · 2018-09-07T11:18:44Z

@v4hn wrote:

I'm not sure whether it makes sense to export everything to yaml, because you will eventually end up with a general-purpose xacro forward to yaml... unless this is exactly what you want.

yes, that is one of the possible end goals.

But nothing will be removed without leaving some time for testing.

ipa-nhg · 2018-09-20T14:37:03Z

ur_description/urdf/ur10.urdf.xacro

-        <limit lower="${shoulder_pan_lower_limit}" upper="${shoulder_pan_upper_limit}" effort="330.0" velocity="2.16"/>
-      </xacro:if>
+      <limit lower="${shoulder_pan_lower_limit}" upper="${shoulder_pan_upper_limit}"
+             effort="${shoulder_pan_effort_limit}" velocity="shoulder_pan_velocity_limit"/>


for the velocity limit the curly brackets are missed:

velocity="${ shoulder_pan_velocity_limit }"/>

ipa-nhg · 2018-09-20T14:38:48Z

ur_description/config/ur5_default.yaml

+
+offsets:
+  shoulder_offset: 0.13585 # measured from model
+  elbow_offset: -0.119 # measured from model


the elbow_offset was originally -0.1197

ipa-nhg · 2018-09-20T14:38:59Z

ur_description/config/ur5_joint_limited.yaml

+
+offsets:
+  shoulder_offset: 0.13585 # measured from model
+  elbow_offset: -0.119 # measured from model


the elbow_offset was originally -0.1197

ipa-nhg · 2018-09-20T14:55:20Z

Great job @ipa-led !!

From the technical point of view:

I tested the simulation of the 3 robots and it is working if you solve the issue I commented.

Also I generated the urdf code of the 3 models using:
rosrun xacro xacro urX_robot.urdf.xacro > urX_robot.urdf

and I compared the outcome with the outcome of the current upstream version, the only differences I found were a mistake copying the numbers, that the world link was removed and that all the xacro:property are not listed.

About the style:

I really like this solution, it is much cleaner and give us the chance to simplify the repository.

dhled · 2018-10-04T11:22:47Z

Made the change requested by @ipa-nhg

I will export also the rest of the parameters.

gavanderhoorn · 2018-10-18T09:07:32Z

This is not finished iiuc @ipa-nhg.

ipa-nhg · 2018-10-18T09:32:03Z

@gavanderhoorn what about merge this PR and create a new PR for the new changes?
The problem I see is a future conflict with https://github.com/ros-industrial/universal_robot/pull/380/files

gavanderhoorn · 2018-10-19T15:38:48Z

@gavanderhoorn what about merge this PR and create a new PR for the new changes?
The problem I see is a future conflict with https://github.com/ros-industrial/universal_robot/pull/380/files

#380 introduces new packages, so none of the files that this PR touches are changed by #380.

ipa-nhg · 2018-11-08T10:07:15Z

@gavanderhoorn I'm agree that #380 introduce new packages, but those packages have the same function as the modified here and have to be updated to follow the same structure

gavanderhoorn · 2019-04-05T08:32:36Z

Is this still something you'd like to continue with @ipa-led?

dhled · 2019-07-02T07:26:07Z

On it for the wrid19

miguelprada

In addition to the, admittedly minor, inline comments, I think this is good to go. Particularly considering it's probably been in use for a while already, via @fmauch's fork.

Apologies for letting this sit idle for so long, @ipa-led.

miguelprada · 2020-02-07T09:11:05Z

ur_bringup/launch/ur10_bringup.launch

+  <!-- robot model parameters files-->
+  <arg name="joints_limits_params" default="$(find ur_description)/config/ur10/joints_limits_parameters.yaml"/>
+  <arg name="physical_params" default="$(find ur_description)/config/ur10/physical_parameters.yaml"/>
+


Any particular reason not to expose the visual_params argument to the ur..._upload.launch files?

not really (or at least I don't remember why I did that) ... I will change that

miguelprada · 2020-02-07T09:13:41Z

ur_description/launch/view_ur10e.launch

@@ -1,10 +1,10 @@
 <?xml version="1.0"?>
 <launch>
-  <include file="$(find ur_e_description)/launch/ur10e_upload.launch"/>
+  <include file="$(find ur_description)/launch/ur10e_upload.launch"/>

  <node name="joint_state_publisher" pkg="joint_state_publisher" type="joint_state_publisher" >
    <param name="use_gui" value="true"/>
  </node>
  <node name="robot_state_publisher" pkg="robot_state_publisher" type="state_publisher" />


Not quite part of this PR, but this should probably be replaced to use robot_state_publisher instead of the deprecated state_publisher.

miguelprada · 2020-02-07T09:16:40Z

ur_description/launch/ur_common.launch

+  <arg name="safety_pos_margin" default="0.15" doc="The lower/upper limits in the safety controller" />
+  <arg name="safety_k_position" default="20" doc="Used to set k position in the safety controller" />
+
+  <param name="robot_description" command="$(find xacro)/xacro '$(find ur_description)/urdf/ur_robot.urdf.xacro' --inorder 


Since this targets melodic-devel, shouldn't --inorder be dropped?

* default in melodic

dhled · 2020-03-04T15:01:27Z

@fmauch @miguelprada @gavanderhoorn

I should have added feature to solve Miguel's comment (sorry, took me way too long).

gavanderhoorn · 2020-03-19T15:47:01Z

@ipa-led: picking this up again. I've just changed the target to a new melodic-devel-staging, to avoid users using melodic-devel and finding it in a broken state (we have a nr of other changes coming in from #448).

We'll merge this as-is now.

Thanks for iterating and for the (enormous) amount of effort. I believe this is definitely an improvement over the current state of things. We trade N almost identical macros for a single one with a nr of arguments. I believe that will scale better wrt maintenance, but let's see.

gavanderhoorn · 2020-03-19T16:11:06Z

@ipa-led: could you clarify why you've chosen to separate the parameters for the various parts of the models into different files? There are different files for the appearance and physical parameters. I'm trying to understand whether it would make sense to merge those two.

The joint limits can certainly be kept separate: if we restructure/extend the file a little bit, it could also be used directly by MoveIt (instead of the separate file that's now hosted in the config/ sub dir of the MoveIt cfg packages).

dhled · 2020-03-20T07:52:49Z

@ipa-led: could you clarify why you've chosen to separate the parameters for the various parts of the models into different files? There are different files for the appearance and physical parameters. I'm trying to understand whether it would make sense to merge those two.

I think at that time I have separated by "concerns" (graphical, physicial, limits) but maybe it doesn't make sense to split. It just that maybe people wants to modify one file and ensuring that it doesn't impact the others.

The joint limits can certainly be kept separate: if we restructure/extend the file a little bit, it could also be used directly by MoveIt (instead of the separate file that's now hosted in the config/ sub dir of the MoveIt cfg packages).

I will have a look at that :)

gavanderhoorn · 2020-03-20T09:50:03Z

You don't have to add anything to this PR. We'll merge into the staging branch and can then continue there with follow-up PRs.

fmauch · 2020-05-05T11:07:45Z

Anything happening here recently?

lianghongzhuo · 2020-05-05T18:33:38Z

Can we please push this forward?

dhled · 2020-05-06T08:44:14Z

Anything happening here recently?

I don't know if we are waiting for this be merge or for me to add other things.

v4hn · 2020-05-06T09:45:00Z

@gavanderhoorn twice now you wrote you will merge this as-is, but that was two months ago.
Are you going to do so?

miguelprada · 2020-05-07T07:53:10Z

Sorry, I had assumed this was already merged. Apologies for letting it sit for so long.

Any reason not to merge already, @gavanderhoorn, @ipa-nhg?

lianghongzhuo · 2020-05-29T10:00:49Z

What's going on here?

ipa-nhg

LGTM 👍 Thanks @ipa-led

gavanderhoorn

Merging this into staging branch.

gavanderhoorn · 2020-06-06T09:08:33Z

Thanks @ipa-led 👍

Great work and I believe this will greatly simplify maintenance of these packages in the future.

🤖 🇫🇷

dhled mentioned this pull request Jul 26, 2018

suggestion: use args for ur_description #234

Open

gavanderhoorn mentioned this pull request Jul 29, 2018

Fix overlapping properties + allow namespacing #365

Closed

dhled force-pushed the ur_description_args branch from 9e8cf62 to d59b16a Compare September 6, 2018 14:38

gavanderhoorn mentioned this pull request Sep 6, 2018

Support for Xamla's rounded cap (ur5) #267

Closed

ipa-nhg requested changes Sep 20, 2018

View reviewed changes

ipa-nhg previously approved these changes Oct 18, 2018

View reviewed changes

dhled dismissed ipa-nhg’s stale review via 200c1cc December 17, 2018 14:28

dhled force-pushed the ur_description_args branch 2 times, most recently from b6118e0 to ede7c5a Compare December 17, 2018 14:35

fmauch mentioned this pull request Mar 29, 2019

Calibrated URDF #414

Closed

gavanderhoorn added the wrid19 label Jun 29, 2019

gavanderhoorn assigned dhled Jul 2, 2019

dhled force-pushed the ur_description_args branch 2 times, most recently from 9320eda to b7d9f88 Compare July 2, 2019 10:53

fmauch mentioned this pull request Jan 21, 2020

Use upstream repo, not fmauch/universal_robot UniversalRobots/Universal_Robots_ROS_Driver#84

Closed

fmauch mentioned this pull request Jan 29, 2020

Add Ur16e support #477

Merged

7 tasks

miguelprada reviewed Feb 7, 2020

View reviewed changes

Ludovic Delval added 3 commits March 4, 2020 15:55

added visual parameters to bringup launch

bcd3e8a

removed --inorder for common launch

7be56f5

* default in melodic

used robot_state_publisher instead of state_publisher

8896cdf

gavanderhoorn changed the base branch from melodic-devel to melodic-devel-staging March 19, 2020 15:42

fmauch mentioned this pull request Mar 30, 2020

UR and URe series cannot be used alongside each other UniversalRobots/Universal_Robots_ROS_Driver#149

Closed

ipa-nhg approved these changes May 29, 2020

View reviewed changes

gavanderhoorn removed the wrid19 label Jun 1, 2020

gavanderhoorn approved these changes Jun 6, 2020

View reviewed changes

gavanderhoorn merged commit 0eed4d7 into ros-industrial:melodic-devel-staging Jun 6, 2020

This was referenced Jun 6, 2020

Add Ur16e support (rebased on-top of melodic-devel-staging) #496

Closed

Misc updates to ur_description #497

Merged

This was referenced Jun 16, 2020

description: use mat def from yaml for base_link. #509

Merged

description: add missing safety_ctrlr for shoulder pan. #514

Merged

fmauch mentioned this pull request May 25, 2021

this package is outdated with calibration_devel branch #573

Closed

gavanderhoorn mentioned this pull request Sep 23, 2021

Parameter naming in joint_limits.yaml UniversalRobots/Universal_Robots_ROS2_Driver#168

Closed

Urdf with args and yaml configuration #371

Urdf with args and yaml configuration #371

Conversation

dhled commented Jul 26, 2018 • edited Loading

gavanderhoorn commented Jul 26, 2018 • edited Loading

gavanderhoorn commented Jul 29, 2018

dhled commented Sep 6, 2018

gavanderhoorn commented Sep 6, 2018

gavanderhoorn commented Sep 6, 2018

dhled commented Sep 6, 2018

v4hn commented Sep 7, 2018

gavanderhoorn commented Sep 7, 2018

ipa-nhg Sep 20, 2018

Choose a reason for hiding this comment

ipa-nhg Sep 20, 2018

Choose a reason for hiding this comment

ipa-nhg Sep 20, 2018

Choose a reason for hiding this comment

ipa-nhg commented Sep 20, 2018

dhled commented Oct 4, 2018

gavanderhoorn commented Oct 18, 2018

ipa-nhg commented Oct 18, 2018

gavanderhoorn commented Oct 19, 2018

ipa-nhg commented Nov 8, 2018

gavanderhoorn commented Apr 5, 2019

dhled commented Jul 2, 2019

miguelprada left a comment

Choose a reason for hiding this comment

miguelprada Feb 7, 2020

Choose a reason for hiding this comment

dhled Feb 10, 2020

Choose a reason for hiding this comment

miguelprada Feb 7, 2020

Choose a reason for hiding this comment

miguelprada Feb 7, 2020

Choose a reason for hiding this comment

dhled commented Mar 4, 2020 • edited Loading

gavanderhoorn commented Mar 19, 2020

gavanderhoorn commented Mar 19, 2020

dhled commented Mar 20, 2020 • edited Loading

gavanderhoorn commented Mar 20, 2020

fmauch commented May 5, 2020

lianghongzhuo commented May 5, 2020

dhled commented May 6, 2020

v4hn commented May 6, 2020

miguelprada commented May 7, 2020

lianghongzhuo commented May 29, 2020

ipa-nhg left a comment

Choose a reason for hiding this comment

gavanderhoorn left a comment

Choose a reason for hiding this comment

gavanderhoorn commented Jun 6, 2020

dhled commented Jul 26, 2018 •

edited

Loading

gavanderhoorn commented Jul 26, 2018 •

edited

Loading

dhled commented Mar 4, 2020 •

edited

Loading

dhled commented Mar 20, 2020 •

edited

Loading