Patch/gripper in step action - #57
Conversation
| } | ||
|
|
||
| /** Remap stretch_gripper_joint → gripper_joint (+ finger_rad → pct) for trajectory goals. */ | ||
| function remapGripperForTrajectory( |
There was a problem hiding this comment.
Remapped operator stretch_gripper_joint (finger rad) → driver gripper_joint (pct) for FollowJointTrajectory.
| * pct-vs-world-rad units). Body first, then gripper-only, matches the path | ||
| * that Step Action already uses successfully. | ||
| */ | ||
| private async runBodyThenGripperTrajectories( |
There was a problem hiding this comment.
Split pose playback into a) body trajectory and b) a gripper-only goal.
There was a problem hiding this comment.
What's the rationale behind this? Are there errors if the gripper tracks a trajectory at the same time as other joints?
There was a problem hiding this comment.
Yes — mixed gripper_joint + body FollowJointTrajectory goals time out. The driver commands gripper in pct but monitors world-rad pos, so the 3s action_timeout fires even after the hand has arrived
There was a problem hiding this comment.
I wonder if this will be patched by @hello-garv here: https://github.com/hello-robot/stretch4_ros2/pull/26/changes
It looks like GripperCommandGroup::monitor_execution() is updated to fix this with the new gripper conventions.
In general, there are several bugs with the gripper behavior that we could either patch with the currently released software or patch after incorporating the PRs that are landing soon across the stack. If the gripper PRs will merge within a few days, I'm inclined to say that we should wait to fix this until those PRs get merged and we can update / test accordingly.
There was a problem hiding this comment.
@hello-lamsey Oh ok. I may be able to wrap up this PR, but maybe I should pause?
@hello-garv Do you know if your PR will land in main soon?
| .filter((pose) => Object.keys(pose).length > 0); | ||
| } | ||
|
|
||
| function finalGripperPose(poses: RobotPose[]): RobotPose | undefined { |
There was a problem hiding this comment.
Multi-pose recordings keep the full body path but only the last gripper aperture.
There was a problem hiding this comment.
What's the rationale for this? Users may want to record multiple gripper poses within a recording. For example, opening the gripper, extending the arm, and then closing the gripper to grab something.
There was a problem hiding this comment.
Recording stores gripper on each waypoint when Hand is selected. During playback finalGripperPose / stripGripperFromPoses are gone, and every waypoint runs through the same body-then-gripper-only path as a single pose goal. Mid-path open > extend > close is preserved including a second play from the end pose.
| setTimeout(() => { | ||
| this.setButtonInactiveState(buttonPadFunction); | ||
| this.setBaseVelocity(0.0, 0.0, 0.0); | ||
| if (baseButtonPadFunctions.has(buttonPadFunction)) { |
There was a problem hiding this comment.
Step Action only calls setBaseVelocity(0) for base buttons now.
| translate_mobile_base: [-30.0, 30.0], | ||
| rotate_mobile_base: [-3.14, 3.14], | ||
| stretch_gripper_joint: [-0.37, 0.17], | ||
| stretch_gripper_joint: [0.0, 0.425], |
There was a problem hiding this comment.
@hello-garv does this remap match the new gripper limits convention?
There was a problem hiding this comment.
The hard coded joint limits should be removed entirely and pulled from the driver with the next update!
| * GripperConversion.get_status (aperture_open_m / finger_length_m). | ||
| */ | ||
| const GRIPPER_TRAJECTORY_JOINT = "gripper_joint"; | ||
| const GRIPPER_PCT_CLOSED = -100; |
There was a problem hiding this comment.
Also @hello-garv will this change once we merge your new stretch4_body PR for grippers?
There was a problem hiding this comment.
Yes, neither of these things should need to be hard coded.
There was a problem hiding this comment.
@hello-garv Would it be possible to get this PR into main and then when the stuff in the other repo lands we do a smaller PR patch?
hello-lamsey
left a comment
There was a problem hiding this comment.
Most functionality seems to work as expected. Two changes, one for sure and one maybe beyond the scope of this PR:
- For sure - record gripper positions throughout a movement recording instead of keeping only the final gripper pose for the whole trajectory. Disallowing recording multiple gripper poses prevents users from teaching grasp / release sequences.
- Maybe - assess the unit conversions used for the gripper in anticipation of upcoming changes to
stretch4_body(hello-robot/stretch4_body#64)
I'll work on this. I def think you're right that it should behave this. I'll keep you posted
I'll let @hello-garv respond to this one because I think that was directed to her! |
| return "success"; | ||
| } | ||
|
|
||
| private async runSplitPose(pose: RobotPose): Promise<TrajectoryOutcome> { |
There was a problem hiding this comment.
Shared the body-then-gripper split between single-pose and playback.
| alert_type: "error", | ||
| }); | ||
| } | ||
| for (const pose of poses) { |
There was a problem hiding this comment.
Dropped the already-at-final-pose skip so a second play from the end pose still walks every waypoint.
|
@hello-lamsey I was able to just finish this PR because it was relatively straightforward. Anna's work may collide with this stuff but, I figured that I might as well ship this so that it can be in a more solid place to pause it. |
|
Superseded by #66 |
Description
This PR fixes bug reported by @hello-lamsey #54.
Opening and closing gripper was not working in Step Action mode. This PR also addresses closely related bugs like should the visual states when gripper is opened/closed at its limit.
Testing procedure
Before opening a pull request
From the top-level of this repository, run:
pre-commit run --all-filesTo merge
Squash & Merge