Skip to content

feat(pick): two-camera grasp verify + skill_lib vision/gemini + cancellation refactor - #550

Closed
theo-michel wants to merge 1 commit into
theo/pick-wrist-servofrom
theo/skill-lib
Closed

feat(pick): two-camera grasp verify + skill_lib vision/gemini + cancellation refactor#550
theo-michel wants to merge 1 commit into
theo/pick-wrist-servofrom
theo/skill-lib

Conversation

@theo-michel

Copy link
Copy Markdown
Contributor

Stacks on theo/pick-wrist-servo (where #548/#549 already merged). Consolidates the remaining pick work plus adjacent cleanups.

pick_any_object

  • Two-camera verify: _grasp_verified sends the head and wrist frames to Gemini in one call. The head view answers "is it still on the floor?"; the wrist view (flagged as mirrored in the prompt) can show the object held in the fingers, so a dangling sock isn't misread as a drop. Degrades to head-only if the wrist frame is missing. Verdict logic and the verify telemetry fields are unchanged (added a cams count).
  • Cancellation refactor: SkillCancelled + a single _checkpoint() at loop tops replaces the if self._cancelled: return None plumbing. Stage return types are now honest (None = not found, not "cancelled"), and cancel-during-search/position is reported as CANCELLED instead of FAILURE. No checkpoint inside close/twist/lift — once the fingers commit, the grasp finishes.

skill_lib

  • Extracted the stateless vision math (frame decode, Gemini parse, LK tracking, color segmentation) into skill_lib/vision.py and the proxy vision call into skill_lib/gemini.py.
  • gemini.ask_image accepts one image or a list (enables the two-camera verify).

Folded in per "everything"

  • pyright: return-type annotations on the Skill base class (types.py) + pyrightconfig.json; in-progress pyright cleanups across other innate_skills/* (from the parallel remediation task).
  • post_update.sh: enforce root:<user> 640 on /etc/innate.env so the non-root launch readers can load the service key (a hand-created 600 root:root file silently dropped the key).
  • webapp: pick tuning panel renamed pickTunePanel.jspickOverlay.js (+ main.js/app.css).

Verification

Compiles + pyright-clean on the pick/skill_lib files; skills server hot-reloads to ready; request-shape test confirms head-then-wrist image ordering. Not yet exercised on a live robot — the robot's Gemini calls 403 until the cloud proxy PR (innate-cloud #80) deploys with the gemini service.

Note

#548 and #549 are already merged into this base branch, so there's nothing to close on them.

…ation refactor

pick_any_object:
- _grasp_verified now sends BOTH head + wrist cameras to Gemini so a
  held object (visible in the mirrored wrist view) isn't misread as
  dropped; degrades to head-only if the wrist frame is absent.
- cancellation via SkillCancelled + _checkpoint() replaces the
  cancelled-as-None plumbing (one raise, one except in execute); fixes
  cancel-during-search being misreported as FAILURE.
- extracted the pure vision math and proxy vision call into
  workspace/skill_lib/{vision,gemini}.py; gemini.ask_image now takes
  one image or a list (multi-image support).

Also folded in (per 'everything'):
- pyright: return-type annotations on Skill base (types.py) +
  pyrightconfig.json; /etc/innate.env perms enforcement in
  post_update.sh; webapp pick tuning panel rename to pickOverlay.js;
  in-progress pyright cleanups across other innate_skills.
@greptile-apps

greptile-apps Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR consolidates the pick skill work: two-camera grasp verification (head + wrist images sent to Gemini), a cleaner cancellation model using SkillCancelled exceptions instead of boolean-checked returns, and extraction of stateless vision/Gemini helpers into skill_lib/vision.py and skill_lib/gemini.py. It also folds in pyright annotation fixes across several skills, a post_update.sh permission fix for /etc/innate.env, and a rename of the webapp pick panel from a tuning interface to a read-only overlay.

  • Two-camera verify (_grasp_verified): head and wrist frames are both passed to Gemini in one call; degrades to head-only when the wrist frame is missing — but a logic gap means if only the wrist frame is available the prompt still says "Image 1 is the head camera", misleading the model.
  • Cancellation refactor: SkillCancelled is raised by _checkpoint() at loop tops and propagates cleanly to the execute() try/except; the finally block guarantees arm rest and head reset on every exit path, including cancellation.
  • skill_lib extraction: gemini.ask_image handles one image or a list; vision.py consolidates all decode/parse/track/segment helpers previously inlined in pick_any_object.py.

Confidence Score: 3/5

Safe to merge for non-live builds; the wrist-only verify mislabeling is a real correctness hole in the new two-camera path, and the code is explicitly not exercised on a live robot until the cloud proxy deploys.

The new two-camera verify logic has a concrete mislabeling bug: when the head camera is unavailable during verify, the images list silently becomes [wrist_image] but the Gemini prompt still says 'Image 1 is the head camera looking at the floor.' Gemini then reasons about the wrong view, which can flip the held/dropped verdict. The cancellation refactor and skill_lib extraction are otherwise clean.

workspace/innate_skills/pick_any_object.py — specifically the _grasp_verified method and the assert in _sweet_box.

Important Files Changed

Filename Overview
workspace/innate_skills/pick_any_object.py Major refactor: SkillCancelled exception replaces boolean-check cancellation; two-camera verify added in _grasp_verified; Gemini/vision helpers delegated to new skill_lib modules. Logic error when only wrist image is available during verify (mislabeled prompt); assert in _sweet_box bypasses cleanup handlers.
workspace/skill_lib/gemini.py New module extracting Gemini proxy calls; ask_image cleanly accepts single image or list; retry logic and logging delegated correctly from pick_any_object.
workspace/skill_lib/vision.py New module with stateless vision helpers (b64 decode, Gemini parse, LK tracking, CamShift color seg); missing one blank line before the first function def (PEP 8).
ros2_ws/src/brain/brain_client/brain_client/skills/types.py Return type annotations added to Skill base class methods (name, execute, guidelines, guidelines_when_running); no logic changes.
scripts/update/post_update.sh Adds idempotent ownership/mode fix for /etc/innate.env (root: 640) to repair hand-created 600 root:root files that silently dropped the service key.
webapp/js/teleop/pickOverlay.js New file replacing pickTunePanel.js; render-only overlay driven by pick debug events, no longer publishes tuning parameters to the robot.
workspace/innate_skills/navigate_with_vision.py Pyright fix: null-guard on goal_handle before accessing .accepted; local variable used for cancel_goal_async instead of potentially stale self._goal_handle.
workspace/innate_skills/detect_opponent_move.py Pyright fixes: null guard on gemini_client before API calls; response.text coalesced to empty string before strip(); list typed as list[Any]; cast() for RobotColor.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant E as execute()
    participant S as _search()
    participant P as _position_above()
    participant G as _grasp_at()
    participant V as _grasp_verified()
    participant GEM as gemlib.ask_image()

    E->>S: prompt
    S->>GEM: head_image, find prompt
    GEM-->>S: pixel coords
    S-->>E: xy (base_link)

    E->>P: prompt, xy
    P->>P: _follow_into_box (LK optical flow)
    P->>GEM: head_image, reseed/confirm
    P-->>E: xy (refined)

    E->>G: prompt, xy
    G->>G: _wrist_servo (CamShift color seg)
    G->>GEM: wrist_image, seed box
    G->>G: _push_to_floor (blind descent)
    Note over G: _checkpoint() last cancel point before fingers commit
    G->>G: _close_twist_lift

    E->>V: prompt
    V->>GEM: head_image + wrist_image, is it on the floor?
    GEM-->>V: YES/NO
    V-->>E: "held=True/False"

    Note over E: finally _rest_arm head reset
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant E as execute()
    participant S as _search()
    participant P as _position_above()
    participant G as _grasp_at()
    participant V as _grasp_verified()
    participant GEM as gemlib.ask_image()

    E->>S: prompt
    S->>GEM: head_image, find prompt
    GEM-->>S: pixel coords
    S-->>E: xy (base_link)

    E->>P: prompt, xy
    P->>P: _follow_into_box (LK optical flow)
    P->>GEM: head_image, reseed/confirm
    P-->>E: xy (refined)

    E->>G: prompt, xy
    G->>G: _wrist_servo (CamShift color seg)
    G->>GEM: wrist_image, seed box
    G->>G: _push_to_floor (blind descent)
    Note over G: _checkpoint() last cancel point before fingers commit
    G->>G: _close_twist_lift

    E->>V: prompt
    V->>GEM: head_image + wrist_image, is it on the floor?
    GEM-->>V: YES/NO
    V-->>E: "held=True/False"

    Note over E: finally _rest_arm head reset
Loading

Reviews (1): Last reviewed commit: "feat(pick): two-camera grasp verify, ski..." | Re-trigger Greptile

Comment on lines +760 to +765
images = [img for img in (self.main_image, self.wrist_image) if img]
wrist_note = (
" Image 2 is the WRIST camera next to the gripper fingers "
"(mirrored) — the object may be visible held in the fingers there."
if len(images) > 1 else ""
)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Wrist-only edge case mislabels image for Gemini

images is built by filtering out falsy values from (self.main_image, self.wrist_image). If main_image is unavailable (e.g. a topic timeout right after the backup drive) but wrist_image is still live, images = [wrist_image]. The prompt still says "Image 1 is the head camera looking at the floor" — Gemini receives the close-up wrist view but is told it is the forward-looking head camera, so it cannot meaningfully evaluate whether the object is on the floor vs held in the fingers. A simple guard tracks which cameras are actually present and builds the prompt accordingly.

Suggested change
images = [img for img in (self.main_image, self.wrist_image) if img]
wrist_note = (
" Image 2 is the WRIST camera next to the gripper fingers "
"(mirrored) — the object may be visible held in the fingers there."
if len(images) > 1 else ""
)
head_img = self.main_image
wrist_img = self.wrist_image
images = [i for i in (head_img, wrist_img) if i]
# Only describe the wrist image if the head image is also present
# (so "Image 1" always refers to the head view).
wrist_note = (
" Image 2 is the WRIST camera next to the gripper fingers "
"(mirrored) — the object may be visible held in the fingers there."
if head_img and wrist_img else ""
)

Comment on lines +17 to +19
from workspace.skill_lib.geometry import IMG_H, IMG_W

def b64_to_gray(image_b64):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Missing blank lines before top-level function definitions — PEP 8 requires two blank lines before each top-level def.

Suggested change
from workspace.skill_lib.geometry import IMG_H, IMG_W
def b64_to_gray(image_b64):
from workspace.skill_lib.geometry import IMG_H, IMG_W
def b64_to_gray(image_b64):

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!

Comment on lines +612 to +614
reason = fail
break
model = self._seg_model(hsv, box) # the view changed: relearn
if model is None:
reason = "lost track"
break
window = box
misses = 0
box = window # last trusted window
px = pt
guess = px
px = tracker.guess # the re-seed detection is this frame's fix

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 assert escapes the skill's exception handlers

assert c is not None raises AssertionError, which is not caught by the except SkillCancelled or except armlib.ArmUnhealthy blocks in execute(). If floor_to_pixel returns None at runtime (e.g. after a live tuning change drives tilt_deg or sweet_x out of range), the assertion propagates as an unhandled exception from inside the try block, bypassing the _rest_arm and head-reset logic in finally. Raising SkillFailed (or another caught exception type) here would keep the cleanup path intact.

@theo-michel

Copy link
Copy Markdown
Contributor Author

Folded directly into #542 — the commit is now on theo/pick-wrist-servo (cherry-picked as e89b072), so #542 → main carries everything in one PR. No longer need the stacked branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant