feat(fw,sw): Z encoder closed loop (firmware 1.6), filter-wheel shortest path, Z tuning tool and motion self-test - #645
hongquanli wants to merge 146 commits into
Conversation
TMC2660 path reproduces master bit-identically (values 29/29/26 pinned). TMC2240 path uses the datasheet form including /sqrt(2) — CURRENT_RANGE selects sine peak full scale, confirmed against the ADI spec line, Klipper and terjeio/Trinamic-library. octoaxes omits it and runs ~29% low. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ping Review finding 1: the clamp's justification was false. Master's uint8_t result wrapped mod 32 inside SGCSCONF's 5-bit CS field, so an over-request landed arbitrarily higher or lower — 1100 mA on X gave CS = 0, minimum current, where the clamp gave CS = 31, full scale. Replace it with a sentinel. Review finding 2: unify the two drivers on TMC_CURRENT_OUT_OF_RANGE; TMC2240_IRUN_OUT_OF_RANGE becomes an alias. Both paths now report. Also: name the correct sqrt(2) regression triple (15/15/19, not 15/22/19) and parenthesise the object-like macros. In-range results are unchanged: the master-equivalence sweep still finds 0 mismatches across all 3769 in-range milliamp values on the three shipped R_sense, and the 29/29/26 and 21/22/27 pins are untouched. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Round 1 rejected at the formula's nominal cscale = 1.0 ceiling, which made CS = 31 unreachable: master saturated at full scale for a band just above that ceiling (1045-1078 mA on X) without wrapping. Under the old guard, raising Z from 500 to 550 mA in the INI would have returned the sentinel and changed no current at all. Threshold is now master's own expression, scaled = cscale * 31 >= 32, which is exactly where the uint8_t left SGCSCONF's 5-bit CS field and wrapped mod 32. 1100 mA on X stays rejected (scaled = 32.65). Extended the equivalence sweep to the recovered band: 0 mismatches against both master spellings across 3891 in-range milliamp values (was 3769), with CS = 31 now reachable for 34/17/71 milliamps on the three shipped R_sense. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 2660 builders are pinned by native tests to master's exact datagrams (0x000900C3 / 0x000A0000 / 0x000C000A / 0x000E00A1), making the bit-identical requirement machine-checked rather than eyeballed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… contract The brief documented TMC2660 SGT as bits [12:8]. That is a 5-bit range for a 7-bit signed field; the datasheet and master's own (sensitivity & 0x7F) << 8 both put it at [14:8]. Code was already correct — only the comment was wrong, and Task 4/5 would have derived a truncating 0x1F mask from it. Also spells out that these are pure encoders that mask rather than validate. Master's config_init_stallGuard constrains sgt to -64..63 and reports failure outside it; that check must stay at the caller, or an out-of-range sensitivity that master clamped to +63 silently encodes as a negative threshold. Same for CHOPCONF.HEND, where hend = -4 wraps to maximum hysteresis end. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 1. driver_math.h returns 0xFF for three different failures (TMC_CURRENT_OUT_OF_RANGE, TMC2240_IRUN_OUT_OF_RANGE, TMC_MRES_INVALID). These builders mask, so an unchecked sentinel launders into the worst possible value rather than an obvious one: tmc2240_ihold_irun_value(_, 0xFF, _) -> IRUN 31, maximum current tmc2660_sgcsconf_datagram(0xFF, ..) -> CS 31, maximum current tmc2240_chopconf_with_mres(s, 0xFF) -> MRES 15, a reserved code Asking 3000 mA of an axis at CURRENT_RANGE = 1 is correctly refused by tmc2240_irun and then, if forwarded, becomes sustained full-scale current into an undersized motor - inverting the contract at driver_math.h:99-102. The Task 4/5 callers do guard, but the hazard was undocumented exactly where it would be introduced, and driver_math.h:126-127 invites the composition by saying MRES 'can be mirrored directly'. No validation added: these are pure value constructors with no error channel, so the check belongs at the caller. Also, three smaller documentation fixes: - SLOPE_CONTROL is DRV_CONF [5:4], not [7:4]; bits 6-7 are reserved. The test masked 0x0F, which would have reported a reserved-bit fault as a wrong slope value. Now asserts the 2-bit field and the reserved bits separately. - 0x000900C3 decomposed into its real chopper fields (TBL 2, CHM 0, HEND 1, HSTRT 4, TOFF 3) instead of presenting bit 16 as an address appendix; bit 16 is TBL[1], so parameterising blanking means editing that field. - TMC2240_SHADOW_COUNT is an array length (0..0x74), not a highest address. Emitted values are unchanged; all 11 datagram tests still pin master. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review round 2. The body was master's opaque constant with the bit layout
explained only in the comment above it. This branch has now produced three
prose-versus-silicon errors (SGT [12:8], SLOPE_CONTROL [7:4], and a TBL
blanking gloss), every one of them in a comment sitting above correct code,
and comments are what Tasks 4 and 5 will read. Putting the layout in the
expression removes the failure mode structurally rather than adding a fourth
comment to get wrong.
ADDR | TBL 2 | CHM 0 | RNDTF 0 | HDEC 0 | HEND 1 | HSTRT 4 | TOFF arg
Emitted words are unchanged: verified bit-identical to the previous expression
across all 256 toff inputs, not just the pinned 0 and 3, and the pinned
assertions (0x000900C3 enable / 0x000900C0 disable) are untouched. Firmware
size is byte-identical.
HEND carries the RAW field value 1 here. The +3 offset convention makes that a
hysteresis end of -2; the comment now distinguishes the raw value (derived from
master's bits, certain) from the offset reading (a datasheet convention), and
names the asymmetry with tmc2240_chopconf_value, which takes the offset-free
value and adds 3 itself.
Only TOFF is parameterised. The remaining fields are master's fixed chopper
settings and must not move without a bench thermal check (M5).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
driver_type defaults to DRIVER_UNKNOWN so an axis that was never probed fails safe. No behavior change yet — nothing reads these fields. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… word The struct comment said driver_type survives re-initialisation via both callback_initialize and init_filterwheel_axis. Only the former is true: init_filterwheel_axis calls tmc4361A_init() on its first line, which resets the field to DRIVER_UNKNOWN, so that path must probe every time it runs. The SPIOUT_CONF comment presented TMC_SPIOUT_CONF_PROBE as the 2660 word with a longer datagram. 0x4440108A ^ 0x4445000A = 0x00051080: besides the COVER_DATA_LENGTH change in bits 18/16, bits 12 and 7 are also cleared (AUTO_DOUBLE_CHOPSYNC / COVER_DONE_ONLY_FOR_COVER and STALL_FLAG_INSTEAD_OF_UV_EN / AUTOREPEAT_COVER_EN, per SPI_OUTPUT_FORMAT), which is why each init() must write its own full word. Comments only; no constant changed. firmware.hex is byte-identical. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Pure rename, no code change. The file is the TMC4361A motion-controller layer; the 2660-specific power-stage bodies move out in the next commit, so keeping the driver name in the filename would be wrong. Also updates src/init.h, which included the old path and is not listed in the task brief. Comment citations to master keep the old filename, qualified with the 856bc0e SHA, because they point at a historical file with historical line numbers.
Behavior-preserving move. Existing exported names delegate. Register words come from the pinned builders, and tmc2660_driver_init restores master's exact SPIOUT_CONF 0x4440108A. Verified by a host harness that links master 856bc0e's four function bodies and the extracted ones against the same recording stubs and diffs the write traces: 98564 comparisons, 0 differences, covering the init sequence, enable/disable, and config_init_stallGuard exhaustively over cs 0..31 x sgt -128..127 x filter x 6 vstall values, return value included. Deviations from the task brief, both to follow master: - config_stallguard keeps master's clamp-then-write and master's bool return (1 = accepted, 0 = clamped). The brief's early return would have left STOP_ON_STALL unconfigured and inverted the success value, since NO_ERR is 0 while master returned 1. - The dispatch declarations stay in stepper_driver.h with its include of TMC4361A.h; TMC4361A.h therefore cannot include stepper_driver.h, so TMC2240_SHADOW_COUNT stays in tmc2240_regs.h. Also collapses the duplicate SGCSCONF/SFILT macros onto the tmc2660_regs.h definitions the native tests pin.
config_stallguard built its datagram with tmc2660_sgcsconf_datagram(cs, ...), which masks cs & 0x1F. Master (856bc0e TMC4361A_TMC2660_Utils.cpp:2146) does `datagram |= tmc4361A->cscaleParam[CSCALE_IDX]` with no mask. Build the SGT/SFILT part with cs = 0 and OR the raw value instead. cs > 31 is reachable: callback_configure_stepper_driver accepts a u16 milliamp value and the current formula stores whatever uint8_t it yields. At 65535 mA on X, cs = 153: master writes SGCSCONF 0x000D0C99, the masked build wrote 0x000D0C19 — a silent motor-current change on X/Y as soon as a later task calls the seam after current has been raised. This is the same argument already applied to tmc4361A_cScaleInit, which also ORs unmasked; the two SGCSCONF writers must agree or the word an axis holds depends on which ran last. Verified with the equivalence harness rebuilt over the FULL cscaleParam domain rather than 0..31 (the range where the defect is invisible by construction): 802052 comparisons, 0 differences, covering cs 0..255 x sgt -128..127 x filter x 6 vstall values, plus out-of-domain int32_t values including negatives, plus the init/enable/disable traces and the return value on every case.
Ported from octoaxes with four corrections. Two were called for by the plan: the current formula includes the /sqrt(2) they omit (M8), and enable() sources TOFF from the shadow cache rather than a cover read -- their own later fix (new-W-axis 8136bff), which postdates PR #571. Two more were found while porting: - config_stallguard() sets sg4_filt_en at BIT 8 of SG4_THRS, read-modify-write from the shadow. Writing the whole register with 1 leaves the filter off and sets the StallGuard4 THRESHOLD (bits [7:0]) to 1 instead, which trips stall detection almost immediately, and clears SG_ANGLE_OFFSET on the way past. Layout pinned by a new case in test_driver_regs. - config_stallguard() sources COOLCONF from the shadow. octoaxes uses tmc2240_fieldWrite, which reads the register first, and their register table marks COOLCONF readable -- so that read goes out over the same unreliable cover path the enable() fix exists to avoid. Same latent bug, one register over. init() also writes SCALE_VALUES and the CURRENT_CONF scale-enable bits. Under SPI_OUTPUT_FORMAT 0x0D the TMC4361A drives the coils, so leaving SCALE_VALUES at reset transmits zero current and the motor never moves; the TMC2660 path gets this from tmc4361A_cScaleInit(), which a 2240 axis cannot call because its first half emits a TMC2660 SGCSCONF cover datagram. driver_toff is set from the CHOPCONF word init() actually wrote rather than inherited from tmc4361A_init()'s TMC2660-shaped default of 3. Nothing calls this yet; Task 7 wires it in. Both 0xFF sentinels (tmc2240_irun, tmc_microsteps_to_mres) are guarded before reaching a masking builder, and shadow writes are bounds-checked against TMC2240_SHADOW_COUNT.
Round 1 review fixes for the TMC2240 driver module. filter_en now reaches COOLCONF.SFILT (bit 24), the StallGuard2 filter, applied in the SAME read-modify-write as SGT because the two share COOLCONF. The previous SG4_THRS.sg4_filt_en write targeted StallGuard4, which only operates under StealthChop and is inert under the SpreadCycle init() configures -- so a 2240 axis ran UNFILTERED StallGuard from the same argument that gives a 2660 axis a filtered one via SGCSCONF.SFILT. Unfiltered SG2 has several times the per-fullstep variance, so M6 bench tuning would have found an SGT stable on 2660 axes that trips spuriously mid-scan on 2240 axes. COOLCONF.SGT was correct; the comment calling it StallGuard4 was not. init() now sets GENERAL_CONF.REVERSE_MOTOR_DIR (bit 28). Under direct_mode the TMC2240's SHAFT bit is inert and direction comes from the TMC4361A microstep table phase sequence, which format 0x0D maps opposite to 0x0A. Without this every 2240 axis runs backwards and homing drives away from the limit switch into the hard stop. Verified this is the only GENERAL_CONF write in the firmware besides tmc4361A_sRampInit's rstBits, which is read-modify-write and preserves bit 28. Chopper constants moved to the octoaxes production set (HSTRT 0, IHOLDDELAY 7), the only values proven on this silicon in this topology; master's HSTRT 4 was tuned for a TMC2660 with an external sense resistor and a different hysteresis decode and is not transferable. Also: cast cscaleParam operands to uint32_t before shifting (255 << 24 is signed-overflow UB, reachable at hold_ratio = 1.0); record the interpolation divergence in the chopper table; drop the now-dead tmc2240_sg4_thrs_with_filt_en and its pin test, keeping the SG4_THRS constants documented as StealthChop-only.
A 2240 is positively identifiable via IOIN.VERSION; a 2660 is not, so inconclusive is defined as "nothing answering" (all-zeros/all-ones) and yields DRIVER_UNKNOWN. Votes across 3 reads because cover reads are inherently unreliable. Deviates from the brief in one decision-table cell: the brief elects DRIVER_TMC2660 whenever fewer than two reads look dead, so one 0x40, one dead read and one other read is called a 2660. That is a flaky bus with conflicting identity evidence, and guessing 2660 on a real 2240 is a silent failure (never enters direct_mode, never moves, accepts every move command) where DRIVER_UNKNOWN is a loud one. A verdict now needs a strict majority of the three reads; with no majority the answer is DRIVER_UNKNOWN. Every other cell of the table is unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The liveness rule is safe only if a live TMC2660 can never reply all-zeros. The alignment half of that argument is confirmed (the octoaxes cover path recovers replies right-aligned to frame length, so the trailing bits are genuinely received); the pass-through half — that SDO keeps shifting our transmitted bits out past bit 20 rather than tri-stating — is confirmed nowhere, and octoaxes is no evidence either way because its probe has no liveness test to be right or wrong about. If it is false, every 2660 axis reads DRIVER_UNKNOWN and refuses to move. Store the last read in TMC4361ATypeDef::driver_probe_raw (zeroed in tmc4361A_init alongside driver_type) so Task 7 can emit it over the packet protocol. The bench gate becomes a log read on any machine instead of a special build, and the coincidental-0x40 risk stays auditable in the field. Diagnostic only; nothing branches on it, and no Serial printing from the probe. Header comment gains three things it was missing: - If the bench does show all-zeros, the remedy is to drop the ZEROS half of the liveness test, NOT to consult COVER_DRV_HIGH_RD. Under the confirmed alignment that register holds reply bits [19:12] — MSTEP[9:2] at RDSEL=0, SG[9:5]/SE[4:3] at RDSEL=2 — the bits most likely to be zero at standstill. If LOW is zeros, HIGH very probably is too. - The warm re-probe path runs at a different RDSEL. init_filterwheel_axis calls tmc4361A_init(), which resets driver_type, so callback_ initfilterwheel re-probes an already-configured 2660 with SDOFF=1 and RDSEL=2, the SG/SE readback — both zero at standstill. That is the path most likely to read all-zeros and the one cold-boot bench testing never exercises. Measure both. - The probe's read datagram is a write from the 2660's point of view: six frames land DRVCTRL = 0. Benign, because DRVCONF sets SDOFF=1 right afterwards and auto-SPI overwrites that register continuously. No change to the voting or liveness rules. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
env:native compiles only src/utils, so tmc2660.cpp, tmc2240.cpp and driver_probe.cpp had no automated coverage at all — the 106 existing cases pin the pure builders in tmc2660_regs.h / tmc2240_regs.h, and a builder returning the right word says nothing about whether the driver calls it, in what order, or at all. An edit that moved SPIOUT_CONF after the cover datagrams, dropped writeSPR(), or deleted the REVERSE_MOTOR_DIR write left every test green and mis-initialised real hardware. test_driver_sequence links recording stand-ins for the seven TMC4361A primitives against the real driver sources, which it #includes directly — the convention test_crc8 already uses for utils/crc8.cpp. Host shims for <Arduino.h> and <SPI.h> live in test/test_driver_sequence/stubs and reach the compiler through a NATIVE-ONLY include path; env:teensy41 and the shared build_src_filter are untouched, and firmware.hex is byte-identical. 22 cases, all expectations spelled as literals rather than as the production constants that produced them. The TMC2660 literals are master 856bc0e's own, read out of git show rather than transcribed, so design M5's bit-identical claim is now checkable in CI. Pinned: both init sequences operation by operation (SPIOUT_CONF first); 2660 enable/disable words and the driver_toff fallback; the four-write stallguard order, its unmasked cs OR and master's inverted 1 = accepted return; both out-of-range sentinels rejected without writing; the TMC2240 REVERSE_MOTOR_DIR write; enable/set_microsteps sourcing CHOPCONF from the shadow with zero cover reads; SGT and SFILT in one COOLCONF write; the shadow bounds guard; and the probe's word, read shape, settle budget, 13-case decision table and driver_probe_raw on every exit path. Every case was checked by mutation: 26 deliberate defects introduced one at a time into the three modules, all 26 caught. The one that initially survived found a real weakness in this file, not in the drivers.
Fix round 1 for task 6b. No production source touched. F1: test_tmc2240_init_writes_expected_sequence seeded driver_toff = 3 and then asserted it was 3 — the precondition guaranteed the conclusion, so deleting tmc2240.cpp's caching line left the case green. Both that case and the enable() round trip now seed 0xAA, so neither can pass through the TMC2240_DEFAULT_TOFF fallback. This is the same defect class the task 6b report flagged in task 5's harness, one file over. F2: three host-closeable behaviours were unpinned and, worse, unnamed — init's TMC_MRES_INVALID -> 0 fallback, the negative/NaN clamp on hold_ratio, and the >31 / >255 ceilings. One case each. Deleting any of the four guards they cover now fails. F3: the fourth guard, the negative/NaN test on ihold, is NOT pinnable and is now labelled as such in the source. Removing it makes (uint8_t)(-21.0f) undefined rather than defined-wrong, and this toolchain produces the same IHOLD 0 as the guarded path; the brief's post-cast form survives too. Both were run as mutations and both survived. Recorded, not hidden. Mutation count is now 32: 30 caught, 2 survived, both the F3 pair. 25 cases, 131 native tests. firmware.hex byte-identical (6edfb95e).
Makes the driver modules reachable: the boot path now probes each axis and dispatches on the result, instead of calling the TMC2660 entry points directly. Replaces tmc4361A_tmc2660_config with driver-agnostic tmc4361A_motor_config at all NINE call sites (design section 5 undercounted at eight - commands.cpp:168, the filter-wheel path, was missed), and probes each axis at init. X/Y/Z probe at boot; W/W2 probe in init_filterwheel_axis, which is where they are first brought up, and which must re-probe on every run because tmc4361A_init() resets driver_type on its first line. r_sense and current_range are set immediately after tmc4361A_init() zeroes them and before anything asks for current: r_sense = 0 encodes CS = 0 on a TMC2660, i.e. minimum current, not a wrong one. tmc4361A_motor_config ends in writeMicrosteps + writeSPR. Without them the five cmd-21 call sites would stop writing STEP_CONF altogether, because tmc2660_driver_set_microsteps is a deliberate no-op and setMicrosteps/setSPR only store to the struct - a host microstepping change would have been silently dropped. The resulting bus sequence on a TMC2660 axis is master's config+update, operation for operation. Configuration now follows SPI.begin() AND tmc_driver_init(), because the seam writes registers where master's config call only wrote struct fields. callback_initialize re-applies run current: master got that free from cScaleInit inside tmc2660 init, but tmc2240_driver_init seeds IHOLD_IRUN to zero by design, so an INITIALIZEd-but-not-configured 2240 axis would have no torque. Adds report_driver_probe(), a boot-time serial log of driver type and the raw probe word (design M7), emitted only for axes actually probed - the field initialises to 0, which is indistinguishable from a genuine all-zeros read, so an unprobed axis must produce no line rather than a misleading one. This is what makes the design's step-0 bench gate a log read instead of a special build. Also removes tmc4361A_config_init_stallGuard, which had no callers left and would have written a TMC2660 SGCSCONF datagram to a TMC2240 axis, and collapses the duplicated microsteps->MRES conversion onto tmc_microsteps_to_mres(). pio run -e teensy41 green (FLASH code 43368 -> 45208, RAM unchanged); pio test -e native 131/131.
The boot report is safe and stays unflagged: setup() runs before loop(), so no status packet has been sent yet. The filter-wheel one is not, and the cost is worse than the "resync warnings" I recorded in the task report. The host accepts any 24-byte window whose last byte is zero, CRC ignored (microcontroller.py:1553, a legacy allowance). Status packets carry buffer_tx[19..21] = 0 on a fixed cadence and the report line contains no zero byte at all, so the first window accepted after the injected text is reliably a MISALIGNED one: trailing ASCII plus the head of the real packet. The host then reads msg[0]/msg[1] out of ASCII and overwrites x/y/z/theta with garbage - a wild stage position presented as a good reading, plus an ack for a command id nobody sent. That is not a dropped packet, it is a corrupted one accepted. INITFILTERWHEEL's report therefore compiles only under -D TMC_PROBE_REPORT_RUNTIME. The bench gate still needs that path - it re-probes an already-configured TMC2660 at RDSEL = 2, where SG and SE are both zero at standstill, and cold boot never exercises it - so it is captured from a purpose-built image instead. Documented in platformio.ini next to the existing interlock flag, at the declaration in init.h, and at the call site. Not gated on DEBUG_MODE: that replaces the status packets with human-readable prints entirely, which is a larger behaviour change than the one being avoided. Verified rather than assumed: objdump finds 1 report_driver_probe call in the flagged commands.cpp.o and 0 in the default. Both builds report identical FLASH code, since section alignment absorbs the call, so the size figures prove nothing on their own. pio run -e teensy41 green (FLASH code 45208, unchanged); pio test -e native 131/131.
An axis whose driver the probe could not identify now returns CMD_EXECUTION_ERROR instead of moving with unknown current scaling. The eight motion entry points in stage_commands.cpp gate on tmc_driver_ready() before touching any state, so a rejection leaves no direction, target, in-progress flag or focusPosition half-written. An identified axis evaluates one comparison and falls through, so a probed TMC2660 keeps master's behaviour exactly (design M5). Three things the brief specified that the code did not support: - The guard reports with report_move_error(), not mark_move_failed(). Every call site rejects before its callback claims mcu_cmd_execution_in_progress, so there is nothing of this command's to unwind; clearing that global would report an unrelated axis still moving as finished, since send_position_update() runs on its own timer. This matches the filter-wheel `enabled` gate already in the dispatcher. - callback_home_or_zero decodes no axis - it switches on the raw protocol constant, which is not an array index - so the guard converts with protocol_axis_to_internal(). Indexing tmc4361[] with buffer_rx[2] directly would consult the wrong axis for X/Y/Z and run off the end of the array for W(5)/W2(6). AXES_XY starts two axes and needs both ready. - Only the homing branch is gated. Zeroing calls setCurrentPosition(), which writes VMAX = 0 and leaves target equal to actual: it re-origins the coordinate and halts rather than commanding motion, and rejecting it would stop an operator re-zeroing an axis they are diagnosing. The predicate lives in stepper_driver.h so test_driver_sequence can pin it against every row of the probe's decision table plus the never-probed axis; stage_commands.cpp is not host-compilable. NOT covered: the eight call sites themselves, and check_joystick(), which still commands X/Y by-passing this fail-safe entirely - see the task 8 report, gap G1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…obe recoverable Rejecting host moves on a DRIVER_UNKNOWN axis was not enough. check_joystick() waits on !X_commanded_movement_in_progress, and rejecting MOVE_X means that flag never becomes true - so the guards held the joystick gate PERMANENTLY OPEN on exactly the axis they locked. The realistic sequence: X probes UNKNOWN, the host reports CMD_EXECUTION_ERROR, the operator reads that as "X is inhibited" and jogs by hand into the sample at whatever current the unconfigured power stage happens to be strapped to. Z had the same hole through the focus wheel: onJoystickPacketReceived() adds the wheel delta to focusPosition and do_focus_control() turns it into moveTo(z, focusPosition) every loop pass. Three gates added in operations.cpp - the X and Y joystick blocks and do_focus_control. They call tmc_driver_ready() directly rather than the axis_driver_ready() helper because they are SILENT: the joystick and focus wheel are not host commands, so writing mcu_cmd_execution_status would pin a hardware fault on whatever unrelated command the host last sent. Gating the joystick blocks also covers their else-branch tmc4361A_stop(), which is correct - an axis that is never commanded has no velocity to halt. The focusPosition clamp stays outside the gate so the wheel cannot free-run a wild target for recovery. callback_initialize now re-probes X/Y/Z, so a boot-probe failure no longer needs a power cycle. Only DRIVER_UNKNOWN axes are re-probed: re-probing an already-configured TMC2660 reads it at SDOFF = 1 / RDSEL = 2, and whether that can return all-zeros is the open question design section 10 step 0 goes to the bench for. An unconditional re-probe would let INITIALIZE brick a working stage axis - the inverse of the recovery it is for - and would insert probe datagrams ahead of the init on a healthy 2660 axis, departing from master on an M5 path. Neither guarded file is host-compilable, so nothing pinned the call sites and deleting a guard left the suite green. test_command_layout now scans both as text: exact reference counts, the report_move_error rejection, no mcu_cmd_execution_status assignment in operations.cpp, and for each of the ten guarded functions that the guard precedes the first moveTo/setSpeed. Brittle to renames by design, and it proves presence and order only - stated in the test. Ledger, not fixed here: callback_home_or_zero indexes tmc4361[]/stage_PID_enabled[] with protocol constants in the AXIS_Y and AXES_XY cases, so homing X disables PID on Y and vice versa. Pre-existing on master; own PR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bumps FIRMWARE_VERSION_MINOR 1.4 -> 1.5 and documents the feature in firmware/README.md: how an axis is identified, what happens to one that cannot be, how to recover, the current/StallGuard model, and the TMC_PROBE_REPORT_RUNTIME bench flag that must never ship. The README states the deployment gate prominently: the probe's liveness rule rests on an assumption that could not be verified in software — that a live TMC2660 never returns all-zeros. If it is wrong, every TMC2660 axis on every existing board refuses to move on first boot. Design doc section 10 step 0 must pass before this firmware goes onto a 2660 board for general use. Also corrects a comment in TMC4361A_Utils.cpp that still said move rejection was "NOT in this commit" — it landed in f54c5f6/56b663b1. No protocol change: constants_protocol.h, software/control/ firmware_sim_serial.py and software/tests/control/test_firmware_protocol.py are byte-identical to base 856bc0e, so this firmware runs against existing host software. Host gating is >= (1, 1) at most, so 1.5 passes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ice on the TMC2240 Two behavioural fixes from the whole-branch review. callback_enable_stage_pid (cmd 26) was an unguarded actuator path. PID_BPG0 sets ENC_IN_CONF.REGULATION_MODUS, after which the TMC4361A drives the motor continuously to null the encoder error with no further host command - on an axis whose driver the probe could not identify, that is motion at unknown current. It now rejects through axis_driver_ready, which is no longer static so that cmd 26 shares the one definition of the guard's contract (report CMD_EXECUTION_ERROR, leave mcu_cmd_execution_in_progress alone) rather than carrying a second copy. Guarded sites: 13 -> 14. hold_ratio was applied twice on the TMC2240 - IHOLD = IRUN x hold_ratio on the chip AND HOLD_SCALE_VAL = hold_ratio x 255 on the TMC4361A - and once on the TMC2660. With Z_MOTOR_I_HOLD = 0.5 a TMC2240 Z axis held at 25% of run current where a TMC2660 Z holds at 50%, the objective-sag direction. IHOLD is now written equal to IRUN, leaving the TMC4361A's HOLD_SCALE_VAL as the single attenuator on both drivers. Collapsing it that way rather than the other way is deliberate: under GCONF.direct_mode the TMC4361A writes coil currents into DIRECT_MODE (0x2D), and the family documentation for that register (TMC2160A/TMC5160 XDIRECT, same address, same 9-bit signed coil fields) says the current is scaled by IHOLD. If that carries to the TMC2240 then the old code was cutting RUN current by hold_ratio, not just hold current. IHOLD = IRUN is correct under that reading, under the reading where the chip still switches IRUN -> IHOLD at standstill, and if both fields are inert under direct_mode; dropping HOLD_SCALE_VAL instead would be correct only under the second. octoaxes is not evidence for the old behaviour - MotorControl.cpp:427 hardcodes HOLD_SCALE_VAL = 128 and :536 sets IHOLD = irun x ratio, so they double-attenuate too. Still a bench measurement: standstill/running coil current on a TMC2240 axis must come out at hold_ratio, not 1.0 and not hold_ratio^2. Coverage: new test_commands_guards_the_pid_actuator_path scans commands.cpp (mutation verified - deleting the guard fails the suite), and new test_hold_ratio_attenuates_exactly_once_on_both_drivers pins IHOLD == IRUN and cross-driver HOLD_SCALE_VAL agreement across the hold_ratio domain (mutation verified). The ihold_f clamp is gone because hold_ratio no longer reaches the chip-side field, which retires the Task 6b "HONEST LIMIT" caveat. pio run -e teensy41 green (FLASH code:45400); pio test -e native 136/136. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019J74cNHRcMGQ38Ppap3QBg
…t-identity, boot order None of these change a single instruction; all three said something stronger than the source supports. firmware/README.md said "every motion path rejects it" of a DRIVER_UNKNOWN axis. It did not - ENABLE_STAGE_PID was open until the previous commit. The four actuator paths are now enumerated rather than asserted (host moves and the homing branch of HOME_OR_ZERO; cmd 26; joystick X/Y; focus-wheel Z), with the paths that are covered transitively named (the PID_BPG0 re-enables in finalize_homing_* are gated by is_homing_* AND stage_PID_enabled[]) and the ones that are deliberately not gated explained (CONFIGURE_STAGE_PID never touches REGULATION_MODUS; DISABLE_STAGE_PID is the safe direction; zeroing is a halt). "Never written to" is tightened to the driver chip - the TMC4361A is still configured. Count corrected to 14. A paragraph on hold current is added, since the previous commit changed what *_MOTOR_I_HOLD means on one driver. driver_math.h claimed bit-identity with master "for every in-range input". That holds for the three shipped R_sense values (0.22, 0.43, 0.105) and is overstated for arbitrary R: master computed the quotient in double and narrowed to a float parameter, this header computes in float throughout, and a wide sweep turns up a handful of +/-1-CS divergences on truncation boundaries. Nothing here can reach them - R_sense comes only from the def_v1.h constants and no command carries it. Re-derived rather than copied from the ledger; the shape is stated instead of a count, because the count depends on which master spelling is compared (init.cpp used / 1000, stage_commands.cpp / 1000.0). tmc4361A_motor_config's comment said its writes "reproduce master's tmc4361A_tmc2660_update() exactly". That is true of the FUNCTION and false of the BOOT SEQUENCE, which the comment was being read as covering: master ran its struct-only config before SPI.begin() and wrote each axis once (15 bus operations), whereas the probe forces driver init to run first here, so a TMC2660 stage axis takes cScaleInit with CS = 0, skips writeMicrosteps (microsteps = 0 is not a legal MRES), writes FS_PER_REV = 0, and then repeats all three with real values - 21 operations. Final state converges to master's and the transient is zero current, so this is a claims problem and not a hazard. Left as-is rather than reordered: matching genuinely would mean splitting the driver seam's set_current into compute and write halves, a late API change to the seam every axis uses, and it conflicts with the TMC2240 path's deliberate IHOLD_IRUN = 0 seed. The one real exception to the function-level equivalence is also now recorded - master always wrote the current scale, this refuses an unencodable request and skips the cScaleInit. Consequence for bring-up: the design's bench step 3 is a final-state register dump and cannot detect any of this, since both boot orders end at identical registers. It has to become a bus transcript. pio run -e teensy41 green (FLASH code:45400, unchanged); pio test -e native 136/136. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019J74cNHRcMGQ38Ppap3QBg
…values Two residual overstated claims from the final review wave. tmc2660.h said "Behavior is bit-identical to master" without qualification. The register sequences and datagram words genuinely are identical; the current scale is bit-identical only at the three shipped R_sense values (0.22, 0.43, 0.105 in def_v1.h), because this path computes in float where master computed in double and narrowed. Wording now matches the correction already made in driver_math.h:21-38. The SDD ledger's Task 7 line "boot order matches master op for op" is likewise corrected in place to point at the Fix E entry (tmc_driver_init now precedes tmc4361A_motor_config: 21 bus ops per stage axis where master had 15, first pass at zero current, end state converging). That file is gitignored by .superpowers/sdd/.gitignore and so is not part of this commit. Documentation only, no behavioural change: pio run -e teensy41 FLASH code:45400 unchanged; pio test -e native 136/136. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019J74cNHRcMGQ38Ppap3QBg
Documentation only; no behavioural change. - stepper_driver.h: tmc_driver_ready() named stage_commands.cpp as if it held all the enforcement, with a stale count of "eight" call sites. Enforcement is spread over stage_commands.cpp (move/home callbacks), commands.cpp (ENABLE_STAGE_PID) and operations.cpp (joystick, focus wheel). Describe the three files and the kind of site each holds instead of a number that goes stale. - stepper_driver.h: "Only these five operations differ between power stages" omitted tmc_driver_probe, which differs too but is not dispatched on driver_type — it runs before the type is known and is the only driver-specific code that reads the part. - driver_math.h: two comments restated bit-identity with master unqualified, contradicting the scoped header block above them. Both now refer up to the shipped-R_sense scope. - TMC4361A_Utils.cpp (sweep): "Everything else in this file is driver-agnostic" is untrue of tmc4361A_cScaleInit(), which emits the TMC2660 SGCSCONF datagram; and the fail-safe note enumerated two enforcing files and "both are pinned" when there are three. - test_command_layout.cpp (sweep): the section comment described two scanned files while the section scans three. pio run -e teensy41: FLASH code:45400 (unchanged). pio test -e native: 136/136. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019J74cNHRcMGQ38Ppap3QBg
…uning tool Firmware 1.6 (feat/tmc2240-driver-support + this): - SET_ENCODER_REPORTING (44): stream one axis's ENC_POS in the unused theta field, loop flags in byte 19 and the int16 loop error in bytes 20-21, or substitute ENC_POS for the axis's position field. Off by default; RESET and INITIALIZE turn it off, so the shipping packet is byte-identical to 1.5. - SET_PID_LIMITS (45): clamp the closed-loop correction velocity (PID_DV_CLIP) and arm a deviation watchdog (check_closed_loop) that disables the loop and latches a fault when |ENC_POS_DEV| exceeds the limit, so a wrong encoder sign or bad gain cannot run an axis into a travel end. - ENABLE_STAGE_PID is refused (CMD_EXECUTION_ERROR) until CONFIGURE_STAGE_PID has scaled the encoder since the last TMC4361A reset. - SET_PID_ARGUMENTS writes PID_P/I/D immediately. Previously the gains only reached the chip through a later CONFIGURE_STAGE_PID, and the host sends CONFIGURE first, so host gains never arrived. - report_move_error() moved to stage_commands.h so commands.cpp shares it. Host: Microcontroller.set_encoder_reporting / set_pid_limits / get_encoder_state, packet decoding of the new fields, ENCODER_REPORTING and ENC_FLAG constants, protocol parity test entries, unit tests. tools/z_encoder_pid_tuner.py: home Z, verify encoder sign/scale open-loop (auto-flip, abort on scale mismatch), open-loop baseline, closed-loop step response and P sweep, all inside a 1.0-4.5 mm window below the top switch at 1 mm/s with the loop clamped and turned off on any anomaly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…t rename; robust shutdown First hardware run of the check phase: guard rejected the initial move from depth 0, and the sampler's _stop Event shadowed threading.Thread._stop. Shutdown now runs each safety step independently. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…_PID on a stale offset First hardware run showed the loop error pinned at the int16 clip after homing: INITIALIZE's TMC4361A reset zeroes ENC_POS wherever the axis stands, while homing re-zeroed only XACTUAL, leaving the encoder frame offset by the axis's position at INITIALIZE. Closing the loop would have slewed Z by that offset. - finalize_homing_x/y/z and HOME_OR_ZERO zero-mode now write ENC_POS = 0 with XACTUAL = 0 (the W homing path already did). - The post-homing PID re-arm indexed stage_PID_enabled/tmc4361 with the protocol axis id (AXIS_X = 0 is internal y); it now uses the internal index. - ENABLE_STAGE_PID is refused (CMD_EXECUTION_ERROR) when |ENC_POS_DEV| already exceeds the watchdog limit. - Tool: rest statistics use the sampler clock and a mean-removed RMS; it refuses to continue when the frame offset after homing is clipped or above a quarter of the watchdog limit, and re-checks the error immediately before each enable. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…duced; history comments exact Review of a97776b: the NOT_EQUAL against 10667 << 8 (2,730,752) was vacuous - the 24.8 value the old code wrote is (1 << 8) * 1.0 * 3200 / 0.3 truncated = 2,730,666. The test now computes that value the old way and rejects it. Three comments said the watch budgeted against the shifted copy throughout; only 6a8b12c did. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Fix pushed: e348c8b — PID_DV_CLIP is written in pulses per second (the clamp was never applied)Codex's review of 6a8b12c was right: that commit shifted the cached clamp for the watch budgets but still wrote Change ( Bench verification before any further closed-loop characterization (AI-docs brief F7): control on 6a8b12c with Also agreed with the review: "ack means converged" is overstated — the dwell reduces violations and does not meet the declared bound; the residual violations are real and will be traced on the fixed head (F8a) before any tolerance change; the 10 µm latency increase is mostly the 25–40 ms ring-down the ack now waits for, so the lever is the loop's tuning, measured as time to a stable exposure window (F8b). Merge stays on hold until F7 and F8 are in. Two pre-existing items the review noted for the follow-up PR: RESET zeroes the clamp caches but leaves 🤖 Generated with Claude Code |
|
Disposition of the two remaining points on e348c8b (record: AI-docs
Wording corrected: the ceiling was misconfigured, not every correction affected — at P 65535 the correction reaches 10,667 pps only above ~41 µsteps of error, so the ladders' small corrections never touched even the intended ceiling; large-offset slews and the watch budgets are what ran without it. 🤖 Generated with Claude Code |
…DV_CLIP and the watch Codex, on e348c8b: a helper-only test cannot see which value the command handler hands to the register; put the production path in a module both the handler and a test call, with the register write behind a small interface the test records. pid_clamp.h (header-only, no Arduino): PidClamp {override_pps, effective_pps} per axis replaces pid_dv_clip_usteps / pid_dv_clip_eff; pid_clamp_configure() (CONFIGURE_STAGE_PID: override else axis default, written and made effective), pid_clamp_set_limit() (SET_PID_LIMITS: override recorded, written and effective once the encoder is configured) and pid_clamp_reset() (RESET). The write is a function the caller supplies: production passes tmc4361A_set_PID_dv_clip on the axis's chip; test/test_pid_clamp passes a recorder and asserts, at the bench geometry, that CONFIGURE writes 10,667 pps (not 2,730,666), that a limit sent before the encoder is configured waits for CONFIGURE and then wins over the default, that 0.02 mm/s writes 213 pps, that after every step the last register write equals the watch's copy, and that RESET clears both. tmc4361A_init_PID() no longer takes or writes the clamp, so the register has exactly one writer. Native 10/10 suites (test_pid_clamp 5/5), teensy41 clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
…forgets it when the chip is reset Review of 823c9e5: (1) CONFIGURE_STAGE_PID on a wheel before INITFILTERWHEEL wrote PID_DV_CLIP through the module where the old code skipped init_PID (and its write) for a wheel not yet enabled - the clamp write and init_PID now share one `configured` condition, and the four-way init_PID chain becomes one call. (2) INITIALIZE and INITFILTERWHEEL reset the chip (PID_DV_CLIP back to 0) and clear encoder_configured, but the cache kept its value while its comment said "what PID_DV_CLIP holds": pid_clamp_chip_reset() clears the effective value there and keeps the host's override, which SET_PID_LIMITS documents as re-applied by every later CONFIGURE (native test). (3) A comment claimed CONFIGURE resets the chip's loop registers; it writes them. The test recorder keeps the last write separately so it cannot read past its buffer. Native 10/10 suites (test_pid_clamp 6/6), teensy41 clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
|
Pushed: 823c9e5 + 51a2b42 — the clamp's production path is now host-tested (head 51a2b42; record Z15/R11, brief F7 rewritten) Codex was right that "not host-compilable" was the wrong frame: the handler is not, the path is.
F7 is now a sampled clamp validation, per the review: at Completion accuracy unchanged and not claimed resolved (F8 after F7). 🤖 Generated with Claude Code |
… for ENABLE as well Finish plan (Codex, 2026-09-13): callback_enable_stage_pid tested the zone strictly (pos > -zone && pos < zone) while check_closed_loop() and pid_engage_pending() include the edge, so at exactly +-zone an explicit ENABLE wrote PID_BPG0 and the next policy pass opened the loop again. pid_in_home_zone() in pid_policy.h is now the one predicate for all three; native tests pin both edges, just inside, just outside, and zone zero. No behaviour change anywhere but at the exact edge. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
|
Finish plan prep done — candidate 78ce69d, package pinned (AI-docs
No merge approval implied; physical-fault and TMC2660 qualification remain separate gates. 🤖 Generated with Claude Code |
|
Bench package re-pinned: squid-bench
🤖 Generated with Claude Code |
…s the ramp and refuses motion on that axis until DISABLE or a validated ENABLE
Finish plan disposition (Codex, adopted 2026-09-14): allowing motion to continue
automatically after a detected fault works against the hard-limit safety
priority. Bounded contract:
1. a fault opens the loop, fails the command in flight and latches its cause
(unchanged), and now STOPS THE RAMP: failing the command's bookkeeping does
not stop a ramp already running (watchdog or switch fault with the loop
engaged in flight, threshold mode) and the joystick's velocity mode keeps
its last VMAX; tmc4361A_stop() writes XTARGET = XACTUAL and leaves velocity
mode, so the axis decelerates where it is (a no-op at rest);
2. ordinary motion on that axis is refused while the fault is latched:
axis_driver_ready() (every MOVE / MOVETO / HOME entry, X Y Z W W2), the
joystick and the focus wheel all test pid_fault; the rejection is the
CMD_EXECUTION_ERROR the host already handles;
3. DISABLE_STAGE_PID acknowledges the fault (deliberate open-loop recovery,
position unverified) and a validated ENABLE_STAGE_PID re-engages; RESET and
INITIALIZE clear it; CONFIGURE_STAGE_PID no longer clears it (it aligns the
frames, it does not restore confidence);
4. other axes are untouched.
Normal-motion speed is unaffected: the gate is one flag test per command.
Host: the CommandAborted raised for a refused or failed move names the latched
fault - axis, cause in the host's words, "refused until DISABLE_STAGE_PID or a
validated ENABLE_STAGE_PID", and a cause-specific recovery (PID_FAULT_CAUSE
.RECOVERY: position faults home after DISABLE; NO_RESPONSE and STOP_SWITCH ask
for inspection before homing) - and is not marked recoverable. The motion
self-test acknowledges a fault with a logged DISABLE before its return move
and still leaves the loop off. Tuner docstrings say which commands clear a
fault. Tests: host abort text and recoverability (two causes), self-test
DISABLE-before-return with a fault away from depth, the source-layout guard
counts the second rejection in axis_driver_ready.
Native 10/10 suites, teensy41 clean, host subset 308 passed.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
…validation, the focus target follows the stop, wheels outside the gate, range-free stop, faults attributed to the command's axis, startup and self-test acknowledgement Review of 1c02911 (Fable subagent) found four defects and three weaknesses: 1. ENABLE_STAGE_PID went through axis_driver_ready() and was refused while a fault was latched, so the "validated ENABLE clears the fault" leg was unreachable. axis_driver_present() (driver check only) is split out; ENABLE uses it, the move/home entries keep the fault gate; layout pins updated. 2. The focus wheel's target still held the interrupted move's target, and do_focus_control() re-issues it every pass once the fault clears - a DISABLE would have resumed the move with no command in flight. pid_trip_fault sets focusPosition = XACTUAL for Z. 3. A software restart skips RESET/INITIALIZE and CONFIGURE no longer clears a fault, so Z could stay refused for a session with no GUI action able to send DISABLE. CephlaStage._configure_axis acknowledges a fault latched from before the configuration with a logged DISABLE (position unverified until homed) before it asks for the loop. 4. The self-test only DISABLEd when a return move was needed; a fault within the no-move band left Z refusing every move after the run. restore() now has an _acknowledge_fault step that always DISABLEs a latched fault, logged. 5. A W/W2 fault is not on the wire (byte 18 carries X/Y/Z) and no host path could acknowledge it: the filter wheels are outside the gate (rotary, no hard limit; loop off and unqualified). 6. _describe_pid_faults() blamed every latched fault for every rejection and made them non-recoverable: it now describes only the aborted command's axes (_last_command_axes from the sent bytes), so a refusal on another axis keeps the plain reason and stays recoverable for the wheel resend logic. 7. tmc4361A_stop() inherits moveTo()'s travel-range check and silently does nothing with XACTUAL outside [xmin, xmax]; tmc4361A_stop_here() writes XTARGET = XACTUAL and leaves velocity mode without it. Also: CONFIGURE comment lists INITFILTERWHEEL as a clearer; the stop comment says an S-ramp at speed overshoots and returns; CommandAborted's docstring covers the non-recoverable fault case; the FakeMcu models CONFIGURE not clearing a fault; a tautological assertion removed; the layout test pins the gate inside axis_driver_ready(), the three operator-path gates, the stop and the focus retarget in pid_trip_fault, and that ENABLE avoids the gate. Native 10/10 suites, teensy41 clean, host subset green. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
…a wheel input; a latched fault keeps the frames CONFIGURE would realign Second review pass (Fable subagent) on 6caa7d5: 1. do_focus_control() re-issued a ramp to focusPosition every pass, so anything that changed that target while Z was faulted - the operator turning the wheel, or the limit clamp on a Z stopped outside the limits - became motion the moment DISABLE landed, on an axis declared unverified. Now: wheel travel is dropped while a Z fault is latched (onJoystickPacketReceived), do_focus_control() returns at once on a latched fault, and it issues a ramp only for a wheel input (focus_wheel_pending), never because a pass found the target different. A commanded move issues its own ramp; a target changed by a limit, a fault stop or a recovery cannot move Z on its own. tmc4361A_stop_here() returns the target it wrote and pid_trip_fault uses that one number for the wheel's target. 2. CONFIGURE_STAGE_PID realigned ENC_POS := XACTUAL unconditionally, so a "validated" ENABLE after a fault read a deviation of ~0 and cleared the fault with nothing verified (the restart path would have done exactly that). The frames are now left as they are while a fault is latched: they are the evidence the ENABLE check runs against; DISABLE + homing, or a validated ENABLE on agreeing frames, are the ways out. 3. The self-test's acknowledgement keys on byte 18's fault mask (present in both packet layouts), not the reporting-on flag; the fake models it. 4. Layout pins are body-bounded: joystick gates before the velocity writes inside check_joystick(), the early return and the pending gate before the ramp inside do_focus_control(), the stop, the retarget and the cleared pending inside pid_trip_fault(), the dropped wheel travel inside onJoystickPacketReceived(), and the fault-conditioned realign inside callback_configure_stage_pid(). 5. Comments corrected: ENABLE is gated on driver presence, not like a move; stepper_driver.cpp names axis_driver_present for ENABLE; globals.h lists every clearer of pid_fault; the contract statement names the wheel exemption; the stage's startup comment says what ENABLE is checked against; the host notes the pre-existing heartbeat window shared with _cmd_id. Native 10/10 suites, teensy41 clean, host suites green. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
…icit; stale comments; session-scoped recovery wording Third review pass on 8290d72: making the focus path issue ramps only for a wheel input silently removed two motions shipped instruments have always had - the lift from the home switch to Z_NEG_LIMIT after homing, and the move to a newly set limit when Z sits outside it - and a wheel detent right after homing would have jumped Z to the floor whatever its direction. Both are now queued explicitly (focus_wheel_pending in finalize_homing_z and SET_LIM's Z branches), still suppressed while a fault is latched (do_focus_control drops anything queued then), and cleared wherever the wheel's target is reset (INITIALIZE, zero, RESET). Comments that still described the every-pass re-issue corrected (pid_trip_fault, callback_move_z, CONFIGURE's realign note); pid_fault_axes() says who clears a fault; the stage's restart logs say the loop stays off for the session unless ENABLE validates on agreeing frames. The layout pin rejects a commented-out guard and pins the post-homing lift. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
|
Post-fault contract adopted and pushed — candidate e320bd3, bench package squid-bench f9e7bc3 (AI-docs finish plan, "Post-fault contract — ADOPTED"; review record Z18, R12–R14) Per Codex's recommendation: a closed-loop fault now (1) opens the loop, fails the in-flight command, latches the cause and stops the ramp — the additional check found that failing the command's bookkeeping left a running ramp (threshold-mode faults in flight) and the joystick's last VMAX in place; Host: a refused or failed move raises Focus path: Z moves from the focus wheel only for a wheel input; wheel travel is dropped while Z is faulted; the post-homing lift to the software floor and the move to a newly set limit — motions shipped instruments have always had — are queued explicitly and suppressed while a fault is latched. Three independent review passes ran before the push (findings and fixes in R12–R14: unreachable ENABLE leg, focus target re-launching the interrupted move, restart leaving Z refused, wheel travel released by DISABLE, CONFIGURE hollowing the ENABLE validation, and more). Native 10/10 suites incl. body-bounded layout pins for every gate, teensy41 clean, host suites green. Bench: Not covered by tests here: the physical behaviour of the stop and the refusal on the controller — that is the fault-path row of the finite sequence's final regression, on the pinned package. 🤖 Generated with Claude Code |
…up no longer acknowledge a latched fault Codex's lockout review (AI-docs 2026-09-13-pr645-lockout-review.md, P1 items 1 and 2): a logged automatic DISABLE is not explicit operator recovery. Now: - Motion self-test: a latched fault is left in place. Its restore emits neither DISABLE nor ENABLE nor a return move after a fault, and says that explicit recovery is required; its preflight refuses to run at all when a fault is already latched (the DISABLE it sends before the open-loop checks would otherwise acknowledge the fault as a side effect); every move is guarded before it is sent, so a fault seen in a read raises with the cause instead of sending a move the firmware refuses. The fake controller refuses moves while faulted, as the firmware does. - Stage startup: a fault latched from a previous session (a software restart skips RESET/INITIALIZE; CONFIGURE does not clear it) is preserved and surfaced as an ERROR naming the cause and the explicit recovery; no DISABLE, no ENABLE, the axis stays refused. - Explicit recovery, stated once (motion_selftest.EXPLICIT_RECOVERY and the startup error): close the application fully and relaunch (the controller is reset and re-homed), or send a deliberate DISABLE_STAGE_PID from the tuner to move open-loop at your own risk. A GUI acknowledgement action is a follow-up. - PID_FAULT_CAUSE.RECOVERY_UNKNOWN no longer prescribes unconditional homing when the cause is hidden (it may be an encoder or switch fault): read the cause with reporting off, or inspect before homing. Tests written failing first: faulted cleanup emits no DISABLE/ENABLE/move and keeps the latch (fault inside and outside the no-move band); a fault latched before the run refuses to start; startup preserves and surfaces the fault. Host subset 312 passed. Firmware unchanged. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
|
Lockout review dispositioned — candidate 02809fd (host only), bench package squid-bench 7ecbe95 (AI-docs Codex was right that a logged automatic DISABLE is not explicit recovery. Nothing in the host acknowledges a latched fault on its own any more:
Bench corrections (Codex items 3–5): Tests: host subset 312 passed (new cases written failing first); squid-bench 26 pass; firmware unchanged, the trace patch applies unchanged. Merge remains on hold pending the finite bench sequence on this pin and one final independent review. 🤖 Generated with Claude Code |
Bench: finish plan step 1 stopped — NO_PROGRESS at the engage with the 0.02 mm/s clamp (2026-09-14)Pins applied exactly as in the plan (Squid 02809fd in a fresh worktree + squid-bench 7ecbe95 patch, clean check; both images built; manifests; host selection 45/1;
Reading: the engage left ≈ 25 µsteps to correct at 213 pps (~120 ms). At this clamp Also from the brief: F6 resting band from the 72 existing schema-2 closed traces — |ENC_POS_DEV| after 30 ms at rest p50 1 / p95 3 / p99 4 / max 7 µsteps, 5.6 % of samples above the 2-µstep deadband, 38/72 moves never leave it (record in AI-docs). Controller left on the plain 02809fd image, reset, Z 0, loop off, no fault. 🤖 Generated with Claude Code |
Bench addendum: the clamp is proven at 213 pps; the step-1 fault is breakaway stiction at the engage (2026-09-14 20:11)One bounded diagnostic with the pinned tools (not the pinned phase): same arguments as
So step 1's NO_PROGRESS came from the engage itself: a 25-µstep frame-offset correction started from a stage that had been at rest for about a second did not move the encoder 2 µsteps within one 28 ms window at 213 pps — breakaway stiction after a dwell — whereas corrections that begin right after a ramp stop progress normally. Owner's call: give the progress window a breakaway allowance (e.g. the first window no shorter than two full steps of drive at the clamp, ≈ 150 ms at 213 pps, mirroring the NO_RESPONSE minimum drive) or accept the watch as stricter than the mechanics at very low clamps. The plan's 0.02 mm/s proof otherwise stands on this evidence; the P ladder and final matrix wait for your word. Tooling note: my diagnostic used 🤖 Generated with Claude Code |
Bench: P ladder stopped by a bench-patch capture overflow (2026-09-14 20:20) — package re-pin needed
From the overflowed trace: 8139 of the 8192 records are Fix belongs in the patch: gate 🤖 Generated with Claude Code |
… an explicit ENABLE from rest, watched Post-fault contract on the host side of the bench tool: ZTuner.loop_off() - every abort and the shutdown cleanup go through it - leaves a latched Z fault alone instead of sending DISABLE_STAGE_PID, which would acknowledge the fault and erase its cause. Recovery is a relaunch (RESET + INITIALIZE + homing) or the operator's deliberate DISABLE; the guard's abort names that. The 2026-09-14 finish bench read its NO_PROGRESS cause and then had the cleanup clear the latch before anyone else could. enableprobe: --enable-reps times, rest --rest-s with the loop off at the working extension, ENABLE watched from the host for --enable-watch-ms (error before the enable, ack time, engaged, time to inside the tolerance, faults with the cause read before anything clears it), then one closed step measured as ackprobe does (the firmware's own re-engage after the ramp is the automatic counterpart), the return, and a deliberate DISABLE before the next rest. A fault ends the run with its row written and the latch kept. The bench trace patch arms captures around both. Tests: loop_off/shutdown preserve the latch and still make the board safe; the enable row records the engage, the time to tolerance, a fault mid-window (cause read once, no DISABLE), and a refused ENABLE (cause read before the abort is acknowledged). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XuE4CfBaLX9MVuBno6ZXBo
Desktop: re-pinned package after the finish-bench assessment (2026-09-15) — Squid
|
…a latched fault; a fault once seen is sticky Codex (AI-docs d08e704) reproduced loop_off() sending DISABLE_STAGE_PID when the fault-status read raised: unknown was taken as "no fault". Now: a fault the tool has seen anywhere in the run (every sighting reads the cause, so _read_z_fault_cause sets fault_observed) means no DISABLE ever, whatever a later packet shows; otherwise loop_off waits for a status packet that arrived after the decision (the packet a host-side abort acted on precedes the firmware's own check in that pass) and treats no such packet within 300 ms, or an unreadable status, as unknown - which keeps the latch. A missed DISABLE costs nothing: a relaunch's RESET opens the loop anyway. Tests: unreadable status, no fresh packet, and a fault observed then a packet showing it clear - none send DISABLE; the no-fault path still does. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Desktop: the three corrections from the ENABLE-package review (AI-docs d08e704) — Squid
|
Bench: re-pinned package (b27dffc / 81e6ba6) — smoke PASS, the engage-time fault captured with its inputs (2026-09-15)Pins applied as written (fresh
Reading: same stage, same 1 s rest, same initial error — 0.6 µm of commanded drive at 213 pps did not break the stage away in 30 ms, 6400 pps did in under 10 ms. Verdict consistent with its inputs; not a clamp, budget-formula or tooling defect. Carried as the unresolved row "explicit ENABLE after rest at a 213 pps clamp"; the progress window is left unchanged as instructed. The P ladder is running next. 🤖 Generated with Claude Code |
|
Bench (Squid+ TMC2240, COM3): finish plan step 2 — P ladder done on b27dffc + squid-bench 81e6ba6. STOP for the gain choice. Setup: diagnostic image, ±10 µm × 10 per direction per arm (open + closed), window 0, clamp 1 mm/s, deadband 2 counts, watchdog 200 µm, rest-only, mic on. 120 traces, all complete; validator OK; clamp gates bounded; 0 faults;
Open arms (reference): 9–10/10 outside the bound at ack, 0.66–1.41 µm, ack 16–19 ms. 100 ms stable window (every sample inside the bound) within the 200 ms post-ack horizon:
Reading: at the ack all three gains are inside the bound in 58/60 closed moves (the two exceptions at 32768, both upward, 3 µsteps). What separates them is the stable window, which the resting jitter (±3–4 µstep excursions) breaks in 16/20 at 65535, 3/20 at 32768, 12/20 at 16384; upward is slightly worse at every gain. No gain gives zero excursions during the window in 20 moves; 32768 comes closest and has the lowest ack latency here. Ten moves per direction is a small sample for the 3/20 vs 12/20 distinction. Traced-image latencies are not production timing. Per the plan the bench stops here; one setting is @hongquanli's choice, then step 3 ( Evidence: AI-docs |
|
Addendum to the ladder, no hardware: the stable-window column was measured at the loop's own deadband, so it was scoring the deadband's ring rather than the gain. Same 60 closed traces, stable window swept over the bound it is measured against (squid-bench
Resting distribution, |encoder - target| pooled from the first sample inside 2 usteps to the horizon: Resting distribution, |encoder − target| pooled from the first sample inside 2 µsteps to the horizon: | 65535 | up | 1488 | 0.272 | 0.087 | 0.024 | 4 | 5 | 6 |
Records: AI-docs |
|
Correction to my two ladder comments above: the up/down labels were swapped. The tuner moves to What the corrected labels change:
Fixed in squid-bench Open-loop control arms, same three runs (60 moves, no new hardware time).
Records: AI-docs |
|
In-flight closed loop, retried on this branch (bench, plain b27dffc, no flash). The operator asked whether the Z loop can stay closed during moves instead of rest-only. Worth retrying because every earlier in-flight run wrote
Two conditions the earlier sessions could not separate. The gain must be low enough not to limit-cycle: at P 65535 the loop is stable in flight only to 0.10 mm/s, so the working clamp bounds the correction velocity without stopping the oscillation. And the correction clamp must be at least the move velocity: a 1 mm/s ceiling cannot follow a 2 mm/s ramp, so the lag runs away into the watchdog, while a 3 mm/s ceiling tracks 3 mm/s cleanly. What it costs (ackprobe, deadband and target tolerance 0.19 µm, 5 reps per cell, same session):
Same accuracy, no faults either side. Full closed loop is slower for a 1 µm focus step and for a 100 µm move and better nowhere, which is what the first-order tracker predicts: the low gain that buys in-flight stability also settles more slowly once the ramp stops. Recommendation unchanged, rest-only. In-flight is now a supported configuration rather than a hazard, and the rule for any future stage that needs it is a low gain with the clamp at or above the traverse velocity. Relevant to review: this exercised the in-flight fault paths for the first time. NO_PROGRESS and the deviation watchdog both fired during a ramp; each stopped the ramp, latched the cause, refused further motion until an explicit relaunch (RESET + INITIALIZE + homing), and the tuner preserved the latch rather than acknowledging it. No stall, no lost steps, no recovery surprises. Evidence: AI-docs |
Desktop: decision inputs after the independent evidence review (AI-docs 6267038) — squid-bench
|
| P | inside for the whole 100 ms after the ack, of 20 — at 2 ust | 3 ust | 4 ust | 5 ust | worst post-ack (ust) | "stable anywhere" at 2 ust | acks outside 2 ust |
|---|---|---|---|---|---|---|---|
| 65535 | 0 | 3 | 13 | 18 | 7 | 4 | 0 |
| 32768 | 3 | 15 | 18 | 19 | 6 | 17 | 2 |
| 16384 | 1 | 20 | 20 | 20 | 3 | 8 | 0 |
P 32768 overshoots to 6 counts right after the ack and settles later; P 16384 never leaves 3 counts after the ack but wanders across 2; P 65535 rings to 7. One count is 0.094 µm. Sampled evidence on the traced image; rare tails unqualified.
Decision for @hongquanli (an imaging requirement): the exposure bound for the 100 ms from the ack.
- keep ±2 counts (0.19 µm): no gain meets it at the ack (best 3/20) → the PR reports it unmet; settling work would follow.
- ±3 counts (0.28 µm), my recommendation: P 16384 rest-only, 20/20 in the sample, worst 3 counts; completion tolerance stays 2 counts (ack accuracy 0.19 µm, met 58/60). Inside the depth of field up to 40x/0.95 NA; marginal for 60x/1.4 oil, stated as a limitation. Cost: ~10–15 ms more ack latency than P 32768 on the traced image.
- ±5 counts (0.47 µm): all gains 18–20/20.
Then step 3 at the actual selected settings: -Phase final -P <P> -ExposureBoundUsteps <N> (the validator writes exposure_summary.json: acks outside the completion bound, captures inside the exposure bound for the whole 100 ms, censored, worst post-ack error), then the plain image, the regression and fault rows, and the final review. The selected settings then become the shipped opt-in defaults (PID_P_Z is 4096 today and was never in the ladder). Rest-only stays; the 213 pps ENABLE row stays a documented minimum clamp. Merge remains on hold.
Details: finish plan "Decision inputs (2026-09-15)"; squid-bench VALIDATION.md.
Decided (Hongquan, 2026-09-15): exposure bound ±3 counts (0.28 µm), P 16384 rest-only, completion tolerance unchanged — step 3 at these settingsBasis: z-stacks on the inverted configuration always run bottom to top, so only the upward arm is exposed after. In that arm on the ladder captures P 16384 holds ±3 counts for the whole 100 ms after the ack in 10 of 10 moves with a worst excursion of 3 counts; P 32768 8 of 10, worst 6; P 65535 3 of 10, worst 7 (per-direction table in the finish plan). The ladder's upward moves approach from below after a reversal, so they are the harder case for stack planes. Settings for step 3: P 16384, I 0, D 0, rest-only, clamp 1 mm/s, watchdog 200 µm, zone 200 µm, completion tolerance = firmware default two counts (0.19 µm), exposure bound 3 counts. No firmware change; the diagnostic and plain images are the ones already built from b27dffc. Bench (exact commands in the finish plan, "Step 3 at the selected settings"; squid-bench
Then the final independent review. Owner follow-up after step 3: the shipped opt-in defaults become these settings ( |
Summary
Firmware 1.6 and the host side of the Z encoder closed loop, plus the filter-wheel shortest path and completion window, the Z tuning tool, and a motion self-test in the GUI. Based on
feat/tmc2240-driver-support; the diff here is the 49 commits on top of it.Firmware 1.6 (protocol commands 44–50, status packet unchanged unless asked)
Also: gains applied immediately; ENABLE_STAGE_PID refused until the encoder is configured or on a stale frame offset; the loop only ever engages at rest (re-engage while moving stalled a motor); the first engage after a homing aligns the encoder frame to the counter (stages whose actuator homes below the stage's stop); RESET returns every loop setting to the firmware default; immediate completion packet and a 1 ms completion check; MOVETO clamps to the travel limits (beyond them the virtual limit hard-stopped the ramp and the command hung).
Completion contract on a closed-loop axis (2026-09-12)
A closed-loop move is acknowledged only once the encoder is at the target: with the loop engaged,
|ENC_POS − target|must be inside the completion window when one is set, else inside the target-reached tolerance (inclusive), and never tighter than the deadband the chip stops correcting inside of. One rule for every axis (commanded_move_complete(),pid_completion_encoder_ok()inpid_policy.h, native-tested). Measured on the 2240 bench before this (ackprobe, 360 rows): the ack fired on the pass that re-engaged the rest-only loop, 0.3–0.7 µm still to correct on 10 µm steps, because the exact-target leg trusted the chip's PID_E readout (below a 2-µstep tolerance while the live ENC_POS_DEV was 3–7 µsteps) and the window leg was an OR-alternative to it. Cost: the ack now waits for the correction, roughly +15–20 ms on a 10 µm focus step at the qualified gains; the bench re-run on this head is the remaining gate.Host
squid.configcarries the loop settings;CephlaStage._configure_axissends encoder direction, gains, limits, zone, tolerance, ramp profile, loop mode and completion window from the ini (before this,PID=Noneon every axis meant none of it reached the firmware, and the encoder direction was never passed).z_home_gap_mm, the floor in[SOFTWARE_POS_LIMIT],z_park_at_min_after_homing.QComboBox.textActivated(the Qt 5-only overload crashed the GUI on a PyQt6 venv); encoder transitions per revolution rounded instead of truncated (2999 for 3000).squid/motion_selftest.py, shared by Utils → Motion Self-Test (Z)… andtools/motion_selftest.py; homing, encoder scale and sign, gap map with floor/zone/park recommendations, lost steps, closed-loop stack and hold; nothing written to the ini.tools/z_encoder_pid_tuner.py(check, zonemap, zonetest, step, stack, hold, accel and velocity ladders, engage probe, residual),tools/filter_wheel_tuner.py.What the bench established (Squid+ TMC2240 Z and a TMC2660 Z, mic on every run)
Cephla-Lab/AI-docs→Squid/to-do/2026-09-07-tmc2240-z-closed-loop-report.md,2026-09-08-z-loop-modes-findings.md,2026-09-07-filter-wheel-switching-report.md,2026-09-06-tmc2240-bench-results.md; action itemsSquid/action-items/2026-09-07-motion-bench.md.Code review
A review of the whole diff against master (seven verifiers) found ten issues; all fixed in b2b3bfb and re-verified on the bench: completion window on a requested loop, fault latch on a refused re-engage, PID_DISABLE in the enable callback's early returns and in RESET, the self-test Start button skipping its confirmation, aborted moves counted as acks, restore after cancel, the main window left live during the run, wheel wrap on pre-1.4 firmware and odd slot counts, a pre-existing wheel enable arity bug, a refused loop enable taking the stage down, and the encoder flip default (now master's False; the Squid+ template sets True).
Tests
tests/squid/test_stage_loop_config.py,test_stage_z_park.py,test_filter_wheel_wrap.py,test_motion_selftest.py; protocol and microcontroller tests extended for 44–50. Final firmware verified on the bench: self-test PASS, MOVETO clamp check, 96-step stack, closed ladder, zone test.Defaults
Repository defaults leave the loop off and every new key at 0; the Squid+ bench ini carries the qualified values (documented in the template's comments).
🤖 Generated with Claude Code
https://claude.ai/code/session_014v4KYGCAH3Z9qZqYXZ2JEr