From badc9c9da1a3c97342157d4b539f77443e2f6498 Mon Sep 17 00:00:00 2001 From: Billard <82095453+iacker@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:57:31 +0200 Subject: [PATCH 1/4] feat(firmware-upload): add data model, APJ reader and bootloader codec Signed-off-by: Billard <82095453+iacker@users.noreply.github.com> --- .../data_model_firmware_upload.py | 344 ++++++++++++++++ tests/test_data_model_firmware_upload.py | 368 ++++++++++++++++++ 2 files changed, 712 insertions(+) create mode 100755 ardupilot_methodic_configurator/data_model_firmware_upload.py create mode 100755 tests/test_data_model_firmware_upload.py diff --git a/ardupilot_methodic_configurator/data_model_firmware_upload.py b/ardupilot_methodic_configurator/data_model_firmware_upload.py new file mode 100755 index 000000000..a45f274e9 --- /dev/null +++ b/ardupilot_methodic_configurator/data_model_firmware_upload.py @@ -0,0 +1,344 @@ +#!/usr/bin/env python3 + +""" +Firmware upload domain model: APJ image decoding, bootloader protocol codec and validation rules. + +No serial, MAVLink, Tkinter or dialogs live here. The backend adapter performs the I/O and +feeds bytes in and out of the pure functions defined in this module. + +The protocol constants and the image normalisation mirror ArduPilot Tools/scripts/uploader.py so the +backend can follow the same erase/program/verify sequence as the reference uploader. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +import base64 +import json +import struct +import zlib +from binascii import crc32 +from dataclasses import dataclass +from enum import Enum +from pathlib import Path + +from ardupilot_methodic_configurator import _ + +# Bootloader protocol bytes, from ArduPilot Tools/scripts/uploader.py +INSYNC = b"\x12" +EOC = b"\x20" +OK = b"\x10" +FAILED = b"\x11" +INVALID = b"\x13" +BAD_SILICON_REV = b"\x14" + +GET_SYNC = b"\x21" +GET_DEVICE = b"\x22" +CHIP_ERASE = b"\x23" +CHIP_VERIFY = b"\x24" +PROG_MULTI = b"\x27" +READ_MULTI = b"\x28" +GET_CRC = b"\x29" +REBOOT = b"\x30" +CHIP_FULL_ERASE = b"\x40" + +INFO_BL_REV = b"\x01" +INFO_BOARD_ID = b"\x02" +INFO_BOARD_REV = b"\x03" +INFO_FLASH_SIZE = b"\x04" +INFO_EXTF_SIZE = b"\x06" + +BL_REV_MIN = 2 +BL_REV_MAX = 5 +PROG_MULTI_MAX = 252 # protocol max is 255, must be a multiple of 4 +READ_MULTI_MAX = 252 + +# Sanity bound on decompressed image size, well above any current flight controller flash +MAX_IMAGE_SIZE = 64 * 1024 * 1024 + + +class FirmwareUploadError(Exception): + """Base class for typed firmware upload errors, `stage` names where it happened.""" + + stage = "unknown" + + +class FirmwareFileError(FirmwareUploadError): + """The firmware file is unreadable, malformed or not a supported format.""" + + stage = "inspecting" + + +class FirmwareCompatibilityError(FirmwareUploadError): + """The image does not match the connected board or does not fit in flash.""" + + stage = "identifying" + + +class BootloaderProtocolError(FirmwareUploadError): + """The bootloader answered something unexpected, or a protocol revision is unsupported.""" + + stage = "identifying" + + +@dataclass(frozen=True) +class FirmwareImageMetadata: + """What the user sees before confirming an upload, read from the APJ descriptor and never from the filename.""" + + path: Path + board_id: int + image_size: int + extf_image_size: int + firmware_version: str + git_identity: str + board_revision: int | None = None + + +@dataclass(frozen=True) +class FirmwareImage: + """Decoded, 4-byte padded image ready for the bootloader.""" + + metadata: FirmwareImageMetadata + image: bytes + extf_image: bytes = b"" + + def crc(self, flash_size: int) -> int: + """CRC32 of the image padded with 0xFF up to flash_size, as computed by the bootloader GET_CRC.""" + state = crc32(self.image, 0) + for _unused in range(len(self.image), flash_size - 1, 4): + state = crc32(b"\xff\xff\xff\xff", state) + return state + + +@dataclass(frozen=True) +class BootloaderInfo: + """Answers to GET_DEVICE queries during identification.""" + + protocol_revision: int + board_id: int + board_revision: int + flash_size: int + extf_size: int = 0 + + +class UploadStage(Enum): + """Workflow state, transitions are validated by `next_stage`.""" + + IDLE = "idle" + INSPECTING = "inspecting" + AWAITING_CONFIRMATION = "awaiting_confirmation" + ENTERING_BOOTLOADER = "entering_bootloader" + IDENTIFYING = "identifying" + ERASING = "erasing" + PROGRAMMING = "programming" + VERIFYING = "verifying" + REBOOTING = "rebooting" + RECONNECTING = "reconnecting" + COMPLETED = "completed" + FAILED = "failed" + CANCELLED = "cancelled" + + +_HAPPY_PATH = [ + UploadStage.IDLE, + UploadStage.INSPECTING, + UploadStage.AWAITING_CONFIRMATION, + UploadStage.ENTERING_BOOTLOADER, + UploadStage.IDENTIFYING, + UploadStage.ERASING, + UploadStage.PROGRAMMING, + UploadStage.VERIFYING, + UploadStage.REBOOTING, + UploadStage.RECONNECTING, + UploadStage.COMPLETED, +] + +# Cancellation is only allowed before the flash is touched. Once erase started, the only +# ways out are completed or failed, so a half written flash is never reported as cancelled. +_CANCELLABLE = frozenset(_HAPPY_PATH[: _HAPPY_PATH.index(UploadStage.ERASING)]) +_TERMINAL = frozenset({UploadStage.COMPLETED, UploadStage.FAILED, UploadStage.CANCELLED}) + + +def next_stage(current: UploadStage, target: UploadStage) -> UploadStage: + """Return target if the transition is legal, raise ValueError otherwise.""" + if current in _TERMINAL: + msg = _("{stage} is terminal").format(stage=current.value) + raise ValueError(msg) + if target == UploadStage.FAILED: + return target + if target == UploadStage.CANCELLED: + if current in _CANCELLABLE: + return target + msg = _("cannot cancel during {stage}").format(stage=current.value) + raise ValueError(msg) + if current == UploadStage.VERIFYING and target == UploadStage.ERASING: + return target + if _HAPPY_PATH.index(target) == _HAPPY_PATH.index(current) + 1: + return target + msg = _("illegal transition {current} -> {target}").format(current=current.value, target=target.value) + raise ValueError(msg) + + +def load_apj(path: Path) -> FirmwareImage: + """Read and decode an APJ file, raising FirmwareFileError before any serial I/O can happen.""" + if path.suffix.lower() != ".apj": + msg = _("unsupported firmware format {suffix}, only .apj is supported").format(suffix=path.suffix) + raise FirmwareFileError(msg) + try: + desc = json.loads(path.read_text(encoding="utf-8")) + except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: + msg = _("cannot read APJ descriptor: {error}").format(error=exc) + raise FirmwareFileError(msg) from exc + if not isinstance(desc, dict): + msg = _("APJ descriptor is not a JSON object") + raise FirmwareFileError(msg) + + image = _decode_blob(desc, "image") + extf_image = _decode_blob(desc, "extf_image") if "extf_image" in desc else b"" + + try: + board_id = int(desc["board_id"]) + image_size = int(desc["image_size"]) + extf_image_size = int(desc.get("extf_image_size", 0)) + except (KeyError, TypeError, ValueError) as exc: + msg = _("APJ descriptor is missing or has invalid metadata: {error}").format(error=exc) + raise FirmwareFileError(msg) from exc + if board_id < 0 or image_size < 0 or extf_image_size < 0 or (image_size == 0 and extf_image_size == 0): + msg = _("APJ metadata values are out of range") + raise FirmwareFileError(msg) + if image_size > len(image) or extf_image_size > len(extf_image): + msg = _("APJ image_size does not match the decoded image") + raise FirmwareFileError(msg) + + board_rev = desc.get("board_revision") + metadata = FirmwareImageMetadata( + path=path, + board_id=board_id, + image_size=image_size, + extf_image_size=extf_image_size, + firmware_version=str(desc.get("version", "")), + git_identity=str(desc.get("git_identity", "")), + board_revision=int(board_rev) if board_rev is not None else None, + ) + return FirmwareImage(metadata=metadata, image=image, extf_image=extf_image) + + +def _decode_blob(desc: dict, key: str) -> bytes: + try: + raw = zlib.decompress(base64.b64decode(desc[key], validate=True), bufsize=MAX_IMAGE_SIZE) + except (KeyError, TypeError, ValueError, zlib.error) as exc: + msg = _("APJ {key} is not valid base64+zlib data: {error}").format(key=key, error=exc) + raise FirmwareFileError(msg) from exc + if len(raw) > MAX_IMAGE_SIZE: + msg = _("APJ {key} exceeds {limit} bytes").format(key=key, limit=MAX_IMAGE_SIZE) + raise FirmwareFileError(msg) + return _pad4(raw) + + +def _pad4(data: bytes) -> bytes: + return data + b"\xff" * (-len(data) % 4) + + +def check_compatibility(image: FirmwareImage, bootloader: BootloaderInfo, *, force: bool = False) -> None: + """Raise FirmwareCompatibilityError unless the image can safely be flashed on this bootloader.""" + if not BL_REV_MIN <= bootloader.protocol_revision <= BL_REV_MAX: + msg = _("unsupported bootloader protocol revision {revision}").format(revision=bootloader.protocol_revision) + raise BootloaderProtocolError(msg) + meta = image.metadata + if meta.board_id != bootloader.board_id and not force: + msg = _("firmware is for board_id {image_id}, board reports {board_id}").format( + image_id=meta.board_id, board_id=bootloader.board_id + ) + raise FirmwareCompatibilityError(msg) + if meta.image_size > bootloader.flash_size: + msg = _("image of {size} bytes exceeds flash of {flash} bytes").format( + size=meta.image_size, flash=bootloader.flash_size + ) + raise FirmwareCompatibilityError(msg) + if meta.extf_image_size > bootloader.extf_size: + msg = _("external image of {size} bytes exceeds external flash of {extf} bytes").format( + size=meta.extf_image_size, extf=bootloader.extf_size + ) + raise FirmwareCompatibilityError(msg) + + +# Bootloader protocol codec. Encoders build the bytes to write, decoders check what came back. + + +def encode_get_sync() -> bytes: + """GET_SYNC command.""" + return GET_SYNC + EOC + + +def encode_get_device(param: bytes) -> bytes: + """GET_DEVICE command for one INFO_* parameter.""" + return GET_DEVICE + param + EOC + + +def encode_chip_erase(*, full: bool = False) -> bytes: + """CHIP_ERASE, or CHIP_FULL_ERASE when full is set.""" + return (CHIP_FULL_ERASE if full else CHIP_ERASE) + EOC + + +def encode_prog_multi(chunk: bytes) -> bytes: + """PROG_MULTI command carrying one chunk of at most PROG_MULTI_MAX bytes.""" + if not 0 < len(chunk) <= PROG_MULTI_MAX or len(chunk) % 4: + msg = _("PROG_MULTI chunk must be 4..{limit} bytes and a multiple of 4, got {size}").format( + limit=PROG_MULTI_MAX, size=len(chunk) + ) + raise ValueError(msg) + return PROG_MULTI + bytes([len(chunk)]) + chunk + EOC + + +def encode_read_multi(length: int) -> bytes: + """READ_MULTI command for length bytes.""" + if not 0 < length <= READ_MULTI_MAX: + msg = _("READ_MULTI length must be 1..{limit}, got {length}").format(limit=READ_MULTI_MAX, length=length) + raise ValueError(msg) + return READ_MULTI + bytes([length]) + EOC + + +def encode_get_crc() -> bytes: + """GET_CRC command.""" + return GET_CRC + EOC + + +def encode_reboot() -> bytes: + """REBOOT command.""" + return REBOOT + EOC + + +def decode_sync(reply: bytes) -> None: + """Validate the two byte INSYNC/status reply that follows every command, raising on anything but OK.""" + if len(reply) < 2: + msg = _("short reply, expected INSYNC and status, got {reply}").format(reply=reply) + raise BootloaderProtocolError(msg) + if reply[0:1] != INSYNC: + msg = _("expected INSYNC, got {byte}").format(byte=reply[0:1]) + raise BootloaderProtocolError(msg) + status = reply[1:2] + if status == OK: + return + messages = { + INVALID: _("bootloader reports INVALID OPERATION"), + FAILED: _("bootloader reports OPERATION FAILED"), + BAD_SILICON_REV: _("programming not supported for this silicon revision"), + } + msg = messages.get(status) or _("unexpected status {status} instead of OK").format(status=status) + raise BootloaderProtocolError(msg) + + +def decode_uint32(reply: bytes) -> int: + """Little endian u32 as returned by GET_DEVICE and GET_CRC, before the sync bytes.""" + if len(reply) < 4: + msg = _("short reply, expected 4 bytes, got {size}").format(size=len(reply)) + raise BootloaderProtocolError(msg) + return int(struct.unpack(" list[bytes]: + """Split a padded image into PROG_MULTI sized chunks, in flash order.""" + return [image[i : i + PROG_MULTI_MAX] for i in range(0, len(image), PROG_MULTI_MAX)] diff --git a/tests/test_data_model_firmware_upload.py b/tests/test_data_model_firmware_upload.py new file mode 100755 index 000000000..aa173cdf8 --- /dev/null +++ b/tests/test_data_model_firmware_upload.py @@ -0,0 +1,368 @@ +#!/usr/bin/env python3 + +""" +Unit tests for the firmware upload domain model. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +import base64 +import json +import struct +import zlib +from binascii import crc32 +from pathlib import Path + +import pytest + +from ardupilot_methodic_configurator import data_model_firmware_upload as fw + +# pylint: disable=redefined-outer-name, too-few-public-methods, too-many-return-statements + + +def write_apj(tmp_path: Path, image: bytes, **overrides: object) -> Path: + desc: dict[str, object] = { + "board_id": 9, + "image_size": len(image), + "image": base64.b64encode(zlib.compress(image)).decode(), + "version": "4.6.0", + "git_identity": "abc1234", + } + desc.update(overrides) + path = tmp_path / "arducopter.apj" + path.write_text(json.dumps(desc), encoding="utf-8") + return path + + +@pytest.fixture +def small_image() -> bytes: + return bytes(range(256)) * 3 + b"\x01\x02\x03" # 771 bytes, not a multiple of 4 + + +class TestApjLoading: + """User selects a firmware file and the model tells them what it is before touching the board.""" + + def test_user_sees_metadata_from_a_valid_apj_file(self, tmp_path: Path, small_image: bytes) -> None: + """ + Test user sees metadata from a valid apj file. + + GIVEN: a well formed APJ file + WHEN: it is loaded + THEN: metadata comes from the descriptor and the image is padded to 4 bytes with 0xFF + """ + image = fw.load_apj(write_apj(tmp_path, small_image)) + + assert image.metadata.board_id == 9 + assert image.metadata.image_size == len(small_image) + assert image.metadata.firmware_version == "4.6.0" + assert image.image == small_image + b"\xff" + assert len(image.image) % 4 == 0 + assert image.extf_image == b"" + + def test_user_can_load_an_external_only_apj_file(self, tmp_path: Path) -> None: + """ + An APJ may contain only an external-flash image. + + GIVEN: An APJ descriptor with an empty internal image and external payload + WHEN: It is loaded for firmware upload + THEN: The empty internal region is accepted and the external payload is preserved + """ + external_image = b"ext" + image = bootloader.load_apj( + write_apj( + tmp_path, + b"", + extf_image_size=len(external_image), + extf_image=base64.b64encode(zlib.compress(external_image)).decode(), + ) + ) + + assert image.metadata.image_size == 0 + assert image.metadata.extf_image_size == len(external_image) + assert image.image == b"" + assert image.extf_image == external_image + b"\xff" + + def test_user_gets_a_clear_error_for_a_non_apj_file(self, tmp_path: Path) -> None: + """ + Test user gets a clear error for a non apj file. + + GIVEN: a .bin file + WHEN: the user tries to load it + THEN: a typed file error explains only .apj is supported + """ + path = tmp_path / "arducopter.bin" + path.write_bytes(b"\x00" * 16) + + with pytest.raises(fw.FirmwareFileError, match=r"only \.apj"): + fw.load_apj(path) + + @pytest.mark.parametrize( + ("content", "reason"), + [ + ("not json", "descriptor"), + ("[1, 2]", "not a JSON object"), + (json.dumps({"board_id": 9, "image_size": 4, "image": "!!notbase64!!"}), "base64"), + (json.dumps({"board_id": 9, "image_size": 4, "image": base64.b64encode(b"notzlib").decode()}), "zlib"), + (json.dumps({"image_size": 4, "image": base64.b64encode(zlib.compress(b"abcd")).decode()}), "metadata"), + ( + json.dumps({"board_id": 9, "image_size": 99, "image": base64.b64encode(zlib.compress(b"abcd")).decode()}), + "image_size", + ), + ], + ) + def test_malformed_apj_is_rejected_before_any_serial_access(self, tmp_path: Path, content: str, reason: str) -> None: + """ + Test malformed apj is rejected before any serial access. + + GIVEN: a corrupt or inconsistent APJ file + WHEN: it is loaded + THEN: a FirmwareFileError names the problem + """ + path = tmp_path / "bad.apj" + path.write_text(content, encoding="utf-8") + + with pytest.raises(fw.FirmwareFileError, match=reason): + fw.load_apj(path) + + def test_oversized_image_is_rejected(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """ + Test oversized image is rejected. + + GIVEN: an APJ whose image decompresses beyond the sanity bound + WHEN: it is loaded + THEN: it is refused instead of allocating the whole thing + """ + monkeypatch.setattr(fw, "MAX_IMAGE_SIZE", 1024) + big = b"\x00" * 4096 + path = write_apj(tmp_path, big) + + with pytest.raises(fw.FirmwareFileError, match="exceeds"): + fw.load_apj(path) + + def test_crc_matches_the_reference_uploader_computation(self, tmp_path: Path, small_image: bytes) -> None: + """ + Test crc matches the reference uploader computation. + + GIVEN: a loaded image and a flash size larger than the image + WHEN: the CRC is computed + THEN: it equals crc32 over the image padded with 0xFF up to flash size, as Tools/scripts/uploader.py does + """ + image = fw.load_apj(write_apj(tmp_path, small_image)) + flash_size = 2048 + expected = crc32(image.image + b"\xff" * (flash_size - len(image.image)), 0) + + assert image.crc(flash_size) == expected + + +class TestCompatibility: + """The model refuses to flash anything that does not match the detected board.""" + + @pytest.fixture + def image(self, tmp_path: Path, small_image: bytes) -> fw.FirmwareImage: + return fw.load_apj(write_apj(tmp_path, small_image)) + + def test_matching_board_and_enough_flash_is_accepted(self, image: fw.FirmwareImage) -> None: + fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 2048)) + + def test_board_id_mismatch_is_refused_by_default(self, image: fw.FirmwareImage) -> None: + with pytest.raises(fw.FirmwareCompatibilityError, match="board_id 9, board reports 42"): + fw.check_compatibility(image, fw.BootloaderInfo(5, 42, 0, 2048)) + + def test_board_id_mismatch_is_allowed_only_with_explicit_force(self, image: fw.FirmwareImage) -> None: + fw.check_compatibility(image, fw.BootloaderInfo(5, 42, 0, 2048), force=True) + + def test_image_larger_than_flash_is_refused_even_when_forced(self, image: fw.FirmwareImage) -> None: + with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): + fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 512), force=True) + + @pytest.mark.parametrize("revision", [1, 6]) + def test_unsupported_bootloader_revision_is_refused(self, image: fw.FirmwareImage, revision: int) -> None: + with pytest.raises(fw.BootloaderProtocolError, match="revision"): + fw.check_compatibility(image, fw.BootloaderInfo(revision, 9, 0, 2048)) + + +class TestStateMachine: + """Cancellation is only offered while it is still safe.""" + + def test_happy_path_walks_every_stage_in_order(self) -> None: + stage = fw.UploadStage.IDLE + for target in list(fw.UploadStage)[1:11]: + stage = fw.next_stage(stage, target) + assert stage is fw.UploadStage.COMPLETED + + def test_user_can_cancel_before_the_flash_is_erased(self) -> None: + assert fw.next_stage(fw.UploadStage.AWAITING_CONFIRMATION, fw.UploadStage.CANCELLED) is fw.UploadStage.CANCELLED + assert fw.next_stage(fw.UploadStage.IDENTIFYING, fw.UploadStage.CANCELLED) is fw.UploadStage.CANCELLED + + def test_dual_region_upload_can_return_to_erasing_for_the_internal_image(self) -> None: + """The state machine permits external verification before internal flashing.""" + stage = fw.UploadStage.IDLE + for target in ( + fw.UploadStage.INSPECTING, + fw.UploadStage.ENTERING_BOOTLOADER, + fw.UploadStage.IDENTIFYING, + fw.UploadStage.AWAITING_CONFIRMATION, + fw.UploadStage.ERASING, + fw.UploadStage.PROGRAMMING, + fw.UploadStage.VERIFYING, + fw.UploadStage.ERASING, + fw.UploadStage.PROGRAMMING, + fw.UploadStage.VERIFYING, + fw.UploadStage.REBOOTING, + fw.UploadStage.RECONNECTING, + fw.UploadStage.COMPLETED, + ): + stage = fw.next_stage(stage, target) + + assert stage is fw.UploadStage.COMPLETED + + @pytest.mark.parametrize("stage", [fw.UploadStage.ERASING, fw.UploadStage.PROGRAMMING, fw.UploadStage.VERIFYING]) + def test_user_cannot_cancel_once_the_flash_is_being_written(self, stage: fw.UploadStage) -> None: + with pytest.raises(ValueError, match="cannot cancel"): + fw.next_stage(stage, fw.UploadStage.CANCELLED) + + def test_any_stage_can_fail(self) -> None: + assert fw.next_stage(fw.UploadStage.PROGRAMMING, fw.UploadStage.FAILED) is fw.UploadStage.FAILED + + def test_skipping_a_stage_is_rejected(self) -> None: + with pytest.raises(ValueError, match="illegal transition"): + fw.next_stage(fw.UploadStage.INSPECTING, fw.UploadStage.ERASING) + + def test_terminal_stages_do_not_move(self) -> None: + with pytest.raises(ValueError, match="terminal"): + fw.next_stage(fw.UploadStage.COMPLETED, fw.UploadStage.IDLE) + + +class FakeBootloader: + """ + Minimal rev 5 bootloader behind a byte pipe. + + It consumes exactly what the codec encodes and answers like the real protocol, so a + mismatch between encoder and decoder fails here instead of on hardware. + """ + + def __init__(self, board_id: int = 9, flash_size: int = 2048) -> None: + self.board_id = board_id + self.flash_size = flash_size + self.flash = b"" + self.erased = False + self.rebooted = False + + def exchange(self, request: bytes) -> bytes: # noqa: PLR0911 + assert request.endswith(fw.EOC) + cmd, body = request[0:1], request[1:-1] + if cmd == fw.GET_SYNC: + return fw.INSYNC + fw.OK + if cmd == fw.GET_DEVICE: + values = { + fw.INFO_BL_REV: 5, + fw.INFO_BOARD_ID: self.board_id, + fw.INFO_BOARD_REV: 0, + fw.INFO_FLASH_SIZE: self.flash_size, + fw.INFO_EXTF_SIZE: 0, + } + return struct.pack(" fw.BootloaderInfo: + fw.decode_sync(bl.exchange(fw.encode_get_sync())) + + def info(param: bytes) -> int: + reply = bl.exchange(fw.encode_get_device(param)) + fw.decode_sync(reply[4:]) + return fw.decode_uint32(reply) + + return fw.BootloaderInfo( + info(fw.INFO_BL_REV), + info(fw.INFO_BOARD_ID), + info(fw.INFO_BOARD_REV), + info(fw.INFO_FLASH_SIZE), + info(fw.INFO_EXTF_SIZE), + ) + + +class TestBootloaderCodec: + """Encoders and decoders agree with a fake bootloader on the full erase, program, verify sequence.""" + + def test_full_upload_sequence_verifies_against_fake_bootloader(self, tmp_path: Path, small_image: bytes) -> None: + """ + Test full upload sequence verifies against fake bootloader. + + GIVEN: a loaded image and a fake bootloader with a matching board id + WHEN: identify, erase, program every chunk, then GET_CRC + THEN: the bootloader CRC equals the image CRC and the reboot is acknowledged + """ + image = fw.load_apj(write_apj(tmp_path, small_image)) + bl = FakeBootloader() + + info = identify(bl) + fw.check_compatibility(image, info) + fw.decode_sync(bl.exchange(fw.encode_chip_erase())) + chunks = fw.program_chunks(image.image) + for chunk in chunks: + fw.decode_sync(bl.exchange(fw.encode_prog_multi(chunk))) + crc_reply = bl.exchange(fw.encode_get_crc()) + fw.decode_sync(crc_reply[4:]) + fw.decode_sync(bl.exchange(fw.encode_reboot())) + + assert info == fw.BootloaderInfo(5, 9, 0, 2048, 0) + assert len(chunks) == -(-len(image.image) // fw.PROG_MULTI_MAX) + assert bl.flash == image.image + assert fw.decode_uint32(crc_reply) == image.crc(info.flash_size) + assert bl.rebooted + + def test_programming_before_erase_surfaces_as_a_protocol_error(self, tmp_path: Path, small_image: bytes) -> None: + image = fw.load_apj(write_apj(tmp_path, small_image)) + bl = FakeBootloader() + + with pytest.raises(fw.BootloaderProtocolError, match="OPERATION FAILED"): + fw.decode_sync(bl.exchange(fw.encode_prog_multi(fw.program_chunks(image.image)[0]))) + + @pytest.mark.parametrize( + ("reply", "reason"), + [ + (b"", "short reply"), + (b"\x00\x10", "expected INSYNC"), + (fw.INSYNC + fw.INVALID, "INVALID"), + (fw.INSYNC + fw.BAD_SILICON_REV, "silicon"), + (fw.INSYNC + b"\x7f", "unexpected status"), + ], + ) + def test_bad_sync_replies_are_typed_errors(self, reply: bytes, reason: str) -> None: + with pytest.raises(fw.BootloaderProtocolError, match=reason): + fw.decode_sync(reply) + + @pytest.mark.parametrize("length", [0, 3, fw.PROG_MULTI_MAX + 4]) + def test_prog_multi_refuses_chunks_the_bootloader_would_reject(self, length: int) -> None: + with pytest.raises(ValueError, match="PROG_MULTI"): + fw.encode_prog_multi(b"\x00" * length) + + def test_read_multi_encodes_length_byte(self) -> None: + assert fw.encode_read_multi(252) == fw.READ_MULTI + b"\xfc" + fw.EOC + with pytest.raises(ValueError, match="READ_MULTI"): + fw.encode_read_multi(253) + + def test_short_uint32_reply_is_a_protocol_error(self) -> None: + with pytest.raises(fw.BootloaderProtocolError, match="4 bytes"): + fw.decode_uint32(b"\x01\x02") From 8f473a55a47eb0313c9f0cea900a54e95a3eeee1 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Sat, 5 Sep 2026 12:58:16 +0200 Subject: [PATCH 2/4] feat(firmware): add safe bootloader upload backend Move APJ file reading and bootloader protocol handling into a dedicated flight-controller adapter with an injected transport interface. Validate decoded APJ payload sizes exactly, bound decompression, handle malformed board revisions consistently, check padded payload capacity, and support the ArduPilot 33-to-9 board compatibility mapping. Implement revision-2 read-back verification, revision-3+ CRC verification, external-flash erase/program/CRC, serial-open retries, erase/CRC timeouts, safe pre-erase cancellation, and guaranteed transport cleanup. Integrate APJ flashing with the FlightController facade: enter bootloader via MAVLink, require a direct serial connection, release and reopen the serial port, reconnect after flashing, and invalidate cached parameters. Add focused tests for protocol revisions, external flash, retries, cancellation, compatibility, parsing validation, and facade lifecycle. --- ARCHITECTURE_firmware_upload.md | 382 ++++++ .../backend_flightcontroller.py | 149 +++ .../backend_flightcontroller_bootloader.py | 686 +++++++++++ .../backend_flightcontroller_commands.py | 8 + .../backend_flightcontroller_connection.py | 8 + .../backend_flightcontroller_protocols.py | 7 + .../data_model_firmware_upload.py | 338 +++--- tests/test_architecture_firmware_upload.py | 28 + ...est_backend_flightcontroller_bootloader.py | 1081 +++++++++++++++++ .../test_backend_flightcontroller_commands.py | 20 + tests/test_bootloader_identity.py | 53 + tests/test_bootloader_partial_read.py | 55 + tests/test_data_model_firmware_upload.py | 226 +++- 13 files changed, 2840 insertions(+), 201 deletions(-) create mode 100644 ARCHITECTURE_firmware_upload.md create mode 100644 ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py create mode 100644 tests/test_architecture_firmware_upload.py create mode 100755 tests/test_backend_flightcontroller_bootloader.py create mode 100644 tests/test_bootloader_identity.py create mode 100644 tests/test_bootloader_partial_read.py diff --git a/ARCHITECTURE_firmware_upload.md b/ARCHITECTURE_firmware_upload.md new file mode 100644 index 000000000..318d6e051 --- /dev/null +++ b/ARCHITECTURE_firmware_upload.md @@ -0,0 +1,382 @@ +# Firmware Upload Architecture + +## Overview + +This feature allows a user to select an ArduPilot firmware image, validate it against +the connected flight controller, flash it through the board bootloader, and reconnect +to verify the result. + +The feature follows the project separation described in [`ARCHITECTURE.md`](ARCHITECTURE.md): + +- `backend_flightcontroller.py` provides the `FlightController` facade. It owns the + active MAVLink connection, bootloader entry/reboot command, and reconnection. +- `backend_flightcontroller_bootloader.py` is the bootloader I/O adapter. It owns the + serial port, ArduPilot bootloader protocol, firmware file reading, and progress events. +- `data_model_firmware_upload.py` is the business/domain model. It owns firmware + metadata, board compatibility, validation, workflow state, and user-facing error + classifications. It does not open files, access serial ports, use Tkinter, or talk + directly to the flight controller. +- `frontend_firmware_upload.py` is the GUI. It owns file selection, confirmation, + progress presentation, cancellation, and translated user messages. It delegates + validation and upload operations to the model and backend. + +The operation is deliberately separate from parameter configuration. Firmware flashing +may destroy the running connection and must not be performed as an implicit part of a +parameter upload or configuration-step transition. + +## External protocol references + +The implementation should be based on the maintained ArduPilot uploader and pymavlink +interfaces, rather than inventing a second bootloader protocol: + +- [ArduPilot `Tools/scripts/uploader.py`](https://github.com/ArduPilot/ardupilot/blob/master/Tools/scripts/uploader.py) + defines the PX4/ArduPilot serial bootloader protocol, APJ image format, board-ID + validation, erase/program/verify sequence, and reboot handling. +- [ArduPilot bootloader documentation](https://ardupilot.org/dev/docs/bootloader.html) + documents the supported serial flashing workflow and baud rates. +- [pymavlink](https://github.com/ArduPilot/pymavlink) supplies MAVLink connection, + message packing, and serial abstractions for entering the bootloader and detecting + the flight controller before and after flashing. + +Firmware programming itself is not a normal MAVLink file transfer. MAVLink is used for +the reboot/bootloader-entry handshake, including a `COMMAND_ACK` check; a rejected +or armed vehicle is reported before the active MAVLink connection is released. Once the bootloader responds, the +bootloader serial protocol performs synchronization, erase, program, read-back verify, +and reboot. + +## Component architecture + +```mermaid +flowchart TD + GUI[frontend_firmware_upload.py\nTkinter workflow] --> FACADE[backend_flightcontroller.py\nFlightController facade] + FACADE --> BACKEND[backend_flightcontroller_bootloader.py\nbootloader I/O adapter] + BACKEND --> MODEL[data_model_firmware_upload.py\nvalidation and state] + FACADE --> MAV[pymavlink\nMAVLink handshake/reconnect] + BACKEND --> SERIAL[pyserial\nbootloader protocol] + BACKEND --> FILE[Local firmware file\nAPJ input] + BACKEND --> FC[ArduPilot bootloader] +``` + +The existing `FlightController` facade and protocol definitions should expose the +feature through delegation, following the connection/params/commands/files managers. +The connection manager remains the source of truth for the active MAVLink connection; +the upload adapter may temporarily close and recreate that connection but must not +maintain a competing long-lived connection state. After reboot, the facade resolves +the captured stable serial identity before reconnecting so a changed serial device +path cannot silently target the old port. When USB metadata is unavailable on Linux, +it captures the resolved `/dev/serial/by-path` link before reboot and reopens that exact +link after re-enumeration. + +## Requirements + +### Functional requirements + +1. Allow the user to choose a local ArduPilot firmware image. +2. Parse firmware metadata before opening or resetting the flight controller. +3. Validate the image format, image size, board ID, and supported bootloader protocol + range before programming. Board revision is retained for display; no authoritative + cross-board revision-compatibility map exists yet. +4. Require an explicit confirmation at the lowest write-capable API immediately before + an irreversible erase/write, and a trusted SHA-256 of the exact APJ descriptor bytes + before bootloader entry. The digest must come from an independently trusted manifest + or release record, not from APJ metadata. +5. Enter the bootloader using the existing MAVLink connection when supported, with a + documented unplug/replug fallback for boards that do not respond. +6. Discover and open the bootloader serial port at the configured baud rate. +7. Report synchronization, identification, erase, program, verify, and reboot stages. +8. Support cancellation only at safe protocol boundaries; never interrupt a write in + the middle of a bootloader packet. +9. Verify the programmed image before rebooting whenever the bootloader supports it. +10. On every pre-erase cancellation, rejection, or failure after opening the + bootloader, best-effort reboot it, close its port, and reconnect to the original + flight controller. If a held bootloader cannot be opened, report that a power cycle + is required; MAVLink reconnection is not attempted because it cannot succeed. +11. Close the bootloader port and reconnect to the flight controller after reboot. +12. Require post-flash `AUTOPILOT_VERSION` identity: the APJ board ID must be present + and match. APJ `version` is retained for display only because it is not the MAVLink + firmware-version field. +13. Return actionable, translated errors without exposing raw tracebacks in the GUI. + +### Safety requirements + +- Never erase or program before image and board compatibility checks pass. +- Refuse every board-ID mismatch. Force flashing is intentionally unsupported until a + separately designed, explicitly authorized, and auditable operation model exists. +- Require a valid APJ board ID from MAVLink before bootloader entry and bind the + bootloader board ID to it. Upload without a pre-reboot board identity is refused. +- Bind the serial target to physical hardware across bootloader re-enumeration. The + active port's USB location or serial number is resolved first on every platform; + every populated physical-device attribute must match the same port. A captured + interface value is not used as a cross-mode tie-break: macOS can report the USB + device parent's location for both CDC interfaces, and the application and + bootloader interface numbers may differ. Ambiguous matches therefore fail closed. + Linux `/dev/serial/by-path` is the fallback when no USB identity is available. + Resolution fails closed rather than opening a stale port, and upload is refused + when neither a USB identity nor a Linux by-path binding is available. A Linux + by-path fallback is captured before reboot and the same persistent link is used + for reconnection; it is never resolved afresh after the board re-enumerates. +- Require a trusted SHA-256 supplied by the caller for the exact APJ descriptor bytes, + binding the payload and board metadata together. The backend computes and retains a + separate payload digest for display, but never treats a self-reported APJ field as + trust evidence. The caller must obtain that digest from an authenticated release + catalogue which publishes per-APJ checksums; an APJ manifest's `git_sha` is not a + substitute. +- Do not flash over network MAVLink connections; require a directly addressable serial + device for the bootloader transport unless a board-specific transport is added. +- Do not treat a lost connection as success. +- Preserve the selected firmware path and metadata in memory only; do not modify the + source image. +- Ensure every serial port is closed in success, cancellation, and exception paths. +- Use the descriptor's unpadded external-image size for external-flash erase and CRC, + matching ArduPilot's reference uploader; writes remain 4-byte padded as required by + `PROG_MULTI`. +- Treat external-flash erase responses below 90% as monotonic percentage bytes, even + when a progress value equals `INSYNC` (decimal 18); accept `INSYNC/OK` only after + the progress threshold is reached. Refresh the timeout only when progress advances + so a stalled erase cannot wait indefinitely. +- Keep v2 read-back verification bounded by `READ_MULTI_MAX` independently of the + programming chunk size. +- Keep the UI responsive by running the blocking flash operation outside Tkinter's + event loop and marshal progress updates back to the GUI thread. + +## `backend_flightcontroller_bootloader.py` + +### Responsibilities + +This module owns bootloader serial and local-file I/O. It exposes small protocols so +real hardware and deterministic test doubles can be substituted. The existing +`backend_flightcontroller.py` facade owns MAVLink bootloader entry and reconnection. + +Planned interfaces: + +```python +class FirmwareUploadBackendProtocol(Protocol): + def inspect_firmware(self, firmware_path: Path) -> FirmwareImageMetadata: ... + def enter_bootloader(self, connection: MavlinkConnection, timeout: float) -> None: ... + def identify_bootloader(self, port: str, baudrate: int) -> BootloaderInfo: ... + def upload( + self, + image: FirmwareImage, + *, + confirmation_requested: Callable[[FirmwareImage, BootloaderInfo], bool], + progress_callback: ProgressCallback | None = None, + cancellation_requested: Callable[[], bool] | None = None, + ) -> None: ... + def reconnect_and_verify(self, device: str, baudrate: int, timeout: float) -> FirmwareIdentity: ... +``` + +The concrete adapter should contain these private stages: + +1. Read and decode APJ JSON, base64, and zlib data using bounded file I/O. +2. Normalize/pad the image exactly as the ArduPilot uploader does. +3. Open the serial port with a controlled timeout and restore the original settings + on failure where possible. Every bounded read must enforce its deadline after + partial as well as empty responses. Resolve the port again before every open attempt + using the captured USB location or serial number, or reopen the captured Linux + by-path link exactly, and reject zero or multiple USB matches. +4. Synchronize with the bootloader and query protocol revision, board ID, board + revision, internal flash size, and external flash size. If an old bootloader + rejects the optional external-size query, clear stale input before fallback + synchronization and report no external flash. +5. Reject unsupported protocol revisions and images that exceed available flash. +6. Erase only the required flash regions. +7. Program in protocol-sized chunks with ACK/INSYNC checking after each chunk. +8. Verify by read-back or the bootloader's supported CRC mechanism. +9. Reboot and close the port. +10. Reboot and close the port on cancellation or any upload failure before erase so the original + firmware can run again. Identification retries close and reopen the still-held + bootloader; only the final failed attempt attempts a recovery reboot. If that + recovery reboot fails, return a typed recovery error and require a power cycle + rather than reporting a normal reconnect. + +The adapter should reuse the uploader's constants and algorithm through a maintained +internal implementation or a clearly isolated vendored adapter. It should not invoke +MAVProxy as a subprocess: subprocess output, cancellation, platform behavior, and +error handling would be difficult to make reliable inside the application. + +### Firmware formats + +- APJ is the initial required format because it contains the image, `image_size`, and + `board_id` metadata required for safe bootloader validation. External-only APJs may + use an empty internal image when they provide a valid external image. +- Raw BIN support must not guess a board ID or flash offset. It may be added only when + the source metadata explicitly supplies the target board and layout, or when a + board-specific mapping is maintained by the application. Otherwise the backend must + reject BIN with a clear explanation and direct the user to an APJ image. +- Unsupported, malformed, compressed, oversized, or non-finite metadata must produce a + typed validation error before any serial I/O begins. + +### Connection integration + +Add a firmware-upload manager/protocol to the existing `FlightController` facade. It +should use the connection manager for: + +- current device, stable USB identity, and baud rate; +- the board ID reported by the currently connected firmware; absence or malformed + identity blocks upload before any reboot command; +- MAVLink reboot/bootloader-entry; +- disconnecting before serial bootloader access; and +- reconnecting and refreshing `FlightControllerInfo` after flashing. + +The manager must invalidate stale parameter and command state after a successful flash; +the normal application flow should require a fresh parameter download before allowing +configuration edits to continue. + +## `data_model_firmware_upload.py` + +### Domain objects + +Define plain, immutable-or-controlled-state objects such as: + +- `FirmwareImageMetadata`: path, format, image size, board ID, board revision, + firmware type/version, external flash size, and a SHA-256 over the exact unpadded + image regions. +- `BootloaderInfo`: protocol revision, board ID/revision, internal and external flash + capacity, and bootloader identity. +- `FirmwareCompatibility`: compatible/incompatible/unknown plus a reason and severity. +- `FirmwareUploadProgress`: stage, completed units, total units, and display text key. +- `FirmwareUploadResult`: success, cancelled, verified, reconnect status, and typed + failure information. + +### Business rules + +The model should: + +1. Validate file extension and metadata invariants. +2. Compare the image board ID against detected bootloader information and compare the + bootloader board ID with the required pre-reboot MAVLink board ID. Board revision + remains informational until an authoritative compatibility map is added. +3. Check image and external-image sizes against flash capacities. +4. Require confirmation at the write-capable boundary; force flashing is not permitted. +5. Define the state machine: + + `idle → inspecting → entering_bootloader → identifying → awaiting_confirmation → + erasing → programming → verifying → rebooting → reconnecting → completed` + + with `failed` and `cancelled` exits from every safe boundary. +6. Translate backend exceptions into stable domain error categories for the frontend. +7. Keep policy independent from timing, serial reads, Tkinter, and gettext dialogs. + +The model should not determine compatibility from a firmware filename alone. Firmware +metadata and bootloader identification are authoritative; filenames are display-only. + +## `frontend_firmware_upload.py` + +### Frontend responsibilities + +Provide a modal or dedicated firmware-upload window consistent with the existing +Tkinter frontend conventions: + +- firmware file picker with an APJ filter and an all-files fallback; +- metadata and detected-board summary; +- explicit mismatch warning; +- confirmation before erase/program; +- stage-specific progress bar and status text; +- cancel button with safe-boundary semantics; +- error dialog with recovery guidance; and +- completion view showing verification and reconnection status. + +The frontend must not parse APJ files, compare board IDs, or call serial/MAVLink APIs +directly. It should call the model and backend through injected protocols and use the +existing progress-window/UI-service patterns where applicable. + +The window must disable conflicting connection, parameter, and project-navigation +actions while flashing. Closing the window during an active operation should request +cancellation, not destroy the worker state or force-close the serial port. + +## Integration workflow + +1. The application opens the firmware-upload entry point only when a directly usable + serial connection is available. +2. The frontend selects a firmware file and asks the backend to inspect it. +3. The model validates metadata, the trusted release digest, and the current FC identity. +4. The backend requests bootloader mode, disconnects the MAVLink parser, and identifies + the bootloader. +5. The model performs the final board/capacity and pre-reboot-target checks. +6. The frontend asks for the final erase/program confirmation after identification. +7. A declined confirmation, cancellation, incompatibility, or pre-erase error sends a + best-effort bootloader reboot and reconnects the original MAVLink connection. +8. The backend erases, programs, and verifies while emitting throttled progress events. +9. The backend reboots, closes the bootloader port, and reconnects through the normal + connection manager. +10. The model requires the new `AUTOPILOT_VERSION` board identity before returning + success; the APJ's display version is not compared with MAVLink firmware version. +11. The frontend reports success and asks the user to re-download parameters before + resuming configuration. + +## Error handling + +Use typed errors internally and map them to translated messages at the frontend: + +- invalid or unreadable firmware file; +- unsupported firmware format; +- board-ID mismatch or a bootloader that differs from the pre-reboot target; +- unsupported bootloader protocol; +- bootloader not found or synchronization timeout; +- serial permission or port ownership failure; +- erase/program/verify failure; +- cancellation; +- reboot/reconnect timeout; and +- post-flash firmware identity mismatch. +- missing or ambiguous physical serial-device identity; and +- trusted firmware-digest missing or mismatch. + +Errors must include the failed stage and recovery advice. A failed verify is never +reported as a successful upload, even if the bootloader rebooted. + +## Testing strategy + +### Backend tests + +- APJ parsing, decompression, padding, metadata, and trusted-digest handling. +- Malformed JSON/base64/zlib and oversized-image rejection without serial access. +- Bootloader packet encoding/decoding and INSYNC/OK/error handling. +- Board-ID and flash-capacity validation, mandatory pre-reboot target binding, physical + serial-device re-enumeration/ambiguity handling, and the documented board-revision + informational policy. +- Erase/program/verify ordering using a fake serial port. +- Chunk boundaries, short reads/writes, timeouts, retries, and cancellation boundaries. +- Port cleanup on every failure path. +- MAVLink bootloader-entry and reconnect behavior with fake connections, including + callback/disconnect exceptions before erase and mandatory post-reconnect identity. + +### Data-model tests + +- Compatibility decisions for matching, mismatching, and unknown metadata. +- State-machine transitions and illegal-transition rejection. +- Error classification and progress aggregation. +- Mandatory confirmation requirements and force-flash rejection. + +### Frontend tests + +- File selection and metadata display. +- Confirmation on compatible and incompatible targets. +- Progress updates without direct backend access from widgets. +- Cancellation and window-close behavior. +- Error and completion dialogs. +- Controls disabled while flashing and restored after completion/failure. + +### Integration and acceptance tests + +Use a fake bootloader and fake MAVLink connection to exercise the complete workflow +without hardware. Hardware validation should be a separately marked test suite and +must require an explicit serial device selection. + +## Implementation sequence + +1. Add domain types and validation rules in `data_model_firmware_upload.py`. +2. Add the fake-serial bootloader protocol and APJ reader tests. +3. Implement the real backend adapter using the ArduPilot uploader algorithm. +4. Add facade/protocol delegation and connection lifecycle integration. +5. Build the frontend window and worker/progress orchestration. +6. Add end-to-end fake-device tests and update user/architecture documentation. +7. Run ruff, type checks, focused tests, and the complete non-GUI test suite. +8. Perform a hardware test on a supported board with a known-good recovery path. + +## Non-goals for the first implementation + +- Bootloader flashing over UDP/TCP or arbitrary MAVLink proxies. +- Bootloader updates, unless separately designed and confirmed by the user. +- Guessing raw BIN board IDs, offsets, or target hardware. +- Automatic firmware downloads from the internet. +- Force flashing or automatic retry after an erase failure. diff --git a/ardupilot_methodic_configurator/backend_flightcontroller.py b/ardupilot_methodic_configurator/backend_flightcontroller.py index efbd0d069..21a163835 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller.py @@ -14,6 +14,7 @@ from logging import warning as logging_warning from os import path as os_path from pathlib import Path +from time import monotonic as time_monotonic from time import sleep as time_sleep from typing import TYPE_CHECKING, Any, cast @@ -22,6 +23,17 @@ from ardupilot_methodic_configurator import _ from ardupilot_methodic_configurator.argparse_check_range import CheckRange +from ardupilot_methodic_configurator.backend_flightcontroller_bootloader import ( + CancellationRequested, + ConfirmationRequested, + FlightControllerBootloaderBackend, + ProgressCallback, + SerialFactory, + capture_serial_device_identity, + has_stable_bootloader_device_identity, + report_progress_safely, + resolve_bootloader_device, +) from ardupilot_methodic_configurator.backend_flightcontroller_commands import ( CompassCalibrationUpdate, FlightControllerCommands, @@ -41,6 +53,16 @@ FlightControllerParamsProtocol, MavlinkConnection, ) +from ardupilot_methodic_configurator.data_model_firmware_upload import ( + BootloaderInfo, + FirmwareBootloaderRecoveryError, + FirmwareConfirmationError, + FirmwareConnectionError, + FirmwareReconnectError, + UploadStage, + verify_expected_firmware_digest, + verify_reconnected_firmware, +) from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo from ardupilot_methodic_configurator.data_model_par_dict import ParDict @@ -48,6 +70,7 @@ from pymavlink.dialects.v20.ardupilotmega import MAVLink_autopilot_version_message DEFAULT_REBOOT_TIME: int = 8 +FIRMWARE_RECONNECT_RESOLVE_TIMEOUT: float = 15.0 # Re-export constants for backwards compatibility __all__ = [ @@ -338,6 +361,132 @@ def disconnect(self) -> None: # Clear parameter cache via params manager self._params_manager.clear_parameters() + def upload_apj_firmware( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-locals,too-many-statements + self, + path: Path, + *, + bootloader_baudrate: int = 115200, + bootloader_timeout: float = 2.0, + full_erase: bool = False, + expected_firmware_sha256: str | None = None, + serial_factory: SerialFactory | None = None, + cancellation_requested: CancellationRequested | None = None, + confirmation_requested: ConfirmationRequested | None = None, + progress_callback: ProgressCallback | None = None, + ) -> BootloaderInfo: + """ + Flash an APJ through the active direct serial connection and reconnect. + + This synchronous facade operation is intended for a worker thread. It is + deliberately unavailable without the current MAVLink connection and its + directly-addressable serial device, so a UDP/TCP connection cannot be used + accidentally for bootloader flashing. + """ + network_prefixes = ("udp:", "udpin:", "udpout:", "tcp:", "tcpin:", "tcpout:", "ws:", "wss:") + device = self.comport_device + if self.master is None or self.comport is None or not device or device.lower().startswith(network_prefixes): + msg = _("firmware upload requires an active direct serial flight-controller connection") + raise FirmwareConnectionError(msg) + if confirmation_requested is None: + msg = _("firmware upload requires explicit confirmation before entering the bootloader") + raise FirmwareConfirmationError(msg) + try: + connected_board_id = int(self.info.apj_board_id) + if connected_board_id < 0: + raise ValueError + except (TypeError, ValueError): + msg = _("firmware upload requires the connected flight controller APJ board_id") + raise FirmwareConnectionError(msg) from None + device_identity = capture_serial_device_identity(device) + entered_bootloader = False + + def report_progress(stage: UploadStage, completed: int, total: int) -> None: + """Keep presentation failures from interrupting the upload workflow.""" + report_progress_safely(progress_callback, stage, completed, total) + + def enter_bootloader() -> None: + nonlocal entered_bootloader + if self.master is None: + msg = _("flight-controller connection was lost before bootloader entry") + raise FirmwareConnectionError(msg) + accepted, error_message = self._commands_manager.reboot_to_bootloader() + if not accepted: + msg = _("flight controller rejected bootloader entry: {error}").format(error=error_message) + raise FirmwareConnectionError(msg) + # From this point on a failed callback, disconnect, or transport + # setup must attempt recovery before returning control to the caller. + entered_bootloader = True + time_sleep(0.3) # Allow the MAVLink frame to leave before releasing the port. + self.disconnect() + + def reconnect_after_bootloader() -> str: + active_baudrate = getattr(self._connection_manager, "active_baudrate", self.baudrate) + deadline = time_monotonic() + FIRMWARE_RECONNECT_RESOLVE_TIMEOUT + last_error: OSError | None = None + 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: # noqa: PERF203 + last_error = exc + time_sleep(0.1) + return _("cannot resolve the flight-controller serial device: {error}").format(error=last_error) + + backend_kwargs: dict[str, Any] = { + "timeout": bootloader_timeout, + "enter_bootloader": enter_bootloader, + "device_identity": device_identity, + } + if serial_factory is not None: + backend_kwargs["serial_factory"] = serial_factory + backend = FlightControllerBootloaderBackend( + device, + bootloader_baudrate, + **backend_kwargs, + ) + image = backend.inspect_firmware(path) + if expected_firmware_sha256 is None: + msg = _("firmware upload requires a trusted SHA-256 for the selected APJ payload") + raise FirmwareConfirmationError(msg) + verify_expected_firmware_digest(image, expected_firmware_sha256) + if not has_stable_bootloader_device_identity(device, device_identity): + msg = _("firmware upload requires a stable USB serial number, USB location, or Linux by-path device") + raise FirmwareConnectionError(msg) + report_progress(UploadStage.ENTERING_BOOTLOADER, 0, 1) + try: + info = backend.upload( + image, + full_erase=full_erase, + cancellation_requested=cancellation_requested, + confirmation_requested=confirmation_requested, + progress_callback=report_progress, + connected_board_id=connected_board_id, + ) + except Exception as exc: + if isinstance(exc, FirmwareBootloaderRecoveryError): + # ``hold_in_bootloader`` disables the normal boot timeout. If + # no bootloader transport could be opened, no in-band reboot is + # possible and a MAVLink reconnect cannot succeed. + raise + if entered_bootloader and getattr(exc, "bootloader_rebooted", False): + recovery_error = reconnect_after_bootloader() + if recovery_error: + logging_warning(_("Unable to reconnect after safe firmware-upload abort: %s"), recovery_error) + raise + # A flashed image invalidates every cached parameter. The reconnect is a + # required part of success, rather than an optimistic best-effort step. + self._params_manager.clear_parameters() + report_progress(UploadStage.RECONNECTING, 0, 1) + reconnect_error = reconnect_after_bootloader() + if reconnect_error: + raise FirmwareReconnectError(reconnect_error) + verify_reconnected_firmware( + image, + board_id=self.info.apj_board_id, + ) + report_progress(UploadStage.RECONNECTING, 1, 1) + return info + @property def banner_text_buffer(self) -> list[str]: """Return the most recently received FC banner text.""" diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py b/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py new file mode 100644 index 000000000..66a85da25 --- /dev/null +++ b/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py @@ -0,0 +1,686 @@ +""" +Flight-controller bootloader adapter for ArduPilot APJ firmware uploads. + +All byte-level bootloader protocol and local-file I/O is deliberately kept here; +``data_model_firmware_upload`` contains only parsing and upload policy. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +from __future__ import annotations + +import struct +from collections.abc import Callable, Iterator +from dataclasses import dataclass +from pathlib import Path +from sys import platform as sys_platform +from time import monotonic +from time import sleep as time_sleep +from typing import Protocol, cast + +import serial +import serial.tools.list_ports + +from ardupilot_methodic_configurator import _ +from ardupilot_methodic_configurator.data_model_firmware_upload import ( + BL_REV_MAX, + BL_REV_MIN, + MAX_ENCODED_BLOB_SIZE, + BootloaderInfo, + BootloaderProtocolError, + FirmwareBootloaderRecoveryError, + FirmwareConfirmationError, + FirmwareFileError, + FirmwareImage, + FirmwareUploadCancelledError, + FirmwareUploadError, + UploadStage, + check_bootloader_matches_connected_board, + check_compatibility, + parse_apj, +) + +# ArduPilot/PX4 bootloader protocol bytes, from Tools/scripts/uploader.py. +INSYNC = b"\x12" +EOC = b"\x20" +OK = b"\x10" +FAILED = b"\x11" +INVALID = b"\x13" +BAD_SILICON_REV = b"\x14" + +GET_SYNC = b"\x21" +GET_DEVICE = b"\x22" +CHIP_ERASE = b"\x23" +CHIP_VERIFY = b"\x24" +PROG_MULTI = b"\x27" +READ_MULTI = b"\x28" +GET_CRC = b"\x29" +REBOOT = b"\x30" +EXTF_ERASE = b"\x34" +EXTF_PROG_MULTI = b"\x35" +EXTF_GET_CRC = b"\x37" +CHIP_FULL_ERASE = b"\x40" + +INFO_BL_REV = b"\x01" +INFO_BOARD_ID = b"\x02" +INFO_BOARD_REV = b"\x03" +INFO_FLASH_SIZE = b"\x04" +INFO_EXTF_SIZE = b"\x06" + +PROG_MULTI_MAX = 252 # protocol maximum is 255; writes must be word aligned +READ_MULTI_MAX = 252 +ERASE_TIMEOUT = 20.0 +EXTF_CRC_TIMEOUT = 10.0 +MAX_APJ_DESCRIPTOR_SIZE = (MAX_ENCODED_BLOB_SIZE * 2) + (1024 * 1024) + + +class BootloaderTransport(Protocol): + """Minimal blocking byte stream required by the bootloader protocol.""" + + def write(self, data: bytes) -> int: ... + + def read(self, size: int = 1) -> bytes: ... + + def flush(self) -> None: ... + + def reset_input_buffer(self) -> None: ... + + def close(self) -> None: ... + + +ProgressCallback = Callable[[UploadStage, int, int], None] +ConfirmationRequested = Callable[[FirmwareImage, BootloaderInfo], bool] +CancellationRequested = Callable[[], bool] +SerialFactory = Callable[[str, int, float], BootloaderTransport] +BootloaderEntry = Callable[[], None] + + +@dataclass(frozen=True) +class SerialDeviceIdentity: + """Stable USB attributes which survive a bootloader serial-port rename.""" + + location: str = "" + serial_number: str = "" + interface: str = "" + persistent_path: str = "" + + +DeviceResolver = Callable[[str, SerialDeviceIdentity | None], str] + + +def _capture_linux_persistent_path(device: str) -> str: + """Return the by-path link currently bound to a serial device, if any.""" + if not sys_platform.startswith("linux"): + return "" + try: + current = Path(device).resolve(strict=True) + for by_path in Path("/dev/serial/by-path").iterdir(): + if by_path.resolve(strict=True) == current: + return str(by_path) + except OSError: + pass + return "" + + +def capture_serial_device_identity(device: str) -> SerialDeviceIdentity | None: + """Capture stable USB location/serial metadata for the active serial port.""" + for port in serial.tools.list_ports.comports(): + if port.device != device: + continue + identity = SerialDeviceIdentity( + location=str(getattr(port, "location", "") or ""), + serial_number=str(getattr(port, "serial_number", "") or ""), + interface=str(getattr(port, "interface", "") or ""), + persistent_path=_capture_linux_persistent_path(device), + ) + return identity if identity.location or identity.serial_number or identity.persistent_path else None + return None + + +def has_stable_bootloader_device_identity(device: str, identity: SerialDeviceIdentity | None) -> bool: + """Return whether reopening this serial target can be tied to physical hardware.""" + if identity is not None: + return True + return bool(_capture_linux_persistent_path(device)) + + +def resolve_bootloader_device(device: str, identity: SerialDeviceIdentity | None = None) -> str: + """ + Resolve by stable Linux path or captured USB identity after re-enumeration. + + With a known USB identity this is fail-closed: a stale port is never opened + while the board is re-enumerating or the hardware match is ambiguous. + """ + # A captured USB identity must take precedence over the old device path. + # During re-enumeration a generic path such as /dev/ttyACM0 may already + # belong to another controller, whereas the location or serial number + # identifies the controller that was connected before the reboot. + if identity is not None: + if identity.persistent_path: + return identity.persistent_path + matches = [ + port + for port in serial.tools.list_ports.comports() + if all( + not expected or getattr(port, attribute, "") == expected + for attribute, expected in ( + ("location", identity.location), + ("serial_number", identity.serial_number), + ) + ) + ] + if len(matches) == 1: + return str(matches[0].device) + msg = _("cannot uniquely locate the bootloader USB device") + raise OSError(msg) + if sys_platform.startswith("linux"): + persistent_path = _capture_linux_persistent_path(device) + if persistent_path: + return persistent_path + return device + + +def report_progress_safely(progress_callback: ProgressCallback | None, stage: UploadStage, completed: int, total: int) -> None: + """Invoke presentation code without allowing it to interrupt a flash operation.""" + if progress_callback is None: + return + try: + progress_callback(stage, completed, total) + except Exception: # pylint: disable=broad-exception-caught + return + + +def load_apj(path: Path) -> FirmwareImage: + """Read an APJ file, then pass its contents to the pure domain parser.""" + if path.suffix.lower() != ".apj": + msg = _("unsupported firmware format {suffix}, only .apj is supported").format(suffix=path.suffix) + raise FirmwareFileError(msg) + try: + if path.stat().st_size > MAX_APJ_DESCRIPTOR_SIZE: + msg = _("APJ descriptor exceeds {limit} bytes").format(limit=MAX_APJ_DESCRIPTOR_SIZE) + raise FirmwareFileError(msg) + with path.open("rb") as descriptor: + contents = descriptor.read(MAX_APJ_DESCRIPTOR_SIZE + 1) + if len(contents) > MAX_APJ_DESCRIPTOR_SIZE: + msg = _("APJ descriptor exceeds {limit} bytes").format(limit=MAX_APJ_DESCRIPTOR_SIZE) + raise FirmwareFileError(msg) + return parse_apj(contents, path=path) + except OSError as exc: + msg = _("cannot read APJ descriptor: {error}").format(error=exc) + raise FirmwareFileError(msg) from exc + + +def encode_get_sync() -> bytes: + return GET_SYNC + EOC + + +def encode_get_device(param: bytes) -> bytes: + return GET_DEVICE + param + EOC + + +def encode_chip_erase(*, full: bool = False) -> bytes: + return (CHIP_FULL_ERASE if full else CHIP_ERASE) + EOC + + +def encode_chip_verify() -> bytes: + return CHIP_VERIFY + EOC + + +def _encode_multi(command: bytes, chunk: bytes) -> bytes: + if not 0 < len(chunk) <= PROG_MULTI_MAX or len(chunk) % 4: + msg = _("PROG_MULTI chunk must be 4..{limit} bytes and a multiple of 4, got {size}").format( + limit=PROG_MULTI_MAX, size=len(chunk) + ) + raise ValueError(msg) + return command + bytes([len(chunk)]) + chunk + EOC + + +def encode_prog_multi(chunk: bytes) -> bytes: + return _encode_multi(PROG_MULTI, chunk) + + +def encode_extf_prog_multi(chunk: bytes) -> bytes: + return _encode_multi(EXTF_PROG_MULTI, chunk) + + +def encode_read_multi(length: int) -> bytes: + if not 0 < length <= READ_MULTI_MAX: + msg = _("READ_MULTI length must be 1..{limit}, got {length}").format(limit=READ_MULTI_MAX, length=length) + raise ValueError(msg) + return READ_MULTI + bytes([length]) + EOC + + +def _encode_external_size(command: bytes, size: int) -> bytes: + if not 0 < size <= 0xFFFFFFFF: + msg = _("external flash size must be 1..4294967295 bytes, got {size}").format(size=size) + raise ValueError(msg) + return command + struct.pack(" bytes: + return _encode_external_size(EXTF_ERASE, size) + + +def encode_extf_get_crc(size: int) -> bytes: + return _encode_external_size(EXTF_GET_CRC, size) + + +def encode_get_crc() -> bytes: + return GET_CRC + EOC + + +def encode_reboot() -> bytes: + return REBOOT + EOC + + +def decode_sync(reply: bytes) -> None: + """Validate an INSYNC/status reply.""" + if len(reply) != 2: + msg = _("short reply, expected INSYNC and status, got {reply}").format(reply=reply) + raise BootloaderProtocolError(msg) + if reply[0:1] != INSYNC: + msg = _("expected INSYNC, got {byte}").format(byte=reply[0:1]) + raise BootloaderProtocolError(msg) + status = reply[1:2] + if status == OK: + return + messages = { + INVALID: _("bootloader reports INVALID OPERATION"), + FAILED: _("bootloader reports OPERATION FAILED"), + BAD_SILICON_REV: _("programming not supported for this silicon revision"), + } + msg = messages.get(status) or _("unexpected status {status} instead of OK").format(status=status) + raise BootloaderProtocolError(msg) + + +def decode_uint32(reply: bytes) -> int: + if len(reply) != 4: + msg = _("short reply, expected 4 bytes, got {size}").format(size=len(reply)) + raise BootloaderProtocolError(msg) + return int(struct.unpack(" Iterator[bytes]: + """Yield protocol-sized chunks without copying the whole image into a list.""" + for offset in range(0, len(image), PROG_MULTI_MAX): + yield image[offset : offset + PROG_MULTI_MAX] + + +class BootloaderClient: + """ + Synchronous bootloader session over an injected transport. + + The transport timeout controls every read. A short read is never accepted as a + protocol response, preventing a timeout or partial serial transfer from being + mistaken for a successful flash operation. + """ + + def __init__( + self, + transport: BootloaderTransport, + *, + timeout: float = 2.0, + clock: Callable[[], float] = monotonic, + ) -> None: + if timeout <= 0: + msg = _("bootloader response timeout must be positive") + raise ValueError(msg) + self._transport = transport + self._timeout = timeout + self._clock = clock + + def close(self) -> None: + self._transport.close() + + def abort_before_erase(self) -> bool: + """Best-effort reboot for a declined or failed upload before anything was erased.""" + try: + self._write(encode_reboot()) + except BootloaderProtocolError: + return False + return True + + def _reset_input_buffer(self) -> None: + try: + self._transport.reset_input_buffer() + except (OSError, serial.SerialException) as exc: + msg = _("cannot clear bootloader serial input: {error}").format(error=exc) + raise BootloaderProtocolError(msg) from exc + + @staticmethod + def _report(progress_callback: ProgressCallback | None, stage: UploadStage, completed: int, total: int) -> None: + report_progress_safely(progress_callback, stage, completed, total) + + def _write(self, request: bytes) -> None: + try: + written = self._transport.write(request) + self._transport.flush() + except (OSError, serial.SerialException) as exc: + msg = _("bootloader transport write failed: {error}").format(error=exc) + raise BootloaderProtocolError(msg) from exc + if written != len(request): + msg = _("bootloader transport wrote {written} of {expected} bytes").format(written=written, expected=len(request)) + raise BootloaderProtocolError(msg) + + def _read_exact(self, size: int, *, timeout: float | None = None, deadline: float | None = None) -> bytes: + received = bytearray() + read_deadline = deadline if deadline is not None else self._clock() + (self._timeout if timeout is None else timeout) + try: + while len(received) < size: + if self._clock() >= read_deadline: + msg = _("timeout waiting for {expected} bootloader bytes; received {actual}").format( + expected=size, actual=len(received) + ) + raise BootloaderProtocolError(msg) + chunk = self._transport.read(size - len(received)) + if not chunk: + if self._clock() < read_deadline: + continue + msg = _("timeout waiting for {expected} bootloader bytes; received {actual}").format( + expected=size, actual=len(received) + ) + raise BootloaderProtocolError(msg) + received.extend(chunk) + if len(received) < size and self._clock() >= read_deadline: + msg = _("timeout waiting for {expected} bootloader bytes; received {actual}").format( + expected=size, actual=len(received) + ) + raise BootloaderProtocolError(msg) + except (OSError, serial.SerialException) as exc: + msg = _("bootloader transport read failed: {error}").format(error=exc) + raise BootloaderProtocolError(msg) from exc + return bytes(received) + + def _sync(self, *, timeout: float | None = None) -> None: + decode_sync(self._read_exact(2, timeout=timeout)) + + def _command(self, request: bytes, reply_size: int = 0) -> bytes: + self._write(request) + reply = self._read_exact(reply_size) if reply_size else b"" + self._sync() + return reply + + def identify(self) -> BootloaderInfo: + self._reset_input_buffer() + self._command(encode_get_sync()) + revision = decode_uint32(self._command(encode_get_device(INFO_BL_REV), 4)) + if not BL_REV_MIN <= revision <= BL_REV_MAX: + msg = _("unsupported bootloader protocol revision {revision}").format(revision=revision) + raise BootloaderProtocolError(msg) + + # Old bootloaders can reject this newer optional query. Resynchronize and + # conservatively report no external flash in that case. + try: + extf_size = decode_uint32(self._command(encode_get_device(INFO_EXTF_SIZE), 4)) + except BootloaderProtocolError: + extf_size = 0 + self._reset_input_buffer() + self._command(encode_get_sync()) + return BootloaderInfo( + protocol_revision=revision, + board_id=decode_uint32(self._command(encode_get_device(INFO_BOARD_ID), 4)), + board_revision=decode_uint32(self._command(encode_get_device(INFO_BOARD_REV), 4)), + flash_size=decode_uint32(self._command(encode_get_device(INFO_FLASH_SIZE), 4)), + extf_size=extf_size, + ) + + def _erase_external(self, size: int) -> None: + self._write(encode_extf_erase(size)) + self._sync() + # The bootloader emits percentage bytes until it is almost done, then the + # final INSYNC/OK acknowledgement. Values below 90 are progress reports. + deadline = self._clock() + ERASE_TIMEOUT + last_pct = 0 + while True: + first = self._read_exact(1, deadline=deadline) + if last_pct >= 90 and first == INSYNC: + decode_sync(first + self._read_exact(1, deadline=deadline)) + return + progress = first[0] + if progress > 100: + msg = _("invalid external-flash erase progress {progress}").format(progress=progress) + raise BootloaderProtocolError(msg) + if progress < last_pct: + msg = _("external-flash erase progress regressed from {previous} to {progress}").format( + previous=last_pct, progress=progress + ) + raise BootloaderProtocolError(msg) + if progress > last_pct: + last_pct = progress + deadline = self._clock() + ERASE_TIMEOUT + + def _verify_v2(self, image: FirmwareImage) -> None: + self._command(encode_chip_verify()) + for offset in range(0, len(image.image), READ_MULTI_MAX): + expected = image.image[offset : offset + READ_MULTI_MAX] + programmed = self._command(encode_read_multi(len(expected)), len(expected)) + if programmed != expected: + msg = _("firmware read-back verification failed") + raise BootloaderProtocolError(msg) + + def _verify_v3(self, image: FirmwareImage, flash_size: int) -> None: + actual = decode_uint32(self._command(encode_get_crc(), 4)) + if actual != image.crc(flash_size): + msg = _("firmware CRC verification failed") + raise BootloaderProtocolError(msg) + + def _verify_external(self, image: FirmwareImage) -> None: + self._write(encode_extf_get_crc(image.metadata.extf_image_size)) + actual = decode_uint32(self._read_exact(4, timeout=EXTF_CRC_TIMEOUT)) + self._sync(timeout=EXTF_CRC_TIMEOUT) + if actual != image.extf_crc(): + msg = _("external firmware CRC verification failed") + raise BootloaderProtocolError(msg) + + def upload( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-branches,too-many-statements + self, + image: FirmwareImage, + *, + full_erase: bool = False, + cancellation_requested: CancellationRequested | None = None, + confirmation_requested: ConfirmationRequested | None = None, + progress_callback: ProgressCallback | None = None, + bootloader: BootloaderInfo | None = None, + connected_board_id: int | None = None, + ) -> BootloaderInfo: + """ + Identify, validate, program all image regions, verify, and reboot. + + The transport is closed on success and on every error path. Revision two + uses CHIP_VERIFY plus READ_MULTI and intentionally receives no reboot ACK; + revision three and later use CRC and require that ACK. + """ + stage = UploadStage.AWAITING_CONFIRMATION + try: + if confirmation_requested is None: + msg = _("firmware upload requires explicit confirmation") + raise FirmwareConfirmationError(msg) + if cancellation_requested is not None and cancellation_requested(): + msg = _("firmware upload cancelled before entering the bootloader") + raise FirmwareUploadCancelledError(msg, stage=stage.value) + stage = UploadStage.IDENTIFYING + self._report(progress_callback, stage, 0, 1) + info = bootloader or self.identify() + self._report(progress_callback, stage, 1, 1) + check_bootloader_matches_connected_board(info, connected_board_id) + check_compatibility(image, info) + stage = UploadStage.AWAITING_CONFIRMATION + self._report(progress_callback, stage, 0, 1) + if not confirmation_requested(image, info): + msg = _("firmware upload was not confirmed") + raise FirmwareUploadCancelledError(msg, stage=stage.value) + self._report(progress_callback, stage, 1, 1) + if cancellation_requested is not None and cancellation_requested(): + msg = _("firmware upload cancelled before erase") + raise FirmwareUploadCancelledError(msg, stage=stage.value) + if image.metadata.extf_image_size: + stage = UploadStage.ERASING + self._report(progress_callback, stage, 0, 1) + self._erase_external(image.metadata.extf_image_size) + self._report(progress_callback, stage, 1, 1) + stage = UploadStage.PROGRAMMING + chunk_count = (len(image.extf_image) + PROG_MULTI_MAX - 1) // PROG_MULTI_MAX + for index, chunk in enumerate(program_chunks(image.extf_image), start=1): + self._command(encode_extf_prog_multi(chunk)) + self._report(progress_callback, stage, index, chunk_count) + stage = UploadStage.VERIFYING + self._report(progress_callback, stage, 0, 1) + self._verify_external(image) + self._report(progress_callback, stage, 1, 1) + if image.metadata.image_size: + stage = UploadStage.ERASING + self._report(progress_callback, stage, 0, 1) + self._write(encode_chip_erase(full=full_erase)) + self._sync(timeout=ERASE_TIMEOUT) + self._report(progress_callback, stage, 1, 1) + stage = UploadStage.PROGRAMMING + chunk_count = (len(image.image) + PROG_MULTI_MAX - 1) // PROG_MULTI_MAX + for index, chunk in enumerate(program_chunks(image.image), start=1): + self._command(encode_prog_multi(chunk)) + self._report(progress_callback, stage, index, chunk_count) + stage = UploadStage.VERIFYING + self._report(progress_callback, stage, 0, 1) + if info.protocol_revision == 2: + self._verify_v2(image) + else: + self._verify_v3(image, info.flash_size) + self._report(progress_callback, stage, 1, 1) + stage = UploadStage.REBOOTING + self._report(progress_callback, stage, 0, 1) + if info.protocol_revision == 2: + self._write(encode_reboot()) + else: + self._command(encode_reboot()) + self._report(progress_callback, stage, 1, 1) + return info + except FirmwareUploadError as exc: + if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION} and not self.abort_before_erase(): + msg = _("cannot reboot the held bootloader; power-cycle the flight controller before reconnecting") + raise FirmwareBootloaderRecoveryError(msg) from exc + if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION}: + exc.bootloader_rebooted = True # type: ignore[attr-defined] + if isinstance(exc, BootloaderProtocolError): + exc.stage = stage.value + raise + except Exception as exc: + if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION} and not self.abort_before_erase(): + msg = _("cannot reboot the held bootloader; power-cycle the flight controller before reconnecting") + raise FirmwareBootloaderRecoveryError(msg) from exc + if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION}: + exc.bootloader_rebooted = True # type: ignore[attr-defined] + raise + finally: + self.close() + + +def open_serial_transport(device: str, baudrate: int, timeout: float) -> BootloaderTransport: + """Open the real serial transport with explicit read/write timeouts.""" + return cast("BootloaderTransport", serial.Serial(device, baudrate, timeout=timeout, write_timeout=timeout, exclusive=True)) + + +class FlightControllerBootloaderBackend: # pylint:disable=too-many-instance-attributes + """Production-facing adapter with injectable bootloader entry and serial transport.""" + + def __init__( # noqa: PLR0913 # pylint: disable=too-many-arguments + self, + device: str, + baudrate: int, + *, + timeout: float = 2.0, + enter_bootloader: BootloaderEntry | None = None, + serial_factory: SerialFactory = open_serial_transport, + device_resolver: DeviceResolver = resolve_bootloader_device, + device_identity: SerialDeviceIdentity | None = None, + open_retries: int = 5, + retry_delay: float = 0.5, + sleep: Callable[[float], None] = time_sleep, + ) -> None: + if timeout <= 0: + msg = _("bootloader response timeout must be positive") + raise ValueError(msg) + self._device = device + self._device_resolver = device_resolver + self._device_identity = device_identity + self._baudrate = baudrate + self._timeout = timeout + self._enter_bootloader = enter_bootloader + self._serial_factory = serial_factory + self._open_retries = open_retries + self._retry_delay = retry_delay + self._sleep = sleep + + def inspect_firmware(self, path: Path) -> FirmwareImage: + return load_apj(path) + + def upload( # pylint: disable=too-many-arguments + self, + image: FirmwareImage, + *, + full_erase: bool = False, + cancellation_requested: CancellationRequested | None = None, + confirmation_requested: ConfirmationRequested | None = None, + progress_callback: ProgressCallback | None = None, + connected_board_id: int | None = None, + ) -> BootloaderInfo: + if confirmation_requested is None: + msg = _("firmware upload requires explicit confirmation") + raise FirmwareConfirmationError(msg) + if cancellation_requested is not None and cancellation_requested(): + msg = _("firmware upload cancelled before entering the bootloader") + raise FirmwareUploadCancelledError(msg, stage=UploadStage.ENTERING_BOOTLOADER.value) + if self._enter_bootloader is not None: + self._enter_bootloader() + client, info = self._wait_for_bootloader() + return client.upload( + image, + full_erase=full_erase, + cancellation_requested=cancellation_requested, + confirmation_requested=confirmation_requested, + progress_callback=progress_callback, + bootloader=info, + connected_board_id=connected_board_id, + ) + + def _wait_for_bootloader(self) -> tuple[BootloaderClient, BootloaderInfo]: + if self._open_retries < 1: + msg = _("bootloader open retries must be at least one") + raise ValueError(msg) + last_error: FirmwareUploadError | OSError | serial.SerialException | None = None + for attempt in range(self._open_retries): + transport, open_error = self._try_open_transport() + if transport is not None: + client = BootloaderClient(transport, timeout=self._timeout) + try: + return client, client.identify() + except BootloaderProtocolError as exc: + last_error = exc + if attempt + 1 == self._open_retries: + 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 + exc.bootloader_rebooted = True # type: ignore[attr-defined] + exc.stage = UploadStage.IDENTIFYING.value + client.close() + else: + last_error = open_error + if attempt + 1 < self._open_retries: + self._sleep(self._retry_delay) + if isinstance(last_error, BootloaderProtocolError): + raise last_error + msg = _( + "cannot open the held bootloader on serial port {device}: {error}; " + "power-cycle the flight controller before reconnecting" + ).format(device=self._device, error=last_error) + raise FirmwareBootloaderRecoveryError(msg) from last_error + + def _try_open_transport(self) -> tuple[BootloaderTransport | None, OSError | serial.SerialException | None]: + try: + device = self._device_resolver(self._device, self._device_identity) + return self._serial_factory(device, self._baudrate, self._timeout), None + except (OSError, serial.SerialException) as exc: + return None, exc diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_commands.py b/ardupilot_methodic_configurator/backend_flightcontroller_commands.py index dddaad954..b960c1477 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_commands.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_commands.py @@ -200,6 +200,14 @@ def send_command_and_wait_ack( # pylint: disable=too-many-arguments,too-many-po logging_error(error_msg) return False, error_msg + def reboot_to_bootloader(self) -> tuple[bool, str]: + """Request reboot into the bootloader and wait for its command acknowledgment.""" + return self.send_command_and_wait_ack( + mavutil.mavlink.MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN, + param1=3, + timeout=self.COMMAND_ACK_TIMEOUT, + ) + def reset_all_parameters_to_default(self) -> tuple[bool, str]: """ Reset all parameters to their factory default values. diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_connection.py b/ardupilot_methodic_configurator/backend_flightcontroller_connection.py index 0f497195a..88246d9f0 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_connection.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_connection.py @@ -143,6 +143,7 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen self.master: MavlinkConnection | None = None self.comport: mavutil.SerialPort | serial.tools.list_ports_common.ListPortInfo | None = None self._baudrate = baudrate + self._active_baudrate = baudrate self._network_ports = list(network_ports) if network_ports is not None else self.DEFAULT_NETWORK_PORTS[:] self._connection_tuples: list[tuple[str, str]] = [] self._logged_connection_tuples: list[tuple[str, str]] | None = None # None = never logged @@ -327,6 +328,7 @@ def connect( """ connection_baudrate = baudrate if baudrate is not None else self._baudrate + self._active_baudrate = connection_baudrate # Always clear cached metadata before attempting a new connection so UI # components never display stale data while we probe ports. @@ -870,6 +872,7 @@ def create_connection_with_retry( # pylint: disable=too-many-arguments, too-man indicating a successful connection. """ + self._active_baudrate = baudrate if self.comport is None or self.comport.device == DEVICE_FC_PARAM_FROM_FILE: # will read parameters from a params.param file instead of a from a flight controller return "" @@ -939,6 +942,11 @@ def baudrate(self) -> int: """Get the default baud rate for serial connections.""" return self._baudrate + @property + def active_baudrate(self) -> int: + """Get the baud rate used for the current or most recent serial session.""" + return self._active_baudrate + def set_master_for_testing( self, master: Optional["MavlinkConnection"], diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py b/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py index 159860d32..a6ae2ad36 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_protocols.py @@ -98,6 +98,11 @@ def baudrate(self) -> int: """Get the default baud rate for serial connections.""" ... # pylint: disable=unnecessary-ellipsis + @property + def active_baudrate(self) -> int: + """Get the baud rate used for the current or most recent serial session.""" + ... # pylint: disable=unnecessary-ellipsis + def discover_connections( self, progress_callback: Callable[[int, int], None] | None = None, @@ -233,6 +238,8 @@ def send_command_and_wait_ack( # pylint: disable=too-many-arguments, too-many-p timeout: float, ) -> tuple[bool, str]: ... + def reboot_to_bootloader(self) -> tuple[bool, str]: ... + def reset_all_parameters_to_default(self) -> tuple[bool, str]: ... def test_motor( # pylint: disable=too-many-arguments, too-many-positional-arguments diff --git a/ardupilot_methodic_configurator/data_model_firmware_upload.py b/ardupilot_methodic_configurator/data_model_firmware_upload.py index a45f274e9..0690330c1 100755 --- a/ardupilot_methodic_configurator/data_model_firmware_upload.py +++ b/ardupilot_methodic_configurator/data_model_firmware_upload.py @@ -17,47 +17,28 @@ """ import base64 +import hashlib +import hmac import json +import re import struct import zlib -from binascii import crc32 +from binascii import Error as BinasciiError from dataclasses import dataclass from enum import Enum from pathlib import Path +from typing import Any from ardupilot_methodic_configurator import _ -# Bootloader protocol bytes, from ArduPilot Tools/scripts/uploader.py -INSYNC = b"\x12" -EOC = b"\x20" -OK = b"\x10" -FAILED = b"\x11" -INVALID = b"\x13" -BAD_SILICON_REV = b"\x14" - -GET_SYNC = b"\x21" -GET_DEVICE = b"\x22" -CHIP_ERASE = b"\x23" -CHIP_VERIFY = b"\x24" -PROG_MULTI = b"\x27" -READ_MULTI = b"\x28" -GET_CRC = b"\x29" -REBOOT = b"\x30" -CHIP_FULL_ERASE = b"\x40" - -INFO_BL_REV = b"\x01" -INFO_BOARD_ID = b"\x02" -INFO_BOARD_REV = b"\x03" -INFO_FLASH_SIZE = b"\x04" -INFO_EXTF_SIZE = b"\x06" - BL_REV_MIN = 2 BL_REV_MAX = 5 -PROG_MULTI_MAX = 252 # protocol max is 255, must be a multiple of 4 -READ_MULTI_MAX = 252 -# Sanity bound on decompressed image size, well above any current flight controller flash +# Sanity bound on each decompressed image, well above any current flight controller flash. MAX_IMAGE_SIZE = 64 * 1024 * 1024 +# A base64 APJ blob can be roughly 4/3 the compressed input. Permit a small +# compression-stream overhead while rejecting a descriptor before decoding it. +MAX_ENCODED_BLOB_SIZE = ((MAX_IMAGE_SIZE + 65536) * 4 // 3) + 4 class FirmwareUploadError(Exception): @@ -65,6 +46,11 @@ class FirmwareUploadError(Exception): stage = "unknown" + def __init__(self, message: str, *, stage: str | None = None) -> None: + super().__init__(message) + if stage is not None: + self.stage = stage + class FirmwareFileError(FirmwareUploadError): """The firmware file is unreadable, malformed or not a supported format.""" @@ -84,8 +70,54 @@ class BootloaderProtocolError(FirmwareUploadError): stage = "identifying" +class FirmwareReconnectError(FirmwareUploadError): + """Firmware was flashed but the normal flight-controller connection did not return.""" + + stage = "reconnecting" + + +class FirmwareConnectionError(FirmwareUploadError): + """The active flight-controller connection cannot be used for firmware upload.""" + + stage = "entering_bootloader" + + +class FirmwareBootloaderRecoveryError(FirmwareConnectionError): + """The held bootloader could not be reached to reboot into the installed firmware.""" + + +class FirmwareTargetMismatchError(FirmwareUploadError): + """The bootloader belongs to a different board than the connected flight controller.""" + + stage = "identifying" + + +class FirmwareIdentityError(FirmwareUploadError): + """The firmware reported after reboot does not match the selected APJ image.""" + + stage = "reconnecting" + + +class FirmwareUploadCancelledError(FirmwareUploadError): + """The user cancelled before firmware erase began.""" + + stage = "identifying" + + +class FirmwareConfirmationError(FirmwareUploadError): + """An irreversible upload was requested without explicit confirmation.""" + + stage = "awaiting_confirmation" + + +class FirmwareIntegrityError(FirmwareUploadError): + """The decoded APJ bytes do not match the trusted digest supplied by the caller.""" + + stage = "inspecting" + + @dataclass(frozen=True) -class FirmwareImageMetadata: +class FirmwareImageMetadata: # pylint: disable=too-many-instance-attributes """What the user sees before confirming an upload, read from the APJ descriptor and never from the filename.""" path: Path @@ -94,6 +126,8 @@ class FirmwareImageMetadata: extf_image_size: int firmware_version: str git_identity: str + content_sha256: str + apj_sha256: str board_revision: int | None = None @@ -107,11 +141,24 @@ class FirmwareImage: def crc(self, flash_size: int) -> int: """CRC32 of the image padded with 0xFF up to flash_size, as computed by the bootloader GET_CRC.""" - state = crc32(self.image, 0) - for _unused in range(len(self.image), flash_size - 1, 4): - state = crc32(b"\xff\xff\xff\xff", state) + state = bootloader_crc32(self.image) + remaining = max(0, flash_size - len(self.image)) + while remaining: + padding_size = min(remaining, 4) + state = bootloader_crc32(b"\xff" * padding_size, state) + remaining -= padding_size return state + def extf_crc(self) -> int: + """CRC32 of the unpadded external-flash payload.""" + return bootloader_crc32(self.extf_image[: self.metadata.extf_image_size]) + + def content_sha256(self) -> str: + """Return the digest of the exact, unpadded payload selected for upload.""" + return firmware_content_sha256( + self.image[: self.metadata.image_size], self.extf_image[: self.metadata.extf_image_size] + ) + @dataclass(frozen=True) class BootloaderInfo: @@ -145,9 +192,9 @@ class UploadStage(Enum): _HAPPY_PATH = [ UploadStage.IDLE, UploadStage.INSPECTING, - UploadStage.AWAITING_CONFIRMATION, UploadStage.ENTERING_BOOTLOADER, UploadStage.IDENTIFYING, + UploadStage.AWAITING_CONFIRMATION, UploadStage.ERASING, UploadStage.PROGRAMMING, UploadStage.VERIFYING, @@ -182,38 +229,41 @@ def next_stage(current: UploadStage, target: UploadStage) -> UploadStage: raise ValueError(msg) -def load_apj(path: Path) -> FirmwareImage: - """Read and decode an APJ file, raising FirmwareFileError before any serial I/O can happen.""" - if path.suffix.lower() != ".apj": - msg = _("unsupported firmware format {suffix}, only .apj is supported").format(suffix=path.suffix) - raise FirmwareFileError(msg) +def parse_apj(contents: str | bytes, *, path: Path = Path()) -> FirmwareImage: + """Purely parse and validate APJ contents; callers own file-system access.""" + source_bytes = contents.encode("utf-8") if isinstance(contents, str) else contents try: - desc = json.loads(path.read_text(encoding="utf-8")) - except (OSError, UnicodeDecodeError, json.JSONDecodeError) as exc: - msg = _("cannot read APJ descriptor: {error}").format(error=exc) + desc: Any = json.loads(contents) + except (UnicodeDecodeError, json.JSONDecodeError, TypeError) as exc: + msg = _("cannot parse APJ descriptor: {error}").format(error=exc) raise FirmwareFileError(msg) from exc if not isinstance(desc, dict): msg = _("APJ descriptor is not a JSON object") raise FirmwareFileError(msg) - image = _decode_blob(desc, "image") - extf_image = _decode_blob(desc, "extf_image") if "extf_image" in desc else b"" + raw_image = _decode_blob(desc, "image") + raw_extf_image = _decode_blob(desc, "extf_image") if "extf_image" in desc else b"" try: - board_id = int(desc["board_id"]) - image_size = int(desc["image_size"]) - extf_image_size = int(desc.get("extf_image_size", 0)) + board_id = _parse_integer_metadata(desc["board_id"]) + image_size = _parse_integer_metadata(desc["image_size"]) + extf_image_size = _parse_integer_metadata(desc.get("extf_image_size", 0)) except (KeyError, TypeError, ValueError) as exc: msg = _("APJ descriptor is missing or has invalid metadata: {error}").format(error=exc) raise FirmwareFileError(msg) from exc if board_id < 0 or image_size < 0 or extf_image_size < 0 or (image_size == 0 and extf_image_size == 0): msg = _("APJ metadata values are out of range") raise FirmwareFileError(msg) - if image_size > len(image) or extf_image_size > len(extf_image): + if image_size != len(raw_image) or extf_image_size != len(raw_extf_image): msg = _("APJ image_size does not match the decoded image") raise FirmwareFileError(msg) board_rev = desc.get("board_revision") + try: + parsed_board_rev = _parse_integer_metadata(board_rev) if board_rev is not None else None + except (TypeError, ValueError) as exc: + msg = _("APJ descriptor is missing or has invalid metadata: {error}").format(error=exc) + raise FirmwareFileError(msg) from exc metadata = FirmwareImageMetadata( path=path, board_id=board_id, @@ -221,124 +271,138 @@ def load_apj(path: Path) -> FirmwareImage: extf_image_size=extf_image_size, firmware_version=str(desc.get("version", "")), git_identity=str(desc.get("git_identity", "")), - board_revision=int(board_rev) if board_rev is not None else None, + content_sha256=firmware_content_sha256(raw_image, raw_extf_image), + apj_sha256=hashlib.sha256(source_bytes).hexdigest(), + board_revision=parsed_board_rev, ) - return FirmwareImage(metadata=metadata, image=image, extf_image=extf_image) + return FirmwareImage(metadata=metadata, image=_pad4(raw_image), extf_image=_pad4(raw_extf_image)) + + +def _parse_integer_metadata(value: object) -> int: + """Parse APJ integer metadata without silently accepting booleans or floats.""" + if isinstance(value, (bool, float)): + msg = _("APJ metadata integer must not be boolean or floating point") + raise ValueError(msg) + if not isinstance(value, (int, str)): + msg = _("APJ metadata integer must be an integer or integer string") + raise TypeError(msg) + return int(value) def _decode_blob(desc: dict, key: str) -> bytes: try: - raw = zlib.decompress(base64.b64decode(desc[key], validate=True), bufsize=MAX_IMAGE_SIZE) - except (KeyError, TypeError, ValueError, zlib.error) as exc: + encoded = desc[key] + if not isinstance(encoded, (str, bytes)): + msg = _("APJ {key} is not valid base64+zlib data: expected text").format(key=key) + raise FirmwareFileError(msg) + if len(encoded) > MAX_ENCODED_BLOB_SIZE: + msg = _("APJ {key} exceeds {limit} encoded bytes").format(key=key, limit=MAX_ENCODED_BLOB_SIZE) + raise FirmwareFileError(msg) + compressed = base64.b64decode(encoded, validate=True) + decompressor = zlib.decompressobj() + raw = decompressor.decompress(compressed, MAX_IMAGE_SIZE + 1) + if len(raw) > MAX_IMAGE_SIZE or decompressor.unconsumed_tail: + msg = _("APJ {key} exceeds {limit} bytes").format(key=key, limit=MAX_IMAGE_SIZE) + raise FirmwareFileError(msg) + raw += decompressor.flush(MAX_IMAGE_SIZE + 1 - len(raw)) + if len(raw) > MAX_IMAGE_SIZE: + msg = _("APJ {key} exceeds {limit} bytes").format(key=key, limit=MAX_IMAGE_SIZE) + raise FirmwareFileError(msg) + if not decompressor.eof: + msg = _("APJ {key} is not valid base64+zlib data: truncated stream").format(key=key) + raise FirmwareFileError(msg) + except (BinasciiError, KeyError, TypeError, ValueError, zlib.error) as exc: msg = _("APJ {key} is not valid base64+zlib data: {error}").format(key=key, error=exc) raise FirmwareFileError(msg) from exc - if len(raw) > MAX_IMAGE_SIZE: - msg = _("APJ {key} exceeds {limit} bytes").format(key=key, limit=MAX_IMAGE_SIZE) - raise FirmwareFileError(msg) - return _pad4(raw) + return raw def _pad4(data: bytes) -> bytes: return data + b"\xff" * (-len(data) % 4) -def check_compatibility(image: FirmwareImage, bootloader: BootloaderInfo, *, force: bool = False) -> None: +def bootloader_crc32(data: bytes, state: int = 0) -> int: + """ + Return the raw CRC-32 state used by ArduPilot's bootloader protocol. + + This intentionally differs from :func:`binascii.crc32`, which applies the + conventional CRC-32 initial/final XOR values. ArduPilot's + ``crc32_small`` and ``Tools/scripts/uploader.py`` instead expose the raw + running state beginning at zero. + """ + for byte in data: + state ^= byte + for _bit in range(8): + state = (state >> 1) ^ (0xEDB88320 if state & 1 else 0) + return state & 0xFFFFFFFF + + +def firmware_content_sha256(image: bytes, extf_image: bytes = b"") -> str: + """Hash both raw APJ payload regions with unambiguous length prefixes.""" + digest = hashlib.sha256() + for region in (image, extf_image): + digest.update(struct.pack(" None: + """Require a release SHA-256 for the exact APJ descriptor bytes selected for upload.""" + if not re.fullmatch(r"[0-9a-fA-F]{64}", expected_sha256): + msg = _("trusted firmware SHA-256 must be 64 hexadecimal characters") + raise FirmwareIntegrityError(msg) + actual = image.metadata.apj_sha256 + if not hmac.compare_digest(actual, expected_sha256.lower()): + msg = _("selected firmware SHA-256 does not match the trusted digest") + raise FirmwareIntegrityError(msg) + + +def _board_ids_are_compatible(bootloader_board_id: int, image_board_id: int) -> bool: + """Return whether the bootloader's board ID accepts the firmware board ID.""" + compatible_board_ids = {33: 9} + return bootloader_board_id == image_board_id or compatible_board_ids.get(bootloader_board_id) == image_board_id + + +def check_bootloader_matches_connected_board(bootloader: BootloaderInfo, connected_board_id: int | None) -> None: + """Refuse a bootloader that differs from the board identified over MAVLink before reboot.""" + if connected_board_id is None or _board_ids_are_compatible(bootloader.board_id, connected_board_id): + return + msg = _("bootloader board_id {bootloader_id} differs from connected board_id {connected_id}").format( + bootloader_id=bootloader.board_id, connected_id=connected_board_id + ) + raise FirmwareTargetMismatchError(msg) + + +def check_compatibility(image: FirmwareImage, bootloader: BootloaderInfo) -> None: """Raise FirmwareCompatibilityError unless the image can safely be flashed on this bootloader.""" if not BL_REV_MIN <= bootloader.protocol_revision <= BL_REV_MAX: msg = _("unsupported bootloader protocol revision {revision}").format(revision=bootloader.protocol_revision) raise BootloaderProtocolError(msg) meta = image.metadata - if meta.board_id != bootloader.board_id and not force: + if not _board_ids_are_compatible(bootloader.board_id, meta.board_id): msg = _("firmware is for board_id {image_id}, board reports {board_id}").format( image_id=meta.board_id, board_id=bootloader.board_id ) raise FirmwareCompatibilityError(msg) - if meta.image_size > bootloader.flash_size: + if len(image.image) > bootloader.flash_size: msg = _("image of {size} bytes exceeds flash of {flash} bytes").format( - size=meta.image_size, flash=bootloader.flash_size + size=len(image.image), flash=bootloader.flash_size ) raise FirmwareCompatibilityError(msg) - if meta.extf_image_size > bootloader.extf_size: + if len(image.extf_image) > bootloader.extf_size: msg = _("external image of {size} bytes exceeds external flash of {extf} bytes").format( - size=meta.extf_image_size, extf=bootloader.extf_size + size=len(image.extf_image), extf=bootloader.extf_size ) raise FirmwareCompatibilityError(msg) -# Bootloader protocol codec. Encoders build the bytes to write, decoders check what came back. - - -def encode_get_sync() -> bytes: - """GET_SYNC command.""" - return GET_SYNC + EOC - - -def encode_get_device(param: bytes) -> bytes: - """GET_DEVICE command for one INFO_* parameter.""" - return GET_DEVICE + param + EOC - - -def encode_chip_erase(*, full: bool = False) -> bytes: - """CHIP_ERASE, or CHIP_FULL_ERASE when full is set.""" - return (CHIP_FULL_ERASE if full else CHIP_ERASE) + EOC - - -def encode_prog_multi(chunk: bytes) -> bytes: - """PROG_MULTI command carrying one chunk of at most PROG_MULTI_MAX bytes.""" - if not 0 < len(chunk) <= PROG_MULTI_MAX or len(chunk) % 4: - msg = _("PROG_MULTI chunk must be 4..{limit} bytes and a multiple of 4, got {size}").format( - limit=PROG_MULTI_MAX, size=len(chunk) +def verify_reconnected_firmware(image: FirmwareImage, *, board_id: str) -> None: + """Require the MAVLink APJ board identity after reboot to match the selected image.""" + if not board_id: + msg = _("reconnected flight controller did not report an APJ board_id") + raise FirmwareIdentityError(msg) + if board_id != str(image.metadata.board_id): + msg = _("reconnected board_id {actual_id} does not match firmware board_id {expected_id}").format( + actual_id=board_id, expected_id=image.metadata.board_id ) - raise ValueError(msg) - return PROG_MULTI + bytes([len(chunk)]) + chunk + EOC - - -def encode_read_multi(length: int) -> bytes: - """READ_MULTI command for length bytes.""" - if not 0 < length <= READ_MULTI_MAX: - msg = _("READ_MULTI length must be 1..{limit}, got {length}").format(limit=READ_MULTI_MAX, length=length) - raise ValueError(msg) - return READ_MULTI + bytes([length]) + EOC - - -def encode_get_crc() -> bytes: - """GET_CRC command.""" - return GET_CRC + EOC - - -def encode_reboot() -> bytes: - """REBOOT command.""" - return REBOOT + EOC - - -def decode_sync(reply: bytes) -> None: - """Validate the two byte INSYNC/status reply that follows every command, raising on anything but OK.""" - if len(reply) < 2: - msg = _("short reply, expected INSYNC and status, got {reply}").format(reply=reply) - raise BootloaderProtocolError(msg) - if reply[0:1] != INSYNC: - msg = _("expected INSYNC, got {byte}").format(byte=reply[0:1]) - raise BootloaderProtocolError(msg) - status = reply[1:2] - if status == OK: - return - messages = { - INVALID: _("bootloader reports INVALID OPERATION"), - FAILED: _("bootloader reports OPERATION FAILED"), - BAD_SILICON_REV: _("programming not supported for this silicon revision"), - } - msg = messages.get(status) or _("unexpected status {status} instead of OK").format(status=status) - raise BootloaderProtocolError(msg) - - -def decode_uint32(reply: bytes) -> int: - """Little endian u32 as returned by GET_DEVICE and GET_CRC, before the sync bytes.""" - if len(reply) < 4: - msg = _("short reply, expected 4 bytes, got {size}").format(size=len(reply)) - raise BootloaderProtocolError(msg) - return int(struct.unpack(" list[bytes]: - """Split a padded image into PROG_MULTI sized chunks, in flash order.""" - return [image[i : i + PROG_MULTI_MAX] for i in range(0, len(image), PROG_MULTI_MAX)] + raise FirmwareIdentityError(msg) diff --git a/tests/test_architecture_firmware_upload.py b/tests/test_architecture_firmware_upload.py new file mode 100644 index 000000000..eea21f5f3 --- /dev/null +++ b/tests/test_architecture_firmware_upload.py @@ -0,0 +1,28 @@ +""" +Documentation contracts for the firmware upload architecture. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +# ruff: noqa: INP001 + +from pathlib import Path + + +def test_firmware_upload_architecture_documents_apj_only_input() -> None: + """ + The firmware-upload architecture documents the format accepted by the model. + + GIVEN: The firmware-upload architecture document describes file selection + WHEN: The documented input format is inspected + THEN: It identifies APJ input and does not advertise unsupported BIN input + """ + architecture = Path(__file__).parents[1] / "ARCHITECTURE_firmware_upload.md" + document = architecture.read_text(encoding="utf-8") + + assert "APJ input" in document + assert "APJ/BIN" not in document diff --git a/tests/test_backend_flightcontroller_bootloader.py b/tests/test_backend_flightcontroller_bootloader.py new file mode 100755 index 000000000..aee6af40c --- /dev/null +++ b/tests/test_backend_flightcontroller_bootloader.py @@ -0,0 +1,1081 @@ +#!/usr/bin/env python3 + +""" +Tests for the injected bootloader transport adapter. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2024-2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +import base64 +import hashlib +import json +import struct +import zlib +from pathlib import Path + +import pytest +from serial.tools.list_ports_common import ListPortInfo + +from ardupilot_methodic_configurator import backend_flightcontroller_bootloader as bl +from ardupilot_methodic_configurator import data_model_firmware_upload as fw +from ardupilot_methodic_configurator.backend_flightcontroller import FlightController +from ardupilot_methodic_configurator.data_model_flightcontroller_info import FlightControllerInfo + +# pylint: disable=too-many-instance-attributes,too-few-public-methods,protected-access,too-many-lines + + +def apj(image: bytes, *, extf_image: bytes = b"", **overrides: object) -> bytes: + desc: dict[str, object] = { + "board_id": 9, + "image_size": len(image), + "image": base64.b64encode(zlib.compress(image)).decode(), + } + if extf_image: + desc.update( + extf_image_size=len(extf_image), + extf_image=base64.b64encode(zlib.compress(extf_image)).decode(), + ) + desc.update(overrides) + return json.dumps(desc).encode() + + +def trusted_digest(image: bytes, extf_image: bytes = b"") -> str: + """Return the independently supplied release digest expected by the facade.""" + return hashlib.sha256(apj(image, extf_image=extf_image)).hexdigest() + + +@pytest.fixture(autouse=True) +def stable_test_device_identity(monkeypatch: pytest.MonkeyPatch) -> None: + """Keep facade tests independent of the host's actual USB inventory.""" + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.capture_serial_device_identity", + lambda _device: None, + ) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.has_stable_bootloader_device_identity", + lambda *_args: True, + ) + + +class FakeBootloaderTransport: + """Byte-oriented fake with short reads, rev-2 support, and external flash.""" + + def __init__(self, *, revision: int = 5, board_id: int = 9, flash_size: int = 2048, extf_size: int = 1024) -> None: + self.revision = revision + self.board_id = board_id + self.flash_size = flash_size + self.extf_size = extf_size + self.flash = b"" + self.extf = b"" + self.extf_erase_size: int | None = None + self.closed = False + self.rebooted = False + self.commands: list[bytes] = [] + self._reply = bytearray() + self._read_offset = 0 + + def write(self, data: bytes) -> int: + assert data.endswith(bl.EOC) + command, body = data[:1], data[1:-1] + self.commands.append(command) + sync = bl.INSYNC + bl.OK + if command == bl.GET_SYNC: + self._reply.extend(sync) + elif command == bl.GET_DEVICE: + values = { + bl.INFO_BL_REV: self.revision, + bl.INFO_BOARD_ID: self.board_id, + bl.INFO_BOARD_REV: 0, + bl.INFO_FLASH_SIZE: self.flash_size, + bl.INFO_EXTF_SIZE: self.extf_size, + } + self._reply.extend(struct.pack("= 3: + self._reply.extend(sync) + return len(data) + + def read(self, size: int = 1) -> bytes: + # Deliberately emulate short serial reads. + count = min(size, 2, len(self._reply)) + data = bytes(self._reply[:count]) + del self._reply[:count] + return data + + def flush(self) -> None: + pass + + def reset_input_buffer(self) -> None: + self._reply.clear() + + def close(self) -> None: + self.closed = True + + +@pytest.mark.parametrize("revision", [2, 5]) +def test_upload_verifies_revision_specific_protocol_and_closes(revision: int) -> None: + image = fw.parse_apj(apj(b"abc")) + transport = FakeBootloaderTransport(revision=revision) + + info = bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) + + assert info.protocol_revision == revision + assert transport.flash == b"abc\xff" + assert transport.closed + + +def test_v2_verification_uses_the_read_chunk_limit(monkeypatch: pytest.MonkeyPatch) -> None: + """ + Legacy read-back verification uses the bootloader's read limit independently. + + GIVEN: Program and read commands have different protocol chunk limits + WHEN: A revision-two image is verified after programming + THEN: Read-back requests stay within the read-command limit + """ + monkeypatch.setattr(bl, "PROG_MULTI_MAX", 252) + monkeypatch.setattr(bl, "READ_MULTI_MAX", 64) + image = fw.parse_apj(apj(bytes(range(100)), board_id=9)) + + info = bl.BootloaderClient(FakeBootloaderTransport(revision=2)).upload(image, confirmation_requested=lambda *_args: True) + + assert info.protocol_revision == 2 + + +def test_identify_resets_stale_bytes_before_old_bootloader_fallback() -> None: + """ + Old bootloader fallback synchronization starts with a clean input buffer. + + GIVEN: The optional external-flash query leaves a stale response on the serial input + WHEN: The client falls back after the query fails + THEN: The stale bytes are cleared before fallback synchronization and identification succeeds + """ + + class OldBootloaderTransport(FakeBootloaderTransport): + """Leave stale bytes after rejecting the external-flash query.""" + + def __init__(self) -> None: + super().__init__(revision=2) + self.reset_count = 0 + + def write(self, data: bytes) -> int: + if data[:1] == bl.GET_DEVICE and data[1:-1] == bl.INFO_EXTF_SIZE: + self._reply.extend(bl.INVALID + b"old") + return len(data) + return super().write(data) + + def read(self, size: int = 1) -> bytes: + if not self._reply: + msg = "old bootloader rejected external-flash query" + raise OSError(msg) + return super().read(size) + + def reset_input_buffer(self) -> None: + self.reset_count += 1 + super().reset_input_buffer() + + transport = OldBootloaderTransport() + + info = bl.BootloaderClient(transport).identify() + + assert info.extf_size == 0 + assert transport.reset_count == 2 + + +def test_uploads_and_verifies_external_flash() -> None: + """ + External-flash erase accepts progress bytes that overlap protocol markers. + + GIVEN: The bootloader reports a monotonic erase-progress ramp containing 18% + WHEN: An APJ with an external image is uploaded + THEN: The client consumes progress and waits for the final INSYNC/OK response + """ + image = fw.parse_apj(apj(b"abcd", extf_image=b"ext")) + transport = FakeBootloaderTransport() + + bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) + + assert transport.extf == b"ext\xff" + assert transport.extf_erase_size == image.metadata.extf_image_size + assert transport.closed + + +def test_external_erase_rejects_regressing_progress() -> None: + class RegressingEraseTransport(FakeBootloaderTransport): + """Reports external erase progress that moves backwards.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_ERASE: + self._reply.extend(bl.INSYNC + bl.OK + bytes((10, 20, 19))) + return len(data) + return super().write(data) + + client = bl.BootloaderClient(RegressingEraseTransport()) + + with pytest.raises(fw.BootloaderProtocolError, match="regressed"): + client._erase_external(3) + + +def test_stalled_external_erase_times_out_without_rebooting_after_erase() -> None: + class StalledEraseTransport(FakeBootloaderTransport): + """Stops reporting progress after starting an external erase.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_ERASE: + self._reply.extend(bl.INSYNC + bl.OK + b"\x00") + return len(data) + return super().write(data) + + now = 0.0 + + def clock() -> float: + nonlocal now + now += 0.25 + return now + + image = fw.parse_apj(apj(b"abcd", extf_image=b"ext")) + transport = StalledEraseTransport() + + with pytest.raises(fw.BootloaderProtocolError, match="timeout waiting"): + bl.BootloaderClient(transport, clock=clock).upload(image, confirmation_requested=lambda *_args: True) + + assert not transport.rebooted + + +def test_external_erase_times_out_when_progress_never_advances() -> None: + """Repeated progress bytes must not keep a stalled erase alive indefinitely.""" + + class RepeatingProgressTransport(FakeBootloaderTransport): + """Reports 1% forever after acknowledging the erase command.""" + + def __init__(self) -> None: + super().__init__() + self.erasing = False + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_ERASE: + self.erasing = True + self._reply.extend(bl.INSYNC + bl.OK) + return len(data) + return super().write(data) + + def read(self, size: int = 1) -> bytes: + reply = super().read(size) + return reply or (b"\x01" if self.erasing else b"") + + now = 0.0 + + def clock() -> float: + nonlocal now + now += 0.25 + return now + + with pytest.raises(fw.BootloaderProtocolError, match="timeout waiting"): + bl.BootloaderClient(RepeatingProgressTransport(), clock=clock)._erase_external(3) + + +def test_external_erase_refreshes_deadline_only_when_progress_advances() -> None: + """A progressing erase may exceed 20 seconds, but each advance refreshes inactivity timeout.""" + + class SlowProgressTransport: + """Deliver one erase-progress byte every two simulated seconds.""" + + def __init__(self) -> None: + self.now = 0.0 + self.reply = bytearray() + + def write(self, data: bytes) -> int: + assert data[:1] == bl.EXTF_ERASE + self.reply.extend(bl.INSYNC + bl.OK + bytes(range(0, 101, 10)) + bl.INSYNC + bl.OK) + return len(data) + + def flush(self) -> None: + pass + + def read(self, size: int = 1) -> bytes: + self.now += 2.0 + count = min(1, size, len(self.reply)) + data = bytes(self.reply[:count]) + del self.reply[:count] + return data + + transport = SlowProgressTransport() + bl.BootloaderClient(transport, timeout=10.0, clock=lambda: transport.now)._erase_external(3) + + assert transport.now > bl.ERASE_TIMEOUT + + +def test_external_only_upload_does_not_touch_internal_flash() -> None: + """External-only firmware skips internal erase, programming, and verification.""" + image = fw.parse_apj(apj(b"", extf_image=b"external")) + transport = FakeBootloaderTransport() + + bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) + + assert transport.flash == b"" + assert transport.extf == b"external" + assert transport.closed + assert not any( + command in {bl.CHIP_ERASE, bl.CHIP_FULL_ERASE, bl.PROG_MULTI, bl.CHIP_VERIFY, bl.READ_MULTI, bl.GET_CRC} + for command in transport.commands + ) + + +def test_failed_upload_still_closes_transport() -> None: + image = fw.parse_apj(apj(b"abcd")) + transport = FakeBootloaderTransport(flash_size=3) + + with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): + bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) + assert transport.closed + + +def test_read_timeout_is_bounded_even_when_serial_keeps_returning_empty_reads() -> None: + class EmptyReadTransport(FakeBootloaderTransport): + """Transport that simulates a serial port timing out without a response.""" + + def read(self, size: int = 1) -> bytes: + return b"" + + clock_values = iter([0.0, 1.0]) + client = bl.BootloaderClient(EmptyReadTransport(), timeout=1.0, clock=lambda: next(clock_values)) + + with pytest.raises(fw.BootloaderProtocolError, match="timeout waiting"): + client._read_exact(1) # Exercise the transport deadline directly. + + +def test_client_requires_confirmation_and_reboots_before_closing() -> None: + transport = FakeBootloaderTransport() + + with pytest.raises(fw.FirmwareConfirmationError, match="explicit confirmation"): + bl.BootloaderClient(transport).upload(fw.parse_apj(apj(b"abcd"))) + + assert transport.rebooted + assert transport.closed + + +def test_declined_confirmation_reboots_before_closing() -> None: + transport = FakeBootloaderTransport() + + with pytest.raises(fw.FirmwareUploadCancelledError, match="not confirmed"): + bl.BootloaderClient(transport).upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: False) + + assert transport.rebooted + assert transport.closed + + +def test_parse_apj_is_pure_and_requires_declared_size_to_match() -> None: + contents = apj(b"four", image_size=1) + + with pytest.raises(fw.FirmwareFileError, match="image_size"): + fw.parse_apj(contents, path=Path("firmware.apj")) + + +def test_invalid_board_revision_is_a_typed_file_error() -> None: + with pytest.raises(fw.FirmwareFileError, match="metadata"): + fw.parse_apj(apj(b"four", board_revision="not-a-number")) + + +def test_padded_payload_capacity_and_compatible_board_mapping_are_checked() -> None: + image = fw.parse_apj(apj(b"abc")) + with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): + fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 3)) + + fw.check_compatibility(image, fw.BootloaderInfo(5, 33, 0, 4)) + + +def test_bounded_decompression_rejects_before_full_output_is_allocated(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(fw, "MAX_IMAGE_SIZE", 32) + + with pytest.raises(fw.FirmwareFileError, match="exceeds"): + fw.parse_apj(apj(b"x" * 64)) + + +def test_parser_rejects_oversized_encoded_blob_before_base64_decode(monkeypatch: pytest.MonkeyPatch) -> None: + monkeypatch.setattr(fw, "MAX_ENCODED_BLOB_SIZE", 8) + + with pytest.raises(fw.FirmwareFileError, match="encoded"): + fw.parse_apj(apj(b"abcd")) + + +def test_reader_rejects_oversized_apj_descriptor_before_reading(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr(bl, "MAX_APJ_DESCRIPTOR_SIZE", 8) + + with pytest.raises(fw.FirmwareFileError, match="descriptor exceeds"): + bl.load_apj(path) + + +def test_bootloader_port_is_re_resolved_by_usb_identity_and_ambiguity_is_refused( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """A re-enumerated bootloader must not reuse an arbitrary stale serial port.""" + monkeypatch.setattr(bl, "sys_platform", "win32") + reenumerated = ListPortInfo("COM11") + reenumerated.location = "1-2.3" + reenumerated.serial_number = "FC-123" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [reenumerated]) + + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123") + assert bl.resolve_bootloader_device("COM7", identity) == "COM11" + + duplicate = ListPortInfo("COM12") + duplicate.location = "1-2.3" + duplicate.serial_number = "FC-123" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [reenumerated, duplicate]) + with pytest.raises(OSError, match="cannot uniquely"): + bl.resolve_bootloader_device("COM7", identity) + + +def test_usb_identity_is_checked_before_linux_by_path_fallback(monkeypatch: pytest.MonkeyPatch) -> None: + """A matching USB identity must win without probing the filesystem fallback.""" + monkeypatch.setattr(bl, "sys_platform", "linux") + reenumerated = ListPortInfo("/dev/ttyACM1") + reenumerated.location = "1-2.3" + reenumerated.serial_number = "FC-123" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [reenumerated]) + + def unexpected_by_path_probe(_device: str) -> str: + message = "Linux by-path fallback must not run after a USB identity match" + raise AssertionError(message) + + monkeypatch.setattr(bl, "_capture_linux_persistent_path", unexpected_by_path_probe) + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123") + + assert bl.resolve_bootloader_device("/dev/ttyACM0", identity) == "/dev/ttyACM1" + + +def test_bootloader_port_prefers_usb_identity_over_reused_linux_device_path(monkeypatch: pytest.MonkeyPatch) -> None: + """A newly assigned tty path must not override the captured controller identity.""" + monkeypatch.setattr(bl, "sys_platform", "linux") + reenumerated = ListPortInfo("/dev/ttyACM1") + reenumerated.location = "1-2.3" + reenumerated.serial_number = "FC-123" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [reenumerated]) + + assert ( + bl.resolve_bootloader_device("/dev/ttyACM0", bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123")) + == "/dev/ttyACM1" + ) + + +def test_bootloader_port_retains_linux_by_path_without_usb_metadata(monkeypatch: pytest.MonkeyPatch) -> None: + """A by-path fallback captured before reboot must not be resolved again afterward.""" + monkeypatch.setattr(bl, "sys_platform", "linux") + port = ListPortInfo("/dev/ttyACM0") + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [port]) + persistent_path = "/dev/serial/by-path/usb-controller-a" + monkeypatch.setattr(bl, "_capture_linux_persistent_path", lambda _device: persistent_path) + + identity = bl.capture_serial_device_identity("/dev/ttyACM0") + + assert identity == bl.SerialDeviceIdentity(persistent_path=persistent_path) + assert bl.resolve_bootloader_device("/dev/ttyACM0", identity) == persistent_path + + +def test_client_reports_recovery_error_when_pre_erase_reboot_fails() -> None: + class RebootFailureTransport(FakeBootloaderTransport): + """Fail reboot writes to exercise the recovery error path.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.REBOOT: + msg = "reboot write failed" + raise OSError(msg) + return super().write(data) + + transport = RebootFailureTransport(board_id=9) + image = fw.parse_apj(apj(b"abcd", board_id=42)) + + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match="power-cycle"): + bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) + + assert transport.closed + + +def test_backend_rejects_invalid_timeout_before_entering_bootloader() -> None: + entered = False + + def enter() -> None: + nonlocal entered + entered = True + + with pytest.raises(ValueError, match="timeout"): + bl.FlightControllerBootloaderBackend("COM7", 115200, timeout=0, enter_bootloader=enter) + + assert not entered + + +class _Master: + def __init__(self) -> None: + self.held_in_bootloader = False + + def reboot_autopilot(self, *, hold_in_bootloader: bool) -> None: + self.held_in_bootloader = hold_in_bootloader + + +class _Connection: + def __init__(self, device: str = "COM7") -> None: + self.master = _Master() + self.comport = object() + self.comport_device = device + self.baudrate = 115200 + self.active_baudrate = 921600 + self.info = FlightControllerInfo() + self.info.apj_board_id = "9" + self.disconnected = False + self.reconnected = False + self.reconnect_baudrate: int | None = None + self.reconnect_device: str | None = None + + def discover_connections(self, **_kwargs: object) -> None: + pass + + def disconnect(self) -> None: + self.disconnected = True + self.master = None # type: ignore[assignment] + + def create_connection_with_retry(self, *_args: object, **kwargs: object) -> str: + self.reconnected = True + self.reconnect_baudrate = kwargs.get("baudrate") if isinstance(kwargs.get("baudrate"), int) else None + return "" + + def connect(self, device: str, **kwargs: object) -> str: + self.reconnect_device = device + self.comport_device = device + self.master = _Master() + return self.create_connection_with_retry(**kwargs) + + +class _Params: + def __init__(self) -> None: + self.cleared = 0 + + def clear_parameters(self) -> None: + self.cleared += 1 + + +class _Commands: + COMMAND_ACK_TIMEOUT = 5.0 + + def __init__(self, result: tuple[bool, str] = (True, "")) -> None: + self.result = result + self.calls: list[dict[str, object]] = [] + + def send_command_and_wait_ack(self, **kwargs: object) -> tuple[bool, str]: + self.calls.append(kwargs) + return self.result + + def reboot_to_bootloader(self) -> tuple[bool, str]: + return self.send_command_and_wait_ack(param1=3) + + +def test_facade_reconnects_using_the_reenumerated_serial_device(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """ + Reconnection follows the controller's stable identity after re-enumeration. + + GIVEN: The controller was connected on COM7 and is captured with a stable USB identity + WHEN: Flashing makes the controller reappear on COM13 + THEN: The facade reconnects through COM13 and updates the active connection + """ + connection = _Connection() + params = _Params() + commands = _Commands() + transport = FakeBootloaderTransport() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=params, # type: ignore[arg-type] + commands_manager=commands, # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", + lambda _device, _identity: "COM13", + ) + + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: transport, + confirmation_requested=lambda *_args: True, + ) + + assert connection.disconnected + assert connection.reconnected + assert connection.reconnect_device == "COM13" + assert connection.reconnect_baudrate == 921600 + assert commands.calls[0]["param1"] == 3 + assert transport.closed + assert params.cleared == 2 + + +def test_facade_waits_for_serial_identity_to_reappear_before_reconnecting( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ + Reconnection waits while the board is temporarily absent during re-enumeration. + + GIVEN: The stable USB identity is unavailable during the first resolver calls + WHEN: The firmware upload completes and starts reconnection + THEN: The facade retries identity resolution and connects after the board reappears + """ + connection = _Connection() + commands = _Commands() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=commands, # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + resolver_calls = 0 + + def resolve(_device: str, _identity: object) -> str: + nonlocal resolver_calls + resolver_calls += 1 + if resolver_calls < 3: + msg = "device is still re-enumerating" + raise OSError(msg) + return "COM13" + + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", + resolve, + ) + + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + assert resolver_calls == 3 + assert connection.reconnect_device == "COM13" + + +def test_facade_reports_rejected_bootloader_entry_without_releasing_serial(tmp_path: Path) -> None: + """ + A rejected bootloader command stops the upload before disconnecting MAVLink. + + GIVEN: The connected vehicle rejects the reboot-and-hold command + WHEN: A firmware upload requests bootloader entry + THEN: The user receives the rejection reason and the active connection remains available + """ + connection = _Connection() + commands = _Commands((False, "Command denied")) + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=commands, # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareConnectionError, match="Command denied"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + confirmation_requested=lambda *_args: True, + ) + + assert not connection.disconnected + assert connection.master is not None + + +def test_facade_refuses_network_mavlink_connection() -> None: + connection = _Connection("udp:127.0.0.1:14550") + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=object(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + + with pytest.raises(fw.FirmwareConnectionError, match="direct serial"): + controller.upload_apj_firmware(Path("ignored.apj")) + + +def test_facade_requires_pre_reboot_board_identity(tmp_path: Path) -> None: + connection = _Connection() + connection.info.apj_board_id = "" + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=object(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareConnectionError, match="APJ board_id"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + confirmation_requested=lambda *_args: True, + ) + + assert not connection.master.held_in_bootloader + + +def test_backend_retries_serial_open_after_bootloader_entry() -> None: + image = fw.parse_apj(apj(b"abcd")) + attempts = 0 + delays: list[float] = [] + entered = False + transport = FakeBootloaderTransport() + + def enter() -> None: + nonlocal entered + entered = True + + def open_transport(*_args: object) -> bl.BootloaderTransport: + nonlocal attempts + attempts += 1 + if attempts == 1: + msg = "bootloader port has not appeared yet" + raise OSError(msg) + return transport + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + enter_bootloader=enter, + serial_factory=open_transport, + open_retries=2, + retry_delay=0.25, + sleep=delays.append, + ) + + backend.upload(image, confirmation_requested=lambda *_args: True) + + assert entered + assert attempts == 2 + assert delays == [0.25] + + +def test_backend_requires_manual_recovery_when_the_held_bootloader_never_opens() -> None: + entered = False + + def enter() -> None: + nonlocal entered + entered = True + + def open_transport(*_args: object) -> bl.BootloaderTransport: + msg = "bootloader port has not appeared" + raise OSError(msg) + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + enter_bootloader=enter, + serial_factory=open_transport, + open_retries=1, + ) + + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match="power-cycle"): + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert entered + + +def test_backend_retries_bootloader_identification_after_stale_serial_data() -> None: + """ + A failed identification retry reuses the held bootloader without re-entry. + + GIVEN: The first bootloader connection returns stale synchronization data + WHEN: The backend closes that connection and retries identification + THEN: It opens the next transport without requesting bootloader entry again + """ + + class StaleTransport(FakeBootloaderTransport): + """Replies with stale bytes to the initial synchronization request.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.GET_SYNC: + self._reply.extend(b"\x00\x10") + return len(data) + return super().write(data) + + stale = StaleTransport() + working = FakeBootloaderTransport() + transports = iter([stale, working]) + entries = 0 + + def enter() -> None: + nonlocal entries + entries += 1 + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + enter_bootloader=enter, + serial_factory=lambda *_args: next(transports), + open_retries=2, + retry_delay=0, + sleep=lambda _delay: None, + ) + + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert stale.closed + assert working.closed + assert entries == 1 + assert not stale.rebooted + + +def test_upload_reports_protocol_stages_and_confirmation_boundary() -> None: + events: list[bl.UploadStage] = [] + confirmed: list[fw.BootloaderInfo] = [] + transport = FakeBootloaderTransport() + + bl.BootloaderClient(transport).upload( + fw.parse_apj(apj(b"abcd")), + confirmation_requested=lambda _image, info: confirmed.append(info) is None, + progress_callback=lambda stage, _done, _total: events.append(stage), + ) + + assert confirmed + assert fw.UploadStage.AWAITING_CONFIRMATION in events + assert fw.UploadStage.ERASING in events + assert fw.UploadStage.PROGRAMMING in events + assert fw.UploadStage.VERIFYING in events + assert fw.UploadStage.REBOOTING in events + + +def test_verify_failure_reports_the_verifying_stage() -> None: + class BadCrcTransport(FakeBootloaderTransport): + """Reports a CRC which cannot match the flashed image.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.GET_CRC: + self._reply.extend(struct.pack(" None: + transport = FakeBootloaderTransport() + + with pytest.raises(fw.FirmwareUploadCancelledError, match="before entering"): + bl.BootloaderClient(transport).upload( + fw.parse_apj(apj(b"abcd")), + cancellation_requested=lambda: True, + confirmation_requested=lambda *_args: True, + ) + + assert transport.flash == b"" + assert transport.extf == b"" + assert transport.closed + + +def test_facade_requires_explicit_confirmation(tmp_path: Path) -> None: + controller = FlightController( + connection_manager=_Connection(), # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=object(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareConfirmationError, match="explicit confirmation"): + controller.upload_apj_firmware(path) + + +def test_facade_recovers_connection_after_declined_confirmation(tmp_path: Path) -> None: + connection = _Connection() + transport = FakeBootloaderTransport() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareUploadCancelledError, match="not confirmed"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: transport, + confirmation_requested=lambda *_args: False, + ) + + assert transport.rebooted + assert connection.reconnected + + +def test_facade_refuses_a_different_bootloader_and_recovers(tmp_path: Path) -> None: + connection = _Connection() + connection.info.apj_board_id = "42" + transport = FakeBootloaderTransport(board_id=9) + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareTargetMismatchError, match="differs from connected"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: transport, + confirmation_requested=lambda *_args: True, + ) + + assert transport.rebooted + assert connection.reconnected + + +def test_facade_recovers_after_confirmation_callback_error(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """A GUI callback failure before erase must not strand the board in bootloader mode.""" + connection = _Connection() + transport = FakeBootloaderTransport() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.capture_serial_device_identity", + lambda _device: None, + ) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.has_stable_bootloader_device_identity", + lambda *_args: True, + ) + + def raise_from_confirmation(*_args: object) -> bool: + msg = "confirmation UI failed" + raise RuntimeError(msg) + + with pytest.raises(RuntimeError, match="confirmation UI failed"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: transport, + confirmation_requested=raise_from_confirmation, + ) + + assert transport.rebooted + assert connection.reconnected + + +def test_facade_does_not_attempt_mavlink_reconnect_without_a_bootloader_reboot( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + connection = _Connection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + def unavailable_upload( + backend: bl.FlightControllerBootloaderBackend, *_args: object, **_kwargs: object + ) -> fw.BootloaderInfo: + assert backend._enter_bootloader is not None + backend._enter_bootloader() + msg = "power-cycle the flight controller before reconnecting" + raise fw.FirmwareBootloaderRecoveryError(msg) + + monkeypatch.setattr(bl.FlightControllerBootloaderBackend, "upload", unavailable_upload) + + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match="power-cycle"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + confirmation_requested=lambda *_args: True, + ) + + assert connection.disconnected + assert not connection.reconnected + + +def test_facade_does_not_reboot_or_reconnect_after_programming_failure( + tmp_path: Path, +) -> None: + """A programming failure must leave recovery to the bootloader safety check.""" + + class ProgrammingFailureTransport(FakeBootloaderTransport): + """Fail the first internal-flash programming command.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.PROG_MULTI: + msg = "programming write failed" + raise OSError(msg) + return super().write(data) + + connection = _Connection() + transport = ProgrammingFailureTransport() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.BootloaderProtocolError, match="programming write failed"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: transport, + confirmation_requested=lambda *_args: True, + ) + + assert not transport.rebooted + assert not connection.reconnected diff --git a/tests/test_backend_flightcontroller_commands.py b/tests/test_backend_flightcontroller_commands.py index 17cdf00ea..89b1c54bc 100755 --- a/tests/test_backend_flightcontroller_commands.py +++ b/tests/test_backend_flightcontroller_commands.py @@ -24,6 +24,26 @@ # pylint: disable=too-many-lines +def test_reboot_to_bootloader_holds_bootloader_without_force_flags() -> None: + """The reboot command must hold in bootloader mode and preserve param6=0.""" + master = MagicMock() + master.target_system = 1 + master.target_component = 1 + acknowledgement = MagicMock() + acknowledgement.command = mavutil.mavlink.MAV_CMD_PREFLIGHT_REBOOT_SHUTDOWN + acknowledgement.result = mavutil.mavlink.MAV_RESULT_ACCEPTED + master.recv_match.return_value = acknowledgement + + connection = Mock() + connection.master = master + commands = FlightControllerCommands(params_manager=Mock(), connection_manager=connection) + + assert commands.reboot_to_bootloader() == (True, "") + + sent_parameters = master.mav.command_long_send.call_args.args[4:11] + assert sent_parameters == (3, 0, 0, 0, 0, 0, 0) + + class TestFlightControllerCommandsInitialization: """Test command manager initialization and setup.""" diff --git a/tests/test_bootloader_identity.py b/tests/test_bootloader_identity.py new file mode 100644 index 000000000..7607c700b --- /dev/null +++ b/tests/test_bootloader_identity.py @@ -0,0 +1,53 @@ +""" +Regression tests for bootloader device identity resolution. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +# ruff: noqa: INP001 + +import pytest +from serial.tools.list_ports_common import ListPortInfo + +from ardupilot_methodic_configurator import backend_flightcontroller_bootloader as bl + + +def test_bootloader_port_refuses_a_partial_usb_identity_match(monkeypatch: pytest.MonkeyPatch) -> None: + """ + A bootloader port must match every captured USB identity attribute. + + GIVEN: A controller was captured with both a USB location and serial number + WHEN: Only one of those attributes matches a re-enumerated port + THEN: The stale or different controller is rejected + """ + port = ListPortInfo("COM11") + port.location = "1-2.3" + port.serial_number = "FC-other" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123") + + with pytest.raises(OSError, match="cannot uniquely"): + bl.resolve_bootloader_device("COM7", identity) + + +def test_bootloader_port_refuses_an_ambiguous_macos_dual_cdc_match(monkeypatch: pytest.MonkeyPatch) -> None: + """An application-port interface identifier cannot safely identify a bootloader port.""" + mavlink_port = ListPortInfo("/dev/cu.usbmodem14101") + mavlink_port.location = "1-2.3" + mavlink_port.serial_number = "FC-123" + mavlink_port.interface = "0" + console_port = ListPortInfo("/dev/cu.usbmodem14102") + console_port.location = "1-2.3" + console_port.serial_number = "FC-123" + console_port.interface = "2" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [mavlink_port, console_port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123", interface="0") + + with pytest.raises(OSError, match="cannot uniquely"): + bl.resolve_bootloader_device("/dev/cu.usbmodem14101", identity) diff --git a/tests/test_bootloader_partial_read.py b/tests/test_bootloader_partial_read.py new file mode 100644 index 000000000..619dc446b --- /dev/null +++ b/tests/test_bootloader_partial_read.py @@ -0,0 +1,55 @@ +""" +Regression tests for bounded bootloader reads. + +This file is part of ArduPilot Methodic Configurator. https://github.com/ArduPilot/MethodicConfigurator + +SPDX-FileCopyrightText: 2026 Amilcar do Carmo Lucas + +SPDX-License-Identifier: GPL-3.0-or-later +""" + +# ruff: noqa: INP001 + +import pytest + +from ardupilot_methodic_configurator import backend_flightcontroller_bootloader as bl + + +def test_read_timeout_is_bounded_when_partial_reads_arrive_after_deadline() -> None: + """ + A partial serial response cannot extend the bootloader read deadline. + + GIVEN: A bootloader read has received only part of its expected response + WHEN: The deadline expires before the remaining bytes arrive + THEN: The read fails with a bounded protocol timeout + """ + + class PartialReadTransport: # pylint: disable=too-few-public-methods + """Return one byte per read to exercise the response deadline.""" + + def read(self, _size: int = 1) -> bytes: + return b"x" + + clock_values = iter([0.0, 1.0]) + client = bl.BootloaderClient(PartialReadTransport(), timeout=1.0, clock=lambda: next(clock_values)) + + with pytest.raises(bl.BootloaderProtocolError, match="timeout waiting"): + client._read_exact(2) # pylint: disable=protected-access + + +@pytest.mark.parametrize("arrival_time", [2.0, 2.05]) +def test_read_accepts_a_complete_reply_at_or_after_deadline(arrival_time: float) -> None: + """A complete serial reply is valid even when the clock reaches its deadline.""" + now = 0.0 + + class CompleteReadTransport: # pylint: disable=too-few-public-methods + """Return the complete response in one serial read.""" + + def read(self, _size: int = 1) -> bytes: + nonlocal now + now = arrival_time + return b"\x12\x10" + + client = bl.BootloaderClient(CompleteReadTransport(), timeout=2.0, clock=lambda: now) + + assert client._read_exact(2) == b"\x12\x10" # pylint: disable=protected-access diff --git a/tests/test_data_model_firmware_upload.py b/tests/test_data_model_firmware_upload.py index aa173cdf8..09c33f3ee 100755 --- a/tests/test_data_model_firmware_upload.py +++ b/tests/test_data_model_firmware_upload.py @@ -11,14 +11,15 @@ """ import base64 +import hashlib import json import struct import zlib -from binascii import crc32 from pathlib import Path import pytest +from ardupilot_methodic_configurator import backend_flightcontroller_bootloader as bootloader from ardupilot_methodic_configurator import data_model_firmware_upload as fw # pylint: disable=redefined-outer-name, too-few-public-methods, too-many-return-statements @@ -54,11 +55,14 @@ def test_user_sees_metadata_from_a_valid_apj_file(self, tmp_path: Path, small_im WHEN: it is loaded THEN: metadata comes from the descriptor and the image is padded to 4 bytes with 0xFF """ - image = fw.load_apj(write_apj(tmp_path, small_image)) + image = bootloader.load_apj(write_apj(tmp_path, small_image)) assert image.metadata.board_id == 9 assert image.metadata.image_size == len(small_image) assert image.metadata.firmware_version == "4.6.0" + assert image.metadata.content_sha256 == fw.firmware_content_sha256(small_image) + assert image.content_sha256() == image.metadata.content_sha256 + assert image.metadata.apj_sha256 == hashlib.sha256((tmp_path / "arducopter.apj").read_bytes()).hexdigest() assert image.image == small_image + b"\xff" assert len(image.image) % 4 == 0 assert image.extf_image == b"" @@ -98,7 +102,7 @@ def test_user_gets_a_clear_error_for_a_non_apj_file(self, tmp_path: Path) -> Non path.write_bytes(b"\x00" * 16) with pytest.raises(fw.FirmwareFileError, match=r"only \.apj"): - fw.load_apj(path) + bootloader.load_apj(path) @pytest.mark.parametrize( ("content", "reason"), @@ -126,7 +130,7 @@ def test_malformed_apj_is_rejected_before_any_serial_access(self, tmp_path: Path path.write_text(content, encoding="utf-8") with pytest.raises(fw.FirmwareFileError, match=reason): - fw.load_apj(path) + bootloader.load_apj(path) def test_oversized_image_is_rejected(self, tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """ @@ -141,7 +145,7 @@ def test_oversized_image_is_rejected(self, tmp_path: Path, monkeypatch: pytest.M path = write_apj(tmp_path, big) with pytest.raises(fw.FirmwareFileError, match="exceeds"): - fw.load_apj(path) + bootloader.load_apj(path) def test_crc_matches_the_reference_uploader_computation(self, tmp_path: Path, small_image: bytes) -> None: """ @@ -149,21 +153,33 @@ def test_crc_matches_the_reference_uploader_computation(self, tmp_path: Path, sm GIVEN: a loaded image and a flash size larger than the image WHEN: the CRC is computed - THEN: it equals crc32 over the image padded with 0xFF up to flash size, as Tools/scripts/uploader.py does + THEN: it equals ArduPilot's raw CRC-32 over the image padded with 0xFF up to flash size """ - image = fw.load_apj(write_apj(tmp_path, small_image)) + image = bootloader.load_apj(write_apj(tmp_path, small_image)) flash_size = 2048 - expected = crc32(image.image + b"\xff" * (flash_size - len(image.image)), 0) + expected = fw.bootloader_crc32(image.image + b"\xff" * (flash_size - len(image.image))) assert image.crc(flash_size) == expected + def test_crc_pads_to_an_unaligned_flash_size(self, tmp_path: Path) -> None: + """CRC padding covers exactly the remaining bytes for an unaligned flash size.""" + image = bootloader.load_apj(write_apj(tmp_path, b"abcd")) + flash_size = 7 + + assert image.crc(flash_size) == fw.bootloader_crc32(b"abcd\xff\xff\xff") + + def test_crc_matches_ardupilot_uploader_known_vector(self) -> None: + """The uploader's raw CRC state is not Python's finalized binascii CRC-32.""" + assert fw.bootloader_crc32(b"abc") == 0xCA6598D0 + assert fw.bootloader_crc32(b"bc", fw.bootloader_crc32(b"a")) == 0xCA6598D0 + class TestCompatibility: """The model refuses to flash anything that does not match the detected board.""" @pytest.fixture def image(self, tmp_path: Path, small_image: bytes) -> fw.FirmwareImage: - return fw.load_apj(write_apj(tmp_path, small_image)) + return bootloader.load_apj(write_apj(tmp_path, small_image)) def test_matching_board_and_enough_flash_is_accepted(self, image: fw.FirmwareImage) -> None: fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 2048)) @@ -172,12 +188,74 @@ def test_board_id_mismatch_is_refused_by_default(self, image: fw.FirmwareImage) with pytest.raises(fw.FirmwareCompatibilityError, match="board_id 9, board reports 42"): fw.check_compatibility(image, fw.BootloaderInfo(5, 42, 0, 2048)) - def test_board_id_mismatch_is_allowed_only_with_explicit_force(self, image: fw.FirmwareImage) -> None: - fw.check_compatibility(image, fw.BootloaderInfo(5, 42, 0, 2048), force=True) + def test_board_id_mismatch_cannot_be_overridden(self, image: fw.FirmwareImage) -> None: + with pytest.raises(fw.FirmwareCompatibilityError, match="board_id 9, board reports 42"): + fw.check_compatibility(image, fw.BootloaderInfo(5, 42, 0, 2048)) + + def test_bootloader_must_match_the_connected_board_when_known(self, image: fw.FirmwareImage) -> None: # pylint: disable=unused-argument + with pytest.raises(fw.FirmwareTargetMismatchError, match="differs from connected"): + fw.check_bootloader_matches_connected_board(fw.BootloaderInfo(5, 9, 0, 2048), 42) + + def test_missing_connected_board_identity_does_not_block_upload(self, image: fw.FirmwareImage) -> None: # pylint: disable=unused-argument + fw.check_bootloader_matches_connected_board(fw.BootloaderInfo(5, 9, 0, 2048), None) - def test_image_larger_than_flash_is_refused_even_when_forced(self, image: fw.FirmwareImage) -> None: + def test_image_larger_than_flash_is_refused(self, image: fw.FirmwareImage) -> None: with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): - fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 512), force=True) + fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 512)) + + def test_reconnected_firmware_board_identity_is_required(self, image: fw.FirmwareImage) -> None: + with pytest.raises(fw.FirmwareIdentityError, match="did not report an APJ board_id"): + fw.verify_reconnected_firmware(image, board_id="") + + with pytest.raises(fw.FirmwareIdentityError, match="board_id 42"): + fw.verify_reconnected_firmware(image, board_id="42") + + fw.verify_reconnected_firmware(image, board_id="9") + + def test_trusted_digest_must_match_the_complete_apj_descriptor(self, image: fw.FirmwareImage) -> None: + fw.verify_expected_firmware_digest(image, image.metadata.apj_sha256.upper()) + + with pytest.raises(fw.FirmwareIntegrityError, match="does not match"): + fw.verify_expected_firmware_digest(image, hashlib.sha256(b"unrelated").hexdigest()) + + def test_trusted_digest_rejects_board_metadata_tampering(self, tmp_path: Path) -> None: + """ + A trusted digest covers APJ metadata as well as the firmware payload. + + GIVEN: A trusted digest was recorded for an APJ descriptor + WHEN: The same payload is rewritten with a different board ID + THEN: The descriptor is rejected before upload + """ + original_path = write_apj(tmp_path, b"abcd", board_id=9) + trusted_digest = hashlib.sha256(original_path.read_bytes()).hexdigest() + tampered_path = tmp_path / "tampered.apj" + tampered_path.write_bytes(write_apj(tmp_path, b"abcd", board_id=42).read_bytes()) + + with pytest.raises(fw.FirmwareIntegrityError, match="does not match"): + fw.verify_expected_firmware_digest(bootloader.load_apj(tampered_path), trusted_digest) + + @pytest.mark.parametrize( + ("field", "value"), + [ + ("board_id", True), + ("board_id", 9.9), + ("image_size", False), + ("image_size", 4.9), + ("extf_image_size", 0.1), + ("board_revision", True), + ], + ) + def test_boolean_and_float_metadata_are_rejected(self, tmp_path: Path, field: str, value: object) -> None: + """ + APJ integer metadata rejects values that would be silently coerced. + + GIVEN: An APJ descriptor contains boolean or floating-point metadata + WHEN: The descriptor is loaded for firmware upload + THEN: Loading fails with a metadata validation error + """ + overrides = {field: value} + with pytest.raises(fw.FirmwareFileError, match="metadata"): + bootloader.load_apj(write_apj(tmp_path, b"abcd", **overrides)) @pytest.mark.parametrize("revision", [1, 6]) def test_unsupported_bootloader_revision_is_refused(self, image: fw.FirmwareImage, revision: int) -> None: @@ -189,8 +267,26 @@ class TestStateMachine: """Cancellation is only offered while it is still safe.""" def test_happy_path_walks_every_stage_in_order(self) -> None: + """ + The happy path confirms the image after bootloader identification. + + GIVEN: A firmware upload starts in the idle state + WHEN: The state machine advances through the successful workflow + THEN: Bootloader identification precedes confirmation and erase + """ stage = fw.UploadStage.IDLE - for target in list(fw.UploadStage)[1:11]: + for target in ( + fw.UploadStage.INSPECTING, + fw.UploadStage.ENTERING_BOOTLOADER, + fw.UploadStage.IDENTIFYING, + fw.UploadStage.AWAITING_CONFIRMATION, + fw.UploadStage.ERASING, + fw.UploadStage.PROGRAMMING, + fw.UploadStage.VERIFYING, + fw.UploadStage.REBOOTING, + fw.UploadStage.RECONNECTING, + fw.UploadStage.COMPLETED, + ): stage = fw.next_stage(stage, target) assert stage is fw.UploadStage.COMPLETED @@ -253,52 +349,52 @@ def __init__(self, board_id: int = 9, flash_size: int = 2048) -> None: self.rebooted = False def exchange(self, request: bytes) -> bytes: # noqa: PLR0911 - assert request.endswith(fw.EOC) + assert request.endswith(bootloader.EOC) cmd, body = request[0:1], request[1:-1] - if cmd == fw.GET_SYNC: - return fw.INSYNC + fw.OK - if cmd == fw.GET_DEVICE: + if cmd == bootloader.GET_SYNC: + return bootloader.INSYNC + bootloader.OK + if cmd == bootloader.GET_DEVICE: values = { - fw.INFO_BL_REV: 5, - fw.INFO_BOARD_ID: self.board_id, - fw.INFO_BOARD_REV: 0, - fw.INFO_FLASH_SIZE: self.flash_size, - fw.INFO_EXTF_SIZE: 0, + bootloader.INFO_BL_REV: 5, + bootloader.INFO_BOARD_ID: self.board_id, + bootloader.INFO_BOARD_REV: 0, + bootloader.INFO_FLASH_SIZE: self.flash_size, + bootloader.INFO_EXTF_SIZE: 0, } - return struct.pack(" fw.BootloaderInfo: - fw.decode_sync(bl.exchange(fw.encode_get_sync())) + bootloader.decode_sync(bl.exchange(bootloader.encode_get_sync())) def info(param: bytes) -> int: - reply = bl.exchange(fw.encode_get_device(param)) - fw.decode_sync(reply[4:]) - return fw.decode_uint32(reply) + reply = bl.exchange(bootloader.encode_get_device(param)) + bootloader.decode_sync(reply[4:]) + return bootloader.decode_uint32(reply[:4]) return fw.BootloaderInfo( - info(fw.INFO_BL_REV), - info(fw.INFO_BOARD_ID), - info(fw.INFO_BOARD_REV), - info(fw.INFO_FLASH_SIZE), - info(fw.INFO_EXTF_SIZE), + info(bootloader.INFO_BL_REV), + info(bootloader.INFO_BOARD_ID), + info(bootloader.INFO_BOARD_REV), + info(bootloader.INFO_FLASH_SIZE), + info(bootloader.INFO_EXTF_SIZE), ) @@ -313,56 +409,58 @@ def test_full_upload_sequence_verifies_against_fake_bootloader(self, tmp_path: P WHEN: identify, erase, program every chunk, then GET_CRC THEN: the bootloader CRC equals the image CRC and the reboot is acknowledged """ - image = fw.load_apj(write_apj(tmp_path, small_image)) + image = bootloader.load_apj(write_apj(tmp_path, small_image)) bl = FakeBootloader() info = identify(bl) fw.check_compatibility(image, info) - fw.decode_sync(bl.exchange(fw.encode_chip_erase())) - chunks = fw.program_chunks(image.image) - for chunk in chunks: - fw.decode_sync(bl.exchange(fw.encode_prog_multi(chunk))) - crc_reply = bl.exchange(fw.encode_get_crc()) - fw.decode_sync(crc_reply[4:]) - fw.decode_sync(bl.exchange(fw.encode_reboot())) + bootloader.decode_sync(bl.exchange(bootloader.encode_chip_erase())) + chunk_count = 0 + for chunk in bootloader.program_chunks(image.image): + bootloader.decode_sync(bl.exchange(bootloader.encode_prog_multi(chunk))) + chunk_count += 1 + crc_reply = bl.exchange(bootloader.encode_get_crc()) + bootloader.decode_sync(crc_reply[4:]) + bootloader.decode_sync(bl.exchange(bootloader.encode_reboot())) assert info == fw.BootloaderInfo(5, 9, 0, 2048, 0) - assert len(chunks) == -(-len(image.image) // fw.PROG_MULTI_MAX) + assert chunk_count == -(-len(image.image) // bootloader.PROG_MULTI_MAX) assert bl.flash == image.image - assert fw.decode_uint32(crc_reply) == image.crc(info.flash_size) + assert bootloader.decode_uint32(crc_reply[:4]) == image.crc(info.flash_size) assert bl.rebooted def test_programming_before_erase_surfaces_as_a_protocol_error(self, tmp_path: Path, small_image: bytes) -> None: - image = fw.load_apj(write_apj(tmp_path, small_image)) + image = bootloader.load_apj(write_apj(tmp_path, small_image)) bl = FakeBootloader() + first_chunk = next(bootloader.program_chunks(image.image)) with pytest.raises(fw.BootloaderProtocolError, match="OPERATION FAILED"): - fw.decode_sync(bl.exchange(fw.encode_prog_multi(fw.program_chunks(image.image)[0]))) + bootloader.decode_sync(bl.exchange(bootloader.encode_prog_multi(first_chunk))) @pytest.mark.parametrize( ("reply", "reason"), [ (b"", "short reply"), (b"\x00\x10", "expected INSYNC"), - (fw.INSYNC + fw.INVALID, "INVALID"), - (fw.INSYNC + fw.BAD_SILICON_REV, "silicon"), - (fw.INSYNC + b"\x7f", "unexpected status"), + (bootloader.INSYNC + bootloader.INVALID, "INVALID"), + (bootloader.INSYNC + bootloader.BAD_SILICON_REV, "silicon"), + (bootloader.INSYNC + b"\x7f", "unexpected status"), ], ) def test_bad_sync_replies_are_typed_errors(self, reply: bytes, reason: str) -> None: with pytest.raises(fw.BootloaderProtocolError, match=reason): - fw.decode_sync(reply) + bootloader.decode_sync(reply) - @pytest.mark.parametrize("length", [0, 3, fw.PROG_MULTI_MAX + 4]) + @pytest.mark.parametrize("length", [0, 3, bootloader.PROG_MULTI_MAX + 4]) def test_prog_multi_refuses_chunks_the_bootloader_would_reject(self, length: int) -> None: with pytest.raises(ValueError, match="PROG_MULTI"): - fw.encode_prog_multi(b"\x00" * length) + bootloader.encode_prog_multi(b"\x00" * length) def test_read_multi_encodes_length_byte(self) -> None: - assert fw.encode_read_multi(252) == fw.READ_MULTI + b"\xfc" + fw.EOC + assert bootloader.encode_read_multi(252) == bootloader.READ_MULTI + b"\xfc" + bootloader.EOC with pytest.raises(ValueError, match="READ_MULTI"): - fw.encode_read_multi(253) + bootloader.encode_read_multi(253) def test_short_uint32_reply_is_a_protocol_error(self) -> None: with pytest.raises(fw.BootloaderProtocolError, match="4 bytes"): - fw.decode_uint32(b"\x01\x02") + bootloader.decode_uint32(b"\x01\x02") From 07dbbc3db648b0144f1243afc736028eed3b6c3f Mon Sep 17 00:00:00 2001 From: Billard <82095453+iacker@users.noreply.github.com> Date: Wed, 9 Sep 2026 00:59:58 +0200 Subject: [PATCH 3/4] fix(firmware): retry reconnect while connect() reports an error connect() signals failure with a non-empty string, not an exception, so the reconnect loop returned after one attempt whenever the device resolved at once. Also drop the unread SerialDeviceIdentity.interface field and the redundant mid-loop deadline check in _read_exact. Signed-off-by: Billard <82095453+iacker@users.noreply.github.com> --- .../backend_flightcontroller.py | 16 +++- .../backend_flightcontroller_bootloader.py | 7 -- ...est_backend_flightcontroller_bootloader.py | 92 +++++++++++++++++++ tests/test_bootloader_identity.py | 2 +- 4 files changed, 105 insertions(+), 12 deletions(-) diff --git a/ardupilot_methodic_configurator/backend_flightcontroller.py b/ardupilot_methodic_configurator/backend_flightcontroller.py index 21a163835..9993824a2 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller.py @@ -422,14 +422,22 @@ def enter_bootloader() -> None: def reconnect_after_bootloader() -> str: active_baudrate = getattr(self._connection_manager, "active_baudrate", self.baudrate) deadline = time_monotonic() + FIRMWARE_RECONNECT_RESOLVE_TIMEOUT - last_error: OSError | None = None + last_error: str = "" 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: # noqa: PERF203 - last_error = exc + except OSError as exc: + last_error = str(exc) time_sleep(0.1) + continue + # connect() reports failure by returning a non-empty error string, not by + # raising, so a stable persistent_path that resolves immediately must still + # spend the retry budget until the re-enumerated board answers. + connect_error = self.connect(reconnect_device, log_errors=False, baudrate=active_baudrate) + if not connect_error: + return "" + last_error = connect_error + time_sleep(0.1) return _("cannot resolve the flight-controller serial device: {error}").format(error=last_error) backend_kwargs: dict[str, Any] = { diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py b/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py index 66a85da25..7990da846 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py @@ -105,7 +105,6 @@ class SerialDeviceIdentity: location: str = "" serial_number: str = "" - interface: str = "" persistent_path: str = "" @@ -134,7 +133,6 @@ def capture_serial_device_identity(device: str) -> SerialDeviceIdentity | None: identity = SerialDeviceIdentity( location=str(getattr(port, "location", "") or ""), serial_number=str(getattr(port, "serial_number", "") or ""), - interface=str(getattr(port, "interface", "") or ""), persistent_path=_capture_linux_persistent_path(device), ) return identity if identity.location or identity.serial_number or identity.persistent_path else None @@ -385,11 +383,6 @@ def _read_exact(self, size: int, *, timeout: float | None = None, deadline: floa ) raise BootloaderProtocolError(msg) received.extend(chunk) - if len(received) < size and self._clock() >= read_deadline: - msg = _("timeout waiting for {expected} bootloader bytes; received {actual}").format( - expected=size, actual=len(received) - ) - raise BootloaderProtocolError(msg) except (OSError, serial.SerialException) as exc: msg = _("bootloader transport read failed: {error}").format(error=exc) raise BootloaderProtocolError(msg) from exc diff --git a/tests/test_backend_flightcontroller_bootloader.py b/tests/test_backend_flightcontroller_bootloader.py index aee6af40c..c7bf88d4e 100755 --- a/tests/test_backend_flightcontroller_bootloader.py +++ b/tests/test_backend_flightcontroller_bootloader.py @@ -437,6 +437,32 @@ def test_reader_rejects_oversized_apj_descriptor_before_reading(tmp_path: Path, bl.load_apj(path) +def test_load_apj_rejects_non_apj_suffix(tmp_path: Path) -> None: + path = tmp_path / "firmware.bin" + path.write_bytes(b"whatever") + + with pytest.raises(fw.FirmwareFileError, match="unsupported firmware format"): + bl.load_apj(path) + + +def test_encode_extf_erase_rejects_out_of_range_size() -> None: + with pytest.raises(ValueError, match="external flash size"): + bl.encode_extf_erase(0) + with pytest.raises(ValueError, match="external flash size"): + bl.encode_extf_erase(0x1_0000_0000) + + +def test_bootloader_client_rejects_non_positive_timeout() -> None: + with pytest.raises(ValueError, match="timeout must be positive"): + bl.BootloaderClient(FakeBootloaderTransport(), timeout=0) + + +def test_identify_rejects_unsupported_protocol_revision() -> None: + transport = FakeBootloaderTransport(revision=1) + with pytest.raises(fw.BootloaderProtocolError, match="unsupported bootloader protocol revision"): + bl.BootloaderClient(transport).identify() + + def test_bootloader_port_is_re_resolved_by_usb_identity_and_ambiguity_is_refused( monkeypatch: pytest.MonkeyPatch, ) -> None: @@ -687,6 +713,51 @@ def resolve(_device: str, _identity: object) -> str: assert connection.reconnect_device == "COM13" +def test_facade_retries_reconnect_when_connect_returns_an_error(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """ + Reconnection keeps trying while connect() reports a transient failure. + + GIVEN: resolve_bootloader_device succeeds at once but the board is not open yet + WHEN: connect() returns an error string on the first attempts then an empty string + THEN: The facade spends the retry budget and reports success once connect() succeeds + """ + connect_results = iter(["port not ready", "port not ready", ""]) + + class _FlakyConnection(_Connection): + def connect(self, device: str, **kwargs: object) -> str: + self.reconnect_device = device + self.comport_device = device + self.master = _Master() + error = next(connect_results) + if not error: + self.reconnected = True + return error + + connection = _FlakyConnection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", + lambda _device, _identity: "COM13", + ) + + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + assert connection.reconnected + assert connection.reconnect_device == "COM13" + + def test_facade_reports_rejected_bootloader_entry_without_releasing_serial(tmp_path: Path) -> None: """ A rejected bootloader command stops the upload before disconnecting MAVLink. @@ -717,6 +788,27 @@ def test_facade_reports_rejected_bootloader_entry_without_releasing_serial(tmp_p assert connection.master is not None +def test_facade_requires_trusted_sha_before_flashing(tmp_path: Path) -> None: + connection = _Connection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareConfirmationError, match="trusted SHA-256"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=None, + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + assert not connection.disconnected + + def test_facade_refuses_network_mavlink_connection() -> None: connection = _Connection("udp:127.0.0.1:14550") controller = FlightController( diff --git a/tests/test_bootloader_identity.py b/tests/test_bootloader_identity.py index 7607c700b..7f962b1fa 100644 --- a/tests/test_bootloader_identity.py +++ b/tests/test_bootloader_identity.py @@ -47,7 +47,7 @@ def test_bootloader_port_refuses_an_ambiguous_macos_dual_cdc_match(monkeypatch: console_port.interface = "2" monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [mavlink_port, console_port]) - identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123", interface="0") + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123") with pytest.raises(OSError, match="cannot uniquely"): bl.resolve_bootloader_device("/dev/cu.usbmodem14101", identity) From bbdcd2880cc2ad3760c51f33c9a68d88f1771928 Mon Sep 17 00:00:00 2001 From: "Dr.-Ing. Amilcar do Carmo Lucas" Date: Wed, 9 Sep 2026 09:34:33 +0200 Subject: [PATCH 4/4] fix(firmware-upload): clarify uncertain bootloader reboot recovery MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bootloader identification failure path can fail to confirm the reboot without proving that the reboot itself failed. Report this uncertainty explicitly and instruct the user to power-cycle the flight controller when the held bootloader cannot be confirmed to have rebooted. Keep the more definitive “cannot reboot” diagnostics for paths where the reboot command itself is known to have failed. Also restore the required pylint suppression for FlightController.upload_apj_firmware() and clarify the architecture documentation’s description of its collaborators. Extend the regression coverage to verify: - injected facade sleep intervals; - per-attempt bootloader identification deadlines; - the bounded abort/reboot budget; - preservation of the normal client timeout after a clamped discovery attempt; - exact retry counts and timeout values; and - preservation of OSError recovery details. --- ARCHITECTURE_firmware_upload.md | 492 ++----- .../backend_flightcontroller.py | 169 ++- .../backend_flightcontroller_bootloader.py | 321 ++++- .../data_model_firmware_upload.py | 65 +- pyproject.toml | 1 + tests/test_backend_flightcontroller.py | 18 +- ...est_backend_flightcontroller_bootloader.py | 1266 ++++++++++++++++- tests/test_bootloader_identity.py | 129 ++ tests/test_bootloader_partial_read.py | 36 +- tests/test_data_model_firmware_upload.py | 33 +- 10 files changed, 1906 insertions(+), 624 deletions(-) diff --git a/ARCHITECTURE_firmware_upload.md b/ARCHITECTURE_firmware_upload.md index 318d6e051..6e23167e1 100644 --- a/ARCHITECTURE_firmware_upload.md +++ b/ARCHITECTURE_firmware_upload.md @@ -1,382 +1,126 @@ # Firmware Upload Architecture -## Overview +Firmware upload flashes an ArduPilot APJ image through the board bootloader and +reconnects to the flight controller. It is separate from parameter configuration +because entering the bootloader interrupts the active MAVLink connection. -This feature allows a user to select an ArduPilot firmware image, validate it against -the connected flight controller, flash it through the board bootloader, and reconnect -to verify the result. - -The feature follows the project separation described in [`ARCHITECTURE.md`](ARCHITECTURE.md): - -- `backend_flightcontroller.py` provides the `FlightController` facade. It owns the - active MAVLink connection, bootloader entry/reboot command, and reconnection. -- `backend_flightcontroller_bootloader.py` is the bootloader I/O adapter. It owns the - serial port, ArduPilot bootloader protocol, firmware file reading, and progress events. -- `data_model_firmware_upload.py` is the business/domain model. It owns firmware - metadata, board compatibility, validation, workflow state, and user-facing error - classifications. It does not open files, access serial ports, use Tkinter, or talk - directly to the flight controller. -- `frontend_firmware_upload.py` is the GUI. It owns file selection, confirmation, - progress presentation, cancellation, and translated user messages. It delegates - validation and upload operations to the model and backend. - -The operation is deliberately separate from parameter configuration. Firmware flashing -may destroy the running connection and must not be performed as an implicit part of a -parameter upload or configuration-step transition. - -## External protocol references - -The implementation should be based on the maintained ArduPilot uploader and pymavlink -interfaces, rather than inventing a second bootloader protocol: - -- [ArduPilot `Tools/scripts/uploader.py`](https://github.com/ArduPilot/ardupilot/blob/master/Tools/scripts/uploader.py) - defines the PX4/ArduPilot serial bootloader protocol, APJ image format, board-ID - validation, erase/program/verify sequence, and reboot handling. -- [ArduPilot bootloader documentation](https://ardupilot.org/dev/docs/bootloader.html) - documents the supported serial flashing workflow and baud rates. -- [pymavlink](https://github.com/ArduPilot/pymavlink) supplies MAVLink connection, - message packing, and serial abstractions for entering the bootloader and detecting - the flight controller before and after flashing. - -Firmware programming itself is not a normal MAVLink file transfer. MAVLink is used for -the reboot/bootloader-entry handshake, including a `COMMAND_ACK` check; a rejected -or armed vehicle is reported before the active MAVLink connection is released. Once the bootloader responds, the -bootloader serial protocol performs synchronization, erase, program, read-back verify, -and reboot. - -## Component architecture +## Responsibilities ```mermaid flowchart TD - GUI[frontend_firmware_upload.py\nTkinter workflow] --> FACADE[backend_flightcontroller.py\nFlightController facade] - FACADE --> BACKEND[backend_flightcontroller_bootloader.py\nbootloader I/O adapter] - BACKEND --> MODEL[data_model_firmware_upload.py\nvalidation and state] - FACADE --> MAV[pymavlink\nMAVLink handshake/reconnect] - BACKEND --> SERIAL[pyserial\nbootloader protocol] - BACKEND --> FILE[Local firmware file\nAPJ input] - BACKEND --> FC[ArduPilot bootloader] + CALLER[Upload caller] --> FACADE[backend_flightcontroller.py] + FACADE --> MAV[MAVLink connection] + FACADE --> BOOT[backend_flightcontroller_bootloader.py] + BOOT --> MODEL[data_model_firmware_upload.py] + BOOT --> SERIAL[pyserial] + BOOT --> FC[ArduPilot bootloader] ``` -The existing `FlightController` facade and protocol definitions should expose the -feature through delegation, following the connection/params/commands/files managers. -The connection manager remains the source of truth for the active MAVLink connection; -the upload adapter may temporarily close and recreate that connection but must not -maintain a competing long-lived connection state. After reboot, the facade resolves -the captured stable serial identity before reconnecting so a changed serial device -path cannot silently target the old port. When USB metadata is unavailable on Linux, -it captures the resolved `/dev/serial/by-path` link before reboot and reopens that exact -link after re-enumeration. - -## Requirements - -### Functional requirements - -1. Allow the user to choose a local ArduPilot firmware image. -2. Parse firmware metadata before opening or resetting the flight controller. -3. Validate the image format, image size, board ID, and supported bootloader protocol - range before programming. Board revision is retained for display; no authoritative - cross-board revision-compatibility map exists yet. -4. Require an explicit confirmation at the lowest write-capable API immediately before - an irreversible erase/write, and a trusted SHA-256 of the exact APJ descriptor bytes - before bootloader entry. The digest must come from an independently trusted manifest - or release record, not from APJ metadata. -5. Enter the bootloader using the existing MAVLink connection when supported, with a - documented unplug/replug fallback for boards that do not respond. -6. Discover and open the bootloader serial port at the configured baud rate. -7. Report synchronization, identification, erase, program, verify, and reboot stages. -8. Support cancellation only at safe protocol boundaries; never interrupt a write in - the middle of a bootloader packet. -9. Verify the programmed image before rebooting whenever the bootloader supports it. -10. On every pre-erase cancellation, rejection, or failure after opening the - bootloader, best-effort reboot it, close its port, and reconnect to the original - flight controller. If a held bootloader cannot be opened, report that a power cycle - is required; MAVLink reconnection is not attempted because it cannot succeed. -11. Close the bootloader port and reconnect to the flight controller after reboot. -12. Require post-flash `AUTOPILOT_VERSION` identity: the APJ board ID must be present - and match. APJ `version` is retained for display only because it is not the MAVLink - firmware-version field. -13. Return actionable, translated errors without exposing raw tracebacks in the GUI. - -### Safety requirements - -- Never erase or program before image and board compatibility checks pass. -- Refuse every board-ID mismatch. Force flashing is intentionally unsupported until a - separately designed, explicitly authorized, and auditable operation model exists. -- Require a valid APJ board ID from MAVLink before bootloader entry and bind the - bootloader board ID to it. Upload without a pre-reboot board identity is refused. -- Bind the serial target to physical hardware across bootloader re-enumeration. The - active port's USB location or serial number is resolved first on every platform; - every populated physical-device attribute must match the same port. A captured - interface value is not used as a cross-mode tie-break: macOS can report the USB - device parent's location for both CDC interfaces, and the application and - bootloader interface numbers may differ. Ambiguous matches therefore fail closed. - Linux `/dev/serial/by-path` is the fallback when no USB identity is available. - Resolution fails closed rather than opening a stale port, and upload is refused - when neither a USB identity nor a Linux by-path binding is available. A Linux - by-path fallback is captured before reboot and the same persistent link is used - for reconnection; it is never resolved afresh after the board re-enumerates. -- Require a trusted SHA-256 supplied by the caller for the exact APJ descriptor bytes, - binding the payload and board metadata together. The backend computes and retains a - separate payload digest for display, but never treats a self-reported APJ field as - trust evidence. The caller must obtain that digest from an authenticated release - catalogue which publishes per-APJ checksums; an APJ manifest's `git_sha` is not a - substitute. -- Do not flash over network MAVLink connections; require a directly addressable serial - device for the bootloader transport unless a board-specific transport is added. -- Do not treat a lost connection as success. -- Preserve the selected firmware path and metadata in memory only; do not modify the - source image. -- Ensure every serial port is closed in success, cancellation, and exception paths. -- Use the descriptor's unpadded external-image size for external-flash erase and CRC, - matching ArduPilot's reference uploader; writes remain 4-byte padded as required by - `PROG_MULTI`. -- Treat external-flash erase responses below 90% as monotonic percentage bytes, even - when a progress value equals `INSYNC` (decimal 18); accept `INSYNC/OK` only after - the progress threshold is reached. Refresh the timeout only when progress advances - so a stalled erase cannot wait indefinitely. -- Keep v2 read-back verification bounded by `READ_MULTI_MAX` independently of the - programming chunk size. -- Keep the UI responsive by running the blocking flash operation outside Tkinter's - event loop and marshal progress updates back to the GUI thread. - -## `backend_flightcontroller_bootloader.py` - -### Responsibilities - -This module owns bootloader serial and local-file I/O. It exposes small protocols so -real hardware and deterministic test doubles can be substituted. The existing -`backend_flightcontroller.py` facade owns MAVLink bootloader entry and reconnection. - -Planned interfaces: - -```python -class FirmwareUploadBackendProtocol(Protocol): - def inspect_firmware(self, firmware_path: Path) -> FirmwareImageMetadata: ... - def enter_bootloader(self, connection: MavlinkConnection, timeout: float) -> None: ... - def identify_bootloader(self, port: str, baudrate: int) -> BootloaderInfo: ... - def upload( - self, - image: FirmwareImage, - *, - confirmation_requested: Callable[[FirmwareImage, BootloaderInfo], bool], - progress_callback: ProgressCallback | None = None, - cancellation_requested: Callable[[], bool] | None = None, - ) -> None: ... - def reconnect_and_verify(self, device: str, baudrate: int, timeout: float) -> FirmwareIdentity: ... -``` - -The concrete adapter should contain these private stages: - -1. Read and decode APJ JSON, base64, and zlib data using bounded file I/O. -2. Normalize/pad the image exactly as the ArduPilot uploader does. -3. Open the serial port with a controlled timeout and restore the original settings - on failure where possible. Every bounded read must enforce its deadline after - partial as well as empty responses. Resolve the port again before every open attempt - using the captured USB location or serial number, or reopen the captured Linux - by-path link exactly, and reject zero or multiple USB matches. -4. Synchronize with the bootloader and query protocol revision, board ID, board - revision, internal flash size, and external flash size. If an old bootloader - rejects the optional external-size query, clear stale input before fallback - synchronization and report no external flash. -5. Reject unsupported protocol revisions and images that exceed available flash. -6. Erase only the required flash regions. -7. Program in protocol-sized chunks with ACK/INSYNC checking after each chunk. -8. Verify by read-back or the bootloader's supported CRC mechanism. -9. Reboot and close the port. -10. Reboot and close the port on cancellation or any upload failure before erase so the original - firmware can run again. Identification retries close and reopen the still-held - bootloader; only the final failed attempt attempts a recovery reboot. If that - recovery reboot fails, return a typed recovery error and require a power cycle - rather than reporting a normal reconnect. - -The adapter should reuse the uploader's constants and algorithm through a maintained -internal implementation or a clearly isolated vendored adapter. It should not invoke -MAVProxy as a subprocess: subprocess output, cancellation, platform behavior, and -error handling would be difficult to make reliable inside the application. - -### Firmware formats - -- APJ is the initial required format because it contains the image, `image_size`, and - `board_id` metadata required for safe bootloader validation. External-only APJs may - use an empty internal image when they provide a valid external image. -- Raw BIN support must not guess a board ID or flash offset. It may be added only when - the source metadata explicitly supplies the target board and layout, or when a - board-specific mapping is maintained by the application. Otherwise the backend must - reject BIN with a clear explanation and direct the user to an APJ image. -- Unsupported, malformed, compressed, oversized, or non-finite metadata must produce a - typed validation error before any serial I/O begins. - -### Connection integration - -Add a firmware-upload manager/protocol to the existing `FlightController` facade. It -should use the connection manager for: - -- current device, stable USB identity, and baud rate; -- the board ID reported by the currently connected firmware; absence or malformed - identity blocks upload before any reboot command; -- MAVLink reboot/bootloader-entry; -- disconnecting before serial bootloader access; and -- reconnecting and refreshing `FlightControllerInfo` after flashing. - -The manager must invalidate stale parameter and command state after a successful flash; -the normal application flow should require a fresh parameter download before allowing -configuration edits to continue. - -## `data_model_firmware_upload.py` - -### Domain objects - -Define plain, immutable-or-controlled-state objects such as: - -- `FirmwareImageMetadata`: path, format, image size, board ID, board revision, - firmware type/version, external flash size, and a SHA-256 over the exact unpadded - image regions. -- `BootloaderInfo`: protocol revision, board ID/revision, internal and external flash - capacity, and bootloader identity. -- `FirmwareCompatibility`: compatible/incompatible/unknown plus a reason and severity. -- `FirmwareUploadProgress`: stage, completed units, total units, and display text key. -- `FirmwareUploadResult`: success, cancelled, verified, reconnect status, and typed - failure information. - -### Business rules - -The model should: - -1. Validate file extension and metadata invariants. -2. Compare the image board ID against detected bootloader information and compare the - bootloader board ID with the required pre-reboot MAVLink board ID. Board revision - remains informational until an authoritative compatibility map is added. -3. Check image and external-image sizes against flash capacities. -4. Require confirmation at the write-capable boundary; force flashing is not permitted. -5. Define the state machine: - - `idle → inspecting → entering_bootloader → identifying → awaiting_confirmation → - erasing → programming → verifying → rebooting → reconnecting → completed` - - with `failed` and `cancelled` exits from every safe boundary. -6. Translate backend exceptions into stable domain error categories for the frontend. -7. Keep policy independent from timing, serial reads, Tkinter, and gettext dialogs. - -The model should not determine compatibility from a firmware filename alone. Firmware -metadata and bootloader identification are authoritative; filenames are display-only. - -## `frontend_firmware_upload.py` - -### Frontend responsibilities - -Provide a modal or dedicated firmware-upload window consistent with the existing -Tkinter frontend conventions: - -- firmware file picker with an APJ filter and an all-files fallback; -- metadata and detected-board summary; -- explicit mismatch warning; -- confirmation before erase/program; -- stage-specific progress bar and status text; -- cancel button with safe-boundary semantics; -- error dialog with recovery guidance; and -- completion view showing verification and reconnection status. - -The frontend must not parse APJ files, compare board IDs, or call serial/MAVLink APIs -directly. It should call the model and backend through injected protocols and use the -existing progress-window/UI-service patterns where applicable. - -The window must disable conflicting connection, parameter, and project-navigation -actions while flashing. Closing the window during an active operation should request -cancellation, not destroy the worker state or force-close the serial port. - -## Integration workflow - -1. The application opens the firmware-upload entry point only when a directly usable - serial connection is available. -2. The frontend selects a firmware file and asks the backend to inspect it. -3. The model validates metadata, the trusted release digest, and the current FC identity. -4. The backend requests bootloader mode, disconnects the MAVLink parser, and identifies - the bootloader. -5. The model performs the final board/capacity and pre-reboot-target checks. -6. The frontend asks for the final erase/program confirmation after identification. -7. A declined confirmation, cancellation, incompatibility, or pre-erase error sends a - best-effort bootloader reboot and reconnects the original MAVLink connection. -8. The backend erases, programs, and verifies while emitting throttled progress events. -9. The backend reboots, closes the bootloader port, and reconnects through the normal - connection manager. -10. The model requires the new `AUTOPILOT_VERSION` board identity before returning - success; the APJ's display version is not compared with MAVLink firmware version. -11. The frontend reports success and asks the user to re-download parameters before - resuming configuration. - -## Error handling - -Use typed errors internally and map them to translated messages at the frontend: - -- invalid or unreadable firmware file; -- unsupported firmware format; -- board-ID mismatch or a bootloader that differs from the pre-reboot target; -- unsupported bootloader protocol; -- bootloader not found or synchronization timeout; -- serial permission or port ownership failure; -- erase/program/verify failure; -- cancellation; -- reboot/reconnect timeout; and -- post-flash firmware identity mismatch. -- missing or ambiguous physical serial-device identity; and -- trusted firmware-digest missing or mismatch. - -Errors must include the failed stage and recovery advice. A failed verify is never -reported as a successful upload, even if the bootloader rebooted. - -## Testing strategy - -### Backend tests - -- APJ parsing, decompression, padding, metadata, and trusted-digest handling. -- Malformed JSON/base64/zlib and oversized-image rejection without serial access. -- Bootloader packet encoding/decoding and INSYNC/OK/error handling. -- Board-ID and flash-capacity validation, mandatory pre-reboot target binding, physical - serial-device re-enumeration/ambiguity handling, and the documented board-revision - informational policy. -- Erase/program/verify ordering using a fake serial port. -- Chunk boundaries, short reads/writes, timeouts, retries, and cancellation boundaries. -- Port cleanup on every failure path. -- MAVLink bootloader-entry and reconnect behavior with fake connections, including - callback/disconnect exceptions before erase and mandatory post-reconnect identity. - -### Data-model tests - -- Compatibility decisions for matching, mismatching, and unknown metadata. -- State-machine transitions and illegal-transition rejection. -- Error classification and progress aggregation. -- Mandatory confirmation requirements and force-flash rejection. - -### Frontend tests - -- File selection and metadata display. -- Confirmation on compatible and incompatible targets. -- Progress updates without direct backend access from widgets. -- Cancellation and window-close behavior. -- Error and completion dialogs. -- Controls disabled while flashing and restored after completion/failure. - -### Integration and acceptance tests - -Use a fake bootloader and fake MAVLink connection to exercise the complete workflow -without hardware. Hardware validation should be a separately marked test suite and -must require an explicit serial device selection. - -## Implementation sequence - -1. Add domain types and validation rules in `data_model_firmware_upload.py`. -2. Add the fake-serial bootloader protocol and APJ reader tests. -3. Implement the real backend adapter using the ArduPilot uploader algorithm. -4. Add facade/protocol delegation and connection lifecycle integration. -5. Build the frontend window and worker/progress orchestration. -6. Add end-to-end fake-device tests and update user/architecture documentation. -7. Run ruff, type checks, focused tests, and the complete non-GUI test suite. -8. Perform a hardware test on a supported board with a known-good recovery path. - -## Non-goals for the first implementation - -- Bootloader flashing over UDP/TCP or arbitrary MAVLink proxies. -- Bootloader updates, unless separately designed and confirmed by the user. -- Guessing raw BIN board IDs, offsets, or target hardware. -- Automatic firmware downloads from the internet. -- Force flashing or automatic retry after an erase failure. +- `backend_flightcontroller.py` owns `upload_apj_firmware()`, the active MAVLink + connection, bootloader entry, disconnection, reconnection, and post-flash identity. +- `backend_flightcontroller_bootloader.py` owns APJ file I/O, serial discovery, + bootloader protocol traffic, progress, retries, and cleanup. +- `data_model_firmware_upload.py` contains APJ parsing, image normalization, + compatibility rules, upload stages, and typed errors. It does not access files, + serial ports, MAVLink, or Tkinter. + +The connection manager remains the source of truth for MAVLink state. The bootloader +adapter may close and recreate that connection, but does not keep a second long-lived +connection state. + +## Upload workflow and safety rules + +1. Require a directly addressable serial connection, a valid connected board ID, + stable USB identity, explicit confirmation, and a trusted SHA-256 for the exact + APJ descriptor. +2. Parse and validate the APJ input before rebooting the controller. APJ is the only + supported input format; raw BIN images are rejected. +3. Enter bootloader mode through MAVLink, release the MAVLink connection, and + rediscover the bootloader serial port after re-enumeration. +4. Identify the bootloader and validate protocol revision, board ID, flash capacity, + and image compatibility. +5. After final confirmation, erase, program, and verify the internal and optional + external image regions, then reboot. +6. Reconnect through the normal connection manager and require the returned board + identity before reporting success. + +The following invariants apply throughout the workflow: + +- Never erase or program before APJ validation, board matching, capacity checks, + trusted-digest verification, and final confirmation pass. +- Reconnection is bound to the captured USB serial number and/or physical location. + Interface metadata is only a disambiguation hint and may differ because the + application and bootloader can expose different CDC interfaces; interface suffixes + such as Linux `1-2.3:1.2` are ignored for physical matching. Ambiguous matches fail + closed. +- Network MAVLink connections and force flashing are unsupported. +- Cancellation is checked at safe boundaries and never interrupts a bootloader + packet. Before erase, the held bootloader is rebooted only after an explicit + successful reboot acknowledgement, except for protocol revision 2 which does + not send one; otherwise the error requires a power cycle. +- Bootloader discovery has a 15-second wall-clock budget. Serial response timeouts + and retry delays are capped by the remaining budget, so a port that opens but + does not answer cannot extend discovery indefinitely. +- After erase begins, any erase, programming, verification, or reboot failure + reports that the flash may be incomplete and instructs the user to power-cycle + before reconnecting. The facade does not attempt a normal MAVLink reconnect + unless a safe bootloader abort has confirmed a reboot. +- Serial transports are closed on successful uploads and handled protocol/error + paths, and cleanup cannot mask the original upload failure. The source APJ is not + modified, and a failed verify or reconnect is never reported as success. +- Full-chip erase is refused because the client cannot yet determine target MCU + capability safely; normal erase remains supported. + +## Implemented modules + +### `backend_flightcontroller_bootloader.py` + +`FlightControllerBootloaderBackend` is an internal transport adapter. It loads the +APJ, enters and discovers the held bootloader, and passes the image to +`BootloaderClient`. The public facade performs trusted-digest and stable-USB-identity +policy checks before invoking it. The client handles protocol synchronization, board +and capacity checks, erase/program/verify, reboot, and transport cleanup; the backend +owns serial-open retries and their wall-clock budget. Discovery resolves the captured +USB identity before each open, fails closed on zero or multiple matches, and shares +injected clock and sleep functions with the client for bounded, testable timing. + +### `data_model_firmware_upload.py` + +The model provides `FirmwareImage`, `BootloaderInfo`, `UploadStage`, APJ parsing, +image padding, CRC helpers, compatibility checks, and typed upload errors. Its +functions are usable without serial or GUI dependencies. + +### `backend_flightcontroller.py` + +`FlightController.upload_apj_firmware()` validates the active connection, trusted +digest, and pre-reboot board identity; delegates bootloader entry and post-reboot +reconnection to dedicated collaborators; and verifies the returned +`AUTOPILOT_VERSION` board ID. A successful flash also invalidates cached parameters. + +## Domain and error model + +`data_model_firmware_upload.py` provides the immutable image and bootloader data +objects, APJ validation, compatibility checks, upload-stage state machine, CRC +helpers, and typed errors. Compatibility comes from APJ metadata and bootloader +identification, never from a filename. + +Errors identify the failed stage and are translated by the caller into user-facing +messages. They cover invalid APJ, digest or identity mismatch, unsupported protocol, +serial discovery, erase/program/verify, cancellation/recovery, and reconnect failure. + +## Testing boundary + +Use pure tests for APJ parsing, image normalization, compatibility, CRCs, state +transitions, and error classification. Use fake serial transports and fake MAVLink +connections for protocol framing, retries, erase/program/verify ordering, USB +re-enumeration, cancellation, cleanup, reconnect, and post-flash identity checks. + +Hardware tests should be separately marked and require an explicitly selected serial +device. + +## References + +- [ArduPilot uploader](https://github.com/ArduPilot/ardupilot/blob/master/Tools/scripts/uploader.py) +- [ArduPilot bootloader documentation](https://ardupilot.org/dev/docs/bootloader.html) +- [pymavlink](https://github.com/ArduPilot/pymavlink) +- [`ARCHITECTURE.md`](ARCHITECTURE.md) diff --git a/ardupilot_methodic_configurator/backend_flightcontroller.py b/ardupilot_methodic_configurator/backend_flightcontroller.py index 9993824a2..0167a3a58 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller.py @@ -28,6 +28,7 @@ ConfirmationRequested, FlightControllerBootloaderBackend, ProgressCallback, + SerialDeviceIdentity, SerialFactory, capture_serial_device_identity, has_stable_bootloader_device_identity, @@ -72,6 +73,77 @@ DEFAULT_REBOOT_TIME: int = 8 FIRMWARE_RECONNECT_RESOLVE_TIMEOUT: float = 15.0 + +class _BootloaderEntryCoordinator: # pylint: disable=too-few-public-methods + """Enter bootloader mode and release the active MAVLink connection.""" + + def __init__( + self, + master_getter: Callable[[], MavlinkConnection | None], + reboot: Callable[[], tuple[bool, str]], + disconnect: Callable[[], None], + sleep: Callable[[float], None], + ) -> None: + self._master_getter = master_getter + self._reboot = reboot + self._disconnect = disconnect + self._sleep = sleep + self.entered = False + + def __call__(self) -> None: + if self._master_getter() is None: + msg = _("flight-controller connection was lost before bootloader entry") + raise FirmwareConnectionError(msg) + accepted, error_message = self._reboot() + if not accepted: + msg = _("flight controller rejected bootloader entry: {error}").format(error=error_message) + raise FirmwareConnectionError(msg) + self.entered = True + self._sleep(0.3) # Allow the MAVLink frame to leave before releasing the port. + self._disconnect() + + +class _BootloaderReconnector: # pylint: disable=too-few-public-methods + """Resolve and reconnect the flight controller after bootloader reboot.""" + + def __init__( # pylint: disable=too-many-arguments,too-many-positional-arguments + self, + device: str, + identity: SerialDeviceIdentity | None, + active_baudrate: Callable[[], int], + resolve_device: Callable[[str, SerialDeviceIdentity | None], str], + connect: Callable[[str, int], str], + clock: Callable[[], float], + sleep: Callable[[float], None], + ) -> None: + self._device = device + self._identity = identity + self._active_baudrate = active_baudrate + self._resolve_device = resolve_device + self._connect = connect + self._clock = clock + self._sleep = sleep + + def __call__(self) -> str: + deadline = self._clock() + FIRMWARE_RECONNECT_RESOLVE_TIMEOUT + last_error = "" + while self._clock() < deadline: + try: + reconnect_device = self._resolve_device(self._device, self._identity) + except OSError as exc: + last_error = str(exc) + self._sleep(0.1) + continue + # ``connect`` reports transient failures as strings rather than + # raising, so keep retrying until the deadline is exhausted. + connect_error = self._connect(reconnect_device, self._active_baudrate()) + if not connect_error: + return "" + last_error = connect_error + self._sleep(0.1) + return _("cannot resolve the flight-controller serial device: {error}").format(error=last_error) + + # Re-export constants for backwards compatibility __all__ = [ "DEFAULT_BAUDRATE", @@ -84,7 +156,7 @@ ] -class FlightController: # pylint: disable=too-many-public-methods +class FlightController: # pylint: disable=too-many-public-methods,too-many-instance-attributes """ Facade for flight controller operations using delegation pattern. @@ -117,7 +189,7 @@ class FlightController: # pylint: disable=too-many-public-methods """ - def __init__( # pylint: disable=too-many-arguments, too-many-positional-arguments + def __init__( # noqa: PLR0913, PLR0917 # pylint: disable=too-many-arguments, too-many-positional-arguments self, reboot_time: int = DEFAULT_REBOOT_TIME, baudrate: int = DEFAULT_BAUDRATE, @@ -128,6 +200,8 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen commands_manager: FlightControllerCommandsProtocol | None = None, files_manager: FlightControllerFilesProtocol | None = None, progress_callback: Callable[[int, int], None] | None = None, + sleep: Callable[[float], None] | None = None, + clock: Callable[[], float] | None = None, ) -> None: """ Initialize the FlightController communication object. @@ -144,6 +218,8 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen progress_callback: Optional callback function for displaying initialization progress. If None, no progress updates are shown. Signature: callback(current, total) Used to provide user feedback during component initialization phases + sleep: Optional delay function used by reset and firmware-upload recovery workflows. + clock: Optional monotonic clock used by firmware-upload reconnection retries. Note: If not provided, managers are created in dependency order: @@ -159,6 +235,8 @@ def __init__( # pylint: disable=too-many-arguments, too-many-positional-argumen progress_callback(5, 100) self._reboot_time = reboot_time + self._sleep = time_sleep if sleep is None else sleep + self._clock = time_monotonic if clock is None else clock self._network_ports = network_ports if network_ports is not None else FlightControllerConnection.DEFAULT_NETWORK_PORTS if progress_callback: @@ -318,7 +396,7 @@ def reset_and_reconnect( # Issue a reset self.master.reboot_autopilot() logging_info(_("Reset command sent to ArduPilot.")) - time_sleep(0.3) # Short delay for command to be sent + self._sleep(0.3) # Short delay for command to be sent self.disconnect() @@ -335,7 +413,7 @@ def reset_and_reconnect( reset_progress_callback(current_step, sleep_time) # Wait for sleep_time seconds - time_sleep(1) + self._sleep(1) current_step += 1 # Call the progress callback with the current progress @@ -361,7 +439,7 @@ def disconnect(self) -> None: # Clear parameter cache via params manager self._params_manager.clear_parameters() - def upload_apj_firmware( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-locals,too-many-statements + def upload_apj_firmware( # pylint: disable=too-many-arguments,too-many-locals self, path: Path, *, @@ -398,52 +476,27 @@ def upload_apj_firmware( # noqa: PLR0915 # pylint: disable=too-many-arguments,t msg = _("firmware upload requires the connected flight controller APJ board_id") raise FirmwareConnectionError(msg) from None device_identity = capture_serial_device_identity(device) - entered_bootloader = False - - def report_progress(stage: UploadStage, completed: int, total: int) -> None: - """Keep presentation failures from interrupting the upload workflow.""" - report_progress_safely(progress_callback, stage, completed, total) - - def enter_bootloader() -> None: - nonlocal entered_bootloader - if self.master is None: - msg = _("flight-controller connection was lost before bootloader entry") - raise FirmwareConnectionError(msg) - accepted, error_message = self._commands_manager.reboot_to_bootloader() - if not accepted: - msg = _("flight controller rejected bootloader entry: {error}").format(error=error_message) - raise FirmwareConnectionError(msg) - # From this point on a failed callback, disconnect, or transport - # setup must attempt recovery before returning control to the caller. - entered_bootloader = True - time_sleep(0.3) # Allow the MAVLink frame to leave before releasing the port. - self.disconnect() - - def reconnect_after_bootloader() -> str: - active_baudrate = getattr(self._connection_manager, "active_baudrate", self.baudrate) - deadline = time_monotonic() + FIRMWARE_RECONNECT_RESOLVE_TIMEOUT - last_error: str = "" - while time_monotonic() < deadline: - try: - reconnect_device = resolve_bootloader_device(device, device_identity) - except OSError as exc: - last_error = str(exc) - time_sleep(0.1) - continue - # connect() reports failure by returning a non-empty error string, not by - # raising, so a stable persistent_path that resolves immediately must still - # spend the retry budget until the re-enumerated board answers. - connect_error = self.connect(reconnect_device, log_errors=False, baudrate=active_baudrate) - if not connect_error: - return "" - last_error = connect_error - time_sleep(0.1) - return _("cannot resolve the flight-controller serial device: {error}").format(error=last_error) + entry = _BootloaderEntryCoordinator( + lambda: self.master, + self._commands_manager.reboot_to_bootloader, + self.disconnect, + self._sleep, + ) + reconnector = _BootloaderReconnector( + device, + device_identity, + lambda: getattr(self._connection_manager, "active_baudrate", self.baudrate), + resolve_bootloader_device, + lambda reconnect_device, baudrate: self.connect(reconnect_device, log_errors=False, baudrate=baudrate), + self._clock, + self._sleep, + ) backend_kwargs: dict[str, Any] = { "timeout": bootloader_timeout, - "enter_bootloader": enter_bootloader, + "enter_bootloader": entry, "device_identity": device_identity, + "sleep": self._sleep, } if serial_factory is not None: backend_kwargs["serial_factory"] = serial_factory @@ -458,16 +511,16 @@ def reconnect_after_bootloader() -> str: raise FirmwareConfirmationError(msg) verify_expected_firmware_digest(image, expected_firmware_sha256) if not has_stable_bootloader_device_identity(device, device_identity): - msg = _("firmware upload requires a stable USB serial number, USB location, or Linux by-path device") + msg = _("firmware upload requires a stable USB serial number or USB location") raise FirmwareConnectionError(msg) - report_progress(UploadStage.ENTERING_BOOTLOADER, 0, 1) + report_progress_safely(progress_callback, UploadStage.ENTERING_BOOTLOADER, 0, 1) try: info = backend.upload( image, full_erase=full_erase, cancellation_requested=cancellation_requested, confirmation_requested=confirmation_requested, - progress_callback=report_progress, + progress_callback=progress_callback, connected_board_id=connected_board_id, ) except Exception as exc: @@ -476,23 +529,27 @@ def reconnect_after_bootloader() -> str: # no bootloader transport could be opened, no in-band reboot is # possible and a MAVLink reconnect cannot succeed. raise - if entered_bootloader and getattr(exc, "bootloader_rebooted", False): - recovery_error = reconnect_after_bootloader() + if entry.entered and getattr(exc, "bootloader_rebooted", False): + recovery_error = reconnector() if recovery_error: logging_warning(_("Unable to reconnect after safe firmware-upload abort: %s"), recovery_error) raise # A flashed image invalidates every cached parameter. The reconnect is a # required part of success, rather than an optimistic best-effort step. self._params_manager.clear_parameters() - report_progress(UploadStage.RECONNECTING, 0, 1) - reconnect_error = reconnect_after_bootloader() + report_progress_safely(progress_callback, UploadStage.RECONNECTING, 0, 1) + reconnect_error = reconnector() if reconnect_error: - raise FirmwareReconnectError(reconnect_error) + msg = _( + "firmware was written and verified, but the flight controller could not be reconnected automatically: " + "{error}. Reconnect it manually before continuing." + ).format(error=reconnect_error) + raise FirmwareReconnectError(msg) verify_reconnected_firmware( image, board_id=self.info.apj_board_id, ) - report_progress(UploadStage.RECONNECTING, 1, 1) + report_progress_safely(progress_callback, UploadStage.RECONNECTING, 1, 1) return info @property diff --git a/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py b/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py index 7990da846..4b0d707c7 100644 --- a/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py +++ b/ardupilot_methodic_configurator/backend_flightcontroller_bootloader.py @@ -15,9 +15,9 @@ import struct from collections.abc import Callable, Iterator +from contextlib import suppress from dataclasses import dataclass from pathlib import Path -from sys import platform as sys_platform from time import monotonic from time import sleep as time_sleep from typing import Protocol, cast @@ -33,6 +33,7 @@ BootloaderInfo, BootloaderProtocolError, FirmwareBootloaderRecoveryError, + FirmwareCompatibilityError, FirmwareConfirmationError, FirmwareFileError, FirmwareImage, @@ -76,6 +77,12 @@ ERASE_TIMEOUT = 20.0 EXTF_CRC_TIMEOUT = 10.0 MAX_APJ_DESCRIPTOR_SIZE = (MAX_ENCODED_BLOB_SIZE * 2) + (1024 * 1024) +BOOTLOADER_ENUMERATION_TIMEOUT = 15.0 +BOOTLOADER_RETRY_DELAY = 0.5 +BOOTLOADER_OPEN_RETRIES = int(BOOTLOADER_ENUMERATION_TIMEOUT / BOOTLOADER_RETRY_DELAY) + 1 +# Recovery is best-effort; do not let an unresponsive held bootloader delay the +# caller for longer than a normal bootloader command timeout. +BOOTLOADER_ABORT_TIMEOUT = 2.0 class BootloaderTransport(Protocol): @@ -99,56 +106,105 @@ def close(self) -> None: ... BootloaderEntry = Callable[[], None] +def _append_recovery_message(exc: Exception, recovery_message: str) -> None: + """ + Append recovery guidance without breaking exception formatting. + + ``OSError`` keeps its rendered message in ``errno``/``strerror`` rather + than deriving it from ``args``. Updating only ``args`` therefore loses + the guidance for errors such as ``OSError(5, "Input/output error")``. + Third-party exceptions may also have no arguments at all. + """ + if isinstance(exc, OSError) and exc.errno is not None: + strerror = exc.strerror or "" + updated = f"{strerror}; {recovery_message}" if strerror else recovery_message + exc.strerror = updated + args = exc.args + if len(args) >= 2: + exc.args = (args[0], updated, *args[2:]) + elif args: + exc.args = (args[0], updated) + else: + exc.args = (exc.errno, updated) + return + + args = exc.args + if args: + exc.args = (f"{args[0]}; {recovery_message}", *args[1:]) + else: + exc.args = (recovery_message,) + + +def _restore_transport_timeout(transport: BootloaderTransport, timeout: float) -> None: + """Restore the normal command timeout after a discovery attempt succeeds.""" + # pyserial exposes both properties; lightweight test/third-party + # transports are not required to, so leave those transports unchanged. + try: + if hasattr(transport, "timeout"): + transport.timeout = timeout # pyright: ignore[reportAttributeAccessIssue] + except (AttributeError, OSError, ValueError): + pass + try: + if hasattr(transport, "write_timeout"): + transport.write_timeout = timeout # pyright: ignore[reportAttributeAccessIssue] + except (AttributeError, OSError, ValueError): + pass + + @dataclass(frozen=True) class SerialDeviceIdentity: """Stable USB attributes which survive a bootloader serial-port rename.""" location: str = "" serial_number: str = "" - persistent_path: str = "" + interface: str = "" DeviceResolver = Callable[[str, SerialDeviceIdentity | None], str] -def _capture_linux_persistent_path(device: str) -> str: - """Return the by-path link currently bound to a serial device, if any.""" - if not sys_platform.startswith("linux"): - return "" - try: - current = Path(device).resolve(strict=True) - for by_path in Path("/dev/serial/by-path").iterdir(): - if by_path.resolve(strict=True) == current: - return str(by_path) - except OSError: - pass - return "" - - def capture_serial_device_identity(device: str) -> SerialDeviceIdentity | None: """Capture stable USB location/serial metadata for the active serial port.""" + try: + resolved_device = Path(device).resolve(strict=True) + except OSError: + resolved_device = None for port in serial.tools.list_ports.comports(): - if port.device != device: + if port.device != device and ( + resolved_device is None or not _serial_port_matches_device(port.device, resolved_device) + ): continue identity = SerialDeviceIdentity( location=str(getattr(port, "location", "") or ""), serial_number=str(getattr(port, "serial_number", "") or ""), - persistent_path=_capture_linux_persistent_path(device), + interface=str(getattr(port, "interface", "") or ""), ) - return identity if identity.location or identity.serial_number or identity.persistent_path else None + return identity if identity.location or identity.serial_number else None return None +def _serial_port_matches_device(port_device: str, resolved_device: Path) -> bool: + """Return whether a listed serial device resolves to the selected device.""" + try: + return Path(port_device).resolve(strict=True) == resolved_device + except OSError: + return False + + def has_stable_bootloader_device_identity(device: str, identity: SerialDeviceIdentity | None) -> bool: """Return whether reopening this serial target can be tied to physical hardware.""" - if identity is not None: - return True - return bool(_capture_linux_persistent_path(device)) + del device # The identity must have been captured before the reboot request. + return identity is not None and bool(identity.location or identity.serial_number) + + +def _physical_usb_location(location: object) -> str: + """Return the USB device location without a platform interface suffix.""" + return str(location or "").split(":", 1)[0] def resolve_bootloader_device(device: str, identity: SerialDeviceIdentity | None = None) -> str: """ - Resolve by stable Linux path or captured USB identity after re-enumeration. + Resolve by captured USB identity after re-enumeration. With a known USB identity this is fail-closed: a stale port is never opened while the board is re-enumerating or the hardware match is ambiguous. @@ -158,27 +214,47 @@ def resolve_bootloader_device(device: str, identity: SerialDeviceIdentity | None # belong to another controller, whereas the location or serial number # identifies the controller that was connected before the reboot. if identity is not None: - if identity.persistent_path: - return identity.persistent_path - matches = [ - port - for port in serial.tools.list_ports.comports() - if all( - not expected or getattr(port, attribute, "") == expected - for attribute, expected in ( - ("location", identity.location), - ("serial_number", identity.serial_number), + if identity.location or identity.serial_number: + matches = [ + port + for port in serial.tools.list_ports.comports() + if all( + not expected + or ( + _physical_usb_location(getattr(port, attribute, "")) == _physical_usb_location(expected) + if attribute == "location" + else getattr(port, attribute, "") == expected + ) + for attribute, expected in ( + ("location", identity.location), + ("serial_number", identity.serial_number), + ) ) - ) - ] - if len(matches) == 1: - return str(matches[0].device) + ] + # Prefer the captured, interface-qualified location when application + # firmware exposes several CDC ports. The bootloader commonly exposes + # only one port with a different suffix, so retain physical matches when + # there is no exact match. + 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 + # An application-port interface identifier need not be reproduced by the + # bootloader. It can only disambiguate ports from one identified device; + # it must never eliminate a sole candidate or select between devices. + # Linux and Windows typically leave this field unset for ArduPilot CDC ports. + if len(matches) > 1 and identity.interface: + locations = {_physical_usb_location(getattr(port, "location", "")) for port in matches} + if len(locations) == 1 and locations != {""}: + narrowed = [port for port in matches if str(getattr(port, "interface", "") or "") == identity.interface] + if len(narrowed) == 1: + matches = narrowed + if len(matches) == 1: + return str(matches[0].device) + msg = _("cannot uniquely locate the bootloader USB device") + raise OSError(msg) msg = _("cannot uniquely locate the bootloader USB device") raise OSError(msg) - if sys_platform.startswith("linux"): - persistent_path = _capture_linux_persistent_path(device) - if persistent_path: - return persistent_path return device @@ -323,6 +399,7 @@ def __init__( *, timeout: float = 2.0, clock: Callable[[], float] = monotonic, + sleep: Callable[[float], None] = time_sleep, ) -> None: if timeout <= 0: msg = _("bootloader response timeout must be positive") @@ -330,14 +407,28 @@ def __init__( self._transport = transport self._timeout = timeout self._clock = clock + self._sleep = sleep def close(self) -> None: - self._transport.close() + # Cleanup must never replace the protocol error which caused the upload + # to fail. pyserial reports close failures as OSError/SerialException, + # while test and third-party transports may use another Exception type. + with suppress(Exception): + self._transport.close() - def abort_before_erase(self) -> bool: - """Best-effort reboot for a declined or failed upload before anything was erased.""" + def abort_before_erase(self, *, deadline: float | None = None, expect_ack: bool = True) -> bool: + """ + Best-effort reboot for a declined or failed upload before anything was erased. + + Protocol revision two reboots without an acknowledgement. Later revisions + must acknowledge the reboot so a failed command is not mistaken for a safe + abort. + """ try: - self._write(encode_reboot()) + if expect_ack: + self._command(encode_reboot(), deadline=deadline) + else: + self._write(encode_reboot()) except BootloaderProtocolError: return False return True @@ -376,7 +467,12 @@ def _read_exact(self, size: int, *, timeout: float | None = None, deadline: floa raise BootloaderProtocolError(msg) chunk = self._transport.read(size - len(received)) if not chunk: - if self._clock() < read_deadline: + remaining = read_deadline - self._clock() + if remaining > 0: + # A non-blocking transport can return empty reads in a tight + # loop. Yield briefly so a malformed transport cannot consume + # an entire CPU while the response deadline is pending. + self._sleep(min(0.01, remaining)) continue msg = _("timeout waiting for {expected} bootloader bytes; received {actual}").format( expected=size, actual=len(received) @@ -388,19 +484,19 @@ def _read_exact(self, size: int, *, timeout: float | None = None, deadline: floa raise BootloaderProtocolError(msg) from exc return bytes(received) - def _sync(self, *, timeout: float | None = None) -> None: - decode_sync(self._read_exact(2, timeout=timeout)) + def _sync(self, *, timeout: float | None = None, deadline: float | None = None) -> None: + decode_sync(self._read_exact(2, timeout=timeout, deadline=deadline)) - def _command(self, request: bytes, reply_size: int = 0) -> bytes: + def _command(self, request: bytes, reply_size: int = 0, *, deadline: float | None = None) -> bytes: self._write(request) - reply = self._read_exact(reply_size) if reply_size else b"" - self._sync() + reply = self._read_exact(reply_size, deadline=deadline) if reply_size else b"" + self._sync(deadline=deadline) return reply - def identify(self) -> BootloaderInfo: + def identify(self, *, deadline: float | None = None) -> BootloaderInfo: self._reset_input_buffer() - self._command(encode_get_sync()) - revision = decode_uint32(self._command(encode_get_device(INFO_BL_REV), 4)) + self._command(encode_get_sync(), deadline=deadline) + revision = decode_uint32(self._command(encode_get_device(INFO_BL_REV), 4, deadline=deadline)) if not BL_REV_MIN <= revision <= BL_REV_MAX: msg = _("unsupported bootloader protocol revision {revision}").format(revision=revision) raise BootloaderProtocolError(msg) @@ -408,16 +504,16 @@ def identify(self) -> BootloaderInfo: # Old bootloaders can reject this newer optional query. Resynchronize and # conservatively report no external flash in that case. try: - extf_size = decode_uint32(self._command(encode_get_device(INFO_EXTF_SIZE), 4)) + extf_size = decode_uint32(self._command(encode_get_device(INFO_EXTF_SIZE), 4, deadline=deadline)) except BootloaderProtocolError: extf_size = 0 self._reset_input_buffer() - self._command(encode_get_sync()) + self._command(encode_get_sync(), deadline=deadline) return BootloaderInfo( protocol_revision=revision, - board_id=decode_uint32(self._command(encode_get_device(INFO_BOARD_ID), 4)), - board_revision=decode_uint32(self._command(encode_get_device(INFO_BOARD_REV), 4)), - flash_size=decode_uint32(self._command(encode_get_device(INFO_FLASH_SIZE), 4)), + board_id=decode_uint32(self._command(encode_get_device(INFO_BOARD_ID), 4, deadline=deadline)), + board_revision=decode_uint32(self._command(encode_get_device(INFO_BOARD_REV), 4, deadline=deadline)), + flash_size=decode_uint32(self._command(encode_get_device(INFO_FLASH_SIZE), 4, deadline=deadline)), extf_size=extf_size, ) @@ -469,7 +565,7 @@ def _verify_external(self, image: FirmwareImage) -> None: msg = _("external firmware CRC verification failed") raise BootloaderProtocolError(msg) - def upload( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-branches,too-many-statements + def upload( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-branches,too-many-statements,too-many-locals self, image: FirmwareImage, *, @@ -488,6 +584,7 @@ def upload( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-branc revision three and later use CRC and require that ACK. """ stage = UploadStage.AWAITING_CONFIRMATION + info = bootloader try: if confirmation_requested is None: msg = _("firmware upload requires explicit confirmation") @@ -497,10 +594,17 @@ def upload( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-branc raise FirmwareUploadCancelledError(msg, stage=stage.value) stage = UploadStage.IDENTIFYING self._report(progress_callback, stage, 0, 1) - info = bootloader or self.identify() + info = info or self.identify() self._report(progress_callback, stage, 1, 1) check_bootloader_matches_connected_board(info, connected_board_id) check_compatibility(image, info) + if full_erase: + # AP_Bootloader only implements CHIP_FULL_ERASE on STM32F7/H7. + # This client does not yet use the bootloader's MCU-identification + # responses to gate that capability, so refuse rather than send a + # command that an unsupported target may ignore until timeout. + msg = _("full firmware erase is unavailable because support cannot yet be determined safely") + raise FirmwareCompatibilityError(msg) stage = UploadStage.AWAITING_CONFIRMATION self._report(progress_callback, stage, 0, 1) if not confirmation_requested(image, info): @@ -551,20 +655,36 @@ def upload( # noqa: PLR0915 # pylint: disable=too-many-arguments,too-many-branc self._report(progress_callback, stage, 1, 1) return info except FirmwareUploadError as exc: - if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION} and not self.abort_before_erase(): + if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION} and not self.abort_before_erase( + expect_ack=info is None or info.protocol_revision != 2 + ): msg = _("cannot reboot the held bootloader; power-cycle the flight controller before reconnecting") raise FirmwareBootloaderRecoveryError(msg) from exc if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION}: exc.bootloader_rebooted = True # type: ignore[attr-defined] if isinstance(exc, BootloaderProtocolError): exc.stage = stage.value + if stage in {UploadStage.ERASING, UploadStage.PROGRAMMING, UploadStage.VERIFYING, UploadStage.REBOOTING}: + recovery_message = _( + "firmware upload failed during {stage}; the firmware flash may be incomplete. " + "power-cycle the flight controller before reconnecting" + ).format(stage=stage.value) + _append_recovery_message(exc, recovery_message) raise except Exception as exc: - if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION} and not self.abort_before_erase(): + if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION} and not self.abort_before_erase( + expect_ack=info is None or info.protocol_revision != 2 + ): msg = _("cannot reboot the held bootloader; power-cycle the flight controller before reconnecting") raise FirmwareBootloaderRecoveryError(msg) from exc if stage in {UploadStage.IDENTIFYING, UploadStage.AWAITING_CONFIRMATION}: exc.bootloader_rebooted = True # type: ignore[attr-defined] + if stage in {UploadStage.ERASING, UploadStage.PROGRAMMING, UploadStage.VERIFYING, UploadStage.REBOOTING}: + recovery_message = _( + "firmware upload failed during {stage}; the firmware flash may be incomplete. " + "power-cycle the flight controller before reconnecting" + ).format(stage=stage.value) + _append_recovery_message(exc, recovery_message) raise finally: self.close() @@ -576,7 +696,14 @@ def open_serial_transport(device: str, baudrate: int, timeout: float) -> Bootloa class FlightControllerBootloaderBackend: # pylint:disable=too-many-instance-attributes - """Production-facing adapter with injectable bootloader entry and serial transport.""" + """ + Internal bootloader transport adapter with injectable entry and serial transport. + + The public upload workflow is ``FlightController.upload_apj_firmware``. It + performs the trusted-digest and stable-USB-identity checks before invoking + this lower-level adapter; direct callers are responsible for those policy + preconditions. + """ def __init__( # noqa: PLR0913 # pylint: disable=too-many-arguments self, @@ -588,9 +715,10 @@ def __init__( # noqa: PLR0913 # pylint: disable=too-many-arguments serial_factory: SerialFactory = open_serial_transport, device_resolver: DeviceResolver = resolve_bootloader_device, device_identity: SerialDeviceIdentity | None = None, - open_retries: int = 5, - retry_delay: float = 0.5, + open_retries: int = BOOTLOADER_OPEN_RETRIES, + retry_delay: float = BOOTLOADER_RETRY_DELAY, sleep: Callable[[float], None] = time_sleep, + clock: Callable[[], float] = monotonic, ) -> None: if timeout <= 0: msg = _("bootloader response timeout must be positive") @@ -605,6 +733,7 @@ def __init__( # noqa: PLR0913 # pylint: disable=too-many-arguments self._open_retries = open_retries self._retry_delay = retry_delay self._sleep = sleep + self._clock = clock def inspect_firmware(self, path: Path) -> FirmwareImage: return load_apj(path) @@ -627,7 +756,8 @@ def upload( # pylint: disable=too-many-arguments raise FirmwareUploadCancelledError(msg, stage=UploadStage.ENTERING_BOOTLOADER.value) if self._enter_bootloader is not None: self._enter_bootloader() - client, info = self._wait_for_bootloader() + client, info = self._wait_for_bootloader(cancellation_requested) + # pylint: disable=duplicate-code return client.upload( image, full_erase=full_erase, @@ -637,24 +767,57 @@ def upload( # pylint: disable=too-many-arguments bootloader=info, connected_board_id=connected_board_id, ) + # pylint: enable=duplicate-code - def _wait_for_bootloader(self) -> tuple[BootloaderClient, BootloaderInfo]: + def _wait_for_bootloader( # pylint: disable=too-many-locals + self, cancellation_requested: CancellationRequested | None = None + ) -> tuple[BootloaderClient, BootloaderInfo]: if self._open_retries < 1: msg = _("bootloader open retries must be at least one") raise ValueError(msg) last_error: FirmwareUploadError | OSError | serial.SerialException | None = None + deadline = self._clock() + BOOTLOADER_ENUMERATION_TIMEOUT for attempt in range(self._open_retries): - transport, open_error = self._try_open_transport() + if cancellation_requested is not None and cancellation_requested(): + msg = _( + "firmware upload was cancelled while waiting for the bootloader; " + "power-cycle the flight controller before reconnecting" + ) + raise FirmwareBootloaderRecoveryError(msg) from last_error + remaining = deadline - self._clock() + if remaining <= 0: + break + attempt_timeout = min(self._timeout, remaining) + transport, open_error = self._try_open_transport(attempt_timeout) if transport is not None: - client = BootloaderClient(transport, timeout=self._timeout) + client = BootloaderClient( + transport, + # Identification belongs to this retry attempt and must not + # consume the remaining global discovery budget through a + # stale transport timeout. + timeout=self._timeout, + clock=self._clock, + sleep=self._sleep, + ) try: - return client, client.identify() + # The global deadline controls how long discovery retries; + # each identification attempt gets its own bounded deadline + # so a slow re-enumeration can be retried. + attempt_deadline = min(deadline, self._clock() + attempt_timeout) + info = client.identify(deadline=attempt_deadline) + _restore_transport_timeout(transport, self._timeout) + return client, info except BootloaderProtocolError as exc: last_error = exc - if attempt + 1 == self._open_retries: - if not client.abort_before_erase(): + retry_would_exhaust_budget = self._clock() + self._retry_delay >= deadline + if attempt + 1 == self._open_retries or retry_would_exhaust_budget: + abort_deadline = self._clock() + min(self._timeout, BOOTLOADER_ABORT_TIMEOUT) + if not client.abort_before_erase(deadline=abort_deadline, expect_ack=True): client.close() - msg = _("cannot reboot the held bootloader; power-cycle the flight controller before reconnecting") + msg = _( + "could not confirm reboot of the held bootloader; " + "power-cycle the flight controller before reconnecting" + ) raise FirmwareBootloaderRecoveryError(msg) from exc exc.bootloader_rebooted = True # type: ignore[attr-defined] exc.stage = UploadStage.IDENTIFYING.value @@ -662,7 +825,9 @@ def _wait_for_bootloader(self) -> tuple[BootloaderClient, BootloaderInfo]: else: last_error = open_error if attempt + 1 < self._open_retries: - self._sleep(self._retry_delay) + remaining = deadline - self._clock() + if remaining > 0: + self._sleep(min(self._retry_delay, remaining)) if isinstance(last_error, BootloaderProtocolError): raise last_error msg = _( @@ -671,9 +836,11 @@ def _wait_for_bootloader(self) -> tuple[BootloaderClient, BootloaderInfo]: ).format(device=self._device, error=last_error) raise FirmwareBootloaderRecoveryError(msg) from last_error - def _try_open_transport(self) -> tuple[BootloaderTransport | None, OSError | serial.SerialException | None]: + def _try_open_transport( + self, timeout: float | None = None + ) -> tuple[BootloaderTransport | None, OSError | serial.SerialException | None]: try: device = self._device_resolver(self._device, self._device_identity) - return self._serial_factory(device, self._baudrate, self._timeout), None + return self._serial_factory(device, self._baudrate, self._timeout if timeout is None else timeout), None except (OSError, serial.SerialException) as exc: return None, exc diff --git a/ardupilot_methodic_configurator/data_model_firmware_upload.py b/ardupilot_methodic_configurator/data_model_firmware_upload.py index 0690330c1..fa29a0072 100755 --- a/ardupilot_methodic_configurator/data_model_firmware_upload.py +++ b/ardupilot_methodic_configurator/data_model_firmware_upload.py @@ -21,7 +21,6 @@ import hmac import json import re -import struct import zlib from binascii import Error as BinasciiError from dataclasses import dataclass @@ -126,7 +125,6 @@ class FirmwareImageMetadata: # pylint: disable=too-many-instance-attributes extf_image_size: int firmware_version: str git_identity: str - content_sha256: str apj_sha256: str board_revision: int | None = None @@ -141,6 +139,9 @@ class FirmwareImage: def crc(self, flash_size: int) -> int: """CRC32 of the image padded with 0xFF up to flash_size, as computed by the bootloader GET_CRC.""" + if flash_size < 0 or flash_size % 4: + msg = _("bootloader flash size must be a non-negative multiple of four") + raise ValueError(msg) state = bootloader_crc32(self.image) remaining = max(0, flash_size - len(self.image)) while remaining: @@ -153,12 +154,6 @@ def extf_crc(self) -> int: """CRC32 of the unpadded external-flash payload.""" return bootloader_crc32(self.extf_image[: self.metadata.extf_image_size]) - def content_sha256(self) -> str: - """Return the digest of the exact, unpadded payload selected for upload.""" - return firmware_content_sha256( - self.image[: self.metadata.image_size], self.extf_image[: self.metadata.extf_image_size] - ) - @dataclass(frozen=True) class BootloaderInfo: @@ -229,12 +224,12 @@ def next_stage(current: UploadStage, target: UploadStage) -> UploadStage: raise ValueError(msg) -def parse_apj(contents: str | bytes, *, path: Path = Path()) -> FirmwareImage: +def parse_apj(contents: str | bytes, *, path: Path = Path()) -> FirmwareImage: # pylint: disable=too-many-locals """Purely parse and validate APJ contents; callers own file-system access.""" - source_bytes = contents.encode("utf-8") if isinstance(contents, str) else contents try: + source_bytes = contents.encode("utf-8") if isinstance(contents, str) else contents desc: Any = json.loads(contents) - except (UnicodeDecodeError, json.JSONDecodeError, TypeError) as exc: + except (RecursionError, UnicodeError, ValueError, TypeError) as exc: msg = _("cannot parse APJ descriptor: {error}").format(error=exc) raise FirmwareFileError(msg) from exc if not isinstance(desc, dict): @@ -258,6 +253,15 @@ def parse_apj(contents: str | bytes, *, path: Path = Path()) -> FirmwareImage: msg = _("APJ image_size does not match the decoded image") raise FirmwareFileError(msg) + firmware_version = desc.get("version", "") + if not isinstance(firmware_version, str): + msg = _("APJ version metadata must be text") + raise FirmwareFileError(msg) + git_identity = desc.get("git_identity", "") + if not isinstance(git_identity, str): + msg = _("APJ git_identity metadata must be text") + raise FirmwareFileError(msg) + board_rev = desc.get("board_revision") try: parsed_board_rev = _parse_integer_metadata(board_rev) if board_rev is not None else None @@ -269,9 +273,8 @@ def parse_apj(contents: str | bytes, *, path: Path = Path()) -> FirmwareImage: board_id=board_id, image_size=image_size, extf_image_size=extf_image_size, - firmware_version=str(desc.get("version", "")), - git_identity=str(desc.get("git_identity", "")), - content_sha256=firmware_content_sha256(raw_image, raw_extf_image), + firmware_version=firmware_version, + git_identity=git_identity, apj_sha256=hashlib.sha256(source_bytes).hexdigest(), board_revision=parsed_board_rev, ) @@ -304,13 +307,19 @@ def _decode_blob(desc: dict, key: str) -> bytes: if len(raw) > MAX_IMAGE_SIZE or decompressor.unconsumed_tail: msg = _("APJ {key} exceeds {limit} bytes").format(key=key, limit=MAX_IMAGE_SIZE) raise FirmwareFileError(msg) - raw += decompressor.flush(MAX_IMAGE_SIZE + 1 - len(raw)) + # ``unconsumed_tail`` above proves the bounded decompress call consumed + # the entire stream. ``flush`` merely releases the decoder's small + # pending buffer; its argument is not an output limit. + raw += decompressor.flush() if len(raw) > MAX_IMAGE_SIZE: msg = _("APJ {key} exceeds {limit} bytes").format(key=key, limit=MAX_IMAGE_SIZE) raise FirmwareFileError(msg) if not decompressor.eof: msg = _("APJ {key} is not valid base64+zlib data: truncated stream").format(key=key) raise FirmwareFileError(msg) + if decompressor.unused_data: + msg = _("APJ {key} is not valid base64+zlib data: trailing bytes").format(key=key) + raise FirmwareFileError(msg) except (BinasciiError, KeyError, TypeError, ValueError, zlib.error) as exc: msg = _("APJ {key} is not valid base64+zlib data: {error}").format(key=key, error=exc) raise FirmwareFileError(msg) from exc @@ -325,25 +334,12 @@ def bootloader_crc32(data: bytes, state: int = 0) -> int: """ Return the raw CRC-32 state used by ArduPilot's bootloader protocol. - This intentionally differs from :func:`binascii.crc32`, which applies the - conventional CRC-32 initial/final XOR values. ArduPilot's - ``crc32_small`` and ``Tools/scripts/uploader.py`` instead expose the raw - running state beginning at zero. + ArduPilot's ``crc32_small`` and ``Tools/scripts/uploader.py`` expose the + raw running state beginning at zero. ``zlib.crc32`` uses the same update + table but applies conventional initial/final XOR values, so translate the + state at the boundary instead of processing one bit at a time in Python. """ - for byte in data: - state ^= byte - for _bit in range(8): - state = (state >> 1) ^ (0xEDB88320 if state & 1 else 0) - return state & 0xFFFFFFFF - - -def firmware_content_sha256(image: bytes, extf_image: bytes = b"") -> str: - """Hash both raw APJ payload regions with unambiguous length prefixes.""" - digest = hashlib.sha256() - for region in (image, extf_image): - digest.update(struct.pack(" None: @@ -384,6 +380,9 @@ def check_compatibility(image: FirmwareImage, bootloader: BootloaderInfo) -> Non image_id=meta.board_id, board_id=bootloader.board_id ) raise FirmwareCompatibilityError(msg) + if bootloader.flash_size < 0 or bootloader.flash_size % 4: + msg = _("bootloader reports an invalid flash size of {flash} bytes").format(flash=bootloader.flash_size) + raise FirmwareCompatibilityError(msg) if len(image.image) > bootloader.flash_size: msg = _("image of {size} bytes exceeds flash of {flash} bytes").format( size=len(image.image), flash=bootloader.flash_size diff --git a/pyproject.toml b/pyproject.toml index f9c375e41..45e7387a3 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -76,6 +76,7 @@ dev = [ "pytest-cov==7.1.0", "pytest-md==0.2.0", "pytest-mock==3.15.1", + "pytest-timeout==2.4.0", "python-gettext==5.0", "ruff==0.16.3", "types-requests==2.33.0.20260712", diff --git a/tests/test_backend_flightcontroller.py b/tests/test_backend_flightcontroller.py index 1415fc188..12f079aaa 100755 --- a/tests/test_backend_flightcontroller.py +++ b/tests/test_backend_flightcontroller.py @@ -15,6 +15,7 @@ import tempfile from argparse import ArgumentParser +from collections.abc import Callable from typing import Any, cast # pylint: disable=unused-import from unittest.mock import MagicMock, patch @@ -42,6 +43,7 @@ def _build_flight_controller_with_mocks( reboot_time: int = 2, + sleep: Callable[[float], None] | None = None, ) -> tuple[FlightController, MagicMock, MagicMock, MagicMock, MagicMock, MagicMock]: """Helper returning a facade wired with MagicMock managers for delegation tests.""" mock_master = MagicMock() @@ -101,6 +103,7 @@ def _build_flight_controller_with_mocks( params_manager=mock_params_mgr, commands_manager=mock_commands_mgr, files_manager=mock_files_mgr, + sleep=sleep, ) return fc, mock_conn_mgr, mock_params_mgr, mock_commands_mgr, mock_files_mgr, mock_master @@ -701,17 +704,17 @@ def test_user_can_reset_and_reconnect_after_configuration(self) -> None: THEN: The autopilot should reboot and reconnect using retries if needed AND: Progress callbacks should reflect each wait step """ - fc, mock_conn_mgr, *_others, mock_master = _build_flight_controller_with_mocks(reboot_time=2) + sleeps: list[float] = [] + fc, mock_conn_mgr, *_others, mock_master = _build_flight_controller_with_mocks(reboot_time=2, sleep=sleeps.append) mock_conn_mgr.create_connection_with_retry.return_value = "RECONNECTED" progress_updates: list[tuple[int, int]] = [] connection_progress = MagicMock() - with patch("ardupilot_methodic_configurator.backend_flightcontroller.time_sleep", return_value=None): - result = fc.reset_and_reconnect( - reset_progress_callback=lambda current, total: progress_updates.append((current, total)), - connection_progress_callback=connection_progress, - extra_sleep_time=1, - ) + result = fc.reset_and_reconnect( + reset_progress_callback=lambda current, total: progress_updates.append((current, total)), + connection_progress_callback=connection_progress, + extra_sleep_time=1, + ) mock_master.reboot_autopilot.assert_called_once() mock_conn_mgr.disconnect.assert_called_once() @@ -724,6 +727,7 @@ def test_user_can_reset_and_reconnect_after_configuration(self) -> None: ) assert progress_updates[0] == (0, 3) assert progress_updates[-1] == (3, 3) + assert sleeps == [0.3, 1, 1, 1] assert result == "RECONNECTED" def test_reset_and_reconnect_returns_immediately_when_disconnected(self) -> None: diff --git a/tests/test_backend_flightcontroller_bootloader.py b/tests/test_backend_flightcontroller_bootloader.py index c7bf88d4e..06638bc90 100755 --- a/tests/test_backend_flightcontroller_bootloader.py +++ b/tests/test_backend_flightcontroller_bootloader.py @@ -14,10 +14,13 @@ import hashlib import json import struct +import sys import zlib +from collections.abc import Callable from pathlib import Path import pytest +import serial from serial.tools.list_ports_common import ListPortInfo from ardupilot_methodic_configurator import backend_flightcontroller_bootloader as bl @@ -75,6 +78,7 @@ def __init__(self, *, revision: int = 5, board_id: int = 9, flash_size: int = 20 self.closed = False self.rebooted = False self.commands: list[bytes] = [] + self.requests: list[bytes] = [] self._reply = bytearray() self._read_offset = 0 @@ -82,6 +86,7 @@ def write(self, data: bytes) -> int: assert data.endswith(bl.EOC) command, body = data[:1], data[1:-1] self.commands.append(command) + self.requests.append(data) sync = bl.INSYNC + bl.OK if command == bl.GET_SYNC: self._reply.extend(sync) @@ -99,6 +104,8 @@ def write(self, data: bytes) -> int: self.extf = b"" self._reply.extend(sync + bytes((0, 6, 12, 18, 25, 31, 37, 43, 50, 56, 62, 68, 75, 81, 87, 93, 100)) + sync) elif command == bl.EXTF_PROG_MULTI: + assert body, "external PROG_MULTI request has no length byte" + assert body[0] == len(body[1:]), "external PROG_MULTI length byte is incorrect" self.extf += body[1:] self._reply.extend(sync) elif command == bl.EXTF_GET_CRC: @@ -108,6 +115,8 @@ def write(self, data: bytes) -> int: self.flash = b"" self._reply.extend(sync) elif command == bl.PROG_MULTI: + assert body, "PROG_MULTI request has no length byte" + assert body[0] == len(body[1:]), "PROG_MULTI length byte is incorrect" self.flash += body[1:] self._reply.extend(sync) elif command == bl.CHIP_VERIFY: @@ -153,6 +162,96 @@ def test_upload_verifies_revision_specific_protocol_and_closes(revision: int) -> assert info.protocol_revision == revision assert transport.flash == b"abc\xff" assert transport.closed + if revision == 2: + assert bl.CHIP_VERIFY in transport.commands + assert bl.GET_CRC not in transport.commands + else: + assert bl.GET_CRC in transport.commands + assert bl.CHIP_VERIFY not in transport.commands + + +def test_protocol_byte_constants_match_the_bootloader_wire_specification() -> None: + """Keep the fake transport from masking a protocol-byte renumbering.""" + assert { + "INSYNC": bl.INSYNC, + "EOC": bl.EOC, + "OK": bl.OK, + "FAILED": bl.FAILED, + "INVALID": bl.INVALID, + "BAD_SILICON_REV": bl.BAD_SILICON_REV, + "GET_SYNC": bl.GET_SYNC, + "GET_DEVICE": bl.GET_DEVICE, + "CHIP_ERASE": bl.CHIP_ERASE, + "CHIP_VERIFY": bl.CHIP_VERIFY, + "PROG_MULTI": bl.PROG_MULTI, + "READ_MULTI": bl.READ_MULTI, + "GET_CRC": bl.GET_CRC, + "REBOOT": bl.REBOOT, + "EXTF_ERASE": bl.EXTF_ERASE, + "EXTF_PROG_MULTI": bl.EXTF_PROG_MULTI, + "EXTF_GET_CRC": bl.EXTF_GET_CRC, + "CHIP_FULL_ERASE": bl.CHIP_FULL_ERASE, + "INFO_BL_REV": bl.INFO_BL_REV, + "INFO_BOARD_ID": bl.INFO_BOARD_ID, + "INFO_BOARD_REV": bl.INFO_BOARD_REV, + "INFO_FLASH_SIZE": bl.INFO_FLASH_SIZE, + "INFO_EXTF_SIZE": bl.INFO_EXTF_SIZE, + } == { + "INSYNC": b"\x12", + "EOC": b"\x20", + "OK": b"\x10", + "FAILED": b"\x11", + "INVALID": b"\x13", + "BAD_SILICON_REV": b"\x14", + "GET_SYNC": b"\x21", + "GET_DEVICE": b"\x22", + "CHIP_ERASE": b"\x23", + "CHIP_VERIFY": b"\x24", + "PROG_MULTI": b"\x27", + "READ_MULTI": b"\x28", + "GET_CRC": b"\x29", + "REBOOT": b"\x30", + "EXTF_ERASE": b"\x34", + "EXTF_PROG_MULTI": b"\x35", + "EXTF_GET_CRC": b"\x37", + "CHIP_FULL_ERASE": b"\x40", + "INFO_BL_REV": b"\x01", + "INFO_BOARD_ID": b"\x02", + "INFO_BOARD_REV": b"\x03", + "INFO_FLASH_SIZE": b"\x04", + "INFO_EXTF_SIZE": b"\x06", + } + + +def test_encode_chip_erase_preserves_the_requested_erase_mode() -> None: + assert bl.encode_chip_erase() == b"\x23\x20" + assert bl.encode_chip_erase(full=True) == b"\x40\x20" + + +@pytest.mark.parametrize( + ("encode", "expected"), + [ + (bl.encode_get_sync, b"\x21\x20"), + (lambda: bl.encode_get_device(bl.INFO_BL_REV), b"\x22\x01\x20"), + (bl.encode_chip_verify, b"\x24\x20"), + (lambda: bl.encode_prog_multi(b"\x01\x02\x03\x04"), b"\x27\x04\x01\x02\x03\x04\x20"), + (lambda: bl.encode_extf_prog_multi(b"\x01\x02\x03\x04"), b"\x35\x04\x01\x02\x03\x04\x20"), + (lambda: bl.encode_read_multi(4), b"\x28\x04\x20"), + (lambda: bl.encode_extf_erase(0x01020304), b"\x34\x04\x03\x02\x01\x20"), + (lambda: bl.encode_extf_get_crc(0x01020304), b"\x37\x04\x03\x02\x01\x20"), + (bl.encode_get_crc, b"\x29\x20"), + (bl.encode_reboot, b"\x30\x20"), + ], +) +def test_encoder_emits_the_bootloader_wire_format(encode: Callable[[], bytes], expected: bytes) -> None: + """ + Each command uses the byte layout expected by AP_Bootloader. + + GIVEN: A command encoder and a representative request + WHEN: The request is encoded + THEN: Its bytes match the independent bootloader wire specification + """ + assert encode() == expected def test_v2_verification_uses_the_read_chunk_limit(monkeypatch: pytest.MonkeyPatch) -> None: @@ -167,9 +266,13 @@ def test_v2_verification_uses_the_read_chunk_limit(monkeypatch: pytest.MonkeyPat monkeypatch.setattr(bl, "READ_MULTI_MAX", 64) image = fw.parse_apj(apj(bytes(range(100)), board_id=9)) - info = bl.BootloaderClient(FakeBootloaderTransport(revision=2)).upload(image, confirmation_requested=lambda *_args: True) + transport = FakeBootloaderTransport(revision=2) + info = bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) assert info.protocol_revision == 2 + read_requests = [request for request in transport.requests if request[:1] == bl.READ_MULTI] + assert [request[1] for request in read_requests] == [64, 36] + assert all(request[2:] == bl.EOC for request in read_requests) def test_identify_resets_stale_bytes_before_old_bootloader_fallback() -> None: @@ -208,7 +311,7 @@ def reset_input_buffer(self) -> None: info = bl.BootloaderClient(transport).identify() - assert info.extf_size == 0 + assert info == bl.BootloaderInfo(protocol_revision=2, board_id=9, board_revision=0, flash_size=2048, extf_size=0) assert transport.reset_count == 2 @@ -228,6 +331,69 @@ def test_uploads_and_verifies_external_flash() -> None: assert transport.extf == b"ext\xff" assert transport.extf_erase_size == image.metadata.extf_image_size assert transport.closed + assert transport.requests == [ + bl.encode_get_sync(), + bl.encode_get_device(bl.INFO_BL_REV), + bl.encode_get_device(bl.INFO_EXTF_SIZE), + bl.encode_get_device(bl.INFO_BOARD_ID), + bl.encode_get_device(bl.INFO_BOARD_REV), + bl.encode_get_device(bl.INFO_FLASH_SIZE), + bl.encode_extf_erase(image.metadata.extf_image_size), + bl.encode_extf_prog_multi(image.extf_image[0 : bl.PROG_MULTI_MAX]), + bl.encode_extf_get_crc(image.metadata.extf_image_size), + bl.encode_chip_erase(), + bl.encode_prog_multi(image.image), + bl.encode_get_crc(), + bl.encode_reboot(), + ] + for request in transport.requests: + if request[:1] in {bl.PROG_MULTI, bl.EXTF_PROG_MULTI}: + assert request[1] == len(request[2:-1]) + + +def test_upload_splits_internal_and_external_payloads_at_the_protocol_boundary() -> None: + """ + Both flash regions use complete, word-aligned protocol chunks. + + GIVEN: Each APJ payload is one byte larger than the maximum bootloader chunk + WHEN: The client uploads and verifies both regions + THEN: Each region is sent as a maximum chunk followed by a padded four-byte chunk + """ + internal = bytes(range(253)) + external = bytes(reversed(range(253))) + image = fw.parse_apj(apj(internal, extf_image=external)) + transport = FakeBootloaderTransport() + + bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) + + internal_requests = [request for request in transport.requests if request[:1] == bl.PROG_MULTI] + external_requests = [request for request in transport.requests if request[:1] == bl.EXTF_PROG_MULTI] + assert [request[1] for request in internal_requests] == [252, 4] + assert [request[1] for request in external_requests] == [252, 4] + assert [request[2:-1] for request in internal_requests] == [internal[:252], internal[252:] + b"\xff\xff\xff"] + assert [request[2:-1] for request in external_requests] == [external[:252], external[252:] + b"\xff\xff\xff"] + assert transport.flash == internal + b"\xff\xff\xff" + assert transport.extf == external + b"\xff\xff\xff" + + +def test_external_crc_mismatch_rejects_the_upload() -> None: + """A bad external-flash CRC must not be accepted after programming.""" + + class BadExternalCrcTransport(FakeBootloaderTransport): + """Return an incorrect CRC after external-flash programming.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_GET_CRC: + self._reply.extend(struct.pack(" None: @@ -246,6 +412,54 @@ def write(self, data: bytes) -> int: client._erase_external(3) +def test_external_erase_rejects_progress_above_one_hundred() -> None: + """An impossible erase percentage is a protocol error, not completion.""" + + class InvalidProgressTransport(FakeBootloaderTransport): + """Reports an external erase percentage outside the protocol range.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_ERASE: + self._reply.extend(bl.INSYNC + bl.OK + bytes((101,))) + return len(data) + return super().write(data) + + with pytest.raises(fw.BootloaderProtocolError, match="invalid external-flash erase progress"): + bl.BootloaderClient(InvalidProgressTransport())._erase_external(3) + + +def test_external_erase_does_not_accept_a_premature_final_status() -> None: + """The final INSYNC marker is valid only after erase progress reaches the completion threshold.""" + + class PrematureCompletionTransport(FakeBootloaderTransport): + """Places a final status marker before the erase is nearly complete.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_ERASE: + self._reply.extend(bl.INSYNC + bl.OK + bytes((10,)) + bl.INSYNC + bl.OK) + return len(data) + return super().write(data) + + with pytest.raises(fw.BootloaderProtocolError, match="regressed"): + bl.BootloaderClient(PrematureCompletionTransport())._erase_external(3) + + +def test_external_erase_reports_a_failed_final_status() -> None: + """A final external erase failure is not mistaken for successful completion.""" + + class FailedEraseTransport(FakeBootloaderTransport): + """Reports a failed final erase acknowledgement after reaching 100%.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.EXTF_ERASE: + self._reply.extend(bl.INSYNC + bl.OK + bytes((100,)) + bl.INSYNC + bl.FAILED) + return len(data) + return super().write(data) + + with pytest.raises(fw.BootloaderProtocolError, match="OPERATION FAILED"): + bl.BootloaderClient(FailedEraseTransport())._erase_external(3) + + def test_stalled_external_erase_times_out_without_rebooting_after_erase() -> None: class StalledEraseTransport(FakeBootloaderTransport): """Stops reporting progress after starting an external erase.""" @@ -272,6 +486,7 @@ def clock() -> float: assert not transport.rebooted +@pytest.mark.timeout(5) def test_external_erase_times_out_when_progress_never_advances() -> None: """Repeated progress bytes must not keep a stalled erase alive indefinitely.""" @@ -351,9 +566,65 @@ def test_external_only_upload_does_not_touch_internal_flash() -> None: ) +def test_upload_reports_each_external_and_internal_region_in_order() -> None: + """ + Progress distinguishes the two flash regions and their verification phases. + + GIVEN: An APJ contains both external and internal firmware regions + WHEN: The bootloader uploads and verifies the APJ + THEN: Progress reports each region in protocol order without merging stages + """ + events: list[tuple[bl.UploadStage, int, int]] = [] + transport = FakeBootloaderTransport() + + bl.BootloaderClient(transport).upload( + fw.parse_apj(apj(b"abcd", extf_image=b"ext")), + confirmation_requested=lambda *_args: True, + progress_callback=lambda stage, done, total: events.append((stage, done, total)), + ) + + assert events == [ + (fw.UploadStage.IDENTIFYING, 0, 1), + (fw.UploadStage.IDENTIFYING, 1, 1), + (fw.UploadStage.AWAITING_CONFIRMATION, 0, 1), + (fw.UploadStage.AWAITING_CONFIRMATION, 1, 1), + (fw.UploadStage.ERASING, 0, 1), + (fw.UploadStage.ERASING, 1, 1), + (fw.UploadStage.PROGRAMMING, 1, 1), + (fw.UploadStage.VERIFYING, 0, 1), + (fw.UploadStage.VERIFYING, 1, 1), + (fw.UploadStage.ERASING, 0, 1), + (fw.UploadStage.ERASING, 1, 1), + (fw.UploadStage.PROGRAMMING, 1, 1), + (fw.UploadStage.VERIFYING, 0, 1), + (fw.UploadStage.VERIFYING, 1, 1), + (fw.UploadStage.REBOOTING, 0, 1), + (fw.UploadStage.REBOOTING, 1, 1), + ] + + +def test_upload_survives_progress_callback_failures() -> None: + """A presentation callback failure cannot interrupt a verified flash.""" + transport = FakeBootloaderTransport() + + def broken_progress(*_args: object) -> None: + msg = "progress UI failed" + raise RuntimeError(msg) + + bl.BootloaderClient(transport).upload( + fw.parse_apj(apj(b"abcd")), + confirmation_requested=lambda *_args: True, + progress_callback=broken_progress, + ) + + assert transport.flash == b"abcd" + assert transport.rebooted + assert transport.closed + + def test_failed_upload_still_closes_transport() -> None: image = fw.parse_apj(apj(b"abcd")) - transport = FakeBootloaderTransport(flash_size=3) + transport = FakeBootloaderTransport(flash_size=0) with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): bl.BootloaderClient(transport).upload(image, confirmation_requested=lambda *_args: True) @@ -364,15 +635,212 @@ def test_read_timeout_is_bounded_even_when_serial_keeps_returning_empty_reads() class EmptyReadTransport(FakeBootloaderTransport): """Transport that simulates a serial port timing out without a response.""" + reads = 0 + def read(self, size: int = 1) -> bytes: + self.reads += 1 return b"" - clock_values = iter([0.0, 1.0]) - client = bl.BootloaderClient(EmptyReadTransport(), timeout=1.0, clock=lambda: next(clock_values)) + transport = EmptyReadTransport() + clock_values = iter([0.0, 0.5, 1.0]) + client = bl.BootloaderClient(transport, timeout=1.0, clock=lambda: next(clock_values)) with pytest.raises(fw.BootloaderProtocolError, match="timeout waiting"): client._read_exact(1) # Exercise the transport deadline directly. + assert transport.reads == 1 + + +def test_client_uses_injected_sleep_for_empty_nonblocking_reads() -> None: + """The read-loop yield is deterministic and does not need a module monkeypatch.""" + + class EmptyReadTransport(FakeBootloaderTransport): + """Return empty reads to exercise the injected yield function.""" + + def read(self, size: int = 1) -> bytes: + return b"" + + sleeps: list[float] = [] + clock_values = iter([0.0, 0.0, 0.5, 1.0]) + client = bl.BootloaderClient(EmptyReadTransport(), timeout=1.0, clock=lambda: next(clock_values), sleep=sleeps.append) + + with pytest.raises(fw.BootloaderProtocolError, match="timeout waiting"): + client._read_exact(1) + + assert sleeps == [0.01] + + +def test_backend_passes_its_injected_sleep_to_the_bootloader_client() -> None: + """ + Bootloader discovery keeps the client's nonblocking-read yield injectable. + + GIVEN: Opening the bootloader succeeds but its first read is empty + WHEN: The backend uploads firmware with an injected sleep function + THEN: The bootloader client uses that function rather than module-global sleep + """ + + class EmptyOnceTransport(FakeBootloaderTransport): + """Return one empty read before providing the bootloader response.""" + + empty_reads = 1 + + def read(self, size: int = 1) -> bytes: + if self.empty_reads: + self.empty_reads -= 1 + return b"" + return super().read(size) + + sleeps: list[float] = [] + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + serial_factory=lambda *_args: EmptyOnceTransport(), + sleep=sleeps.append, + ) + + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert sleeps == [0.01] + + +@pytest.mark.parametrize( + ("operation", "error_type", "message"), + [ + ("write", OSError, "transport write failed"), + ("read", serial.SerialException, "transport read failed"), + ("read", OSError, "transport read failed"), + ("flush", serial.SerialException, "transport write failed"), + ("reset", OSError, "cannot clear bootloader serial input"), + ("reset", serial.SerialException, "cannot clear bootloader serial input"), + ], +) +def test_transport_errors_are_converted_to_typed_protocol_errors( + operation: str, error_type: type[Exception], message: str +) -> None: + """Serial-layer failures must not escape as untyped transport exceptions.""" + + class FailingTransport: + """Raise the selected serial failure from one transport operation.""" + + def write(self, _data: bytes) -> int: + if operation == "write": + msg = "transport failed" + raise error_type(msg) + return 0 + + def read(self, _size: int = 1) -> bytes: + if operation == "read": + msg = "transport failed" + raise error_type(msg) + return b"" + + def flush(self) -> None: + if operation == "flush": + msg = "transport failed" + raise error_type(msg) + + def reset_input_buffer(self) -> None: + if operation == "reset": + msg = "transport failed" + raise error_type(msg) + + def close(self) -> None: + pass + + client = bl.BootloaderClient(FailingTransport(), sleep=lambda _delay: None) + action = { + "write": lambda: client._write(bl.encode_get_sync()), + "read": lambda: client._read_exact(1), + "flush": lambda: client._write(bl.encode_get_sync()), + "reset": client._reset_input_buffer, + }[operation] + + with pytest.raises(fw.BootloaderProtocolError, match=message): + action() + + +@pytest.mark.parametrize( + ("operation", "reply_size"), + [ + ("get_sync", 0), + ("get_device", 4), + ("chip_erase", 0), + ("full_erase", 0), + ("chip_verify", 0), + ("prog_multi", 0), + ("extf_prog_multi", 0), + ("extf_erase", 0), + ("read_multi", 4), + ("get_crc", 4), + ("extf_get_crc", 4), + ("reboot", 0), + ], +) +def test_each_bootloader_command_rejects_a_failed_status(operation: str, reply_size: int) -> None: + """Every command path propagates a failed bootloader status as a typed error.""" + + class FailedStatusTransport: + """Return a failed status for any command while preserving expected reply lengths.""" + + def __init__(self) -> None: + self.reply = bytearray() + + def write(self, data: bytes) -> int: + command = data[:1] + if command in {bl.GET_DEVICE, bl.READ_MULTI, bl.GET_CRC, bl.EXTF_GET_CRC}: + self.reply.extend(b"\x00" * 4) + self.reply.extend(bl.INSYNC + bl.FAILED) + return len(data) + + def read(self, size: int = 1) -> bytes: + data = bytes(self.reply[:size]) + del self.reply[:size] + return data + + def flush(self) -> None: + pass + + def reset_input_buffer(self) -> None: + self.reply.clear() + + def close(self) -> None: + pass + + requests = { + "get_sync": bl.encode_get_sync(), + "get_device": bl.encode_get_device(bl.INFO_BL_REV), + "chip_erase": bl.encode_chip_erase(), + "full_erase": bl.encode_chip_erase(full=True), + "chip_verify": bl.encode_chip_verify(), + "prog_multi": bl.encode_prog_multi(b"\x00\x00\x00\x00"), + "extf_prog_multi": bl.encode_extf_prog_multi(b"\x00\x00\x00\x00"), + "extf_erase": bl.encode_extf_erase(4), + "read_multi": bl.encode_read_multi(4), + "get_crc": bl.encode_get_crc(), + "extf_get_crc": bl.encode_extf_get_crc(4), + "reboot": bl.encode_reboot(), + } + client = bl.BootloaderClient(FailedStatusTransport()) + + with pytest.raises(fw.BootloaderProtocolError, match="OPERATION FAILED"): + client._command(requests[operation], reply_size) + + +def test_partial_transport_write_is_rejected() -> None: + class PartialWriteTransport: + """Accept only part of each request.""" + + def write(self, data: bytes) -> int: + return len(data) - 1 + + def flush(self) -> None: + pass + + client = bl.BootloaderClient(PartialWriteTransport()) + + with pytest.raises(fw.BootloaderProtocolError, match="wrote 1 of 2 bytes"): + client._write(bl.encode_get_sync()) + def test_client_requires_confirmation_and_reboots_before_closing() -> None: transport = FakeBootloaderTransport() @@ -401,15 +869,43 @@ def test_parse_apj_is_pure_and_requires_declared_size_to_match() -> None: fw.parse_apj(contents, path=Path("firmware.apj")) +def test_parse_apj_wraps_invalid_unicode_text_as_a_file_error() -> None: + """The public text parser must not leak UnicodeEncodeError.""" + with pytest.raises(fw.FirmwareFileError, match="cannot parse APJ descriptor"): + fw.parse_apj("\ud800") + + +def test_parse_apj_wraps_excessively_nested_json_as_a_file_error() -> None: + """Deep but size-bounded APJ JSON must not leak RecursionError.""" + contents = b'{"x":' * 50_000 + b"0" + b"}" * 50_000 + assert len(contents) < bl.MAX_APJ_DESCRIPTOR_SIZE + + with pytest.raises(fw.FirmwareFileError): + fw.parse_apj(contents) + + def test_invalid_board_revision_is_a_typed_file_error() -> None: with pytest.raises(fw.FirmwareFileError, match="metadata"): fw.parse_apj(apj(b"four", board_revision="not-a-number")) +@pytest.mark.skipif(not hasattr(sys, "get_int_max_str_digits"), reason="CPython integer digit limit was added in Python 3.11") +def test_parse_apj_wraps_json_integer_digit_limit_as_file_error() -> None: + """A JSON number beyond CPython's digit limit must not escape untyped.""" + digit_limit = sys.get_int_max_str_digits() + if digit_limit == 0: + pytest.skip("CPython integer digit limit is disabled") + + contents = b'{"board_id":' + (b"9" * (digit_limit + 1)) + b"}" + + with pytest.raises(fw.FirmwareFileError, match="cannot parse APJ descriptor"): + fw.parse_apj(contents) + + def test_padded_payload_capacity_and_compatible_board_mapping_are_checked() -> None: image = fw.parse_apj(apj(b"abc")) with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): - fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 3)) + fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 0)) fw.check_compatibility(image, fw.BootloaderInfo(5, 33, 0, 4)) @@ -417,8 +913,39 @@ def test_padded_payload_capacity_and_compatible_board_mapping_are_checked() -> N def test_bounded_decompression_rejects_before_full_output_is_allocated(monkeypatch: pytest.MonkeyPatch) -> None: monkeypatch.setattr(fw, "MAX_IMAGE_SIZE", 32) + limits: list[int] = [] + + class BoundedDecompressor: + """Expose the decompressor output limit supplied by the parser.""" + + unconsumed_tail = b"still compressed" + unused_data = b"" + eof = True + + def decompress(self, _compressed: bytes, max_length: int) -> bytes: + limits.append(max_length) + return b"x" * max_length + + def flush(self) -> bytes: + return b"" + + monkeypatch.setattr(fw.zlib, "decompressobj", BoundedDecompressor) + with pytest.raises(fw.FirmwareFileError, match="exceeds"): fw.parse_apj(apj(b"x" * 64)) + assert limits == [33] + + +def test_parser_rejects_trailing_compressed_data_and_non_text_version() -> None: + encoded = base64.b64encode(zlib.compress(b"abcd") + b"unexpected").decode() + trailing_data = json.dumps({"board_id": 9, "image_size": 4, "image": encoded}) + + with pytest.raises(fw.FirmwareFileError, match="trailing bytes"): + fw.parse_apj(trailing_data) + with pytest.raises(fw.FirmwareFileError, match="version metadata"): + fw.parse_apj(apj(b"abcd", version=46)) + with pytest.raises(fw.FirmwareFileError, match="git_identity metadata"): + fw.parse_apj(apj(b"abcd", git_identity=42)) def test_parser_rejects_oversized_encoded_blob_before_base64_decode(monkeypatch: pytest.MonkeyPatch) -> None: @@ -467,7 +994,6 @@ def test_bootloader_port_is_re_resolved_by_usb_identity_and_ambiguity_is_refused monkeypatch: pytest.MonkeyPatch, ) -> None: """A re-enumerated bootloader must not reuse an arbitrary stale serial port.""" - monkeypatch.setattr(bl, "sys_platform", "win32") reenumerated = ListPortInfo("COM11") reenumerated.location = "1-2.3" reenumerated.serial_number = "FC-123" @@ -484,19 +1010,13 @@ def test_bootloader_port_is_re_resolved_by_usb_identity_and_ambiguity_is_refused bl.resolve_bootloader_device("COM7", identity) -def test_usb_identity_is_checked_before_linux_by_path_fallback(monkeypatch: pytest.MonkeyPatch) -> None: - """A matching USB identity must win without probing the filesystem fallback.""" - monkeypatch.setattr(bl, "sys_platform", "linux") +def test_usb_identity_is_used_for_reenumerated_linux_devices(monkeypatch: pytest.MonkeyPatch) -> None: + """A matching USB identity must win over the old Linux device path.""" reenumerated = ListPortInfo("/dev/ttyACM1") reenumerated.location = "1-2.3" reenumerated.serial_number = "FC-123" monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [reenumerated]) - def unexpected_by_path_probe(_device: str) -> str: - message = "Linux by-path fallback must not run after a USB identity match" - raise AssertionError(message) - - monkeypatch.setattr(bl, "_capture_linux_persistent_path", unexpected_by_path_probe) identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123") assert bl.resolve_bootloader_device("/dev/ttyACM0", identity) == "/dev/ttyACM1" @@ -504,7 +1024,6 @@ def unexpected_by_path_probe(_device: str) -> str: def test_bootloader_port_prefers_usb_identity_over_reused_linux_device_path(monkeypatch: pytest.MonkeyPatch) -> None: """A newly assigned tty path must not override the captured controller identity.""" - monkeypatch.setattr(bl, "sys_platform", "linux") reenumerated = ListPortInfo("/dev/ttyACM1") reenumerated.location = "1-2.3" reenumerated.serial_number = "FC-123" @@ -516,18 +1035,29 @@ def test_bootloader_port_prefers_usb_identity_over_reused_linux_device_path(monk ) -def test_bootloader_port_retains_linux_by_path_without_usb_metadata(monkeypatch: pytest.MonkeyPatch) -> None: - """A by-path fallback captured before reboot must not be resolved again afterward.""" - monkeypatch.setattr(bl, "sys_platform", "linux") - port = ListPortInfo("/dev/ttyACM0") - monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [port]) - persistent_path = "/dev/serial/by-path/usb-controller-a" - monkeypatch.setattr(bl, "_capture_linux_persistent_path", lambda _device: persistent_path) +def test_bootloader_upload_requires_usb_metadata(monkeypatch: pytest.MonkeyPatch) -> None: + """The upload gate rejects a connection with no stable USB metadata.""" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", list) identity = bl.capture_serial_device_identity("/dev/ttyACM0") - assert identity == bl.SerialDeviceIdentity(persistent_path=persistent_path) - assert bl.resolve_bootloader_device("/dev/ttyACM0", identity) == persistent_path + assert identity is None + assert not bl.has_stable_bootloader_device_identity("/dev/ttyACM0", identity) + + +def test_bootloader_port_refuses_a_different_usb_serial(monkeypatch: pytest.MonkeyPatch) -> None: + """A different USB serial number cannot satisfy the captured identity.""" + replacement = ListPortInfo("/dev/ttyACM1") + replacement.location = "1-2.3" + replacement.serial_number = "OTHER" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [replacement]) + identity = bl.SerialDeviceIdentity( + location="1-2.3", + serial_number="ORIGINAL", + ) + + with pytest.raises(OSError, match="cannot uniquely"): + bl.resolve_bootloader_device("/dev/ttyACM0", identity) def test_client_reports_recovery_error_when_pre_erase_reboot_fails() -> None: @@ -549,6 +1079,73 @@ def write(self, data: bytes) -> int: assert transport.closed +def test_abort_before_erase_requires_a_successful_reboot_ack() -> None: + class FailedRebootTransport(FakeBootloaderTransport): + """Reject the best-effort reboot command instead of merely accepting its write.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.REBOOT: + self._reply.extend(bl.INSYNC + bl.FAILED) + return len(data) + return super().write(data) + + assert not bl.BootloaderClient(FailedRebootTransport()).abort_before_erase() + + +def test_client_accepts_no_ack_reboot_when_pre_erase_abort_uses_protocol_v2() -> None: + class V2NoAckRebootTransport(FakeBootloaderTransport): + """Protocol revision two reboots immediately and does not acknowledge it.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.REBOOT: + self.rebooted = True + self.commands.append(bl.REBOOT) + self.requests.append(data) + return len(data) + return super().write(data) + + transport = V2NoAckRebootTransport(revision=2) + + with pytest.raises(fw.FirmwareUploadCancelledError, match="not confirmed"): + bl.BootloaderClient(transport).upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: False) + + assert transport.rebooted + assert transport.closed + + +def test_close_failure_does_not_mask_upload_failure() -> None: + class CloseFailureTransport(FakeBootloaderTransport): + """Raise while closing so cleanup cannot hide the protocol failure.""" + + def close(self) -> None: + msg = "close failed" + raise RuntimeError(msg) + + def write(self, data: bytes) -> int: + if data[:1] == bl.PROG_MULTI: + msg = "programming write failed" + raise OSError(msg) + return super().write(data) + + with pytest.raises(fw.BootloaderProtocolError, match="programming write failed"): + bl.BootloaderClient(CloseFailureTransport()).upload( + fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True + ) + + +def test_full_erase_is_refused_before_any_flash_command() -> None: + """The client refuses full erase until it can determine the board capability safely.""" + transport = FakeBootloaderTransport() + + with pytest.raises(fw.FirmwareCompatibilityError, match="full firmware erase"): + bl.BootloaderClient(transport).upload( + fw.parse_apj(apj(b"abcd")), full_erase=True, confirmation_requested=lambda *_args: True + ) + + assert bl.CHIP_FULL_ERASE not in transport.commands + assert transport.flash == b"" + + def test_backend_rejects_invalid_timeout_before_entering_bootloader() -> None: entered = False @@ -667,64 +1264,142 @@ def test_facade_reconnects_using_the_reenumerated_serial_device(tmp_path: Path, assert params.cleared == 2 -def test_facade_waits_for_serial_identity_to_reappear_before_reconnecting( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """ - Reconnection waits while the board is temporarily absent during re-enumeration. +def test_facade_routes_bootloader_and_entry_delays_to_injected_sleep(tmp_path: Path) -> None: + sleeps: list[float] = [] + + class EmptyOnceTransport(FakeBootloaderTransport): + """Return one empty read before providing the bootloader response.""" + + empty_reads = 1 + + def read(self, size: int = 1) -> bytes: + if self.empty_reads: + self.empty_reads -= 1 + return b"" + return super().read(size) - GIVEN: The stable USB identity is unavailable during the first resolver calls - WHEN: The firmware upload completes and starts reconnection - THEN: The facade retries identity resolution and connects after the board reappears - """ connection = _Connection() - commands = _Commands() controller = FlightController( connection_manager=connection, # type: ignore[arg-type] params_manager=_Params(), # type: ignore[arg-type] - commands_manager=commands, # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] files_manager=object(), # type: ignore[arg-type] + sleep=sleeps.append, ) path = tmp_path / "firmware.apj" path.write_bytes(apj(b"abcd")) - resolver_calls = 0 - - def resolve(_device: str, _identity: object) -> str: - nonlocal resolver_calls - resolver_calls += 1 - if resolver_calls < 3: - msg = "device is still re-enumerating" - raise OSError(msg) - return "COM13" - - monkeypatch.setattr( - "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", - resolve, - ) controller.upload_apj_firmware( path, expected_firmware_sha256=trusted_digest(b"abcd"), - serial_factory=lambda *_args: FakeBootloaderTransport(), + serial_factory=lambda *_args: EmptyOnceTransport(), confirmation_requested=lambda *_args: True, ) - assert resolver_calls == 3 - assert connection.reconnect_device == "COM13" + assert sleeps == [0.3, 0.01] -def test_facade_retries_reconnect_when_connect_returns_an_error(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: +def test_facade_verifies_board_identity_after_reconnect(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: """ - Reconnection keeps trying while connect() reports a transient failure. + A successful serial reconnect still requires the expected APJ board identity. - GIVEN: resolve_bootloader_device succeeds at once but the board is not open yet - WHEN: connect() returns an error string on the first attempts then an empty string - THEN: The facade spends the retry budget and reports success once connect() succeeds + GIVEN: The bootloader flashes successfully but the reconnected MAVLink board is different + WHEN: The facade completes the reconnect step + THEN: It rejects the result with a firmware identity error """ - connect_results = iter(["port not ready", "port not ready", ""]) - class _FlakyConnection(_Connection): - def connect(self, device: str, **kwargs: object) -> str: + class WrongBoardAfterReconnect(_Connection): + """Change the reported board identity when the new MAVLink connection opens.""" + + def create_connection_with_retry(self, *_args: object, **kwargs: object) -> str: + result = super().create_connection_with_retry(*_args, **kwargs) + self.info.apj_board_id = "42" + return result + + connection = WrongBoardAfterReconnect() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", + lambda _device, _identity: "COM13", + ) + + with pytest.raises(fw.FirmwareIdentityError, match="does not match firmware"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + assert connection.reconnected + assert connection.reconnect_device == "COM13" + + +def test_facade_waits_for_serial_identity_to_reappear_before_reconnecting( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ + Reconnection waits while the board is temporarily absent during re-enumeration. + + GIVEN: The stable USB identity is unavailable during the first resolver calls + WHEN: The firmware upload completes and starts reconnection + THEN: The facade retries identity resolution and connects after the board reappears + """ + connection = _Connection() + commands = _Commands() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=commands, # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + resolver_calls = 0 + + def resolve(_device: str, _identity: object) -> str: + nonlocal resolver_calls + resolver_calls += 1 + if resolver_calls < 3: + msg = "device is still re-enumerating" + raise OSError(msg) + return "COM13" + + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", + resolve, + ) + + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + assert resolver_calls == 3 + assert connection.reconnect_device == "COM13" + + +def test_facade_retries_reconnect_when_connect_returns_an_error(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """ + Reconnection keeps trying while connect() reports a transient failure. + + GIVEN: resolve_bootloader_device succeeds at once but the board is not open yet + WHEN: connect() returns an error string on the first attempts then an empty string + THEN: The facade spends the retry budget and reports success once connect() succeeds + """ + connect_results = iter(["port not ready", "port not ready", ""]) + + class _FlakyConnection(_Connection): + def connect(self, device: str, **kwargs: object) -> str: self.reconnect_device = device self.comport_device = device self.master = _Master() @@ -758,6 +1433,66 @@ def connect(self, device: str, **kwargs: object) -> str: assert connection.reconnect_device == "COM13" +def test_facade_reports_reconnect_retry_exhaustion_after_a_verified_flash( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ + Reconnect failures consume the bounded retry window and preserve the recovery message. + + GIVEN: The bootloader flash succeeds but every MAVLink reconnect attempt fails + WHEN: The facade exhausts its reconnect deadline + THEN: It reports that the verified controller must be reconnected manually + """ + now = 0.0 + connect_calls = 0 + + def clock() -> float: + return now + + def sleep(delay: float) -> None: + nonlocal now + now += delay + + class NeverReadyConnection(_Connection): + """Keep reporting a transient connection error until the retry deadline.""" + + def connect(self, device: str, **kwargs: object) -> str: + nonlocal connect_calls + connect_calls += 1 + self.reconnect_device = device + self.comport_device = device + self.master = _Master() + return "port not ready" + + monkeypatch.setattr("ardupilot_methodic_configurator.backend_flightcontroller.FIRMWARE_RECONNECT_RESOLVE_TIMEOUT", 0.25) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.resolve_bootloader_device", + lambda _device, _identity: "COM13", + ) + connection = NeverReadyConnection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + sleep=sleep, + clock=clock, + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + + with pytest.raises(fw.FirmwareReconnectError, match=r"written and verified.*Reconnect it manually"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + assert connect_calls == 3 + assert now == pytest.approx(0.6) + + def test_facade_reports_rejected_bootloader_entry_without_releasing_serial(tmp_path: Path) -> None: """ A rejected bootloader command stops the upload before disconnecting MAVLink. @@ -809,6 +1544,57 @@ def test_facade_requires_trusted_sha_before_flashing(tmp_path: Path) -> None: assert not connection.disconnected +def test_facade_validates_the_trusted_digest_before_bootloader_entry(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """The facade must not allow a selected APJ to bypass its release digest.""" + calls: list[str] = [] + connection = _Connection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.verify_expected_firmware_digest", + lambda _image, digest: calls.append(digest), + ) + + controller.upload_apj_firmware( + path, + expected_firmware_sha256="0" * 64, + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + assert calls == ["0" * 64] + + +def test_facade_reports_a_verified_flash_when_automatic_reconnect_fails( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """A post-flash port ambiguity must tell the user to reconnect manually.""" + connection = _Connection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr("ardupilot_methodic_configurator.backend_flightcontroller.FIRMWARE_RECONNECT_RESOLVE_TIMEOUT", 0) + + with pytest.raises(fw.FirmwareReconnectError, match=r"written and verified.*Reconnect it manually"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + serial_factory=lambda *_args: FakeBootloaderTransport(), + confirmation_requested=lambda *_args: True, + ) + + def test_facade_refuses_network_mavlink_connection() -> None: connection = _Connection("udp:127.0.0.1:14550") controller = FlightController( @@ -844,6 +1630,32 @@ def test_facade_requires_pre_reboot_board_identity(tmp_path: Path) -> None: assert not connection.master.held_in_bootloader +def test_facade_requires_stable_usb_identity_before_bootloader_entry(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """An unidentifiable serial connection must not be rebooted into the bootloader.""" + connection = _Connection() + controller = FlightController( + connection_manager=connection, # type: ignore[arg-type] + params_manager=_Params(), # type: ignore[arg-type] + commands_manager=_Commands(), # type: ignore[arg-type] + files_manager=object(), # type: ignore[arg-type] + ) + path = tmp_path / "firmware.apj" + path.write_bytes(apj(b"abcd")) + monkeypatch.setattr( + "ardupilot_methodic_configurator.backend_flightcontroller.has_stable_bootloader_device_identity", + bl.has_stable_bootloader_device_identity, + ) + + with pytest.raises(fw.FirmwareConnectionError, match="stable USB"): + controller.upload_apj_firmware( + path, + expected_firmware_sha256=trusted_digest(b"abcd"), + confirmation_requested=lambda *_args: True, + ) + + assert not connection.master.held_in_bootloader + + def test_backend_retries_serial_open_after_bootloader_entry() -> None: image = fw.parse_apj(apj(b"abcd")) attempts = 0 @@ -880,6 +1692,262 @@ def open_transport(*_args: object) -> bl.BootloaderTransport: assert delays == [0.25] +def test_backend_requires_manual_recovery_after_all_serial_open_retries_fail() -> None: + """ + Exhausting the bootloader port-open budget is reported as a recovery action. + + GIVEN: The controller entered bootloader mode but its serial port never opens + WHEN: Every configured serial-open attempt fails + THEN: The backend reports all attempts and asks the user to power-cycle the controller + """ + attempts = 0 + delays: list[float] = [] + + def enter() -> None: + pass + + def open_transport(*_args: object) -> bl.BootloaderTransport: + nonlocal attempts + attempts += 1 + msg = "bootloader port is still absent" + raise OSError(msg) + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + enter_bootloader=enter, + serial_factory=open_transport, + open_retries=3, + retry_delay=0.25, + sleep=delays.append, + ) + + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match="power-cycle") as error: + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert attempts == 3 + assert delays == [0.25, 0.25] + assert "bootloader port is still absent" in str(error.value) + + +def test_backend_clamps_final_retry_sleep_to_remaining_budget() -> None: + delays: list[float] = [] + now = 0.0 + + def clock() -> float: + return now + + def sleep(delay: float) -> None: + nonlocal now + delays.append(delay) + now += delay + + def open_transport(*_args: object) -> bl.BootloaderTransport: + msg = "bootloader port is still absent" + raise OSError(msg) + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + serial_factory=open_transport, + open_retries=10, + retry_delay=10.0, + clock=clock, + sleep=sleep, + ) + + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match="power-cycle"): + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert delays == [10.0, 5.0] + assert now == bl.BOOTLOADER_ENUMERATION_TIMEOUT + + +def test_backend_honours_cancellation_during_bootloader_discovery() -> None: + attempts = 0 + + def enter() -> None: + pass + + def open_transport(*_args: object) -> bl.BootloaderTransport: + nonlocal attempts + attempts += 1 + msg = "bootloader port is still absent" + raise OSError(msg) + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + enter_bootloader=enter, + serial_factory=open_transport, + open_retries=3, + retry_delay=0.25, + sleep=lambda _delay: None, + ) + + checks = iter([False, False, True]) + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match=r"cancelled.*power-cycle"): + backend.upload( + fw.parse_apj(apj(b"abcd")), + cancellation_requested=lambda: next(checks), + confirmation_requested=lambda *_args: True, + ) + + assert attempts == 1 + + +def test_backend_default_open_retry_budget_covers_bootloader_enumeration() -> None: + backend = bl.FlightControllerBootloaderBackend("COM7", 115200) + + assert backend._open_retries == bl.BOOTLOADER_OPEN_RETRIES # pylint: disable=protected-access + assert bl.BOOTLOADER_ENUMERATION_TIMEOUT >= 10.0 + assert backend._open_retries * bl.BOOTLOADER_RETRY_DELAY >= bl.BOOTLOADER_ENUMERATION_TIMEOUT # pylint: disable=protected-access + + +def test_backend_bootloader_discovery_uses_a_wall_clock_budget() -> None: + now = 0.0 + attempts = 0 + timeouts: list[float] = [] + + class EmptyTransport(FakeBootloaderTransport): + """Never answer identification, as with a serial port in the wrong mode.""" + + def read(self, size: int = 1) -> bytes: + del size + return b"" + + def clock() -> float: + return now + + def sleep(delay: float) -> None: + nonlocal now + now += delay + + def open_transport(_device: str, _baudrate: int, timeout: float) -> bl.BootloaderTransport: + nonlocal attempts + attempts += 1 + timeouts.append(timeout) + return EmptyTransport() + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + serial_factory=open_transport, + open_retries=100, + timeout=2.0, + retry_delay=0.7, + clock=clock, + sleep=sleep, + ) + + with pytest.raises(fw.FirmwareBootloaderRecoveryError, match=r"could not confirm reboot.*power-cycle"): + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + # The final identification attempt starts with only 1.5 seconds remaining, + # and the best-effort reboot gets its own bounded recovery budget. + assert now == pytest.approx(bl.BOOTLOADER_ENUMERATION_TIMEOUT + bl.BOOTLOADER_ABORT_TIMEOUT) + assert attempts == 6 + assert timeouts[-1] == pytest.approx(1.5) + + +def test_backend_retries_after_a_silent_port_until_the_bootloader_appears() -> None: + """A silent serial port must not consume the entire discovery deadline.""" + now = 0.0 + attempts = 0 + + class SilentTransport(FakeBootloaderTransport): + """Accept opens but never answer bootloader identification.""" + + def read(self, size: int = 1) -> bytes: + del size + return b"" + + def clock() -> float: + return now + + def sleep(delay: float) -> None: + nonlocal now + now += delay + + def open_transport(_device: str, _baudrate: int, _timeout: float) -> bl.BootloaderTransport: + nonlocal attempts + attempts += 1 + return SilentTransport() if now < 3.0 else FakeBootloaderTransport() + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + serial_factory=open_transport, + open_retries=10, + timeout=2.0, + retry_delay=0.5, + clock=clock, + sleep=sleep, + ) + + info = backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert info.board_id == 9 + assert attempts == 3 + assert now == pytest.approx(5.0) + + +def test_backend_keeps_command_timeout_when_final_open_attempt_is_clamped(monkeypatch: pytest.MonkeyPatch) -> None: + """A short final discovery attempt must not shorten later bootloader commands.""" + now = 0.0 + attempts = 0 + open_timeouts: list[float] = [] + client_timeouts: list[float] = [] + info = bl.BootloaderInfo(5, 9, 0, 2048, 1024) + + def clock() -> float: + return now + + def sleep(delay: float) -> None: + nonlocal now + now += delay + + def open_transport(_device: str, _baudrate: int, timeout: float) -> object: + nonlocal attempts + attempts += 1 + open_timeouts.append(timeout) + if attempts == 1: + msg = "bootloader port has not appeared yet" + raise OSError(msg) + return object() + + class StubBootloaderClient: + """Record discovery construction without requiring protocol traffic.""" + + def __init__(self, _transport: object, *, timeout: float, **_kwargs: object) -> None: + client_timeouts.append(timeout) + + def identify(self, *, deadline: float | None = None) -> bl.BootloaderInfo: + del deadline + return info + + def upload(self, *_args: object, **_kwargs: object) -> bl.BootloaderInfo: + return info + + monkeypatch.setattr(bl, "BootloaderClient", StubBootloaderClient) + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + serial_factory=open_transport, + open_retries=2, + timeout=2.0, + retry_delay=14.0, + clock=clock, + sleep=sleep, + ) + + result = backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert result is info + assert open_timeouts == [2.0, 1.0] + assert client_timeouts == [2.0] + + def test_backend_requires_manual_recovery_when_the_held_bootloader_never_opens() -> None: entered = False @@ -950,23 +2018,56 @@ def enter() -> None: assert not stale.rebooted +def test_backend_restores_command_timeout_after_discovery() -> None: + class TimeoutAwareTransport(FakeBootloaderTransport): + """Expose pyserial-like timeout attributes for the restoration check.""" + + def __init__(self) -> None: + super().__init__() + self.timeout = 0.1 + self.write_timeout = 0.1 + + transport = TimeoutAwareTransport() + + backend = bl.FlightControllerBootloaderBackend( + "COM7", + 115200, + timeout=3.0, + serial_factory=lambda *_args: transport, + open_retries=1, + ) + + backend.upload(fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True) + + assert transport.timeout == 3.0 + assert transport.write_timeout == 3.0 + + def test_upload_reports_protocol_stages_and_confirmation_boundary() -> None: - events: list[bl.UploadStage] = [] + events: list[tuple[bl.UploadStage, int, int]] = [] confirmed: list[fw.BootloaderInfo] = [] transport = FakeBootloaderTransport() bl.BootloaderClient(transport).upload( fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda _image, info: confirmed.append(info) is None, - progress_callback=lambda stage, _done, _total: events.append(stage), + progress_callback=lambda stage, done, total: events.append((stage, done, total)), ) - assert confirmed - assert fw.UploadStage.AWAITING_CONFIRMATION in events - assert fw.UploadStage.ERASING in events - assert fw.UploadStage.PROGRAMMING in events - assert fw.UploadStage.VERIFYING in events - assert fw.UploadStage.REBOOTING in events + assert confirmed == [fw.BootloaderInfo(5, 9, 0, 2048, 1024)] + assert events == [ + (fw.UploadStage.IDENTIFYING, 0, 1), + (fw.UploadStage.IDENTIFYING, 1, 1), + (fw.UploadStage.AWAITING_CONFIRMATION, 0, 1), + (fw.UploadStage.AWAITING_CONFIRMATION, 1, 1), + (fw.UploadStage.ERASING, 0, 1), + (fw.UploadStage.ERASING, 1, 1), + (fw.UploadStage.PROGRAMMING, 1, 1), + (fw.UploadStage.VERIFYING, 0, 1), + (fw.UploadStage.VERIFYING, 1, 1), + (fw.UploadStage.REBOOTING, 0, 1), + (fw.UploadStage.REBOOTING, 1, 1), + ] def test_verify_failure_reports_the_verifying_stage() -> None: @@ -1161,7 +2262,7 @@ def write(self, data: bytes) -> int: path = tmp_path / "firmware.apj" path.write_bytes(apj(b"abcd")) - with pytest.raises(fw.BootloaderProtocolError, match="programming write failed"): + with pytest.raises(fw.BootloaderProtocolError, match=r"programming write failed.*incomplete.*power-cycle"): controller.upload_apj_firmware( path, expected_firmware_sha256=trusted_digest(b"abcd"), @@ -1171,3 +2272,28 @@ def write(self, data: bytes) -> int: assert not transport.rebooted assert not connection.reconnected + + +def test_client_preserves_recovery_guidance_for_empty_argument_exceptions() -> None: + class EmptyMessageFailureTransport(FakeBootloaderTransport): + """Raise an exception with no message during programming.""" + + def write(self, data: bytes) -> int: + if data[:1] == bl.PROG_MULTI: + raise RuntimeError + return super().write(data) + + with pytest.raises(RuntimeError, match="power-cycle"): + bl.BootloaderClient(EmptyMessageFailureTransport()).upload( + fw.parse_apj(apj(b"abcd")), confirmation_requested=lambda *_args: True + ) + + +def test_recovery_guidance_preserves_oserror_text_and_errno() -> None: + error = OSError(5, "Input/output error") + + bl._append_recovery_message(error, "power-cycle the flight controller") # pylint: disable=protected-access + + assert error.errno == 5 + assert error.strerror == "Input/output error; power-cycle the flight controller" + assert str(error) == "[Errno 5] Input/output error; power-cycle the flight controller" diff --git a/tests/test_bootloader_identity.py b/tests/test_bootloader_identity.py index 7f962b1fa..531c44067 100644 --- a/tests/test_bootloader_identity.py +++ b/tests/test_bootloader_identity.py @@ -10,6 +10,8 @@ # ruff: noqa: INP001 +from pathlib import Path + import pytest from serial.tools.list_ports_common import ListPortInfo @@ -51,3 +53,130 @@ def test_bootloader_port_refuses_an_ambiguous_macos_dual_cdc_match(monkeypatch: with pytest.raises(OSError, match="cannot uniquely"): bl.resolve_bootloader_device("/dev/cu.usbmodem14101", identity) + + +def test_bootloader_port_matches_linux_interface_qualified_location(monkeypatch: pytest.MonkeyPatch) -> None: + """A second Linux CDC interface still maps to the same bootloader device.""" + bootloader_port = ListPortInfo("/dev/ttyACM0") + bootloader_port.location = "1-2.3:1.0" + bootloader_port.serial_number = "FC-123" + bootloader_port.interface = None + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [bootloader_port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3:1.2", serial_number="FC-123") + + assert bl.resolve_bootloader_device("/dev/ttyACM1", identity) == "/dev/ttyACM0" + + +def test_application_port_prefers_an_exact_interface_qualified_location(monkeypatch: pytest.MonkeyPatch) -> None: + """A dual-CDC application reconnects to the originally selected port.""" + mavlink_port = ListPortInfo("/dev/ttyACM0") + mavlink_port.location = "1-2.3:1.0" + mavlink_port.serial_number = "FC-123" + mavlink_port.interface = None + console_port = ListPortInfo("/dev/ttyACM1") + console_port.location = "1-2.3:1.2" + console_port.serial_number = "FC-123" + console_port.interface = None + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [mavlink_port, console_port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3:1.0", serial_number="FC-123") + + assert bl.resolve_bootloader_device("/dev/ttyACM0", identity) == "/dev/ttyACM0" + + +def test_bootloader_port_refuses_identical_macos_dual_cdc_interfaces(monkeypatch: pytest.MonkeyPatch) -> None: + """Identical CDC metadata must remain fail-closed after reboot.""" + first_port = ListPortInfo("/dev/cu.usbmodem14101") + first_port.location = "1-2.3" + first_port.serial_number = "FC-123" + first_port.interface = "0" + second_port = ListPortInfo("/dev/cu.usbmodem14102") + second_port.location = "1-2.3" + second_port.serial_number = "FC-123" + second_port.interface = "0" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [first_port, second_port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123", interface="0") + + with pytest.raises(OSError, match="cannot uniquely"): + bl.resolve_bootloader_device("/dev/cu.usbmodem14101", identity) + + +def test_bootloader_port_selects_the_captured_macos_dual_cdc_interface(monkeypatch: pytest.MonkeyPatch) -> None: + """A captured CDC interface disambiguates the two ports exposed by macOS.""" + mavlink_port = ListPortInfo("/dev/cu.usbmodem14101") + mavlink_port.location = "1-2.3" + mavlink_port.serial_number = "FC-123" + mavlink_port.interface = "0" + console_port = ListPortInfo("/dev/cu.usbmodem14102") + console_port.location = "1-2.3" + console_port.serial_number = "FC-123" + console_port.interface = "2" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [mavlink_port, console_port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123", interface="0") + + assert bl.resolve_bootloader_device("/dev/cu.usbmodem14101", identity) == "/dev/cu.usbmodem14101" + + +def test_bootloader_port_accepts_a_sole_match_when_its_interface_changes(monkeypatch: pytest.MonkeyPatch) -> None: + """The application interface must not exclude the sole bootloader candidate.""" + port = ListPortInfo("/dev/cu.usbmodem14101") + port.location = "1-2.3" + port.serial_number = "FC-123" + port.interface = "Bootloader" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [port]) + + identity = bl.SerialDeviceIdentity(location="1-2.3", serial_number="FC-123", interface="ArduPilot") + + assert bl.resolve_bootloader_device("/dev/cu.usbmodem14101", identity) == "/dev/cu.usbmodem14101" + + +def test_bootloader_port_refuses_duplicate_serials_on_different_devices_despite_interface_match( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """An interface name must not select one of several physical USB devices.""" + bootloader_port = ListPortInfo("/dev/ttyACM0") + bootloader_port.serial_number = "DUP" + bootloader_port.location = "1-2.3" + bootloader_port.interface = "Bootloader" + other_port = ListPortInfo("/dev/ttyACM1") + other_port.serial_number = "DUP" + other_port.location = "1-2.4" + other_port.interface = "ArduPilot" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [bootloader_port, other_port]) + + identity = bl.SerialDeviceIdentity(serial_number="DUP", interface="ArduPilot") + + with pytest.raises(OSError, match="cannot uniquely"): + bl.resolve_bootloader_device("/dev/ttyACM0", identity) + + +def test_capture_serial_identity_follows_a_symlinked_device_path(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> None: + """USB metadata is retained when the selected port is a symlink such as by-id.""" + device = tmp_path / "ttyACM0" + device.touch() + device_link = tmp_path / "usb-ArduPilot" + device_link.symlink_to(device) + port = ListPortInfo(str(device)) + port.location = "1-2.3" + port.serial_number = "FC-123" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [port]) + + assert bl.capture_serial_device_identity(str(device_link)) == bl.SerialDeviceIdentity( + location="1-2.3", serial_number="FC-123" + ) + + +def test_capture_serial_identity_preserves_macos_cdc_interface(monkeypatch: pytest.MonkeyPatch) -> None: + """The selected macOS CDC interface is retained for post-reboot resolution.""" + port = ListPortInfo("/dev/cu.usbmodem14101") + port.location = "1-2.3" + port.serial_number = "FC-123" + port.interface = "0" + monkeypatch.setattr(bl.serial.tools.list_ports, "comports", lambda: [port]) + + assert bl.capture_serial_device_identity(port.device) == bl.SerialDeviceIdentity( + location="1-2.3", serial_number="FC-123", interface="0" + ) diff --git a/tests/test_bootloader_partial_read.py b/tests/test_bootloader_partial_read.py index 619dc446b..c498b4f83 100644 --- a/tests/test_bootloader_partial_read.py +++ b/tests/test_bootloader_partial_read.py @@ -27,14 +27,19 @@ def test_read_timeout_is_bounded_when_partial_reads_arrive_after_deadline() -> N class PartialReadTransport: # pylint: disable=too-few-public-methods """Return one byte per read to exercise the response deadline.""" + reads = 0 + def read(self, _size: int = 1) -> bytes: + self.reads += 1 return b"x" - clock_values = iter([0.0, 1.0]) - client = bl.BootloaderClient(PartialReadTransport(), timeout=1.0, clock=lambda: next(clock_values)) + transport = PartialReadTransport() + clock_values = iter([0.0, 0.5, 1.0]) + client = bl.BootloaderClient(transport, timeout=1.0, clock=lambda: next(clock_values)) with pytest.raises(bl.BootloaderProtocolError, match="timeout waiting"): client._read_exact(2) # pylint: disable=protected-access + assert transport.reads == 1 @pytest.mark.parametrize("arrival_time", [2.0, 2.05]) @@ -53,3 +58,30 @@ def read(self, _size: int = 1) -> bytes: client = bl.BootloaderClient(CompleteReadTransport(), timeout=2.0, clock=lambda: now) assert client._read_exact(2) == b"\x12\x10" # pylint: disable=protected-access + + +def test_empty_reads_yield_until_the_deadline() -> None: + """A non-blocking empty transport must not busy-spin while awaiting bytes.""" + now = 0.0 + sleeps: list[float] = [] + + class EmptyReadTransport: # pylint: disable=too-few-public-methods + """Always report no data without blocking.""" + + def read(self, _size: int = 1) -> bytes: + return b"" + + def clock() -> float: + return now + + def sleep(delay: float) -> None: + nonlocal now + sleeps.append(delay) + now += delay + + client = bl.BootloaderClient(EmptyReadTransport(), timeout=0.025, clock=clock, sleep=sleep) + + with pytest.raises(bl.BootloaderProtocolError, match="timeout waiting"): + client._read_exact(1) # pylint: disable=protected-access + + assert sleeps == pytest.approx([0.01, 0.01, 0.005]) diff --git a/tests/test_data_model_firmware_upload.py b/tests/test_data_model_firmware_upload.py index 09c33f3ee..ace10c18a 100755 --- a/tests/test_data_model_firmware_upload.py +++ b/tests/test_data_model_firmware_upload.py @@ -60,8 +60,7 @@ def test_user_sees_metadata_from_a_valid_apj_file(self, tmp_path: Path, small_im assert image.metadata.board_id == 9 assert image.metadata.image_size == len(small_image) assert image.metadata.firmware_version == "4.6.0" - assert image.metadata.content_sha256 == fw.firmware_content_sha256(small_image) - assert image.content_sha256() == image.metadata.content_sha256 + assert image.metadata.board_revision is None assert image.metadata.apj_sha256 == hashlib.sha256((tmp_path / "arducopter.apj").read_bytes()).hexdigest() assert image.image == small_image + b"\xff" assert len(image.image) % 4 == 0 @@ -161,18 +160,38 @@ def test_crc_matches_the_reference_uploader_computation(self, tmp_path: Path, sm assert image.crc(flash_size) == expected - def test_crc_pads_to_an_unaligned_flash_size(self, tmp_path: Path) -> None: - """CRC padding covers exactly the remaining bytes for an unaligned flash size.""" + def test_crc_rejects_an_unaligned_flash_size(self, tmp_path: Path) -> None: + """CRC cannot safely represent an unaligned bootloader flash region.""" image = bootloader.load_apj(write_apj(tmp_path, b"abcd")) flash_size = 7 - assert image.crc(flash_size) == fw.bootloader_crc32(b"abcd\xff\xff\xff") + with pytest.raises(ValueError, match="multiple of four"): + image.crc(flash_size) def test_crc_matches_ardupilot_uploader_known_vector(self) -> None: """The uploader's raw CRC state is not Python's finalized binascii CRC-32.""" assert fw.bootloader_crc32(b"abc") == 0xCA6598D0 assert fw.bootloader_crc32(b"bc", fw.bootloader_crc32(b"a")) == 0xCA6598D0 + def test_external_crc_uses_only_the_declared_unpadded_payload(self, tmp_path: Path) -> None: + external_image = b"ext" + image = bootloader.load_apj( + write_apj( + tmp_path, + b"abcd", + extf_image_size=len(external_image), + extf_image=base64.b64encode(zlib.compress(external_image)).decode(), + ) + ) + + assert image.extf_image == external_image + b"\xff" + assert image.extf_crc() == fw.bootloader_crc32(external_image) + + def test_valid_board_revision_is_preserved(self, tmp_path: Path) -> None: + image = bootloader.load_apj(write_apj(tmp_path, b"abcd", board_revision=7)) + + assert image.metadata.board_revision == 7 + class TestCompatibility: """The model refuses to flash anything that does not match the detected board.""" @@ -203,6 +222,10 @@ def test_image_larger_than_flash_is_refused(self, image: fw.FirmwareImage) -> No with pytest.raises(fw.FirmwareCompatibilityError, match="exceeds flash"): fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 512)) + def test_unaligned_bootloader_flash_size_is_refused(self, image: fw.FirmwareImage) -> None: + with pytest.raises(fw.FirmwareCompatibilityError, match="invalid flash size"): + fw.check_compatibility(image, fw.BootloaderInfo(5, 9, 0, 513)) + def test_reconnected_firmware_board_identity_is_required(self, image: fw.FirmwareImage) -> None: with pytest.raises(fw.FirmwareIdentityError, match="did not report an APJ board_id"): fw.verify_reconnected_firmware(image, board_id="")