-
Notifications
You must be signed in to change notification settings - Fork 485
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
Add jerk to the robot model #683
Conversation
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.
lgtm
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 think we should also update computeVariableBoundsMsg and operator<<(std::ostream& out, const VariableBounds& b) to include the new fields
I released moveit_msgs an hour ago but we need to add it to the .repos
file until it gets synced to the Debians
4a23e2a
to
8888cc6
Compare
Changes look good to me. Let's hold off on merging this until the MoveIt release is done. |
91d569f
to
3d2ac57
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.
Could you add jerk to the RobotTrajectory::print()
function (#715) right below efforts? LGTM after that
Codecov Report
@@ Coverage Diff @@
## main #683 +/- ##
==========================================
- Coverage 57.92% 54.15% -3.77%
==========================================
Files 310 195 -115
Lines 26091 20343 -5748
==========================================
- Hits 15111 11015 -4096
+ Misses 10980 9328 -1652
Continue to review full report at Codecov.
|
- acceleration_[robot_model_->getVariableIndex(it.first)] = it.second;
+ effort_[robot_model_->getVariableIndex(it.first)] = it.second;
A bonus bug fix!
Nice. Luckily not an actual bug because until now the two pointers are assigned to the same value. :)
|
Not an actual bug because both arrays share the same memory. As mentioned in moveit/moveit2#683 (review)
Not an actual bug because both arrays share the same memory. As mentioned in moveit/moveit2#683 (review)
2c107c5
to
861575b
Compare
I see that I'll plan to leave it as it was. |
I see that `has_acceleration_` and `has_effort_` were mutually exclusive before. I'm not sure why
I don't know for sure, but I believe this was done to reduce the memory footprint of the class. I'm not convinced it's an issue these days, but then again manipulation piplines such as MTC instantiates a lot of instances.
Allocating effort and jerk by default for [a bimanual shadow hand setup](https://www.youtube.com/watch?v=rEoq7DMgaEc) would add `(6+23)*2 * 2 * 8 = 928` bytes for each instance. As long as the memory is not accessed you might rely on virtual memory to not waste RAM, but then again I still think it's bad design.
Also I've never seen anyone use the effort fields myself, so it might be quite some deadweight.
Planning a geometric trajectory for effort control ahead of time is probably quite meaningless in practice anyway.
I would expect it to be most useful for *recordings* of actual executions, but then I would not want to give up acceleration in a recording either.
Jerk will also remain a special case for some time I guess. I'm wondering whether it would be better to reallocate memory if effort or jerk are used or maybe allocate these fields separately from the rest of the values as needed.
This needs thorough profiling though, possibly in an MTC setup or some other complex pipeline ***@***.*** might be pretty interested as he also maintains a big parallel planning system - in ROS though).
|
@v4hn thanks for a critical review that looks at the bigger picture. I only modified the RobotState so I could add jerk to |
6afab4a
to
3a4e174
Compare
This pull request is in conflict. Could you fix it @AndyZe? |
1 similar comment
This pull request is in conflict. Could you fix it @AndyZe? |
3a4e174
to
65b2dad
Compare
Now that we have some MoveIt capabilities that can process jerk limits, I think we should add jerk limits to the robot model. This simply adds the jerk parameters to joint_limits.yaml and adds the parsing code to robot_model_loader.cpp.
Fixes #574