feat: control coordinator gripper api refactor - #3381
Open
jhengyilin wants to merge 33 commits into
Open
Conversation
…i future 3rd party multi-joint gripper
jhengyilin
requested review from
Dreamsorcerer,
leshy,
mustafab0,
paul-nechifor and
spomichter
as code owners
August 5, 2026 23:59
Contributor
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 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
mustafab0
requested changes
Aug 6, 2026
TomCC7
reviewed
Aug 6, 2026
TomCC7
marked this pull request as draft
August 6, 2026 01:12
TomCC7
reviewed
Aug 6, 2026
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
TomCC7
reviewed
Aug 8, 2026
TomCC7
requested changes
Aug 8, 2026
This was referenced Aug 9, 2026
GripperAdapter moves to jhengyi/gripper_adapter_pr2; velocity writes carry all joints; task gains get_normalized; module retains no gripper state.
TomCC7
reviewed
Aug 11, 2026
TomCC7
changed the title
TomCC7
reviewed
Aug 11, 2026
TomCC7
marked this pull request as ready for review
August 11, 2026 20:27
github-actions
Bot
added
the
ready-to-merge
github-actions
Bot
added
ready-to-merge
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Contribution path
Problem
Gripper commands bypassed the normal control-task path through coordinator RPCs and adapter-specific scalar APIs. That skipped task ownership and arbitration, spread gripper-specific joint splitting and unit conversion across the generic control API, and constrained the abstraction to a single special-cased joint. It could also apply conversion more than once, such as reducing xArm's native 0–850 command range.
Solution
get_dof(), reads, writes, and limits now cover every adapter-owned joint; adapter-specific arm/gripper splitting stays inside the adapter.GripperControlTaskas the dedicated owner of configured gripper joints. It accepts native or normalized targets, resolves limits by joint name, participates in normal arbitration, and exposes normalized task RPCs.How to Test
uv run pytest dimos/control/tasks/gripper_task dimos/hardware/manipulators/mock dimos/hardware/manipulators/sim/test_shm_adapter.py dimos/hardware/manipulators/xarm/test_adapter.py dimos/robot/manipulators/a1z/blueprints/test_teleop.py dimos/robot/manipulators/piper/blueprints/test_teleop.py dimos/robot/manipulators/xarm/blueprints/test_teleop.py dimos/simulation/engines/test_mujoco_sim_module.py -m 'not (self_hosted or mujoco or self_hosted_large)' uv run --group lint mypyAI assistance
OpenAI Codex with GPT-5 was used extensively for implementation, review, test cleanup, and CI debugging. The author reviewed the resulting changes.
Checklist