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

Parameter naming in joint_limits.yaml #168

Closed
christianlandgraf opened this issue Sep 16, 2021 · 5 comments · Fixed by #187
Closed

Parameter naming in joint_limits.yaml #168

christianlandgraf opened this issue Sep 16, 2021 · 5 comments · Fixed by #187

Comments

@christianlandgraf
Copy link

Hi,

when trying to test the Pilz Planner in ROS 2 with the UR, I noticed joint_limits.yaml in the ROS 2 driver specifies the limits with regard to a generic name (e.g. wrist_3, wrist_2 etc) instead of the actual joint name (e.g. wrist_3_joint, wrist_2_joint) in the urdf.

See ROS 1 and ROS 2

Is there any reason for changing this?
Since the Pilz motion planner checks if velocity limits (and acceleration limits) exist for each joint in a planning group, it does not correctly load the parameters (e.g. here).

I didn't encounter a problem somewhere else in MoveIt, so far, but theoretically it could appear in other parts as well?

@destogl
Copy link
Contributor

destogl commented Sep 23, 2021

This seems rather as an issue with wrong configuration of joint to plan for. You have to specify them somewhere with the suffix _joint. Please try to check this.

@livanov93, @urrsk do you think we should change names as they were in ROS1?

@livanov93
Copy link
Contributor

livanov93 commented Sep 23, 2021

@destogl The thing is that links that have been compared are not from the same packages. The ROS1 version that @christianlandgraf has referenced is from urXYZ_moveit_config package and the one ROS2 version referenced is in ur_description package.
If we take a look in ROS1 version of ur_description it is obvious it has the same form (without _joint suffix).

The ones from the description package are used for propagating parameters to robot_description via yaml file (the final steps can be seen here) and the other one is used as parameters in moveit planning (see example here).

I propose following action(s):

If we immediately change our ur_description joint limits yaml, we will have to change some of the core xacros in the same package which is actually shared all over the ROS1 and ROS2 community at the moment.

@urrsk
Copy link
Member

urrsk commented Sep 23, 2021

In the past, people have limit the robot actually joint limits used for planning to help moveit. This might be the reason why the are named differently.

In addition, I can see that the angle limits for UR3's wrist 3 is set to ±2pi, though it is in reality unlimited.
https://github.com/UniversalRobots/Universal_Robots_ROS2_Driver/blob/main/ur_description/config/ur3e/joint_limits.yaml

@destogl
Copy link
Contributor

destogl commented Sep 23, 2021

In addition, I can see that the angle limits for UR3's wrist 3 is set to ±2pi, though it is in reality unlimited.

We should remove this. Though this could be useful for MoveIt to reduce planning space. Sometimes algorithms "get lost" on infinite joints.

@gavanderhoorn
Copy link
Contributor

gavanderhoorn commented Sep 23, 2021

I'd suggest to carefully consider whether to change these kinds of things.

Especially joint limits have been configured over time to reduce problems with motion planners (MoveIt being one of them). Hardware stating limits have certain values does not mean they are always useful (the elbow joint comes to mind).

Infinite limits (or continuous) joints can be nice, but infinite planning spaces typically do not always result in better plans. Users targetting applications such as screwing or drilling would probably not control that action using the driver, making the need to 'unlimit' wrist 3 even less.

I would attribute the discrepancy between joint names to a simple mistake. In the early stages of the ROS 2 driver, things have been copied from upstream packages without much discussion. The .yaml file being discussed was not intended to be used as joint limits for MoveIt, but instead only as input for the respective .xacro files. The xacro:macro didn't use the full joint names as it didn't need to.

For context:

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 a pull request may close this issue.

5 participants