Pilz multi-group incompatibility #1856
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #1856 +/- ##
==========================================
- Coverage 50.53% 50.51% -0.02%
==========================================
Files 386 386
Lines 31736 31730 -6
==========================================
- Hits 16034 16024 -10
- Misses 15702 15706 +4
☔ View full report in Codecov by Sentry. |
moveit_planners/pilz_industrial_motion_planner/src/trajectory_functions.cpp
Outdated
Show resolved
Hide resolved
| moveit::core::RobotState rstate(robot_model); | ||
| // By setting the robot state to default values, we basically allow | ||
| // the user of this function to supply an incomplete or even empty seed. | ||
| rstate.setToDefaultValues(); |
There was a problem hiding this comment.
As stated here, we would drop support for incomplete states by removing this line. This is not necessarily a blocker, though. But I agree that it would be unsafe to assume that default values are given as long as the other groups are not initialized. If the user wants to ignore other groups, this should be done by providing a fixed ACM. We should check that this info is not lost or that it won't conflict with the tutorials.
There was a problem hiding this comment.
It's also problematic that we can't check for a complete state here since there is no access to the PSM. Maybe we should enforce this more on the runtime interface side: MoveItCpp/MoveGroup could run "CurrentStateMonitor::waitForCompleteState()" by default.
There was a problem hiding this comment.
As stated here, we would drop support for incomplete states by removing this line. This is not necessarily a blocker, though. But I agree that it would be unsafe to assume that default values are given as long as the other groups are not initialized. If the user wants to ignore other groups, this should be done by providing a fixed ACM. We should check that this info is not lost or that it won't conflict with the tutorials.
I think that we are still supporting incomplete and even empty seed with the only difference that joint values not specified are set from the current robot state instead of the default one.
It's also problematic that we can't check for a complete state here since there is no access to the PSM. Maybe we should enforce this more on the runtime interface side: MoveItCpp/MoveGroup could run "CurrentStateMonitor::waitForCompleteState()" by default.
Totally agree with you on this point.
There was a problem hiding this comment.
Can we open follow-up issues for this?
|
This PR is stale because it has been open for 45 days with no activity. Please tag a maintainer for help on completing this PR, or close it if you think it has become obsolete. |
github-actions
bot
added
the
stale
sjahr
added
backport-humble
(cherry picked from commit 5bbe21b)
Description
The current pilz implementation does not support a robot with multiple groups. In particular the problem is that in internal FK and IK functions, the whole state of the robot is set to default values. By doing so, the state of the other groups (that are not the one that is currently moved) does not match the current state of the robot (if those groups are not in default value).
The proposed solution consists in using the current robot state provided by the planning scene.