-
Notifications
You must be signed in to change notification settings - Fork 222
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 sjtc to correctly aborts on violation of constraints #810
Conversation
@destogl as discussed this contains our updates on the SJTC. That could be a base for re-addressing ros-controls/ros2_controllers#301. |
60a0acd
to
39b5d60
Compare
39b5d60
to
2932698
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could also be nice to add a test that verifies that you can about a trajectory as well with the SJTC
if (normalize_joint_error_[index]) { | ||
// if desired, the shortest_angular_distance is calculated, i.e., the error is | ||
// normalized between -pi<error<pi | ||
error.positions[index] = angles::shortest_angular_distance(current.positions[index], desired.positions[index]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When should this be actual. I think we always would like to catch the absolute angle. More or less all the UR joints have a range of +-2pi if the physically and UR3e's wrist3 joint can rotation infinty of resolutions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That particular line is (as most of the update function) from the upstream JTC. This is used only if
- closed loop control is activated.
angle_wraparound
is set totrue
in the controller configuration.
Both isn't true inside our controller configuration and also has to be explicitly activated when desired. I would vote for sticking as close to the upstream code as possible because
a) we want to make our lives easy to adapt to future changes
b) we want to switch to the upstream JTC as soon as the scaling feature got merged there.
you mean preempt? Yes, we could add that, as well. |
Yes, to check that the about function also works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall it looks very good. I have verified that the trajectory is aborted correctly and that the speed scaling works.
|
Merging since the necessary changes in ros2_controllers have just been released. |
This aims at fixing both #800 and #804.
In a first step I'd like to add a test reproducing the error reported in #800 and add a fix afterwards which I assume to be the same fix as #804 requests.