fix(cameras): release vanished-camera capture handles; route servo-bus and camera errors to honest hints - #77
Open
Mokuroh54 wants to merge 10 commits into
Open
fix(cameras): release vanished-camera capture handles; route servo-bus and camera errors to honest hints#77Mokuroh54 wants to merge 10 commits into
Mokuroh54 wants to merge 10 commits into
Conversation
A transient camera failure during robot.connect() leaked the follower bus
and the already-opened cameras' background read threads for the rest of
the process, so every later recording attempt failed until makerlab was
restarted — first with the misleading "FeetechMotorsBus is already
connected", then by timing out against the leaked read threads still
holding the OS camera devices.
The retry path did call robot.disconnect() (under suppress), but that is
structurally incapable of releasing a partially-connected robot:
SOFollower.is_connected is all-or-nothing (bus.is_connected and all(
cam.is_connected ...)) and disconnect() carries @check_if_not_connected.
connect() opens the bus, then the cameras in dict order, so a camera
failure leaves every later camera unopened -> is_connected is False ->
the guard raises before releasing a single component, and suppress()
swallows it. The teardown was a no-op and the retry was guaranteed to
hit the still-open bus.
The leak also crossed sessions: the read-failure spam for OpenCVCamera(1)
precedes the first connect attempt, i.e. that thread belonged to the
previous failed session and was what made this session's camera come up
frame-dead. The existing sleep(2.0) "wait for camera resources to be
released" cannot help, because the holder is in-process.
Add teleoperate.force_disconnect_partial(): component-wise teardown with
each component independently guarded (cameras from .cameras, tolerating
DeviceNotConnectedError for never-opened ones; then buses via
_device_buses(), covering bimanual sub-arms). Safe on fully, partially,
and never-connected devices. Call it on every failed robot-connect
attempt, not just retried ones, so a terminal failure also leaves the
process clean — and on the teleop-connect failure path, so a leader
failure cannot strand the follower.
Recording is the only in-process flow affected: teleop and calibration
build follower configs with cameras == {} (bus-only is_connected, so
disconnect() works) and rollout runs as a subprocess.
Tests pin the mechanism against lerobot's real check_if_not_connected
decorator and real is_connected shape, so they fail if upstream ever
makes disconnect() tolerant of partial state.
Recording N12 (P1).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
(cherry picked from commit 025d4da)
…artial bus.disconnect() disables torque internally too, but a single motor's failed write there aborts the loop and leaves every motor after it energized (the same failure mode force_disable_torque's docstring already documents). force_disconnect_partial relied on disconnect() alone for torque release; call force_disable_torque first, matching the ordering _cleanup_after_setup_failure already uses for the same "torque may be on after an incomplete connect" situation.
…ailure, return problems Addresses three review findings on force_disconnect_partial: - B: force_disable_torque ran unconditionally against every bus returned by _device_buses(), including one that never opened (the ordinary "wrong port"/"arm unplugged" case, since buses are constructed from config, not from a successful connect). That wrote to a closed port for every motor and printed the single most alarming string the system can emit -- "TORQUE MAY STILL BE ENABLED ... unplug its power" -- for an arm that was never energized, on what is the most common failure path by far. Fixed at the source (force_disable_torque's own per-bus loop) so every caller benefits, not just this one; test doubles that don't model connection state default to connected so existing torque-only tests are unaffected. - C2: if bus.disconnect() itself raised, force_disconnect_partial just logged a warning and moved on, leaking the port handle for the rest of the process -- the one place in the codebase that gives up, when every other teardown path (utils/devices.py's _force_close_device_resources) treats a serial port as a trust boundary and force-closes past a failed disconnect. Added the same clearPort/is_using/closePort fallback, scoped per-bus so bimanual sub-arms each get their own. - C3: the function discarded force_disable_torque's problem list and returned None, unlike its sibling _cleanup_after_setup_failure. Now returns list[str], same shape, so callers can surface a partial teardown instead of it vanishing into a debug log. Also fixes five tests left importing from the pre-rename `makerlab` module (rebase artifact -- line-based merge can't rewrite import paths inside function bodies); the package is `makermodslab` since the 2026-08-04 rename.
The connect-retry path this branch adds was silent in two ways:
- force_disconnect_partial logs through makermodslab.teleoperate's
logger, which wasn't in _RECORD_LOG_LOGGER_NAMES -- a real teardown
failure ("Could not release robot bus on...") only ever reached the
server console, invisible to an operator watching the Record page
during a failed/retried connect. Added the teleoperate logger to the
attach list (the existing ring-buffer mechanism already generalizes to
any logger name, no new plumbing).
- The retry window can now run a full component-wise teardown plus up to
three 2s backoff sleeps behind one static "Connecting arm & cameras..."
label, with no signal anything is happening. Added a
"reconnecting_robot" current_phase substep (mirrors the existing
connecting_robot/connecting_teleop pattern) plus connect_retry_attempt/
connect_retry_max on the status payload, logged at INFO through
record's own logger so it lands in the visible panel too. Frontend
reads the new fields to show "Camera hiccup, retrying (2/3)..."; an
unrecognized phase string already falls back to the neutral
"preparing" styling, so this is additive.
… force_disconnect_partial docstring connect_retry_attempt was set to the attempt that just failed instead of the one about to run, disagreeing with the adjacent log line's (attempt + 1) convention that the Record page's retry UI is meant to mirror.
… on bus liveness Move the reconnecting_robot phase/attempt-counter update before the component-wise teardown instead of after: the teardown is the slow part (a wedged camera's disconnect joins a read thread waiting out a frame timeout), so bracketing only the backoff sleep left the operator staring at a static "Connecting arm & cameras..." for the window this substep exists to explain. Reset connect_retry_attempt to 0 once a retry succeeds so it stays true to its "0 unless a retry is in flight" contract. Add a bus-liveness probe (_bus_has_a_responding_motor) so force_disable_torque only warns "TORQUE MAY STILL BE ENABLED" when a motor actually failed to respond to the disable write, not whenever the port opened but no motor answers a ping (unpowered/browned-out/wrong-baud arm) - that case now gets its own accurate message instead of a misleading rigid-arm warning. force_disconnect_partial's bus.disconnect() call now passes disable_torque=False since force_disable_torque already did that pass motor-by-motor, matching the pattern used elsewhere in this codebase (rollout.py, motor_power.py, identify.py, auto_calibrate.py).
…learer camera-connect hints
Salvaged from the pre-PR#41 WIP stash (the browser-stream release gate
and pause flag it also carried are obsolete under backend previews and
were discarded):
- force_disconnect_partial: a camera whose device vanished mid-connect
reports is_connected=False with no read thread, yet its videocapture
still holds the OS capture session — left in place it poisons every
later open of the replugged camera until restart. Release the raw
handle directly on that path.
- friendly_hint: translate 'failed to open camera' (unplugged / stale
index — ordered before the network branch that would misclassify
lerobot's ConnectionError) and the connect-time turbulence family
('failed to set fps' / warmup-frame timeouts, where a vanished device
reports a nonsense actual_fps) into held-or-unplugged guidance.
Carried forward from rig 07d5f4e, with the makerlab -> MakerMods Lab
naming from rig 342f4cd already folded in (the original text predates
that rebrand and would have imported a module that no longer exists).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ld not download the model" lerobot raises every serial failure as ConnectionError, and the Hub-network hint branch matched the bare token "connectionerror" (whose second conjunct was self-satisfied by the word itself), so a robot.connect() torque/ID failure surfaced as a model-download failure. Add a servo-bus branch keyed on motors-bus phrasing, and require a Hub-specific token or the download-step prefix for the download hint. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Mokuroh54
added
bug
The connect-time turbulence branch keyed on "do not match configured", which cannot reach a synchronous caller. lerobot detects a wrong native format in _postprocess_image, whose only caller is _read_loop (camera_opencv.py:453); connect()'s warmup goes through async_read. The mismatch text therefore never propagates — the caller sees the dead reader, "read thread is not running", which this branch did not match and which fell through to no hint at all. So the branch handled the string that cannot arrive and missed the one that does. Add "read thread is not running"; keep "do not match configured" as a forward guard, documented as unreachable today so it is never again the sole marker for that failure. Marker set now matches record.py's _is_transient_camera_error, keeping the retry classifier and the terminal hint in sync: anything retried there ends with advice here. Propagation path established by j-ctang in #38; verified against the pinned lerobot v0.6.0 build. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three collisions, all cases where this branch and main fixed the same thing independently while neither could see the other. force_disable_torque's liveness probe. Both sides added _bus_has_a_responding_motor to stop reporting "TORQUE MAY STILL BE ENABLED — unplug its power" on an arm that has no power. This branch probed first and skipped the torque pass on a silent bus; main attempts every write and consults the probe only afterwards, to pick the wording. Took main's: a degraded-but-recoverable bus can fail every zero-retry ping while the retried (num_retry=5) write still lands, so gating the write on the probe would leave a genuinely energized arm rigid. Main also ships the regression test for exactly that case (test_force_disable_torque_still_writes_when_the_probe_fails_but_the_bus_is_alive), and the _FakeConnectableBus ping_dead flag it needs. friendly_hint's "failed to set fps" marker. Merge base had neither side; both invented a rule for it, meaning opposite things — this branch read it as camera-session turbulence (vanished device reports a nonsense actual_fps), main as a permanent misconfiguration (an operator typed an fps the device can't do). Git kept both branches, and the turbulence one shadowed main's, making its hint dead code and failing test_every_retryable_camera_marker_that_can_be_permanent_has_a_hint. Dropped the marker from the turbulence set: nothing in the string separates the two causes, "click Auto" is actionable for the misconfiguration and harmless for a vanished device, and the reverse is not. No test on this branch pinned the turbulence reading. The markers that stay there cannot be produced by a settings mistake. The vanished-camera capture release in force_disconnect_partial — this branch's actual subject — is kept as-is against main's `pass`, with its test.
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
2 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.
Carved from the
rigintegration branch. Two related fixes: one real resource leak in camera teardown, and the hint routing that made both camera and arm failures lie to the user.(a) A vanished camera's stale capture handle poisons every later open
force_disconnect_partialreleases each camera viacam.disconnect()and swallowsDeviceNotConnectedErroras "never opened — nothing to do". That is right for the common case, but one real failure lands there too and is not harmless.When a camera's device vanishes mid-connect — unplugged between the cv2 open and the read thread starting — lerobot's
OpenCVCameraends up withis_connected == False(isOpened()is False on the dead device) andthread is None. Itsdisconnect()guard is literallyif not self.is_connected and self.thread is None: raise, so it raises without releasingself.videocapture. TheVideoCaptureobject still holds the OS capture session.Left in place, that stale in-process session poisons every subsequent open of the camera once it is replugged — it opens "successfully" but comes up with a degraded fps profile or delivers no frames at all — and stays broken until MakerMods Lab restarts. So on exactly that path we now release the raw handle directly and null it out, with a warning if even that fails.
This is the same family as the documented macOS AVFoundation stale-device behaviour: the process-local handle outlives the physical device, and nothing else reclaims it.
(b) Two hint mis-routings
friendly_hintturns raw exception text into plain-language guidance. Two families were being answered with the wrong story:Camera failures read as network failures. lerobot raises camera-open failures as
ConnectionError, and the Hub-download hint branch keyed on a bare"connectionerror"token — so an unplugged camera surfaced as "couldn't download the model". Added two camera branches, ordered ahead of the Hub branches:"failed to open" + "camera"→ the device is unplugged, or the index went stale after a port change."failed to set fps", warmup-frame timeouts,"do not match configured") → the camera opened but came up degraded: either another app still holds it or it was just unplugged. A vanished device reports a nonsenseactual_fps, which is why lerobot's own message is misleading here. This branch must precede the slow-frames branch, since"timed out waiting for frame"would otherwise be caught by"waiting for frame".Servo-bus errors read as model-download failures. lerobot raises every serial failure as
ConnectionError, so arobot.connect()torque/ID failure hit the same Hub branch and surfaced as a download failure — the second conjunct of that condition was self-satisfied by the token"connectionerror"itself, which contains"connect".Correction worth flagging: the functional part of this fix — the
txrxresult/motors-bus branch plus requiring a Hub-specific token or thein_download_stepprefix — already landed onmainvia feat(inference): multi-episode evaluation suite — persistent runner, RTC engine, honest logs #63 (43d185b), and is therefore already present on this PR's base. Whate1c5c87contributes here is only the comment reconciling the two hint families now that the camera branches sit alongside the arm branches. Reviewers should read commit 2 as documentation, not behaviour.Stacked on #37
Base branch is
fix/record-connect-partial-teardown(#37's head), notmain. The teleoperate hunk edits thecam.disconnect()/except DeviceNotConnectedErrorcamera-release loop that #37's lineage introduced — that loop does not exist onmain, so this cannot be based there. Classic branch-of-branch stack: merge #37 first, and this PR retargets tomainautomatically. Review the diff of the two commits below, not the full base comparison.Tests
tests/test_teleoperate.pygainstest_force_disconnect_partial_releases_vanished_cameras_capture_handle, which modelsOpenCVCamera's exact shape at the vanished-device point (is_connected=False,thread=None, livevideocapture) with lerobot's disconnect guard reproduced verbatim, and asserts the raw handle is released, nulled, and that the bus still gets released afterwards. Existingfriendly_hinttests intests/test_rollout.pystill pass unchanged — the new branches are additive and sit ahead of branches none of them exercise.Checks:
pytest tests/test_teleoperate.py tests/test_rollout.py tests/test_record.py→ 324 passed.ruff check+ruff format --checkon touched files → clean.pre-commit run --all-files→ all 19 hooks pass.Provenance
07d5f4e— fix(teleoperate): release a vanished camera's stale capture handle; clearer camera-connect hintse1c5c87— fix(inference): route servo-bus errors to an arm hint instead of "could not download the model"Carried forward with one adaptation:
07d5f4epredates themakerlab→makermodslabrebrand (342f4cd), so the test'sfrom makerlab.teleoperate import ...and two prose references were updated to match the current package name. Verbatim, the test would have failed to import.🤖 Generated with Claude Code