fix: firmware reports failed moves (CMD_EXECUTION_ERROR); filter wheel uses absolute MOVETO - #540
Merged
Merged
Conversation
Two coordinated changes that together let the host detect and recover
from silent filter-wheel move failures:
1. Root-cause fix for the silent CMD_EXECUTION_ERROR class
- New mcu_cmd_execution_status global, reset to COMPLETED_WITHOUT_ERRORS
at the start of every received command and overwritten with
CMD_EXECUTION_ERROR by a mark_move_failed() helper on the failure
path. send_position_update reports it when the MCU is idle.
- All move callbacks (MOVE_X/Y/Z/W/W2, MOVETO_X/Y/Z/W) restructured to
set in_progress = true BEFORE tmc4361A_moveTo and to call
mark_move_failed() on the failure branch. The move_filterwheel and
callback_move_to_w early-returns for enable_filterwheel == false
now mark failed instead of silently no-op'ing.
- Without this fix the firmware reported COMPLETED_WITHOUT_ERRORS for
filter-wheel moves that never executed (e.g. move arrived before
INITFILTERWHEEL), leaving the host's tracked position off by one
until the user manually re-homed.
2. Enable host-side absolute addressing for filter wheels
- finalize_homing_w / finalize_homing_w2 now call
tmc4361A_setCurrentPosition(&tmc4361[w/w2], 0), anchoring the
driver coordinate to 0 at the home reference so the host can
target absolute slot positions as slot_index * usteps_per_slot.
- New MOVETO_W2 protocol command (= 43) + callback_move_to_w2 mirror
of callback_move_to_w, registered in cmd_map. This completes
symmetric absolute-move coverage for dual-wheel setups.
FIRMWARE_VERSION_MINOR bumped 1 -> 2 to reflect the new MOVETO_W2 command
and CMD_EXECUTION_ERROR semantics on the wire.
…ON_ERROR
Two coordinated changes that take advantage of the firmware support
added in the previous commit:
1. CMD_EXECUTION_ERROR handling
- When the firmware reports CMD_EXECUTION_ERROR for the in-flight
command, the host now calls abort_current_command(recoverable=True)
immediately instead of waiting the full 5 s ack timeout. This is
load-bearing for the cheap-resend path described below.
- New `recoverable` kwarg on abort_current_command logs the abort at
WARNING (caller will retry) rather than ERROR.
- SimSerial bumped to firmware version (1, 2) to match. Existing
tests that asserted (1, 1) updated to track the simulated version.
2. Filter wheel uses absolute MOVETO_W / MOVETO_W2
- SquidFilterWheel._move_to_position computes an absolute microstep
target as (target_pos - min_index) * usteps_per_slot + offset_usteps,
anchored to the firmware coordinate frame (which is now zeroed at
the home reference by the firmware change). Issues MOVETO_W /
MOVETO_W2 instead of relative MOVE_W / MOVE_W2.
- Recovery flow splits on failure type:
* CommandAborted (firmware-confirmed no motion): cheap software
resend without re-home (saves ~4 s).
* TimeoutError (motor state uncertain): re-home + retry against
the same absolute target.
- _home_wheel's offset move also switched to absolute MOVETO for
symmetry; every motion in the controller now goes through one path.
- New move_w_to_usteps / move_w2_to_usteps helpers in microcontroller.py
and MOVETO_W2 wired through firmware_sim_serial.py.
The key safety property absolute moves give us: if any silent failure
ever slips past the firmware fix, the divergence is contained to one
move instead of compounding into a persistent off-by-N desync. The next
successful MOVETO drives the wheel to the correct absolute slot
regardless of any stale host-side belief.
7 tasks
Post-review cleanup based on three reviewer agents: Reuse / quality: - Added Microcontroller._move_axis_to_usteps helper to unify the five near-identical move_*_to_usteps methods (X/Y/Z/W/W2). Same shape as the existing _move_axis_usteps for relative moves. - Unified the firmware filter-wheel callbacks: relative MOVE_W/W2 and absolute MOVETO_W/W2 now share a single dispatch_filterwheel_move helper. Each callback decodes its own target (current+relative vs absolute) and passes it in. - Added decode_payload_int32() helper to remove the byte-shift incantation repeated across stage_commands.cpp. - Replaced `motor_slot == 3 / == 4` magic-number checks in cephla.py with a _MOTOR_SLOT_MCU_METHODS dispatch table + _mcu_method helper. Correctness: - FirmwareSimSerial now models w2 as a distinct state. Previously MOVE_W2/MOVETO_W2 aliased onto self.w, which would give wrong results for any test driving both wheels. Comment hygiene: - Trimmed several verbose comments that narrated WHAT the code does or referenced the original incident. Kept WHY comments that flag cross-file invariants (e.g. the firmware-anchor assumption in _target_pos_to_usteps). Verified: 162 passed, 1 skipped, 1 xfailed across affected suites; black --check clean.
hongquanli
changed the title
…W2 + caplog tests Addresses review feedback on PR #540: 1. Hard firmware-version gate (red issue): SquidFilterWheel._configure_wheel now raises RuntimeError if microcontroller.firmware_version < (1, 2). The host sends MOVETO_W against a post-home X_ACTUAL=0 frame that older firmware does not establish; without the gate, host upgrades against pre-v1.2 firmware would silently send absolute targets that land at the wrong slot — re-introducing the same off-by-one bug this PR fixes. The check runs in _configure_wheel only (skip_init=False path). Restart flows with skip_init=True bypass the check on the assumption the firmware was validated on first init. 2. Parametrized W/W2 recovery tests (yellow): test_command_aborted_triggers_software_resend_not_rehome and test_timeout_skips_resend_and_goes_straight_to_rehome now run for both motor_slot=3 (W) and motor_slot=4 (W2), via pytest.parametrize. 3. caplog test for recoverable kwarg (yellow): test_abort_current_command_recoverable_logs_at_warning asserts abort_current_command(recoverable=True) emits WARNING and the default emits ERROR. 4. Firmware version gate tests: TestSquidFilterWheelFirmwareVersionGate covers (0,0)/(1,0)/(1,1) reject cases + (1,2)/(2,0) accept cases + the skip_init=True bypass. Test fixtures updated to set mock.firmware_version = (1, 2) so existing skip_init tests still construct the controller successfully.
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes a class of silent filter-wheel position desyncs by (1) closing a firmware root-cause where failed tmc4361A_moveTo calls (and pre-INITFILTERWHEEL early-returns) were silently acked as COMPLETED_WITHOUT_ERRORS, and (2) switching the host's W/W2 filter wheel control to absolute MOVETO_W / MOVETO_W2 against a home-anchored coordinate frame so any single missed move self-corrects on the next successful command. Adds a new mcu_cmd_execution_status byte and mark_move_failed() helper in firmware, a CMD_EXECUTION_ERROR fail-fast path + recoverable abort log level in the host, and a firmware-version gate on SquidFilterWheel. Firmware bumped to v1.2.
Changes:
- Firmware:
mark_move_failed()+mcu_cmd_execution_statuspropagation; move callbacks setin_progress = truebeforetmc4361A_moveTo;finalize_homing_w/_w2anchor X_ACTUAL to 0; newMOVETO_W2command (43) andcallback_move_to_w2. - Host:
SquidFilterWheel._move_to_positionuses absolute MOVETO;CommandAbortedtriggers cheap resend,TimeoutErrortriggers re-home+retry;abort_current_command(recoverable=True)logs at WARNING; firmware-version gate (>= v1.2) at configure-time. - Sim/tests:
firmware_sim_serial.pyhandlesMOVE_W2/MOVETO_W2; broad test additions for absolute-move math, recovery paths, version gate, and recoverable-abort log level; v1.1→v1.2 updates in version-detection tests.
Reviewed changes
Copilot reviewed 19 out of 19 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| firmware/controller/src/constants.h | Bump FIRMWARE_VERSION_MINOR to 2 with version log comment. |
| firmware/controller/src/constants_protocol.h | Add MOVETO_W2 = 43. |
| firmware/controller/src/globals.h / globals.cpp | Add mcu_cmd_execution_status global. |
| firmware/controller/src/serial_communication.cpp | Reset status on each rx; status byte in idle path is now the new global. |
| firmware/controller/src/commands/stage_commands.h / .cpp | mark_move_failed() + dispatch_filterwheel_move(); all X/Y/Z/W/W2 callbacks restructured; new callback_move_to_w2. |
| firmware/controller/src/commands/commands.cpp | Register MOVETO_W2 in cmd_map. |
| firmware/controller/src/operations.cpp | finalize_homing_w/_w2 set driver position to 0. |
| software/control/_def.py | Add CMD_SET.MOVETO_W2 = 43. |
| software/control/microcontroller.py | _move_axis_to_usteps helper; move_w_to_usteps / move_w2_to_usteps; MOVETO_W2 name; abort_current_command(recoverable); CMD_EXECUTION_ERROR fail-fast branch; sim firmware version 1.2. |
| software/control/firmware_sim_serial.py | Simulate MOVE_W2 / MOVETO_W2 and W2 axis zeroing. |
| software/squid/filter_wheel_controller/cephla.py | Absolute MOVETO path, slot→method dispatch table, firmware-version gate, split CommandAborted/Timeout recovery. |
| software/tests/squid/test_filter_wheel.py | New tests for absolute moves, recovery paths, firmware version gate. |
| software/tests/control/test_microcontroller.py | New test for recoverable vs default abort log levels. |
| software/tests/test_watchdog.py + multiport tests | Update expected sim firmware version (1.1 → 1.2). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+186
to
+208
| except CommandAborted as e: | ||
| _log.warning(f"Filter wheel {wheel_id} command aborted ({e}); resending in software...") | ||
| try: | ||
| self._move_wheel(wheel_id, delta) | ||
| self._move_to_usteps(wheel_id, target_usteps) | ||
| self.microcontroller.wait_till_operation_is_completed() | ||
| self._positions[wheel_id] = target_pos | ||
| _log.info(f"Filter wheel {wheel_id} recovery successful, now at position {target_pos}") | ||
| except TimeoutError: | ||
| _log.error( | ||
| f"Filter wheel {wheel_id} movement failed even after re-home. " f"Hardware may need attention." | ||
| ) | ||
| raise | ||
| _log.info(f"Filter wheel {wheel_id} software resend succeeded, now at position {target_pos}") | ||
| return | ||
| except self._RECOVERABLE_MOVE_ERRORS as e2: | ||
| _log.warning(f"Filter wheel {wheel_id} resend also failed ({e2}); re-homing to re-sync...") | ||
| except TimeoutError as e: | ||
| _log.warning(f"Filter wheel {wheel_id} move uncertain ({e}); re-homing to re-sync...") | ||
|
|
||
| self._home_wheel(wheel_id) | ||
| target_usteps = self._target_pos_to_usteps(config, target_pos) | ||
| try: | ||
| self._move_to_usteps(wheel_id, target_usteps) | ||
| self.microcontroller.wait_till_operation_is_completed() | ||
| self._positions[wheel_id] = target_pos | ||
| _log.info(f"Filter wheel {wheel_id} recovery via re-home succeeded, now at position {target_pos}") | ||
| except self._RECOVERABLE_MOVE_ERRORS: | ||
| _log.error(f"Filter wheel {wheel_id} movement failed even after re-home. Hardware may need attention.") | ||
| raise |
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Fixed in commit 8fc1e78 — call acknowledge_aborted_command() immediately after catching CommandAborted (and after the inner resend failure when it was also a CommandAborted, gated on isinstance(e2, CommandAborted) to avoid the "ack with nothing to ack" path on TimeoutError).
| _log.warning(f"Filter wheel {wheel_id} move uncertain ({e}); re-homing to re-sync...") | ||
|
|
||
| self._home_wheel(wheel_id) | ||
| target_usteps = self._target_pos_to_usteps(config, target_pos) |
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Fixed in commit 8fc1e78 — dropped the redundant recompute. Agree it was misleading-as-self-documentation.
Comment on lines
211
to
216
| """Home a wheel, then drive to slot 0 absolutely. | ||
|
|
||
| Args: | ||
| wheel_id: The ID of the wheel to home. | ||
| The firmware anchors the driver's X_ACTUAL counter to 0 at the | ||
| home reference, so the host can target absolute slot positions | ||
| as `slot_index * usteps_per_slot + offset_usteps` thereafter. | ||
| """ |
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Fixed in commit 8fc1e78 — docstring now reads "drive to its first slot (config.min_index) absolutely".
Comment on lines
+90
to
+100
| def _configure_wheel(self, wheel_id: int, config: SquidFilterWheelConfig): | ||
| """Configure a single filter wheel motor.""" | ||
| fw = self.microcontroller.firmware_version | ||
| if fw < self._MIN_FIRMWARE_VERSION: | ||
| min_major, min_minor = self._MIN_FIRMWARE_VERSION | ||
| raise RuntimeError( | ||
| f"SquidFilterWheel requires firmware >= v{min_major}.{min_minor} " | ||
| f"(got v{fw[0]}.{fw[1]}). Older firmware does not anchor the W " | ||
| f"axis to 0 after homing, so absolute MOVETO_W targets would " | ||
| f"land at the wrong slot. Re-flash firmware from firmware/controller." | ||
| ) |
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Already fixed in commit c140113 (pushed before your review landed). The version check now runs unconditionally in __init__, including on the skip_init=True restart path — exactly your suggestion. Test test_skip_init_also_checks_version covers this.
Comment on lines
+29
to
+32
| // Default the status to success; callbacks call mark_move_failed() | ||
| // on the failure path to override it before the next status broadcast. | ||
| mcu_cmd_execution_status = COMPLETED_WITHOUT_ERRORS; | ||
|
|
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Partially fixed in commit 8fc1e78 — added a guard if (buffer_rx[1] != HEARTBEAT) around the mcu_cmd_execution_status reset, so heartbeat traffic no longer clobbers a pending CMD_EXECUTION_ERROR. This addresses the dominant race (heartbeat is the only background-sender today). The general case (two failed commands back-to-back before a broadcast, or future background senders) still has the same lossy-state-broadcast issue; the protocol-level fix (tag the status with its cmd_id + reset-after-observe) is bigger than this PR. Will scope as a follow-up.
Comment on lines
+3
to
+8
| // Surface a failed move so the next position-update reports CMD_EXECUTION_ERROR. | ||
| static inline void mark_move_failed() | ||
| { | ||
| mcu_cmd_execution_status = CMD_EXECUTION_ERROR; | ||
| mcu_cmd_execution_in_progress = false; | ||
| } |
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Fixed in commit 8fc1e78 — split into mark_move_failed() (for paths that have already claimed mcu_cmd_execution_in_progress = true, unwinds the flag) and report_move_error() (for early-return paths that haven't, leaves the flag alone). The !enabled branch in dispatch_filterwheel_move now uses report_move_error(). Invariant: only the function that set in_progress is allowed to clear it.
Comment on lines
+217
to
+257
| @pytest.mark.parametrize("motor_slot,move_to_attr,move_rel_attr,home_attr", AXIS_PARAMS) | ||
| def test_command_aborted_triggers_software_resend_not_rehome( | ||
| self, motor_slot, move_to_attr, move_rel_attr, home_attr | ||
| ): | ||
| """CMD_EXECUTION_ERROR → resend the same MOVETO; do NOT re-home.""" | ||
| from control.microcontroller import CommandAborted | ||
|
|
||
| wheel_inst, mc, _ = self._build_wheel(motor_slot) | ||
|
|
||
| # First wait raises CommandAborted, second succeeds. | ||
| mc.wait_till_operation_is_completed.side_effect = [ | ||
| CommandAborted(reason="firmware reported CMD_EXECUTION_ERROR", command_id=1), | ||
| None, | ||
| ] | ||
|
|
||
| wheel_inst._move_to_position(1, 4) | ||
|
|
||
| assert getattr(mc, move_to_attr).call_count == 2 | ||
| getattr(mc, home_attr).assert_not_called() | ||
| assert wheel_inst._positions[1] == 4 | ||
|
|
||
| @pytest.mark.parametrize("motor_slot,move_to_attr,move_rel_attr,home_attr", AXIS_PARAMS) | ||
| def test_timeout_skips_resend_and_goes_straight_to_rehome(self, motor_slot, move_to_attr, move_rel_attr, home_attr): | ||
| """Ack timeout → re-home + retry (no cheap resend, motor state is uncertain).""" | ||
| wheel_inst, mc, _ = self._build_wheel(motor_slot) | ||
|
|
||
| # First move times out; home succeeds; retry succeeds. | ||
| mc.wait_till_operation_is_completed.side_effect = [ | ||
| TimeoutError("ack timeout"), | ||
| None, # home wait | ||
| None, # home offset move wait | ||
| None, # retry MOVETO wait | ||
| ] | ||
|
|
||
| wheel_inst._move_to_position(1, 4) | ||
|
|
||
| getattr(mc, home_attr).assert_called_once() | ||
| # Three MOVETO calls: the failed initial attempt, the home-offset | ||
| # move inside _home_wheel, and the post-home retry to slot 4. | ||
| assert getattr(mc, move_to_attr).call_count == 3 | ||
| assert wheel_inst._positions[1] == 4 |
Contributor
Author
There was a problem hiding this comment.
[Claude Code] Fixed in commit 8fc1e78 — added getattr(mc, move_rel_attr).assert_not_called() to both parametrized recovery tests. Enforces that the absolute-MOVETO path never falls back to relative MOVE.
Addresses self-review feedback on the v1.2 firmware gate: - Move the version check from _configure_wheel to __init__. It now runs unconditionally, including on the skip_init=True restart path — firmware could have been re-flashed (or downgraded) between launches, so the gate must not be bypassed. Also eliminates the per-wheel duplication (previously ran once per configured wheel). - Flip the corresponding test: skip_init=True with pre-v1.2 firmware now raises RuntimeError. Renamed test_skip_init_still_checks_version → test_skip_init_also_checks_version to match the behavior. - Drop the "intentional gap" rationale from the PR body's firmware-gate subsection. Minor: hoist the in-function `import logging` to the top of test_microcontroller.py.
…tbeat-skip, etc. Addresses six issues from the copilot-pull-request-reviewer bot review: #1 (cephla.py): Call acknowledge_aborted_command() after catching CommandAborted (and after the inner resend failure if it was also a CommandAborted), so the next send_command doesn't log the spurious "Last command aborted and not cleared before new command sent!" warning. The inner ack is gated on isinstance(e2, CommandAborted) to avoid the "ack with nothing to ack" path on TimeoutError. #2 (cephla.py): Drop the redundant `target_usteps = ...` recompute after _home_wheel. config and target_pos haven't changed and _target_pos_to_usteps doesn't depend on current_pos. #3 (cephla.py): Fix _home_wheel docstring — wheel is driven to config.min_index (typically slot 1), not "slot 0". #5 (firmware/serial_communication.cpp): Skip the `mcu_cmd_execution_status = COMPLETED_WITHOUT_ERRORS` reset when processing a HEARTBEAT. The keepalive has no result to report, and resetting would clobber a pending CMD_EXECUTION_ERROR from the previous command if the broadcast hasn't fired yet. Eliminates the narrow race where heartbeat traffic interleaves a failure broadcast. #6 (firmware/stage_commands.cpp): Split mark_move_failed() into two helpers — mark_move_failed() (for paths that already set mcu_cmd_execution_in_progress = true) and report_move_error() (for early-return paths that didn't). The !enabled branch in dispatch_filterwheel_move now uses report_move_error() so it doesn't spuriously unwind in_progress for an unrelated motion in flight on another axis. Invariant: only the function that claimed in_progress gets to clear it. #7 (test_filter_wheel.py): Add `getattr(mc, move_rel_attr).assert_not_called()` to both parametrized CommandAborted/TimeoutError tests, so the absolute-MOVETO recovery path is enforced — fall-back to relative MOVE would now be caught.
…l version tests Post-review cleanup based on three reviewer agents on commits be5c600/c140113e/8fc1e789: - Hoist `_make_squid_config()` and `_make_mock_mc()` to test_filter_wheel.py module scope. Three test classes were building the same 5-field SquidFilterWheelConfig and three places hard-coded mock.firmware_version = (1, 2). All fixtures and _build_wheel now go through the helpers. - Replace `isinstance(e2, CommandAborted)` in cephla.py's recovery path with a state-based check on `microcontroller.last_command_aborted_error`. The state check is authoritative (TimeoutError doesn't set the field, so it naturally skips ack), and the new placement just before _home_wheel consolidates the inner-and-outer paths into one decision point. - Merge test_init_succeeds_on_v1_2_firmware + test_init_succeeds_on_newer_firmware into a single parametrized test_init_succeeds_on_supported_firmware. Skipped per all three agents: firmware mark_move_failed/report_move_error split (names document the precondition intentionally), HEARTBEAT-skip "list" single-entry (comment is the forcing function for future entries), 4-tuple AXIS_PARAMS (still readable), context-manager for acks (over-engineering), all efficiency findings (appropriate as-is).
Addresses three review findings on the latest PR state: - Test coverage gap: added test_rehome_retry_failure_propagates (W/W2 parametrized) so the final `raise` path in _move_to_position is now exercised. Asserts the TimeoutError propagates and _positions[wheel_id] is NOT updated to the target slot. - Error message specificity: firmware-version-gate message no longer says "W axis" specifically (some users only have W2). Now reads "the filter-wheel driver position" + explicit mention that MOVETO_W2 also doesn't exist on older firmware. - home() failure visibility (partial from PR-body follow-ups): wrapped _home_wheel body in two try/excepts that log clearly which step failed (the home command itself vs. the offset move). On home-failure, position is unknown; on offset-move-failure, position is at the home reference, not slot min_index. Does not add real recovery (still bubbles up) but gives bench operators an actionable error. Out-of-scope follow-ups deferred (see PR body): - _delta_to_usteps → AxisConfig.convert_real_units_to_ustep: requires schema change to SquidFilterWheelConfig (pydantic model with many required fields) and changes rounding semantics (int vs round). - MCU status-byte attribution race: protocol change (status tagged with cmd_id + reset-after-observe).
5 tasks
4 tasks
Alpaca233
added a commit
that referenced
this pull request
May 19, 2026
## Summary Add five INFO/WARNING log lines to `SquidFilterWheel` to make filter-wheel position drift easier to diagnose. Targets cases that aren't already covered by the existing recovery-path warnings introduced in #540. - `__init__`: firmware version + `skip_init` flag - `__init__`: per-wheel `motor_slot`, slot range, offset — config sanity check - `__init__`: WARNING when `skip_init=True` (tracked position assumed, never verified by homing — a known way for tracked/physical to diverge) - `_move_to_position`: `wheel N: current -> target (usteps=X)` on every move. The absolute usteps target is the most direct signal for diagnosing slot-misalignment — if usteps is wrong, the config math is suspect; if usteps is right but the slot is wrong, it's hardware/firmware. - `_home_wheel`: entry (prev tracked position) + exit (elapsed time) No behavior changes. +15 lines, no deletions. ## Test plan - [ ] Run microscope, switch filters, verify `Filter wheel N: A -> B (usteps=X)` appears in log - [ ] Run with `--skip-init` and verify the WARNING surfaces - [ ] Trigger a home (e.g. via timeout-recovery path or explicit home call) and verify the entry/exit pair appears - [ ] Confirm no log spam during normal acquisition (per-move INFO is one line per filter change, not per FOV) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This was referenced Jun 12, 2026
Alpaca233
added a commit
that referenced
this pull request
Jun 15, 2026
…et move (#559) ## Problem A microscope crashed at startup with an uncaught exception: ``` squid.filter_wheel_controller.cephla - ERROR - Filter wheel 1 home succeeded but offset move failed; wheel is at the home reference, not slot 1. ... control.microcontroller.CommandAborted: firmware reported CMD_EXECUTION_ERROR ``` The home succeeded, but the **post-home offset move** to slot 1 was rejected by firmware with `CMD_EXECUTION_ERROR`. Because `MicroscopeAddons.prepare_for_use` calls `emission_filter_wheel.home()` with no error handling, the exception propagated through `Microscope.build_from_global_config` to `main_hcs.py` and killed the app before the GUI opened. ## Root cause An asymmetry introduced by #540: that PR taught `_move_to_position` (normal slot changes) to **recover** from `CMD_EXECUTION_ERROR` — acknowledge the abort and resend the MOVETO. But it left the **identical** MOVETO command inside `_home_wheel` with no recovery, so the same recoverable rejection is gracefully absorbed during operation yet **fatal at startup**. Verified against firmware (`firmware/controller`): a filter-wheel `MOVETO_W` returns `CMD_EXECUTION_ERROR` from exactly two sites (`mark_move_failed` / `report_move_error`), both meaning the motor never moved (wheel not enabled, or target outside `[xmin,xmax]` in `tmc4361A_moveTo`). So a plain resend is the documented-safe recovery — exactly what `_move_to_position` already does. ## Fix (host-side) - **`squid/filter_wheel_controller/cephla.py`** — `_home_wheel`'s offset move now mirrors `_move_to_position`: on `CommandAborted`, acknowledge + resend once. A failed resend or any other error type (e.g. `TimeoutError`) still propagates; re-homing is not attempted from inside the home path. - **`control/microscope.py`** — `prepare_for_use` now contains a filter-wheel homing failure: it logs the error with traceback and continues with the wheel position unknown, so a recoverable hiccup no longer bricks the whole GUI (the operator can re-home). Added a module logger. ## Tests (TDD — written first, confirmed failing, then green) - `_home_wheel` resends on `CommandAborted` and completes homing (W + W2) - a failed resend re-raises and leaves the tracked position untouched (W + W2) - a `TimeoutError` on the offset move is **not** resent (guard against over-reach) - `prepare_for_use` no longer propagates a filter-wheel homing failure `46 passed` across the filter-wheel and microscope suites; black-clean at 120 chars. ## Out of scope (flagged for later) A latent firmware robustness gap, not the proven trigger: the W/W2 axes are never range-calibrated (`xmin/xmax` stay full int32) and `finalize_homing_w/w2` ignores the `ERR_OUT_OF_RANGE` return from `setCurrentPosition`. Worth hardening separately if filter-wheel `CMD_EXECUTION_ERROR`s recur on a specific machine. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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
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.
Summary
Fixes a class of silent filter-wheel position desyncs by combining a firmware root-cause fix with a host-side switch to absolute MOVETO addressing. Alternative design to #539 — same firmware fix, but defers the W-position broadcast + post-move verification layer in favor of containment via absolute addressing. Lighter wire format, symmetric W/W2 coverage, simpler recovery flow.
Failure mode it fixes
Production incident:
main_hcs.log.1showed aMOVE_Wfor slot 2→1 returning "complete" in 5.9 ms when the physical move requires ~150 ms; host trusted the ack and updatedtracked_position, leaving every subsequent filter switch off by one slot until the user manually re-homed.Root cause in
stage_commands.cpp: each move callback only setmcu_cmd_execution_in_progress = trueinside theif (tmc4361A_moveTo(...) == 0)success branch. WhenmoveToreturned non-zero, or the earlyif (!enable_filterwheel) return;fired, the flag stayed false, and the next status broadcast reportedCOMPLETED_WITHOUT_ERRORSfor the newcmd_ideven though no motion happened. With no encoder feedback on W/W2 (HAS_ENCODER_W is False), the host had no way to notice.X/Y/Z don't manifest this as a persistent desync because the firmware broadcasts their position in every status packet — the host doesn't keep a separate tracker that can drift. The filter wheel was the one axis where this turned into off-by-one until manual intervention.
What changed
Firmware
Root-cause fix:
mcu_cmd_execution_statusbyte global; reset toCOMPLETED_WITHOUT_ERRORSon every received command and overwritten toCMD_EXECUTION_ERRORby amark_move_failed()helper.MOVE_X/Y/Z/W/W2,MOVETO_X/Y/Z/W) restructured to setin_progress = truebeforetmc4361A_moveToand callmark_move_failed()on the failure / disabled-filterwheel paths.send_position_updatereports the new status byte when the MCU is idle.Enable absolute filter-wheel addressing:
finalize_homing_w/finalize_homing_w2now calltmc4361A_setCurrentPosition(&tmc4361[w/w2], 0), anchoring the driver coordinate to 0 at the home reference. Previously X_ACTUAL was left at the hardware-dependent limit-switch latch value, making absolute targets meaningless.MOVETO_W2protocol command (= 43) +callback_move_to_w2(mirror ofcallback_move_to_w). Completes symmetric absolute-move coverage for dual-wheel setups.FIRMWARE_VERSION_MINORbumped 1 → 2.Host
CMD_EXECUTION_ERROR fail-fast:
CMD_EXECUTION_ERRORfrom ack timeout and aborts immediately viaabort_current_command(recoverable=True), saving the full 5 swait_till_operation_is_completedwait when the firmware tells us the move was rejected.recoverable=Truelogs at WARNING — caller will retry.Filter wheel absolute moves:
SquidFilterWheel._move_to_positioncomputes absolute microstep target =(target_pos - min_index) * usteps_per_slot + offset_usteps, anchored to the now-zeroed firmware coordinate frame. IssuesMOVETO_W/MOVETO_W2instead of relativeMOVE_W/MOVE_W2._home_wheel's offset move switched to absolute MOVETO for symmetry; every motion in the controller now goes through one path.move_w_to_usteps/move_w2_to_ustepshelpers +MOVETO_W2wired throughfirmware_sim_serial.py.Firmware version gate:
SquidFilterWheel.__init__raisesRuntimeErrorifmicrocontroller.firmware_version < (1, 2). The host now sendsMOVETO_Wagainst a post-homeX_ACTUAL = 0frame that older firmware does not establish; running the new host against pre-v1.2 firmware would silently send absolute targets that land at the wrong slot. Loud-fail at startup beats silent miscoordination weeks later.skip_init=Truerestart path — because firmware could have been re-flashed (or downgraded) between launches.Why absolute + fail-fast is enough (vs. broadcast + verify)
Containment, not persistent detection. With relative
MOVE_W, a silently-failed move leaves the host's_positionscache pointing at slot N+1 while hardware is still at slot N. The next relative move computes delta from the (drifted) host position, so hardware reaches slot N+1 while host thinks slot N+2 — divergence compounds.With absolute
MOVETO_Wtargeting slot N+2, the firmware drives to N+2 regardless of host's stale belief. Hardware and host re-converge on the very next successful move. One missed slot vs. persistent N-slot drift.What's still possible: a mechanical stall mid-move where the driver thinks it stepped but the motor didn't. Open-loop steppers without encoders can't detect this firmware-side. Detecting it requires the W-position broadcast + verification (PR #539's second layer). That can be added in a follow-up PR if a stall-class incident is ever reported.
Failure-mode coverage
tmc4361A_moveToreturns non-zeroenable_filterwheel == false(move before INITFILTERWHEEL)RuntimeErrorat construction). Re-flash firmware to proceed.Known limitations / follow-up
home()failure path. The publichome()method has no built-in recovery: if firmware homing itself times out (e.g. limit switch fault, stuck wheel), the exception bubbles up and_positionsis left un-updated. Callers must handle / re-home manually._home_wheelnow logs a clear message identifying which sub-step failed (the home command itself vs. the post-home offset move) so bench operators get an actionable error, but actual recovery is left as a follow-up._delta_to_ustepsduplication. The static helper incephla.pyduplicates the formula insquid.config.AxisConfig.convert_real_units_to_ustep. A clean refactor would giveSquidFilterWheelConfigaW_AXIS: AxisConfigfield built from the W-axis constants, mirroring the stage pattern. Out of scope for this PR —AxisConfigis a heavy pydantic model with many required fields, and it usesround()while the current helper usesint(); switching changes positioning by ±1 ustep at fractional boundaries.mcu_cmd_execution_status = COMPLETED_WITHOUT_ERRORSat the start of every received command, which can clobber a pendingCMD_EXECUTION_ERRORbefore the next position broadcast fires. TheHEARTBEATskip guard added in this PR closes the dominant in-practice case; the general fix (status tagged withcmd_id+ reset-after-observe) is a protocol change tracked separately.Test plan
pytest tests/squid/test_filter_wheel.py— 23 passed. Adds: absolute-target math, MOVETO_W2 routing, CommandAborted resend + TimeoutError re-home parametrized for both W and W2, home-via-absolute-offset, firmware-version gate (3 reject + 2 accept cases).pytest tests/control/test_microcontroller.py— 7 passed, 1 skipped. Adds:abort_current_command(recoverable=True)logs at WARNING; default logs at ERROR.pytest tests/control/test_firmware_sim_serial.py— 23 passed.pytest tests/control/test_firmware_protocol.py— 14 passed.black --check --config pyproject.tomlclean on all edited Python files.MOVE_WbeforeINITFILTERWHEEL. Confirm the host aborts within ~ms (not the 5 s ack timeout) and re-homes instead of advancingtracked_position. The production incident likely went through the siblingtmc4361A_moveTonon-zero path, which is harder to repro deterministically; both paths share the same fix.MOVETO_Wto slot N's expected microstep address after homing; confirm the wheel physically lands on slot N.SquidFilterWheelconstruction raisesRuntimeErrorwith a clear remediation message.Relationship to #539
This is an alternative design to #539. Both:
MOVETO_W2symmetry.The two PRs differ in the second layer:
Maintainers' call which approach to ship; this PR can stand alone, and the broadcast/verify layer can be added on top later if a stall-class incident is ever reported.
🤖 Generated with Claude Code