fix: detect silent filter-wheel move failures

[Alpaca233]

Summary

Adds firmware + host detection for a class of silent filter-wheel desync where the MCU reports a move as completed but the motor never moved. Observed in production: main_hcs.log.1 shows a MOVE_W for slot 2→1 returning "complete" in 5.9 ms when the physical move requires ~150 ms; the host then trusted the ack and updated tracked_position, leaving every subsequent filter switch off by one slot until the user manually re-homed.

Root cause is in stage_commands.cpp: each move callback only set mcu_cmd_execution_in_progress = true inside the if (tmc4361A_moveTo(...) == 0) success branch. When moveTo returned non-zero (or the early if (!enabled) return; fired), the flag stayed false, and the next periodic status broadcast reported COMPLETED_WITHOUT_ERRORS for the new cmd_id even though no motion happened. With no encoder feedback on W/W2 (HAS_ENCODER_W is False, cephla.py:114), the host had no way to notice.

Same bug pattern exists in MOVE_X / MOVE_Y / MOVE_Z and the MOVETO_* callbacks, but for those axes the firmware also broadcasts real position in every status packet (bytes 2-17), so absolute moves self-correct. The filter wheel is the one axis where this manifests as a persistent silent error.

What changed

Firmware

• New mcu_cmd_execution_status byte global; reset to COMPLETED_WITHOUT_ERRORS on every received command and overwritten to CMD_EXECUTION_ERROR when a move callback fails. send_position_update reports it in byte 1 when the MCU is idle.
• Move callbacks (MOVE_X/Y/Z/W/W2, MOVETO_X/Y/Z/W) restructured to set in_progress = true before tmc4361A_moveTo and call a mark_move_failed() helper on failure. The move_filterwheel and callback_move_to_w early-returns when enable_filterwheel == false now also mark failed instead of silently no-op'ing.
• W axis microstep position broadcast in status-packet bytes 19-20 (signed int16, big-endian). Gated on enable_filterwheel because tmc4361[w] is uninitialized at boot (init.cpp:123 only loops < STAGE_AXES); reading from it would deref a NULL config pointer.
• FIRMWARE_VERSION_MINOR bumped 1 → 2.

Host (Python)

• Microcontroller parses bytes 19-20 into self.w_pos when firmware >= v1.2. New supports_w_pos_broadcast() helper.
• SquidFilterWheel._move_to_position snapshots microcontroller.w_pos before each move and verifies the broadcast position changed by the commanded delta (within 50% tolerance) after wait_till_operation_is_completed returns. On mismatch it raises TimeoutError → falls into the existing re-home + retry path (cephla.py:138-159).
• Extracted _move_and_verify helper so the initial try and the retry-after-rehome branch share one code path.

Failure-mode coverage

Scenario	Before	After
tmc4361A_moveTo returns non-zero	Silent COMPLETED ack, host desyncs	Firmware reports CMD_EXECUTION_ERROR, host times out at 5 s, re-homes
Motor stalls / misses steps mid-move	Silent COMPLETED ack	Host sees W position delta < expected/2, raises TimeoutError, re-homes
enable_filterwheel == false (move arrives before INITFILTERWHEEL)	Silent COMPLETED ack	Same fail path
Old firmware (< v1.2)	n/a	Host skips verification — preserves current behavior

Known follow-up: W2

The broadcast only carries W; byte 21 is still reserved. The _verify_w_move path is gated on motor_slot_index == 3, so dual-wheel setups still get the old trust-the-ack behavior for the second wheel. Adding W2 broadcast needs a design decision on encoding (raw int16 — needs another byte and a packet bump, or 1-byte slot index — needs firmware to know usteps_per_slot). Punted because no W2 silent-failure incident is on file.

Test plan

• pytest tests/squid/test_filter_wheel.py — 8 passed
• pytest tests/control/test_microcontroller.py — 6 passed, 1 skipped (existing skip)
• Full suite pytest --ignore=tests/control/test_HighContentScreeningGui.py — 1206 passed, 7 skipped, 1 xfailed
• black --config pyproject.toml --check clean on edited Python files
• Build firmware locally and flash to a test Teensy — I can't compile firmware from this environment; please verify the C++ builds and the v1.2 status packet parses correctly on a real bench unit before merging
• Reproduce the original incident on bench: trigger a MOVE_W while enable_filterwheel == false (boot without sending INITFILTERWHEEL) and confirm the host re-homes instead of advancing tracked_position
• Confirm boot safety: cold-boot a unit with no filter wheel hardware connected; confirm no SPI hang or hard fault from the (now-gated) W broadcast path

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes a class of silent filter-wheel desync where the firmware acked a MOVE_W as completed even though the motor never moved (e.g. tmc4361A_moveTo returned non-zero, or enable_filterwheel == false so the early-return fired). The host had no encoder feedback on W and trusted the ack, leaving every subsequent slot index off by one until the user manually re-homed. The fix bumps firmware to v1.2 to (a) report CMD_EXECUTION_ERROR from move callbacks that fail and (b) broadcast the W microstep position in status-packet bytes 19-20, then teaches the host to snapshot W before each filter-wheel move and verify the broadcast position changed by the commanded delta (within 50%). On mismatch it raises TimeoutError, falling into the existing re-home + retry path.

Changes:

• Firmware: new mcu_cmd_execution_status byte; move callbacks set in_progress=true before tmc4361A_moveTo and call mark_move_failed() on failure; W axis position broadcast in bytes 19-20 (gated on enable_filterwheel); FIRMWARE_VERSION_MINOR 1→2.
• Host: Microcontroller parses bytes 19-20 into self.w_pos when fw ≥ 1.2; new supports_w_pos_broadcast() helper; new RESPONSE_BYTE_W_POS_* and MIN_FW_VERSION_W_POS_BROADCAST constants.
• Filter wheel controller: extracted _delta_to_usteps, added _verify_w_move and _move_and_verify helpers that snapshot W, command the move, wait, and verify; W2 still falls back to trust-the-ack.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
firmware/controller/src/constants.h	Bumps FIRMWARE_VERSION_MINOR to 2 with version-history comment.
firmware/controller/src/globals.h / globals.cpp	Declares/defines new mcu_cmd_execution_status byte global.
firmware/controller/src/serial_communication.cpp	Resets execution status per command; broadcasts W position into bytes 19-20 (gated on enable_filterwheel); reports mcu_cmd_execution_status in byte 1 when idle.
firmware/controller/src/commands/stage_commands.cpp	Adds mark_move_failed() helper; restructures every move/move-to callback to set in_progress before tmc4361A_moveTo and report failure on non-zero return or disabled wheel.
software/control/_def.py	Adds RESPONSE_BYTE_W_POS_HI/LO and MIN_FW_VERSION_W_POS_BROADCAST = (1, 2) constants.
software/control/microcontroller.py	Parses bytes 19-20 into self.w_pos when firmware ≥ v1.2; adds supports_w_pos_broadcast() helper.
software/squid/filter_wheel_controller/cephla.py	Adds _delta_to_usteps, _verify_w_move, _move_and_verify; uses W broadcast to detect silent failures and trigger the existing re-home + retry path.

Comments suppressed due to low confidence (2)

software/control/microcontroller.py:1624

• When the firmware now reports CMD_EXECUTION_ERROR in byte 1 (e.g. because tmc4361A_moveTo returned non-zero, or move_filterwheel was called with enable_filterwheel == false), the host's receive loop in microcontroller.py does not recognize that status: only COMPLETED_WITHOUT_ERRORS and CMD_CHECKSUM_ERROR are handled (see lines around 1561-1605). mcu_cmd_execution_in_progress therefore stays True until wait_till_operation_is_completed hits its 5 s timeout. The recovery still works (TimeoutError → re-home), but the host pays a full 5 s of latency for every silent-failure case the firmware now explicitly reports — defeating the point of the new status byte for the X/Y/Z move-only callbacks where there is no W-pos cross-check. Consider handling CMD_EXECUTION_ERROR explicitly so the host can fail fast (clear mcu_cmd_execution_in_progress and surface the error immediately).

                # W position broadcast added in firmware v1.2; older firmware
                # leaves bytes 19-20 as zero. Callers gate on
                # supports_w_pos_broadcast() before trusting w_pos.
                if self.firmware_version >= MIN_FW_VERSION_W_POS_BROADCAST:
                    self.w_pos = self._payload_to_int(msg[RESPONSE_BYTE_W_POS_HI : RESPONSE_BYTE_W_POS_LO + 1], 2)

software/squid/filter_wheel_controller/cephla.py:149

• In _move_and_verify, w_pos_before and expected_usteps_delta are bound only inside the if can_verify_w_pos: branch and then read again later inside another if can_verify_w_pos: branch. This works today because the predicate is evaluated against the same local variable, but static analyzers (and human readers) flag it as "possibly unbound local". Initializing both names to None before the first branch (or restructuring as a single if can_verify_w_pos: that wraps both the snapshot, the move, the wait, and the verify) would make the data flow explicit and prevent accidental rearrangement from breaking it.

        if can_verify_w_pos:
            w_pos_before = self.microcontroller.w_pos
            expected_usteps_delta = self._delta_to_usteps(delta)
        self._move_wheel(wheel_id, delta)
        self.microcontroller.wait_till_operation_is_completed()
        if can_verify_w_pos:
            self._verify_w_move(wheel_id, w_pos_before, expected_usteps_delta)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.

[hongquanli]

Opened #540 as an alternative design for the same root-cause bug. Same firmware fix (in_progress flag + CMD_EXECUTION_ERROR + mark_move_failed), but defers the W-position broadcast + verification layer in favor of switching the filter wheel to absolute MOVETO addressing — which contains any future silent-failure to a single move instead of compounding into persistent N-slot drift. Also adds MOVETO_W2 + post-home setCurrentPosition(0) so dual-wheel setups get symmetric coverage from the start.

Maintainers' call which approach to ship; if #540 lands, the broadcast/verify layer can be added on top later if a stall-class incident is ever reported.