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

Rename 'world' frame to 'robot_base' in URDF #284

Conversation

davetcoleman
Copy link
Contributor

The transform between base_link and world is currently located in the URDF:

  <link name="world" />

  <joint name="world_joint" type="fixed">
    <parent link="world" />
    <child link = "base_link" />
    <origin xyz="0.0 0.0 0.0" rpy="0.0 0.0 0.0" />
  </joint>

as well as a virtual_joint in MoveIt! here

    <virtual_joint name="fixed_base" type="fixed" parent_frame="world" child_link="base_link" />

Which is bad for MoveIt! and complains:

ros.rosconsole_bridge.console_bridge: Skipping virtual joint 'fixed_base' because its child frame 'base_link' does not match the URDF frame 'world'

I propose we rename the fixed link to the world in the URDF so that MoveIt! can still modify the world transform to allow the robot to move. I do not think we should remove the fixed link in the URDF because the KDL warns about the base link having inertia.

@shaun-edwards
Copy link
Member

@davetcoleman, thanks for the contribution. What do you think about using the ROS-I standard frame base?

I do wonder why that frame transform is in the URDF and what might depend on it? IK Solvers (ur_kinematics could have hard-coded frames)? Gazebo?(Gazebo expects a world frame and this might be why it's in the URDF) Any thoughts on these?

Assuming we can overcome these issues, is there a way to make this change backwards compatible(not sure I can think of one)?

@davetcoleman
Copy link
Contributor Author

What do you think about using the ROS-I standard frame base?

I tried base but something else already is using that frame... I just investigated and its the:

ROS base_link to UR 'Base' Coordinates transform

I do wonder why that frame transform is in the URDF and what might depend on it?

No telling, but its a bug with MoveIt! and in general its wrong - the URDF should not assume how the robot is connected to the rest of the world, and what the world frame is. What if you connect it to a table then the \world? What if the UR5 is on a mobile base, like many are? \world should be the origin and static, and cannot be assumed to be a static transform to the robot base.

I think this should be moved to the kinetic branch, since that is a safer place for potentially breaking changes like this.

@shaun-edwards shaun-edwards changed the base branch from indigo-devel to kinetic-devel January 5, 2017 14:57
@shaun-edwards
Copy link
Member

I updated the PR to kinetic-devel. I'm in total agreement that the world frame doesn't belong in the URDF, but it's there to support gazebo (I've now verified this). In order to remove the frame, we would need to create a URDF specific to the gazebo support packages. We do this in other ROS-I packages, here.

@fmessmer
Copy link
Contributor

fmessmer commented Jan 5, 2017

The fixed joint connecting the robot to world was needed for gazebo simulation - without it, the arm kept "rolling" around in simulation.
If you need to put the robot on mobile base or just don't want to use the world jount use the urN.urdf.xacro defining the macro staring with the robot's base link

@shaun-edwards
Copy link
Member

Welcome back @ipa-fxm!

@davetcoleman
Copy link
Contributor Author

@ipa-fmw that's a good point. If that is the case, the *_moveit_config package is pointing to the wrong URDF here because there is still a MoveIt! bug

@BrettHemes
Copy link
Member

BrettHemes commented Apr 26, 2017

@shaun-edwards and possibly @davetcoleman
Question regarding this topic in general... I understand the issue wrt the UR and the base name collision but in general when I try to use 'base' for the virtual joint in the MoveIt Steup Assistant I still get warnings at launch to the effect of
[ WARN] [1493219645.675131723]: Skipping virtual joint 'fixed_base' because its child frame 'base' does not match the URDF frame 'base_link'
It seems that MoveIt needs the child-link of the virtual joint to the world to be the root link in the robot's URDF? BUT all the ROS-I URDF's use the standard from here that put the artificial "base" link as a child of "base-link".

Am I missing something or do the "base" definitions need to be flipped to read (child/parent swapped):

<link name="${prefix}base" />

<joint name="${prefix}base_link-base" type="fixed">
  <child link="${prefix}base_link" />
  <parent link="${prefix}base" />
  <origin xyz="0 0 0" rpy="0 0 0" />
</joint>

If I modify an industrial-hosted urdf as above by flipping the child/parents of the base-link or equivalent then MoveIt! is happy...

OR... is this MoveIt! just being too picky wrt the world-to-robot virtual joint?

@gavanderhoorn
Copy link
Member

I would not use the base frame for this at all. It's really just meant to be used to be able to transform points / poses into the Cartesian coord sys of the ind controller, not for anything else.

For all MoveIt configs that I've created, I've always made base_link (or the equivalent in composite urdfs (for workcells fi)) the child of the FixedBase virtual joint.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Apr 26, 2017

@BrettHemes wrote:

BUT all the ROS-I URDF's use the standard from here that put the artificial "base" link as a child of "base-link".

Nitpicky perhaps, but base is not an "artificial base link". It's an additional frame that allows us to setup a mapping between coordinate frames (ie: TF and the one the ind controller uses). That is also the reason why base is not part of the kinematic chain (or doesn't have to be): it's almost a semantic layer on-top of the regular chain that is defined in the xacro model.

@BrettHemes
Copy link
Member

Thanks for the notes @gavanderhoorn, this helps.

@BrettHemes
Copy link
Member

I tried changing to base_link with a few moveit configs and it works as described. Using the universal_robot urdfs (urN.urdf.xacro), however, does result in a KDL warning (as Dave alluded to):

The root link base_link has an intertia specified in the URDF, but KDL does not support a root link with an inertia.  As a workaround, you can add an extra dummy link to your URDF.

Testing on real hardware there were no apparent issues despite the warning.

@gavanderhoorn
Copy link
Member

I think that warning is not really a problem in your case. Afaik, it only becomes a problem if we want to ask KDL to calculate dynamics-related things for us, which we don't.

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Sep 12, 2017

What I'd like to suggest is to create three additional xacros in ur_gazebo that essentially wrap the urN.urdf.xacros and add the world link there. Only Gazebo would see it, and it should avoid the problem with the robot floating / flying away.

Non-Gazebo-based consumers would use the urN_robot.urdf.xacros in ur_description, and these would then not include the world link. Ideally those would also not include the common.gazebo.xacro bits, but that would probably cause a lot of bw-compatibility issues.

The MoveIt configs would then use the non-Gazebo versions, which should address the issue reported by @davetcoleman.

@jacknlliu
Copy link
Contributor

just make a duplication of urN.urdf.xacro file which with world, and then just remove world. And just used this file for moveit config, is it OK?

@gavanderhoorn
Copy link
Member

gavanderhoorn commented Jun 25, 2018

The changes suggested by this PR are conceptually ok: the urdfs/xacros should not contain a world frame. That is something for a top-level/composite xacro to add to the scenegraph.

However, my suggestion would be to do something similar to what @jacknlliu proposes: the world frame, if we want to keep it around, should be moved to a wrapper xacro that could be used when the robot is loaded stand-alone (in Gazebo fi).

In other robot support packages we've done exactly that, where as much of this in delegated to Gazebo support packages (although it's not necessarily Gazebo specific of course). See abb_irb120_gazebo/urdf/irb120_3_58.xacro for an example.

Doing it this way would introduce a significant breaking change, so that would need to be a conscious decision.


Tagging this with wrid18 so if @davetcoleman isn't up for this a replacement PR could be submitted by participants.

@davetcoleman
Copy link
Contributor Author

Yes I'm not focused on UR efforts right now so would welcome someone else take this on

@matteolucchi

This comment has been minimized.

4 similar comments
@matteolucchi

This comment has been minimized.

@matteolucchi

This comment has been minimized.

@matteolucchi

This comment has been minimized.

@matteolucchi

This comment has been minimized.

Copy link

@matteolucchi matteolucchi left a comment

Choose a reason for hiding this comment

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

Hoping that this will be helpful for other people with this issue I post here the two additional urdf created following the suggestion from @jacknlliu and @gavanderhoorn . I post here the ones created for UR10

ur10_moveit.urdf.xacro to be added in /ur_description/urdf

<?xml version="1.0"?>
<robot xmlns:xacro="http://ros.org/wiki/xacro"
       name="ur10" >

  <!-- common stuff -->
  <xacro:include filename="$(find ur_description)/urdf/common.gazebo.xacro" />

  <!-- ur10 -->
  <xacro:include filename="$(find ur_description)/urdf/ur10.urdf.xacro" />

  <!-- arm -->
  <xacro:ur10_robot prefix="" joint_limited="false"/>



</robot>

ur10_joint_limited_moveit.urdf.xacro to be added in /ur_description/urdf

<?xml version="1.0"?>
<robot xmlns:xacro="http://ros.org/wiki/xacro"
       name="ur10" >

  <!-- common stuff -->
  <xacro:include filename="$(find ur_description)/urdf/common.gazebo.xacro" />

  <!-- ur10 -->
  <xacro:include filename="$(find ur_description)/urdf/ur10.urdf.xacro" />

  <!-- arm -->
  <xacro:ur10_robot prefix="" joint_limited="true"/>



</robot>

edit ur10_moveit_config/launch/planning_context.launch referencing the 2 new file just added

<!-- Load universal robot description format (URDF) -->
  <group if="$(arg load_robot_description)">
    <param unless="$(arg limited)" name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find ur_description)/urdf/ur10_moveit.urdf.xacro'" />
    <param if="$(arg limited)" name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find ur_description)/urdf/ur10_joint_limited_moveit.urdf.xacro'" />
  </group>

edit ur10_moveit_config/launch/move_group.launch adding the arg to load the robot_description

  <include file="$(find ur10_moveit_config)/launch/planning_context.launch">
    <arg name="limited" value="$(arg limited)" />
    <arg name="load_robot_description" value="true" />
  </include>

Copy link

@matteolucchi matteolucchi left a comment

Choose a reason for hiding this comment

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

Hoping that this will be helpful for other people with this issue I post here the two additional urdf created following the suggestion from @jacknlliu and @gavanderhoorn . I post here the ones created for UR10

ur10_moveit.urdf.xacro to be added in /ur_description/urdf

<?xml version="1.0"?>
<robot xmlns:xacro="http://ros.org/wiki/xacro"
       name="ur10" >

  <!-- common stuff -->
  <xacro:include filename="$(find ur_description)/urdf/common.gazebo.xacro" />

  <!-- ur10 -->
  <xacro:include filename="$(find ur_description)/urdf/ur10.urdf.xacro" />

  <!-- arm -->
  <xacro:ur10_robot prefix="" joint_limited="false"/>



</robot>

ur10_joint_limited_moveit.urdf.xacro to be added in /ur_description/urdf

<?xml version="1.0"?>
<robot xmlns:xacro="http://ros.org/wiki/xacro"
       name="ur10" >

  <!-- common stuff -->
  <xacro:include filename="$(find ur_description)/urdf/common.gazebo.xacro" />

  <!-- ur10 -->
  <xacro:include filename="$(find ur_description)/urdf/ur10.urdf.xacro" />

  <!-- arm -->
  <xacro:ur10_robot prefix="" joint_limited="true"/>



</robot>

edit ur10_moveit_config/launch/planning_context.launch referencing the 2 new file just added

<!-- Load universal robot description format (URDF) -->
  <group if="$(arg load_robot_description)">
    <param unless="$(arg limited)" name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find ur_description)/urdf/ur10_moveit.urdf.xacro'" />
    <param if="$(arg limited)" name="$(arg robot_description)" command="$(find xacro)/xacro --inorder '$(find ur_description)/urdf/ur10_joint_limited_moveit.urdf.xacro'" />
  </group>

edit ur10_moveit_config/launch/move_group.launch adding the arg to load the robot_description

  <include file="$(find ur10_moveit_config)/launch/planning_context.launch">
    <arg name="limited" value="$(arg limited)" />
    <arg name="load_robot_description" value="true" />
  </include>

@zouhbwll
Copy link

just modify the virtual_joint in the urX.srdf , let child_link match the link "world" in the urX_robot.urdf.xacro.
<virtual_joint name="fixed_base" type="fixed" parent_frame="world" child_link="world" />

I don't know why, but it worked.

@SnehalDikhale
Copy link

SnehalDikhale commented Apr 15, 2020

I modified the virtual joint in the srdf file and renamed the parent frame with 'robot_base' and the child frame with 'world'. It worked!

<virtual_joint name="virtual_joint" type="fixed" parent_frame="base_link" child_link="world" />

@gavanderhoorn
Copy link
Member

The world link is no longer included in the base robot urdfs in the new setup (currently in melodic-devel-staging). It is instead only added in the "Gazebo wrappers":

<link name="world" />
<joint name="world_joint" type="fixed">
<parent link="world" />
<child link = "base_link" />
<origin xyz="0.0 0.0 0.0" rpy="0.0 0.0 0.0" />
</joint>

And then only in the top-level .xacro file (so the macro does not include it, as should be the case).

As such, this is not needed any more for melodic-devel. kinetic-devel no longer has development focus and it would also seem undesirable to change something which has been like this for so long -- there are probably many people 'depending' on world being there.

The MoveIt configurations will be updated as part of #448, which will include the update to the virtual joint.

@gavanderhoorn
Copy link
Member

Thanks for submitting the PR @davetcoleman.

This will partly be merged into melodic-devel (partly, as the addition of the robot_base link will not be included).

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.

9 participants