From 46ad16ec33c3364d9196ab9fd6d20d0717998fe8 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Fri, 11 Sep 2026 21:44:29 +0000 Subject: [PATCH 01/10] Offer the Franka arm's error recovery on a channel a console asks on MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The arm ran recover_from_errors only inside the driver's own loop, on no channel, and that loop fires only while state().error is set. A latched fault leaves the arm in mode Reflex with error reading 0, so the automatic path never runs and nothing outside the driver can reach it. Add a recover receiver and a recovery_result emitter on franka.Robot. The run loop reads the receiver each tick; an ask runs recover_from_errors and emits whether it cleared. This is the positronic half of internal#1255; the console binds console.recover_arm to recover and reports recovery_result. Ticket: none — positronic half of internal#1255; PR body links it. --- positronic/drivers/roboarm/franka.py | 10 +++ .../drivers/roboarm/tests/test_franka.py | 63 ++++++++++++++++++- 2 files changed, 72 insertions(+), 1 deletion(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index b045a95b7..929d3d4f9 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -499,6 +499,11 @@ def __init__( self.sync_move = pimm.calls.ControlSystemHandler[command.CommandType, None](self) self.state = pimm.ControlSystemEmitter[FrankaState](self) self.robot_meta = pimm.ControlSystemEmitter(self) + # A console asks here to clear a latched fault the automatic path never reaches: the arm can sit + # in libfranka mode Reflex while ``state().error`` reads 0, so nothing else fires recovery. The + # emitter reports whether the recovery cleared it. + self.recover = pimm.ControlSystemReceiver[bool](self) + self.recovery_result = pimm.ControlSystemEmitter[bool](self) self._load = load self._collision_coeff = collision_coeff self._desk_credentials = _read_desk_credentials() if manage_desk else None @@ -629,6 +634,11 @@ def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[p goal = robot.goal() arm.note_refusals(goal) + if pimm.read_updated(self.recover) is not None: + cleared = robot.recover_from_errors() + logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') + self.recovery_result.emit(cleared) + in_error, entered_error = _check_error(st.error != 0, in_error) if entered_error: logger.warning(f'Robot error: {st.error_message}') diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index e58f81ca9..14bca55e2 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -110,8 +110,9 @@ def set_target_joints(self, target) -> None: self.targets.append(np.asarray(target, dtype=np.float64)) self._polls = 0 - def recover_from_errors(self) -> None: + def recover_from_errors(self) -> bool: self._record(Call.RECOVER_FROM_ERRORS) + return self.error == 0 def stop(self) -> None: self.calls.append(Call.STOP) @@ -1077,3 +1078,63 @@ def test_a_command_pinning_no_mode_returns_the_arm_to_its_native_law(desk): _drive(loop, clock) assert isinstance(arm.modes[mark], franka.pf.InternalImpedance) + + +def test_a_console_recover_ask_runs_recovery_and_reports_the_result(desk): + """A console asks the arm to clear a latched fault, and the driver runs the recovery and reports it.""" + arm = FakeArm(PARK) + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + ask = ManualCommandReceiver() + driver.recover._bind(ask) + results = RecordingEmitter() + driver.recovery_result._bind(results) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(3): # init + the opening move + next(loop) + before = arm.calls.count(Call.RECOVER_FROM_ERRORS) + ask.push(True) + next(loop) + + assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before + 1 + assert results.emitted[-1][1] is True # a clear arm reports the recovery cleared + + +def test_a_console_recover_ask_reports_a_fault_that_does_not_clear(desk): + """The recovery a console asks for reaches a fault libfranka will not clear, and the driver says so.""" + arm = FakeArm(PARK) + arm.error = 1 # a fault recover_from_errors does not clear + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + ask = ManualCommandReceiver() + driver.recover._bind(ask) + results = RecordingEmitter() + driver.recovery_result._bind(results) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(3): # init + the opening move + next(loop) + ask.push(True) + next(loop) + + assert results.emitted[-1][1] is False + + +def test_the_driver_reports_no_recovery_result_without_an_ask(desk): + """The result is the answer to an ask, so an untouched arm reports none.""" + arm = FakeArm(PARK) + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + driver.recover._bind(ManualCommandReceiver()) + results = RecordingEmitter() + driver.recovery_result._bind(results) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(5): + next(loop) + + assert results.emitted == [] From fde40cfa586f4ea7eca98cd36489691256892d26 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 08:08:12 +0000 Subject: [PATCH 02/10] Serve the console's recovery as a CALL, so the reply is the answer to that ask MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two Codex P2 findings, and one shape answers both. `recover` and `recovery_result` were independent signals, so nothing tied a result to the request it came from; and a signal named for an activity carried a bare bool. `pimm.calls` is the repo's request/reply mechanism and this class already uses it for `sync_move`: a call is answered once, the answer is bound to its request, and the contract holds across a process boundary, which the console needs. So `recover` is now a `ControlSystemHandler[None, bool]` and there is no second port — the reply IS whether the arm came out of error. The platform half binds a `ControlSystemCaller` rather than an emitter. Nothing binds it yet (platform#479's control greys itself while the wire is bound to nothing), so no merged code changes shape. The three driver tests call through `_recoverer`, the way the existing move tests call through `_mover`: the fault clears, the fault does not clear, and a loop nobody called runs no recovery. Ticket: Positronic-Robotics/internal#1285 #refs --- positronic/drivers/roboarm/franka.py | 11 ++--- .../drivers/roboarm/tests/test_franka.py | 46 ++++++++++--------- 2 files changed, 29 insertions(+), 28 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index 929d3d4f9..130494896 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -499,11 +499,10 @@ def __init__( self.sync_move = pimm.calls.ControlSystemHandler[command.CommandType, None](self) self.state = pimm.ControlSystemEmitter[FrankaState](self) self.robot_meta = pimm.ControlSystemEmitter(self) - # A console asks here to clear a latched fault the automatic path never reaches: the arm can sit + # A console calls here to clear a latched fault the automatic path never reaches: the arm can sit # in libfranka mode Reflex while ``state().error`` reads 0, so nothing else fires recovery. The - # emitter reports whether the recovery cleared it. - self.recover = pimm.ControlSystemReceiver[bool](self) - self.recovery_result = pimm.ControlSystemEmitter[bool](self) + # reply is whether the arm came out of error. + self.recover = pimm.calls.ControlSystemHandler[None, bool](self) self._load = load self._collision_coeff = collision_coeff self._desk_credentials = _read_desk_credentials() if manage_desk else None @@ -634,10 +633,10 @@ def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[p goal = robot.goal() arm.note_refusals(goal) - if pimm.read_updated(self.recover) is not None: + for asked_to_recover in self.recover.incoming(): cleared = robot.recover_from_errors() logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') - self.recovery_result.emit(cleared) + asked_to_recover.set_result(cleared) in_error, entered_error = _check_error(st.error != 0, in_error) if entered_error: diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index 14bca55e2..07d7ad974 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -217,6 +217,13 @@ def _mover(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[command return caller +def _recoverer(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[None, bool]: + """A caller on ``driver.recover``, the same way ``_mover`` calls a move.""" + caller = pimm.calls.ControlSystemCaller[None, bool](driver) + wire_call(world, caller, driver.recover) + return caller + + def test_park_drives_the_arm_to_the_park_pose(): arm = FakeArm(JOGGED) @@ -1080,61 +1087,56 @@ def test_a_command_pinning_no_mode_returns_the_arm_to_its_native_law(desk): assert isinstance(arm.modes[mark], franka.pf.InternalImpedance) -def test_a_console_recover_ask_runs_recovery_and_reports_the_result(desk): - """A console asks the arm to clear a latched fault, and the driver runs the recovery and reports it.""" +def test_a_console_recover_call_is_answered_that_the_fault_cleared(desk, world): + """A console calls the arm to clear a latched fault: the driver runs the recovery and the answer to + THAT call carries what it returned.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) - ask = ManualCommandReceiver() - driver.recover._bind(ask) - results = RecordingEmitter() - driver.recovery_result._bind(results) clock = MockClock() loop = driver.run(StopFlag(), clock) for _ in range(3): # init + the opening move next(loop) before = arm.calls.count(Call.RECOVER_FROM_ERRORS) - ask.push(True) + answer = _recoverer(world, driver)(None) next(loop) assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before + 1 - assert results.emitted[-1][1] is True # a clear arm reports the recovery cleared + assert answer.result() is True # a clear arm came out of error -def test_a_console_recover_ask_reports_a_fault_that_does_not_clear(desk): - """The recovery a console asks for reaches a fault libfranka will not clear, and the driver says so.""" +def test_a_console_recover_call_is_answered_that_the_fault_did_not_clear(desk, world): + """The recovery a console calls for reaches a fault libfranka will not clear, and the answer says so.""" arm = FakeArm(PARK) arm.error = 1 # a fault recover_from_errors does not clear driver = _driver(arm) driver.state._bind(RecordingEmitter()) - ask = ManualCommandReceiver() - driver.recover._bind(ask) - results = RecordingEmitter() - driver.recovery_result._bind(results) clock = MockClock() loop = driver.run(StopFlag(), clock) for _ in range(3): # init + the opening move next(loop) - ask.push(True) + answer = _recoverer(world, driver)(None) next(loop) - assert results.emitted[-1][1] is False + assert answer.result() is False -def test_the_driver_reports_no_recovery_result_without_an_ask(desk): - """The result is the answer to an ask, so an untouched arm reports none.""" +def test_an_arm_nobody_called_runs_no_recovery(desk, world): + """The boundary of the two above: the recovery is the answer to a call, so a loop nobody called + never runs one.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) - driver.recover._bind(ManualCommandReceiver()) - results = RecordingEmitter() - driver.recovery_result._bind(results) + _recoverer(world, driver) clock = MockClock() loop = driver.run(StopFlag(), clock) + for _ in range(3): # init + the opening move + next(loop) + before = arm.calls.count(Call.RECOVER_FROM_ERRORS) for _ in range(5): next(loop) - assert results.emitted == [] + assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before From a5495a86a0d484432bb5140abb07304b9d02bc28 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 08:10:54 +0000 Subject: [PATCH 03/10] Say what the untouched-arm test pins, without ranking it against its neighbours The writing check on the last push: the docstring opened by announcing the test's place in a sequence. Ticket: Positronic-Robotics/internal#1285 #refs --- positronic/drivers/roboarm/tests/test_franka.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index 07d7ad974..e57669f28 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -1124,8 +1124,7 @@ def test_a_console_recover_call_is_answered_that_the_fault_did_not_clear(desk, w def test_an_arm_nobody_called_runs_no_recovery(desk, world): - """The boundary of the two above: the recovery is the answer to a call, so a loop nobody called - never runs one.""" + """The recovery is the answer to a call, so a loop nobody called never runs one.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) From 9b69f8aed533ad53fe9bd51356cdf3d490269e92 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 08:12:25 +0000 Subject: [PATCH 04/10] Cut the recover port's comment to the footgun and the reply The writing check on the last push: three lines where two carry it, and the middle one restated the rationale the pull request body already gives. Ticket: Positronic-Robotics/internal#1285 #refs --- positronic/drivers/roboarm/franka.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index 130494896..65b15f035 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -499,8 +499,7 @@ def __init__( self.sync_move = pimm.calls.ControlSystemHandler[command.CommandType, None](self) self.state = pimm.ControlSystemEmitter[FrankaState](self) self.robot_meta = pimm.ControlSystemEmitter(self) - # A console calls here to clear a latched fault the automatic path never reaches: the arm can sit - # in libfranka mode Reflex while ``state().error`` reads 0, so nothing else fires recovery. The + # FOOTGUN: recovers whatever ``state().error`` reads, since a latched Reflex reads 0. The # reply is whether the arm came out of error. self.recover = pimm.calls.ControlSystemHandler[None, bool](self) self._load = load From 3e2ed49c1d12592ce672677a21373a16f366c86c Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 08:34:12 +0000 Subject: [PATCH 05/10] Answer a recovery call the vendor fails, and name the outcome it carries Two findings from the review of 615057d1, both on the `recover` port. A `recover_from_errors()` that raises left the caller waiting for ever. The handler takes the call off `incoming()` before it runs the vendor call, so `fail_queued()` can no longer reach it, and the exception ended the driver loop with the console's `Answer` still incomplete. The block now runs under `pimm.calls.raise_to`, the way `sync_move` does in the Kinova and SO-101 drivers: the caller hears the exception and the loop serves the next call. The reply is a `RecoveryOutcome` rather than a bare `bool`. `recover` acts and reports, which `misleading-name` answers with an action name over an enum whose members carry the verdict, so `CLEARED` / `NOT_CLEARED` says at the call site what the port's comment had to say in prose. That comment loses the sentence and keeps the footgun. Nothing binds the port yet, so no consumer changes shape. The platform caller tracked on Positronic-Robotics/internal#1285 binds `RecoveryOutcome`. A fourth driver test pins the vendor raising: the caller gets that exception, and the loop answers the call after it. Ticket: Positronic-Robotics/internal#1255 #refs --- positronic/drivers/roboarm/franka.py | 21 +++++++--- .../drivers/roboarm/tests/test_franka.py | 42 ++++++++++++++++--- 2 files changed, 51 insertions(+), 12 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index 65b15f035..fc6e6106b 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -6,6 +6,7 @@ import time import xml.etree.ElementTree as ET from collections.abc import Callable, Generator, Iterator, Mapping +from enum import Enum, auto from pathlib import Path from typing import Any, NamedTuple @@ -460,6 +461,13 @@ def close_if_idle(self, goal: pf.Goal) -> None: self._closed = True +class RecoveryOutcome(Enum): + """Whether the arm came out of error.""" + + CLEARED = auto() + NOT_CLEARED = auto() + + class Robot(pimm.ControlSystem): def __init__( self, @@ -499,9 +507,8 @@ def __init__( self.sync_move = pimm.calls.ControlSystemHandler[command.CommandType, None](self) self.state = pimm.ControlSystemEmitter[FrankaState](self) self.robot_meta = pimm.ControlSystemEmitter(self) - # FOOTGUN: recovers whatever ``state().error`` reads, since a latched Reflex reads 0. The - # reply is whether the arm came out of error. - self.recover = pimm.calls.ControlSystemHandler[None, bool](self) + # FOOTGUN: recovers whatever ``state().error`` reads, since a latched Reflex reads 0. + self.recover = pimm.calls.ControlSystemHandler[None, RecoveryOutcome](self) self._load = load self._collision_coeff = collision_coeff self._desk_credentials = _read_desk_credentials() if manage_desk else None @@ -633,9 +640,11 @@ def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[p arm.note_refusals(goal) for asked_to_recover in self.recover.incoming(): - cleared = robot.recover_from_errors() - logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') - asked_to_recover.set_result(cleared) + with pimm.calls.raise_to(asked_to_recover): + cleared = robot.recover_from_errors() + logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') + outcome = RecoveryOutcome.CLEARED if cleared else RecoveryOutcome.NOT_CLEARED + asked_to_recover.set_result(outcome) in_error, entered_error = _check_error(st.error != 0, in_error) if entered_error: diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index e57669f28..c2bcdefd1 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -65,8 +65,9 @@ class FakeArm: """In-memory ``pf.Robot``: a commanded joint target is reached after ``polls_to_reach`` reads of ``goal``. ``goal_status`` pins the reported status, so a move that never lands can be scripted; ``raises``, once - set, is what every call but ``stop`` raises, and ``ik_raises`` what only the solver raises; ``error`` is - the vendor fault flag every state carries. + set, is what every call but ``stop`` raises, ``ik_raises`` what only the solver raises, and + ``recover_raises`` what only ``recover_from_errors`` raises; ``error`` is the vendor fault flag every + state carries. """ def __init__(self, q, *, polls_to_reach: int = 2, goal_status: 'franka.pf.GoalStatus | None' = None): @@ -78,6 +79,7 @@ def __init__(self, q, *, polls_to_reach: int = 2, goal_status: 'franka.pf.GoalSt self.raises: Exception | None = None self.raises_once: Exception | None = None self.ik_raises: Exception | None = None + self.recover_raises: Exception | None = None self.polls_to_reach = polls_to_reach self._polls = 0 self.goal_status = goal_status @@ -112,6 +114,8 @@ def set_target_joints(self, target) -> None: def recover_from_errors(self) -> bool: self._record(Call.RECOVER_FROM_ERRORS) + if self.recover_raises is not None: + raise self.recover_raises return self.error == 0 def stop(self) -> None: @@ -217,9 +221,9 @@ def _mover(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[command return caller -def _recoverer(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[None, bool]: +def _recoverer(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[None, franka.RecoveryOutcome]: """A caller on ``driver.recover``, the same way ``_mover`` calls a move.""" - caller = pimm.calls.ControlSystemCaller[None, bool](driver) + caller = pimm.calls.ControlSystemCaller[None, franka.RecoveryOutcome](driver) wire_call(world, caller, driver.recover) return caller @@ -1103,7 +1107,7 @@ def test_a_console_recover_call_is_answered_that_the_fault_cleared(desk, world): next(loop) assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before + 1 - assert answer.result() is True # a clear arm came out of error + assert answer.result() is franka.RecoveryOutcome.CLEARED def test_a_console_recover_call_is_answered_that_the_fault_did_not_clear(desk, world): @@ -1120,7 +1124,33 @@ def test_a_console_recover_call_is_answered_that_the_fault_did_not_clear(desk, w answer = _recoverer(world, driver)(None) next(loop) - assert answer.result() is False + assert answer.result() is franka.RecoveryOutcome.NOT_CLEARED + + +def test_a_console_recover_call_is_answered_when_the_vendor_raises(desk, world): + """libfranka throws mid-recovery: the caller hears that exception, and the driver loop carries on rather + than leaving a call it has already taken off the queue unanswered for ever.""" + arm = FakeArm(PARK) + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + recover = _recoverer(world, driver) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(3): # init + the opening move, both of which recover on their own + next(loop) + arm.recover_raises = RuntimeError('libfranka: control command rejected') + answer = recover(None) + next(loop) + + assert answer.done() + with pytest.raises(RuntimeError, match='control command rejected'): + answer.result() + + arm.recover_raises = None + answer = recover(None) + next(loop) + assert answer.result() is franka.RecoveryOutcome.CLEARED def test_an_arm_nobody_called_runs_no_recovery(desk, world): From b7e3c06dfb0f5f3ae56766b6cf62290d1d41521d Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 10:32:18 +0000 Subject: [PATCH 06/10] Run the Franka recovery once a tick, from one call site `Robot.run` called `recover_from_errors` twice on one tick. The console handler called it under `raise_to`; the automatic retry called it unguarded a few lines later. Two defects came from that. A tick that read a fault, took a console call and met a vendor throw ran the recovery twice. The guard handed the first throw to the caller and swallowed it. Execution reached the unguarded call, the vendor threw again, and that exception left `run` and stopped the driver. This is the review's open P2. The automatic retry reads `in_error` from the state at the top of the tick. A console call that clears the fault leaves that reading one call out of date, so the retry ran a second recovery against a fault that was already gone. A flag that skips the retry hides this one, which is why the shape changes instead. `_recover` now runs the recovery once and answers every console that asked on that tick. A throw reaches those callers when a console asked. A throw reaches nobody when none asked, so it ends the run, as it does on `main`. A tick that recovers commands nothing: the next `robot.state()` is what reads the arm the recovery left behind. The vendor-raises test now sets `error` to 1, so it drives the path that crashed, and it counts the recoveries on that tick. Three tests come with it: a console call that clears the fault runs no second recovery, an arm in error recovers with nobody asking, and a throw nobody asked for ends the run. The fake arm gains `recover_clears`, so a test can script a recovery that works. Ticket: Positronic-Robotics/internal#1255 #refs --- positronic/drivers/roboarm/franka.py | 35 +++++--- .../drivers/roboarm/tests/test_franka.py | 81 +++++++++++++++++-- 2 files changed, 100 insertions(+), 16 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index fc6e6106b..352a18ac2 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -468,6 +468,28 @@ class RecoveryOutcome(Enum): NOT_CLEARED = auto() +def _recover(robot: pf.Robot, asked: list[pimm.calls.Call[None, RecoveryOutcome]]) -> None: + """Run the arm's error recovery once, and answer every console that asked for it on this tick. + + A console that asked hears what the recovery returned, the vendor's throw included. A recovery nobody + asked for has no one to hear it, so its throw ends the run. + """ + try: + cleared = robot.recover_from_errors() + except Exception as exc: + if not asked: + raise + logger.exception('The recovery a console asked for failed') + for call in asked: + call.set_exception(exc) # the consoles that asked hold the failure, so the run carries on + return + if asked: + logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') + outcome = RecoveryOutcome.CLEARED if cleared else RecoveryOutcome.NOT_CLEARED + for call in asked: + call.set_result(outcome) + + class Robot(pimm.ControlSystem): def __init__( self, @@ -639,19 +661,14 @@ def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[p goal = robot.goal() arm.note_refusals(goal) - for asked_to_recover in self.recover.incoming(): - with pimm.calls.raise_to(asked_to_recover): - cleared = robot.recover_from_errors() - logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') - outcome = RecoveryOutcome.CLEARED if cleared else RecoveryOutcome.NOT_CLEARED - asked_to_recover.set_result(outcome) - in_error, entered_error = _check_error(st.error != 0, in_error) if entered_error: logger.warning(f'Robot error: {st.error_message}') - if in_error: - robot.recover_from_errors() + asked_to_recover = list(self.recover.incoming()) + if asked_to_recover or in_error: + _recover(robot, asked_to_recover) + # What the recovery left behind is what the next tick reads, so this one commands nothing. yield arm.limiter.wait() continue diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index c2bcdefd1..b4810dc55 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -67,7 +67,7 @@ class FakeArm: ``goal_status`` pins the reported status, so a move that never lands can be scripted; ``raises``, once set, is what every call but ``stop`` raises, ``ik_raises`` what only the solver raises, and ``recover_raises`` what only ``recover_from_errors`` raises; ``error`` is the vendor fault flag every - state carries. + state carries, and ``recover_clears`` whether a recovery puts it back to 0. """ def __init__(self, q, *, polls_to_reach: int = 2, goal_status: 'franka.pf.GoalStatus | None' = None): @@ -80,6 +80,7 @@ def __init__(self, q, *, polls_to_reach: int = 2, goal_status: 'franka.pf.GoalSt self.raises_once: Exception | None = None self.ik_raises: Exception | None = None self.recover_raises: Exception | None = None + self.recover_clears = False self.polls_to_reach = polls_to_reach self._polls = 0 self.goal_status = goal_status @@ -116,6 +117,8 @@ def recover_from_errors(self) -> bool: self._record(Call.RECOVER_FROM_ERRORS) if self.recover_raises is not None: raise self.recover_raises + if self.recover_clears: + self.error = 0 return self.error == 0 def stop(self) -> None: @@ -1127,9 +1130,10 @@ def test_a_console_recover_call_is_answered_that_the_fault_did_not_clear(desk, w assert answer.result() is franka.RecoveryOutcome.NOT_CLEARED -def test_a_console_recover_call_is_answered_when_the_vendor_raises(desk, world): - """libfranka throws mid-recovery: the caller hears that exception, and the driver loop carries on rather - than leaving a call it has already taken off the queue unanswered for ever.""" +def test_a_recovery_the_vendor_fails_answers_the_console_rather_than_ending_the_run(desk, world): + """libfranka throws mid-recovery on an arm the tick also reads in error. One recovery serves the + console and the fault, so the throw reaches the caller instead of being raised a second time with + nobody to hear it.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) @@ -1139,22 +1143,85 @@ def test_a_console_recover_call_is_answered_when_the_vendor_raises(desk, world): for _ in range(3): # init + the opening move, both of which recover on their own next(loop) + arm.error = 1 arm.recover_raises = RuntimeError('libfranka: control command rejected') answer = recover(None) + before = arm.calls.count(Call.RECOVER_FROM_ERRORS) next(loop) + assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before + 1, 'the tick ran the recovery twice' assert answer.done() with pytest.raises(RuntimeError, match='control command rejected'): answer.result() - arm.recover_raises = None + arm.recover_raises, arm.error = None, 0 answer = recover(None) next(loop) + assert answer.result() is franka.RecoveryOutcome.CLEARED, 'the run ended on the failed recovery' + + +def test_a_recovery_that_clears_the_fault_leaves_the_tick_no_second_one(desk, world): + """The console's recovery clears the fault this tick read, so nothing is left for the automatic retry: + it would run on a reading one call out of date.""" + arm = FakeArm(PARK) + arm.recover_clears = True + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + recover = _recoverer(world, driver) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(3): # init + the opening move + next(loop) + arm.error = 1 + answer = recover(None) + before = arm.calls.count(Call.RECOVER_FROM_ERRORS) + next(loop) + + assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before + 1, 'the tick ran the recovery twice' assert answer.result() is franka.RecoveryOutcome.CLEARED + assert arm.error == 0 + + +def test_an_arm_in_error_recovers_with_no_console_asking(desk, world): + """A fault the driver reads is one it clears itself, whether or not a console asked.""" + arm = FakeArm(PARK) + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + _recoverer(world, driver) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(3): # init + the opening move + next(loop) + arm.error = 1 + before = arm.calls.count(Call.RECOVER_FROM_ERRORS) + next(loop) + + assert arm.calls.count(Call.RECOVER_FROM_ERRORS) == before + 1 + + +def test_a_recovery_no_console_asked_for_lets_the_vendor_throw_end_the_run(desk, world): + """A throw from the recovery the driver runs for itself has no caller to hand it to, so it ends the + run rather than going unreported.""" + arm = FakeArm(PARK) + driver = _driver(arm) + driver.state._bind(RecordingEmitter()) + _recoverer(world, driver) + clock = MockClock() + loop = driver.run(StopFlag(), clock) + + for _ in range(3): # init + the opening move + next(loop) + arm.error = 1 + arm.recover_raises = RuntimeError('libfranka: control command rejected') + + with pytest.raises(RuntimeError, match='control command rejected'): + next(loop) -def test_an_arm_nobody_called_runs_no_recovery(desk, world): - """The recovery is the answer to a call, so a loop nobody called never runs one.""" +def test_an_arm_with_no_fault_nobody_called_runs_no_recovery(desk, world): + """An arm carrying no fault gives the driver nothing to recover from, so only a call runs a recovery.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) From be7fb07fd98e550e90b49a001ccaadcd481f91ff Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 10:43:40 +0000 Subject: [PATCH 07/10] Cut two clefts and the rationale the pull request already carries The writing check on the last push. Two comments used the `X is what Y` shape `no_meta_framing.md` bans, and the helper's docstring argued the design instead of stating the contract a caller acts on. Ticket: Positronic-Robotics/internal#1255 #refs --- positronic/drivers/roboarm/franka.py | 5 ++--- positronic/drivers/roboarm/tests/test_franka.py | 2 +- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index 352a18ac2..62cc59c8a 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -471,8 +471,7 @@ class RecoveryOutcome(Enum): def _recover(robot: pf.Robot, asked: list[pimm.calls.Call[None, RecoveryOutcome]]) -> None: """Run the arm's error recovery once, and answer every console that asked for it on this tick. - A console that asked hears what the recovery returned, the vendor's throw included. A recovery nobody - asked for has no one to hear it, so its throw ends the run. + A throw reaches the consoles that asked; one nobody asked for reaches no caller, so it ends the run. """ try: cleared = robot.recover_from_errors() @@ -668,7 +667,7 @@ def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[p asked_to_recover = list(self.recover.incoming()) if asked_to_recover or in_error: _recover(robot, asked_to_recover) - # What the recovery left behind is what the next tick reads, so this one commands nothing. + # This tick commands nothing; the next one reads the arm the recovery left behind. yield arm.limiter.wait() continue diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index b4810dc55..ed2317109 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -1184,7 +1184,7 @@ def test_a_recovery_that_clears_the_fault_leaves_the_tick_no_second_one(desk, wo def test_an_arm_in_error_recovers_with_no_console_asking(desk, world): - """A fault the driver reads is one it clears itself, whether or not a console asked.""" + """The driver clears a fault it reads itself, whether or not a console asked.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) From a0b9b2885ceb95117a382a33637254277eef0432 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 10:51:19 +0000 Subject: [PATCH 08/10] Move the recovery helper into its one user, and waive the transfer Two P1 findings from the review of b7e3c06d, both on `_recover`. `stranded-definition`: the helper had one user, `Robot.run`, and sat at module scope above the whole class. It is a `@staticmethod` on `Robot` directly above `run` now. It touches no instance state, which the rule names as the case `@staticmethod` answers rather than a reason to stay at module level. `swallowed-error`: the `except` catches every vendor throw and returns, so the rule asks for a waiver. It carries one, in the shape `pimm.calls.raise_to` uses for the same contract. The inline comment on `set_exception` goes: the waiver says what it said. Ticket: Positronic-Robotics/internal#1255 #refs --- positronic/drivers/roboarm/franka.py | 45 ++++++++++++++-------------- 1 file changed, 23 insertions(+), 22 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index 62cc59c8a..dab72ab65 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -468,27 +468,6 @@ class RecoveryOutcome(Enum): NOT_CLEARED = auto() -def _recover(robot: pf.Robot, asked: list[pimm.calls.Call[None, RecoveryOutcome]]) -> None: - """Run the arm's error recovery once, and answer every console that asked for it on this tick. - - A throw reaches the consoles that asked; one nobody asked for reaches no caller, so it ends the run. - """ - try: - cleared = robot.recover_from_errors() - except Exception as exc: - if not asked: - raise - logger.exception('The recovery a console asked for failed') - for call in asked: - call.set_exception(exc) # the consoles that asked hold the failure, so the run carries on - return - if asked: - logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') - outcome = RecoveryOutcome.CLEARED if cleared else RecoveryOutcome.NOT_CLEARED - for call in asked: - call.set_result(outcome) - - class Robot(pimm.ControlSystem): def __init__( self, @@ -641,6 +620,28 @@ def _arm(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock, safe_inputs: safe_inputs, ) + @staticmethod + def _recover(robot: pf.Robot, asked: list[pimm.calls.Call[None, RecoveryOutcome]]) -> None: + """Run the arm's error recovery once, and answer every console that asked for it on this tick. + + A throw reaches the consoles that asked; one nobody asked for reaches no caller, so it ends the run. + """ + try: + cleared = robot.recover_from_errors() + # rules-allow: swallowed-error — the throw is not dropped but handed to every console that asked + except Exception as exc: + if not asked: + raise + logger.exception('The recovery a console asked for failed') + for call in asked: + call.set_exception(exc) + return + if asked: + logger.info(f'A console asked to clear a fault; recover_from_errors returned {cleared}') + outcome = RecoveryOutcome.CLEARED if cleared else RecoveryOutcome.NOT_CLEARED + for call in asked: + call.set_result(outcome) + def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[pimm.Command]: safe_inputs = _SafeInputs(self._ip, self._desk_credentials) with self._desk_session() as desk, safe_inputs, self._arm(should_stop, clock, safe_inputs) as arm: @@ -666,7 +667,7 @@ def run(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock) -> Iterator[p asked_to_recover = list(self.recover.incoming()) if asked_to_recover or in_error: - _recover(robot, asked_to_recover) + self._recover(robot, asked_to_recover) # This tick commands nothing; the next one reads the arm the recovery left behind. yield arm.limiter.wait() continue From 65afd661325ebc6d5e9a2bd039c2742015338155 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 20:51:41 +0000 Subject: [PATCH 09/10] Cut the antithesis and the emphasis from four comments Two open review threads and two findings from the writing check on the last push. All four are prose. `_recoverer` defined itself by what `_mover` does, so its documentation goes stale when `_mover` changes. It states what it is now, in the form `_mover` uses. A test docstring wrote `THAT` in capitals for emphasis. The word is lower case now. The `swallowed-error` waiver and one test docstring used the `not X but Y` antithesis the writing rules ban. Each one states what happens. `pimm.calls.raise_to` carries the same waiver in the same antithesis shape. This commit leaves that line as it is. It is outside the lines this pull request adds. Ticket: Positronic-Robotics/internal#1255 #refs --- positronic/drivers/roboarm/franka.py | 2 +- positronic/drivers/roboarm/tests/test_franka.py | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index dab72ab65..26a05021b 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -628,7 +628,7 @@ def _recover(robot: pf.Robot, asked: list[pimm.calls.Call[None, RecoveryOutcome] """ try: cleared = robot.recover_from_errors() - # rules-allow: swallowed-error — the throw is not dropped but handed to every console that asked + # rules-allow: swallowed-error — the throw is handed to every console that asked except Exception as exc: if not asked: raise diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index ed2317109..6286bea35 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -225,7 +225,7 @@ def _mover(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[command def _recoverer(world: pimm.World, driver: franka.Robot) -> pimm.calls.Caller[None, franka.RecoveryOutcome]: - """A caller on ``driver.recover``, the same way ``_mover`` calls a move.""" + """A caller on ``driver.recover``, for a test that pumps its generator rather than running a World.""" caller = pimm.calls.ControlSystemCaller[None, franka.RecoveryOutcome](driver) wire_call(world, caller, driver.recover) return caller @@ -1096,7 +1096,7 @@ def test_a_command_pinning_no_mode_returns_the_arm_to_its_native_law(desk): def test_a_console_recover_call_is_answered_that_the_fault_cleared(desk, world): """A console calls the arm to clear a latched fault: the driver runs the recovery and the answer to - THAT call carries what it returned.""" + that call carries what it returned.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) @@ -1132,8 +1132,7 @@ def test_a_console_recover_call_is_answered_that_the_fault_did_not_clear(desk, w def test_a_recovery_the_vendor_fails_answers_the_console_rather_than_ending_the_run(desk, world): """libfranka throws mid-recovery on an arm the tick also reads in error. One recovery serves the - console and the fault, so the throw reaches the caller instead of being raised a second time with - nobody to hear it.""" + console and the fault, so the throw reaches the caller and the run goes on.""" arm = FakeArm(PARK) driver = _driver(arm) driver.state._bind(RecordingEmitter()) From dd2b59ff9dcc3e12c40f44e3f792a6654200e3e1 Mon Sep 17 00:00:00 2001 From: Vladimir Yakunin Date: Mon, 14 Sep 2026 21:01:30 +0000 Subject: [PATCH 10/10] Name the recovery's caller by what it is, and drop a false setup clause Two P1 findings from the review of 65afd661. Both are prose. `_recover` called every caller a console. `recover` is a public handler, and any control system can wire to it. The docstring and the waiver say `caller` now. The two log lines keep the word `console`: they are strings the driver writes, not comments, and a grep can read them. The setup comment in `test_a_recovery_the_vendor_fails_...` claimed that the init and the opening move both recover on their own. The init does not recover. It sets the collision behaviour, the control mode and the load. `_Arm.park` holds the one `recover_from_errors` those three steps reach, so the arm records one recovery, not two. The comment reads `init + the opening move`, which is what the other 15 sites of this comment read. Ticket: Positronic-Robotics/internal#1255 #refs --- positronic/drivers/roboarm/franka.py | 6 +++--- positronic/drivers/roboarm/tests/test_franka.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/positronic/drivers/roboarm/franka.py b/positronic/drivers/roboarm/franka.py index 26a05021b..8ce2d665e 100644 --- a/positronic/drivers/roboarm/franka.py +++ b/positronic/drivers/roboarm/franka.py @@ -622,13 +622,13 @@ def _arm(self, should_stop: pimm.SignalReceiver, clock: pimm.Clock, safe_inputs: @staticmethod def _recover(robot: pf.Robot, asked: list[pimm.calls.Call[None, RecoveryOutcome]]) -> None: - """Run the arm's error recovery once, and answer every console that asked for it on this tick. + """Run the arm's error recovery once, and answer every caller that asked for it on this tick. - A throw reaches the consoles that asked; one nobody asked for reaches no caller, so it ends the run. + A throw reaches the callers that asked; one nobody asked for reaches no caller, so it ends the run. """ try: cleared = robot.recover_from_errors() - # rules-allow: swallowed-error — the throw is handed to every console that asked + # rules-allow: swallowed-error — the throw is handed to every caller that asked except Exception as exc: if not asked: raise diff --git a/positronic/drivers/roboarm/tests/test_franka.py b/positronic/drivers/roboarm/tests/test_franka.py index 6286bea35..a23961399 100644 --- a/positronic/drivers/roboarm/tests/test_franka.py +++ b/positronic/drivers/roboarm/tests/test_franka.py @@ -1140,7 +1140,7 @@ def test_a_recovery_the_vendor_fails_answers_the_console_rather_than_ending_the_ clock = MockClock() loop = driver.run(StopFlag(), clock) - for _ in range(3): # init + the opening move, both of which recover on their own + for _ in range(3): # init + the opening move next(loop) arm.error = 1 arm.recover_raises = RuntimeError('libfranka: control command rejected')