Follow-up to #14. Two real bugs and two doc-rot items surfaced in Copilot's review of the SPI-timing fallback in handleJedecProbe (introduced in 66c69ba). Deferred to a follow-up rather than another round of squash-and-force-push on the PR.
Bugs
-
readSavePage errors propagate out and undo the identification. The fallback's "probe both flags" loop runs inside the outer try { ... } catch (e) { this.cachedSaveChip = null; } in detectCartridgeInner. A timeout / STALL from the wrong-flag probe (which we know wedges the firmware on some configurations — see [[project_sms4_test_carts]]) would bubble up to that catch and nuke the otherwise-valid identification we'd already cached. Fix: wrap the fallback's readSavePage calls in their own try/catch and fail closed to the already-identified chip; log the failure but keep the identification intact. src/lib/drivers/sms4/sms4-driver.ts:601.
-
Fallback doesn't validate short bulk reads. UsbTransport.receive() can return fewer bytes than requested, and readSavePage doesn't guard against that. If a primary probe comes back as e.g. 4 bytes instead of 16, the non-zero count is artificially deflated and the fallback could spuriously switch to the alternate flag on a chip that's actually responding correctly. Fix: validate s.length === 16 for each sample; abort the timing comparison (or treat the short read as a probe failure) if any sample is short. src/lib/drivers/sms4/sms4-driver.ts:567.
Doc rot
- The block comment in
handleJedecProbe references "256-byte boundaries" but the actual offsets are 0, 0x80, 0x100 — 0x80 is 128, not 256. Reword. src/lib/drivers/sms4/sms4-driver.ts:547.
- Same block comment still describes the older strict-allZero discriminator from the previous fallback design; the current implementation always probes both flags and compares non-zero counts (≥4 AND > 2× primary). Update the comment to match the implementation.
src/lib/drivers/sms4/sms4-driver.ts:547.
Scope
All four issues live inside the if (chip.kind === "FLASH") fallback block. Not blocking — the fallback works on the test cart (verified: gameCode IRAO dumped cleanly, 12/48 non-zero on the alternate flag, save data validates by trainer-name + Pokémon-roster extraction). But the error-handling and short-read guards are real correctness fixes worth landing before another IR-cart variant hits an edge case neither of us anticipated.
Follow-up to #14. Two real bugs and two doc-rot items surfaced in Copilot's review of the SPI-timing fallback in
handleJedecProbe(introduced in66c69ba). Deferred to a follow-up rather than another round of squash-and-force-push on the PR.Bugs
readSavePageerrors propagate out and undo the identification. The fallback's "probe both flags" loop runs inside the outertry { ... } catch (e) { this.cachedSaveChip = null; }indetectCartridgeInner. A timeout / STALL from the wrong-flag probe (which we know wedges the firmware on some configurations — see [[project_sms4_test_carts]]) would bubble up to that catch and nuke the otherwise-valid identification we'd already cached. Fix: wrap the fallback'sreadSavePagecalls in their own try/catch and fail closed to the already-identified chip; log the failure but keep the identification intact.src/lib/drivers/sms4/sms4-driver.ts:601.Fallback doesn't validate short bulk reads.
UsbTransport.receive()can return fewer bytes than requested, andreadSavePagedoesn't guard against that. If a primary probe comes back as e.g. 4 bytes instead of 16, the non-zero count is artificially deflated and the fallback could spuriously switch to the alternate flag on a chip that's actually responding correctly. Fix: validates.length === 16for each sample; abort the timing comparison (or treat the short read as a probe failure) if any sample is short.src/lib/drivers/sms4/sms4-driver.ts:567.Doc rot
handleJedecProbereferences "256-byte boundaries" but the actual offsets are0,0x80,0x100—0x80is 128, not 256. Reword.src/lib/drivers/sms4/sms4-driver.ts:547.src/lib/drivers/sms4/sms4-driver.ts:547.Scope
All four issues live inside the
if (chip.kind === "FLASH")fallback block. Not blocking — the fallback works on the test cart (verified: gameCode IRAO dumped cleanly,12/48non-zero on the alternate flag, save data validates by trainer-name + Pokémon-roster extraction). But the error-handling and short-read guards are real correctness fixes worth landing before another IR-cart variant hits an edge case neither of us anticipated.