Answer a waiting caller when the Franka arm stays in error - #743
Answer a waiting caller when the Franka arm stays in error#743v-positronic wants to merge 1 commit into
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2de067c565
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
|
||
| yield arm.limiter.wait() | ||
|
|
||
| arm.abandon_deferred() # the world stopped with a move still held |
There was a problem hiding this comment.
If robot.state(), recover_from_errors(), or another operation raises after _deferred has been dequeued, control skips this statement and _Arm.__exit__ only stops the robot. The handler shutdown logic can fail only calls it never reached (pimm/calls.py:12-16), so this reached call remains unanswered despite the call contract; move this cleanup into a finally path or _Arm.__exit__.
Useful? React with 👍 / 👎.
| if entered_error: | ||
| logger.warning(f'Robot error: {st.error_message}') | ||
| arm.note_error_entered(st) |
There was a problem hiding this comment.
Rule hidden-dependency violated:
recover_and_serve reads _error_started and _q_at_error, but their correctness depends on every caller invoking note_error_entered first. Fold this capture into recover_and_serve when entered is true, or pass a complete recovery-state value, so the ordering is enforced rather than remembered by the caller.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| def recover_and_serve( | ||
| self, st: pf.State, in_error: bool, entered: bool, brakes: '_Brakes' | ||
| ) -> Generator[pimm.Command, None, bool]: |
There was a problem hiding this comment.
Rule misleading-name violated:
recover_and_serve performs recovery and may serve a move, yet returns a bare bool whose verdict is only explained as “whether it handled the tick.” Return a named enum outcome so the conditional states what the result means while retaining the action name.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| """The wedge: while the arm errors the driver loops on recovery and never serves a waiting move, so a | ||
| prepare hangs with no bound. The driver now answers the caller once recovery has not cleared in time.""" |
There was a problem hiding this comment.
Rule diff-comments violated:
This test docstring narrates the former wedge and says what “the driver now” does, so it is written for a reader of the change rather than the file. Replace it with the current invariant: a move waiting through a persistent arm fault is refused after the recovery grace.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
The driver looped on recover_from_errors while the arm held an error and never reached moves.next_request, so a prepare that waited on the arm hung with no bound. The driver now holds one waiting sync move while it recovers: it serves the move once the error clears, and answers it with MoveRefused(reasons, moved_rad) once recovery has not cleared the error in _RECOVERY_GRACE_S seconds. It drains a jog issued while the arm errors, and logs the recovery result and whether the error cleared or persists. Ticket: none — positronic half of the wedge diagnosis; PR body links internal#1285.
2de067c to
418738d
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 418738d3d3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| """Answer a held move with ``MoveRefused`` once recovery has run past the grace without clearing.""" | ||
| if self._deferred is None or self.clock.now() - self._error_started < _RECOVERY_GRACE_S: | ||
| return | ||
| moved = float(np.max(np.abs(st.q - self._q_at_error))) |
There was a problem hiding this comment.
Rule misleading-name violated:
moved_rad is documented as the largest joint motion during recovery, but _refuse_if_stuck computes only the displacement between error entry and refusal. If a joint moves away and returns, this reports zero and can mislead callers into treating the arm as stationary; track the maximum across recovery samples, or rename the field and contract to describe endpoint displacement.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| _PARK_JOINTS = np.array([0.0, -0.31, 0.0, -1.65, 0.0, 1.522, 0.0]) | ||
|
|
||
| # Recovery this long into an unclearing error answers a waiting caller instead of holding it. | ||
| _RECOVERY_GRACE_S = 2.0 |
There was a problem hiding this comment.
Rule stranded-definition violated:
_RECOVERY_GRACE_S is used only by _Arm in this file but is left at module scope far above that class. Make it an _Arm class constant beside _MOVE_GRACE_S and update its references so the definition stays with the entity that owns it.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
| ``reasons`` is the arm's own error message. ``moved_rad`` is the largest joint motion the arm made | ||
| while the driver tried to recover; a caller reads it to judge whether the path is blocked. |
There was a problem hiding this comment.
Rule diff-comments violated:
MoveRefused's docstring says that a caller reads moved_rad to decide whether the path is blocked, but the exception does not depend on that consumer policy and no such caller exists here. Keep only the stable field meaning, such as the joint displacement observed during recovery.
AGENTS.md reference: AGENTS.md:L7-L8
Useful? React with 👍 / 👎.
1 participant
The defect
The Franka run loop looped on
recover_from_errors()while the arm held an error and reachedmoves.next_request()only after the error cleared (franka.py, thein_errorbranch). A sync move that arrived while the arm errored sat in the handler queue, unanswered, for as long as the error stood. On 2026-09-11 acartesian_reflexstood fourteen minutes under continuous recovery, so a prepare hung with no bound and the console wedged. Diagnosis: Positronic-Robotics/internal#1285.What this ships
The driver now holds one waiting sync move while it recovers, on
_Arm:MoveRefused(reasons, moved_rad)once recovery has not cleared the error in_RECOVERY_GRACE_S(2 s).reasonsis the arm's own error message;moved_radis the largest joint motion during recovery.MoveAbandoned.MoveRefusedis a new exception indrivers/utils.py. It is the retriable signal the harness retry (P1,internal#1285) will catch; this PR only introduces it and raises it.Scope
run(); positronic#678 landed first and this branch is rebased onto it (below).Tests
Driver-level, no rig, through the
FakeArmand aMockClock: a move waiting on an erroring arm is refused after the grace and not before; it is served once recovery clears; a jog issued while a move is held is dropped; a held move is answered when the world stops. The refusal test fails without the fix, because the call is never answered.uv run --locked pytest positronic/drivers/roboarm/tests/test_franka.pyis green (36 passed).pre-commit(ruff, ruff format, basedpyright) passed on commit.The thresholds in the plan (2 s, and the 0.02 rad path-blocked bound in P1) have no data behind them and are used as given.
The
check-rulesfan-out is skipped this pass to keep the shared box free while other agents work; the rules check is due before merge.Rebased onto positronic#678 2026-09-12
positronic#678 merged as
8ea1eb5dand took therun()lines this branch warned it would. Rebased onto that sha; head2de067c5->418738d3.Two conflicts, both the same shape — two independent insertions at one point — so both sides are kept: positronic#678's
SAFE_INPUT_STATE/_Reading/_SafeInputsbeside this branch's_RECOVERY_GRACE_S, and positronic#678'ssafe_inputs/_refusals/_refused/_quiet_atbeside its_deferred/_error_started/_q_at_error.The composition was checked rather than assumed.
note_refusals(goal)runs at the top of every tick, BEFORE the error branch, so thecontinueinsiderecover_and_servedoes not skip positronic#678's refusal logging;close_if_idle(goal)keeps its new signature. The diff against main deletes exactly five lines, all of them ones this commit deliberately replaces, and every positronic#678 marker is still in the file.test_franka.py52 passed on the rebased head (36 this branch's, the rest positronic#678's, now running together). ruff, ruff format and basedpyright clean.One thing the rebase deliberately did not settle: positronic#678's
note_refusalsnames a move libfranka rejects on a triggered safe input, and this PR'sMoveRefusedanswers a caller when an error outlasts the grace. They are distinct mechanisms that now sit in one file. Whether a safe-input refusal should also answer a waiting caller is a design question, not a merge conflict.The
check-rulesfan-out is still owed before merge.