-
Notifications
You must be signed in to change notification settings - Fork 938
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
Refactor Servo velocity bounds enforcement #2471
Refactor Servo velocity bounds enforcement #2471
Conversation
…y limits correctly.
Codecov Report
@@ Coverage Diff @@
## master #2471 +/- ##
==========================================
- Coverage 60.26% 60.24% -0.02%
==========================================
Files 351 351
Lines 26477 26472 -5
==========================================
- Hits 15955 15946 -9
- Misses 10522 10526 +4
Continue to review full report at Codecov.
|
The reason it scales velocities down uniformly is so the robot follows a straight line. Thus, I'm not a huge fan of this change. If you can show it's really important from a performance standpoint, we could make your new method optional. |
@jliukkonen I hope you can finish this PR! I know you're pretty busy but it's a nice simplification. |
Definitely! According to our previous tests it is essential, that you keep the direction, when scaling the velocity vector. Nevertheless I suspect, there is some problem hidden in |
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.
I added some commits for unit testing. Looks good to me, now. Thanks for a nice simplification.
Thanks for fixing some of the bugs and adding unit tests @AndyZe! I think I managed to nail all the remaining issues. I added some comments to the code, please check them out when re-reviewing. |
} | ||
const double unbounded_velocity = velocity(joint_delta_index); | ||
// Clamp each joint velocity to a joint specific [min_velocity, max_velocity] range. | ||
const auto bounded_velocity = std::min(std::max(unbounded_velocity, bounds.min_velocity_), bounds.max_velocity_); |
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.
This was wrong in my previous version. The velocities were bounded before applying velocity scaling factor so velocity was possibly scaled down twice depending on the velocity values and joint limits. First by applying velocity bounds and then by applying scaling factor to the already bounded velocities. Now the velocity bounds are only used to determine the scaling factor.
enforceVelLimits(delta_theta); | ||
|
||
// From Panda arm MoveIt joint_limits.yaml. The largest velocity limits for a joint. | ||
const double panda_max_joint_vel = 2.610; // rad/s |
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.
Since joint_limits.yaml
take precedence over the URDF the value is actually 2.61
and not 2.871
. Took me a while to understand that that was the source of errors. :)
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.
good catch there
enforceVelLimits(delta_theta); | ||
|
||
// From Panda arm MoveIt joint_limits.yaml. The largest velocity limits for a joint. | ||
const double panda_max_joint_vel = 2.610; // rad/s |
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.
good catch there
const double publish_period = parameters.publish_period; | ||
|
||
// Request joint angle changes that are too fast, given the control period in servo settings YAML file. | ||
Eigen::ArrayXd delta_theta(7); |
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.
i see you made this a 7-vector because the joint group has 7 joints. Good catch.
Small numerical errors are throwing off the unit test, for example:
Prob need to use |
I'll push a commit to do that with, say, |
8e58e45
to
fb5b233
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.
This looks much cleaner than the previous implementation.
* Refactor/fix Servo enforceVelLimits function to utilize joint velocity limits correctly. * Use uniform scaling factor instead of joint specific velocity bounds. * Move joint increment outside of if() * Add friend test for velocity limits * Clean up debug code, clang format * Test for negative velocities, too * Apply velocity scaling correctly and skip clamping zero velocities. * Improve enforceVelLimits unit tests. * Use EXPECT_NEAR in the new unit test Co-authored-by: AndyZe <andyz@utexas.edu>
* Refactor/fix Servo enforceVelLimits function to utilize joint velocity limits correctly. * Use uniform scaling factor instead of joint specific velocity bounds. * Move joint increment outside of if() * Add friend test for velocity limits * Clean up debug code, clang format * Test for negative velocities, too * Apply velocity scaling correctly and skip clamping zero velocities. * Improve enforceVelLimits unit tests. * Use EXPECT_NEAR in the new unit test Co-authored-by: AndyZe <andyz@utexas.edu>
* Refactor/fix Servo enforceVelLimits function to utilize joint velocity limits correctly. * Use uniform scaling factor instead of joint specific velocity bounds. * Move joint increment outside of if() * Add friend test for velocity limits * Clean up debug code, clang format * Test for negative velocities, too * Apply velocity scaling correctly and skip clamping zero velocities. * Improve enforceVelLimits unit tests. * Use EXPECT_NEAR in the new unit test Co-authored-by: AndyZe <andyz@utexas.edu>
See moveit/moveit2#526 for MoveIt2 patch. moveit#2471 was not sufficiently reviewed.
The
enforceVelLimits
function struck me as bit odd so I figured I make this PR to start a conversation about what I think it is doing and I why I think it looks like it might not be correct.Prior to my changes the function would calculate joint angle velocity for each joint based on the delta theta (joint angle change) and would determined the smallest scaling factor for the velocities based on the biggest out-of-bounds velocity value. So if one would have joint velocities of, say,
[1, 1, 1, 2, 3, 100]
, and each joint was bounded to[-1, 1]
range, then the scaling factor would be0.01
. That's at least how I interpreted it. This scaling factor was then applied to the delta thetas meaning if one joint received a command that would translate into very large velocity, the other joints would slow to a crawl.I felt it would make more sense and be more correct to just apply the velocity limits as they are used elsewhere in MoveIt, that is, make them upper and lower bounds for the joint velocities by clamping the velocities to
[min_vel, max_vel]
range.One thing I don't currently understand is how the delta thetas and joint bounds are matched together. Is it guaranteed that the bounds are in correct order with respect to the
delta_theta
vector and just applying them in order is enough?