Skip to content

Allow position control limits outside [-pi,pi]#477

Closed
scpeters wants to merge 1 commit intoros-controls:melodic-develfrom
scpeters:position_controllers_large_limits
Closed

Allow position control limits outside [-pi,pi]#477
scpeters wants to merge 1 commit intoros-controls:melodic-develfrom
scpeters:position_controllers_large_limits

Conversation

@scpeters
Copy link
Copy Markdown

@scpeters scpeters commented Apr 1, 2020

Several joint position controllers use the angles::shortest_angular_distance_with_limits function for REVOLUTE joints to compute the position error, but this function only supports joint limits within the interval [-pi, pi].

The angles::shortest_angular_distance_with_large_limits function supports limits outside the interval [-pi, pi], which I think is more suitable for this computation. A difference from the other API is that the lower limit is required to be smaller than the upper limit.

Closes #261.

Several joint position controllers use the
angles::shortest_angular_distance_with_limits
function for REVOLUTE joints to compute the position error,
but this function only supports joint limits within the
interval [-pi, pi].
The angles::shortest_angular_distance_with_large_limits
function supports limits outside the interval [-pi, pi],
which I think is more suitable for this computation.
Fixes ros-controls#261.
Copy link
Copy Markdown
Member

@matthew-reynolds matthew-reynolds left a comment

Choose a reason for hiding this comment

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

LGTM but I think this should target Noetic since it changes behaviour. If people are taking advantage of the wrapping that angles::shortest_angular_distance_with_limits() does, then requiring lower < upper might make a mess for them.

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Apr 4, 2020

#466 ?

@scpeters
Copy link
Copy Markdown
Author

scpeters commented Apr 4, 2020

LGTM but I think this should target Noetic since it changes behaviour.

I don't think this repository has a noetic-devel branch yet

@scpeters
Copy link
Copy Markdown
Author

scpeters commented Apr 4, 2020

#466 ?

Oops, you're right I didn't notice that when I made this PR. I believe this PR matches what is recommended in the following comment thread: #466 (comment)

As I look back further, #264 is also related, but I think it predates the angles::shortest_angular_distance_with_large_limits function.

@matthew-reynolds
Copy link
Copy Markdown
Member

#466 ?

Whoops, I already forgot about that guy. Thanks @bmagyar :)

@scpeters
Copy link
Copy Markdown
Author

scpeters commented Apr 8, 2020

LGTM but I think this should target Noetic since it changes behaviour.

I don't think this repository has a noetic-devel branch yet

bump @bmagyar; if this should target noetic-devel, can you create that branch?

@scpeters
Copy link
Copy Markdown
Author

scpeters commented Apr 8, 2020

never mind, #466 is a better version of this since it has a test.

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Apr 9, 2020

@matthew-reynolds while I agree that it changes behaviour slightly, some may say this is a bug, not a feature, even if people found ways to work around it. I think it should still go into melodic

@matthew-reynolds
Copy link
Copy Markdown
Member

@bmagyar Yep, you're right. I also forgot that the joint limit logic requires lower < upper, so if people have upper < lower they've already got problems. Definitely good for Melodic.

@bmagyar
Copy link
Copy Markdown
Member

bmagyar commented Apr 16, 2020

#466 was merged, closing this

@bmagyar bmagyar closed this Apr 16, 2020
@scpeters scpeters deleted the position_controllers_large_limits branch April 16, 2020 21:09
@swinterbotix
Copy link
Copy Markdown

Not sure if this is the right place to put this, but...

I've literally just ran into this exact issue on a robot that I'm working with. Gazebo behaves nicely when using effort_controllers/JointTrajectoryController, but not so much when using effort_controllers/JointPositionController. In my URDF, I specify limits for the revolute joint in question as...

<limit effort="100" lower="${-pi}" upper="${pi}" velocity="${pi}"/>

...but this causes the joint to rotate to pi right after the controllers are loaded and oscillate like mad around pi a few degrees. However, if I redefine the limits to...

<limit effort="100" lower="-3.14" upper="3.14" velocity="${pi}"/>

...the problem goes away. My guess is that there is a tiny bit of overlap between ${-pi} and ${pi} that's causing this issue. Not sure how this should be addressed, but thought I'd point it out.

I'm currently using Ubuntu 16.04 and ROS Kinetic.

@francofusco
Copy link
Copy Markdown
Contributor

francofusco commented Apr 27, 2020

Not sure about the joint moving to PI in the very beginning, but I was indeed experiencing oscillations when limits were slightly overlapping. The changes proposed here and in #466 should fix the problem - though I think they have been merged only in the melodic branch, not kinetic...

If you need a quick fix, perhaps the best is to clone the controllers locally and modify them manually to use angular_distance_with_large_limits instead of angular_distance_with_limits

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.

Don't wrap angle differences optionally in joint position controllers

5 participants