chore(firmware): the bootloader's wire constants were written down twice

[tylerkron]

What was wrong

The bytes that make up the PIC32 bootloader protocol — the start/end framing bytes, the escape byte, and the command opcodes — were written down twice, once in the class that sends messages to the bootloader and once in the class that reads its replies. Both copies were in use. Nothing checked that they still agreed, and nothing would have complained if they stopped: a one-sided edit would simply mis-frame a live firmware flash on real hardware, which is the worst place in this library to find out you have two sources of truth.

How it was fixed

The values now live in one internal Pic32BootloaderWireFormat class that both sides read from, unchanged byte for byte. The one thing worth pushing back on is that consolidating quietly removes a safety net: while there were two copies, a bad edit to one of them broke the other, and now a bad edit to the single copy just changes what Core puts on the wire. So the new tests restate the byte values explicitly — the second copy is now a test that has to be edited too, rather than a second copy in production code — and separately prove that the opcode the producer sends is the one the consumer expects back, without either value being named in the test.

Verification

• Every existing Pic32* test is unmodified and green; they hardcode the byte values independently, so they are the check that nothing moved.
• 10 new tests. Proven catchers: changing an opcode value in the new class fails 10 tests (2 of them new); re-declaring a diverging opcode inside the consumer fails the matching agreement test plus the guard that forbids either class from declaring wire constants of its own — mutation-verified for all four opcodes.
• Byte-for-byte equivalence to main was measured, not assumed. A throwaway harness dumped every message the producer builds (including 256 program-flash records chosen to exercise DLE escaping) and every decode outcome across the full 0–255 byte range, built against this branch and against origin/main: 1,285 lines, identical.
• Full suite green on net9.0 and net10.0 (3,053 Core + 86 MCP), 0 warnings.
• No bench run: the only code path this touches is a bootloader flash session, which needs a device reboot into the bootloader and is destructive by nature. The equivalence dump above is the substitute, and it is a stronger check than a single bench pass would have been.

closes #483

Not merging — for review.

[tylerkron]

/agentic_review

[qodo-code-review]

PR Summary by Qodo

Single-source PIC32 bootloader wire constants and add drift-guard tests

✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Centralize PIC32 bootloader framing/opcode bytes into one internal wire-format class.
• Update producer and consumer to reference shared constants, preventing protocol drift.
• Add tests that pin byte values and enforce producer/consumer opcode agreement.

Diagram

graph TD
  T["Pic32BootloaderWireFormatTests"] --> P["MessageProducer"] --> W["WireFormat constants"]
  T --> C["MessageConsumer"] --> W
  T --> W

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Keep duplication but add an agreement test

• ➕ Retains the 'two independent copies' safety net in production code
• ➕ Only adds tests; avoids changing runtime code paths
• ➖ Still risks a one-sided edit if tests are skipped or incomplete
• ➖ Leaves ongoing maintenance burden and invites future divergence

2. Model opcodes as an enum + mapping utilities

• ➕ Improves readability and type-safety at call sites
• ➕ Centralizes opcode list and can reduce raw byte usage
• ➖ Doesn’t inherently address framing byte duplication unless still centralized
• ➖ Potentially larger refactor with more surface area than needed

3. Golden-vector roundtrip tests only (no pinned constants)

• ➕ Validates end-to-end behavior with realistic frames
• ➕ Avoids restating literal values in tests
• ➖ A coordinated but incorrect change to both producer and consumer could pass
• ➖ Less direct protection against silent protocol byte edits

Recommendation: Keep the PR’s current approach: a single internal wire-format constant source plus explicit pinning tests and producer/consumer agreement tests. This removes drift risk in production while preserving the former redundancy as a deliberate, review-visible test contract.

Files changed (4) +278 / -38

Refactor (3) +90 / -38

Pic32BootloaderMessageConsumer.csConsume shared wire-format constants for response decoding +17/-20

Consume shared wire-format constants for response decoding

• Removes locally-declared framing/opcode byte constants and replaces all uses with static imports from Pic32BootloaderWireFormat. Adds remarks documenting the single-source contract shared with the producer.

src/Daqifi.Core/Firmware/Pic32BootloaderMessageConsumer.cs

Pic32BootloaderMessageProducer.csProduce messages using shared wire-format constants +14/-18

Produce messages using shared wire-format constants

• Removes locally-declared framing/opcode byte constants and uses static imports from Pic32BootloaderWireFormat when constructing framed, escaped, CRC-appended packets. Adds remarks documenting the shared wire-format contract.

src/Daqifi.Core/Firmware/Pic32BootloaderMessageProducer.cs

Pic32BootloaderWireFormat.csIntroduce centralized PIC32 bootloader wire-format constants +59/-0

Introduce centralized PIC32 bootloader wire-format constants

• Adds an internal static class defining SOH/EOT/DLE framing bytes and all command opcodes, with documentation warning against quiet edits and pointing to tests as the pinning authority.

src/Daqifi.Core/Firmware/Pic32BootloaderWireFormat.cs

Tests (1) +188 / -0

Pic32BootloaderWireFormatTests.csAdd wire-format pinning and producer/consumer agreement tests +188/-0

Add wire-format pinning and producer/consumer agreement tests

• Introduces tests that (1) restate and pin framing/opcode byte values, (2) verify producer-sent opcodes are accepted by consumer decode paths without hardcoding opcodes, and (3) use reflection to ensure producer/consumer do not re-declare byte constants.

src/Daqifi.Core.Tests/Firmware/Pic32BootloaderWireFormatTests.cs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

Great, no issues found!

Qodo reviewed your code and found no material issues that require review

Tip of the day

💡 Did you know, you can type 'qodo, fix this' on a finding and the fix lands right on your PR

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[tylerkron]

Qodo-clean, CI green — ready for review. (1 round on head b25787d: Bugs (0) / Rule violations (0) / Requirement gaps (0), 0 inline threads, SHA reference matches head; settle re-check at +5 min came back byte-identical.) Not merging — for review.