feat(firmware-upload): add data model, APJ reader and bootloader codec

[iacker]

Description

First step of #2017, covering steps 1 and 2 of the implementation sequence. Adds the firmware-upload model and production bootloader integration:

• APJ firmware parsing, validation, bounded decompression, and trusted digest checks
• Board and flash compatibility rules
• Upload state machine
• Bootloader packet codec and protocol handling
• Serial bootloader transport with erase, program, verify, reboot, and recovery flows
• Reconnection after bootloader re-enumeration
• Fail-closed USB identity matching using captured serial/location metadata, with a captured Linux by-path fallback only when no USB metadata is available
• Bounded serial reads and erase-progress timeouts

Constants and image padding follow ArduPilot Tools/scripts/uploader.py.

AI assistance was used. I reviewed the changes and ran the tests below.

Checklist

• Run pre-commit checks locally
• Verified by a human programmer
• All commits are signed off (use git commit --signoff)
• Code follows our coding standards
• Documentation updated if needed
• No breaking changes or properly documented

Testing

• Unit tests pass: 172 tests covering APJ parsing, bootloader protocol behavior, serial identity resolution, timeout handling, and full erase/program/CRC verification against a fake rev 5 bootloader
• Integration tests pass
• Manual testing performed
• Tested on flight controller hardware

[coveralls]

Coverage Report for CI Build 33909389085

Coverage at 89.404% (no base build to compare)

Details

• Coverage remained the same as the base build.
• Patch coverage: No coverable lines changed in this PR.
• No coverage regressions found.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

No coverage regressions found.

---

Coverage Stats

Relevant Lines:	19234
Covered Lines:	17196
Line Coverage:	89.4%
Relevant Branches:	5746
Covered Branches:	4745
Branch Coverage:	82.58%
Branches in Coverage %:	No
Coverage Strength:	2.66 hits per line

---

💛 - Coveralls

[amilcarlucas]

I helped out a bit and did two backend commits.

[Copilot]

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

Adds the initial firmware-upload feature foundation (APJ parsing + bootloader protocol codec + upload workflow) to support issue #2017 without introducing UI/serial-MAVLink coupling in the domain model.

Changes:

• Introduces a pure firmware-upload domain model (APJ parsing, compatibility rules, state machine, typed errors).
• Adds a bootloader adapter/client implementing the ArduPilot/PX4 serial bootloader protocol and a FlightController facade entrypoint.
• Adds extensive unit tests (APJ parsing, protocol encoding/decoding, fake bootloader transport, reconnection flow) and an architecture document.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
tests/test_data_model_firmware_upload.py	New unit tests for APJ parsing, compatibility checks, state machine, and codec behavior with a fake bootloader.
tests/test_backend_flightcontroller_bootloader.py	New unit tests for the bootloader backend/client (short reads, rev2 vs rev5, ext flash, retries, facade integration).
ardupilot_methodic_configurator/data_model_firmware_upload.py	New domain model: types, parsing, bounds checks, padding, CRC, compatibility policy, and upload state transitions.
ardupilot_methodic_configurator/backend_flightcontroller_protocols.py	Adds active_baudrate to the connection protocol for reconnecting after flashing.
ardupilot_methodic_configurator/backend_flightcontroller_connection.py	Tracks and exposes active_baudrate across connect/retry flows.
ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py	New backend module: APJ file reading limit, bootloader packet codec, BootloaderClient, retrying backend adapter.
ardupilot_methodic_configurator/backend_flightcontroller.py	Adds upload_apj_firmware() facade method coordinating bootloader entry, flashing, and reconnection.
ARCHITECTURE_firmware_upload.md	New architecture/design doc for firmware upload flow and layering.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

Unresolved critical and moderate findings affect firmware authenticity, device identity, timeout and recovery handling, metadata validation, and workflow consistency.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (4)

ARCHITECTURE_firmware_upload.md:55

• The diagram advertises BIN input, but load_apj() explicitly rejects every non-.apj file and the firmware-formats section says raw BIN is not supported. Label this as APJ input so the architecture reflects the implemented scope.

    BACKEND --> FILE[Local firmware file\nAPJ/BIN input]

ARCHITECTURE_firmware_upload.md:227

• This state sequence contradicts the integration workflow below, which requests final confirmation after bootloader identification, and the backend emits that latter order. Update this sequence so the documented state machine matches the implemented data flow.

   `idle → inspecting → awaiting_confirmation → entering_bootloader → identifying →
   erasing → programming → verifying → rebooting → reconnecting → completed`

ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py:614

• A successful abort_before_erase() sends REBOOT, but the loop then retries opening the bootloader without issuing another bootloader-entry command. On real hardware the first successful abort has already returned the board to the application, so identification retries cannot work; on the final attempt the code also reports that a power cycle is required even after a successful reboot. Keep the board in bootloader mode between retries and perform recovery only when retries are exhausted.

                    if not client.abort_before_erase():
                        client.close()
                        msg = _("cannot reboot the held bootloader; power-cycle the flight controller before reconnecting")
                        raise FirmwareBootloaderRecoveryError(msg) from exc
                    client.close()

ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py:161

• When both USB location and serial number were captured, this or admits a device that matches only one attribute. If the original controller is absent and another device occupies the same USB location with a different serial number, it becomes the sole match and can be opened for flashing. Require every populated identity field to match.

    matches = [
        port.device
        for port in serial.tools.list_ports.comports()
        if (identity.location and getattr(port, "location", "") == identity.location)
        or (identity.serial_number and getattr(port, "serial_number", "") == identity.serial_number)
    ]

• Files reviewed: 8/8 changed files
• Comments generated: 8
• Review effort level: Balanced

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-07)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Reviewed at head ab04b0a3e3. Full report: https://uav.tridgell.net/DevCallReviews/2026_09_08_AIReview/devcall_pr_reviews.html#prMethodicConfigurator-2027

The protocol layer is careful work — all 27 opcodes and response codes match Tools/scripts/uploader.py by exact byte value, endianness is right everywhere, CHIP_ERASE correctly gets its own 20 s timeout, the CRC is byte-identical to the reference, and board-ID validation has no force/override path (I proved that last one by mutation). The findings below are all in the parts the unit tests can't reach without a realistic transport.

Verdict: REQUEST CHANGES

BUG — backend_flightcontroller_bootloader.py:401-414: _erase_external misreads an 18% progress byte as INSYNC, aborting every external-flash upload

INSYNC is b"\x12" = decimal 18 (uploader.py:209), and the bootloader emits a raw stream of pct_done bytes in 0..100 during PROTO_EXTF_ERASE (Tools/AP_Bootloader/bl_protocol.cpp:700-731, cout(&pct_done, sizeof(pct_done))). _erase_external tests if first == INSYNC: on every progress byte.

The reference deliberately does not look for INSYNC until the percentage passes 90, for exactly this reason:

# Tools/scripts/uploader.py:663-671
last_pct = 0
while True:
    if last_pct < 90:
        pct = self.__recv_uint8()
        ...
    elif self.__trySync():

Feeding your own BootloaderClient the exact byte stream bl_protocol.cpp produces:

256 sectors (1MB @ 4KB): FAILED -> unexpected status b'\x12' instead of OK
 32 sectors (2MB @64KB): FAILED -> unexpected status b'\x15' instead of OK
 16 sectors (1MB @64KB): FAILED -> unexpected status b'\x19' instead of OK
                          pct values emitted: [0, 6, 12, 18, 25, 31, ...]
  8 sectors            : OK   pct values: [0, 12, 25, 37, 50, 62, 75, 100]  (18 never emitted)

With 4 KB external-flash sectors every value 0..100 is emitted, so it is guaranteed. Because pct_done is monotonic, the byte after the false INSYNC is never 0x10, so it never silently "succeeds" — it always fails, blaming the bootloader.

The aftermath is the bad part: _erase_external is called at upload():482 with stage = UploadStage.ERASING, and the handler at :519 only calls abort_before_erase() for IDENTIFYING/AWAITING_CONFIRMATION. So the user is left with a partially-erased external flash and a board still held in the bootloader.

Blast radius: 7 hwdefs set a non-zero EXT_FLASH_SIZE_MB, including CubeRedPrimary (32 MB), SPRacingH7/H7RF, RADIX2HD, YJUAV_A6SE, DevEBoxH7v2 and H757I_EVAL.

Why the tests miss it: tests/test_backend_flightcontroller_bootloader.py:94-97 models EXTF_ERASE as sync + b"\x64" + sync — a single 100 byte. The real progress ramp is never exercised.

Fix: mirror the reference — track last_pct, treat every byte as progress until last_pct >= 90, then look for INSYNC. And make the fake transport emit a realistic ramp including 18, or this comes back.

BUG — backend_flightcontroller_bootloader.py:597-623: the retry loop reboots the board out of the bootloader, then retries talking to the bootloader

On a BootloaderProtocolError the handler calls client.abort_before_erase() — which sends REBOOT + EOC — and then loops. A successful abort returns the board to the application, so attempts 2..5 open the port and try identify() against a running flight stack, which cannot work. The loop therefore always burns all 5 retries and ends at :619 telling the user to "power-cycle the flight controller before reconnecting" — even though the board rebooted cleanly and is fine. Copilot raised this as a suppressed comment and it's still present at head. Only reboot once the retries are exhausted.

BUG — backend_flightcontroller_bootloader.py:146,156-161: device identity matching isn't fail-closed, despite the docstring

Two ways in. :156-161 joins the identity tests with or, so when both location and serial_number were captured, a device matching only the USB location — the same physical port with a different board in it — is accepted as the sole match and opened for flashing. And :146 returns early on the Linux by-path without consulting the captured USB identity at all: a second reviewer reproduced this by moving board A to ttyACM1 and letting board B take ttyACM0, and resolution returned B. If both boards share a board ID, the compatibility check then permits flashing the wrong one.

Fix: require every populated field to match, and capture the persistent path before the reboot.

BUG — backend_flightcontroller.py:420: reconnection after flashing doesn't resolve the controller's new port

The reconnect reuses the connection manager's original comport.device. Reproduced with the real connection manager: flashing and reboot succeeded and the USB identity resolved to COM13, but MAVLink attempted only COM7 and raised FirmwareReconnectError. A flight controller commonly enumerates on a different port after a bootloader cycle, so this will bite routinely. Re-resolve the captured physical identity before reconnecting.

ISSUE — backend_flightcontroller.py:413: reboot_autopilot(hold_in_bootloader=True) is fire-and-forget, so an armed vehicle silently gets the "power-cycle" path

pymavlink's mavutil.reboot_autopilot sends MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN and never reads the COMMAND_ACK. ArduPilot refuses it when armed (GCS_Common.cpp handle_preflight_reboot returns MAV_RESULT_FAILED without the magic force value). The code then sleeps 0.3 s, disconnects, opens the port at 115200 and speaks bootloader protocol at a live flight stack; _wait_for_bootloader fails and the facade re-raises without reconnecting. Net result for an armed vehicle: connection dropped, told to power-cycle, no explanation. The repo already has ACK-checking infrastructure at backend_flightcontroller_commands.py:140-180 — reuse it, and/or refuse to start an upload while armed.

ISSUE — two smaller robustness items

:350-367 _read_exact only checks the deadline on an empty read, so a transport dribbling one byte per serial timeout never trips it — making ERASE_TIMEOUT/EXTF_CRC_TIMEOUT mean "N seconds of silence" rather than the wall-clock bound the name and docstrings promise. Arguably what you want for extf erase, but document it or move the check out of the if not chunk: branch.

:388-392 the INFO_EXTF_SIZE fallback resyncs without flushing input, where uploader.py:734 uses __sync() which calls flushInput() first. It works for the exact 2-byte reply an old bootloader gives, but any extra byte desynchronises the rest of identification. self._reset_input_buffer() already exists — call it.

NOTE — two divergences from uploader.py that are correct; please don't "fix" them to match

1. data_model_firmware_upload.py:141-149 pads the CRC by exact byte count where the reference pads in whole 4-byte words. Measured: identical for every word-aligned flash size (1024, 2048, 0x100000 all match), diverging only at 2045/2047 — which cannot occur, because bl_protocol.cpp:911 computes the CRC as for (p = 0; p < fw_size; p += 4) so INFO_FLASH_SIZE is always word-aligned.
2. :305 _pad4 pads with 0xff, while uploader.py:181-183 does self.image += bytes(0xFF) — which in Python 3 is bytes(255), i.e. 255 NUL bytes, not one 0xFF. A 4093-byte image becomes 4348 bytes in the reference and 4096 here. That's a latent bug in uploader.py, not here (real ArduPilot images are 4-byte aligned so the loop never runs) — possibly worth a separate uploader.py PR.

NOTE — smaller items

:78 MAX_APJ_DESCRIPTOR_SIZE is ~172 MiB, so load_apj will read a 172 MB file and decompress up to 64 MB twice before any validation; real APJs are 1–2 MB and the largest supported flash is 2 MB internal + 32 MB external. :416-422 _verify_v2 splits by PROG_MULTI_MAX where the reference splits by READ_MULTI_MAX — both 252 today, but they could move independently. :393-399 relies on Python evaluating constructor kwargs left-to-right to produce the correct on-wire GET_DEVICE order; correct today, but fragile hidden inside a constructor call — assign to locals first. :66,206-207 CHIP_FULL_ERASE is only compiled for STM32F7/H7 (bl_protocol.cpp:617-620); elsewhere 0x40 hits default: continue and the bootloader sends nothing, so _sync(timeout=ERASE_TIMEOUT) blocks 20 s then times out — uploader.py shares that weakness, but full_erase is a new user-facing option here. :268 firmware_version is always "0.1" because Tools/ardupilotwaf/chibios.py:362 hardcodes it in every APJ descriptor — a field with that name shown before confirming an upload will confuse; prefer git_identity plus the board name.

NOTE — PR body is stale, and it matters here

It still says "No serial, MAVLink or Tkinter code in this PR" and "36 tests", but open_serial_transport (:534) is a real serial.Serial, upload_apj_firmware (backend_flightcontroller.py:362) does real MAVLink bootloader entry and reconnection, and there are 70 tests. Worth saying explicitly that no hardware test has been done, since the Testing checklist is unticked and this is code that writes flash.

The deliberate staging — data model and codec landing ahead of the UI as step 1 of #2017 — is clear from the thread, so "not reachable from the UI" is not reported as a defect here.

---

On CI: the failing Publish Tests Results job is the repo's coverage report --fail-under=89 gate reporting 88%, and it is failing on master too (the last five master runs of pytest.yml all fail the same step). Not attributable to this PR.

Verified clean and worth not re-deriving: board-ID safety is right and proven by mutation — replacing check_compatibility with if False: makes exactly test_board_id_mismatch_is_refused_by_default and test_board_id_mismatch_cannot_be_overridden fail (68/2), restoring gives 70/70, and check_bootloader_matches_connected_board additionally cross-checks the MAVLink-reported apj_board_id before erase; the {33: 9} compatible-ID map matches uploader.py:112; APJ decode order is base64→zlib with both decoded lengths validated and truncated streams caught via decompressor.eof; parse_apj/crc/extf_crc were cross-validated against uploader.py's own firmware class on a descriptor built exactly as chibios.py builds them; the hand-rolled 0xEDB88320 CRC32 is byte-identical to the reference over buffers of 0/1/4/17/255/1024 bytes; PROG_MULTI chunking is 252 and word-aligned including the last chunk; rev-2 vs rev≥3 verify and the rev≥3-only reboot ACK both match; the transport is closed on every path; every user-facing string is wrapped in _() (66 calls, 60 msgids); and ruff, ruff-format, pylint, mypy and pyright all pass locally with 70/70 tests green.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-07)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head 5098d1a406; my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_08_0541/devcall_pr_reviews.html#prMethodicConfigurator-2027

Previous round: four of six findings are properly fixed and pinned — including BUG A, the one that would have broken every external-flash upload. The verdict stays REQUEST CHANGES, but for different reasons than last time: two of the fixes do not work in the production wiring, and I proved both end-to-end rather than by reading. CI is also newly red on two jobs that pass on master.

Previous round

• BUG A, _erase_external misreads an 18% progress byte as INSYNC — RESOLVED, both halves. :421-436 now tracks last_pct and only looks for INSYNC once last_pct >= 90, matching uploader.py:663-671, and tolerates 91..100 in the second loop. Validated against a byte-faithful replay of the PROTO_EXTF_ERASE stream from bl_protocol.cpp:700-731 across five flash geometries: at head all five pass; with only the new while last_pct < 90 block reverted, all four that emit byte 18 fail with unexpected status b'\x12', including 32 MB@64 KB (CubeRedPrimary). You did the test half too — the fake transport at tests/test_backend_flightcontroller_bootloader.py:98 now emits (0,6,12,18,25,…,93,100), which contains 18 and crosses 90, and reverting the production fix turns test_uploads_and_verifies_external_flash red. Properly closed.
• BUG C, identity matching not fail-closed — RESOLVED in production, but half of it is unpinned. :150-165 now requires every populated field and runs before the Linux by-path branch, and the identity is captured before the reboot. Reverting all to or turns test_bootloader_port_refuses_a_partial_usb_identity_match red. But moving the by-path block back in front of the identity block leaves all 88 tests green: test_bootloader_port_prefers_usb_identity_over_reused_linux_device_path patches bl.sys_platform but not the filesystem, so Path("/dev/ttyACM0").resolve(strict=True) raises and the by-path branch is never taken — on every CI runner. Shimming bl.Path so the paths exist proves the ordering does matter (identity-first resolves to our board, by-path-first to the other one) and that the test named for it passes either way. Please make it hermetic.
• ISSUE E, fire-and-forget reboot_autopilot — RESOLVED. backend_flightcontroller.py:414-427 now waits for the ACK and raises before disconnect(), so the link survives a refusal. Cross-checked against ArduPilot: param1=3 is REBOOT_SHUTDOWN_ACTION_REBOOT_TO_BOOTLOADER, param6=0 so the armed-force magic is not triggered, and handle_preflight_reboot sends the ACK before rebooting, so the happy path is not broken. Minor: the surfaced text is the generic "Command failed" — an armed vehicle deserves a specific hint.
• ISSUE F — both parts RESOLVED and pinned. :373-377 now checks the deadline after non-empty partial reads; :406 adds self._reset_input_buffer().
• The two "correct divergences" were correctly left alone — crc() still pads by exact byte count and _pad4 still uses 0xff. Thank you for not "fixing" those to match the reference.
• _verify_v2 now splits by READ_MULTI_MAX — RESOLVED and pinned. MAX_APJ_DESCRIPTOR_SIZE (measured 171.8 MiB), the GET_DEVICE kwarg-order reliance, the CHIP_FULL_ERASE stall on non-F7/H7, and firmware_version always "0.1" are STILL OPEN and unchanged.
• PR body — STILL OPEN. It still says "No serial, MAVLink or Tkinter code in this PR" and "36 tests", but there are now 88 firmware-upload tests, a real serial.Serial and a real MAVLink COMMAND_ACK round trip. "Tested on flight controller hardware" is still unticked, on code that writes flash.

Verdict: REQUEST CHANGES

1. BUG — backend_flightcontroller_bootloader.py:633: BUG B's fix cannot work — the retry re-enters through a facade that has already disconnected MAVLink

The fix sets reenter_bootloader = True after abort_before_erase() and calls self._enter_bootloader() on the next attempt. That callback is backend_flightcontroller.py:409-432, whose last two statements are time_sleep(0.3) and self.disconnect(). On the second call self.master is None, so it raises at :411-413 before any transport is opened.

Driven end-to-end through the real facade (your own _Connection/_Commands fakes, whose disconnect() sets master=None, plus your own StaleTransport) through a failed first identification:

FirmwareConnectionError: flight-controller connection was lost before bootloader entry

The board has already been rebooted out of the bootloader by attempt 1, the exception escapes _wait_for_bootloader uncaught (the try only wraps client.identify()), and the user gets a message about bootloader entry.

test_backend_retries_bootloader_identification_after_stale_serial_data's assert entries == 2 passes only because its fake enter() is a no-op counter that never disconnects — it pins a state that cannot occur in production. The original suggestion (only reboot once the retries are exhausted) would not have had this problem.

2. BUG — backend_flightcontroller.py:434-440: BUG D's fix resolves the USB identity exactly once, with no allowance for re-enumeration, so a successful flash is reported as a failure

upload() sends REBOOT and returns; the facade immediately calls reconnect_after_bootloader(), which calls resolve_bootloader_device() once. While the board is off the bus re-enumerating, comports() has no match, len(matches) != 1 → OSError → FirmwareReconnectError — and connect() is never reached, so its own 3×5 s retry never runs.

Driven through the real facade with comports() returning the board until it reboots and nothing afterwards:

HEAD:            FirmwareReconnectError: cannot resolve the flight-controller serial device
                 connection.reconnected = False
previous head:   upload + reconnect succeeded
                 connection.reconnected = True

So the fix for BUG D traded one reconnect failure for another. The same one-shot resolve is on the abort/recovery path. Fix: retry the resolve with a bounded deadline, not just the connect.

3. BUG — data_model_firmware_upload.py:252: external-only APJs are rejected

An APJ with an empty internal image and a non-empty external image raises "metadata values are out of range" here, but loads fine in the reference, which explicitly skips internal flashing for that case (Tools/scripts/uploader.py:966). Accept it and conditionally skip the internal erase/program/verify.

4. ISSUE — backend_flightcontroller_bootloader.py:421-433: the erase deadline is both too weak and too strong

deadline = self._clock() + ERASE_TIMEOUT (20 s) covers the whole external-flash erase. Two problems pulling in opposite directions:

• Too weak while bytes flow. _read_exact only tests the deadline when a read returns empty, so a bootloader emitting an unchanging progress byte keeps the upload alive indefinitely, with cancellation disabled. The while last_pct < 90 loop has no bound of its own either.
• Too strong once it lapses. Past 20 s every read runs with timeout=0.0, so the first read() returning b"" aborts a live erase. Same transport and progress stream, only the position of one empty-read window differs: empty read at ~5 s completes; at ~40 s gives timeout waiting for 1 bootloader bytes. A 32 MB external flash is 512×64 KB sector erases, which will be well past 20 s. uploader.py:657-671 imposes no wall-clock bound at all.

And upload():502-506 runs this with stage = ERASING, which is in neither the facade's recovery set (:479-483) nor the client's abort set (:542) — so the user is left with a partially erased external flash and a board held in the bootloader, the same aftermath BUG A had.

5. ISSUE — CI: pylint and Docker Build and Test are newly red, and both pass on master

pylint (3.10) / pylint (3.14): C0115 missing-class-docstring at tests/test_backend_flightcontroller_bootloader.py:182,351 and tests/test_bootloader_preflight.py:28; C0115 + R0903 too-few-public-methods at tests/test_bootloader_partial_read.py:27, W0212 protected-access at :35. All in this PR's own new test files.

Docker Build and Test: 4915 tests pass, then the lint service exits 8 on R0801 duplicate-code — backend_flightcontroller:[416:422] against backend_flightcontroller_commands:[231:237] and [410:415], i.e. the new param1..param7 block introduced by the ISSUE E fix itself.

Master is Pylint = success and Docker CI = success at this PR's base, so both are attributable. (Publish Tests Results is the coverage --fail-under=89 gate at 88% and fails on master too — not attributable, same as last round.)

6. NOTE — smaller items

data_model_firmware_upload.py:349: the now-mandatory expected_firmware_sha256 covers the whole .apj, but nothing publishes that value — Tools/scripts/generate_manifest.py:111-128 emits only git_sha, no per-file checksum, so firmware.ardupilot.org's manifest cannot supply it. Worth saying in the PR where a caller is expected to get it, or the feature is unusable when the UI lands. Also metadata.content_sha256 and FirmwareImage.content_sha256() are both unread in production — two similarly named digests, one dead.

tests/test_bootloader_preflight.py:19-59 duplicates two tests verbatim from tests/test_backend_flightcontroller_bootloader.py:355-390 (both copies go red together — no extra coverage, double maintenance). ARCHITECTURE_firmware_upload.md:105-107 now describes the platform order backwards (the USB identity is consulted first on all platforms; by-path is the no-identity fallback), and the numbered list at :160-175 lost its indentation so items 3 and 4 will render broken.

7. NOTE (unconfirmed) — macOS dual-CDC boards may become unresolvable

serial/tools/list_ports_osx.py:289-291 sets location and serial_number from the USB device, not the interface, so a board exposing two CDC-ACM interfaces yields two comports with identical identity → len(matches) == 2 → OSError and the upload is refused with no fallback. 87 hwdef.dat files put OTG2 in SERIAL_ORDER, and CubeRedPrimary/hwdef-bl.dat also defines HAL_HAVE_DUAL_USB_CDC. Linux and Windows are unaffected (both per-interface). Marked unconfirmed — there is no macOS host here, so this is code evidence only. Suggest tie-breaking on port.interface when all candidates share one physical device, rather than refusing.

---

Checked and clean: board-ID safety is unchanged and still has no force/override path — a grep for force|override|skip_check|allow_ across both new modules returns nothing, and check_compatibility still refuses on board ID, internal size and external size. The new _parse_integer_metadata correctly rejects bool/float for the four integer metadata fields. 88 firmware-upload tests green at head; 11 of 12 applied mutations produce a red test naming the corresponding behaviour, with the two exceptions called out above.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-08)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head 7f26f78852 (previously 5098d1a406); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_09_0151/devcall_pr_reviews.html#prMethodicConfigurator-2027

Both blocking bugs from the last round are genuinely fixed, and I proved both by execution.

• BUG :633 — the retry re-entering through an already-disconnected facade — RESOLVED. You restructured rather than patched: _enter_bootloader is out of the retry loop entirely and abort_before_erase() runs only on the final attempt (:648), which is exactly the direction suggested. Driven through the real facade with your own _Connection/_Commands fakes and StaleTransport: with 1, 2 or 4 failed identifications the upload now succeeds, with exactly one MAVLink bootloader-entry command and no premature reboot; with all 5 failing, the abort fires only on attempt 5 and the facade reconnects. The same harness against the old head's files gives FirmwareConnectionError: flight-controller connection was lost before bootloader entry every time.
• BUG backend_flightcontroller.py:434-440 — one-shot resolve reporting a successful flash as a failure — RESOLVED and pinned. The resolve now retries under FIRMWARE_RECONNECT_RESOLVE_TIMEOUT = 15.0; 0 s / 3 s / 10 s absences all reconnect, a board that never returns fails in a bounded 15.0 s, and reverting to one-shot turns test_facade_waits_for_serial_identity_to_reappear_before_reconnecting red.
• Also resolved: external-only APJs accepted; the erase deadline is now a 20 s inactivity bound with a monotonicity check (a byte-faithful 512-sector replay completes at 117 s and 312 s simulated, where removing just the deadline reset fails both at 20 s); pylint ×2 and Docker Build and Test are green again; the duplicate test_bootloader_preflight.py is gone; the ARCHITECTURE doc's platform order and numbered list are fixed.

93 firmware-upload tests at head, up from 88. Verdict is still REQUEST CHANGES, but for a new regression and two things the fixes did not reach — not for lack of progress.

1. BUG — backend_flightcontroller_bootloader.py:378-383: new regression — _read_exact discards a complete reply that lands at or after the deadline

The len(received) < size guard was dropped, so the deadline is now tested even on the iteration that completes the read. With a transport delivering the whole 2-byte reply in one read and a deterministic clock, timeout=2.0:

arrives at 1.90 s -> returns b'\x12\x10'
arrives at 2.00 s -> RAISES  timeout waiting for 2 bootloader bytes; received 2
arrives at 2.05 s -> RAISES  timeout waiting for 2 bootloader bytes; received 2

All three return the data against the old head. (Note the message contradicts itself — "received 2" of 2.) This sits on the path of every bootloader command — _command, _sync, identify, and _verify_external at EXTF_CRC_TIMEOUT = 10 s, which the reference uploader.py:703 also budgets at 10 s but accepts a reply landing near the edge.

The change was necessary, not gratuitous: restoring the old guard makes test_external_erase_times_out_when_progress_never_advances hang forever, because a transport that always returns a byte can never trip an incomplete-read-only check. It just overshoots. Suggested shape, which keeps the wall-clock bound and never throws away good data:

while len(received) < size:
    if self._clock() >= deadline:
        raise ...
    received.extend(self._transport.read(size - len(received)))

2. BUG — backend_flightcontroller_bootloader.py:135: with no USB metadata, the Linux by-path fallback can flash a different controller

capture_serial_device_identity() returns None when the port exposes neither a USB location nor a serial_number. has_stable_bootloader_device_identity() then returns True if some entry under /dev/serial/by-path currently resolves to the device — it answers "a stable path exists" but never records which one, and the caller keeps only the original device string. After the reboot, resolve_bootloader_device() with identity is None resolves by path afresh and returns whatever controller now occupies it.

Demonstrated through the real facade using filesystem links to simulate reassignment: preflight accepted board A, the backend then opened by-path/usb-B, programmed B, and reported success. Matching board IDs do not help when both boards are the same type.

Exposure is narrow — Linux only, only with no USB identity at all, and only with a second board taking the path — but the consequence is flashing the wrong hardware, and your USB-identity path is already correctly fail-closed by comparison. Fix: capture and retain the resolved persistent path before the reboot, and reopen exactly that path afterwards.

3. ISSUE — backend_flightcontroller.py:476-480: the client now reboots at every stage, but the facade still reconnects for only three

:552/:559 dropped the stage gate, so abort_before_erase() now also runs for ERASING/PROGRAMMING/VERIFYING — that half of last round's item 4 is fixed. But :476-480 still lists only ENTERING_BOOTLOADER, IDENTIFYING and AWAITING_CONFIRMATION. With a transport that fails the first PROG_MULTI after a successful erase, the bootloader is told to reboot and the facade does not reconnect, so you are left disconnected with a raw BootloaderProtocolError(stage=programming).

Being precise about severity, because the two review passes disagreed and I settled it against the bootloader source: one pass argued this "finalizes damaged firmware" because PROTO_BOOT commits the deferred lead words before booting. The first half is right (bl_protocol.cpp:1147), but the conclusion overstates it — jump_to_app() at bl_protocol.cpp:250-255 calls check_good_firmware() first and refuses to boot on failure, and AP_CHECK_FIRMWARE_ENABLED is emitted as 1 by chibios_hwdef.py:1704 with no hwdef disabling it. So a half-written image does not execute; the board stays in the bootloader. The real cost is that the lead-word deferral — a deliberate mechanism to make an incomplete upload self-evident — is destroyed, leaving check_good_firmware() as the only guard, plus an unhelpful disconnected error. It is also plainly wrong where the reboot does restore working firmware, e.g. the CHIP_FULL_ERASE 20 s stall on a non-F7/H7 board where nothing was erased. Suggest keying the reconnect on "the abort reboot succeeded" rather than a stage allow-list, and restricting the automatic reboot to failures before erase.

4. ISSUE — :166-169: the macOS dual-CDC tie-break added this round cannot disambiguate

list_ports_osx.py:291 sets info.interface from a locationID taken from the USB device parent, while scan_interfaces() keys on the interface parent's locationID and returns the first match — so both CDC ports of one board get the same interface. The tie-break only runs when location and serial_number already tie, which is exactly when the interfaces tie too, so len(interface_matches) == 1 cannot hold. test_bootloader_port_uses_the_captured_interface_to_resolve_dual_cdc passes only because it hand-sets interface = "0"/"2", a shape the macOS backend never produces. Second-order: the identity is captured from the application firmware and used to resolve the bootloader's port, and the interface name generally differs between the two. (Code analysis — I have no macOS host, so the runtime behaviour is unconfirmed.)

5. ISSUE — data_model_firmware_upload.py:224: the stage model cannot represent a successful dual-region upload

_HAPPY_PATH is strictly linear and next_stage() allows only index(target) == index(current) + 1, so external erase/program/verify followed by internal erase/program/verify needs VERIFYING -> ERASING and raises illegal transition verifying -> erasing. Exposure stated honestly: next_stage is referenced only from its own module and the tests, never from the upload path, so nothing breaks today — this bites when the UI lands and starts driving the model.

6. NOTE — five correct behaviours with no test at all

Both passes ran independent mutation suites (15 and 26 mutations). These stayed green with the production line broken:

• reboot_to_bootloader() has no test whatsoever. param1=3 (hold in bootloader) → param1=1 (reboot into the application) leaves everything green. Nothing pins param1=3, and nothing pins param6 staying 0 (the armed-force magic). Found independently by both passes.
• The identity-before-by-path ordering is still unpinned — unchanged from last round, and the one item explicitly asked for then. test_bootloader_port_prefers_usb_identity_over_reused_linux_device_path still patches only bl.sys_platform, not the filesystem, so Path("/dev/ttyACM0").resolve(strict=True) raises on every CI runner, the by-path branch never executes, and swapping the two blocks leaves all tests green. This is the same non-hermeticity behind finding 2.
• :525 skip-internal-when-empty. Replacing the guard with if True: issues CHIP_ERASE on an external-only APJ while test_external_only_upload_does_not_touch_internal_flash still passes — its only internal assertion is transport.flash == b"", true either way. That would wipe running firmware, so it deserves an assertion on the command stream.
• :445 erase-deadline reset — the difference between a 32 MB erase completing and failing at 20 s — yet all bootloader tests pass without it, because no erase test is long-and-progressing.
• :648 abort-only-on-the-final-attempt, i.e. the fix for the previously blocking bug. The test asserts entries == 1, pinning the removal of the re-entry call but not the "do not reboot between retries" half, because the fake's abort_before_erase() has no effect on the fake board.

Carried over, re-measured, unchanged

MAX_APJ_DESCRIPTOR_SIZE still admits 171.8 MiB; CHIP_FULL_ERASE still stalls 20 s on non-F7/H7; firmware_version still hardcoded "0.1" at chibios.py:361; metadata.content_sha256 and FirmwareImage.content_sha256() still both unread in production. One finding I withdraw: the GET_DEVICE kwarg-order reliance was over-weighted last round — each GET_DEVICE is its own synchronous round trip, so a different evaluation order would change the wire order without ever misassigning a value. Cosmetic, sorry.

PR body, third round running: still says "No serial, MAVLink or Tkinter code in this PR" and "36 tests", when test_data_model_firmware_upload.py alone now has 50, the feature has 93, and there is a real serial.Serial and a real MAVLink COMMAND_ACK round trip. "Tested on flight controller hardware" is still unticked on code that writes flash.

CI: the single failure is not attributable — Publish Tests Results fails at coverage --fail-under=89 with TOTAL 88%, and master fails the identical job and step. Everything else is green, including the three checks that were red last round.

Commit hygiene is good — one focused fix per commit with its tests and doc update — except that 6e2b957e "re-enter bootloader after retry abort" adds a mechanism that 221e0396 later removes, so the branch carries a commit describing behaviour it no longer has. Worth squashing.

Re-checked and clean: erase progress handling matches bl_protocol.cpp:700-731 exactly including the trailing pct_done = 100; PROG_MULTI_MAX = 252 is within the 256-byte flash_buffer; identify() satisfies CHECK_GET_DEVICE_FINISHED before any erase; check_compatibility still refuses on board ID and both sizes with no force path anywhere in the new code; all 23 byte-valued protocol constants match uploader.py; oversized descriptors and truncated zlib streams are rejected.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-08)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head a211c3f35e (previously 7f26f78852); my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_09_0533/devcall_pr_reviews.html#prMethodicConfigurator-2027

Every fix from the last round survived the rewrite, and I proved each one by mutation rather than by reading.

The force-push was a rewrite, not a re-split (2 commits, rebased onto current master), which is exactly where a fix goes missing quietly. None did. Nine mutations against a 204-test baseline, all nine caught:

Mutation
reboot_to_bootloader param1=3 → 1	caught (new test asserts (3,0,0,0,0,0,0))
identity block moved after the by-path block	caught
persistent_path capture removed	caught
internal-erase guard → if True:	caught
erase-deadline reset removed	caught
abort on every attempt instead of only the last	caught
_enter_bootloader moved back inside the retry loop	caught
single-shot resolve (while → if)	caught
erase progress-monotonicity check removed	caught

That is the item I asked for most insistently last round, and it is done. Per-finding:

• BUG 1 (_read_exact discarding a complete reply at/after the deadline) — RESOLVED, in the exact shape suggested: :373 is now while len(received) < size: with the deadline tested at the top. tests/test_bootloader_partial_read.py parametrises 2.00 s and 2.05 s against timeout=2.0 and asserts the data comes back. Reverting fails 2 tests.
• BUG 2 (Linux by-path flashing a different controller) — RESOLVED for the enumerated case. SerialDeviceIdentity gained persistent_path; the by-path link is captured before the reboot (:138) and returned verbatim afterwards (:163-164). (Narrow residual below.)
• ISSUE 3 (reboot/reconnect stage mismatch) — RESOLVED, differently from the suggestion: the client now only issues the abort reboot for IDENTIFYING/AWAITING_CONFIRMATION and flags bootloader_rebooted, which the facade keys on. Both sides agree now.
• ISSUE 4 (macOS dual-CDC tie-break) — RESOLVED by removal, fail-closed with OSError, pinned by tests/test_bootloader_identity.py:39. (Consequence below.)
• ISSUE 5 (dual-region stage model) — RESOLVED: explicit VERIFYING → ERASING transition, _HAPPY_PATH reordered to match what the client emits.
• Carried over: firmware_version now reads desc.get("version","") — fixed. MAX_APJ_DESCRIPTOR_SIZE is still 171.8 MiB but now derived, with blobs bounded at 85.4 MiB and images at 64 MiB — defensible, and I withdraw it as a finding.

Feature tests: 101, up from 93. Verdict stays REQUEST CHANGES, for one new regression.

1. BUG — the persistent_path fix defeats the 15 s reconnect retry on Linux, so a successful flash is reported as a failure

This regresses the bug that the other fix closed last round, by a different route.

backend_flightcontroller.py:426-432:

while time_monotonic() < deadline:
    try:
        reconnect_device = resolve_bootloader_device(device, device_identity)
        return self.connect(reconnect_device, log_errors=False, baudrate=active_baudrate)
    except OSError as exc:
        last_error = exc
        time_sleep(0.1)

The loop only retries when resolve_bootloader_device raises. But connect() signals failure by returning a non-empty error string, and that return leaves the loop on the first iteration. Previously this was survivable: with a USB identity, resolve_bootloader_device reliably raised OSError("cannot uniquely locate…") while the board was still absent. At this head backend_flightcontroller_bootloader.py:163-164 short-circuits on identity.persistent_path — which is populated for essentially every Linux USB serial port — so it never raises, and the 15 s budget is never spent.

Proved end-to-end through the real facade with your own _Connection/FakeBootloaderTransport, using the production resolve_bootloader_device (not patched), and a connect() that fails twice then succeeds:

persistent_path='/dev/serial/by-path/pci-0000:00:14.0-usb-0:2:1.0'
  connect_calls=1  elapsed=0.30s -> FirmwareReconnectError: could not open port ...

One attempt, 0.30 s out of 15 s — and the flash had already completed successfully. Treating a non-empty connect() return as retryable inside the loop gives connect_calls=3, elapsed=0.50s -> SUCCESS, with the firmware/bootloader suite still green (140 passed, 1 skipped).

The suite cannot catch this, because test_facade_waits_for_serial_identity_to_reappear_before_reconnecting (tests/test_backend_flightcontroller_bootloader.py:644) monkeypatches resolve_bootloader_device to raise OSError — precisely the condition the short-circuit removes. Also structural: whenever persistent_path is set, the location/serial_number match at :165-183 is unreachable, and a stale persistent_path is returned with no check that the link still exists.

2. ISSUE — narrow residual of BUG 2: an unenumerated device is still unanchored

backend_flightcontroller_bootloader.py:135-152. capture_serial_device_identity() iterates comports() and returns None when no entry has port.device == device. has_stable_bootloader_device_identity() then falls back to bool(_capture_linux_persistent_path(device)) — it answers "a stable path exists" but still never records which one — and resolve_bootloader_device(device, None) resolves afresh, returning whatever board now occupies the path. Reproduced with real symlinks: preflight accepted board A, the backend opened board B's link. Matching board IDs don't help when both boards are the same type. Much narrower than last round, but the same failure mode — capture the resolved path on this branch too.

3. ISSUE — only 2.3 s is allowed for the bootloader to enumerate

open_retries=5, retry_delay=0.5 (:598-599) with a fail-fast open puts the last attempt 2.3 s after the reboot command. Measured with a timestamp-recording serial_factory: open attempts at t = [0.3, 0.8, 1.3, 1.8, 2.3], then FirmwareBootloaderRecoveryError telling the user to power-cycle. The reference Tools/scripts/uploader.py:1396 instead polls the port list every 50 ms in an unbounded loop until the bootloader appears. 2.3 s looks tight for an FC reboot plus USB re-enumeration, and it is oddly asymmetric with the 15 s on the reconnect side. Unconfirmed on hardware — there is no flight controller on the review box, which is also what the unticked "Tested on flight controller hardware" box would settle.

4. ISSUE — macOS dual-CDC boards now cannot reconnect after a successful flash

Removing the tie-break made the code honest, and fail-closed is the right call. But on a dual-CDC board both CDC ports share location and serial_number, the match is ambiguous, and the facade raises FirmwareReconnectError after programming and reboot both succeeded. (I also ran pyserial's list_ports_osx.py with mocked IOKit and confirmed both ports get the same interface label, so the removed tie-break could indeed never have worked — that part was right to delete.) The user-visible result is still "successful flash reported as an error"; it deserves either a targeted disambiguation or a message saying the flash succeeded.

Notes

• :388-392, the mid-loop if len(received) < size and self._clock() >= read_deadline: guard, can be deleted in full with all 140 firmware/bootloader tests still passing — redundant with the loop-top check. And the test named for it, test_read_timeout_is_bounded_when_partial_reads_arrive_after_deadline, supplies clock_values = iter([0.0, 1.0]) with timeout=1.0, so the loop-top check fires on entry: instrumented, transport.read() is called 0 times. It never performs a partial read.
• Unconfirmed: persistent_path assumes the app and bootloader expose the same USB interface number. …-usb-0:2:1.N encodes interface index N; on a dual-CDC board the second console is :1.2, which doesn't exist in bootloader mode — resolve_bootloader_device would return an absent path and all five opens would fail, where the old USB-location match would have found the board.
• SerialDeviceIdentity.interface is still declared and populated but never read — dead field. metadata.content_sha256 and FirmwareImage.content_sha256() remain unread in production, superseded by apj_sha256 which is used; consider deleting the dead pair.
• A failure at ERASING/PROGRAMMING/VERIFYING now issues no reboot and no reconnect, and the raw BootloaderProtocolError carries no power-cycle guidance. Safe, but the user is left disconnected with the board in the bootloader and no instruction.
• PR body, third round: still says "No serial, MAVLink or Tkinter code in this PR" and "36 tests" when that file collects 51 and the feature totals 101; :582 open_serial_transport is a real serial.Serial(..., exclusive=True) and reboot_to_bootloader() is a real MAVLink COMMAND_ACK round trip.

CI

Only Publish Tests Results fails (coverage --fail-under), and the identical job fails on master — not attributable. All four pytest platform jobs, mypy, pyright, pylint ×2, ruff, CodeQL and Docker pass.

Checked and clean

All 27 opcodes and status bytes re-verified byte-for-byte against a freshly fetched uploader.py (58544 bytes) — INSYNC/EOC/OK/FAILED/INVALID/BAD_SILICON_REV, GET_SYNC…CHIP_FULL_ERASE, all INFO_*, PROG_MULTI_MAX = READ_MULTI_MAX = 252 — exact match. Copilot's external-flash erase/program size mismatch and FirmwareImage.crc padding points are both non-issues (the reference does the same, and _pad4 makes the divergence unreachable). apj_sha256 digests the whole APJ read in binary, so it authenticates board_id. _parse_integer_metadata rejects bool and float before int(); BinasciiError is caught explicitly. _erase_external's last_pct >= 90 gate and 91..100 tolerance still match uploader.py:664-671. The full pytest tests/ (4485 non-GUI tests) shows zero backend or firmware-upload failures — the 57 failures and 278 errors are all frontend_tkinter*/gui_* wanting an X display.

[iacker]

Fixed the reconnect loop in 4470778. connect() returns an error string instead of raising, so the loop now retries on a non-empty return until the deadline. The new test fails on the previous head. I also dropped the unread interface field and the redundant deadline check in _read_exact. The coverage gate is red on master too, same 88 percent.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-09)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head 447077805a (previously a211c3f35e); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_09_1121/devcall_pr_reviews.html

Last round's blocking regression is genuinely fixed — I proved it by A/B against the old head

Previous finding	Status
BUG — persistent_path defeats the 15 s reconnect retry; a successful flash reported as failure	RESOLVED
Note — SerialDeviceIdentity.interface declared and populated but never read	RESOLVED — field and its populate line deleted; identity.interface has 0 remaining readers
Note — the mid-loop read-deadline guard can be deleted in full	RESOLVED — my own suggestion, correctly applied
ISSUE — unenumerated device is still unanchored	STILL OPEN — and now the blocker, below
ISSUE — only 2.3 s allowed for the bootloader to enumerate	STILL OPEN — open_retries=5, retry_delay=0.5 unchanged
ISSUE — macOS dual-CDC boards cannot reconnect after a successful flash	STILL OPEN — resolve_bootloader_device:174-177 unchanged

Replaying my previous repro: old head connect_calls=1, 0.30 s → FirmwareReconnectError; new head connect_calls=3, 0.50 s → SUCCESS. Budget exhaustion gives connect_calls=150, elapsed=15.32 s against the 15.0 s constant, with last_error carried into the message. The comment's claim that connect() signals failure by returning a string rather than raising is accurate for every failure mode pyserial and pymavlink actually produce.

And the removed read timeout — the change I most expected to have to object to — is behaviour-preserving. The loop-top check runs on every iteration including the one right after a partial read, so the deleted lines only removed a raise the next iteration performs anyway. Five constructed transports driven through the real _read_exact under a hard wall-clock kill behave identically at both heads, and end-to-end a bootloader that goes silent mid-transfer always terminates (2.00 s identifying, 20.01 s erasing at the intended ERASE_TIMEOUT, 2.00 s programming). Every production construction path sets timeout and write_timeout. All six new tests go red when their guard is reverted — including the reconnect one against the exact pre-fix shape.

The blocker: the tool can flash a different flight controller than the one selected

This is the carried-over identity issue, and it is no longer theoretical — two reproductions now open, erase and program the wrong fake board.

(a) backend_flightcontroller_bootloader.py:161-162 — the by-path shortcut bypasses the serial check. Resolution returns the captured by-path link immediately, even when a USB serial number was captured. Substituting a device with serial OTHER at the path where capture recorded ORIGINAL is accepted, and the facade proceeds to erase and program it. Matching board IDs do not prevent this.

(b) :146, with rediscovery at :179 — the binding is lost across the reboot. When comports() has no exact match, capture_serial_device_identity returns None, yet the mere presence of a by-path link is still treated as authorising the upload; resolution then re-discovers the device after the bootloader reboot. A fake presenting slot A before reboot and slot B after selects B, and B is what gets erased and programmed.

Both live at :128-147/:161-179, byte-identical to the previously reviewed head. Suggested direction: resolve the captured USB identity first and use by-path only as a fallback, and retain the original link across the reboot rather than re-resolving.

I am raising this to a blocker rather than carrying it as an open issue again, and I want to be explicit about why, since I did not do so last round: for a tool whose job is to flash flight controllers, "opens the wrong board" is the one failure mode that has to be closed before merge, and it now has two reproduced mechanisms rather than one argued one. The rest of this PR is in good shape.

Also worth fixing

• tests/test_bootloader_partial_read.py:33 never performs a partial read. Its clock_values deadline has already expired on entry, so transport.read() is called zero times despite the test's name; test_backend_flightcontroller_bootloader.py:370 (the empty-read test) has the same defect. Moving the deadline guard outside the loop leaves both green — so the bound they exist to protect is currently unguarded by them. The real partial-read path is covered only incidentally, by the erase test.
• data_model_firmware_upload.py:237 catches (UnicodeDecodeError, json.JSONDecodeError, TypeError), but CPython's 4300-digit integer-string limit makes json.loads raise a plain ValueError for a long JSON number literal, so a malformed APJ escapes untyped instead of as FirmwareFileError. One-token fix: json.JSONDecodeError → ValueError (a strict superset). Four fields reproduce it. No board risk — this is pre-bootloader-entry — so robustness, not safety.
• test_external_erase_times_out_when_progress_never_advances only catches its mutation by hanging; without --timeout the run wedges. A pytest-timeout marker would make CI fail instead.
• The PR body is now well out of date — it still says "No serial, MAVLink or Tkinter code in this PR" and "36 tests", where :573 opens a real serial.Serial(..., exclusive=True) and the feature totals 106. On a PR that flashes flight controllers the body is what a reviewer reads first. "Tested on flight controller hardware" is also still unticked, and the two remaining open issues (enumeration budget, macOS dual-CDC) are exactly what only hardware settles.

Notes

• _read_exact:378-380 busy-spins on a transport that returns b"" instantly — 5,984,566 read() calls and 1.0 s of 100% CPU. Identical at the old head and unreachable in production, since open_serial_transport always sets a positive timeout. Defensive only.
• Correcting myself from last round: I said _pad4 made the FirmwareImage.crc() divergence from uploader.py unreachable. That reasoning was wrong — the correct condition is flash_size % 4. Identical for every 4-aligned flash_size (100 checked); all 33 divergent cases have flash_size % 4 != 0. Real INFO_FLASH_SIZE values are always 4-aligned so it stays unreachable, but for a different reason than I gave, and flash_size is never validated for alignment.

---

Checked and clean: 27 protocol constants re-derived from uploader.py and diffed — 27 matched, 0 mismatched, including all INFO_*, BL_REV_MIN=2/MAX=5 and PROG_MULTI_MAX = READ_MULTI_MAX = 252. Framing matches __program_multi/__program_multi_extf; bootloader_crc32 matches uploader.crc32 byte-for-byte on 7 vectors; encode_extf_erase's 0 < size <= 0xFFFFFFFF matches the wire's 32-bit little-endian field exactly; identify() follows the reference order; reboot ACK semantics match over revisions 2-5. APJ decode is properly bounded — a 200 MB zip bomb is refused in 0.17 s at 128 MB peak. The erase path's deadline resets only on increasing progress. 174 passed across the 6 touched test files; the 106-test feature suite run 25× with random ordering, 0 flakes; full non-GUI suite 3574 passed, 0 failed (34 errors are headless-display artefacts of this box). ruff clean, pylint 10.00/10.

CI: every real job green — all four pytest platform jobs, mypy, pyright ×2, pylint ×2, ruff, CodeQL, Docker, pre-commit.ci. Only Publish Tests Results fails, at its coverage step (88% vs fail-under=89), and it fails identically on master — not attributable to this PR, as you noted in the thread.

Verdict: REQUEST CHANGES — on the identity binding alone. Everything this round changed is an improvement, and the reconnect fix is solid.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-09)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head 35c60747da (previously 447077805a); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_09_2022/devcall_pr_reviews.html#prMethodicConfigurator-2027

The identity blocker is closed. I replayed both wrong-board reproductions against the old head and confirmed they are rejected at this one, then reverted each half of the fix and watched its own regression test go red. REQUEST CHANGES → COMMENT.

Previous round — triage

Finding	Status	Evidence
BLOCKER (a) by-path shortcut bypasses the captured serial (:161-162)	RESOLVED	resolve_bootloader_device now tests identity.location or identity.serial_number first, requires exactly one comports() match, else raises. Replay: capture serial=ORIGINAL + by-path P, post-reboot a board with serial=OTHER at P → nothing opened, nothing flashed. Putting the shortcut back first → FAILED test_bootloader_port_refuses_persistent_path_with_a_different_usb_serial
BLOCKER (b) binding lost across the reboot (:146, :179)	RESOLVED	The by-path link is captured before the reboot (:130, :140) and returned verbatim; the post-reboot rediscovery is gone. Reverting the capture → FAILED test_bootloader_port_retains_linux_by_path_without_usb_metadata
Both timeout tests never performed a read	RESOLVED	Confirmed they called read() zero times at the old head. Now [0.0, 0.5, 1.0] plus assert transport.reads == 1; reverting the clock sequences fails both
json.JSONDecodeError → ValueError (:237)	RESOLVED	Reverting the one token → FAILED test_parse_apj_wraps_json_integer_digit_limit_as_file_error
Stalled-erase mutation caught only by hanging	RESOLVED	pytest-timeout in the dev extra + @pytest.mark.timeout(5); the mutation now fails in ~5 s instead of wedging
PR body out of date	RESOLVED	Rewritten; the "no serial code" sentence is gone and the 107-test claim matches what I measured
~2.3 s bootloader enumeration budget	STILL OPEN	open_retries=5, retry_delay=0.5 unchanged at :592-593
macOS dual-CDC reconnect	STILL OPEN	Both CDC interfaces share a location/serial on macOS, so the post-flash resolve raises. It now fails safe rather than picking wrong, which is the right direction, but such a board still cannot be flashed

105 tests pass across the three touched files; 176 across the wider firmware set.

ISSUE — the new USB binding is silently skipped for a symlinked device path

backend_flightcontroller_bootloader.py:132 compares port.device != device as raw strings. comports() reports /dev/ttyACM0, so a user who connects by a stable alias gets no match, capture falls through to :140, and the identity carries only persistent_path. Measured, same physical device throughout:

/dev/ttyACM0                                            -> location='1-2:1.0'  serial='FC123'
/dev/serial/by-id/usb-ArduPilot_CubeOrange_2F0034-if00  -> location=''         serial=''
/dev/serial/by-path/usb-0:2:1.0                         -> location=''         serial=''

So the strong USB-serial binding that closes the blocker is discarded for exactly the user who took the trouble to address the board by its stable ID. It degrades to the weaker topology binding rather than to nothing, so this is not a live wrong-board bug — but it is the one input where the new guarantee quietly does not apply, and it is reachable from --device and from the GUI's "Add another" path. One line: resolve both sides with Path(...).resolve() before comparing, keeping the string compare as a fast path.

NOTE — the gate that enforces all of this has no regression guard

Deleting the has_stable_bootloader_device_identity check at backend_flightcontroller.py:460-462 outright leaves 558 passed / 187 errors, byte-identical to the unmutated baseline (the 187 errors are the same headless-display artefacts in both runs, so they are not masking a failure). That check is what refuses to flash without a stable identity and it sits immediately before backend.upload(); with it gone and no identity, resolve_bootloader_device falls through to return device — the raw path, which is exactly the unanchored case mechanism (b) was about. Worth one test that drives the facade with an unresolvable identity and asserts nothing is opened, erased or programmed.

Correcting myself: I also flagged the has_stable_bootloader_device_identity simplification itself as unpinned. That was wrong — the old and new forms are provably equivalent, because capture_serial_device_identity returns None only when persistent_path is empty too, which is precisely when the old form's re-probe returned False. I measured all three cases and they agree. Behaviour-neutral cleanup, not a test gap.

Smaller notes

• data_model_firmware_upload.py:234 — the contents.encode("utf-8") line sits outside the try, so a lone surrogate escapes as a bare UnicodeEncodeError rather than FirmwareFileError. Same class as the ValueError issue you just fixed, one line earlier. Unreachable from load_apj, which always passes bytes.
• :377-383 — the inner empty-read deadline branch is dead and unpinned: making it always continue instead of raising leaves all 54 tests green, because the loop-top guard raises on the next iteration anyway. Cosmetic; just don't count it as covered.
• Once any USB metadata exists the by-path fallback at :177 is now unreachable — a failed or ambiguous match raises instead. That is the right fail-closed choice and it looks safe for stock ArduPilot (bootloader and application both expand HAL_USB_STRING_SERIAL="%SERIAL%" to the same MCU UDID, and no hwdef overrides it), but it is a policy change worth stating in the PR body. Unconfirmed on hardware.

Checked and clean

Hoisting _capture_linux_persistent_path out of the comports() loop is platform-safe — it returns "" on non-Linux at :116-117 before touching the filesystem. resolve_bootloader_device is only ever called before a transport is opened or after everything, never mid-erase, so the stricter matching carries no brick risk. No protocol constant, framing, CRC, APJ decode or erase/program path changed in this delta, so the previous round's protocol audit still stands.

CI: 17 pass / 1 fail — Publish Tests Results at its coverage step (88 vs fail-under=89), which fails identically on master. Not attributable, as you noted in the thread.

Verdict: COMMENT — no blockers. The two carried-over items are hardware-settled questions, which makes the unticked "Tested on flight controller hardware" box the last real gate on this PR.

[amilcarlucas]

This looks good, thanks

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-09)

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head 433ff9fa58 (previously 35c60747da); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_09_2323/devcall_pr_reviews.html#prMethodicConfigurator-2027

Two of the previous round's findings are resolved and I verified both by mutation. But the commit's macOS dual-CDC fix cannot work on macOS, and the test covering it asserts a port shape pyserial never produces there — so COMMENT → REQUEST CHANGES.

Previous round — triage

Finding	Status	Evidence
~2.3 s bootloader enumeration budget	RESOLVED	Now derived: BOOTLOADER_ENUMERATION_TIMEOUT = 15.0, OPEN_RETRIES = int(15.0/0.5)+1 = 31 retries, about 15 s (:78-81)
Inner empty-read deadline branch dead and unpinned	RESOLVED	Mutation if remaining > 0: to if True: — the exact mutation that stayed green last round — now gives FAILED test_read_timeout_is_bounded_even_when_serial_keeps_returning_empty_reads
ISSUE — USB binding skipped for a symlinked device path	STILL OPEN	Path(...).resolve() was not applied; :136 still string-compares port.device != device. Verified by execution: the same board via /dev/serial/by-id/... yields location='' serial_number='', via /dev/ttyACM0 yields both
NOTE — the has_stable_bootloader_device_identity gate has no regression guard	STILL OPEN	Deleting backend_flightcontroller.py:460-462 outright still leaves the suite byte-identical to baseline
macOS dual-CDC reconnect	STILL OPEN	Attempted here via interface; cannot work — below
contents.encode("utf-8") outside the try	STILL OPEN	:237 still outside; a lone surrogate leaks a bare UnicodeEncodeError. You did widen the caught type to ValueError (a strict superset) and pinned the digit-limit case. Still unreachable from load_apj, which opens "rb"

ISSUE — :175: interface is a mandatory AND-criterion; it cannot disambiguate on any platform, and where non-empty it both refuses correct matches and can select wrong ones

The field is captured from the application firmware at :141 and then required to match the bootloader's enumeration — two different USB configurations. Your own new test docstring at tests/test_bootloader_identity.py:39 states the principle exactly: "An application-port interface identifier cannot safely identify a bootloader port." The code at :175 then does that.

What the field actually contains, traced through the pinned pyserial==3.5:

• Linux — list_ports_linux.py:62 reads the sysfs interface attribute, i.e. the USB iInterface string. Every ArduPilot CDC descriptor sets iInterface to 0 (usbcfg.c:101,145; usbcfg_dualcdc.c:121,161,186), so there is no string and pyserial reports None. Criterion inert.
• Windows — .interface is never assigned anywhere in list_ports_windows.py. Criterion inert.
• macOS — list_ports_osx.py:291 sets it from search_for_locationID_in_interfaces(..., locationID), keyed on the USB device-level locationID that :289-290 also uses for location. Both CDC ports of one board therefore get the same value. It cannot break the tie it was added for.

test_bootloader_port_selects_the_captured_macos_dual_cdc_interface passes only because it hand-sets interface = "0" and "2". Commenting the criterion out makes exactly that one test red and nothing else — so the fabricated value is the test's entire content.

Because the criterion is ANDed, a non-empty captured value the bootloader does not reproduce removes the only correct port; and where two devices share a serial with no location, it can pick the wrong one. Against the real resolver:

two boards, serial "DUP", no location, ours re-enumerated as "Bootloader",
the other still "ArduPilot", identity captured as "ArduPilot"
    at 433ff9fa58                -> OPENED /dev/ttyACM1   (the OTHER board)
    with the criterion removed   -> REFUSED               (fail-closed, correct)

one board, identity interface "ArduPilot", bootloader presents "Bootloader"
    at 433ff9fa58                -> REFUSED               (correct board rejected)
    with the criterion removed   -> OPENED /dev/ttyACM0

To be clear about severity: this is not reachable on stock ArduPilot hardware, because iInterface is 0 and the field is empty. It is a latent hole, not a live wrong-board bug. But it is a wrong-board path added to code whose whole purpose is to make wrong-board impossible, in exchange for no working behaviour on any platform.

Suggested fix, tested here. Drop ("interface", identity.interface) from the all(...) tuple and use it only to break a tie between ports of the same physical device:

            # The interface string is captured from the application firmware, which the
            # bootloader need not reproduce, so it must never exclude the sole candidate.
            # It may only break a tie between ports of the SAME physical device.
            if len(matches) > 1 and identity.interface:
                locations = {getattr(p, "location", "") or "" for p in matches}
                if len(locations) == 1 and locations != {""}:
                    narrowed = [p for p in matches
                                if (getattr(p, "interface", "") or "") == identity.interface]
                    if len(narrowed) == 1:
                        matches = narrowed

That gives 60/60 tests passing and the right answer in all five cases: wrong-board pair refused, correct sole match opened whether the bootloader changes or drops its interface string, macOS dual-CDC with distinct values disambiguated, and macOS dual-CDC with the real identical values refused. Note the simpler "tie-break whenever len(matches) > 1" version is not sufficient — I tried that first and it still opens the wrong board in the duplicate-serial case, because there the tie is between two different physical devices.

Consequence on macOS (traced, not run on a Mac): the bootloader is single-CDC so the flash succeeds; it is reconnect_after_bootloader() at backend_flightcontroller.py:428, running after the board reboots back into the dual-CDC application, that gets two matches, raises, retries and reports "cannot resolve the flight-controller serial device". A successful flash reported as a failure. Linux and Windows are unaffected.

Smaller notes

• tests/test_backend_flightcontroller_bootloader.py:918 — the new budget assertion _open_retries * BOOTLOADER_RETRY_DELAY >= BOOTLOADER_ENUMERATION_TIMEOUT is a tautology given :81 derives OPEN_RETRIES as int(T/D)+1; setting BOOTLOADER_ENUMERATION_TIMEOUT = 2.0 leaves it green. The first half (that the backend default uses the constant) does pin real wiring; assert BOOTLOADER_ENUMERATION_TIMEOUT >= 10.0 would pin the value.
• pytest-timeout is justified, but the reason it is needed is that :389 yields via the module-global time_sleep while the deadline comes from the injected self._clock, so any test injecting a clock gets a real sleep that never advances it. An injectable sleep would remove the need for the marker.
• :183-184 — a by-path-only identity is returned with no cross-check, so whatever now occupies that topology slot is opened. This is the residue of the by-id finding: the users who land here are exactly those whose USB serial was discarded at capture. Fixing :136 closes this too.
• data_model_firmware_upload.py:240 — RecursionError still escapes parse_apj untyped on the production bytes path via deeply nested JSON, well under MAX_APJ_DESCRIPTOR_SIZE. Pre-existing.

Checked and clean

I specifically checked whether a "harden reconnect" commit had reopened the hole, and it has not — it narrows. Trying USB location/serial before persistent_path is now pinned (restoring the old ordering gives FAILED test_bootloader_port_refuses_persistent_path_with_a_different_usb_serial). The scenario matrix against the real resolver — two boards with distinct serials, board A gone with B at A's location, board absent mid-enumeration, clone boards with identical serials, hub reshuffle — is fail-closed throughout. No resource leaks on the new error paths, no half-erase risk, and the new crc()/check_compatibility alignment guards are correctly ordered with real ArduPilot flash sizes unaffected.

CI: 16 pass / 1 fail / 1 skipped. The failure is Publish Tests Results at its coverage step (88 vs fail-under=89), which fails with the byte-identical message on master at 00703cf5e4 — this PR's own merge base. Not attributable, unchanged from last round.

PR body is now materially accurate. Two nits: it says "107 tests" where I measure 112 across the four touched test files, and "Tested on flight controller hardware" is still unticked on code that erases and writes flash. Both remaining identity findings are exactly the kind only hardware settles, so that box matters more than usual here.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-09)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head 1b2662f4de; my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/2026_09_10_AIReview/devcall_pr_reviews.html#prMethodicConfigurator-2027

Verdict: REQUEST CHANGES → COMMENT. You resolved every actionable item from the last round, and I proved each by fetching both heads into worktrees and executing them side by side rather than by reading the diff.

Previous round — triage

Finding	Status	Evidence
ISSUE — interface is a mandatory AND-criterion; refuses correct boards and can select wrong ones	RESOLVED, in exactly the suggested shape	matrix below
ISSUE — USB binding skipped for a symlinked device path	RESOLVED	_serial_port_matches_device() (:154-159) + resolved-device fallback (:135-142). Same board, same mocked port: old → via /dev/ttyACM0 location='1-2.3' serial='FC-123', via by-id symlink identity=None, has_stable=False, upload refused. new → both paths give the location and serial, has_stable=True. Pinned: dropping _serial_port_matches_device from the scan fails a test
NOTE — has_stable_bootloader_device_identity gate has no regression guard	RESOLVED	The mutation I named last round — deleting backend_flightcontroller.py:459-461 — survived at 433ff9fa58 (112 passed) and is caught at 1b2662f4de (1 failed / 117 passed). has_stable → return True also flips survived → caught
NOTE — contents.encode("utf-8") outside the try at :237	RESOLVED	Moved inside (:238), UnicodeError added to the caught tuple (:240). parse_apj('{"board_id": 9, "x": "\ud800"}'): old → bare UnicodeEncodeError; new → FirmwareFileError
NOTE — RecursionError escapes untyped on the production bytes path	RESOLVED	RecursionError added to the same tuple. Deeply nested JSON as bytes: old → untyped RecursionError at depth 100 000 and 400 000 (0.19 / 0.76 MiB, far under the 171.8 MiB cap); new → FirmwareFileError at every depth
NOTE — the budget assertion at :918 is a tautology	RESOLVED, as suggested	tests/test_backend_flightcontroller_bootloader.py:963 now reads assert bl.BOOTLOADER_ENUMERATION_TIMEOUT >= 10.0; setting the timeout to 2.0 survived at the old head and now fails with assert 2.0 >= 10.0
~2.3 s enumeration budget	RESOLVED, no regression	:79-81 unchanged (31 retries ≈ 15.5 s), and now value-pinned rather than merely present
Inner empty-read deadline branch	RESOLVED, no regression	if remaining > 0: → if True: is caught at both heads
macOS dual-CDC reconnect	STILL OPEN — by design	see below
NOTE — non-injectable sleep, so pytest-timeout is load-bearing	STILL OPEN	:411 still calls the module-global time_sleep; BootloaderClient.__init__ still takes no sleep= (only FlightControllerBootloaderBackend does, :625); tests still monkeypatch.setattr(bl, "time_sleep", …)
NOTE — by-path-only identity returned with no cross-check	STILL OPEN, materially narrowed	Fixing the symlink compare closed the dominant case, not the branch. At old a by-id-connected board fell to by-path-only in all three sub-cases; at new it is USB-bound whenever the port reports location or serial, and by-path-only survives only when pyserial reports neither, or the device is absent from comports(). :205-206 still returns the captured path with no check the link still resolves to the same hardware

The interface fix, executed at both heads

You removed ("interface", identity.interface) from the mandatory all(...) tuple and reinstated it as a narrowing tie-break guarded by len(matches) > 1, a single non-empty shared location, and len(narrowed) == 1 — including the locations != {""} guard. Re-running the five-case matrix from my last comment against the real resolver:

                                                     433ff9fa58              1b2662f4de
dup-serial, 2 boards, identity interface="ArduPilot"  OPENED ttyACM1 (other) REFUSED   <- wrong-board path GONE
sole match, bootloader interface differs              REFUSED                OPENED    <- correct-board refusal GONE
sole match, bootloader drops the interface string     REFUSED                OPENED    <- correct-board refusal GONE
macOS dual-CDC, fabricated distinct interfaces        OPENED cu.…14101       OPENED cu.…14101
macOS dual-CDC, REAL identical interfaces             REFUSED                REFUSED   <- fail-closed, unchanged

The first three lines reproduce the old behaviour exactly and all five are now correct. Re-adding the criterion to the tuple fails 2 tests, so it is pinned.

What remains is the inert half, and it is inert on every platform: Linux reads interface from the sysfs iInterface string, which ArduPilot sets to 0 everywhere (usbcfg.c:101,145; usbcfg_dualcdc.c:120,161) → None; Windows never assigns .interface; macOS gives both CDC ports of one board the same value. So the tie-break cannot fire on real hardware, and tests/test_bootloader_identity.py:58-72 and :125-136 pass only because they hand-set interface = "0"/"2". Downgraded to NOTE — not worth blocking on now the dangerous form is gone, but worth a comment saying the criterion is inert on real devices so nobody later trusts it.

macOS dual-CDC reconnect therefore stays open, and that is now a deliberate fail-closed: both ports carry the same interface, so the tie-break cannot narrow and the resolver refuses, raising FirmwareReconnectError after a flash that actually succeeded. That is exactly what my suggested fix produces. Worth deciding explicitly whether a macOS user should see "flash succeeded, reconnect manually" instead of an error.

The one thing I would still close before a UI drives this

• ISSUE — backend_flightcontroller_bootloader.py:496-502: the external-flash CRC comparison has no negative test. Replacing if actual != image.extf_crc(): with if False and … leaves all 186 tests passing, so a board returning the wrong external CRC would be accepted silently. The internal equivalent at :492 is properly pinned — that mutation is caught — which is what makes this one worth closing: the asymmetry looks accidental. A fake returning a corrupted EXTF_GET_CRC value, asserting BootloaderProtocolError, is enough.

Other coverage gaps, found by mutation

• NOTE — :48-72: not one protocol byte constant is pinned. Nine single-byte mutations each left all 186 tests green (GET_DEVICE 0x22→0x26, CHIP_ERASE 0x23→0x26, PROG_MULTI 0x27→0x26, GET_CRC 0x29→0x2a, REBOOT 0x30→0x31, EXTF_ERASE 0x34→0x36, INSYNC 0x12→0x15, OK 0x10→0x1a, EOC 0x20→0x2f). The cause is structural: FakeBootloaderTransport decodes using the same module constants, so it stays self-consistent under any renumbering. All 23 values are correct against Tools/scripts/uploader.py:209-254 — the gap is that a future edit has no guard. One table-driven test asserting the literal bytes closes it. An independent Codex pass found this by the same method.
• NOTE — four more behaviours stay green while broken: forcing _verify_v3 for all revisions (the fake answers GET_CRC regardless of revision; a real rev-2 bootloader has none); making encode_chip_erase ignore full_erase; removing the MAX_IMAGE_SIZE + 1 decompression cap (the post-hoc len(raw) check yields the same error — but the cost is real: on a 1 GiB zlib bomb the mutated code peaks at 2095 MiB RSS / 2.56 s against 176 MiB / 0.13 s as shipped); and removing either the FirmwareReconnectError raise at backend_flightcontroller.py:481-482 or the facade's verify_expected_firmware_digest call at :462.

Carried over, low exposure

• ISSUE — :250-251, :562-563: full_erase=True on any non-F7/H7 board stalls for the full 20 s ERASE_TIMEOUT and then fails, leaving the board held in the bootloader with nothing erased. Mechanism confirmed at source: Tools/AP_Bootloader/bl_protocol.cpp:617-620 compiles case PROTO_CHIP_FULL_ERASE only under #if defined(STM32F7) || defined(STM32H7), so elsewhere 0x40 falls to the switch's default: continue;, which sends no response at all — not even INVALID. Inherited from uploader.py's __erase, and no caller sets full_erase, so exposure today is nil. Gate it on board family or document it as F7/H7-only.
• NOTE — data_model_firmware_upload.py:310: decompressor.flush(MAX_IMAGE_SIZE + 1 - len(raw)) reads as a second output cap but is not one — flush()'s argument is the initial output buffer size. Measured: after decompress(comp, 10) on a 100 000-byte payload, flush(5) returns 99 990 bytes. The bound is really the unconsumed_tail check at :307, and I could not construct a bypass (4095 max_length values brute-forced against a 4 MiB-expanding stream: zero cases with empty unconsumed_tail and pending output). Comprehension hazard, not a hole — drop the argument and say what actually bounds it.
• NOTE — the pure-Python bit-at-a-time CRC measures 2.13 MiB/s (0.470 s per MiB), so FirmwareImage.crc() over a 2 MiB flash with a 1 MiB image is 0.97 s and an 8 MiB external image about 4 s. It is exactly replaceable: bootloader_crc32(d, state) == zlib.crc32(d, state ^ 0xFFFFFFFF) ^ 0xFFFFFFFF proven over 200 random (state, data) pairs — 2881× faster, identical output. Correctness is not in question; this is purely speed.
• NOTE — content_sha256 is still computed and never read in production (:129, :159). Two parser laxities from a 23-case fuzz: trailing bytes after a valid zlib stream are ignored (unused_data unchecked), and a non-string JSON version is str()-ed into the metadata. Both harmless given image_size == len(raw_image) and the trusted digest.
• NOTE — the PR body still says "107 tests"; the four touched test files now hold 118. "Tested on flight controller hardware" is still unticked on code that writes flash — which is what would settle the macOS reconnect question and the full-erase stall.

Checked and clean

The CI failure is not yours: 18 of 19 checks pass, and the sole failure is Publish Tests Results / Check coverage (TOTAL 88 vs --fail-under=89), which fails at the identical step on the merge-base master head 00703cf5e4.

All 23 protocol byte values match uploader.py:209-254. The CRC is byte-identical to uploader.py's crctab implementation across six input sizes including empty and 65 537 bytes, and FirmwareImage.crc(padlen) matches fw.crc(padlen) over a 5×7 grid. PROG_MULTI_MAX = 252 is within the bootloader's flash_buffer[256] (bl_protocol.cpp:501-504, checked at :766). The external-erase progress handling matches bl_protocol.cpp:695-731 exactly, including the guaranteed trailing pct_done = 100 that makes the last_pct >= 90 gate safe. compatible_board_ids = {33: 9} matches uploader.py:116; the 20 s erase and 10 s external-CRC timeouts match; <I little-endian throughout and the >I mutation is caught. Every user-facing string is _()-wrapped. vermin -t=3.10 is clean over the production files against requires-python = ">=3.10".

Method, so you can weigh the above: 55 mutations run against the checkout (never site-packages) on a venv with pyautogui stubbed, baseline 186 passing — 33 caught, 19 survived, 3 equivalent. Both heads were fetched by full SHA into worktrees so before/after behaviour could be executed rather than argued.

Two corrections to my own process, since they affected what I told you: my first pass this round triaged against a three-rounds-stale comment (a211c3f35e) and wrongly reported that the head in my records was bogus — it was not, and redoing the triage against 433ff9fa58 reversed my conclusion from borderline REQUEST CHANGES to COMMENT. And compare/433ff9fa58...1b2662f4de is useless here: the two are sibling versions of the same top commit, so it reports ahead_by 1, behind_by 1 where the real head-to-head diff is 4 files, +120/−4.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-09)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head fa46be448c; my earlier comment above is superseded.

Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_10_0530/devcall_pr_reviews.html#prMethodicConfigurator-2027

Every one of the eleven open items from my last comment is resolved, and I proved each coverage claim by mutation rather than by reading the tests. That is a thorough response and the engineering is good.

Despite that I am moving the verdict COMMENT → REQUEST CHANGES, and I want to be clear it is not because you regressed anything. Two things drive it: a one-word typo in a pylint pragma that is currently breaking CI, and a reconnect defect that predates this push and that my earlier rounds missed.

The two things to fix

1. data_model_firmware_upload.py:227 — malformed pylint pragma, breaking CI.

def parse_apj(...) -> FirmwareImage:  # pylint: too-many-locals

It is missing disable=. The correct form is the one you already use correctly at :119. As written it does two bad things: it emits E0011: Unrecognized file option 'too-many-locals', an error-category message, and it does not suppress R0914: Too many local variables (16/15). Confirmed three ways — reproduced standalone with pylint 4.0.8, found verbatim as the only pylint output in your CI log, and pylint exits 10 = 2 (error) + 8 (refactor), which is exactly the Docker job's exit code. Fix:

def parse_apj(...) -> FirmwareImage:  # pylint: disable=too-many-locals

2. backend_flightcontroller_bootloader.py:166-172 — a board connected through its second CDC port can never be re-found in the bootloader, and the attempt leaves it held there needing a power cycle.

The match requires the captured location to be equal. On Linux pyserial builds location from the USB interface path whenever bNumInterfaces > 1 (list_ports_linux.py:54-58, self.location = os.path.basename(self.usb_interface_path)), and every CDC ACM device has at least two interfaces — so the value carries an interface suffix, 1-2.3:1.0 rather than 1-2.3. A dual-CDC application (any hwdef whose SERIAL_ORDER includes OTG2) exposes :1.0 and :1.2; the bootloader exposes only :1.0. So a user on the second port captures 1-2.3:1.2 and nothing in the bootloader ever matches.

I executed this against your real resolver with realistic pyserial values and interface = None (which is what ArduPilot hardware gives on Linux — iInterface is 0 in every hwdef):

single-CDC, connected on the only port      -> RESOLVED /dev/ttyACM0
dual-CDC,   connected on ttyACM0 (:1.0)     -> RESOLVED /dev/ttyACM0
dual-CDC,   connected on ttyACM1 (:1.2)     -> REFUSED "cannot uniquely locate the bootloader USB device"

Note has_stable_bootloader_device_identity() returns True for that third case, so the pre-flight gate passes and the failure lands after the reboot. Tracing it: _try_open_transport swallows the OSError, all BOOTLOADER_OPEN_RETRIES attempts fail identically, and _wait_for_bootloader raises FirmwareBootloaderRecoveryError, which the facade deliberately re-raises without recovery because hold_in_bootloader has disabled the boot timeout. The user gets correct guidance ("power-cycle"), but no firmware is written and the board needs physical intervention on every attempt.

This is not something this push introduced — I ran the same case at 1b2662f4de and it was refused identically, so the by-path removal neither caused nor worsened it. Suggested fix: compare the USB device portion of location (split at :) instead of the whole value; it still identifies the physical port, and the serial-number criterion already in the tuple does the rest. Found by an independent cold pass and then reproduced here before I wrote it up. Windows is reported to share the mechanism — I could not confirm that, the Windows pyserial backend is not importable on this machine.

Also new, lower stakes

• ARCHITECTURE_firmware_upload.md now describes code this PR deleted, and in one place says the opposite of what the code does. By-path is still promised at :67, :113, :115-116, :176 while persistent_path has zero occurrences in any .py. :120 still promises "a separate payload digest for display" — that was content_sha256, now deleted with zero references. And :110-112 says the captured interface value "is not used as a cross-mode tie-break" while backend_flightcontroller_bootloader.py:178-183 does exactly that when more than one port shares a location. That last one is the mechanism I asked to have documented last round, so doc and code disagree precisely where it matters most.
• :520-522 — the full_erase refusal is a fair call but its stated reason is wrong. The comment says AP_Bootloader "does not report the MCU family" over the wire. It does: bl_protocol.cpp implements PROTO_GET_CHIP 0x2c (MCU IDCODE) at :107/:1020 and PROTO_GET_CHIP_DES 0x2e at :109/:1034, and uploader.py sends both (:429-438) and maps mcu_id & 0xfff to F4/F7 families at :821-848. Refusing unconditionally is reasonable policy for a client that has not implemented that detection — it just is not a protocol necessity, and the comment will mislead whoever next tries capability gating. Two smaller points: the refusal fires at stage IDENTIFYING, i.e. after the board is already in the bootloader, though full_erase is known before any I/O; and CHIP_FULL_ERASE, the full= encoder branch and the parameter on four call layers are now unreachable-on-success. I executed the path and the board is correctly rebooted back to application firmware, so it is a wart, not a hazard.
• :663-687 — a cancellation request is not honoured during bootloader discovery. upload() checks cancellation_requested once at :646, then _wait_for_bootloader() runs the full retry budget with no cancellation callback. BOOTLOADER_ENUMERATION_TIMEOUT is 15 s, so a cancel pressed just after the reboot is ignored for at least that long, longer when each open attempt blocks.
• The sleep injection is not itself pinned. Deleting sleep=self._sleep from :670 leaves all 236 tests passing — the client's use is pinned, the backend's forwarding is not. Residual: backend_flightcontroller.py:321, 338, 419, 431, 440 still call the module-global time_sleep.
• PR body is stale — still "107 tests" (collected count across the five touched test files is now 168) and still describes the by-path fallback. "Tested on flight controller hardware" is still unticked, which is what would settle the second-CDC finding.

Previous round — all resolved, each proved by mutation

Baseline at head: 236 passed in 7.4 s for the six firmware-upload test files, up from 186. Full non-GUI suite 3838 passed, no collateral damage.

Previous finding	Evidence
ISSUE — external-flash CRC has no negative test ("the one thing I'd close before a UI drives this")	test_external_crc_mismatch_rejects_the_upload (tests/…bootloader.py:379). Mutating :483 to if False and … is now CAUGHT (1 failed / 235 passed); internal control at :475 caught too. Asymmetry gone.
NOTE — no protocol byte constant pinned; nine renumberings survived	test_protocol_byte_constants_… (:173-223) pins all 23. All nine previously-surviving mutations now die. All 23 literals re-checked against uploader.py:209-254: correct. Plus a 10-case wire-frame test.
NOTE — four behaviours green while broken	All four now CAUGHT. The zlib-bomb bound still holds despite flush() losing its argument: 1 GiB bomb raises at peak RSS 174 MiB / 0.134 s, matching the pre-change figure.
ISSUE — full_erase stalls 20 s on non-F7/H7	Refused up front; mutation removing the refusal CAUGHT.
NOTE — flush(...) argument reads as a cap	Bare flush() with a comment saying exactly that; real bound (unconsumed_tail) unchanged and mutation-pinned.
NOTE — CRC at 2.13 MiB/s	Now zlib.crc32(data, state ^ 0xFFFFFFFF) ^ 0xFFFFFFFF. I reproduced equivalence over 400 random (state, data) pairs including empty input and chained state — 0 mismatches; ~2400× faster. Also checked end-to-end against a real CubeOrange .apj: padded images byte-identical to uploader.py's and CRCs identical at three flash sizes.
NOTE — non-injectable sleep	sleep= on BootloaderClient.__init__, schedule pinned with assert sleeps == pytest.approx([0.01, 0.01, 0.005]).
NOTE — by-path returned with no cross-check	Removed. I checked this is legitimate rather than cosmetic: 60-serial.rules creates by-path links only for ttyUSB*/ttyACM*, and for exactly those pyserial sets location from a real sysfs basename, which is never empty — so "by-path link exists but location empty" is unreachable. No dangling references anywhere; no test lost.
macOS dual-CDC reconnect	Resolved at the message level, verbatim as suggested, pinned by two tests.
NOTE — content_sha256 unread; trailing bytes; non-string version	All three closed, each mutation CAUGHT. Verified no real ArduPilot .apj is rejected by the new strictness.
NOTE — say the interface tie-break is inert on real hardware	Still open — :174-176 explains the semantics but never says the field is None on Linux and unset on Windows.

CI — six red, and only one is yours

The previous round's reading ("the only failure is the pre-existing coverage gate") no longer applies; that gate never ran.

Check	Cause
pylint (3.10)	Yours — the E0011/R0914 pair above, the only output
Docker Build and Test	Same one line. The suite ran and passed inside Docker (4996 passed, 6 skipped, 82 deselected, 4 xfailed), then the linter step exited 10
pylint (3.14)	Not an independent failure — conclusion cancelled, empty step list, matrix fail-fast after 3.10
pytest (ubuntu 3.10), (3.14)	Infrastructure — Failed to fetch dl.google.com/linux/chrome-stable… Hash Sum mismatch, apt exit 100, before pytest ran
Publish Tests Results	Cascade — the coverage artifact was never produced

So the pragma fix should turn three of the six green, and the other three are a runner package-index problem a re-run will clear.

One thing from the thread, not from me

Copilot's comment that _erase_external should use the padded len(image.extf_image) rather than extf_image_size is wrong. ArduPilot's own uploader does exactly what you do: Tools/scripts/uploader.py:962, self.erase_extflash("Erase ExtF ", fw.property('extf_image_size', 0)). Worth saying so in the thread so it does not get "fixed" into divergence from upstream.

Checked and clean

The deletions are surgical: persistent_path, _capture_linux_persistent_path, sys_platform, content_sha256 and firmware_content_sha256 have zero occurrences in any .py, and import struct was correctly dropped alongside. FirmwareCompatibilityError is genuinely used, not a dangling import. No test was deleted — only two monkeypatch.setattr lines that stubbed the removed helper, and that file net gained a test. ruff passes on all seven changed files under lint.select = ["ALL"]; mypy, ty and both pyright jobs are green. The new reconnect message is correctly formed — _() wraps the whole implicitly-concatenated template rather than a pre-formatted string, one placeholder, one matching kwarg, and that pattern is established in this repo and handled by pygettext3 --keyword=_. Removing content_sha256 weakens nothing: parsing hashes the complete original APJ bytes and verification compares that against the trusted expected digest.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-09)

Automated review note — AI-generated (Claude), validated against the live diff. Please sanity-check before acting.

Re-reviewed at head ce70b955c5; my earlier comment above is superseded. Full report: AIReview, 2026-09-10.

Verdict: REQUEST CHANGES — one new blocker. Everything from the previous four rounds is closed, and the protocol layer I now consider verified rather than plausible.

Resolved since fa46be448c

• Malformed pylint pragma breaking CI → now # pylint: disable=too-many-locals; both pylint jobs and the Docker build are green.
• Second-CDC port unfindable in the bootloader → _physical_usb_location() (backend_flightcontroller_bootloader.py:151-153). Executed against the production resolver: a device captured on :1.2 went REFUSED at the old head → RESOLVED /dev/ttyACM0 here, and reverting the split fails a test. (But see below.)
• ARCHITECTURE doc describing deleted code → rewritten 374 → 113 lines; by-path, persistent_path and content_sha256 now have zero occurrences in the doc and zero in any .py.
• full_erase refusal comment gave a wrong reason → reworded accurately, with the residual sub-points documented in ARCHITECTURE_firmware_upload.md:61-62.
• Cancellation not honoured during discovery → _wait_for_bootloader(cancellation_requested) (:673-686); mutating it to if False: is caught.

New blocker: the dual-CDC fix regressed the reconnect on the same boards

backend_flightcontroller_bootloader.py:167-197, mechanism at :151-153.

Stripping the interface suffix makes the bootloader (one CDC port) findable. But the post-flash reconnect resolves against the application firmware, and a dual-CDC board in application mode exposes two ports sharing the same physical location and the same serial number. matches is then length 2; identity.interface is empty on Linux, so the tie-break at :188 cannot narrow it; and :196 raises OSError("cannot uniquely locate the bootloader USB device") for the whole 15 s budget. The facade turns that into FirmwareReconnectError after firmware has been written and verified, so verify_reconnected_firmware() never runs.

Driven end-to-end through the production facade (real resolve_bootloader_device and capture_serial_device_identity, your own _Connection/FakeBootloaderTransport, comports switching app→bootloader→app) with realistic pyserial values (location='1-2.3:1.0'/'1-2.3:1.2', shared serial_number, interface=None):

port	at fa46be448c	at ce70b955c5
/dev/ttyACM0 (:1.0)	SUCCESS, reconnected	FirmwareReconnectError ← regression
/dev/ttyACM1 (:1.2)	FirmwareBootloaderRecoveryError	FirmwareReconnectError ← improved, flash now happens

ttyACM0 is the port users normally connect on (OTG1 / SERIAL0), so the previously-working path is the one that broke. Windows is affected identically — list_ports_windows.py:366-369 appends ':{}.{}', giving 1-4:x.0 / 1-4:x.2. macOS is unaffected by this change (both its CDC ports already share one location; that ambiguity is the pre-existing, deliberate fail-closed).

Blast radius, measured on ArduPilot master rather than estimated: 87 hwdefs declare OTG2 in SERIAL_ORDER directly, 134 resolve to it after the include chain, and 131 of those ship a bootloader — so roughly 130 application boards, including Pixhawk6X, CUAV-V6X, ARKV6X and mRoPixracerPro. (A second reviewer using ArduPilot's own hwdef parser got 86 and 133; treat the number as ~130 rather than exact.)

This is an availability bug, not a safety one — the physical USB port still cannot match a different board, so no wrong board can be opened.

Suggested fix — prefer an exact location match among the physical-location matches, inserted before the interface tie-break at :188:

if len(matches) > 1 and identity.location:
    exact = [port for port in matches if str(getattr(port, "location", "") or "") == identity.location]
    if len(exact) == 1:
        matches = exact

That keeps the bootloader case working (no exact match there, so the single physical match wins) and restores the reconnect. Both reviewers ran it independently: all four dual-CDC cases resolve correctly with Linux and Windows metadata, the end-to-end facade runs return SUCCESS, they fail again when the patch is removed, and the existing 169 focused tests pass with and without it.

Notes

• The test gap that hid it — tests/test_bootloader_identity.py:58-68 pins the new behaviour with only a single bootloader port present. Your suite does already cover multiple ports from one device, in the macOS tests at :89; the specifically missing case is two application-mode ports with an empty interface (the Linux/Windows shape). One test with two ports at 1-2.3:1.0 / 1-2.3:1.2, same serial, interface=None, asserting the captured port is returned, closes it.
• The head commit message describes code that no longer exists — ce70b955c5 mentions "before using a Linux by-path fallback" and "unverifiable Linux by-path identities". That fallback was deleted two heads ago. Worth fixing before the squash, since the message outlives the thread.
• (re-raised) Sleep injection still not pinned — deleting sleep=self._sleep from :689 leaves all 169 tests green, and backend_flightcontroller.py:419, 431, 440 still call the module-global time_sleep. Cosmetic; the only one of nine mutations that survived.
• (re-raised) The interface tie-break is inert on real hardware and does not say so — :185-187 explains the semantics but never states that the field is None on Linux (every ArduPilot hwdef CDC descriptor has iInterface=0) and unset on Windows, so the branch cannot fire on real ArduPilot hardware. One sentence.
• (re-raised) PR body is stale — it says "107 tests"; the five firmware-upload test files now collect 170. "Tested on flight controller hardware" is still unticked, which matters here because the finding above is a hardware-only failure mode.

What was checked and came out clean

All 23 opcode/status/INFO bytes and PROG_MULTI_MAX = READ_MULTI_MAX = 252 match Tools/scripts/uploader.py:209-254 exactly, and your tests pin all 23 as literals plus 10 full wire frames as literal byte strings — known-good bytes, not encoder/decoder round-tripping, which is the right shape for this. CRC checked numerically twice: bootloader_crc32 vs uploader.crc32 over 500 random (data, state) pairs including empty input, 65 537 bytes and chained state — 0 mismatches; the second reviewer repeated it with 1 000 randomised comparisons plus 100 APJ/internal/external CRC comparisons, also clean. Against a real CubeOrange arducopter.apj (1 497 213 B): padded image byte-identical to uploader.py's (1 644 436 B), crc(1966080) = 0x81365DF8 and crc(2097152) = 0x8125DA3B identical, and 15× faster. Your _pad4 pads with 0xFF, which is what the erased-flash CRC needs — upstream's bytes(0xFF) actually appends 255 zero bytes per iteration, so you are the more correct of the two. <I throughout, and mutating either _encode_external_size or decode_uint32 to >I is caught. identify() and upload ordering, rev-2-vs-rev-3 reboot ACK, ERASE_TIMEOUT = 20.0, EXTF_CRC_TIMEOUT = 10.0, the 2.0 s default and exclusive=True all match upstream. 20 malformed-APJ cases each raise FirmwareFileError with zero unhandled exceptions, and a 1 GiB zlib bomb is rejected in 0.14 s. Nine mutations, eight caught. 3 074 passed / 49 skipped across all backend, data-model and bootloader tests. Copilot's _erase_external size point is a non-issue (uploader.py:962 passes extf_image_size too).

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-10)

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Re-reviewed at head 54fec1a441; my earlier comment above is superseded. Full report: follow-up report.

Verdict: REQUEST CHANGES → COMMENT. The blocker is fixed, and this is the cleanest round on this PR so far: the patch is exactly what was suggested, it works on Linux and Windows, on both ports, and it is pinned by a test that fails when you revert it. Nothing here blocks merge.

Worth stating up front, because it changes how to read the diff: the PR was rebased, so most of the commit range is base movement. git diff ce70b955c5 54fec1a441 is 9 lines of production code plus 50 lines of tests.

Resolved since ce70b955c5

• The dual-CDC application reconnect regression → backend_flightcontroller_bootloader.py:189-192, placed ahead of the interface tie-break. Driven end-to-end through the production facade with interface=None and a shared serial number:

  scenario	port	at 54fec1a441	with :189-192 reverted
  Linux 1-2.3:1.0/1-2.3:1.2	/dev/ttyACM0	SUCCESS	FirmwareReconnectError
  Linux	/dev/ttyACM1	SUCCESS	FirmwareReconnectError
  Windows 1-4:x.0/1-4:x.2	COM7	SUCCESS	FirmwareReconnectError
  Windows	COM8	SUCCESS	FirmwareReconnectError

  The bootloader half still works: identity 1-2.3:1.2 with a single bootloader CDC at 1-2.3:1.0 resolves to /dev/ttyACM0 — no exact match, so the sole physical match wins, exactly as :185-188 claims.

• The test gap that hid it → tests/test_bootloader_identity.py:71-85. Deleting :189-192 gives 1 failed, 171 passed, and the failure is precisely that test.

• Sleep injection → :698 now passes sleep=self._sleep; removing it fails tests/test_backend_flightcontroller_bootloader.py:673. That was the one mutation of nine that survived last round.

• The interface tie-break now says it is inert on real hardware → :196. Accurate: every ArduPilot hwdef CDC descriptor has iInterface=0.

Notes — nothing blocking

• A failure at or after ERASING gives a raw protocol string and no recovery guidance. :594-609 sets bootloader_rebooted / attempts an abort only for IDENTIFYING and AWAITING_CONFIRMATION, so backend_flightcontroller.py:473-483 produces no recovery message for ERASING/PROGRAMMING/VERIFYING/REBOOTING. Not reconnecting is right — the board really is still in the bootloader — but a lost erase ACK, a CRC mismatch and a lost reboot ACK all surface as a bare BootloaderProtocolError: timeout waiting for 2 bootloader bytes. The case that matters is the bootloader answering REBOOT with INSYNC+FAILED: ArduPilot's Tools/AP_Bootloader/bl_protocol.cpp:1147-1194 does goto cmd_fail when flash_write_flush() or the deferred first-word write fails, and because ArduPilot deliberately defers RESERVE_LEAD_WORDS, flash is then programmed except the vector table — the board will not boot, and the app says only "bootloader reports OPERATION FAILED". It is recoverable (RTC_BOOT_HOLD sets timeout = 0, Tools/AP_Bootloader/AP_Bootloader.cpp:112-113, so the bootloader stays held), but nothing tells the user that. Suggest reusing your existing "power-cycle the flight controller before reconnecting" wording for any stage ≥ ERASING, and saying the flash is incomplete.
• abort_before_erase() reports success without reading the answer. :366-372 returns True if the encode_reboot() write succeeded, so a bootloader replying INSYNC+INVALID/FAILED still sets bootloader_rebooted = True and the user is told the board rebooted when it may still be held. Your docstring does say "best-effort", and in this context cmd_fail is unlikely (nothing erased, no deferred first word), so it is the same diagnostics gap as above rather than a flashing hazard.
• The 15 s discovery budget is a sleep budget, not a wall clock. :79-81 gives BOOTLOADER_OPEN_RETRIES = 31, but the per-attempt identify() cost is not counted. With a transport that opens and never answers — a board enumerated but not actually in the bootloader — measured elapsed is 78.1 s against a constant that says 15.0 (77 s analytically at the default timeout=2.0). The loop terminates, cannot spin, and polls cancellation about every 2.5 s, so this is naming rather than a hang. Relatedly tests/test_backend_flightcontroller_bootloader.py:1682 asserts on attempts rather than intervals, so dropping the + 1 leaves all 172 tests green even though the budget falls to 14.5 s — the + 1 is right, it just isn't pinned.
• close() in a finally can mask the real error. close() (:363-364) is unguarded and called from upload()'s finally (:610-611) and :705/:710; a transport whose close() raises replaces a BootloaderProtocolError with a bare SerialException that has no .stage, bypassing the facade's mapping. Caveat: I could not demonstrate a real trigger — pyserial's serialposix.py close() cannot realistically raise. contextlib.suppress is a one-liner if you want it closed anyway.
• (re-raised) A commit message still describes deleted code. by-path has zero occurrences in any .py/.md at this head, but 86639df117's body still says "before using a Linux by-path fallback" and "unverifiable Linux by-path identities". The head commit is clean now, so my previous wording no longer applies — but the text survives in the history and in the PR body.
• (partly re-raised) PR body. "107 tests" → "172 tests" is correct; I reproduced exactly 172. Still wrong: "with a captured Linux by-path fallback only when no USB metadata is available" describes code that does not exist. Still unticked: "Manual testing performed" and "Tested on flight controller hardware" — which matters here, because the finding just closed was a hardware-only failure mode on roughly 130 application boards.
• (re-raised, cosmetic) backend_flightcontroller.py still calls the module-global time_sleep at :321, :338, :419, :431, :440.

Two things the cold reviewer raised that I am not passing on as findings

The independent pass reported the macOS dual-CDC reconnect failure and the lost-reboot-ACK path (backend_flightcontroller.py:413) as bugs. Both are real behaviours, neither is a defect introduced here: the macOS case is your documented deliberate fail-closed, and for the lost ACK, ArduPilot sends the COMMAND_ACK before rebooting (libraries/GCS_MAVLink/GCS_Common.cpp:3696, :3708 — "send ack before we reboot"), so it needs a dropped frame rather than normal operation. Flagging them so you know they were considered and set aside.

Checked and clean

reconnect_after_bootloader (backend_flightcontroller.py:422-441) terminates and cannot spin; connect() reports failure by return value, so the loop genuinely spends its budget — your account of what c99a58014d fixed is correct. _wait_for_bootloader now calls abort_before_erase() only on the last attempt (:703), closing the older Copilot point about rebooting the board and then retrying against it. I had a finding about a leaked open serial port on a non-protocol exception and dropped it before writing it: _write, _read_exact and _reset_input_buffer all translate OSError/SerialException into BootloaderProtocolError, and decode_sync/decode_uint32 raise the same, so the except BootloaderProtocolError in _wait_for_bootloader catches everything and client.close() always runs. Parameter-cache invalidation holds on every error path (disconnect() at :360-362). _erase_external cannot loop forever — the deadline resets only on a strictly increasing percentage.

CI: the one red check is Publish Tests Results, and its failing step is Check coverage — coverage report --fail-under=89 against TOTAL 20863 2149 6324 628 88%. master fails identically at the same 88%, as does every recent master run of pytest.yml, so it is not yours. This corrects what I told you last round, where I described this check as reporting success before an unrelated downstream step failed; it is a repo-wide coverage gate. All four pytest matrix jobs are green (ubuntu 3.10/3.14, macOS 3.14, windows 3.14), plus pylint, mypy, pyright, ruff, CodeQL and the Docker build.

[tridge]

Deprecated — see below for the updated review.

Previous review (2026-09-10)

Automated review note — AI-generated (Claude), independently cross-checked by a second model and re-verified against the live diff. Please sanity-check before acting.

Re-reviewed at head eb720ecc87; my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_10_2312/devcall_pr_reviews.html#prMethodicConfigurator-2027

Four of the five notes from last round are genuinely closed, and closed the way they were suggested — the 77 s discovery budget is now a real 16 s wall clock, post-erase failures carry power-cycle guidance, close() no longer masks, and the facade no longer uses module-global sleeps. Good work.

You pushed again while I was reviewing (8bf6bc422b → eb720ecc87), and that push already fixes one of the two bugs I was about to raise — the revision-2 abort. It's credited as resolved below rather than asked for, and I've re-verified every finding against eb720ecc87; all line numbers below are that head's.

I'm still moving COMMENT → REQUEST CHANGES, for the other one, which is untouched at this head: a one-line argument that collapses bootloader discovery to a single attempt, undoing the very property 86639df117 set out to add.

Resolved

• Recovery guidance at/after ERASING → ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py:604-612 and :617-625. This covers the REBOOT-answered-with-INSYNC+FAILED vector-table hole specifically. Pinned: reverting it fails tests/test_backend_flightcontroller_bootloader.py:2026.
• The 15 s discovery budget was a sleep budget, not a wall clock → measured with a real monotonic clock and a transport that opens and blocks: 77.01 s at 54fec1a441 → 16.07 s here. The 1.07 s overshoot is the last blocking read starting just under the deadline; harmless.
• close() in a finally masking the real error → :367-369 now wraps it in contextlib.suppress(Exception); pinned by :1095. For the record, I flagged that one with an explicit caveat that I couldn't demonstrate a real trigger, and that's still true — it was closed on cheapness, not evidence.
• Module-global time_sleep in the facade → new sleep ctor arg at ardupilot_methodic_configurator/backend_flightcontroller.py:131; all five call sites now use self._sleep. Not covered by any test, though — see the mutation results.

BUG — identify(deadline=deadline) collapses discovery to a single open attempt

ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py:743

_read_exact at :412 computes read_deadline = deadline if deadline is not None else self._clock() + ... — an explicit deadline replaces the per-read timeout. So passing the global 15 s deadline into identify() lets the first attempt that manages to open the port consume the entire budget, after which remaining <= 0 at :730-732 breaks the loop.

Measured against a real clock, with a transport that opens and answers nothing:

	elapsed	open attempts
54fec1a441	77.01 s	31
this PR	16.07 s	1
same, only deadline=deadline dropped from :743	16.50 s	6

So the wall-clock bound survives without it — attempt_timeout = min(self._timeout, remaining) at :733 already provides it.

This matters because each attempt re-runs self._device_resolver(...) at :773, re-resolving the USB identity to whatever node the bootloader occupies now. That is the entire point of the loop during re-enumeration, and taking longer than one attempt's timeout to re-enumerate is the normal case, not an edge case. Demonstrated end-to-end with a board whose bootloader CDC resolves only after 3 s:

• at this head: FAILED after 15.1 s, opens=1 — FirmwareBootloaderRecoveryError: cannot reboot the held bootloader; power-cycle...
• with the one-line change: SUCCESS after 5.1 s, opens=3, and the 15 s bound still held at 16.50 s.

This is the reason for the verdict. It directly undoes what 86639df117 set out to add.

Already fixed by your eb720ecc87 push — credit where due

I had a second BUG written up here and your mid-review push closed it, so I'm recording it as resolved rather than asking for it. At 8bf6bc422b, abort_before_erase() required an INSYNC reply to REBOOT, which a revision-2 bootloader never sends — so declining the confirmation dialog on a rev-2 board turned FirmwareUploadCancelledError into FirmwareBootloaderRecoveryError: cannot reboot the held bootloader with bootloader_rebooted=False, while the board had in fact rebooted. Proven with your own fixture. eb720ecc87 adds expect_ack at :371 and passes expect_ack=info is None or info.protocol_revision != 2 at both upload() call sites (:611, :628), with a new test test_client_accepts_no_ack_reboot_when_pre_erase_abort_uses_protocol_v2. That's exactly the right shape. One call site was missed — see the first item below.

Also worth fixing

• The timed-out abort is handed a deadline that has already expired — and this is the one abort_before_erase() call site that missed the expect_ack fix. :748, abort_before_erase(deadline=deadline). On the discovery-timeout path deadline is by definition in the past, so _read_exact's first check fires before any read — the traced transport events are exactly ['write:REBOOT', 'close'], zero read attempts. So the abort always returns False there and the user is unconditionally told "cannot reboot the held bootloader", even when the reboot went out and was honoured. Note that on this path identify() never succeeded, so the revision is unknown, and expect_ack=info is None or ... resolves unknown to "require the ACK" — the stricter choice. Fixing the expired deadline makes that question live again for rev-2 boards here. Give the abort its own small budget.
• The recovery-message handler can crash on an exception with empty args. :624 and :639 (unchanged at this head): exc.args = (f"{exc.args[0]}; {recovery_message}", *exc.args[1:]). :639 sits in a bare except Exception, so exc.args[0] raises IndexError when args is empty — reproduced with a transport whose write() raises RuntimeError() during PROG_MULTI, yielding IndexError: tuple index out of range, i.e. the handler destroys the diagnostic it exists to improve. Caveat: I could not find a production path that produces an empty-args exception, so real reachability needs a third-party transport, MemoryError or StopIteration. Guard is one token: exc.args[0] if exc.args else "".
  Second, measurable half of the same point: mutating .args does not change str() for OSError subclasses — OSError(5, "Input/output error") still prints [Errno 5] Input/output error after the rewrite — and serial.SerialException subclasses OSError, so any un-translated one reaching :639 loses the advice entirely. Building a new exception, or storing the advice on an attribute the facade reads, is more robust than rewriting args.

NOTE — the upload's per-command timeout is inherited from the leftover discovery budget

:733 and :738-742. attempt_timeout is used both to open the port and as BootloaderClient._timeout for the entire erase/program/verify/reboot sequence, and the programming loop calls _command(encode_prog_multi(chunk)) with no explicit timeout. Measured with an injected clock and a port enumerating only at t = 14.8 s, the client's timeout for the whole upload is 0.1 s. With the shipped defaults the floor is ~0.4–0.5 s, so this is unlikely to bite in practice, and ERASE_TIMEOUT/EXTF_CRC_TIMEOUT are passed explicitly and unaffected — but a user-supplied bootloader_timeout is silently reduced by an unrelated budget at exactly the moment the board is being slow. Two different timeouts are being conflated.

Mutation testing — five genuine coverage gaps

Measured at 8bf6bc422b; your mid-review push doesn't touch any of them. Every production hunk reverted in the checkout, full 243-test PR suite re-run. Caught: the close() suppression, the abort ACK, the FirmwareUploadError recovery branch, the backend clock injection, retry_would_exhaust_budget. Survived with nothing failing: the generic-exception recovery branch (the one containing the IndexError above); the retry-sleep clamp; _try_open_transport(timeout) at :734 — its test's assert all(timeout <= 2.0 ...) is vacuous, since self._timeout is 2.0 there; and the entire facade sleep-injection fix, both the sleep= pass-through and all five self._sleep call sites. In fairness, reverting the whole old discovery loop together is caught by test_backend_bootloader_discovery_uses_a_wall_clock_budget, so the new test does pin the original 77 s bug.

Still open from before

• (re-raised) 86639df117 still describes "a Linux by-path fallback" and "unverifiable Linux by-path identities"; by-path has zero occurrences in any .py/.md at this head.
• (re-raised) The PR body still promises that by-path fallback, says "172 tests" where I measure 175, and leaves Manual testing performed and Tested on flight controller hardware unticked. Those two boxes matter more this round, not less — this delta changes the pre-erase abort and the discovery loop, and both bugs above are hardware-shaped.

Checked and cleared

_read_exact's deadline is checked at the top of every loop iteration, so Copilot's inline note about partial reads resetting it is stale — a dribbling transport can't extend it. _erase_external still can't loop forever. The except BootloaderProtocolError in _wait_for_bootloader still catches everything the IO helpers raise, so close() always runs and no port leaks. str() on FirmwareUploadError does follow the mutated args, so the recovery text reaches the facade for the types that matter. The dual-CDC identity fix from last round and its regression test are untouched and still pass, and .interface remains correctly documented as inert on ArduPilot hardware. Nothing in the delta touches APJ parsing, the trusted-digest path, or the stable-identity gate. Locally: ruff check clean, ruff format --check clean, 802 passed / 49 skipped across every firmware and bootloader test file (only failures are tkinter $DISPLAY errors from a headless box).

CI at eb720ecc87: 15 check-runs reconciled against total_count — 6 passing, 0 failing, 0 cancelled, 9 pending; the run was still in flight, so I also ran the linters locally at the reviewed head. Re-verified rather than assumed: the last six pytest.yml runs on master all conclude failure, and in the most recent completed one all four pytest matrix jobs pass with only Publish Tests Results failing — the --fail-under=89 coverage gate is repo-wide and pre-existing, not yours.

Verdict: REQUEST CHANGES

[tridge]

Automated review note — AI-generated (Claude), validated against the live diff (Claude + Codex cross-checked). Please sanity-check before acting.

Re-reviewed at head fbfc7a458e (previously eb720ecc87); my earlier comment above is superseded. Full report: https://uav.tridgell.net/DevCallReviews/followups/2026_09_14_0747/devcall_pr_reviews.html#prMethodicConfigurator-2027

Last round's BUG is fixed, and so are the other three code findings. One new problem: this push turns pylint red. The verdict moves REQUEST CHANGES → COMMENT, but the pylint failure needs fixing before merge.

Previous findings

Previous finding	Status
BUG — identify(deadline=deadline) collapses discovery to one open attempt	RESOLVED — ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py:806-807 gives each attempt its own deadline. Same real-clock repro as last round: a board whose bootloader appears after 3 s now succeeds in 5.0 s with 3 opens (was: failed after 16.1 s with 1 open). A silent transport now gets 6 opens in 16.5 s.
Timed-out abort handed an already-expired deadline	RESOLVED — :814-815 gives it its own 2 s budget. The REBOOT ACK is now read and bootloader_rebooted=True is set (was: 0 reads, "cannot reboot").
Recovery message: IndexError on empty args, and OSError str() losing the advice	RESOLVED — _append_recovery_message at :109-135. The guidance now survives in str() for OSError(5, ...), FileNotFoundError (filename kept), SerialException and empty-args exceptions.
NOTE — upload per-command timeout inherited from the leftover discovery budget	RESOLVED — :798 uses self._timeout, and :808 restores pyserial timeout and write_timeout. With the port appearing at t=14.8 s, the upload timeout was effectively 0 s and is now 2.0 s.
Coverage gaps: generic-exception recovery branch, retry-sleep clamp	RESOLVED — both are now caught by a test when reverted.
Coverage gap: facade sleep injection	PARTIAL — the pass-through, entry and reconnector sleeps are now pinned. reset_and_reconnect's self._sleep(0.3) and self._sleep(1) at backend_flightcontroller.py:399 and :416 can still be reverted with nothing failing.
Coverage gap: _try_open_transport(attempt_timeout)	STILL OPEN (re-raised) — the assert all(timeout <= 2.0 ...) at tests/test_backend_flightcontroller_bootloader.py:1850 is unchanged. 2.0 equals self._timeout there, so it cannot fail.
Commit 86639df117 describes a "Linux by-path fallback"	STILL OPEN (re-raised) — by-path still appears in no .py or .md file.
PR body: by-path promise, "172 tests", manual/hardware testing unticked	STILL OPEN (re-raised) — I count 179 tests at this head. The hardware box matters again: this delta rewrites discovery timing, the abort path and the transport timeouts.

Must fix — pylint regression (CI red)

• ardupilot_methodic_configurator/backend_flightcontroller.py:442 — the refactor narrowed # pylint: disable=too-many-arguments,too-many-locals,too-many-statements to just too-many-arguments, but upload_apj_firmware still has 24 locals. pylint (3.14) and Docker Build and Test both fail with R0914: Too many local variables (24/15) on that line. Both passed at eb720ecc87. Restore too-many-locals, or move the entry/reconnector/backend construction into a helper.

Worth fixing

• The fixes that close the BUG have no tests. Reverting any one of these leaves the PR's tests green (both reviewers reproduced this independently):

  • the per-attempt identify deadline, backend_flightcontroller_bootloader.py:806-807;
  • the abort's own budget, :814;
  • the client timeout=self._timeout, :798;
  • the OSError branch of _append_recovery_message, :118-128.

  Suggested test: an injected clock and a resolver that returns a silent port until t≈3 s, then a working bootloader. Assert success with more than one open. That test fails at eb720ecc87.

• Doc nit: ARCHITECTURE_firmware_upload.md:96-97 calls the new collaborators "injectable". _BootloaderEntryCoordinator and _BootloaderReconnector are built inline in upload_apj_firmware (backend_flightcontroller.py:479-493), with no way to substitute them. "Dedicated collaborators", your commit message's wording, would be accurate.

Note (no change needed)

• On the discovery-timeout path the abort passes expect_ack=True (backend_flightcontroller_bootloader.py:815). The protocol revision is unknown there, so a rev-2 board that did reboot is still told "cannot reboot … power-cycle". Requiring the ACK is the defensible choice, but the message could say "could not confirm the reboot" instead. This is not a regression: the previous call required the ACK too.

Checked and cleared

The refactor preserves behaviour:

• progress reports still go through report_progress_safely;
• entry.entered is set at the same point as the old entered_bootloader;
• the recovery-error re-raise, reconnect-after-flash and FirmwareReconnectError translation are unchanged;
• identity matching and the transport close() are untouched;
• no dead code is left behind.

Locally: 248 PR tests pass, 555 passed / 49 skipped across 15 related test files, and ruff check and ruff format --check are clean.

CI at fbfc7a458e: 18 check-runs — 14 passing, 3 failing, 1 cancelled. The failures are pylint (3.14) and Docker Build and Test (new, above) and Publish Tests Results, which is the repo-wide --fail-under=89 coverage gate and fails on master too. pylint (3.10) was cancelled by fail-fast.

Verdict: COMMENT (fix the pylint failure before merge)