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

Fix overlapping variable names between robot definition files #356

Conversation

felixvd
Copy link
Contributor

@felixvd felixvd commented Jun 21, 2018

Uploading multiple different UR definitions currently breaks the previous robot definitions by overwriting their link lengths and other parameters. This commit makes the property names for each robot model unique so they are independent of the others.

See the before and after pictures in Gazebo below. The UR5 was the last model to be loaded in the scene definition, so the link lengths of the UR3 have become too long, and those of the UR10 too short.

gazebo-before

gazebo-after

I don't think this change affects anything outside of these files and I have had no problems using it in our own development.

Uploading a different UR definition used to break the previous definition files by overwriting their link lengths. This commit makes the property names for each robot unique so they are independent of the other robot models.
@gavanderhoorn
Copy link
Member

Thanks for the PR. This is indeed an issue.

Instead of renaming all the properties, would moving their declarations down a bit so they are inside the ur10_robot macro definition be an option?

According to wiki/xacro - Local properties:

Properties and macros defined within a macro are local to this macro, i.e. not visible outside.

@atomoclast
Copy link

One possible suggestion too is to namespace each robot. Either way this would be a great PR!

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 25, 2018

@atomoclast: moving the declarations into the macro def should automatically achieve this. The properties should essentially get namespaced that way.

This is the cleaner solution for what the previous commit was meant to solve
@felixvd
Copy link
Contributor Author

felixvd commented Jun 26, 2018

I agree, that's the cleaner way. I updated the PR accordingly and took the liberty to remove the prefix from the kinematic parameters, too. I confirmed that it works in our simulation.

@dhled
Copy link

dhled commented Jul 26, 2018

Tested on simulation.

@Chamango90
Copy link

Reviewed and accepted

@gavanderhoorn
Copy link
Member

Accepting and merging this will break any urdfs that are currently directly using any of the xacro:property elements, so we will have to increase the package's version to convey that fact and post an announcement on ROS Discourse about it.

As the constants are 'almost' internal constants, I don't expect that to have happened to often, but we can still be clear about it.

For future readers: ${namespace.property} can be used to access properties inside namespaces. See also the xacro documentation on the wiki page linked to in #356 (comment).

Copy link
Member

@gavanderhoorn gavanderhoorn left a comment

Choose a reason for hiding this comment

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

Approving based on reviews by @ipa-led and @ipa-jfh.

@gavanderhoorn gavanderhoorn merged commit ff58d33 into ros-industrial:kinetic-devel Jul 29, 2018
AustinDeric pushed a commit to AustinDeric/universal_robot that referenced this pull request Nov 11, 2018
…dustrial#356)

Squashed commits:

* Fix overlapping properties between robot definitions

   Uploading a different UR definition used to break the previous definition files by overwriting their link lengths. This commit makes the property names for each robot unique so they are independent of the other robot models.

* Move robot-specific parameters inside macro

   This is the cleaner solution for what the previous commit was meant to solve
dhled pushed a commit to dhled/universal_robot that referenced this pull request Dec 17, 2018
…dustrial#356)

Squashed commits:

* Fix overlapping properties between robot definitions

   Uploading a different UR definition used to break the previous definition files by overwriting their link lengths. This commit makes the property names for each robot unique so they are independent of the other robot models.

* Move robot-specific parameters inside macro

   This is the cleaner solution for what the previous commit was meant to solve
ipa-nhg pushed a commit to ipa-nhg/universal_robot that referenced this pull request Jul 2, 2019
…dustrial#356)

Squashed commits:

* Fix overlapping properties between robot definitions

   Uploading a different UR definition used to break the previous definition files by overwriting their link lengths. This commit makes the property names for each robot unique so they are independent of the other robot models.

* Move robot-specific parameters inside macro

   This is the cleaner solution for what the previous commit was meant to solve
ipa-nhg pushed a commit to ipa-nhg/universal_robot that referenced this pull request Jul 2, 2019
…dustrial#356)

Squashed commits:

* Fix overlapping properties between robot definitions

   Uploading a different UR definition used to break the previous definition files by overwriting their link lengths. This commit makes the property names for each robot unique so they are independent of the other robot models.

* Move robot-specific parameters inside macro

   This is the cleaner solution for what the previous commit was meant to solve
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants