feat(skills): Pick Anything Skill + Other changes - #542
Conversation
Greptile SummaryThis PR ships two layered changes: a new annotation-driven Skills API (replacing per-skill boilerplate with class-level type declarations, typed robot-state values, and a
Confidence Score: 4/5Safe to merge with the understanding that the wrist-servo stage and per-run Nav2 TF buffer need hardware validation before relying on them in production. The major correctness issues from the previous review round are addressed. The one new finding is a stall-counter bookkeeping gap in _wrist_descend that can cause premature handoff to the blind push — the failure mode is a slightly earlier exit to a proven fallback rather than a crash or servo trip. Files Needing Attention: workspace/innate_skills/pick_any_object.py (stall-counter fix, experimental wrist-servo stage), ros2_ws/src/brain/brain_client/brain_client/nodes/skills_server.py (cancel-latch + slot lifecycle), and the arm C++ files (gain atomics, per-run Nav2 warm-up). Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant SkillsServer
participant RobotState
participant Skill
participant Invoker
participant ArmControl
Agent->>SkillsServer: ExecuteSkill goal
SkillsServer->>SkillsServer: _claim_skill_slot()
SkillsServer->>SkillsServer: _instantiate_for_run(entry)
SkillsServer->>RobotState: start_subscriptions()
SkillsServer->>RobotState: wait_for_required_states(skill)
Note over RobotState: per-feed grace period
RobotState-->>SkillsServer: injected typed state
SkillsServer->>RobotState: "begin_continuous_updates @ 50Hz"
SkillsServer->>Skill: "execute(**inputs)"
Skill->>Invoker: self.gripper_open()
Invoker->>Skill: child.execute()
Skill-->>Invoker: result
Invoker->>SkillsServer: _dispose_run_instance(child)
Skill->>ArmControl: manipulation.move_checked()
ArmControl-->>Skill: ok
Skill-->>SkillsServer: SkillResult
SkillsServer->>SkillsServer: _dispose_run_instance(skill)
SkillsServer->>RobotState: stop_subscriptions()
SkillsServer->>SkillsServer: _release_skill_slot()
SkillsServer-->>Agent: ExecuteSkill.Result
Reviews (51): Last reviewed commit: "fix(skills): address code review — call ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Folded #555 in (merge |
…rator + loader hardening Decorator (review items from #542): - ctx.fail() annotated NoReturn so pyright narrows past it - ctx.on_cancel(cb): per-run hook fired on the cancelling thread right after the latch sets — the function-style equivalent of a cancel() override that stops the base / cancels a Nav2 goal immediately; hooks clear at run end - @Skill(on_shutdown=...): retire hook for module-owned long-lived resources (the class form's shutdown()) Loader (mechanical guards instead of comments): - a file defining >1 skill is rejected loudly (catalog id comes from the filename — a second skill would silently collide) - a skill whose name differs from its filename stem gets a load-time warning Migration — every skill in workspace/innate_skills/ is now a @Skill function except pick_any_object, which keeps the class form deliberately: it creates its tuning/debug topic IO at registration time (guidelines()) so the teleop panel's {} sync handshake acks before any run, and shutdown() mutes the retired instance; the decorator has no registration-time hook and inventing one for a single caller is the speculative surface the RFC rejects. Equivalence check (stubbed-ROS harness, HEAD vs migrated): 24/24 skills have identical name, execute() signature (schema), guidelines text, and declared robot states. Two guidelines gained a missing space ("map.If" -> "map. If"). Interfaces now uniformly declare all three per the decorator's design — injection is a singleton reference assignment, so this costs nothing. Behavior tests: on_cancel/on_shutdown wiring, cancel latch, hook clearing, and both loader guards (14/14 pass). ruff check + format clean.
…s/head_position, Camera declaration
Non-camera state no longer needs a declaration: the server already
subscribes every feed per run, so the Skill base now exposes read-through
properties backed by new RobotStateProvider.current_* snapshot accessors
(each read converts the newest cached message — fresher than the 50 Hz
inject). Cameras keep a declaration, renamed to its real job:
image = Camera("main"|"wrist") — the server must know before execute()
to start the camera and warm the first frame.
- self.battery returns a typed innate.Battery (Odometry precedent),
with the soft-deprecated dict-compat layer for old battery["..."] code
- legacy RobotState descriptors keep working and shadow the ambient
properties of the same name
- shipped skills migrated (pick_any_object deferred: in-flight on PR #542)
- tests: test_ambient_robot_state.py, wired into ci/run_integration_tests.sh
… Mobility, image: MainImage
The dataclass idiom replaces descriptor assignments as the canonical
declaration form:
class FetchSock(Skill):
mobility: Mobility
head: Head | None # optional — checkers force the guard
image: MainImage # required — run fails if no frame arrives
- Skill.__init_subclass__ scans bare annotations and mints the existing
Interface/Camera descriptors (type identifies the feed, | None makes it
optional); annotated constants and non-feed types are untouched; string
annotations resolve against the skill module. Pure sugar: server,
warmup, injection, and every legacy form keep working.
- The annotation is real typing: pyright/mypy see the actual interface
and Image types, so required feeds are non-Optional and self.mobility.
autocompletes.
- Required cameras now fail the run after the warmup grace instead of
handing execute() a None — send_picture/record_position/recalibrate/
follow_aruco drop their manual no-image guards.
- MainImage/WristImage (Image subclasses) and DepthMap (ndarray view)
carry the feed identity; injected values use them.
- Head state type renamed HeadState; innate exports Mobility/Manipulation/
Head lazily (PEP 562) since they pull ROS/Nav2 modules.
- All shipped skills migrated (pick_any_object still deferred to PR #542).
Framework: - exact begin/end pairing for the 50 Hz state slot: the "No data" return path exited before begin_continuous_updates, and the unmatched end handed a chaining parent's slot back too early, freezing its ambient state mid-run - optional/legacy camera declarations get the same bounded first-frame wait as required ones (their frame is just as in-flight; the skill merely chose to handle a miss itself); the wait is now cancel-aware - CameraProvider.start(feeds) subscribes only the declared feeds — raw depth is ~600 KB/frame and was streaming for skills that never asked - loader: private/abstract Skill subclasses are helper bases, exempt from the one-skill-per-file rule Skills: - detect_opponent_move: cameras are '| None' — detection degrades to whichever camera is live (every use is guarded) instead of failing the run up front when one is offline - follow_aruco: '| None' + an explicit 5 s wait_for; the 3 s required grace is too tight for a cold camera in sim - pick_any_object: cancel chaining via on_cancel/check_cancelled (the cancel() override skipped child-skill cancellation), whole-word "NO" in the floor-clear check (hedges like "CANNOT" read as clear), off-image pick box returns None, flow-follow tracks only new frames, no per-frame EE fetch (~10 ms each), drop the redundant name property and interface guards the server now enforces - post_update.sh: chown/chmod best-effort under set -e - pickOverlay: box:null clears the drawn box (skill projects off-image)
- loader: register skill modules in sys.modules during exec so string / `from __future__ import annotations` annotations resolve on the REAL load path (they silently minted nothing before); leave INNATE_OS_ROOT on sys.path so lazy in-function `workspace.*` imports work too - Interface: hand-built Interface(InterfaceType.X) is tolerant again (old behavior, same rule as legacy RobotState); constants/annotations keep declaring-is-requiring - skills_server: a cancel landing during the pre-run state warmup now reports CANCELLED instead of a phantom "No data from the ..." failure - SkillCancelled is a BaseException (asyncio.CancelledError precedent): a skill's broad `except Exception` can no longer swallow a cancel - normalize_skill_result: a forwarded child SkillOutput keeps its .data - robot_state: memoize the Lidar conversion per scan message (the 50 Hz thread was copying ~1100 ranges per tick against a ~10 Hz topic) - dictcompat: FutureWarning, not DeprecationWarning — the default filter hides the latter everywhere except pytest, so authors never saw it - pick_any_object: migrate to the annotation API (was the last legacy- style skill); clamp tuning-topic values into per-knob hard ranges and reject non-finite floats before they can reach the servos; retired _DebugIO drops its skill ref so reloads don't pin dead instances - record_position/recalibrate_manual: wrist image back to best-effort — it only feeds a debug snapshot and must not abort calibration - retrieve_emails: don't re-wrap our own SkillFailed in the broad except - pickOverlay: guard releasePointerCapture on pointercancel so the final wrist-box position still publishes - tests: loader-path future-annotations coverage (red before the fix), legacy-interface tolerance, SkillOutput passthrough
theo-michel
changed the title
Six findings from a recall-oriented review of the branch: - vision: clamp Gemini's normalized 0-1000 coords (_norm1k). They drift out of range, and an off-image/negative box reached seg_model's numpy slice and CamShift, raising cv2.error out past execute()'s handlers instead of taking the designed "lost track -> blind descent" fallback. - loader: a file defining two skills no longer loses both. Keep the one the filename names (the catalog id), drop the extras with an actionable error — rejecting the whole file vanished a previously-working skill because of a stray helper class. - camera_provider: refcount per feed, not just overall. A feed only a nested child declared (raw depth, ~600 KB/frame) streamed for the rest of the parent's run — exactly the cost start(feeds) exists to avoid. Dropping one parks the private spin thread first, since destroying entities under a spinning executor races it (#497); survivors miss at most one frame interval. - robot_state: the pre-run warmup now waits for every declared feed, not just required ones plus cameras. An optional/legacy declaration's first message is just as much in flight, so a skill reading self.odom in its opening lines saw None for one publish period and silently took its degraded path — pick's rotate_by fell back to open-loop with no log at all. The wait still ends as soon as the values land. Also gives moblib.rotate_by the open-loop warning drive already had. - types: reject `resource: SomeResource | None` at class definition. The optional flag was computed and then dropped, so the annotation built the same eager, never-None resource as the bare form and left the author's None guards as dead code. - pick_any_object: guidelines text moves into the class docstring (the API's own rule) and the override goes. It was also building the debug_io resource as a side effect, so every catalog metadata refresh created a publisher + subscription on the shared node — which _DebugIO deliberately never destroys. debug_io now builds on the first _dbg. Also applies ruff format to pick_any_object.py, which was left unformatted by eb4852f (one list reflow) — the format gate is red on the branch without it. Tests: two new cases in test_ambient_robot_state.py (loader keeps the filename skill; optional resource annotation rejected). 27 pass.
theo-michel
changed the title
- gripper_open: trust j6 over the driver's return — a False status with the claw verifiably open (driver wait timeout) no longer fails the skill; the status only decides when j6 gives no evidence. - pick_any_object: _grasp_verified falls back to the gripper (j6) evidence when Gemini returns no verdict at all, instead of reporting a demonstrably held object as a missed grasp. A hedged reply still counts as not-clear. - robot_state: the pre-run warmup wait covers required feeds only. Optional (| None / legacy) declarations no longer stall the start — a dead optional feed used to cost its full grace (wrist camera 3 s, battery 6 s) on every run. Skills needing an optional feed in their opening lines wait themselves (follow_aruco already does).
- pick: wrist_box_v is live-tuned to 380, but its comment and the pickOverlay.js seed both still said 300 — comment rewritten to match the tuned value, overlay seed synced. - types: when the stdlib batch annotation eval fails, _own_annotations now retries per name (same eval context), so one typo'd annotation costs only its own declaration instead of silently dropping every feed on the class — which also blinded the server's up-front missing-interface gate. - types: @resource no longer caches a None factory result. None is the documented "unavailable" signal (missing credentials, dead network), and skills are long-lived singletons — caching it pinned a transient outage for the instance's lifetime; now the next access retries. - camera_provider: guard the depth-frame reshape like its sibling in perception/camera.py — a padded/truncated frame reads as "no frame" instead of raising ValueError out of the pre-run state wait.
|
First: the PR description no longer matches the code Major findings Lost cancels via /brain/cancel_skill — skills_server.py:249-256: a service cancel landing between slot claim and _active_code_skill = skill is silently dropped. It invokes cancel_callback directly, which never transitions the action goal to CANCELING, so the _begin_run recovery check of is_cancel_requested stays False and the skill runs to completion after the service replied "Cancellation requested". A parallel hole exists for physical skills (the stale-cancel guard at line 646 eats the request). Hardware safety Cancellation latency Behavior delta needing hardware validation Test coverage Minor findings (worth fixing, not blocking) |
2c045dc to
f373ca9
Compare
Code review — recall pass (xhigh)Reviewed 1. The wrist visual-servo descent has no cancel point on its fast path
Failure: operator presses Stop while the arm is stepping down at 15 Hz wrist frames → 2.
|
|
Heads-up on the
Both failing stretches died in Cloud Build "step 2" ( The blocker for diagnosing further: with I'll keep watching; if attempt 3 passes again with no code change, that's the third same-code green today. Generated by Claude Code |
4f0ce89 to
ce0406d
Compare
…es by annotation
One rule for all robot state, hardware, and composition: annotate it.
`mobility: Mobility`, `odom: Odometry`, `image: MainImage`, a sub-skill
(`gripper_open: GripperOpen`), or a trained policy
(`pick_socks = PhysicalSkill("pick_socks")`). Plain annotation = required —
the server waits (bounded per feed: cameras 3 s, battery 6 s, else 2 s) and
fails the run before execute() if nothing arrives, so skills need no
`if self.x is None` guards. `| None` (or a `= None` default) = best effort.
Reading an undeclared feed raises with the annotation to add.
The base Skill supplies what every file used to repeat: the class name is the
skill name (snake_cased), the docstring is guidelines(), plus self.fail(),
self.cancelled / check_cancelled(), per-run on_cancel() hooks, self.feedback(),
self.wait_for(), self.storage, and None/str returns treated as success.
Instances are per-run: constructed for the run, disposed at its end —
@resource declares expensive per-run objects with generator-style teardown
that runs while interfaces are still live. Skills no longer override
__init__; the interfaces are plain typed attributes the framework fills in.
Robot state arrives as typed, ROS-free values under brain_client/state/
instead of raw dicts: Odometry, Pose (map frame), Battery, Lidar (+ min_range
sector helper), Arm, Map (lazy .grid), JointStates, HeadState,
Image/MainImage/WristImage (the value is the b64 string, .jpeg gives bytes),
DepthMap (numpy). Legacy dict access keeps working via dictcompat.LegacyMapping,
soft-deprecated with a warning. Main/wrist frames are memoized per frame in the
50 Hz injection path, and CameraProvider.start(feeds) subscribes only the
declared feeds, so raw depth no longer streams for skills that never asked
for it.
Returns collapse onto one object: SkillReturn carries success, message, and
data, replacing the per-skill dict conventions. Hardware interfaces lose the
`Interface` suffix (Mobility, Head, Manipulation) and the public authoring
namespace is `innate` — `from innate import Skill, Mobility, SkillReturn`,
with camera math, vision, and Gemini helpers under `innate.geometry`,
`innate.vision`, `innate.gemini`.
…n drop-in packages Skills directories under workspace/ are ordinary Python packages, imported rather than scanned: workspace/ goes on sys.path, every module is imported, and defining a Skill subclass *is* the registration. The class is the identity, so files organize freely — several skills per file, a skill split across a subpackage, helpers next to the skills — and packages import each other by bare name (from innate_skills import arm_utils). innate_skills/ (shipped) and custom_skills/ (yours, gitignored) are just the two built-in packs; any other folder dropped into workspace/ — or symlinked in (ln -s /opt/team/skills workspace/team_skills) — is a pack, ids namespaced by folder name (<pack>/<name>). This replaces the 0.6.x extra_skill_dirs / extra_agent_dirs setting (settings knob, webapp catalog entry, and the media-route serving lanes), and 0.7 no longer scans ~/skills / ~/agents in place — migrate_user_data.sh moves both home lanes into workspace/custom_* so nothing silently stops loading (sudo-safe via ACTUAL_HOME). Physical (trained) skills join the same surface: the catalog generates a typed reference class per recording, written as an __init__.py inside the recording folder, so `from innate_skills.pick_socks import PickSocks` type-checks against what the robot actually has. Orphaned shims are pruned when the roster is republished, and the emitted files are already ruff-format clean. A module that fails to import no longer vanishes: it shows up in the web-app skills menu as a disabled row carrying its load error (SkillInfo.load_error), is never registered with the cloud agent, and clears when fixed. An empty metadata.json — the training node touches one into every recording dir — no longer rosters a phantom broken skill. Hot reload (/brain/reload_primitives or the save-watcher) covers helper modules too, survives root-level filesystem events, and follows symlinked packs. The menu groups skills by folder (SkillInfo.group). The old scan-based skill loader, the string-based composition path, and the legacy import shims are deleted along with test_backwards_compat.py, which guarded them. dynamic_loader stays for agents and inputs only.
get_skills() and get_inputs() take classes, not id strings: an agent lists
`[MoveStraight, PickSocks]` and `[Joystick]` and the loader resolves them to
ids, so a renamed or deleted skill is a type error in the editor instead of a
silent no-op at runtime. Strings still work — the two forms mix freely — and
physical skills participate through the generated reference classes imported
from their recording folders.
Every shipped agent moves onto the typed surface, and the input-device module
layout settles: brain_client/inputs/{types,loader}.py with the ROS bridge in
nodes/input_manager.py, replacing the flat input_types/agent_types shims.
Each skill drops its __init__, name property, guidelines(), Interface declarations, and None-guards in favour of annotations, the class docstring, and SkillReturn. The suites that belong together become packages — innate_skills/chess/, innate_skills/email/, innate_skills/arm/ — now that a folder is a legal place for a skill to live and helpers import by bare name. Behaviour changes that fell out of the migration: navigate_to_position resolves local goals from the injected odometry instead of re-reading the topic itself, run_routine_demo calls pick_socks through its typed reference, and arm_zero_position waits for the arm to actually settle.
Grasp an object named in natural language: Gemini locates it in the wrist camera, the skill servos the arm onto the target over successive frames, and closes the gripper under current control once it is within reach. The aim point sits deliberately short of the detection centroid so the approach stops on the object rather than pushing past it. New wrist frames are detected by identity, not by content: the state provider builds exactly one Image object per ROS message, so `img is raw` means "same message". Comparing JPEG bytes deadlocked in sim, where MuJoCo renders a static scene byte-identically and the skill never saw a frame it considered new. Teardown is intentionally non-cancellable: once the gripper closes, a Stop must not unwind mid-grip and drop what the robot is holding.
The gripper (servo 6) moves to Dynamixel control mode 5: it drives toward its goal position with output capped at goal_current, so a deep close squeezes whatever it holds at a constant safe force instead of tripping overload — no software force loop. current_limit is raised above the grip current (it is the EEPROM overload threshold, not the torque cap) and the close profile is slowed so the jaws press in rather than impact. Two things the arm had to stop doing for a grip to survive a move: - Trajectories now spline the gripper from its last *commanded* goal, not its measured position. While gripping, the servo stalls short of the goal and that standing position error IS the grip force; re-seeding from the measured position zeroed it and dropped the object at the start of every arm move. - A finished trajectory keeps its gain mode instead of falling back to teleop gains, which let the arm sag between the stepped moves a skill sends. The control loop decays the hold back to teleop after a quiet timeout so an idle arm is never held stiff, and the idle decay is blocked (and re-asserted per waypoint) while a trajectory runs.
The stereo depth estimator and the simple filters logged on every frame, which at stream rate costs measurable CPU and buries anything worth reading. Throttle them to state changes and errors.
…party isort Small unrelated fixes that rode along with the branch: - post_update.sh repairs /etc/innate.env to 640 root:$ACTUAL_USER only when it is not already there, and survives a failed chown instead of aborting the update (a root-unreadable file made the service key silently vanish). - The web-app joystick hides/shows on `j`, releasing any latched command on hide so the robot never drives from a joystick the operator cannot see. - ruff's isort knows workspace/, innate, and innate_proxy as first-party, and the two files that were sorted the old way follow. - ci/run_integration_tests.sh drops test_backwards_compat.py from the fast unit-test lane, since the shims it guarded are gone.
self.sleep wakes and raises SkillCancelled the moment a Stop lands; time.sleep blocks to completion, so a skill that uses it keeps the robot moving after the user pressed Stop. Document the cancellation contract in AGENTS.md and the rule in CLAUDE.md, including the one deliberate exception: teardown and already-committed physical actions stay non-cancellable.
0d38ac8 to
6f62f90
Compare
Split the lowercase decorator class into _Resource (descriptor) + a resource() factory function with overloads; typed __get__ for class vs instance access. Riding along: list[float] in arm_zero_position, and a comment guarding arm_utils' invalid-command fail from unreachable-code cleanup.
The agent existed only to expose the demo skill; removing the skill alone left it importing a deleted module.
micro_input imported sounddevice at module scope, and sounddevice dlopen()s PortAudio on import. On any machine without an audio stack (CI, sim, a laptop) that raised OSError while the loader was merely *importing* the module, so every agent naming MicroInput in get_inputs() was discarded at discovery — six of the seven shipped agents, which is why the brain launch tests failed with security_guard_agent never registering. The library turned out to have no consumer at all. MicStreamer, its only caller, had no call sites here, on main, or on feat/add-mic, whose LocalMicStreamer wraps ArecordStreamer rather than reviving it. Every real capture path goes through the arecord subprocess. So the class goes, and sounddevice with it: no consumer, no dependency, and no library left for a future module-level import to fail on. DTYPE was MicStreamer's only user. portaudio19-dev stays in apt-dependencies.hardware.txt — dropping it is a separate call about the robot image, and it is harmless there.
Recording folders ship in git as metadata.json plus the generated ref shim, with the trajectory itself fetched from metadata["downloads"] — so in a fresh checkout (and in every CI image) wave/ has no episode_0.h5. Validation read that as damage and dropped the skill from the roster, which then made the shim look orphaned: the prune pass deleted a committed __init__.py mid-run, and demo_agent stopped loading on `from innate_skills.wave import Wave`. A named replay_file that is simply absent is now (valid, in_training), the same treatment a learned skill's missing checkpoint already gets: the skill stays rostered, its typed ref keeps existing, and running it is refused with a reason instead of "unknown skill". A file that exists but isn't a usable recording is still invalid. prune_dir_shims now skips any folder that still holds real metadata, via the catalog's own has_physical_metadata so both agree on empty files. It only reclaims folders that stopped being recording folders, so no transient fault can delete a tracked shim again. in_training's user-facing reason no longer claims an un-fetched recording is "training".
502d4f1 to
b4e1c25
Compare
…it pull / innate update) (#591) * fix(skills): stop tracking generated physical-skill shims The dir shims written by physical_refs.py were also committed (PR #542), giving each path two writers: the running robot and git. Any robot whose brain code ran ahead of its checkout wrote the shims as untracked files, and the next git pull / innate update then aborted with 'untracked working tree files would be overwritten by merge'. Make the runtime the only owner: untrack the two committed shims and gitignore */__init__.py under workspace/innate_skills. Robots already stuck un-stick on their next pull (the target tree no longer contains the colliding paths); robots that pulled the tracked copies have them deleted at checkout and regenerated on the next roster publish, with git staying clean since the paths are now ignored. * fix(skills): backstop missing dir shims at agent init; pin shim rendering Review follow-ups for the untracking change: - initialize_agents now recreates missing recording-folder shims from disk (metadata.json marks the folders — no roster needed), closing the window where a tracked agent's `from innate_skills.wave import Wave` fails after the update that deletes the once-tracked shims: brain_client's watcher only covers agent dirs, so an agent broken at boot stayed broken until a manual reload. Create-only — the skills server keeps sole ownership of updates and pruning, so the two writers can't fight over content. - test_physical_refs.py pins the rendered shim (exact content, marker prefix, collision alias, removal marker) — the committed copies were the only thing pinning the template, and they're gone. - gitignore also covers the atomic-write __init__.py.*.tmp leftover, and its comment now names the code-subpackage catch explicitly. - physical.py's ship-in-git comment updated to the new ownership; the identifier-safety rationale for unquoted {name} restored in physical_refs. * chore(skills): drop the shim render tests, tighten the added comments Reverts the test_physical_refs.py added in the previous commit and cuts the comments there back to the load-bearing facts.
(#621) * fix(brain): restore agent_types/skill_types compat shims dropped in #542 PR #542 deleted the brain_client.agent_types and brain_client.skill_types star-import shims, which every pre-#542 custom agent and skill in the field imports — after an OS update, all of them roster as broken with ModuleNotFoundError (all 21 custom agents on R7-27 were down). Restore both shims verbatim, export Agent/SkillRef/InputRef from the innate facade so new agents author against the stable namespace, and add regression tests: the legacy and facade import paths must load an agent onto the roster, and the shims must re-export the same class objects (registration and isinstance depend on identity). * test(brain): legacy skill loads end-to-end through workspace discovery Greptile flagged that the skill shim was only covered by direct-import identity checks while field skills load via discovery. Add a legacy-authored skill (old import path, Interface/RobotState declarations, tuple return) loaded through import_workspace_packages + registered_workspace_skills — the same functions the catalog calls. The fixture clears Skill._registry (other test modules register Skills, some deliberately broken, and discovery both rosters and prunes whatever the live registry holds).
3 participants
What
Three things, in dependency order:
mobility: Mobility,odom: Odometry,image: MainImage, and now also sub-skills (arm_rest: ArmRestPosition) and physical skills (pick_socks = PhysicalSkill("pick_socks")). The baseSkillsupplies what every file used to repeat, and robot state arrives as typed values instead of raw ROS-shaped dicts. Every shipped skill is migrated onto it.Skillsubclass is the registration; ids are namespaced by package (innate-os/<name>,local/<name>,<pack>/<name>); a pack installs by dropping (or symlinking) a folder intoworkspace/. A module that fails to import shows up in the web app marked broken with its error instead of vanishing.pick_any_object— a wrist-camera fine-alignment grasp. It's the first skill written against the new API at full size, which is what shook most of the API out.1. Skills API
Declare by annotating
The type identifies the feed. Plain annotation = guaranteed: the server waits, bounded per feed (cameras 3 s, battery 6 s, everything else 2 s), and fails the run before
execute()if nothing arrives — so no skill needs anif self.x is Noneguard.| None= best effort — and so is a= Noneclass default (image: MainImage | None = Nonedeclares the feed too, it doesn't opt out of injection). Reading an undeclared feed raises with the annotation to add, and your editor flags the typo before you ship.The class name is the skill name (snake_cased), and the class docstring is
guidelines()— so a skill states its purpose exactly once.Composition is a declaration too
A declared sub-skill is constructed per run, wired like a root skill (same run node, interfaces, feeds, invoker, feedback), and shares the parent's cancel latch. It sits on the attribute as a callable:
self.arm_rest()raisesSkillFailed/SkillCancelledinstead of returning a status.PhysicalSkill("id")gives trained/recorded policies the same declaration block and the same call shape, and moves an unknown-id error to run start instead of mid-routine. The old string-based composition path is deleted.Robot state is typed values, not dicts
odom["pose"]["pose"]["position"]["x"]→odom.x.joint_states["position"][5]→arm.gripper. These live in their ownbrain_client/state/package — they were never skill machinery, only the values skills consume:Odometry,Pose(map frame),Battery,Lidar(+min_range()sector helper),Arm,Map(lazy.grid),JointStates,HeadState,Image/MainImage/WristImage(the value is the b64 string,.jpeggives bytes),DepthMap(numpy). Every one is ROS-free and importable outside rclpy.Old dict access still works everywhere via
dictcompat.LegacyMapping— every legacy-injected feed keeps its historical dict shape, soft-deprecated with a warning, no removal scheduled.One result object, not a tuple
SkillOutputis a real object instead of astrsubclass with.databolted on:.message,.status(aSkillResultenum, never a bare string),.datafor structured payloads, and.okas the success check.str(output)and f-strings still give the message, and legacymessage, status = ...unpacking still works behind a deprecation warning.For skill authors this means a skill returns one value —
return "Picked it up"— and signals failure withself.fail(...), rather than hand-assembling(message, SkillResult.FAILURE)tuples and catchingSkillCancelled/SkillFailedto convert them. The framework owns those exceptions now. Every shipped skill is migrated onto it.Also in the base class
self.fail()(NoReturn),self.cancelled/check_cancelled(), per-runself.on_cancel(hook)for braking on the cancelling thread,self.feedback(),self.wait_for(read, timeout),self.storage, andNone/strreturns treated as success. Skill instances are per-run: constructed for the run, disposed at its end.Resources — expensive objects a skill owns, with a per-run lifecycle:
(This means e.g. the Nav2 stack is constructed per navigation rather than kept warm for the process — see Notes for review.)
pyrightconfig.jsonscopes pyright to the skills tree, which is standard-mode clean — and caught a real bug on the way (move_to_cartesian_pose(duration: int)while every caller passed fractional seconds).2. Discovery & packaging
Skillsubclass registers it (like a PyTorchnn.Module). The class is the identity, so files organize freely — several skills per file, a skill split across a subpackage, helpers next to it. One-way doors avoided: skill-id collision behavior is unchanged from main.workspace/is a plain Python package.innate_skills/(shipped) andcustom_skills/(yours, gitignored) are just the two built-in packs; any other folder dropped in — or symlinked in (ln -s /opt/team/skills workspace/team_skills) — is a pack, ids namespaced by folder name. Folder skills work (innate_skills/wave/,pick_socks/). Packages import each other by bare name (from innate_skills import arm_utils).extra_skill_dirs/extra_agent_dirssetting (settings knob, webapp catalog entry, and the media-route serving lanes are gone), and 0.7 no longer scans~/skills/~/agentsin place —migrate_user_data.shmoves both home lanes intoworkspace/custom_*so nothing silently stops loading (sudo-safe viaACTUAL_HOME)./brain/reload_primitivesor save-watcher) covers helper modules too.workspace/skill_lib/now lives in the platform:innate.gemini,innate.geometry,innate.vision, and theArm*exceptions on the manipulation interface.3. pick_any_object
After the base parks the object in the head-camera pick box, the grasp:
WRIST_SEARCH_ARM.wrist_stop_z, where the existing blind ladder finishes the grasp, then close/twist/lift. Persistent tracking loss buys a Gemini re-seed (budget =wrist_steps − 1).Cancel semantics were tightened on review: a Stop cancels the search-pose move too, and once the fingers commit the skill never releases a grasped object — it folds with the grip kept.
Why segmentation, not optical flow
During the descent the object grows ~2.5× in the wrist image and fabric deforms, which slides LK optical-flow patches onto the carpet — this failed repeatedly in live testing. Tracking is HSV color segmentation + CamShift: a likelihood-ratio color model (object vs the floor immediately around it) back-projected each frame is scale- and deformation-proof, and its score honestly reports occlusion/loss instead of confidently tracking carpet.
Observability
No visualization or debug channel: tuning is by editing
PARAMSinpick_any_object.py+ hot-reload, and the skill's observable surface is its log lines (localization fixes, wrist-stage exit reasons, the grasp verdict).Also riding along
mars_arm): closing uses agoal_currentcap instead of software force loops, so grip strength is a hardware limit rather than a control loop;strength/percentare clamped to hardware-safe ranges.arm_control'sgain_mode_is atomic with a per-waypoint re-assert (idle-decay TOCTOU).on_cancelhooks can't deadlock the server; run instances dispose before interfaces stop so@resourceteardowns can command the arm; a Stop racing the invoker's timeout watchdog latches the routine cancel; Gemini retries take a cancel predicate; main/wrist frames are memoized per frame in the 50 Hz injection path; the grasp-verify prompt labels images by the camera that actually supplied them.post_update.sh: enforceroot:<user> 640on an existing/etc/innate.env— a hand-created 600 root:root file is unreadable to the non-root launch readers, silently dropping the service key (proxy "not configured").CameraProvider.start(feeds)subscribes only the declared feeds — raw depth (~600 KB/frame) no longer streams for skills that never asked for it.workspace,innate,innate_proxyas first-party; a repo-wide pass removed narrating comments.Testing
pre-commit run --all-files(ruff + ruff format + clang-format + config guards) clean; pyright standard-mode clean on the skills tree.ci/run_integration_tests.sh. The no-ROS CI bucket now runs onlytest_fake_cloud_selftest.pyandtest_config_validation.py;test_backwards_compat.pyis deleted. Everything below is manual verification — a reviewer should not read this section as "the refactor is covered".innate/geometry.pyverified bit-identical to the math it replaced (round-trip + old-vs-new comparison over sample poses).mars(hot-reloaded via/brain/reload_primitives).Notes for review
wrist_kx/wrist_ky) are unverified against the physical wrist-cam mount orientation. If the servo diverges on hardware, flip them inPARAMS+ hot-reload; divergence is bounded bywrist_step_maxand the reach clamp, so the failure mode is a mis-aimed grasp and a timeout, not a runaway arm. Until confirmed on-robot, treat the wrist stage as experimental;wrist_steps=0falls back to the proven blind grasp.local_frame=Truegoals must fill a cold TF buffer within the 2 s lookup timeout — sanity-check local goals and mid-navigation cancels on a robot.