propose refactor spec on gripper api - #3381
Conversation
…i future 3rd party multi-joint gripper
Greptile SummaryThe PR proposes a unified gripper API that routes gripper commands through normal task arbitration and represents gripper joints in normalized units alongside arm joints.
Confidence Score: 4/5The documentation-only PR is safe to merge, though the specification should clarify omitted gripper configuration migrations and multi-joint read behavior before implementation. The proposed architecture is coherent overall, but two specification gaps could guide subsequent implementation toward silently disabled gripper paths or an API that still cannot represent multiple measured joint positions. Files Needing Attention: dimos/hardware/GRIPPER-SPEC.md Important Files Changed
Sequence DiagramsequenceDiagram
participant Skill as Agent skill
participant Coord as Coordinator task_invoke
participant Task as GripperTask
participant Arb as Arbitration / tick loop
participant HW as ConnectedHardware
participant Adapter as Manipulator adapter
participant SDK as Vendor SDK
Skill->>Coord: set_position(normalized targets)
Coord->>Task: invoke command
Task->>Arb: emit gripper joint targets
Arb->>HW: unified all-joint command
HW->>Adapter: write_joint_positions(...)
Adapter->>SDK: convert normalized values to vendor units
Reviews (1): Last reviewed commit: "combine the meeting discussion and propo..." | Re-trigger Greptile |
| *Why the adapter.* It is already the unit boundary for every joint — | ||
| `xarm/adapter.py:222-227` converts wire radians into vendor degrees. R7 applies that | ||
| existing rule rather than inventing a second site. Any higher placement would force the | ||
| converting layer to branch on *which joints are grippers*, re-introducing the exact |
There was a problem hiding this comment.
Defaults are currently relied upon
The statement that every gripper-bearing blueprint supplies explicit values excludes keyboard_teleop_a1z and both task constructions in coordinator_teleop_dual. Following this migration guidance leaves their gripper_joint unset, so the corresponding task-level gripper commands remain disabled and implementers can overlook required configuration updates.
Knowledge Base Used: Control Coordinator and Control Tasks
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| **R24.** `_set_gripper_position()` calls `task_invoke("gripper", "set_position", {...})`. | ||
|
|
There was a problem hiding this comment.
Scalar read contradicts multi-joint support
R25 retains a scalar get_gripper() and refers to “the gripper entry,” while R31 permits multiple gripper joints. The specification therefore leaves consumers without a defined way to select or return multiple measured positions, undermining the stated multi-joint API goal.
Knowledge Base Used: Control Coordinator and Control Tasks
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #3381 +/- ##
==========================================
- Coverage 75.75% 75.36% -0.39%
==========================================
Files 1172 1184 +12
Lines 113650 116757 +3107
Branches 10272 10542 +270
==========================================
+ Hits 86093 87993 +1900
- Misses 24566 25713 +1147
- Partials 2991 3051 +60
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| def write_joint_positions(self, positions, velocity=1.0): | ||
| arm, grip = positions[: self._arm_dof], positions[self._arm_dof :] | ||
| ok = self._arm.set_servo_angle_j([math.degrees(p) for p in arm], ...) == 0 | ||
| if grip: | ||
| ok = self._arm.set_gripper_position(grip[0] * _XARM_GRIPPER_MAX_SDK, wait=False) == 0 and ok | ||
| return ok |
There was a problem hiding this comment.
No I don't like this at all.
There was a problem hiding this comment.
We should still control gripper with a seperate GripperControlTask.
Gripper JointStates should be routed to the task, and the task can call self._arm.set_gripper_position.
your implementation works only for Arms with their specific grippers, like the xarm and its first party gripper. I won't be able to use this to control the elephant grippers
There was a problem hiding this comment.
this is internal hardware adapter implementation, the upper level representation is not related here
| **R5.** `get_dof()` MUST return the total (arm + gripper). Adapters branching on arm DOF | ||
| internally — e.g. xArm's 6- vs 7-DOF initial pose (`xarm/adapter.py:278-280`) — keep a | ||
| private `_arm_dof`. |
There was a problem hiding this comment.
Unsure about this.
get_dof() should be callable by the arm and the gripper independently and we can then add them later if we want to.
In most cases I don't think so.
| **R6.** A gripper joint value on `joint_command` and `coordinator_joint_state` is | ||
| normalized: `0.0` fully closed, `1.0` fully open. Out-of-range values are clamped. Arm | ||
| joints are unchanged (radians). |
There was a problem hiding this comment.
This only makes sense for our simple use cases that we discussed.
SweepVolumeGrippers to borrow terminology from GraspGen. This defeats our reason to refactor.
I agree with the normalized 0 to 1 range, but it shouldn't be a single scalar. But a list of joint normalized vector.
For, e.g list of 9 joints for each elephant gripper
| converting layer to branch on *which joints are grippers*, re-introducing the exact | ||
| special-casing this PR removes. And the range constants already live in the adapters. | ||
|
|
||
| **R8.** Each adapter MUST declare its own gripper travel range as a module constant. Two |
| **R10.** Task-level `gripper_open_pos` / `gripper_closed_pos` are now interpreted as | ||
| normalized. **No task code changes** — the fields and their `0.0` defaults stay as they |
There was a problem hiding this comment.
I think you are constraining the LLM to develop a system based on the current GripperTask implementation.
We should be able to support multi-joint grippers, pneumatic grippers, soft body grippers etc.
|
|
||
| ### 3.7 Control modes | ||
|
|
||
| **R28.** An adapter MUST refuse a mode it cannot honour by returning `False` from |
There was a problem hiding this comment.
I am considering having 2 modes by default.
- Simple parallel jaw gripper-like sweep actions, even for elephant robot grippers (Ruthwik mentioned yesterday)
- Joint level control mode.
Let me know your thoughts
|
|
||
| ## 7. Non-goals | ||
|
|
||
| - A `GripperAdapter` protocol / `HardwareType.GRIPPER`. |
There was a problem hiding this comment.
Isn't this the main goal?
What is the plan to implement. Is it after the major spec change?
HardwareComponent stores all_joints + gripper_dof; arm_joints and gripper_joints become derived views, split at len - gripper_dof so a gripper-less arm is not silently inverted by -0. make_gripper_joints takes a count, lifting the single-joint ceiling. No behaviour changes. Adapters, units, tasks and RPCs untouched; ConnectedHardware still normalizes, and the xArm still loses 15% of its gripper travel. Verified: 411 passed / 1 skipped, identical to the pre-change baseline, no behavioural test edits; mypy unchanged (152, all pre-existing, none in changed source); the SS3.5 chain still yields 722.5 at the SDK, asserted. New test_components.py pins the -0 trap. Hardware: nothing to test at this part. "Nothing changed" is proven by the unmodified suite, not a bench. Hardware verification starts at 1.2, where fully-open must reach the xArm SDK as 850.0 instead of 722.5. Ref: GRIPPER-SPEC.md R28, R28a, R29 (7.1 part 1.1)
The gripper joins the adapter's array. write_joint_positions covers every joint with the gripper trailing; read_gripper_position/write_gripper_position are gone. get_gripper_dof() reports the count the component supplies, and get_limits() grows to cover all joints as the single authoritative declaration of a gripper's travel. Units settle in the same commit because they cannot settle separately: once the gripper is in the array, whatever converts it has to be decided. ConnectedHardware loses its gripper branch and both conversion helpers, so it now performs no conversion at all. xArm drops the cartesian mm factor it had been reusing and declares its real 0-850 SDK scale. This kills the SS3.5 bug: fully-open reaches the xArm SDK as 850.0, not 722.5. 15% of gripper travel recovered. Adapter ranges: xarm (0,850), piper (0,0.08)m, a1z (0,0.1)m, a750 (0,0.06)m unverified, mock (0,1), sim from the MJCF over SHM, openarm none. Velocity writes stay arm-only while reads cover all joints -- deliberate, documented, and pinned by a test: nothing produces a gripper velocity, and sending a gripper position as one would make it creep. Verified: 435 passed / 1 skipped; mypy clean on all 22 changed source files; ruff clean. New test_gripper_array_contract.py asserts 850.0 at a stubbed SDK and checks array/limit lengths per adapter. The old tests that pinned the double conversion now pin its absence. Transitional, all removed in 1.4: the two gripper RPCs route through the array instead of being deleted, and blueprint/skill endpoints carry native values, because nothing replaces them until GripperControlTask exists. Keyboard [/] still publishes normalized on joint_command and leans on adapter clamping until 1.3 moves it to gripper_command. Hardware: this is the first part worth a bench. Command fully-open on xArm7 and read what reaches set_gripper_position -- expect 850.0. Same for Piper (0.08 m) and a1z (0.1 m). One number, one layer that could have caused it. Ref: GRIPPER-SPEC.md R4, R4a, R5-R8, R11-R13a, R15, R25 (7.1 part 1.2)
Drives the real chain (ConnectedHardware -> XArmAdapter -> SDK) and reports what reaches set_gripper_position. --fake runs it against a stub so the script can be checked without a robot; read-only by default, --move to command. Arm joints hold their measured pose; only the gripper moves.
The arm reports code=9 unless its servos are enabled, which read as a failure even though the gripper claim had passed. Say so explicitly, add --enable-arm to exercise the arm path, and make the verdict compare the measured shortfall against the 15% the double conversion produced rather than expecting an exact hard-stop value.
Generalizes the part 1.2 check to every device in the step-1 hardware list and moves it up a level. Each adapter's endpoint was declared in the wrong place; the script names what each one was capped at so the verdict is a comparison rather than an absolute: xarm 722.5 of 850, piper 0.07 of 0.08, a1z already correct.
A new `gripper` task type owns a device's gripper joints and converts exactly once, using the range its adapter declares. Gripper-language goes in -- a Bool toggle, an analog trigger, a numeric target -- and a plain joint vector in native units comes out. Below it the path is an arm joint's: arbitration, tick loop, one array, one write_joint_positions. Two modes. set_position/set_normalized are per-joint vectors; set_sweep is one number scaling between the vendor's grasp pose and fully open. A single jaw derives its grasp from the closed limit; a multi-joint hand must declare one and set_sweep refuses without it, because interpolating against joint limits is a fist, not a grasp. is_active() is always True and compute() decides what to emit. The tick loop only hands state to active tasks, so a task that went idle at the end of its hold would report a stale position exactly while the gripper sits still. hold_duration defaults to 0.0 (hold indefinitely), and the hold never ends on "measured reached target" -- a gripper stalled on an object never arrives. The keyboard stops publishing a joint value it cannot know. It sent 1.0 on joint_command, which on an a1z (0-0.1 m) is ten times over-range and survived only by adapter clamping; it now sends a Bool on gripper_command. Both keyboard blueprints migrate together, since that move removes the publisher both hand-rolled servo-gripper tasks read. The VR trigger inverts (squeeze closes) and keeps today's engagement gate: without it a disengaged operator resting a finger would close the gripper. Verified: 484 passed / 1 skipped; mypy and ruff clean. 41 new tests pin the polarity, the refusal, the hold, always-active freshness, non-blocking commands, R14a limit resolution, and that no other task claims a gripper joint in either blueprint. Demonstrated one task driving 850 / 0.08 / 0.1 from the same keypress. Hardware: drive the a1z and Piper grippers from the keyboard and confirm get_position agrees with coordinator_joint_state. The xArm teleop path is untouched here -- its arm tasks still own its gripper until 1.4. Ref: GRIPPER-SPEC.md R14, R14a, R16, R18-R22 (7.1 part 1.3)
Adds a mode that builds the real task against a live adapter and drives it with the same Bool the keyboard sends, so part 1.3 is provable on whatever device is on the bench rather than only on the two blueprints the spec picked. Cross-checks task.get_position() against the measured joint state each step, which caught the reader's own ordering: get_position only sees state through compute(), so it needs a post-command snapshot.
teleop_task and eef_twist_task drop their gripper claims, config and handlers; claim_with_gripper and append_gripper_position are deleted. The two coordinator RPCs that went straight to hardware are removed, and with them the last path that skipped task, arbitration, priority and preemption. Every gripper-bearing blueprint now carries a gripper task; no endpoint value survives above it -- XARM_GRIPPER_PARAMS, GripperTaskOverrides and every 1.2 transitional line are gone. The skill surface speaks one scale. set_gripper takes 0.0-1.0 of travel and get_gripper returns it, normalized on the way out from the joint-state stream the module already consumes, so set_gripper(get_gripper()) is a no-op. open/close go through set_sweep, which reaches the vendor's grasp pose rather than driving every joint to its limit -- a fist on a multi-finger hand. New get_gripper_limits serves the other consumer: a grasp planner with a physical target reads the range and commands the task's set_position in native units. The single-owner audit INSTANTIATES tasks and reads claim(). A config scan would have passed throughout the old code: claim_with_gripper widened the claim at runtime, so static inspection saw an arm task claiming only arm joints while the tick loop saw it claiming the gripper too -- exactly the double ownership SS3.4 describes, invisible to inspection. Blueprints whose tasks need model assets are still covered: they must declare no gripper joints and must not override claim() at all. Verified: 1253 passed / 4 skipped; mypy and ruff clean. The audit was tested by reintroducing a runtime-widened claim, which it caught and the static half did not. Runtime ownership confirmed across all six gripper-bearing blueprints: one owner each, always the gripper task. Hardware: VR teleop on xArm7 -- the trigger drives the gripper, arm tasks no longer claim it, and preempting the arm does not disturb a grasp. On an xArm6, keyboard-teleop-xarm6 exercises the same path. Ref: GRIPPER-SPEC.md R2, R17, R17a, R23, R26, R27, R30 (7.1 part 1.4)
Each of the four parts now carries its outcome alongside its criteria: test
counts, the xArm6 bench measurements, and -- named explicitly -- what was
NOT measured, so "verified on hardware" is never read as covering more than
one device.
New 7.2 records three things the plan did not anticipate:
1. The single-owner audit must instantiate tasks. claim_with_gripper
widened the claim at runtime, so a TaskConfig scan -- the obvious check,
and the one first written -- saw an arm task claiming only arm joints
while the tick loop saw the gripper too. SS3.4's double ownership was
invisible to static inspection.
2. SS3.5 had a second instance in Piper: blueprints declared 0.07 while the
adapter's stroke is 0.08, costing 12.5% of travel. Invisible from the
xArm measurement because the two fail differently -- one a doubled
multiplication, the other a wrong number in the wrong place.
3. The standalone gripper shape already works. R28's split degenerates
when gripper_dof == len(all_joints), so a gripper that is its own
device is a ManipulatorAdapter owning zero arm joints. Demonstrated
through the real coordinator with no new code, which makes R9 and R24
probably unnecessary and shrinks PR 2 to a blueprint plus a test.
R8 loses its GripperAdapter carve-out, which that finding makes redundant.
R9 and R24 are re-scoped from "moved to PR 2" to "deferred, and probably
unnecessary". 7.3 lists what is carried forward: Piper and a1z bench runs,
the VR trigger, preemption during a grasp, and the two pre-existing
unreachable grippers this work surfaced rather than caused.
Documentation only -- no code.
The previous spec commit went beyond its mandate of recording what step 1 delivered: it rescoped R9 and R24 to "probably unnecessary", deleted R8's GripperAdapter carve-out, and rewrote the step-2 row -- scope decisions that were never signed off. All four are restored to the signed-off text. The 7.2 finding stands, reworded to what it actually is: the standalone shape working with zero new code DE-RISKS PR 2's design; it does not make it. The four decisions PR 2 owns are listed as open: R9's shape (protocol vs base-class defaults vs neither), R8's standalone get_dof convention, R24's type and registry, and R16's broadcast routing once two grippers coexist. get_gripper_limits() -- added to ManipulationModule during 1.4 without spec cover -- is ratified into R26 by explicit decision: it reads the same cached range get_gripper() normalizes by, and serves the consumer with a physical target. Documentation only -- no code.
Three decisions confirmed after checking the 2026-08-04 design-meeting transcript and the PR #3381 review threads against this spec: R9 -- GripperAdapter ships as a separate MINIMAL protocol, the fourth adapter kind, per the meeting ("keep separate initially, merge later if similar"). Per-device folders under hardware/grippers/; SDK-less devices (H100) carry their own driver/transport. The shared joint-array surface stays signature-identical with ManipulatorAdapter, enforced by a conformance test so two statements of one contract cannot drift silently. Proposal first, team approval, then build -- the sequence agreed in the meeting. R8 -- the carve-out stands: on a GripperAdapter, get_dof() reports its own joints. It only looked odd while a standalone gripper rode ManipulatorAdapter. R24 -- HardwareType.GRIPPER and the registry ship as part of R9's package, mirroring drive_trains and whole_body. Deferred by confirmation: by_task_name gripper routing and the multi-joint get_gripper() skill surface land in step 3, the PR that first makes two grippers coexist. R10's soft-body exclusion stands as signed; the review thread gets an answer rather than a reopen. Documentation only -- no code.
The concrete proposal the team approves before anything is built, per the sequence from the 2026-08-04 meeting. 14 methods, every one exercised by the existing stack -- the coordinator's lifecycle path, ConnectedHardware's mode switch and joint arrays, the task's R14a limit resolution. Nothing included for completeness; ten ManipulatorAdapter methods deliberately omitted with an explicit add-on-review invitation. KP/KD stays private per the meeting. The drift guard is a parity rule: GripperAdapter MUST remain a strict signature-subset of ManipulatorAdapter, enforced by a conformance test over inspect.signature, so the one array contract stays declared in one authoritative place and a later merge stays mechanical. Wiring per R24: HardwareType.GRIPPER, a lazy registry over dimos/hardware/grippers/, per-device folders (H100 carries its own driver/transport), a MockGripperAdapter, and a GRIPPER component invariant gripper_dof == len(all_joints) validated at construction. ConnectedHardware and GripperControlTask are untouched. The H100 example declares (0, 100) per joint -- its firmware's dimensionless scale, R12's special case. Verified against the code: all 14 proposed methods exist on ManipulatorAdapter today with the signatures cited. Documentation only -- no code.
write_enable reclassified honestly -- the coordinator's elif fallback never fires when activate exists, so 13 of 14 are exercised and write_enable is the flagged trim candidate. write_joint_velocities gains its full justification (typed-union completeness; unreachable behind the mode gate; a refusal, not a capability). Open-loop devices echo their last commanded target rather than reporting zeros. Section status: approved, implemented on this branch, team review on the PR.
… section 8)
A standalone gripper -- its own connection, its own driver, no arm -- gets
its own protocol, registry and hardware type, per the approved section 8
proposal and the 2026-08-04 design meeting ("a gripper adapter protocol as
a fourth protocol; keep separate initially, merge later if similar").
grippers/spec.py declares 14 methods, every one a signature-subset of
ManipulatorAdapter -- one array contract, declared once, enforced by
test_spec_parity so structural typing can never let the two drift apart
silently. get_limits() is documented as the one method that may never
refuse; write_joint_velocities is a defined refusal kept for typed-union
completeness; open-loop devices echo their last commanded target.
Wiring: HardwareType.GRIPPER, a lazy manifest registry over
hardware/grippers/, a coordinator branch passing dof=component.gripper_dof,
the same type-mismatch TypeError the other kinds get, and ConnectedHardware
widened to ManipulatorAdapter | GripperAdapter (mypy checks every
write_command branch against the union -- the reason the velocity method
exists). New component invariant: a GRIPPER component must have
gripper_dof == len(all_joints), so R28's degenerate split is checked, not
conventional.
MockGripperAdapter mimics an H100-like device (six joints, dimensionless
0-100), and coordinator-gripper-mock is the reference blueprint the H100
copies: GRIPPER component + {hardware_id}_gripper task + a declared grasp
posture per R19a.
Verified: 1041 passed across the affected tree; 93 new-path tests including
parity, registry discovery, conformance over three vendor scales, the
degenerate-split invariant, and the end-to-end that pins 7.2's transcript
demonstration in CI -- the same Bool the keyboard sends drives a six-joint
hand onto its grasp pose, and the task resolves (0,100) unprompted. mypy
and ruff clean.
The H100 validates this protocol on real hardware in step 3; nothing here
claims hardware verification.
Ref: GRIPPER-SPEC.md section 8, R9, R24 (delivered); R7, R8, R13, R19a
…re we own A test vehicle for GRIPPER-SPEC 8, explicitly not a production shape: in production an integrated gripper rides its arm's adapter (R1). This adapter opens its OWN connection to an xArm controller and exposes only the gripper as a one-joint standalone GripperAdapter declaring (0, 850), so the entire new path -- GRIPPER component -> grippers registry -> adapter -> wrapper -> task -- can be verified on a real device before the H100 arrives. The safety promise is tested structurally: the fake SDK raises on ANY arm call (motion_enable, set_mode, set_servo_angle_j, ...), so the adapter surviving a full lifecycle proves it never touches the arm. Commands and reads pass through unconverted; transient read failures hold the last good value rather than glitching. keyboard-teleop-gripper-xarm is the run vehicle: [ and ] drive the gripper through gripper_command -> GripperControlTask -> one-joint array, with the arm never commanded. No mock fallback -- a missing IP fails loudly at startup, per the silent-fallback trap found earlier. Verified: 104 gripper-path tests; 1256 passed / 4 skipped across control, hardware, robot, manipulation and teleop; mypy and ruff clean. Ref: GRIPPER-SPEC.md section 8.5 (witness), R1, R13
| """Get joint limits.""" | ||
| """Arm limits in radians, then the gripper's jaw opening in metres.""" | ||
| gripper = self._config.gripper | ||
| upper = [gripper.max_opening_m] if self._gripper_dof and gripper else [] |
There was a problem hiding this comment.
should just take place in xarm adapter
| # Gripper state, both fed from streams the module already consumes: | ||
| # the measured position from coordinator_joint_state, the range from | ||
| # one get_state call per task (ranges are static). | ||
| self._latest_gripper_position: dict[str, float] = {} |
There was a problem hiding this comment.
should just treat gripper as joint no special handling
| return bool(self._control_coordinator.task_invoke(task, method, kwargs)) | ||
|
|
||
| @rpc | ||
| def get_gripper(self, robot_name: RobotName | None = None) -> float | None: |
There was a problem hiding this comment.
manip module should not retain gripper range information. gripper control task should provide rpc for fetching current gripper state in normalized range
| if hw_id is None: | ||
| return None if hw_id is None else f"{hw_id}_gripper" | ||
|
|
||
| def _gripper_range(self, robot_name: RobotName | None = None) -> tuple[float, float] | None: |
There was a problem hiding this comment.
why we need to special-handle gripper range given that gripper range is also provided as robot model's joint limit
| # 3. Release | ||
| logger.info("Releasing object...") | ||
| self._set_gripper_position(0.85, rname) | ||
| self._gripper_invoke("set_sweep", {"value": 1.0}, rname) |
There was a problem hiding this comment.
this is api call leakage, why use this when we have manip module defined the set_gripper rpc?
There was a problem hiding this comment.
gripper adapter should not stay within this pr
| if (self._gripper_open is None) != (self._gripper_closed is None): | ||
| raise ValueError("gripper open/closed positions must be set together") | ||
| self._joint_names: list[JointName] = list(component.all_joints) | ||
| # Velocity writes carry arm joints only; positions carry everything. |
There was a problem hiding this comment.
we should not assume that gripper can't have velocity sensor... if a gripper can't carry it it's a capability issue and the adpater's responsibility to provide either place-holder or not populate at all
Propose the spec for gripper api refactor so that future gripper is not limited to 1 joint but can adapt with mult-joint or dexterous hand that have multi-joint, using the same command path we currently control the arm as a gripper_task rather than previously the rpc call directly bypass the path and directly reach adapter