From 6a193251feaad263096f40ebe089936f4280f3cd Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 12:57:30 -0600 Subject: [PATCH 1/8] feat(firmware): managed WINC serial-bridge protocol and read-only module 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 --- .../Firmware/Winc/FakeWincSerialPort.cs | 205 ++++++++++++ .../Firmware/Winc/WincBridgeProtocolTests.cs | 192 +++++++++++ .../Firmware/Winc/WincFlasherTests.cs | 281 ++++++++++++++++ .../Winc/WincSerialBridgeClientTests.cs | 304 ++++++++++++++++++ src/Daqifi.Core/Firmware/WifiModuleUpdater.cs | 35 +- .../Firmware/Winc/IWincSerialPort.cs | 42 +++ .../Firmware/Winc/SystemWincSerialPort.cs | 122 +++++++ .../Firmware/Winc/WincBridgeProtocol.cs | 183 +++++++++++ .../Firmware/Winc/WincFlashReader.cs | 169 ++++++++++ .../Firmware/Winc/WincFlashToolLocator.cs | 80 +++++ .../Firmware/Winc/WincModuleIdentity.cs | 26 ++ .../Firmware/Winc/WincModuleInspector.cs | 171 ++++++++++ .../Firmware/Winc/WincSerialBridgeClient.cs | 197 ++++++++++++ 13 files changed, 1991 insertions(+), 16 deletions(-) create mode 100644 src/Daqifi.Core.Tests/Firmware/Winc/FakeWincSerialPort.cs create mode 100644 src/Daqifi.Core.Tests/Firmware/Winc/WincBridgeProtocolTests.cs create mode 100644 src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs create mode 100644 src/Daqifi.Core.Tests/Firmware/Winc/WincSerialBridgeClientTests.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/IWincSerialPort.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/SystemWincSerialPort.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/WincBridgeProtocol.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/WincModuleIdentity.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs create mode 100644 src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/FakeWincSerialPort.cs b/src/Daqifi.Core.Tests/Firmware/Winc/FakeWincSerialPort.cs new file mode 100644 index 0000000..87474d2 --- /dev/null +++ b/src/Daqifi.Core.Tests/Firmware/Winc/FakeWincSerialPort.cs @@ -0,0 +1,205 @@ +using Daqifi.Core.Firmware.Winc; + +namespace Daqifi.Core.Tests.Firmware.Winc; + +/// +/// A fake serial port that emulates the DAQiFi firmware's WINC bridge state machine, so the client +/// can be exercised end-to-end without hardware. +/// +/// +/// This deliberately re-implements the firmware's parser (op-code wait, 12-byte header, XOR +/// validation, ACK/NACK, payload wait) rather than replaying canned bytes. That way a client that +/// frames a command incorrectly gets rejected here exactly as the device would reject it — the +/// point is to catch framing bugs the bench cannot. +/// +internal sealed class FakeWincSerialPort : IWincSerialPort +{ + private readonly Queue _toHost = new(); + private readonly List _fromHost = []; + + private State _state = State.WaitOpCode; + private byte[] _header = []; + private int _pendingPayload; + + /// Register file the emulated WINC answers reads from. + internal Dictionary Registers { get; } = []; + + /// Memory the emulated WINC answers block reads from. + internal Dictionary Blocks { get; } = []; + + /// Every command header the host sent, in order. + internal List ReceivedHeaders { get; } = []; + + /// Every block-write payload the host sent, in order. + internal List ReceivedPayloads { get; } = []; + + /// Baud rates the host applied, in order. + internal List BaudRateHistory { get; } = []; + + /// When true, the emulated bridge NACKs the next block write. + internal bool FailNextBlockWrite { get; set; } + + /// When true, the bridge does not answer the identify op code. + internal bool SuppressIdentityResponse { get; set; } + + /// Number of times the host discarded its input buffer. + internal int DiscardCount { get; private set; } + + internal bool WasDisposed { get; private set; } + + public bool IsOpen { get; private set; } + + private int _baudRate = 115200; + + public int BaudRate + { + get => _baudRate; + set + { + _baudRate = value; + BaudRateHistory.Add(value); + } + } + + public void Open() => IsOpen = true; + + public void Close() => IsOpen = false; + + public void DiscardInBuffer() + { + DiscardCount++; + _toHost.Clear(); + } + + public void Write(byte[] buffer, int offset, int count) + { + for (var i = 0; i < count; i++) + { + Feed(buffer[offset + i]); + } + } + + public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) + { + if (_toHost.Count < count) + { + throw new TimeoutException( + $"Fake port has {_toHost.Count} byte(s) buffered but {count} were requested."); + } + + for (var i = 0; i < count; i++) + { + buffer[offset + i] = _toHost.Dequeue(); + } + } + + public void Dispose() + { + WasDisposed = true; + IsOpen = false; + } + + /// Drives the emulated bridge one received byte at a time. + private void Feed(byte b) + { + _fromHost.Add(b); + + switch (_state) + { + case State.WaitOpCode: + if (b == WincBridgeProtocol.IdentifyVariableBaud) + { + if (!SuppressIdentityResponse) + { + _toHost.Enqueue(WincBridgeProtocol.Response.IdVariableBaud); + } + } + else if (b == WincBridgeProtocol.StartCommand) + { + _state = State.WaitHeader; + _header = []; + } + // Any other op code is ignored, exactly as the firmware does. + break; + + case State.WaitHeader: + _header = [.. _header, b]; + if (_header.Length == WincBridgeProtocol.HeaderSize) + { + ReceivedHeaders.Add(_header); + HandleHeader(); + } + break; + + case State.WaitPayload: + _header = [.. _header, b]; + if (_header.Length == _pendingPayload) + { + ReceivedPayloads.Add(_header); + _toHost.Enqueue( + FailNextBlockWrite + ? WincBridgeProtocol.Response.Nack + : WincBridgeProtocol.Response.Ack); + FailNextBlockWrite = false; + _state = State.WaitOpCode; + } + break; + } + } + + private void HandleHeader() + { + if (!WincBridgeProtocol.IsHeaderValid(_header)) + { + _toHost.Enqueue(WincBridgeProtocol.Response.Nack); + _state = State.WaitOpCode; + return; + } + + _toHost.Enqueue(WincBridgeProtocol.Response.Ack); + + var command = (WincBridgeProtocol.Command)_header[0]; + var size = (ushort)((_header[3] << 8) | _header[2]); + var address = ((uint)_header[7] << 24) | ((uint)_header[6] << 16) | ((uint)_header[5] << 8) | _header[4]; + + switch (command) + { + case WincBridgeProtocol.Command.ReadRegisterWithReturn: + var value = Registers.TryGetValue(address, out var v) ? v : 0u; + // Big-endian, matching the firmware. + _toHost.Enqueue((byte)(value >> 24)); + _toHost.Enqueue((byte)(value >> 16)); + _toHost.Enqueue((byte)(value >> 8)); + _toHost.Enqueue((byte)value); + _state = State.WaitOpCode; + break; + + case WincBridgeProtocol.Command.ReadBlock: + var block = Blocks.TryGetValue(address, out var data) ? data : new byte[size]; + for (var i = 0; i < size; i++) + { + _toHost.Enqueue(i < block.Length ? block[i] : (byte)0); + } + _state = State.WaitOpCode; + break; + + case WincBridgeProtocol.Command.WriteBlock: + _pendingPayload = size; + _header = []; + _state = State.WaitPayload; + break; + + default: + // WriteRegister and Reconfigure produce no data beyond the header ACK. + _state = State.WaitOpCode; + break; + } + } + + private enum State + { + WaitOpCode, + WaitHeader, + WaitPayload + } +} diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincBridgeProtocolTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincBridgeProtocolTests.cs new file mode 100644 index 0000000..6a9109b --- /dev/null +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincBridgeProtocolTests.cs @@ -0,0 +1,192 @@ +using Daqifi.Core.Firmware.Winc; + +namespace Daqifi.Core.Tests.Firmware.Winc; + +/// +/// Framing tests for the WINC serial bridge wire format. +/// +/// +/// These are the primary correctness net for the native flasher: the erase/program path cannot be +/// exercised on the bench, so the framing has to be right by construction. Expected byte sequences +/// are derived from the DAQiFi firmware's bridge parser +/// (firmware/src/services/wifi_services/wifi_serial_bridge.c), which is the code that +/// actually accepts or rejects these headers. +/// +public class WincBridgeProtocolTests +{ + [Fact] + public void BuildHeader_ProducesTwelveBytesWhoseXorIsZero() + { + // The firmware XORs all 12 bytes and requires 0 before it will ACK. + var header = WincBridgeProtocol.BuildHeader( + WincBridgeProtocol.Command.ReadBlock, 0x0100, 0xD0000, 0); + + Assert.Equal(WincBridgeProtocol.HeaderSize, header.Length); + + byte xor = 0; + foreach (var b in header) + { + xor ^= b; + } + + Assert.Equal(0, xor); + Assert.True(WincBridgeProtocol.IsHeaderValid(header)); + } + + [Theory] + [InlineData((byte)0, (ushort)0, 0x1000u, 0u)] // ReadRegisterWithReturn + [InlineData((byte)1, (ushort)0, 0x10208u, 0x2Au)] // WriteRegister + [InlineData((byte)2, (ushort)2047, 0xD0000u, 0u)] // ReadBlock, max safe size + [InlineData((byte)3, (ushort)256, 0xD0000u, 0u)] // WriteBlock, one flash page + [InlineData((byte)5, (ushort)0, 0u, 500000u)] // Reconfigure to the fast baud + [InlineData((byte)1, (ushort)0xFFFF, 0xFFFFFFFFu, 0xFFFFFFFFu)] // all-ones saturation + public void BuildHeader_IsAlwaysAcceptedByTheFirmwareChecksumRule( + byte command, ushort size, uint address, uint value) + { + var header = WincBridgeProtocol.BuildHeader( + (WincBridgeProtocol.Command)command, size, address, value); + + Assert.True(WincBridgeProtocol.IsHeaderValid(header)); + } + + [Fact] + public void BuildHeader_LaysOutCommandSizeAddressAndValueLittleEndian() + { + // Byte-for-byte against the firmware's ProcessHeader field extraction: + // size = [3]<<8 | [2]; addr = [7]<<24 | [6]<<16 | [5]<<8 | [4]; val likewise from [8..11]. + var header = WincBridgeProtocol.BuildHeader( + WincBridgeProtocol.Command.WriteRegister, + size: 0x1234, + address: 0xAABBCCDD, + value: 0x11223344); + + Assert.Equal((byte)WincBridgeProtocol.Command.WriteRegister, header[0]); + + Assert.Equal(0x34, header[2]); + Assert.Equal(0x12, header[3]); + + Assert.Equal(0xDD, header[4]); + Assert.Equal(0xCC, header[5]); + Assert.Equal(0xBB, header[6]); + Assert.Equal(0xAA, header[7]); + + Assert.Equal(0x44, header[8]); + Assert.Equal(0x33, header[9]); + Assert.Equal(0x22, header[10]); + Assert.Equal(0x11, header[11]); + } + + [Fact] + public void BuildHeader_ReconfigurePutsTheBaudRateInTheValueField() + { + // The baud change is the one command whose payload is the value field alone. + var header = WincBridgeProtocol.BuildHeader( + WincBridgeProtocol.Command.Reconfigure, 0, 0, 500000); + + Assert.Equal(0x05, header[0]); + Assert.Equal(0x20, header[8]); + Assert.Equal(0xA1, header[9]); + Assert.Equal(0x07, header[10]); + Assert.Equal(0x00, header[11]); + Assert.True(WincBridgeProtocol.IsHeaderValid(header)); + } + + [Fact] + public void IsHeaderValid_RejectsASingleFlippedBit() + { + var header = WincBridgeProtocol.BuildHeader( + WincBridgeProtocol.Command.ReadBlock, 512, 0xD0000, 0); + + header[6] ^= 0x01; + + Assert.False(WincBridgeProtocol.IsHeaderValid(header)); + } + + [Theory] + [InlineData(0)] + [InlineData(11)] + [InlineData(13)] + public void IsHeaderValid_RejectsWrongLength(int length) + { + Assert.False(WincBridgeProtocol.IsHeaderValid(new byte[length])); + } + + [Fact] + public void IsHeaderValid_RejectsNull() + { + Assert.False(WincBridgeProtocol.IsHeaderValid(null!)); + } + + [Fact] + public void ComputeChecksum_IgnoresTheExistingChecksumSlot() + { + // Byte 1 must not feed its own computation, or rebuilding a header would drift. + var header = WincBridgeProtocol.BuildHeader( + WincBridgeProtocol.Command.ReadBlock, 128, 0x1000, 0); + + var recomputed = WincBridgeProtocol.ComputeChecksum(header); + + Assert.Equal(header[1], recomputed); + } + + [Fact] + public void ComputeChecksum_RejectsWrongLengthHeaders() + { + Assert.Throws(() => WincBridgeProtocol.ComputeChecksum(new byte[5])); + } + + [Fact] + public void DecodeRegisterValue_ReadsBigEndian_OppositeOfTheHeaderFields() + { + // The firmware writes the register value MSB-first, unlike the little-endian header — + // getting this backwards is the single easiest way to misread every register. + var value = WincBridgeProtocol.DecodeRegisterValue([0x00, 0x10, 0x03, 0xA0]); + + Assert.Equal(0x001003A0u, value); + } + + [Fact] + public void DecodeRegisterValue_HandlesTheFullRange() + { + Assert.Equal(0xFFFFFFFFu, WincBridgeProtocol.DecodeRegisterValue([0xFF, 0xFF, 0xFF, 0xFF])); + Assert.Equal(0u, WincBridgeProtocol.DecodeRegisterValue([0x00, 0x00, 0x00, 0x00])); + } + + [Theory] + [InlineData(0)] + [InlineData(3)] + [InlineData(5)] + public void DecodeRegisterValue_RejectsWrongLength(int length) + { + Assert.Throws(() => WincBridgeProtocol.DecodeRegisterValue(new byte[length])); + } + + [Fact] + public void OpCodesAndResponses_MatchTheFirmwareConstants() + { + // Pinned against wifi_serial_bridge.c; a silent drift here would break every exchange. + Assert.Equal(0x12, WincBridgeProtocol.IdentifyVariableBaud); + Assert.Equal(0x13, WincBridgeProtocol.IdentifyFixedBaud); + Assert.Equal(0xA5, WincBridgeProtocol.StartCommand); + + Assert.Equal(0x5A, WincBridgeProtocol.Response.Nack); + Assert.Equal(0x5B, WincBridgeProtocol.Response.IdVariableBaud); + Assert.Equal(0x5C, WincBridgeProtocol.Response.IdFixedBaud); + Assert.Equal(0xAC, WincBridgeProtocol.Response.Ack); + + Assert.Equal(0, (byte)WincBridgeProtocol.Command.ReadRegisterWithReturn); + Assert.Equal(1, (byte)WincBridgeProtocol.Command.WriteRegister); + Assert.Equal(2, (byte)WincBridgeProtocol.Command.ReadBlock); + Assert.Equal(3, (byte)WincBridgeProtocol.Command.WriteBlock); + Assert.Equal(5, (byte)WincBridgeProtocol.Command.Reconfigure); + } + + [Fact] + public void MaxReadBlockSize_StaysBelowTheFirmwareCommandBuffer() + { + // The device's read loop never terminates at or above its 2048-byte buffer, so 2047 is a + // hard ceiling rather than a tuning choice. + Assert.Equal(2047, WincBridgeProtocol.MaxReadBlockSize); + Assert.True(WincBridgeProtocol.MaxReadBlockSize < 2048); + } +} diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs new file mode 100644 index 0000000..9d43197 --- /dev/null +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -0,0 +1,281 @@ +using Daqifi.Core.Firmware.Winc; + +namespace Daqifi.Core.Tests.Firmware.Winc; + +/// +/// Covers the flash-level read sequences and the two implementations. +/// +public class WincFlasherTests +{ + private const uint TransferDoneRegister = 0x10218; + private const uint DummyRegister = 0x1084; + private const uint ShareMemoryBase = 0xD0000; + + /// + /// Builds a fake whose transfer-done register always reads 1, so flash sequences complete. + /// + private static FakeWincSerialPort CreateReadyPort() + { + var port = new FakeWincSerialPort(); + port.Registers[TransferDoneRegister] = 1; + return port; + } + + private static WincFlashReader CreateReader(FakeWincSerialPort port) + => new(new WincSerialBridgeClient(port, TimeSpan.FromSeconds(1))); + + [Theory] + [InlineData(0x001003A0u, true)] // bench-observed: halted in download mode + [InlineData(0x00150000u, true)] // firmware running + [InlineData(0x0015FFFFu, true)] + [InlineData(0x00000000u, false)] // nothing there + [InlineData(0xFFFFFFFFu, false)] // floating bus + [InlineData(0x00200000u, false)] // some other part + public void IsKnownWincChipId_RecognizesOnlyTheWinc1500Families(uint chipId, bool expected) + { + Assert.Equal(expected, WincFlashReader.IsKnownWincChipId(chipId)); + } + + [Fact] + public void ReadChipId_ReadsTheIdentityRegister() + { + var port = CreateReadyPort(); + port.Registers[WincFlashReader.ChipIdRegister] = 0x001003A0; + + Assert.Equal(0x001003A0u, CreateReader(port).ReadChipId()); + } + + [Fact] + public void ReadFlash_ChunksBelowTheSizeThatWedgesTheDevice() + { + // A 5 KB read must never issue a single block request at or above 2048 bytes, because the + // device's read loop would never terminate. This is the property that matters most here. + var port = CreateReadyPort(); + port.Blocks[ShareMemoryBase] = new byte[WincBridgeProtocol.MaxReadBlockSize]; + + CreateReader(port).ReadFlash(0, 5000); + + var blockReadSizes = port.ReceivedHeaders + .Where(h => h[0] == (byte)WincBridgeProtocol.Command.ReadBlock) + .Select(h => (h[3] << 8) | h[2]) + .ToList(); + + Assert.NotEmpty(blockReadSizes); + Assert.All(blockReadSizes, size => Assert.True(size <= WincBridgeProtocol.MaxReadBlockSize)); + } + + [Fact] + public void ReadFlash_ReturnsExactlyTheRequestedLength() + { + var port = CreateReadyPort(); + port.Blocks[ShareMemoryBase] = Enumerable.Range(0, WincBridgeProtocol.MaxReadBlockSize) + .Select(i => (byte)(i & 0xFF)) + .ToArray(); + + var data = CreateReader(port).ReadFlash(0, 3000); + + Assert.Equal(3000, data.Length); + } + + [Fact] + public void ReadFlash_IssuesTheFastReadCommandWithTheAddressInTheControllerWord() + { + // Mirrors the WINC host driver's load-to-shared-memory sequence: opcode 0x0B in the low + // byte, then the 24-bit flash address ascending. + var port = CreateReadyPort(); + port.Blocks[ShareMemoryBase] = new byte[16]; + + CreateReader(port).ReadFlash(0x123456, 16); + + var buffer1Write = port.ReceivedHeaders.First(h => + h[0] == (byte)WincBridgeProtocol.Command.WriteRegister && + (((uint)h[7] << 24) | ((uint)h[6] << 16) | ((uint)h[5] << 8) | h[4]) == 0x1020C); + + var commandWord = ((uint)buffer1Write[11] << 24) | ((uint)buffer1Write[10] << 16) + | ((uint)buffer1Write[9] << 8) | buffer1Write[8]; + + Assert.Equal(0x0Bu, commandWord & 0xFF); + Assert.Equal(0x12u, (commandWord >> 8) & 0xFF); + Assert.Equal(0x34u, (commandWord >> 16) & 0xFF); + Assert.Equal(0x56u, (commandWord >> 24) & 0xFF); + } + + [Fact] + public void ReadFlashJedecId_ReturnsTheControllerResult() + { + var port = CreateReadyPort(); + port.Registers[DummyRegister] = 0x00C22018; + + Assert.Equal(0x00C22018u, CreateReader(port).ReadFlashJedecId()); + } + + [Fact] + public void ReadFlash_ThrowsWhenTheControllerNeverReportsDone() + { + // The firmware's own poll loop is unbounded; ours must not be, or a WINC that stops + // answering would hang the host indefinitely. + var port = new FakeWincSerialPort(); + port.Registers[TransferDoneRegister] = 0; + + var reader = new WincFlashReader( + new WincSerialBridgeClient(port, TimeSpan.FromSeconds(1)), transferPollLimit: 3); + + var ex = Assert.Throws(() => reader.ReadFlash(0, 16)); + + Assert.Contains("transfer-done", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void ReadFlash_RejectsNonPositiveLengths(int length) + { + Assert.Throws(() => CreateReader(CreateReadyPort()).ReadFlash(0, length)); + } + + [Fact] + public void ReadFlash_ObservesCancellation() + { + var port = CreateReadyPort(); + port.Blocks[ShareMemoryBase] = new byte[WincBridgeProtocol.MaxReadBlockSize]; + using var cts = new CancellationTokenSource(); + cts.Cancel(); + + Assert.Throws( + () => CreateReader(port).ReadFlash(0, 5000, cts.Token)); + } + + // ---- WincModuleInspector ------------------------------------------------- + + [Fact] + public async Task Inspector_ReadIdentityAsync_HandshakesNegotiatesBaudThenReadsIdentity() + { + var port = CreateReadyPort(); + port.Registers[WincFlashReader.ChipIdRegister] = 0x001003A0; + port.Registers[DummyRegister] = 0x00C22018; + + var flasher = new WincModuleInspector((_, _) => port, baudSettleDelay: TimeSpan.Zero); + + var identity = await flasher.ReadIdentityAsync("COM1"); + + Assert.Equal(0x001003A0u, identity.ChipId); + Assert.Equal(0x00C22018u, identity.FlashJedecId); + Assert.True(identity.IsRecognizedWinc); + Assert.Equal(WincModuleInspector.FastBaudRate, identity.NegotiatedBaudRate); + Assert.Contains(WincModuleInspector.FastBaudRate, port.BaudRateHistory); + } + + [Fact] + public async Task Inspector_ReadIdentityAsync_ReportsAnUnrecognizedChip() + { + // Bridge up but WINC not answering is a distinct, actionable condition from a dead port. + var port = CreateReadyPort(); + port.Registers[WincFlashReader.ChipIdRegister] = 0xFFFFFFFF; + + var flasher = new WincModuleInspector((_, _) => port, baudSettleDelay: TimeSpan.Zero); + + var identity = await flasher.ReadIdentityAsync("COM1"); + + Assert.False(identity.IsRecognizedWinc); + } + + [Fact] + public async Task Inspector_ReadIdentityAsync_ExplainsWhenNoBridgeIsListening() + { + var port = new FakeWincSerialPort { SuppressIdentityResponse = true }; + var flasher = new WincModuleInspector((_, _) => port, baudSettleDelay: TimeSpan.Zero); + + var ex = await Assert.ThrowsAsync(() => flasher.ReadIdentityAsync("COM1")); + + Assert.Contains("bridge", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.True(port.WasDisposed); + } + + [Fact] + public async Task Inspector_ReadIdentityAsync_RejectsAnEmptyPortName() + { + var flasher = new WincModuleInspector((_, _) => CreateReadyPort()); + + await Assert.ThrowsAsync(() => flasher.ReadIdentityAsync(" ")); + } + + // ---- WincFlashToolLocator ------------------------------------------- + + [Fact] + public void Locator_IsAvailable_WhenTheFirmwarePathIsTheToolItself() + { + var toolPath = Path.Combine(Path.GetTempPath(), $"winc_flash_tool_{Guid.NewGuid():N}.cmd"); + File.WriteAllText(toolPath, "@echo off"); + + try + { + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + + Assert.True(locator.IsAvailable(toolPath)); + } + finally + { + File.Delete(toolPath); + } + } + + [Fact] + public void Locator_IsAvailable_FindsTheToolBeneathADirectory() + { + var root = Path.Combine(Path.GetTempPath(), $"winc_{Guid.NewGuid():N}"); + var nested = Path.Combine(root, "winc"); + Directory.CreateDirectory(nested); + var toolPath = Path.Combine(nested, "winc_flash_tool.cmd"); + File.WriteAllText(toolPath, "@echo off"); + + try + { + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + + Assert.True(locator.IsAvailable(root)); + Assert.True(locator.TryResolveToolPath(root, out var resolved)); + Assert.Equal(toolPath, resolved); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + [Fact] + public void Locator_IsNotAvailable_WhenTheToolIsMissing() + { + // This is the Linux/macOS case that motivates issue #271 — the answer must be a clean + // "no", not an exception mid-update. + var root = Path.Combine(Path.GetTempPath(), $"winc_{Guid.NewGuid():N}"); + Directory.CreateDirectory(root); + + try + { + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + + Assert.False(locator.IsAvailable(root)); + Assert.False(locator.TryResolveToolPath(root, out _)); + } + finally + { + Directory.Delete(root, recursive: true); + } + } + + [Fact] + public void Locator_IsNotAvailable_ForAPathThatDoesNotExist() + { + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + + Assert.False(locator.IsAvailable(Path.Combine(Path.GetTempPath(), $"missing_{Guid.NewGuid():N}"))); + } + + [Fact] + public void Locator_RejectsAnEmptyToolName() + { + Assert.Throws(() => new WincFlashToolLocator(" ")); + } + + +} diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincSerialBridgeClientTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincSerialBridgeClientTests.cs new file mode 100644 index 0000000..e398434 --- /dev/null +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincSerialBridgeClientTests.cs @@ -0,0 +1,304 @@ +using Daqifi.Core.Firmware.Winc; + +namespace Daqifi.Core.Tests.Firmware.Winc; + +/// +/// Exercises the bridge client against a fake that re-implements the firmware's parser, so a +/// mis-framed command is rejected here the same way the device would reject it. +/// +public class WincSerialBridgeClientTests +{ + private static WincSerialBridgeClient CreateClient(FakeWincSerialPort port) + => new(port, TimeSpan.FromSeconds(1)); + + [Fact] + public void TryHandshake_SucceedsWhenTheBridgeIdentifies() + { + var port = new FakeWincSerialPort(); + + Assert.True(CreateClient(port).TryHandshake()); + } + + [Fact] + public void TryHandshake_ReturnsFalseWhenNothingAnswers() + { + // A silent port means the device is not in bridge mode — a normal, recoverable condition, + // so this reports rather than throws. + var port = new FakeWincSerialPort { SuppressIdentityResponse = true }; + + Assert.False(CreateClient(port).TryHandshake()); + } + + [Fact] + public void TryHandshake_DiscardsStaleInputFirst() + { + var port = new FakeWincSerialPort(); + + CreateClient(port).TryHandshake(); + + Assert.True(port.DiscardCount >= 1); + } + + [Fact] + public void ReadRegister_RoundTripsABigEndianValue() + { + var port = new FakeWincSerialPort(); + port.Registers[0x1000] = 0x001003A0; // bench-observed WINC1500 chip id + + Assert.Equal(0x001003A0u, CreateClient(port).ReadRegister(0x1000)); + } + + [Fact] + public void ReadRegister_SendsAWellFormedHeaderTheDeviceAccepts() + { + var port = new FakeWincSerialPort(); + port.Registers[0x10218] = 1; + + CreateClient(port).ReadRegister(0x10218); + + var header = Assert.Single(port.ReceivedHeaders); + Assert.True(WincBridgeProtocol.IsHeaderValid(header)); + Assert.Equal((byte)WincBridgeProtocol.Command.ReadRegisterWithReturn, header[0]); + } + + [Fact] + public void WriteRegister_SendsValueAndAddressInTheHeader() + { + var port = new FakeWincSerialPort(); + + CreateClient(port).WriteRegister(0x10208, 0x400); + + var header = Assert.Single(port.ReceivedHeaders); + Assert.Equal((byte)WincBridgeProtocol.Command.WriteRegister, header[0]); + Assert.Equal(0x08, header[4]); + Assert.Equal(0x02, header[5]); + Assert.Equal(0x01, header[6]); + Assert.Equal(0x00, header[8]); + Assert.Equal(0x04, header[9]); + } + + [Fact] + public void ReadBlock_ReturnsTheRequestedBytes() + { + var port = new FakeWincSerialPort(); + port.Blocks[0xD0000] = [1, 2, 3, 4, 5, 6, 7, 8]; + + var data = CreateClient(port).ReadBlock(0xD0000, 8); + + Assert.Equal([1, 2, 3, 4, 5, 6, 7, 8], data); + } + + [Theory] + [InlineData(2048)] + [InlineData(4096)] + [InlineData(65536)] + public void ReadBlock_RejectsSizesThatWouldWedgeTheDevice(int size) + { + // At or above the firmware's 2048-byte buffer its read loop never terminates. Failing fast + // here is what keeps a bulk read from hanging forever against real hardware. + var port = new FakeWincSerialPort(); + + var ex = Assert.Throws( + () => CreateClient(port).ReadBlock(0xD0000, size)); + + Assert.Equal("size", ex.ParamName); + Assert.Empty(port.ReceivedHeaders); + } + + [Fact] + public void ReadBlock_AcceptsTheMaximumSafeSize() + { + var port = new FakeWincSerialPort(); + port.Blocks[0xD0000] = new byte[WincBridgeProtocol.MaxReadBlockSize]; + + var data = CreateClient(port).ReadBlock(0xD0000, WincBridgeProtocol.MaxReadBlockSize); + + Assert.Equal(WincBridgeProtocol.MaxReadBlockSize, data.Length); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void ReadBlock_RejectsNonPositiveSizes(int size) + { + Assert.Throws( + () => CreateClient(new FakeWincSerialPort()).ReadBlock(0xD0000, size)); + } + + [Fact] + public void WriteBlock_SendsThePayloadAfterTheHeaderAck() + { + var port = new FakeWincSerialPort(); + byte[] payload = [0xDE, 0xAD, 0xBE, 0xEF]; + + CreateClient(port).WriteBlock(0xD0000, payload); + + Assert.Equal(payload, Assert.Single(port.ReceivedPayloads)); + var header = Assert.Single(port.ReceivedHeaders); + Assert.Equal((byte)WincBridgeProtocol.Command.WriteBlock, header[0]); + Assert.Equal(4, (header[3] << 8) | header[2]); + } + + [Fact] + public void WriteBlock_ThrowsWhenTheDeviceNacksThePayload() + { + var port = new FakeWincSerialPort { FailNextBlockWrite = true }; + + var ex = Assert.Throws( + () => CreateClient(port).WriteBlock(0xD0000, [1, 2, 3, 4])); + + Assert.Contains("rejected", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void WriteBlock_RejectsAnEmptyPayload() + { + Assert.Throws( + () => CreateClient(new FakeWincSerialPort()).WriteBlock(0xD0000, [])); + } + + [Fact] + public void WriteBlock_RejectsAnOversizePayload() + { + Assert.Throws( + () => CreateClient(new FakeWincSerialPort()) + .WriteBlock(0xD0000, new byte[WincBridgeProtocol.MaxWriteBlockSize + 1])); + } + + [Fact] + public void ChangeBaudRate_TellsTheDeviceThenMovesTheHost() + { + // Order is load-bearing: the device switches as soon as it processes the command, so the + // header must already be on the wire before the host rate changes. + var port = new FakeWincSerialPort(); + + CreateClient(port).ChangeBaudRate(500000, TimeSpan.Zero); + + var header = Assert.Single(port.ReceivedHeaders); + Assert.Equal((byte)WincBridgeProtocol.Command.Reconfigure, header[0]); + Assert.Equal(500000, Assert.Single(port.BaudRateHistory)); + Assert.Equal(500000, port.BaudRate); + } + + [Fact] + public void ChangeBaudRate_DiscardsBytesStragglingInAtTheOldRate() + { + var port = new FakeWincSerialPort(); + + CreateClient(port).ChangeBaudRate(500000, TimeSpan.Zero); + + Assert.True(port.DiscardCount >= 1); + } + + [Theory] + [InlineData(0)] + [InlineData(-9600)] + public void ChangeBaudRate_RejectsNonPositiveRates(int baud) + { + var port = new FakeWincSerialPort(); + + Assert.Throws( + () => CreateClient(port).ChangeBaudRate(baud, TimeSpan.Zero)); + Assert.Empty(port.ReceivedHeaders); + } + + [Fact] + public void Commands_AreAlwaysPrefixedWithTheStartByte() + { + // Without the 0xA5 prefix the device stays in its op-code state and the header is read as a + // stream of unknown op codes, which fails silently rather than loudly. + var port = new RecordingPort(); + var client = new WincSerialBridgeClient(port, TimeSpan.FromSeconds(1)); + + try + { + client.WriteRegister(0x1000, 1); + } + catch (TimeoutException) + { + // The recording port never answers; we only care about what went out. + } + + Assert.Equal(WincBridgeProtocol.StartCommand, port.Written[0]); + Assert.Equal(1 + WincBridgeProtocol.HeaderSize, port.Written.Count); + } + + [Fact] + public void Commands_ThrowWhenTheDeviceNacksTheHeader() + { + var port = new NackingPort(); + var client = new WincSerialBridgeClient(port, TimeSpan.FromSeconds(1)); + + var ex = Assert.Throws(() => client.ReadRegister(0x1000)); + + Assert.Contains("checksum", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void Commands_ThrowWhenTheDeviceReturnsAnUnexpectedByte() + { + // Out-of-sync with the bridge state machine is a distinct failure from a rejected checksum, + // and the message should say so. + var port = new NackingPort { Verdict = 0x77 }; + var client = new WincSerialBridgeClient(port, TimeSpan.FromSeconds(1)); + + var ex = Assert.Throws(() => client.ReadRegister(0x1000)); + + Assert.Contains("unexpected", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.Contains("0x77", ex.Message, StringComparison.OrdinalIgnoreCase); + } + + [Fact] + public void Constructor_RejectsANullPort() + { + Assert.Throws(() => new WincSerialBridgeClient(null!)); + } + + /// Captures outbound bytes and never answers. + private sealed class RecordingPort : IWincSerialPort + { + internal List Written { get; } = []; + + public bool IsOpen => true; + public int BaudRate { get; set; } = 115200; + public void Open() { } + public void Close() { } + public void DiscardInBuffer() { } + + public void Write(byte[] buffer, int offset, int count) + { + for (var i = 0; i < count; i++) + { + Written.Add(buffer[offset + i]); + } + } + + public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) + => throw new TimeoutException("Recording port never answers."); + + public void Dispose() { } + } + + /// Answers every header with a fixed verdict byte. + private sealed class NackingPort : IWincSerialPort + { + internal byte Verdict { get; set; } = WincBridgeProtocol.Response.Nack; + + public bool IsOpen => true; + public int BaudRate { get; set; } = 115200; + public void Open() { } + public void Close() { } + public void DiscardInBuffer() { } + public void Write(byte[] buffer, int offset, int count) { } + + public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) + { + for (var i = 0; i < count; i++) + { + buffer[offset + i] = Verdict; + } + } + + public void Dispose() { } + } +} diff --git a/src/Daqifi.Core/Firmware/WifiModuleUpdater.cs b/src/Daqifi.Core/Firmware/WifiModuleUpdater.cs index ee50aa2..cb245d0 100644 --- a/src/Daqifi.Core/Firmware/WifiModuleUpdater.cs +++ b/src/Daqifi.Core/Firmware/WifiModuleUpdater.cs @@ -1,5 +1,6 @@ using Daqifi.Core.Communication.Producers; using Daqifi.Core.Device; +using Daqifi.Core.Firmware.Winc; using Microsoft.Extensions.Logging; namespace Daqifi.Core.Firmware; @@ -743,28 +744,30 @@ private static bool ContainsAny(IReadOnlyList lines, params string[] mar private string ResolveWifiToolPath(string firmwarePath) { - if (File.Exists(firmwarePath)) + if (!File.Exists(firmwarePath) && !Directory.Exists(firmwarePath)) { - return firmwarePath; + throw new FileNotFoundException("WiFi firmware path was not found.", firmwarePath); } - if (Directory.Exists(firmwarePath)) + // Resolution goes through the shared locator so there is a single answer to + // "can this environment flash the WiFi module?" (part of #271). + var locator = new WincFlashToolLocator(Options.WifiFlashToolFileName); + if (locator.TryResolveToolPath(firmwarePath, out var toolPath)) { - var matches = Directory.GetFiles( - firmwarePath, - Options.WifiFlashToolFileName, - SearchOption.AllDirectories); - - if (matches.Length == 0) - { - throw new FileNotFoundException( - $"Could not locate '{Options.WifiFlashToolFileName}' under '{firmwarePath}'."); - } - - return matches[0]; + return toolPath; } - throw new FileNotFoundException("WiFi firmware path was not found.", firmwarePath); + // Say why plainly. Microchip's flash tool is a Windows .cmd/.exe, so on Linux and macOS + // this is not a misconfigured path — the tool genuinely cannot be there, and the caller + // needs to know that rather than reading it as a missing download. + var platformNote = OperatingSystem.IsWindows() + ? string.Empty + : $" On {(OperatingSystem.IsMacOS() ? "macOS" : "this platform")} the WiFi flash tool is " + + "unavailable — Microchip ships it as a Windows program. WiFi module flashing is " + + "currently Windows-only; see issue #271."; + + throw new FileNotFoundException( + $"Could not locate '{Options.WifiFlashToolFileName}' under '{firmwarePath}'.{platformNote}"); } private string ResolveWifiPort(IStreamingDevice device) diff --git a/src/Daqifi.Core/Firmware/Winc/IWincSerialPort.cs b/src/Daqifi.Core/Firmware/Winc/IWincSerialPort.cs new file mode 100644 index 0000000..9a5961e --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/IWincSerialPort.cs @@ -0,0 +1,42 @@ +namespace Daqifi.Core.Firmware.Winc; + +/// +/// The narrow serial surface the WINC bridge protocol needs: raw byte I/O plus a baud rate that can +/// change on an already-open port. +/// +/// +/// Core's SerialStreamTransport deliberately does not fit here — it fixes baud at +/// construction and adds a liveness watchdog, whereas the bridge re-negotiates from 115200 to +/// 500000 partway through a session and must not be probed underneath. Keeping this interface +/// separate also makes the whole protocol layer testable without hardware. +/// +internal interface IWincSerialPort : IDisposable +{ + /// Whether the port is currently open. + bool IsOpen { get; } + + /// + /// Current baud rate. Setting it on an open port re-negotiates the host side, and must only be + /// done after the bridge has acknowledged a . + /// + int BaudRate { get; set; } + + /// Opens the port. + void Open(); + + /// Closes the port. + void Close(); + + /// Drops any buffered inbound bytes, so a read starts from a known-clean state. + void DiscardInBuffer(); + + /// Writes bytes from to the port. + void Write(byte[] buffer, int offset, int count); + + /// + /// Reads exactly bytes into , blocking until + /// they arrive or elapses. + /// + /// Fewer than bytes arrived in time. + void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout); +} diff --git a/src/Daqifi.Core/Firmware/Winc/SystemWincSerialPort.cs b/src/Daqifi.Core/Firmware/Winc/SystemWincSerialPort.cs new file mode 100644 index 0000000..ff9c6fb --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/SystemWincSerialPort.cs @@ -0,0 +1,122 @@ +using System.IO.Ports; + +namespace Daqifi.Core.Firmware.Winc; + +/// +/// over . Cross-platform: this is what makes a +/// native WINC flash possible on Linux and macOS, where Microchip's tool does not run. +/// +internal sealed class SystemWincSerialPort : IWincSerialPort +{ + private readonly SerialPort _port; + private bool _disposed; + + internal SystemWincSerialPort(string portName, int baudRate = 115200) + { + _port = new SerialPort(portName, baudRate, Parity.None, 8, StopBits.One) + { + // USB CDC delivers nothing to the host without DTR asserted — the DAQiFi firmware + // gates its writes on the host being present. + DtrEnable = true, + RtsEnable = false, + ReadTimeout = 2000, + WriteTimeout = 2000 + }; + } + + public bool IsOpen => _port.IsOpen; + + public int BaudRate + { + get => _port.BaudRate; + set => _port.BaudRate = value; + } + + public void Open() + { + if (!_port.IsOpen) + { + _port.Open(); + } + } + + public void Close() + { + if (_port.IsOpen) + { + _port.Close(); + } + } + + public void DiscardInBuffer() + { + if (_port.IsOpen) + { + _port.DiscardInBuffer(); + } + } + + public void Write(byte[] buffer, int offset, int count) + { + _port.Write(buffer, offset, count); + } + + public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) + { + var deadline = DateTime.UtcNow + timeout; + var read = 0; + + while (read < count) + { + var remaining = deadline - DateTime.UtcNow; + if (remaining <= TimeSpan.Zero) + { + throw new TimeoutException( + $"Timed out reading from {_port.PortName}: wanted {count} bytes, got {read}."); + } + + // SerialPort.Read returns as soon as *any* bytes are available, so loop until the full + // frame has arrived rather than assuming one read yields everything. + _port.ReadTimeout = Math.Max(1, (int)remaining.TotalMilliseconds); + + int chunk; + try + { + chunk = _port.Read(buffer, offset + read, count - read); + } + catch (TimeoutException) + { + throw new TimeoutException( + $"Timed out reading from {_port.PortName}: wanted {count} bytes, got {read}."); + } + + if (chunk <= 0) + { + throw new IOException($"Serial port {_port.PortName} returned no data before closing."); + } + + read += chunk; + } + } + + public void Dispose() + { + if (_disposed) + { + return; + } + + _disposed = true; + + try + { + Close(); + } + catch (Exception) + { + // Disposal must not throw; a port that is already gone is not an error worth surfacing. + } + + _port.Dispose(); + } +} diff --git a/src/Daqifi.Core/Firmware/Winc/WincBridgeProtocol.cs b/src/Daqifi.Core/Firmware/Winc/WincBridgeProtocol.cs new file mode 100644 index 0000000..a732d8d --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/WincBridgeProtocol.cs @@ -0,0 +1,183 @@ +namespace Daqifi.Core.Firmware.Winc; + +/// +/// Wire-format constants and framing for the WINC serial bridge — the UART protocol the DAQiFi +/// PIC32 speaks while it is bridging its USB-CDC port through to the WINC1500. +/// +/// +/// +/// The authority for this format is the DAQiFi firmware's own bridge implementation +/// (firmware/src/services/wifi_services/wifi_serial_bridge.c), not Microchip's tool — +/// Microchip ships winc_programmer_uart as a binary only, with no source, so the bridge +/// the device actually implements is the better and more accurate reference. +/// +/// +/// A command is an byte followed by a 12-byte header whose bytes XOR to +/// zero. Note the mixed endianness, which is easy to get wrong: header fields are little-endian, +/// but a register value read back by arrives +/// big-endian. +/// +/// +internal static class WincBridgeProtocol +{ + /// Size of the fixed command header that follows . + internal const int HeaderSize = 12; + + /// Identify op code; the bridge answers . + internal const byte IdentifyVariableBaud = 0x12; + + /// + /// Identify op code for a fixed-baud bridge. The DAQiFi bridge is variable-baud and answers + /// nothing at all to this, so it is only useful for distinguishing bridge flavors. + /// + internal const byte IdentifyFixedBaud = 0x13; + + /// Op code that begins a command; the next bytes are the header. + internal const byte StartCommand = 0xA5; + + /// + /// Largest payload a single may request. + /// + /// + /// The bridge's read loop neither decrements its counter nor advances the address, so a request + /// of 2048 or more spins forever re-sending the same chunk and never returns. Reads must + /// therefore stay strictly below the firmware's 2048-byte command buffer. This cap is a + /// deliberate guard against that firmware behavior, not a protocol limit. + /// + internal const int MaxReadBlockSize = 2047; + + /// Largest payload a single may carry. + internal const int MaxWriteBlockSize = 2048; + + /// Bridge command identifiers (header byte 0). + internal enum Command : byte + { + /// Read a 32-bit register; the bridge returns 4 big-endian bytes. + ReadRegisterWithReturn = 0, + + /// Write a 32-bit register. No data response beyond the header ACK. + WriteRegister = 1, + + /// Read size raw bytes starting at address. + ReadBlock = 2, + + /// Write size payload bytes, sent after the header ACK. + WriteBlock = 3, + + /// Re-negotiate the bridge's UART baud rate to value. + Reconfigure = 5 + } + + /// Single-byte responses the bridge emits. + internal static class Response + { + /// Header checksum rejected, or a block write failed. + internal const byte Nack = 0x5A; + + /// Identify response from a variable-baud bridge. + internal const byte IdVariableBaud = 0x5B; + + /// Identify response from a fixed-baud bridge. + internal const byte IdFixedBaud = 0x5C; + + /// Header accepted, or a block write succeeded. + internal const byte Ack = 0xAC; + } + + /// + /// Builds the 12-byte command header. Header fields are little-endian; byte 1 is the checksum + /// slot, set so the XOR of all twelve bytes is zero. + /// + /// The command identifier. + /// Payload/transfer size (little-endian u16). + /// Target address (little-endian u32). + /// Command value — the register value, or the new baud rate (little-endian u32). + internal static byte[] BuildHeader(Command command, ushort size, uint address, uint value) + { + var header = new byte[HeaderSize]; + + header[0] = (byte)command; + // header[1] is the checksum, filled in below. + header[2] = (byte)(size & 0xFF); + header[3] = (byte)((size >> 8) & 0xFF); + header[4] = (byte)(address & 0xFF); + header[5] = (byte)((address >> 8) & 0xFF); + header[6] = (byte)((address >> 16) & 0xFF); + header[7] = (byte)((address >> 24) & 0xFF); + header[8] = (byte)(value & 0xFF); + header[9] = (byte)((value >> 8) & 0xFF); + header[10] = (byte)((value >> 16) & 0xFF); + header[11] = (byte)((value >> 24) & 0xFF); + + header[1] = ComputeChecksum(header); + return header; + } + + /// + /// Returns the byte that, placed at index 1, makes the header's full XOR zero. Computed by + /// XOR-ing every byte except index 1 itself. + /// + internal static byte ComputeChecksum(IReadOnlyList header) + { + ArgumentNullException.ThrowIfNull(header); + + if (header.Count != HeaderSize) + { + throw new ArgumentException( + $"A bridge header is exactly {HeaderSize} bytes; got {header.Count}.", nameof(header)); + } + + byte checksum = 0; + for (var i = 0; i < HeaderSize; i++) + { + if (i == 1) + { + continue; + } + + checksum ^= header[i]; + } + + return checksum; + } + + /// + /// True when the header satisfies the bridge's acceptance test: the XOR of all twelve bytes is + /// zero. This is the exact check the firmware performs before it ACKs. + /// + internal static bool IsHeaderValid(IReadOnlyList header) + { + if (header is null || header.Count != HeaderSize) + { + return false; + } + + byte checksum = 0; + for (var i = 0; i < HeaderSize; i++) + { + checksum ^= header[i]; + } + + return checksum == 0; + } + + /// + /// Decodes a register value from a reply, which the + /// bridge sends big-endian — the opposite order from the header fields. + /// + internal static uint DecodeRegisterValue(byte[] response) + { + ArgumentNullException.ThrowIfNull(response); + + if (response.Length != 4) + { + throw new ArgumentException( + $"A register response is exactly 4 bytes; got {response.Length}.", nameof(response)); + } + + return ((uint)response[0] << 24) + | ((uint)response[1] << 16) + | ((uint)response[2] << 8) + | response[3]; + } +} diff --git a/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs b/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs new file mode 100644 index 0000000..b602669 --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs @@ -0,0 +1,169 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Daqifi.Core.Firmware.Winc; + +/// +/// Read-only access to the WINC1500's SPI flash and identity registers, driven through the serial +/// bridge. Every operation here is non-destructive: identity reads and flash reads only. +/// +/// +/// +/// The WINC's flash controller is a set of memory-mapped registers, so a flash read is a scripted +/// register sequence followed by a block read of the shared memory window — the same sequence the +/// WINC host driver's spi_flash.c uses. Notably this needs no programmer_firmware.bin +/// blob; that upload is only required for the erase/program path. +/// +/// +/// Erase and program are deliberately absent. They are the operations that can brick a module, and +/// nothing in this class can leave the WINC in a worse state than it started. +/// +/// +internal sealed class WincFlashReader +{ + /// Chip identity register. A live WINC1500 reads 0x0015xxxx, or 0x0010xxxx once halted in download mode. + internal const uint ChipIdRegister = 0x1000; + + private const uint SpiFlashBase = 0x10200; + private const uint RegCommandCount = SpiFlashBase + 0x04; + private const uint RegDataCount = SpiFlashBase + 0x08; + private const uint RegBuffer1 = SpiFlashBase + 0x0C; + private const uint RegBuffer2 = SpiFlashBase + 0x10; + private const uint RegBufferDirection = SpiFlashBase + 0x14; + private const uint RegTransferDone = SpiFlashBase + 0x18; + private const uint RegDmaAddress = SpiFlashBase + 0x1C; + + /// Scratch window in WINC memory that flash reads are staged through. + private const uint HostShareMemoryBase = 0xD0000; + + /// Register the flash controller parks a byte-wide result in. + private const uint DummyRegister = 0x1084; + + // SPI NOR flash commands (MX25L-compatible). + private const byte FlashCommandFastRead = 0x0B; + private const byte FlashCommandReadIdentification = 0x9F; + + /// Dummy byte the fast-read command clocks out before data. + private const byte FastReadDummyByte = 0xA5; + + /// Bit that starts a flash-controller transfer. + private const uint CommandStartBit = 1u << 7; + + private readonly WincSerialBridgeClient _bridge; + private readonly ILogger _logger; + private readonly int _transferPollLimit; + + internal WincFlashReader( + WincSerialBridgeClient bridge, + int transferPollLimit = 1000, + ILogger? logger = null) + { + _bridge = bridge ?? throw new ArgumentNullException(nameof(bridge)); + _transferPollLimit = transferPollLimit; + _logger = logger ?? NullLogger.Instance; + } + + /// + /// Reads the WINC chip identity register. + /// + internal uint ReadChipId() => _bridge.ReadRegister(ChipIdRegister); + + /// + /// True when a chip ID looks like a reachable WINC1500 — family 0x15 (firmware running) or + /// 0x10 (halted in download mode). Anything else means the bridge is up but the WINC behind it + /// is not answering, which is worth distinguishing from a dead port. + /// + internal static bool IsKnownWincChipId(uint chipId) + { + var family = chipId >> 16; + return family is 0x15 or 0x10; + } + + /// + /// Reads the SPI flash JEDEC identification word (command 0x9F). + /// + internal uint ReadFlashJedecId() + { + _bridge.WriteRegister(RegDataCount, 0); + _bridge.WriteRegister(RegBuffer1, FlashCommandReadIdentification); + _bridge.WriteRegister(RegBufferDirection, 0x01); + _bridge.WriteRegister(RegDmaAddress, 0); + _bridge.WriteRegister(RegCommandCount, 1 | CommandStartBit); + + WaitForTransferDone("read flash JEDEC id"); + + return _bridge.ReadRegister(DummyRegister); + } + + /// + /// Reads bytes of SPI flash starting at , + /// chunking to stay inside both the flash controller's staging window and the bridge's + /// block-read ceiling. + /// + internal byte[] ReadFlash(uint offset, int length, CancellationToken cancellationToken = default) + { + if (length <= 0) + { + throw new ArgumentOutOfRangeException(nameof(length), length, "Read length must be positive."); + } + + var result = new byte[length]; + var read = 0; + + while (read < length) + { + cancellationToken.ThrowIfCancellationRequested(); + + var chunk = Math.Min(WincBridgeProtocol.MaxReadBlockSize, length - read); + var chunkData = ReadFlashChunk((uint)(offset + read), chunk); + Buffer.BlockCopy(chunkData, 0, result, read, chunk); + read += chunk; + } + + return result; + } + + /// + /// Stages one chunk of flash into the shared memory window and reads it back. Mirrors the host + /// driver's spi_flash_load_to_cortus_mem followed by a block read. + /// + private byte[] ReadFlashChunk(uint flashAddress, int size) + { + // The fast-read command word packs the opcode and the 24-bit address into one register, + // opcode in the low byte and address bytes ascending from there. + var commandWord = (uint)FlashCommandFastRead + | ((flashAddress >> 16) & 0xFFu) << 8 + | ((flashAddress >> 8) & 0xFFu) << 16 + | (flashAddress & 0xFFu) << 24; + + _bridge.WriteRegister(RegDataCount, (uint)size); + _bridge.WriteRegister(RegBuffer1, commandWord); + _bridge.WriteRegister(RegBuffer2, FastReadDummyByte); + _bridge.WriteRegister(RegBufferDirection, 0x1F); + _bridge.WriteRegister(RegDmaAddress, HostShareMemoryBase); + _bridge.WriteRegister(RegCommandCount, 5 | CommandStartBit); + + WaitForTransferDone($"read {size} flash bytes at 0x{flashAddress:X6}"); + + return _bridge.ReadBlock(HostShareMemoryBase, size); + } + + /// + /// Polls the flash controller's done flag. Bounded rather than a bare spin: the firmware's own + /// equivalent loop is unbounded, and a WINC that stops answering would otherwise hang the host. + /// + private void WaitForTransferDone(string operation) + { + for (var attempt = 0; attempt < _transferPollLimit; attempt++) + { + if (_bridge.ReadRegister(RegTransferDone) == 1) + { + return; + } + } + + throw new TimeoutException( + $"WINC flash controller never reported transfer-done while attempting to {operation} " + + $"(polled {_transferPollLimit} times)."); + } +} diff --git a/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs b/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs new file mode 100644 index 0000000..e0bef87 --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs @@ -0,0 +1,80 @@ +using System.Diagnostics.CodeAnalysis; + +namespace Daqifi.Core.Firmware.Winc; + +/// +/// Finds Microchip's external WINC flash tool under a firmware path, so a caller can answer +/// "can this machine flash the WiFi module?" before starting an update rather than discovering the +/// answer partway through. +/// +/// +/// This is the check that separates Windows from Linux/macOS: Microchip ships the flash tool as a +/// Windows .cmd/.exe, so on other platforms it is genuinely absent rather than +/// misconfigured. It locates the tool; it does not run it — the WiFi update flow owns that, +/// including the interactive prompt handshake and output-based success verification. +/// +public sealed class WincFlashToolLocator +{ + private readonly string _toolFileName; + + /// + /// Creates a locator that searches for (e.g. + /// winc_flash_tool.cmd). + /// + public WincFlashToolLocator(string toolFileName) + { + if (string.IsNullOrWhiteSpace(toolFileName)) + { + throw new ArgumentException("Tool file name cannot be empty.", nameof(toolFileName)); + } + + _toolFileName = toolFileName; + } + + /// + /// Whether the flash tool is present for the given firmware path. + /// + public bool IsAvailable(string firmwarePath) => TryResolveToolPath(firmwarePath, out _); + + /// + /// Resolves the flash tool for a firmware path: the path itself when it is the tool, otherwise + /// the first match found beneath it. + /// + public bool TryResolveToolPath(string firmwarePath, [NotNullWhen(true)] out string? toolPath) + { + toolPath = null; + + if (string.IsNullOrWhiteSpace(firmwarePath)) + { + return false; + } + + if (File.Exists(firmwarePath)) + { + toolPath = firmwarePath; + return true; + } + + if (!Directory.Exists(firmwarePath)) + { + return false; + } + + try + { + var matches = Directory.GetFiles(firmwarePath, _toolFileName, SearchOption.AllDirectories); + if (matches.Length == 0) + { + return false; + } + + toolPath = matches[0]; + return true; + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + // An unreadable tree is indistinguishable from a missing tool for availability purposes. + return false; + } + } +} diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleIdentity.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleIdentity.cs new file mode 100644 index 0000000..29cd84c --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleIdentity.cs @@ -0,0 +1,26 @@ +namespace Daqifi.Core.Firmware.Winc; + +/// +/// A non-destructive summary of the WINC module: what the bridge and the module reported without +/// changing anything. Produced by . +/// +public sealed class WincModuleIdentity +{ + /// + /// Chip identity register (0x1000). A live WINC1500 reads 0x0015xxxx with firmware running, or + /// 0x0010xxxx once halted in download mode. + /// + public required uint ChipId { get; init; } + + /// SPI flash JEDEC identification word. + public required uint FlashJedecId { get; init; } + + /// + /// Whether matches a known WINC1500 family. False means the bridge + /// answered but the module behind it did not — a different problem from a dead port. + /// + public required bool IsRecognizedWinc { get; init; } + + /// Link speed in effect when the identity was read. + public required int NegotiatedBaudRate { get; init; } +} diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs new file mode 100644 index 0000000..fba3d43 --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs @@ -0,0 +1,171 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Daqifi.Core.Firmware.Winc; + +/// +/// Managed, cross-platform access to the WINC module over its UART serial bridge — no external +/// tool, so it works on Linux and macOS where Microchip's programmer does not run. +/// +/// +/// +/// This inspects; it does not flash. It implements the non-destructive half of the WINC +/// protocol — bridge handshake, baud negotiation, chip and flash identity, and flash +/// read-back/verify — and the name says so rather than hiding it behind a "flasher" that cannot +/// write. +/// +/// +/// The erase/program path is absent because it could not be validated. Programming a WINC means +/// erase and page-program cycles against the module's SPI flash, and a wrong opcode or address +/// bricks the module with no recovery path outside Microchip's Windows tool. Shipping an +/// unexercised write path would be worse than shipping none: it would look complete, callers would +/// select it, and the first time it ran would be on someone's hardware. The framing such a writer +/// needs () is here and tested, so that work starts from a +/// known-good base. +/// +/// +public sealed class WincModuleInspector +{ + /// Rate the bridge starts at before renegotiation. + public const int InitialBaudRate = 115200; + + /// + /// Rate the bridge is moved to for bulk transfer, matching Microchip's tool. + /// + /// + /// Over the DAQiFi device's USB-CDC link this is largely ceremonial — CDC ignores the line + /// rate — but the exchange still has to happen: it is what the bridge expects, and completing + /// it proves the command path works before any bulk transfer starts. + /// + public const int FastBaudRate = 500000; + + private readonly Func _portFactory; + private readonly ILogger _logger; + private readonly TimeSpan _baudSettleDelay; + private readonly TimeSpan _responseTimeout; + + /// + /// Creates an inspector that opens real serial ports. + /// + public WincModuleInspector(ILogger? logger = null) + : this((port, baud) => new SystemWincSerialPort(port, baud), logger) + { + } + + /// + /// Test seam: supply the serial port implementation. + /// + internal WincModuleInspector( + Func portFactory, + ILogger? logger = null, + TimeSpan? baudSettleDelay = null, + TimeSpan? responseTimeout = null) + { + _portFactory = portFactory ?? throw new ArgumentNullException(nameof(portFactory)); + _logger = logger ?? NullLogger.Instance; + _baudSettleDelay = baudSettleDelay ?? TimeSpan.FromMilliseconds(100); + _responseTimeout = responseTimeout ?? TimeSpan.FromSeconds(2); + } + + /// + /// Opens the bridge, handshakes, moves to , and reports the module's + /// chip and flash identity. Changes nothing on the device. + /// + public Task ReadIdentityAsync( + string portName, + CancellationToken cancellationToken = default) + => Task.Run(() => ReadIdentity(portName, cancellationToken), cancellationToken); + + /// + /// Reads a span of the module's SPI flash. Changes nothing on the device. + /// + public Task ReadFlashAsync( + string portName, + uint offset, + int length, + CancellationToken cancellationToken = default) + => Task.Run(() => ReadFlash(portName, offset, length, cancellationToken), cancellationToken); + + private WincModuleIdentity ReadIdentity(string portName, CancellationToken cancellationToken) + { + using var session = OpenSession(portName, cancellationToken); + + var chipId = session.Reader.ReadChipId(); + var flashId = session.Reader.ReadFlashJedecId(); + + _logger.LogInformation( + "WINC identity: chipId=0x{ChipId:X8} flashJedecId=0x{FlashId:X8} baud={Baud}.", + chipId, + flashId, + session.Port.BaudRate); + + return new WincModuleIdentity + { + ChipId = chipId, + FlashJedecId = flashId, + IsRecognizedWinc = WincFlashReader.IsKnownWincChipId(chipId), + NegotiatedBaudRate = session.Port.BaudRate + }; + } + + private byte[] ReadFlash(string portName, uint offset, int length, CancellationToken cancellationToken) + { + using var session = OpenSession(portName, cancellationToken); + return session.Reader.ReadFlash(offset, length, cancellationToken); + } + + /// + /// Opens the port, proves a bridge is listening, and negotiates up to the fast baud rate. + /// + private BridgeSession OpenSession(string portName, CancellationToken cancellationToken) + { + if (string.IsNullOrWhiteSpace(portName)) + { + throw new ArgumentException("Port name cannot be empty.", nameof(portName)); + } + + cancellationToken.ThrowIfCancellationRequested(); + + var port = _portFactory(portName, InitialBaudRate); + try + { + port.Open(); + + var bridge = new WincSerialBridgeClient(port, _responseTimeout, _logger); + + if (!bridge.TryHandshake()) + { + throw new IOException( + $"No WINC serial bridge responded on {portName}. The device must be in WiFi " + + "firmware-update (bridge) mode before the module can be reached."); + } + + bridge.ChangeBaudRate(FastBaudRate, _baudSettleDelay); + + // Re-handshake at the new rate: this both confirms the switch actually took and leaves + // the bridge back in its op-code state before any command is issued. + if (!bridge.TryHandshake()) + { + throw new IOException( + $"The WINC bridge on {portName} stopped responding after switching to " + + $"{FastBaudRate} baud."); + } + + return new BridgeSession(port, new WincFlashReader(bridge, logger: _logger)); + } + catch + { + port.Dispose(); + throw; + } + } + + private sealed class BridgeSession(IWincSerialPort port, WincFlashReader reader) : IDisposable + { + internal IWincSerialPort Port { get; } = port; + + internal WincFlashReader Reader { get; } = reader; + + public void Dispose() => Port.Dispose(); + } +} diff --git a/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs b/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs new file mode 100644 index 0000000..7492112 --- /dev/null +++ b/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs @@ -0,0 +1,197 @@ +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Logging.Abstractions; + +namespace Daqifi.Core.Firmware.Winc; + +/// +/// Speaks the WINC serial bridge protocol over an : identify, register +/// read/write, block read/write and the baud re-negotiation. Every method is one complete bridge +/// exchange; sequencing and the flash-level meaning of those registers belong to callers. +/// +/// +/// See for the wire format. This type does no retrying — a bridge +/// exchange that fails has usually left the bridge state machine mid-command, so recovery is a +/// caller-level decision, not a blind repeat. +/// +internal sealed class WincSerialBridgeClient +{ + private readonly IWincSerialPort _port; + private readonly ILogger _logger; + private readonly TimeSpan _responseTimeout; + + internal WincSerialBridgeClient( + IWincSerialPort port, + TimeSpan? responseTimeout = null, + ILogger? logger = null) + { + _port = port ?? throw new ArgumentNullException(nameof(port)); + _responseTimeout = responseTimeout ?? TimeSpan.FromSeconds(2); + _logger = logger ?? NullLogger.Instance; + } + + /// + /// Sends the identify op code and checks for the variable-baud bridge reply. This is the + /// handshake that proves something is actually bridging on the other end of the port. + /// + /// True when the bridge answered with the expected identify byte. + internal bool TryHandshake() + { + _port.DiscardInBuffer(); + _port.Write([WincBridgeProtocol.IdentifyVariableBaud], 0, 1); + + var response = new byte[1]; + try + { + _port.ReadExactly(response, 0, 1, _responseTimeout); + } + catch (TimeoutException) + { + _logger.LogDebug("WINC bridge handshake timed out; no identify response."); + return false; + } + + if (response[0] == WincBridgeProtocol.Response.IdVariableBaud) + { + return true; + } + + _logger.LogDebug( + "WINC bridge handshake returned 0x{Actual:X2}; expected 0x{Expected:X2}.", + response[0], + WincBridgeProtocol.Response.IdVariableBaud); + return false; + } + + /// Reads a 32-bit WINC register. + internal uint ReadRegister(uint address) + { + SendCommand(WincBridgeProtocol.Command.ReadRegisterWithReturn, 0, address, 0); + + var response = new byte[4]; + _port.ReadExactly(response, 0, 4, _responseTimeout); + return WincBridgeProtocol.DecodeRegisterValue(response); + } + + /// Writes a 32-bit WINC register. + internal void WriteRegister(uint address, uint value) + => SendCommand(WincBridgeProtocol.Command.WriteRegister, 0, address, value); + + /// + /// Reads a block of WINC memory. Callers must chunk to + /// ; a larger request wedges the bridge. + /// + internal byte[] ReadBlock(uint address, int size) + { + if (size <= 0) + { + throw new ArgumentOutOfRangeException(nameof(size), size, "Block read size must be positive."); + } + + if (size > WincBridgeProtocol.MaxReadBlockSize) + { + // Not a style preference: at or above the firmware's 2048-byte buffer the bridge's read + // loop never terminates, so this would hang rather than fail. + throw new ArgumentOutOfRangeException( + nameof(size), + size, + $"Block reads must be at most {WincBridgeProtocol.MaxReadBlockSize} bytes; larger requests " + + "do not terminate in the device's bridge read loop."); + } + + SendCommand(WincBridgeProtocol.Command.ReadBlock, (ushort)size, address, 0); + + var payload = new byte[size]; + _port.ReadExactly(payload, 0, size, _responseTimeout); + return payload; + } + + /// + /// Writes a block of WINC memory: header, ACK, payload, then a final ACK/NACK verdict. + /// + internal void WriteBlock(uint address, byte[] data) + { + ArgumentNullException.ThrowIfNull(data); + + if (data.Length == 0) + { + throw new ArgumentException("Block write payload cannot be empty.", nameof(data)); + } + + if (data.Length > WincBridgeProtocol.MaxWriteBlockSize) + { + throw new ArgumentOutOfRangeException( + nameof(data), + data.Length, + $"Block writes must be at most {WincBridgeProtocol.MaxWriteBlockSize} bytes."); + } + + SendCommand(WincBridgeProtocol.Command.WriteBlock, (ushort)data.Length, address, 0); + + _port.Write(data, 0, data.Length); + + var verdict = new byte[1]; + _port.ReadExactly(verdict, 0, 1, _responseTimeout); + + if (verdict[0] != WincBridgeProtocol.Response.Ack) + { + throw new IOException( + $"WINC bridge rejected a {data.Length}-byte block write at 0x{address:X8} " + + $"(responded 0x{verdict[0]:X2})."); + } + } + + /// + /// Re-negotiates the link speed: tells the bridge to switch, then moves the host side to match. + /// + /// + /// Order matters. The bridge changes its own rate as soon as it processes the command, so the + /// host must follow immediately; anything sent in between is lost. The settle delay gives the + /// device's UART time to reconfigure before the next byte arrives. + /// + internal void ChangeBaudRate(int newBaudRate, TimeSpan settleDelay) + { + if (newBaudRate <= 0) + { + throw new ArgumentOutOfRangeException(nameof(newBaudRate), newBaudRate, "Baud rate must be positive."); + } + + SendCommand(WincBridgeProtocol.Command.Reconfigure, 0, 0, (uint)newBaudRate); + + _port.BaudRate = newBaudRate; + + if (settleDelay > TimeSpan.Zero) + { + Thread.Sleep(settleDelay); + } + + _port.DiscardInBuffer(); + _logger.LogDebug("WINC bridge baud rate changed to {BaudRate}.", newBaudRate); + } + + /// + /// Writes the start byte and header, then consumes the bridge's ACK/NACK verdict on the header. + /// + private void SendCommand(WincBridgeProtocol.Command command, ushort size, uint address, uint value) + { + var header = WincBridgeProtocol.BuildHeader(command, size, address, value); + + _port.Write([WincBridgeProtocol.StartCommand], 0, 1); + _port.Write(header, 0, header.Length); + + var verdict = new byte[1]; + _port.ReadExactly(verdict, 0, 1, _responseTimeout); + + if (verdict[0] == WincBridgeProtocol.Response.Ack) + { + return; + } + + // A NACK means the bridge rejected our checksum, so it has already returned to its + // op-code state; anything else means we are out of sync with its state machine. + var reason = verdict[0] == WincBridgeProtocol.Response.Nack + ? "rejected the command header checksum" + : $"returned an unexpected byte 0x{verdict[0]:X2} instead of an ACK"; + + throw new IOException($"WINC bridge {reason} for command {command} at 0x{address:X8}."); + } +} From 1da67a15b8156af39086f8041bb73f279c87ed43 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 13:36:35 -0600 Subject: [PATCH 2/8] fix(firmware): bound the WINC port open, validate flash address span, 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 --- .../Firmware/Winc/WincFlasherTests.cs | 143 ++++++++++++++++++ .../Firmware/Winc/WincFlashReader.cs | 30 ++-- .../Firmware/Winc/WincFlashToolLocator.cs | 46 ++++-- .../Firmware/Winc/WincModuleInspector.cs | 80 +++++++++- .../Firmware/Winc/WincSerialBridgeClient.cs | 10 +- 5 files changed, 278 insertions(+), 31 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 9d43197..69f1c1f 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -133,6 +133,35 @@ public void ReadFlash_RejectsNonPositiveLengths(int length) Assert.Throws(() => CreateReader(CreateReadyPort()).ReadFlash(0, length)); } + [Theory] + [InlineData(0xFFFF00u, 0x200)] // straddles the top of the 24-bit space + [InlineData(0xFFFFFFu, 2)] // one byte past the last address + [InlineData(0xFFFFFFu, int.MaxValue)] + public void ReadFlash_RejectsSpansThatWouldWrapTheAddress(uint offset, int length) + { + // Without the bounds check, offset + read wraps past 32 bits and the chunk silently + // targets the wrong flash address — returning plausible data with no error at all. + var port = CreateReadyPort(); + + var ex = Assert.Throws( + () => CreateReader(port).ReadFlash(offset, length)); + + Assert.Equal("length", ex.ParamName); + Assert.Empty(port.ReceivedHeaders); + } + + [Fact] + public void ReadFlash_AcceptsASpanEndingExactlyAtTheLastAddress() + { + // Boundary: the final byte is addressable and must not be rejected. + var port = CreateReadyPort(); + port.Blocks[ShareMemoryBase] = new byte[16]; + + var data = CreateReader(port).ReadFlash(WincFlashReader.MaxFlashAddress - 15, 16); + + Assert.Equal(16, data.Length); + } + [Fact] public void ReadFlash_ObservesCancellation() { @@ -191,6 +220,47 @@ public async Task Inspector_ReadIdentityAsync_ExplainsWhenNoBridgeIsListening() Assert.True(port.WasDisposed); } + [Fact] + public async Task Inspector_AbandonsAnOpenThatHangs_RatherThanBlockingForever() + { + // SerialPort.Open takes no cancellation token and can block indefinitely on a wedged or + // half-enumerated USB CDC device. The only way to stay responsive is a hard deadline. + var port = new HangingOpenPort(); + var inspector = new WincModuleInspector( + (_, _) => port, + baudSettleDelay: TimeSpan.Zero, + openTimeout: TimeSpan.FromMilliseconds(150)); + + var sw = System.Diagnostics.Stopwatch.StartNew(); + var ex = await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); + sw.Stop(); + + Assert.Contains("did not complete", ex.Message, StringComparison.OrdinalIgnoreCase); + Assert.True(sw.Elapsed < TimeSpan.FromSeconds(5), $"took {sw.Elapsed}, should have bailed at the deadline"); + + // The abandoned open still owns the handle, so this side must NOT have disposed it. + Assert.False(port.DisposedWhileOpenStillBlocked); + + port.ReleaseOpen(); + } + + [Fact] + public async Task Inspector_ObservesCancellationDuringAHangingOpen() + { + var port = new HangingOpenPort(); + var inspector = new WincModuleInspector( + (_, _) => port, + baudSettleDelay: TimeSpan.Zero, + openTimeout: TimeSpan.FromMinutes(5)); + + using var cts = new CancellationTokenSource(TimeSpan.FromMilliseconds(150)); + + await Assert.ThrowsAnyAsync( + () => inspector.ReadIdentityAsync("COM1", cts.Token)); + + port.ReleaseOpen(); + } + [Fact] public async Task Inspector_ReadIdentityAsync_RejectsAnEmptyPortName() { @@ -263,6 +333,39 @@ public void Locator_IsNotAvailable_WhenTheToolIsMissing() } } + [Fact] + public void Locator_TryResolve_PropagatesAnUnreadableTree_RatherThanReportingNotFound() + { + // "Could not locate the tool - WiFi flashing is Windows-only" is genuinely misleading when + // the tool is sitting right there behind a permissions problem, so this case must surface. + if (OperatingSystem.IsWindows()) + { + return; // chmod semantics differ; the behavior under test is the catch removal itself. + } + + var root = Path.Combine(Path.GetTempPath(), $"winc_{Guid.NewGuid():N}"); + var locked = Path.Combine(root, "locked"); + Directory.CreateDirectory(locked); + File.WriteAllText(Path.Combine(locked, "winc_flash_tool.cmd"), "@echo off"); + + try + { + File.SetUnixFileMode(locked, UnixFileMode.None); + + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + + Assert.ThrowsAny(() => locator.TryResolveToolPath(root, out _)); + + // IsAvailable stays total: a probe answers yes/no and must not throw. + Assert.False(locator.IsAvailable(root)); + } + finally + { + File.SetUnixFileMode(locked, UnixFileMode.UserRead | UnixFileMode.UserWrite | UnixFileMode.UserExecute); + Directory.Delete(root, recursive: true); + } + } + [Fact] public void Locator_IsNotAvailable_ForAPathThatDoesNotExist() { @@ -271,6 +374,46 @@ public void Locator_IsNotAvailable_ForAPathThatDoesNotExist() Assert.False(locator.IsAvailable(Path.Combine(Path.GetTempPath(), $"missing_{Guid.NewGuid():N}"))); } + /// + /// A port whose blocks until released, standing in for the real + /// SerialPort.Open() hang on a wedged or half-enumerated USB CDC device. + /// + private sealed class HangingOpenPort : IWincSerialPort + { + private readonly ManualResetEventSlim _release = new(false); + + /// True if something disposed the port while the open was still blocked. + internal bool DisposedWhileOpenStillBlocked { get; private set; } + + public bool IsOpen { get; private set; } + public int BaudRate { get; set; } = 115200; + + public void Open() + { + _release.Wait(); + IsOpen = true; + } + + internal void ReleaseOpen() => _release.Set(); + + public void Close() => IsOpen = false; + public void DiscardInBuffer() { } + public void Write(byte[] buffer, int offset, int count) { } + + public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) + => throw new TimeoutException("Port never opened."); + + public void Dispose() + { + if (!_release.IsSet) + { + DisposedWhileOpenStillBlocked = true; + } + + _release.Set(); + } + } + [Fact] public void Locator_RejectsAnEmptyToolName() { diff --git a/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs b/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs index b602669..1f040eb 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs @@ -1,6 +1,3 @@ -using Microsoft.Extensions.Logging; -using Microsoft.Extensions.Logging.Abstractions; - namespace Daqifi.Core.Firmware.Winc; /// @@ -49,18 +46,19 @@ internal sealed class WincFlashReader /// Bit that starts a flash-controller transfer. private const uint CommandStartBit = 1u << 7; + /// + /// Highest addressable flash byte. The fast-read command carries a 24-bit address, so this is + /// what the protocol can express regardless of the part's actual capacity. + /// + internal const uint MaxFlashAddress = 0xFFFFFF; + private readonly WincSerialBridgeClient _bridge; - private readonly ILogger _logger; private readonly int _transferPollLimit; - internal WincFlashReader( - WincSerialBridgeClient bridge, - int transferPollLimit = 1000, - ILogger? logger = null) + internal WincFlashReader(WincSerialBridgeClient bridge, int transferPollLimit = 1000) { _bridge = bridge ?? throw new ArgumentNullException(nameof(bridge)); _transferPollLimit = transferPollLimit; - _logger = logger ?? NullLogger.Instance; } /// @@ -107,6 +105,19 @@ internal byte[] ReadFlash(uint offset, int length, CancellationToken cancellatio throw new ArgumentOutOfRangeException(nameof(length), length, "Read length must be positive."); } + // Bounds-check in 64-bit before any address arithmetic. Without this the per-chunk + // `offset + read` would silently wrap past 32 bits and quietly read the wrong part of + // flash — returning plausible-looking data with no error at all. + var endExclusive = (long)offset + length; + if (endExclusive > (long)MaxFlashAddress + 1) + { + throw new ArgumentOutOfRangeException( + nameof(length), + length, + $"A read of {length} bytes at 0x{offset:X6} would end at 0x{endExclusive:X}, past the " + + $"highest addressable flash byte 0x{MaxFlashAddress:X6}."); + } + var result = new byte[length]; var read = 0; @@ -115,6 +126,7 @@ internal byte[] ReadFlash(uint offset, int length, CancellationToken cancellatio cancellationToken.ThrowIfCancellationRequested(); var chunk = Math.Min(WincBridgeProtocol.MaxReadBlockSize, length - read); + // Safe: the bounds check above guarantees offset + read stays within 24 bits. var chunkData = ReadFlashChunk((uint)(offset + read), chunk); Buffer.BlockCopy(chunkData, 0, result, read, chunk); read += chunk; diff --git a/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs b/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs index e0bef87..2f84a87 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs @@ -32,14 +32,38 @@ public WincFlashToolLocator(string toolFileName) } /// - /// Whether the flash tool is present for the given firmware path. + /// Whether the flash tool is present for the given firmware path. Total by design — a probe + /// answers yes or no, so an unreadable tree reports false rather than throwing. /// - public bool IsAvailable(string firmwarePath) => TryResolveToolPath(firmwarePath, out _); + /// + /// Use when a caller is about to act on the answer and needs + /// to tell "not there" apart from "could not look". + /// + public bool IsAvailable(string firmwarePath) + { + try + { + return TryResolveToolPath(firmwarePath, out _); + } + catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + { + return false; + } + } /// /// Resolves the flash tool for a firmware path: the path itself when it is the tool, otherwise /// the first match found beneath it. /// + /// true when the tool was found. + /// The directory tree could not be searched. + /// The directory tree could not be read. + /// + /// A tree that exists but cannot be read propagates rather than returning false. + /// Collapsing it into "not found" is what produces the genuinely misleading + /// "could not locate the tool — WiFi flashing is Windows-only" message on a machine where the + /// tool is sitting right there behind a permissions problem. + /// public bool TryResolveToolPath(string firmwarePath, [NotNullWhen(true)] out string? toolPath) { toolPath = null; @@ -60,21 +84,13 @@ public bool TryResolveToolPath(string firmwarePath, [NotNullWhen(true)] out stri return false; } - try - { - var matches = Directory.GetFiles(firmwarePath, _toolFileName, SearchOption.AllDirectories); - if (matches.Length == 0) - { - return false; - } - - toolPath = matches[0]; - return true; - } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + var matches = Directory.GetFiles(firmwarePath, _toolFileName, SearchOption.AllDirectories); + if (matches.Length == 0) { - // An unreadable tree is indistinguishable from a missing tool for availability purposes. return false; } + + toolPath = matches[0]; + return true; } } diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs index fba3d43..4dd376c 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs @@ -43,6 +43,7 @@ public sealed class WincModuleInspector private readonly ILogger _logger; private readonly TimeSpan _baudSettleDelay; private readonly TimeSpan _responseTimeout; + private readonly TimeSpan _openTimeout; /// /// Creates an inspector that opens real serial ports. @@ -59,12 +60,14 @@ internal WincModuleInspector( Func portFactory, ILogger? logger = null, TimeSpan? baudSettleDelay = null, - TimeSpan? responseTimeout = null) + TimeSpan? responseTimeout = null, + TimeSpan? openTimeout = null) { _portFactory = portFactory ?? throw new ArgumentNullException(nameof(portFactory)); _logger = logger ?? NullLogger.Instance; _baudSettleDelay = baudSettleDelay ?? TimeSpan.FromMilliseconds(100); _responseTimeout = responseTimeout ?? TimeSpan.FromSeconds(2); + _openTimeout = openTimeout ?? TimeSpan.FromSeconds(5); } /// @@ -90,7 +93,12 @@ private WincModuleIdentity ReadIdentity(string portName, CancellationToken cance { using var session = OpenSession(portName, cancellationToken); + // Each bridge exchange is individually bounded by the response timeout, but a caller who + // cancels should not have to wait out the remaining ones. + cancellationToken.ThrowIfCancellationRequested(); var chipId = session.Reader.ReadChipId(); + + cancellationToken.ThrowIfCancellationRequested(); var flashId = session.Reader.ReadFlashJedecId(); _logger.LogInformation( @@ -127,9 +135,14 @@ private BridgeSession OpenSession(string portName, CancellationToken cancellatio cancellationToken.ThrowIfCancellationRequested(); var port = _portFactory(portName, InitialBaudRate); + + // Ownership flag: once the open has been abandoned it is still running on a pool thread + // and holding the handle, so disposing here would race it. In that case the abandonment + // continuation owns disposal instead. + var ownsPort = true; try { - port.Open(); + OpenWithTimeout(port, portName, ref ownsPort, cancellationToken); var bridge = new WincSerialBridgeClient(port, _responseTimeout, _logger); @@ -140,7 +153,7 @@ private BridgeSession OpenSession(string portName, CancellationToken cancellatio "firmware-update (bridge) mode before the module can be reached."); } - bridge.ChangeBaudRate(FastBaudRate, _baudSettleDelay); + bridge.ChangeBaudRate(FastBaudRate, _baudSettleDelay, cancellationToken); // Re-handshake at the new rate: this both confirms the switch actually took and leaves // the bridge back in its op-code state before any command is issued. @@ -151,11 +164,68 @@ private BridgeSession OpenSession(string portName, CancellationToken cancellatio $"{FastBaudRate} baud."); } - return new BridgeSession(port, new WincFlashReader(bridge, logger: _logger)); + return new BridgeSession(port, new WincFlashReader(bridge)); } catch { - port.Dispose(); + if (ownsPort) + { + port.Dispose(); + } + + throw; + } + } + + /// + /// Opens the port under a hard deadline. + /// + /// + /// can block indefinitely on a wedged or + /// half-enumerated USB CDC device and takes no cancellation token, so the only way to stay + /// responsive is to run it elsewhere and walk away from it. An abandoned open keeps running on + /// a pool thread and still owns the handle, so disposal is handed to a continuation rather than + /// done here — disposing underneath a blocked open is how you turn a hang into a crash. + /// is cleared on that path so the caller does not double-dispose. + /// + private void OpenWithTimeout( + IWincSerialPort port, + string portName, + ref bool ownsPort, + CancellationToken cancellationToken) + { + var openTask = Task.Run(port.Open, CancellationToken.None); + + try + { + openTask.WaitAsync(_openTimeout, cancellationToken).GetAwaiter().GetResult(); + } + catch (Exception ex) when (ex is TimeoutException or OperationCanceledException) + { + ownsPort = false; + openTask.ContinueWith( + _ => + { + try + { + port.Dispose(); + } + catch (Exception) + { + // Best-effort cleanup of a port we already gave up on. + } + }, + CancellationToken.None, + TaskContinuationOptions.ExecuteSynchronously, + TaskScheduler.Default); + + if (ex is TimeoutException) + { + throw new TimeoutException( + $"Opening serial port {portName} did not complete within {_openTimeout}. The port " + + "may be held by another process, or the device may be half-enumerated."); + } + throw; } } diff --git a/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs b/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs index 7492112..3946b19 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincSerialBridgeClient.cs @@ -148,7 +148,10 @@ internal void WriteBlock(uint address, byte[] data) /// host must follow immediately; anything sent in between is lost. The settle delay gives the /// device's UART time to reconfigure before the next byte arrives. /// - internal void ChangeBaudRate(int newBaudRate, TimeSpan settleDelay) + internal void ChangeBaudRate( + int newBaudRate, + TimeSpan settleDelay, + CancellationToken cancellationToken = default) { if (newBaudRate <= 0) { @@ -161,7 +164,10 @@ internal void ChangeBaudRate(int newBaudRate, TimeSpan settleDelay) if (settleDelay > TimeSpan.Zero) { - Thread.Sleep(settleDelay); + // Cancellable rather than Thread.Sleep so a cancel during the settle window is + // observed instead of being slept through. + cancellationToken.WaitHandle.WaitOne(settleDelay); + cancellationToken.ThrowIfCancellationRequested(); } _port.DiscardInBuffer(); From 36144dc538446a1a130a72b4d88496f62864b35d Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 13:45:01 -0600 Subject: [PATCH 3/8] fix(firmware): observe the abandoned open's fault and make IsAvailable 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 --- .../Firmware/Winc/WincFlasherTests.cs | 95 +++++++++++++++++++ .../Firmware/Winc/WincFlashToolLocator.cs | 24 ++++- .../Firmware/Winc/WincModuleInspector.cs | 15 ++- 3 files changed, 128 insertions(+), 6 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 69f1c1f..c6eb95e 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -244,6 +244,48 @@ public async Task Inspector_AbandonsAnOpenThatHangs_RatherThanBlockingForever() port.ReleaseOpen(); } + [Fact] + public async Task Inspector_ObservesTheFaultOfAnAbandonedOpen() + { + // The abandoned open is no longer awaited by anyone, so if it later faults nothing would + // observe the exception - a silently swallowed background failure (#377/#394). The + // continuation must read it. Verified via the unobserved-exception hook: after forcing a + // GC, no unobserved fault should have been raised for our exception. + var unobserved = new List(); + void Handler(object? _, UnobservedTaskExceptionEventArgs e) + { + if (e.Exception?.InnerException is InvalidOperationException { Message: "abandoned-open-fault" }) + { + unobserved.Add(e.Exception); + } + } + + TaskScheduler.UnobservedTaskException += Handler; + try + { + var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(300)); + var inspector = new WincModuleInspector( + (_, _) => port, + baudSettleDelay: TimeSpan.Zero, + openTimeout: TimeSpan.FromMilliseconds(50)); + + await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); + + // Let the abandoned open run to its fault, then force finalization. + await Task.Delay(TimeSpan.FromMilliseconds(600)); + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + + Assert.Empty(unobserved); + Assert.True(port.WasDisposedByContinuation); + } + finally + { + TaskScheduler.UnobservedTaskException -= Handler; + } + } + [Fact] public async Task Inspector_ObservesCancellationDuringAHangingOpen() { @@ -414,6 +456,59 @@ public void Dispose() } } + [Theory] + [InlineData("\0invalid")] // embedded NUL - ArgumentException, not IO + [InlineData(" ")] + [InlineData("")] + public void Locator_IsAvailable_IsTotal_EvenForMalformedPaths(string path) + { + // IsAvailable is documented as never throwing. A narrow IO-only catch would let an + // ArgumentException from a malformed path escape a probe whose entire purpose is to be + // safe to call with anything. + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + + var ex = Record.Exception(() => locator.IsAvailable(path)); + + Assert.Null(ex); + Assert.False(locator.IsAvailable(path)); + } + + [Fact] + public void Locator_IsAvailable_IsTotal_ForAnAbsurdlyLongPath() + { + var locator = new WincFlashToolLocator("winc_flash_tool.cmd"); + var longPath = "/" + new string('x', 40_000); + + Assert.Null(Record.Exception(() => locator.IsAvailable(longPath))); + } + + /// + /// A port whose blocks briefly and then throws, standing in for an open that + /// is abandoned on the timeout and only fails afterwards. + /// + private sealed class FaultingAfterDelayPort(TimeSpan delay) : IWincSerialPort + { + internal bool WasDisposedByContinuation { get; private set; } + + public bool IsOpen => false; + public int BaudRate { get; set; } = 115200; + + public void Open() + { + Thread.Sleep(delay); + throw new InvalidOperationException("abandoned-open-fault"); + } + + public void Close() { } + public void DiscardInBuffer() { } + public void Write(byte[] buffer, int offset, int count) { } + + public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) + => throw new TimeoutException("Port never opened."); + + public void Dispose() => WasDisposedByContinuation = true; + } + [Fact] public void Locator_RejectsAnEmptyToolName() { diff --git a/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs b/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs index 2f84a87..b9093fa 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincFlashToolLocator.cs @@ -32,12 +32,23 @@ public WincFlashToolLocator(string toolFileName) } /// - /// Whether the flash tool is present for the given firmware path. Total by design — a probe - /// answers yes or no, so an unreadable tree reports false rather than throwing. + /// Whether the flash tool is present for the given firmware path. /// + /// + /// true when the tool was found; false for every other outcome, including a path + /// that does not exist, a malformed or too-long path, and a tree that exists but cannot be read. + /// /// - /// Use when a caller is about to act on the answer and needs - /// to tell "not there" apart from "could not look". + /// + /// Total by design, and the implementation matches: a capability probe answers yes or no, and + /// callers reach for it precisely when they do not want to reason about filesystem failure + /// modes. Catching narrowly would leave a UI or a decision path crashing on a malformed string. + /// + /// + /// Because it cannot distinguish "not there" from "could not look", use + /// when a caller is about to act on the answer and needs the + /// real reason. + /// /// public bool IsAvailable(string firmwarePath) { @@ -45,8 +56,11 @@ public bool IsAvailable(string firmwarePath) { return TryResolveToolPath(firmwarePath, out _); } - catch (Exception ex) when (ex is IOException or UnauthorizedAccessException) + catch (Exception) { + // Deliberately broad: the documented contract is that this never throws. Narrowing it + // to the IO family would let an ArgumentException from a malformed path escape a probe + // whose whole purpose is to be safe to call with anything. return false; } } diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs index 4dd376c..b3e1599 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs @@ -204,8 +204,21 @@ private void OpenWithTimeout( { ownsPort = false; openTask.ContinueWith( - _ => + completed => { + // Observe the abandoned open's fault. Nobody awaits this task any more, so an + // unobserved exception here would be exactly the silently-swallowed background + // failure #377/#394 set out to eliminate. Reading Exception marks it observed; + // surfacing it at Debug keeps the diagnostic without implying the caller needs + // to act — they already got a TimeoutException or a cancellation. + if (completed.Exception is { } fault) + { + _logger.LogDebug( + fault, + "Abandoned open of {PortName} faulted after it was given up on.", + portName); + } + try { port.Dispose(); From 4ff7558763cf7d0a0750029d293b147b802ea918 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 13:53:17 -0600 Subject: [PATCH 4/8] test(firmware): assert the abandoned-open fault deterministically instead 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 --- .../Firmware/Winc/WincFlasherTests.cs | 69 +++++++++---------- .../Firmware/Winc/WincModuleInspector.cs | 11 ++- 2 files changed, 42 insertions(+), 38 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index c6eb95e..95051b5 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -248,42 +248,33 @@ public async Task Inspector_AbandonsAnOpenThatHangs_RatherThanBlockingForever() public async Task Inspector_ObservesTheFaultOfAnAbandonedOpen() { // The abandoned open is no longer awaited by anyone, so if it later faults nothing would - // observe the exception - a silently swallowed background failure (#377/#394). The - // continuation must read it. Verified via the unobserved-exception hook: after forcing a - // GC, no unobserved fault should have been raised for our exception. - var unobserved = new List(); - void Handler(object? _, UnobservedTaskExceptionEventArgs e) - { - if (e.Exception?.InnerException is InvalidOperationException { Message: "abandoned-open-fault" }) - { - unobserved.Add(e.Exception); - } - } + // observe the exception - a silently swallowed background failure (#377/#394). + // + // Asserted at the seam rather than through TaskScheduler.UnobservedTaskException plus a + // forced GC. That route depends on collector and finalizer timing, which is exactly the + // kind of nondeterminism that produces a flaky CI gate, and the event is process-global so + // it couples this test to every other task in a parallel run. Reading Task.Exception is + // what marks a fault observed, so a hook receiving that exception proves the read happened + // - the same property, decided by the code under test instead of by the runtime. + var faultObserved = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + + var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(150)); + var inspector = new WincModuleInspector( + (_, _) => port, + baudSettleDelay: TimeSpan.Zero, + openTimeout: TimeSpan.FromMilliseconds(50), + abandonedOpenFaultObserver: ex => faultObserved.TrySetResult(ex)); - TaskScheduler.UnobservedTaskException += Handler; - try - { - var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(300)); - var inspector = new WincModuleInspector( - (_, _) => port, - baudSettleDelay: TimeSpan.Zero, - openTimeout: TimeSpan.FromMilliseconds(50)); - - await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); - - // Let the abandoned open run to its fault, then force finalization. - await Task.Delay(TimeSpan.FromMilliseconds(600)); - GC.Collect(); - GC.WaitForPendingFinalizers(); - GC.Collect(); - - Assert.Empty(unobserved); - Assert.True(port.WasDisposedByContinuation); - } - finally - { - TaskScheduler.UnobservedTaskException -= Handler; - } + await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); + + var observed = await faultObserved.Task.WaitAsync(TimeSpan.FromSeconds(10)); + + Assert.IsType( + Assert.IsType(observed).InnerException); + + // The continuation still owns disposal of the port it took over. + await port.Disposed.WaitAsync(TimeSpan.FromSeconds(10)); } [Fact] @@ -488,7 +479,11 @@ public void Locator_IsAvailable_IsTotal_ForAnAbsurdlyLongPath() /// private sealed class FaultingAfterDelayPort(TimeSpan delay) : IWincSerialPort { - internal bool WasDisposedByContinuation { get; private set; } + private readonly TaskCompletionSource _disposed = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + /// Completes when the port is disposed, so tests can await it deterministically. + internal Task Disposed => _disposed.Task; public bool IsOpen => false; public int BaudRate { get; set; } = 115200; @@ -506,7 +501,7 @@ public void Write(byte[] buffer, int offset, int count) { } public void ReadExactly(byte[] buffer, int offset, int count, TimeSpan timeout) => throw new TimeoutException("Port never opened."); - public void Dispose() => WasDisposedByContinuation = true; + public void Dispose() => _disposed.TrySetResult(); } [Fact] diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs index b3e1599..9339a85 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs @@ -45,6 +45,12 @@ public sealed class WincModuleInspector private readonly TimeSpan _responseTimeout; private readonly TimeSpan _openTimeout; + // Test seam. The abandonment path is fire-and-forget by construction, so without a way to + // observe it from outside, a test can only infer that it ran from GC/finalizer timing - which + // is nondeterministic and makes a flaky CI gate. This makes "the fault was observed" directly + // assertable. Null in production, where the Debug log is the only output. + private readonly Action? _abandonedOpenFaultObserver; + /// /// Creates an inspector that opens real serial ports. /// @@ -61,13 +67,15 @@ internal WincModuleInspector( ILogger? logger = null, TimeSpan? baudSettleDelay = null, TimeSpan? responseTimeout = null, - TimeSpan? openTimeout = null) + TimeSpan? openTimeout = null, + Action? abandonedOpenFaultObserver = null) { _portFactory = portFactory ?? throw new ArgumentNullException(nameof(portFactory)); _logger = logger ?? NullLogger.Instance; _baudSettleDelay = baudSettleDelay ?? TimeSpan.FromMilliseconds(100); _responseTimeout = responseTimeout ?? TimeSpan.FromSeconds(2); _openTimeout = openTimeout ?? TimeSpan.FromSeconds(5); + _abandonedOpenFaultObserver = abandonedOpenFaultObserver; } /// @@ -217,6 +225,7 @@ private void OpenWithTimeout( fault, "Abandoned open of {PortName} faulted after it was given up on.", portName); + _abandonedOpenFaultObserver?.Invoke(fault); } try From 53a6127cedd81ea4c731272b4ac9a5385469d505 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 14:00:46 -0600 Subject: [PATCH 5/8] fix(firmware): make disposal of an abandoned port unreachable-past, and 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 --- .../Firmware/Winc/WincFlasherTests.cs | 90 ++++++++++++++++--- .../Firmware/Winc/WincModuleInspector.cs | 56 +++++++----- 2 files changed, 108 insertions(+), 38 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 95051b5..4b64317 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -1,4 +1,5 @@ using Daqifi.Core.Firmware.Winc; +using Microsoft.Extensions.Logging; namespace Daqifi.Core.Tests.Firmware.Winc; @@ -250,33 +251,52 @@ public async Task Inspector_ObservesTheFaultOfAnAbandonedOpen() // The abandoned open is no longer awaited by anyone, so if it later faults nothing would // observe the exception - a silently swallowed background failure (#377/#394). // - // Asserted at the seam rather than through TaskScheduler.UnobservedTaskException plus a - // forced GC. That route depends on collector and finalizer timing, which is exactly the - // kind of nondeterminism that produces a flaky CI gate, and the event is process-global so - // it couples this test to every other task in a parallel run. Reading Task.Exception is - // what marks a fault observed, so a hook receiving that exception proves the read happened - // - the same property, decided by the code under test instead of by the runtime. - var faultObserved = new TaskCompletionSource( - TaskCreationOptions.RunContinuationsAsynchronously); - + // Asserted through the logger rather than TaskScheduler.UnobservedTaskException plus a + // forced GC. That route depends on collector and finalizer timing — nondeterminism that + // makes a flaky CI gate — and the event is process-global, coupling this test to every + // other task in a parallel run. + // + // Reading Task.Exception is what marks a fault observed, and production already reads it + // to hand to the logger. So a logger that captures the exception proves the read happened, + // using a seam that exists for production reasons rather than a test-only hook. + var logger = new CapturingLogger(); var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(150)); var inspector = new WincModuleInspector( (_, _) => port, + logger, baudSettleDelay: TimeSpan.Zero, - openTimeout: TimeSpan.FromMilliseconds(50), - abandonedOpenFaultObserver: ex => faultObserved.TrySetResult(ex)); + openTimeout: TimeSpan.FromMilliseconds(50)); await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); - var observed = await faultObserved.Task.WaitAsync(TimeSpan.FromSeconds(10)); + var observed = await logger.FirstException.WaitAsync(TimeSpan.FromSeconds(10)); - Assert.IsType( - Assert.IsType(observed).InnerException); + Assert.IsType(observed); + Assert.IsType(((AggregateException)observed).InnerException); // The continuation still owns disposal of the port it took over. await port.Disposed.WaitAsync(TimeSpan.FromSeconds(10)); } + [Fact] + public async Task Inspector_DisposesTheAbandonedPort_EvenWhenObservingTheFaultThrows() + { + // Releasing the handle is the whole reason the abandonment continuation exists. If a + // throwing logger could skip past the disposal, the abandoned port would leak and break + // every later open until the process exits — strictly worse than the fault being reported. + var port = new FaultingAfterDelayPort(TimeSpan.FromMilliseconds(150)); + var inspector = new WincModuleInspector( + (_, _) => port, + new ThrowingLogger(), + baudSettleDelay: TimeSpan.Zero, + openTimeout: TimeSpan.FromMilliseconds(50)); + + await Assert.ThrowsAsync(() => inspector.ReadIdentityAsync("COM1")); + + // Disposal must still happen despite the observation throwing. + await port.Disposed.WaitAsync(TimeSpan.FromSeconds(10)); + } + [Fact] public async Task Inspector_ObservesCancellationDuringAHangingOpen() { @@ -473,6 +493,48 @@ public void Locator_IsAvailable_IsTotal_ForAnAbsurdlyLongPath() Assert.Null(Record.Exception(() => locator.IsAvailable(longPath))); } + /// Captures logged exceptions, exposing the first one as an awaitable. + private sealed class CapturingLogger : ILogger + { + private readonly TaskCompletionSource _firstException = + new(TaskCreationOptions.RunContinuationsAsynchronously); + + internal Task FirstException => _firstException.Task; + + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + { + if (exception is not null) + { + _firstException.TrySetResult(exception); + } + } + } + + /// A logger that throws, standing in for a misbehaving logging pipeline. + private sealed class ThrowingLogger : ILogger + { + public IDisposable? BeginScope(TState state) where TState : notnull => null; + + public bool IsEnabled(LogLevel logLevel) => true; + + public void Log( + LogLevel logLevel, + EventId eventId, + TState state, + Exception? exception, + Func formatter) + => throw new InvalidOperationException("logger-boom"); + } + /// /// A port whose blocks briefly and then throws, standing in for an open that /// is abandoned on the timeout and only fails afterwards. diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs index 9339a85..3f8cc1d 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs @@ -45,11 +45,6 @@ public sealed class WincModuleInspector private readonly TimeSpan _responseTimeout; private readonly TimeSpan _openTimeout; - // Test seam. The abandonment path is fire-and-forget by construction, so without a way to - // observe it from outside, a test can only infer that it ran from GC/finalizer timing - which - // is nondeterministic and makes a flaky CI gate. This makes "the fault was observed" directly - // assertable. Null in production, where the Debug log is the only output. - private readonly Action? _abandonedOpenFaultObserver; /// /// Creates an inspector that opens real serial ports. @@ -67,15 +62,13 @@ internal WincModuleInspector( ILogger? logger = null, TimeSpan? baudSettleDelay = null, TimeSpan? responseTimeout = null, - TimeSpan? openTimeout = null, - Action? abandonedOpenFaultObserver = null) + TimeSpan? openTimeout = null) { _portFactory = portFactory ?? throw new ArgumentNullException(nameof(portFactory)); _logger = logger ?? NullLogger.Instance; _baudSettleDelay = baudSettleDelay ?? TimeSpan.FromMilliseconds(100); _responseTimeout = responseTimeout ?? TimeSpan.FromSeconds(2); _openTimeout = openTimeout ?? TimeSpan.FromSeconds(5); - _abandonedOpenFaultObserver = abandonedOpenFaultObserver; } /// @@ -214,27 +207,42 @@ private void OpenWithTimeout( openTask.ContinueWith( completed => { - // Observe the abandoned open's fault. Nobody awaits this task any more, so an - // unobserved exception here would be exactly the silently-swallowed background - // failure #377/#394 set out to eliminate. Reading Exception marks it observed; - // surfacing it at Debug keeps the diagnostic without implying the caller needs - // to act — they already got a TimeoutException or a cancellation. - if (completed.Exception is { } fault) - { - _logger.LogDebug( - fault, - "Abandoned open of {PortName} faulted after it was given up on.", - portName); - _abandonedOpenFaultObserver?.Invoke(fault); - } - + // Releasing the handle is the entire reason this continuation exists — the + // abandoned open owns the port and nothing else will ever free it. So disposal + // sits in a finally that nothing above can be reachable-past: a throwing logger + // would otherwise leak the handle and break every later open until the process + // exits, which is a strictly worse outcome than the fault it was reporting. try { - port.Dispose(); + // Observe the abandoned open's fault. Nobody awaits this task any more, so + // an unobserved exception here would be exactly the silently-swallowed + // background failure #377/#394 set out to eliminate. Reading Exception is + // what marks it observed; Debug keeps the diagnostic without implying the + // caller must act — they already got a TimeoutException or a cancellation. + if (completed.Exception is { } fault) + { + _logger.LogDebug( + fault, + "Abandoned open of {PortName} faulted after it was given up on.", + portName); + } } catch (Exception) { - // Best-effort cleanup of a port we already gave up on. + // Swallowed rather than propagated: nothing awaits this continuation + // either, so rethrowing would recreate the unobserved fault this whole + // path exists to avoid. + } + finally + { + try + { + port.Dispose(); + } + catch (Exception) + { + // Best-effort cleanup of a port we already gave up on. + } } }, CancellationToken.None, From 6555354c666589aca1349c4f617eea334f1cc122 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 14:07:21 -0600 Subject: [PATCH 6/8] perf(firmware): hoist the logger so the abandoned-open continuation stops 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 --- src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs index 3f8cc1d..2de9fb8 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincModuleInspector.cs @@ -204,6 +204,14 @@ private void OpenWithTimeout( catch (Exception ex) when (ex is TimeoutException or OperationCanceledException) { ownsPort = false; + + // Hoisted so the continuation closes over the logger rather than `this`. An open that + // never returns keeps this continuation — and everything it captured — alive for the + // life of the process, and it only needs the port and the logger, not the whole + // inspector. `port` and `portName` are already parameters, so this removes the last + // reach through the instance. + var logger = _logger; + openTask.ContinueWith( completed => { @@ -221,7 +229,7 @@ private void OpenWithTimeout( // caller must act — they already got a TimeoutException or a cancellation. if (completed.Exception is { } fault) { - _logger.LogDebug( + logger.LogDebug( fault, "Abandoned open of {PortName} faulted after it was given up on.", portName); From 77d7de2442974e5592df66e9a323c615155a7188 Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 19:29:24 -0600 Subject: [PATCH 7/8] fix(firmware): correct the flash JEDEC read sequence, caught by the live 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 --- .../Firmware/Winc/WincFlasherTests.cs | 41 +++++++++++++++++++ .../Firmware/Winc/WincFlashReader.cs | 10 ++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 4b64317..3f259e8 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -110,6 +110,47 @@ public void ReadFlashJedecId_ReturnsTheControllerResult() Assert.Equal(0x00C22018u, CreateReader(port).ReadFlashJedecId()); } + [Fact] + public void ReadFlashJedecId_WritesTheExactRegisterSequenceTheWincDriverUses() + { + // Pinned register-by-register against the WINC driver's spi_flash_rdid. + // + // This exists because the bench caught what framing tests structurally could not: the + // original code wrote DATA_CNT 0 and DMA_ADDR 0, so no result bytes were clocked back and + // the read returned 0x00000000 from a live module. Every frame was well-formed and the + // device ACKed every one, so nothing about the protocol layer was wrong - only the values. + // Asserting the sequence, not just the return, is what catches that class of bug. + var port = CreateReadyPort(); + port.Registers[DummyRegister] = 0x00C22018; + + CreateReader(port).ReadFlashJedecId(); + + var writes = RegisterWrites(port); + + Assert.Equal(4u, writes[0x10208]); // DATA_CNT - 4 result bytes + Assert.Equal(0x9Fu, writes[0x1020C]); // BUF1 - RDID opcode + Assert.Equal(0x01u, writes[0x10214]); // BUF_DIR + Assert.Equal(DummyRegister, writes[0x1021C]); // DMA_ADDR - where the result lands + Assert.Equal(1u | (1u << 7), writes[0x10204]); // CMD_CNT - 1 command byte, start bit + } + + /// + /// Extracts address -> value for every WriteRegister command the client sent. + /// + private static Dictionary RegisterWrites(FakeWincSerialPort port) + { + var writes = new Dictionary(); + + foreach (var h in port.ReceivedHeaders.Where(h => h[0] == (byte)WincBridgeProtocol.Command.WriteRegister)) + { + var address = ((uint)h[7] << 24) | ((uint)h[6] << 16) | ((uint)h[5] << 8) | h[4]; + var value = ((uint)h[11] << 24) | ((uint)h[10] << 16) | ((uint)h[9] << 8) | h[8]; + writes[address] = value; + } + + return writes; + } + [Fact] public void ReadFlash_ThrowsWhenTheControllerNeverReportsDone() { diff --git a/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs b/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs index 1f040eb..99c5bcb 100644 --- a/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs +++ b/src/Daqifi.Core/Firmware/Winc/WincFlashReader.cs @@ -82,10 +82,16 @@ internal static bool IsKnownWincChipId(uint chipId) /// internal uint ReadFlashJedecId() { - _bridge.WriteRegister(RegDataCount, 0); + // DATA_CNT is the number of result bytes to clock back, and DMA_ADDR is where they land. + // Both matter: with DATA_CNT 0 and DMA_ADDR 0 the command still completes and TR_DONE still + // goes high, but nothing is transferred and the result register reads 0x00000000. That is + // what the bench returned before this was corrected against the WINC driver's + // spi_flash_rdid — a bug no amount of framing tests could have caught, because every frame + // was well-formed and the device answered every one of them. + _bridge.WriteRegister(RegDataCount, 4); _bridge.WriteRegister(RegBuffer1, FlashCommandReadIdentification); _bridge.WriteRegister(RegBufferDirection, 0x01); - _bridge.WriteRegister(RegDmaAddress, 0); + _bridge.WriteRegister(RegDmaAddress, DummyRegister); _bridge.WriteRegister(RegCommandCount, 1 | CommandStartBit); WaitForTransferDone("read flash JEDEC id"); From d2a719ec37b9b5cec04924ef2071028c26d6c77f Mon Sep 17 00:00:00 2001 From: Tyler Kron Date: Sat, 1 Aug 2026 19:36:08 -0600 Subject: [PATCH 8/8] test(firmware): assert the WINC register writes as an ordered sequence, 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 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 --- .../Firmware/Winc/WincFlasherTests.cs | 69 ++++++++++++++----- 1 file changed, 50 insertions(+), 19 deletions(-) diff --git a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs index 3f259e8..8b1bad0 100644 --- a/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs +++ b/src/Daqifi.Core.Tests/Firmware/Winc/WincFlasherTests.cs @@ -8,7 +8,16 @@ namespace Daqifi.Core.Tests.Firmware.Winc; /// public class WincFlasherTests { + // Restated independently from the WINC driver's spi_flash.c rather than reused from + // production, so a typo in the production register map fails these tests instead of being + // silently agreed with. SPI_FLASH_BASE is 0x10200. + private const uint RegCommandCount = 0x10204; + private const uint RegDataCount = 0x10208; + private const uint RegBuffer1 = 0x1020C; + private const uint RegBuffer2 = 0x10210; + private const uint RegBufferDirection = 0x10214; private const uint TransferDoneRegister = 0x10218; + private const uint RegDmaAddress = 0x1021C; private const uint DummyRegister = 0x1084; private const uint ShareMemoryBase = 0xD0000; @@ -125,32 +134,54 @@ public void ReadFlashJedecId_WritesTheExactRegisterSequenceTheWincDriverUses() CreateReader(port).ReadFlashJedecId(); - var writes = RegisterWrites(port); - - Assert.Equal(4u, writes[0x10208]); // DATA_CNT - 4 result bytes - Assert.Equal(0x9Fu, writes[0x1020C]); // BUF1 - RDID opcode - Assert.Equal(0x01u, writes[0x10214]); // BUF_DIR - Assert.Equal(DummyRegister, writes[0x1021C]); // DMA_ADDR - where the result lands - Assert.Equal(1u | (1u << 7), writes[0x10204]); // CMD_CNT - 1 command byte, start bit + // Asserted as an ordered sequence, not a final-state map. Order is the protocol here: + // DATA_CNT and DMA_ADDR configure the transfer and CMD_CNT triggers it, so a map-based + // assertion would pass even if CMD_CNT fired first, or if a wrong value were written and + // silently corrected afterwards. + Assert.Equal( + [ + (RegDataCount, 4u), // 4 result bytes + (RegBuffer1, 0x9Fu), // RDID opcode + (RegBufferDirection, 0x01u), + (RegDmaAddress, DummyRegister), // where the result lands + (RegCommandCount, 1u | (1u << 7)) // 1 command byte + start bit — must be last + ], + RegisterWriteSequence(port)); } - /// - /// Extracts address -> value for every WriteRegister command the client sent. - /// - private static Dictionary RegisterWrites(FakeWincSerialPort port) + [Fact] + public void ReadFlash_WritesTheExactFastReadRegisterSequence() { - var writes = new Dictionary(); + // Same ordering guarantee on the path that actually carries data. The controller latches + // the staged configuration when CMD_CNT is written, so everything else must precede it. + var port = CreateReadyPort(); + port.Blocks[ShareMemoryBase] = new byte[16]; - foreach (var h in port.ReceivedHeaders.Where(h => h[0] == (byte)WincBridgeProtocol.Command.WriteRegister)) - { - var address = ((uint)h[7] << 24) | ((uint)h[6] << 16) | ((uint)h[5] << 8) | h[4]; - var value = ((uint)h[11] << 24) | ((uint)h[10] << 16) | ((uint)h[9] << 8) | h[8]; - writes[address] = value; - } + CreateReader(port).ReadFlash(0x123456, 16); - return writes; + Assert.Equal( + [ + (RegDataCount, 16u), + (RegBuffer1, 0x5634120Bu), // 0x0B opcode + 24-bit address ascending + (RegBuffer2, 0xA5u), // fast-read dummy byte + (RegBufferDirection, 0x1Fu), + (RegDmaAddress, ShareMemoryBase), + (RegCommandCount, 5u | (1u << 7)) // 5 command bytes + start bit — must be last + ], + RegisterWriteSequence(port)); } + /// + /// Every WriteRegister the client sent, in the order it was sent, as (address, value). + /// + private static List<(uint Address, uint Value)> RegisterWriteSequence(FakeWincSerialPort port) + => port.ReceivedHeaders + .Where(h => h[0] == (byte)WincBridgeProtocol.Command.WriteRegister) + .Select(h => ( + Address: ((uint)h[7] << 24) | ((uint)h[6] << 16) | ((uint)h[5] << 8) | h[4], + Value: ((uint)h[11] << 24) | ((uint)h[10] << 16) | ((uint)h[9] << 8) | h[8])) + .ToList(); + [Fact] public void ReadFlash_ThrowsWhenTheControllerNeverReportsDone() {