feat(firmware): managed WINC serial-bridge protocol and read-only module inspector (part of #271) - #423
Conversation
|
/agentic_review |
PR Summary by QodoAdd managed WINC serial-bridge protocol + read-only module inspector
AI Description
Diagram
High-Level Assessment
Files changed (13)
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
… surface unreadable tool trees Qodo round 1 on #423. 1. Unused logger in WincFlashReader - removed the field and the ctor parameter. Qodo's stated mechanism was wrong: it claimed CS0414 under TreatWarningsAsErrors makes this a merge-blocking compile failure. It does not. A clean rebuild (obj/bin removed) reports 0 warnings, 0 errors, and a verbose build contains zero CS0414 occurrences - CS0414 does not fire for a field assigned a non-constant expression. The field was still genuinely dead, so it is gone on its own merit, not because the build was broken. 2. Cancellation and the uncancellable SerialPort.Open hang - the real one. Open() takes no token and can block indefinitely on a wedged or half-enumerated USB CDC device, so it now runs under a hard deadline (default 5s) and is abandoned on timeout or cancel. An abandoned open is still running on a pool thread and still owns the handle, so disposal is handed to a continuation instead of being done underneath it - disposing under a blocked open turns a hang into a crash. An ownership flag keeps the caller from double-disposing. Cancellation is also now observed between bridge exchanges and during the baud settle wait (was Thread.Sleep). 3. Flash address wrap - ReadFlash added a uint offset to an int and cast back to uint with no validation, so a large span wrapped past 32 bits and silently read the wrong part of flash, returning plausible data with no error. Now bounds-checked in 64-bit against the 24-bit address the fast-read command can actually carry. 4. Tool lookup masking IO errors - TryResolveToolPath no longer swallows IOException/UnauthorizedAccessException. A tree that exists but cannot be read was being reported as "could not locate the tool - WiFi flashing is Windows-only", which is actively misleading when the tool is sitting there behind a permissions problem. IsAvailable stays total (a probe answers yes/no) and catches for itself. No logging added in either catch, per #98. Five tests added, covering the open deadline (including that the abandoned open is not disposed underneath), cancellation during a hanging open, address-wrap rejection, the exact-last-address boundary, and the unreadable-tree split between TryResolveToolPath and IsAvailable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 1 addressed — 4 bugs, all fixed in 3e1b4aaThree accepted as stated. One had a wrong stated mechanism, which I want on the record rather than quietly agreed to. 1. Unused logger — fixed, but the premise was falseThe claim was that this "triggers CS0414" and is therefore "a merge-blocking compile failure" under CS0414 is scoped to fields assigned a constant; 2. Cancellation /
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 3e1b4aa |
…e genuinely total Qodo round 2 on #423. Both findings are the tail of round 1's own fixes. 1. Unobserved open task fault. The abandonment path added last round left the Task.Run(Open) with nobody watching it, so a later Open() failure was a silently swallowed background error - exactly what #377/#394 set out to eliminate. The continuation now reads completed.Exception (which marks it observed) and surfaces it at Debug. Debug rather than Warning because the caller already received a TimeoutException or a cancellation; this is diagnostic context, not a second failure to act on. Note this is not the logging-in-catch pattern rejected in #98 - it is a continuation observing a faulted task, and the log sits outside the dispose try/catch. 2. IsAvailable was documented "total by design" but only caught the IO family, so an ArgumentException from a malformed path could escape a probe whose whole purpose is to be safe to call with anything. Made the implementation match the contract rather than weakening the doc: the catch is now broad and the comment says why, and the returns tag spells out every case that yields false. Five tests added. The unobserved-fault test is mutation-verified: removing the completed.Exception read makes it fail, so it is not a no-op. It hooks TaskScheduler.UnobservedTaskException, forces a GC after the abandoned open has faulted, and also asserts the continuation still disposed the port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 2 addressed — both fixed in 616be5cBoth findings are the tail of my own round-1 fixes rather than new territory, which is the expected shape for a second round. Worth flagging for anyone reading later: the top-level summary reported Bugs (0) — these two exist only as inline threads. A summary-only check would have missed both. 1. Unobserved open task faultRound 1 added a hard deadline around The continuation now reads Mutation-verified, because unobserved-exception tests are easy to write as accidental no-ops: removing the 2.
|
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 616be5c |
…tead of via GC timing Qodo round 3 on #423 - and fairly, the finding is about the test I mutation-verified last round. Mutation proved it CAN catch the regression; it did not prove it catches it RELIABLY on a loaded CI machine, which is a different claim. Two independent problems with the old test: - Nondeterminism. It inferred "the fault was observed" from TaskScheduler.UnobservedTaskException after a forced GC. Collector and finalizer timing is not something to hang a CI gate on. - No isolation. UnobservedTaskException is process-global and the handler never called SetObserved, so an unrelated task faulting anywhere in a parallel run could reach this handler, and this one could leak to other subscribers. Replaced rather than patched. Reading Task.Exception is what marks a fault observed, so the abandonment path now reports the observed exception through an injectable hook and the test asserts on that directly - the same property, decided by the code under test rather than by the runtime. No GC, no global event, no SetObserved needed because nothing subscribes to the global event any more. The mutation property is preserved: removing the observation still fails the test. On success it now completes in ~150 ms instead of waiting out a forced collection, and it ran stable across four repeat runs. The hook is null in production, where the Debug log remains the only output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 3 addressed — fixed in f4c49e2One finding, and it was about the test I mutation-verified in round 2. Fair catch. What that verification actually proved. It showed the test can catch the regression. It said nothing about whether it catches it reliably under CI load — a different claim, and I conflated the two. Both halves of the finding were real and independent:
Replaced, not patched. Reading Evidence the rewrite kept its teeth:
The hook is null in production; the Debug log remains the only output there. Suite 4,875 green across net9.0 and net10.0. Clean rebuild: 0 warnings, 0 errors. No production behavior changed by this round — the only source change is the injectable observation point. Unchanged and still flagged: the bridge protocol has never run against a live bridge, and erase/program remains unimplemented. Bridge mode is a one-way door (no software exit; recovery needs a physical power cycle) and this bench is shared, so that run stays deferred until someone is at the hardware. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit f4c49e2 |
…nd drop the test-only hook Qodo round 4 on #423. A production hazard introduced by round 3's test seam. In the abandoned-open continuation the fault observation ran before an unprotected port.Dispose(), so a throwing observer or logger skipped the disposal entirely - leaking the serial handle and breaking every later open until process exit. That is strictly worse than the unobserved fault round 2 set out to fix, so the ordering here is load-bearing. Releasing the handle is the entire reason the continuation exists, so disposal now sits in a finally that nothing above can be reachable-past, with the observation in its own catch. The catch swallows rather than rethrows because nothing awaits this continuation either - propagating would recreate the unobserved fault the path exists to avoid. Also removed the injectable observer added in round 3. Two consecutive findings came from this continuation carrying a test-only seam, so it was worth re-weighing rather than patching again. It turns out the seam was never needed: production already reads completed.Exception to hand to the logger, and reading Task.Exception is precisely what marks a fault observed - so a capturing logger proves the same property through a seam that exists for production reasons. This repo already has CapturingLogger/ThrowingLogger precedent (DaqifiDeviceLoggerTests). Note the guard is required regardless of the hook: a throwing logger has the identical hazard, which is why ThrowingLogger exists in this repo at all. Removing the hook narrows the exposure; it does not replace the fix. Both properties are mutation-verified. Reverting to the unguarded ordering fails the new throwing-logger disposal test; removing the Exception read fails the observation test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 4 addressed — fixed in 84243a4One finding, and the sharpest of the four: a production hazard that my own round-3 test seam introduced. The chain is worth stating plainly. Round 2 fixed an unobserved fault. Round 3 replaced the flaky GC-based test with an injectable observer. Round 4 found that observer could skip the The fixDisposal now sits in a The test seam is goneTwo consecutive findings came from this continuation carrying a test-only hook, so I re-weighed it rather than patching again — and it was never needed. Production already reads To be precise: the guard is needed regardless of the hook — a throwing logger has the identical hazard, which is presumably why EvidenceBoth properties mutation-verified:
Net effect of this round on production code: the continuation is guarded, and one test-only member is gone from Suite 4,877 green across net9.0 and net10.0. Clean rebuild: 0 warnings, 0 errors. Unchanged and still flagged: the bridge protocol has never run against a live bridge, and erase/program remains unimplemented. Bridge mode is a one-way door — no software exit, recovery needs a physical power cycle — and this bench is shared, so that run stays deferred until someone is at the hardware. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 84243a4 |
…tops capturing `this` Qodo round 5 on #423. The abandoned-open continuation logged through the instance field _logger, so the closure captured `this`. In the scenario this code exists for - an open that never returns - the still-running task retained the whole inspector object graph for the life of the process rather than just the port and logger it needs for cleanup. Hoisting _logger into a local removes the last reach through the instance; `port` and `portName` were already parameters, so the continuation now captures only what it uses. Managed retention, not a handle leak - materially smaller than round 4's disposal hazard. Taken because it is a one-liner with no risk, not because it is serious. No test: asserting on closure capture is brittle and would cost more than it protects, and the existing tests already cover the behavior that matters (the fault is observed, the port is disposed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 5 addressed — fixed in 93a8b6eOne finding, and a one-line fix. The abandoned-open continuation logged through the instance field Hoisted Calibration, stated plainly: this is managed retention, not a handle leak — materially smaller than round 4's disposal hazard. Taken because it is a one-liner with no behavioral risk, not because it is serious. No test added. Asserting on closure capture would mean reflecting over compiler-generated display classes — brittle against codegen changes, and costing more than it protects. The existing tests cover the behavior that matters: the fault is observed, and the port is disposed even when observation throws. Suite 4,877 green across net9.0 and net10.0. Clean rebuild: 0 warnings, 0 errors. Where this PR standsFive rounds in, the findings have moved from real correctness bugs — an address wrap that silently returned wrong flash data, a handle leak that would break every later port open — to object-graph retention. That is the normal shape of convergence, and I think this is the natural stopping point. Unchanged and still flagged, because it is the thing that actually matters here: the bridge protocol has never run against a live bridge, and erase/program remains unimplemented. Bridge mode is a one-way door — no software exit, recovery needs a physical power cycle — and this bench is shared, so that run stays deferred until someone is at the hardware. Everything validated so far is the failure path plus 85 framing tests against a fake that re-implements the firmware's own parser. That is a real net, but it is not hardware validation. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 93a8b6e |
Bench run complete — the protocol is now demonstrated against a live WINCThe user was at the bench and approved the session. PR body updated; commit 722a43d. What was provedHandshake, baud negotiation to 500000, chip identity, and flash read-back all work against real hardware. Chip ID The bench found a bug 85 framing tests could not
Every frame was well-formed and the device ACKed every one. Nothing about the protocol layer was wrong; two register values were. This is structurally invisible to framing tests, because the fake faithfully answers exactly what the device answers. Fixed against the driver's This is the single best argument for having run it: five rounds of review hardened the code, and the first contact with hardware still found something none of it could reach. Correction: bridge mode is not a one-way doorI stated in earlier revisions — repeatedly, and confidently — that escaping bridge mode required a physical power cycle, on the reasoning that CDC is transparent while bridged. That was wrong. I had reasoned from the firmware's re-init path without checking Core's own deactivation path — which was in a file I had already opened. That belief also caused me to defer this run once. Worth recording plainly. Still not validatedErase and program remain unimplemented and unexercised. No write of any kind was performed and none is possible through this code. The user authorized a bridge session, not a flash write — that remains a separate decision. Suite 4,879 green across net9.0 and net10.0 (+2). Clean rebuild: 0 warnings, 0 errors. Device confirmed healthy and streaming after the session. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 722a43d |
…e, not a final-state map Qodo round 6 on #423. The test's central claim was false. ReadFlashJedecId_WritesTheExactRegisterSequenceTheWincDriverUses said "exact register sequence" in its name but collapsed the writes into a Dictionary<uint,uint> and asserted only the final value per address. That passes on a wrong ordering, and passes on a wrong value later overwritten by a correct one. For this command path order IS the protocol: DATA_CNT and DMA_ADDR stage the transfer and CMD_CNT triggers it, so a map cannot express the property the name promises - and the name is what a future reader trusts. Now asserted as an ordered list of (address, value) pairs. Extended the same guarantee to the fast-read path, which is the one that actually carries data and has the identical staging requirement. Register addresses are restated independently in the test from the WINC driver's spi_flash.c rather than reused from production, so a typo in the production map fails these tests instead of being silently agreed with. Mutation-verified both, using reorderings that leave every value correct: - JEDEC: firing CMD_CNT before DMA_ADDR turns it red. Notably the value-only test alongside it still passes, which is exactly the blind spot being closed. - Fast read: swapping BUF2 and BUF_DIR turns it red. Production code is untouched by this commit - test-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Qodo round 6 addressed — fixed in d2ff50dOne finding, on the very test that pins the hardware fix — and it was right. Not a style nit: the test's central claim was false.
For this command path order is the protocol — What changed
Mutation-verified, using reorderings that leave every value correct
The JEDEC run demonstrates the finding exactly: Production code untouched by this commit; test-only. Suite 4,881 green across net9.0 and net10.0. Clean rebuild: 0 warnings, 0 errors. Where #423 standsSix rounds, all findings resolved, and the bench evidence is in. The arc is worth recording: review caught real correctness bugs (a silent flash-address wrap, a handle leak), hardware caught a bug review structurally could not (two wrong register values inside perfectly well-formed, ACKed frames), and this final round caught a test that overstated what it verified. The scope boundary is unchanged and deliberate: erase and program remain unimplemented and unexercised. No write of any kind has been performed. Cross-platform WiFi flashing is still open — this PR is titled "part of #271" and does not close it. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit d2ff50d |
…ule inspector (part of #271) WiFi module flashing shells out to Microchip's winc_flash_tool, which is a Windows .cmd/.exe, so WiFi flashing is unavailable on Linux and macOS. This lands the foundation for a cross-platform path. It does NOT make WiFi flashing cross-platform - see "what is missing" below. The issue's premise turned out to be wrong. It says Microchip ships the winc_programmer_uart SOURCE in the ATWINC15x0 package; they do not. Both the Harmony repo (wireless_wifi/utilities/wifi/winc/tools) and the local winc1500-Manual-UART-Firmware-Update package ship binaries only - verified via the GitHub tree API, zero .c files for the programmer. DAQiFi's own firmware implements the bridge (wifi_serial_bridge.c), so the protocol here is derived from that instead, which is a better reference than anything Microchip publishes. What lands: - WincBridgeProtocol - wire format: op codes, the 12-byte XOR-checked header, and the mixed endianness (header fields little-endian, register replies big-endian) - WincSerialBridgeClient - one complete exchange per method - WincFlashReader - read-only flash/identity register sequences - SystemWincSerialPort - System.IO.Ports, so the transport is cross-platform - WincModuleInspector - handshake, baud negotiation, chip/flash identity, flash read-back. It inspects; it does not flash. - WincFlashToolLocator - "can this machine flash?" answered up front What is missing: erase and program. That path could not be validated - a wrong opcode or address bricks the module with no recovery outside Microchip's Windows tool - so it is absent rather than shipped unexercised behind an API that looks complete. Found a firmware defect while deriving the protocol: the bridge's READ_BLOCK loop never decrements its counter nor advances the address, so any block read of >= 2048 bytes loops forever re-sending the same chunk. MaxReadBlockSize is capped at 2047 with tests pinning both the rejection and the chunking. Also: the 500000-baud step is ceremonial over USB CDC, which ignores line rate. Documented on the constant so nobody expects a speedup from it. 75 new tests, most of them protocol framing, against a fake that re-implements the firmware's parser so mis-framed commands are rejected exactly as the device would reject them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… surface unreadable tool trees Qodo round 1 on #423. 1. Unused logger in WincFlashReader - removed the field and the ctor parameter. Qodo's stated mechanism was wrong: it claimed CS0414 under TreatWarningsAsErrors makes this a merge-blocking compile failure. It does not. A clean rebuild (obj/bin removed) reports 0 warnings, 0 errors, and a verbose build contains zero CS0414 occurrences - CS0414 does not fire for a field assigned a non-constant expression. The field was still genuinely dead, so it is gone on its own merit, not because the build was broken. 2. Cancellation and the uncancellable SerialPort.Open hang - the real one. Open() takes no token and can block indefinitely on a wedged or half-enumerated USB CDC device, so it now runs under a hard deadline (default 5s) and is abandoned on timeout or cancel. An abandoned open is still running on a pool thread and still owns the handle, so disposal is handed to a continuation instead of being done underneath it - disposing under a blocked open turns a hang into a crash. An ownership flag keeps the caller from double-disposing. Cancellation is also now observed between bridge exchanges and during the baud settle wait (was Thread.Sleep). 3. Flash address wrap - ReadFlash added a uint offset to an int and cast back to uint with no validation, so a large span wrapped past 32 bits and silently read the wrong part of flash, returning plausible data with no error. Now bounds-checked in 64-bit against the 24-bit address the fast-read command can actually carry. 4. Tool lookup masking IO errors - TryResolveToolPath no longer swallows IOException/UnauthorizedAccessException. A tree that exists but cannot be read was being reported as "could not locate the tool - WiFi flashing is Windows-only", which is actively misleading when the tool is sitting there behind a permissions problem. IsAvailable stays total (a probe answers yes/no) and catches for itself. No logging added in either catch, per #98. Five tests added, covering the open deadline (including that the abandoned open is not disposed underneath), cancellation during a hanging open, address-wrap rejection, the exact-last-address boundary, and the unreadable-tree split between TryResolveToolPath and IsAvailable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e genuinely total Qodo round 2 on #423. Both findings are the tail of round 1's own fixes. 1. Unobserved open task fault. The abandonment path added last round left the Task.Run(Open) with nobody watching it, so a later Open() failure was a silently swallowed background error - exactly what #377/#394 set out to eliminate. The continuation now reads completed.Exception (which marks it observed) and surfaces it at Debug. Debug rather than Warning because the caller already received a TimeoutException or a cancellation; this is diagnostic context, not a second failure to act on. Note this is not the logging-in-catch pattern rejected in #98 - it is a continuation observing a faulted task, and the log sits outside the dispose try/catch. 2. IsAvailable was documented "total by design" but only caught the IO family, so an ArgumentException from a malformed path could escape a probe whose whole purpose is to be safe to call with anything. Made the implementation match the contract rather than weakening the doc: the catch is now broad and the comment says why, and the returns tag spells out every case that yields false. Five tests added. The unobserved-fault test is mutation-verified: removing the completed.Exception read makes it fail, so it is not a no-op. It hooks TaskScheduler.UnobservedTaskException, forces a GC after the abandoned open has faulted, and also asserts the continuation still disposed the port. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tead of via GC timing Qodo round 3 on #423 - and fairly, the finding is about the test I mutation-verified last round. Mutation proved it CAN catch the regression; it did not prove it catches it RELIABLY on a loaded CI machine, which is a different claim. Two independent problems with the old test: - Nondeterminism. It inferred "the fault was observed" from TaskScheduler.UnobservedTaskException after a forced GC. Collector and finalizer timing is not something to hang a CI gate on. - No isolation. UnobservedTaskException is process-global and the handler never called SetObserved, so an unrelated task faulting anywhere in a parallel run could reach this handler, and this one could leak to other subscribers. Replaced rather than patched. Reading Task.Exception is what marks a fault observed, so the abandonment path now reports the observed exception through an injectable hook and the test asserts on that directly - the same property, decided by the code under test rather than by the runtime. No GC, no global event, no SetObserved needed because nothing subscribes to the global event any more. The mutation property is preserved: removing the observation still fails the test. On success it now completes in ~150 ms instead of waiting out a forced collection, and it ran stable across four repeat runs. The hook is null in production, where the Debug log remains the only output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nd drop the test-only hook Qodo round 4 on #423. A production hazard introduced by round 3's test seam. In the abandoned-open continuation the fault observation ran before an unprotected port.Dispose(), so a throwing observer or logger skipped the disposal entirely - leaking the serial handle and breaking every later open until process exit. That is strictly worse than the unobserved fault round 2 set out to fix, so the ordering here is load-bearing. Releasing the handle is the entire reason the continuation exists, so disposal now sits in a finally that nothing above can be reachable-past, with the observation in its own catch. The catch swallows rather than rethrows because nothing awaits this continuation either - propagating would recreate the unobserved fault the path exists to avoid. Also removed the injectable observer added in round 3. Two consecutive findings came from this continuation carrying a test-only seam, so it was worth re-weighing rather than patching again. It turns out the seam was never needed: production already reads completed.Exception to hand to the logger, and reading Task.Exception is precisely what marks a fault observed - so a capturing logger proves the same property through a seam that exists for production reasons. This repo already has CapturingLogger/ThrowingLogger precedent (DaqifiDeviceLoggerTests). Note the guard is required regardless of the hook: a throwing logger has the identical hazard, which is why ThrowingLogger exists in this repo at all. Removing the hook narrows the exposure; it does not replace the fix. Both properties are mutation-verified. Reverting to the unguarded ordering fails the new throwing-logger disposal test; removing the Exception read fails the observation test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tops capturing `this` Qodo round 5 on #423. The abandoned-open continuation logged through the instance field _logger, so the closure captured `this`. In the scenario this code exists for - an open that never returns - the still-running task retained the whole inspector object graph for the life of the process rather than just the port and logger it needs for cleanup. Hoisting _logger into a local removes the last reach through the instance; `port` and `portName` were already parameters, so the continuation now captures only what it uses. Managed retention, not a handle leak - materially smaller than round 4's disposal hazard. Taken because it is a one-liner with no risk, not because it is serious. No test: asserting on closure capture is brittle and would cost more than it protects, and the existing tests already cover the behavior that matters (the fault is observed, the port is disposed). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ive bridge run The bench session found a real bug that 85 framing tests could not. ReadFlashJedecId wrote DATA_CNT 0 and DMA_ADDR 0. Against a live WINC the command still completed and TR_DONE still went high, but no result bytes were clocked back, so the read returned 0x00000000. Every frame was well-formed and the device ACKed every one - nothing about the protocol layer was wrong, only two register values, which is precisely the class of defect framing tests cannot see. Corrected against the WINC driver's spi_flash_rdid: DATA_CNT is the number of result bytes (4) and DMA_ADDR is where they land (DUMMY_REGISTER). Verified on hardware: 0x00000000 before, 0xC21320C2 after - Macronix (0xC2), type 0x20, capacity 0x13 = 4 Mbit / 512 KB, which matches the 4 Mb WINC SPI flash the issue describes. Adds a test pinning the register sequence value-by-value against the firmware, rather than only asserting the return, so this class of bug is caught in CI from now on. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e, not a final-state map Qodo round 6 on #423. The test's central claim was false. ReadFlashJedecId_WritesTheExactRegisterSequenceTheWincDriverUses said "exact register sequence" in its name but collapsed the writes into a Dictionary<uint,uint> and asserted only the final value per address. That passes on a wrong ordering, and passes on a wrong value later overwritten by a correct one. For this command path order IS the protocol: DATA_CNT and DMA_ADDR stage the transfer and CMD_CNT triggers it, so a map cannot express the property the name promises - and the name is what a future reader trusts. Now asserted as an ordered list of (address, value) pairs. Extended the same guarantee to the fast-read path, which is the one that actually carries data and has the identical staging requirement. Register addresses are restated independently in the test from the WINC driver's spi_flash.c rather than reused from production, so a typo in the production map fails these tests instead of being silently agreed with. Mutation-verified both, using reorderings that leave every value correct: - JEDEC: firing CMD_CNT before DMA_ADDR turns it red. Notably the value-only test alongside it still passes, which is exactly the blind spot being closed. - Fast read: swapping BUF2 and BUF_DIR turns it red. Production code is untouched by this commit - test-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
d2ff50d to
d2a719e
Compare
1 participant
Why
WiFi module flashing shells out to Microchip's
winc_flash_tool, which is a Windows.cmd/.exe. On Linux and macOS there is nothing to run, so WiFi flashing simply isn't available — a problem for the desktop app's move to Avalonia.This PR does not fix that. It lands the foundation, and I want to be blunt about the gap up front: WiFi flashing is still Windows-only after this merges.
What actually happened to the plan
The issue says Microchip ships the
winc_programmer_uartsource in the ATWINC15x0 package. They don't. The Harmony repo (wireless_wifi/utilities/wifi/winc/tools) and the localwinc1500-Manual-UART-Firmware-Updatepackage both ship binaries only —winc_programmer_uart(Linux ELF),winc_programmer_uart.exe,image_tool. I checked the full repo tree through the GitHub API: zero.cfiles for the programmer. So the "port the open-source tool" route in the issue isn't available.The good news is there's a better reference. DAQiFi's own firmware implements the bridge (
wifi_serial_bridge.c), so the protocol here is derived from the code that actually accepts or rejects these commands, not from a third party's docs.What lands
WincBridgeProtocolWincSerialBridgeClientWincFlashReaderSystemWincSerialPortSystem.IO.Ports, so the transport itself is cross-platformWincModuleInspectorWincFlashToolLocatorOne subtlety worth flagging: the header fields are little-endian but a register value comes back big-endian. That asymmetry is the easiest thing to get wrong, and there are tests pinning both.
What is missing, and why
Erase and program. Writing a WINC means erase and page-program cycles against its SPI flash, and a wrong opcode or address bricks the module with no recovery path outside Microchip's Windows tool. I could not validate that on the bench (see below), so it is absent rather than shipped unexercised. An API that looks complete but has never run is worse than no API: callers select it, and the first execution happens on someone's hardware.
I also dropped the
IWincFlasherseam the issue suggests. I built it, then deleted it. Its centralFlashAsyncwas thrown from by both implementations — the native one can't write, and the external tool's run is owned by the WiFi flow (whose prompt handshake and output-based success verification are hard-won and Windows-only, so they can't be moved and re-validated). A two-property interface with one implementation and no consumer is noise. It comes back when there is a native writer and genuinely two things to abstract. The same reasoning renamedNativeWincFlasher→WincModuleInspector: it inspects, it doesn't flash, and the name should say so.Two findings worth carrying out of this work
1. A firmware hang. The bridge's
READ_BLOCKhandler never decrements its counter nor advances the address:Any block read of ≥2048 bytes loops forever, re-sending the same chunk.
MaxReadBlockSizeis capped at 2047 with the reason written down, and tests assert both that oversize requests are rejected before hitting the wire and that a 5 KB flash read never issues a single oversize block. Being surfaced separately as a firmware ticket.2. The 500000-baud step is ceremonial over USB CDC, which ignores line rate entirely. The exchange still has to happen — the bridge expects it, and completing it proves the command path works — but nobody should expect a throughput gain from it. Documented on the constant rather than left misleading.
Bench evidence
Nyquist 1, FW 3.7.2,
/dev/cu.usbmodem1101, over USB CDC. The device was put into real bridge mode and the protocol was exercised against a live WINC.What this proves. The protocol derived from
wifi_serial_bridge.cis correct on real hardware:0x001003A0— byte-for-byte the value the firmware source records as bench-observed. Family0x10= halted in download mode, exactly as expected afterm2m_wifi_download_mode(). This validates the big-endian register decode, which is the asymmetry most likely to be wrong.NMIS, the WINC image signature. Real content, not zeros or noise. Re-reading is byte-identical (deterministic), and a different offset returns different data (the address field genuinely steers the read).0xC21320C2— Macronix (0xC2), type0x20, capacity0x13= 4 Mbit / 512 KB, matching the 4 Mb WINC SPI flash this issue describes.The bench found a bug the tests could not
ReadFlashJedecIdoriginally returned0x00000000from the live module. It wroteDATA_CNT = 0andDMA_ADDR = 0, so no result bytes were clocked back — while the command still completed andTR_DONEstill went high. Every frame was well-formed and the device ACKed every one. Nothing about the protocol layer was wrong; two register values were. No amount of framing testing against a fake could have caught that, because the fake faithfully answered exactly what the device answered.Corrected against the driver's
spi_flash_rdid(DATA_CNT = 4,DMA_ADDR = DUMMY_REGISTER) and re-verified on hardware. A test now pins the register sequence value-by-value rather than only asserting the return.Correction: bridge mode is not a one-way door
Earlier revisions of this PR stated that entering bridge mode required a physical power cycle to escape, because the CDC is transparent while bridged. That was wrong.
WifiBridgeActivator.Deactivate—SetUsbTransparencyMode(0)+LAN:APPLY, already in Core — restored SCPI cleanly, twice. The device was left healthy and streaming, never stranded. I had reasoned from the firmware's re-init path without checking Core's own documented deactivation path, which was in a file I had already opened.Still NOT validated
Erase and program remain unimplemented and unexercised — no write of any kind was performed, and none is possible through this code. That is a deliberate scope boundary, not an oversight: the user authorized a bridge session, not a flash write. Everything above is read-only.
Test suite
4,829 green across net9.0 and net10.0 (+75). Build clean, 0 warnings,
TreatWarningsAsErrorson.🤖 Generated with Claude Code