feat(fw): TMC2240 stepper-driver support behind a per-axis driver seam (firmware 1.5) - #644
Draft
hongquanli wants to merge 28 commits into
Draft
feat(fw): TMC2240 stepper-driver support behind a per-axis driver seam (firmware 1.5)#644hongquanli wants to merge 28 commits into
hongquanli wants to merge 28 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
… instead of silently keeping the previous current while the rest of CONFIGURE_STEPPER_DRIVER is applied tmc2660/tmc2240_driver_set_current and tmc_driver_set_current return whether the request was applied; tmc4361A_motor_config passes it on; callback_configure_stepper_driver reports a refusal to the host. Found by code review: with R_sense 0.22 the TMC2660 ceiling is ~1.08 A, and an ini above it left the axis at its boot current with no error. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014v4KYGCAH3Z9qZqYXZ2JEr
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
1 participant
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
Firmware 1.5: TMC2240 stepper-driver support behind a per-axis driver seam, with a boot-time probe that identifies the driver on each axis (TMC2660 or TMC2240) and a fail-safe that rejects motion on an axis whose driver could not be identified.
stepper_driverseam: the TMC2660 code moves behind it unchanged in behaviour; a TMC2240 module with shadow-register state sits next to it; register datagram builders for both are pure functions with native tests.Bench verification
Cephla-Lab/AI-docs→Squid/to-do/2026-09-06-tmc2240-bench-results.md(results, Step 0 detail) and2026-09-06-tmc2240-bench-log-raw.md(raw log).Open items (not blocking)
Merge order
This PR first;
feat/z-encoder-pid-interface(firmware 1.6, Z closed loop) is based on it and follows.🤖 Generated with Claude Code
https://claude.ai/code/session_014v4KYGCAH3Z9qZqYXZ2JEr