From 23eaed1ea34cc2a9bdc180e38bd38096c9023ccb Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:20:03 +0300 Subject: [PATCH 1/8] rockusb: speak Rockchip MaskROM and rockusb natively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rockchip's boot ROM has no UART download path at all — recovery is USB-only. That matters most for the case a test rig actually hits: an erased or half-written flash leaves no valid IDB, so the boot ROM abandons flash and enters MaskROM by itself at power-up. No button, no strap, no serial. Cutting power is enough to make the board recoverable, which is what makes unattended recovery possible. This does not go through the protocol/ registry or the Transport ABC. Every protocol there is a UART boot-ROM dialect over a byte stream; these two stages (vendor control transfers, then a Mass-Storage-shaped CBW/CSW bulk protocol) are not byte streams, and forcing the fit would have been a leaky abstraction. New sibling package instead, with no entry point. Framing is kept in pure functions so the fiddly parts are testable with no hardware and no libusb: - the 4096-byte chunk quirks, where a payload ending 4095 mod 4096 needs a pad byte before the CRC (else the CRC straddles a chunk boundary) and one ending 4094 needs a trailing short packet to close the transfer - the mixed endianness, where the command wrapper is little-endian but the address and count inside its CDB are big-endian - RKBOOT entry parsing, read backwards from the entry stride because emType is a C enum of ambiguous width and guessing wrong silently shifts every later offset The CRC is pinned to the standard CCITT-FALSE check value and cross-checked against an independent bitwise implementation, so the seed and bit order are proven rather than merely self-consistent. pyusb is an optional extra, imported lazily, so installs without it keep working for every UART SoC. Written from xboot/xrock (MIT) and rkflashtool's rkcrc.h (BSD-2). rkdeveloptool is GPL-2 and was read only to understand behaviour, never copied. Co-Authored-By: Claude Opus 4.8 --- pyproject.toml | 7 + src/defib/recovery/events.py | 4 + src/defib/rockusb/__init__.py | 59 ++++++ src/defib/rockusb/codec.py | 58 ++++++ src/defib/rockusb/device.py | 338 +++++++++++++++++++++++++++++++++ src/defib/rockusb/loader.py | 153 +++++++++++++++ src/defib/rockusb/maskrom.py | 86 +++++++++ src/defib/rockusb/protocol.py | 172 +++++++++++++++++ src/defib/rockusb/recovery.py | 196 +++++++++++++++++++ tests/test_rockusb_codec.py | 95 +++++++++ tests/test_rockusb_loader.py | 139 ++++++++++++++ tests/test_rockusb_maskrom.py | 108 +++++++++++ tests/test_rockusb_protocol.py | 145 ++++++++++++++ uv.lock | 15 +- 14 files changed, 1574 insertions(+), 1 deletion(-) create mode 100644 src/defib/rockusb/__init__.py create mode 100644 src/defib/rockusb/codec.py create mode 100644 src/defib/rockusb/device.py create mode 100644 src/defib/rockusb/loader.py create mode 100644 src/defib/rockusb/maskrom.py create mode 100644 src/defib/rockusb/protocol.py create mode 100644 src/defib/rockusb/recovery.py create mode 100644 tests/test_rockusb_codec.py create mode 100644 tests/test_rockusb_loader.py create mode 100644 tests/test_rockusb_maskrom.py create mode 100644 tests/test_rockusb_protocol.py diff --git a/pyproject.toml b/pyproject.toml index 19630c6..0bf9c80 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -28,6 +28,7 @@ dependencies = [ [project.optional-dependencies] tui = ["textual>=0.40"] web = ["fastapi>=0.100", "uvicorn>=0.20", "jinja2>=3.1"] +rockchip = ["pyusb>=1.2"] dev = [ "pytest>=7", "pytest-asyncio>=0.21", @@ -67,6 +68,12 @@ strict = true module = "defib.tui.*" disallow_subclassing_any = false +# pyusb ships no stubs and is an optional extra, so it is absent from the +# type-check environment entirely. +[[tool.mypy.overrides]] +module = ["usb", "usb.*"] +ignore_missing_imports = true + [tool.pytest.ini_options] asyncio_mode = "auto" testpaths = ["tests"] diff --git a/src/defib/recovery/events.py b/src/defib/recovery/events.py index 8fd30f8..f427acd 100644 --- a/src/defib/recovery/events.py +++ b/src/defib/recovery/events.py @@ -19,6 +19,10 @@ class Stage(str, Enum): AUX_AREA = "aux_area" BOOT_IMAGE = "boot_image" BOARD_ID = "board_id" + # Rockchip USB recovery: the usbplug upload that follows DDR_INIT, and the + # block writes it enables. + USBPLUG = "usbplug" + FLASH_WRITE = "flash_write" COMPLETE = "complete" diff --git a/src/defib/rockusb/__init__.py b/src/defib/rockusb/__init__.py new file mode 100644 index 0000000..d779849 --- /dev/null +++ b/src/defib/rockusb/__init__.py @@ -0,0 +1,59 @@ +"""Rockchip MaskROM / rockusb recovery over USB. + +This is a separate subsystem from :mod:`defib.protocol`, and deliberately so. +Every protocol in that package is a UART boot-ROM dialect layered on the +byte-stream :class:`~defib.transport.base.Transport`. Rockchip's boot ROM has +no UART download path at all — recovery is USB-only, and splits into two +stages that are not byte streams: + +1. **MaskROM** — vendor control transfers (``bRequest=0x0C``) push a DDR init + blob and then a "usbplug" blob into SRAM. See :mod:`.maskrom`. +2. **rockusb** — the usbplug then speaks a USB Mass-Storage-shaped bulk + protocol (CBW/CSW) with Rockchip opcodes. See :mod:`.protocol`. + +Because neither stage fits ``Transport``, none of this registers with the +``defib.protocols`` entry-point group. + +The payoff for a test rig: when SPI NAND holds no valid IDB — an erased or +half-written flash — the boot ROM falls into MaskROM *by itself* at power-up, +with no button press and no strap. Power-cycling the board is enough to make +it recoverable, which is what makes unattended recovery possible at all. + +Protocol details were derived from the MIT-licensed xboot/xrock and the +BSD-2-licensed rkflashtool. rkdeveloptool is GPL-2 and was **not** used as a +source for this implementation. +""" + +from __future__ import annotations + +from defib.rockusb.codec import RK_RC4_KEY, rc4, rk_crc16 +from defib.rockusb.loader import LoaderBlobs, LoaderFormatError, parse_loader +from defib.rockusb.maskrom import CODE_471, CODE_472, build_maskrom_chunks +from defib.rockusb.protocol import ( + SECTOR_SIZE, + CommandStatus, + Opcode, + ResetSubcode, + RockusbError, + build_cbw, + parse_csw, +) + +__all__ = [ + "CODE_471", + "CODE_472", + "RK_RC4_KEY", + "SECTOR_SIZE", + "CommandStatus", + "LoaderBlobs", + "LoaderFormatError", + "Opcode", + "ResetSubcode", + "RockusbError", + "build_cbw", + "build_maskrom_chunks", + "parse_csw", + "parse_loader", + "rc4", + "rk_crc16", +] diff --git a/src/defib/rockusb/codec.py b/src/defib/rockusb/codec.py new file mode 100644 index 0000000..50417ad --- /dev/null +++ b/src/defib/rockusb/codec.py @@ -0,0 +1,58 @@ +"""Wire codecs for the Rockchip MaskROM stage: CRC-16 and RC4. + +Both are needed only by :mod:`defib.rockusb.maskrom` — the rockusb bulk stage +carries no checksum of its own (USB already provides one) and is never +encrypted. +""" + +from __future__ import annotations + +from defib.protocol.crc import CRC_TABLE + +# Rockchip's boot ROM fixes this key for the MaskROM code path. Published in +# rkflashtool's README.maskrom as the argument to ``openssl rc4 -K``, and +# identical to the key constant in xboot/xrock. +RK_RC4_KEY = bytes( + [124, 78, 3, 4, 85, 5, 9, 7, 45, 44, 123, 56, 23, 13, 23, 17] +) + +# CRC-16/CCITT seed used by the MaskROM loader. Note this is *not* the same +# variant as :func:`defib.protocol.crc.calc_crc`, which HiSilicon seeds at 0 +# and finalises with two zero bytes. Only the polynomial table is shared. +CRC16_INIT = 0xFFFF + + +def rk_crc16(data: bytes | bytearray, crc: int = CRC16_INIT) -> int: + """CRC-16/CCITT as the Rockchip boot ROM computes it. + + Poly 0x1021, seeded 0xFFFF, MSB-first, no final XOR — matching + ``rkcrc16()`` in rkflashtool's BSD-2 ``rkcrc.h``:: + + crc = (crc << 8) ^ crc16table[(crc >> 8) ^ *buf++]; + """ + for byte in data: + crc = ((crc << 8) & 0xFFFF) ^ CRC_TABLE[((crc >> 8) ^ byte) & 0xFF] + return crc & 0xFFFF + + +def rc4(data: bytes | bytearray, key: bytes = RK_RC4_KEY) -> bytes: + """Plain RC4. Self-inverse, so this both encrypts and decrypts. + + Only used when a loader's header does *not* set the "RC4 disabled" flag. + Newer parts — RV1106 among them — ship loaders built with RC4 off, so this + path is normally dead for that SoC. + """ + s = list(range(256)) + j = 0 + for i in range(256): + j = (j + s[i] + key[i % len(key)]) & 0xFF + s[i], s[j] = s[j], s[i] + + out = bytearray(len(data)) + i = j = 0 + for n, byte in enumerate(data): + i = (i + 1) & 0xFF + j = (j + s[i]) & 0xFF + s[i], s[j] = s[j], s[i] + out[n] = byte ^ s[(s[i] + s[j]) & 0xFF] + return bytes(out) diff --git a/src/defib/rockusb/device.py b/src/defib/rockusb/device.py new file mode 100644 index 0000000..458b190 --- /dev/null +++ b/src/defib/rockusb/device.py @@ -0,0 +1,338 @@ +"""libusb plumbing for the two Rockchip USB stages. + +Kept deliberately thin: everything that can be decided without a device on the +bus lives in :mod:`.maskrom`, :mod:`.protocol` and :mod:`.loader`. What is +left here is enumeration, endpoint discovery and the blocking transfers, all +of which need real hardware. + +``pyusb`` is an optional dependency — install the ``rockchip`` extra. It is +imported lazily so that a defib install without it keeps working for every +UART-based SoC. +""" + +from __future__ import annotations + +import asyncio +import logging +import secrets +from dataclasses import dataclass +from enum import Enum +from typing import Any + +from defib.rockusb.protocol import ( + CSW_LENGTH, + CommandStatus, + Opcode, + RockusbError, + build_cbw, + parse_csw, +) + +logger = logging.getLogger(__name__) + +ROCKCHIP_VID = 0x2207 + +# The rockusb interface advertises itself as vendor-specific. MaskROM exposes +# a barer descriptor, so interface matching is best-effort and we fall back to +# scanning every interface for a bulk pair. +ROCKUSB_CLASS = 0xFF +ROCKUSB_SUBCLASS = 0x06 +ROCKUSB_PROTOCOL = 0x05 + + +class DeviceMode(str, Enum): + """Which of the two stages the device is currently answering.""" + + MASKROM = "maskrom" + LOADER = "loader" + + +class RockusbUsbError(RockusbError): + """USB-level failure: no device, cannot claim, transfer error.""" + + +def _require_usb() -> Any: + try: + import usb.core # noqa: PLC0415 + import usb.util # noqa: PLC0415 + except ImportError as e: # pragma: no cover - depends on install extras + raise RockusbUsbError( + "pyusb is required for Rockchip USB recovery — " + "install it with: pip install 'defib[rockchip]'" + ) from e + return usb + + +@dataclass +class FoundDevice: + """A Rockchip device on the bus, and which stage it is in.""" + + mode: DeviceMode + bus: int + address: int + product_id: int + handle: Any # usb.core.Device + + def __str__(self) -> str: + return ( + f"{self.mode.value} device {ROCKCHIP_VID:04x}:{self.product_id:04x} " + f"at bus {self.bus} addr {self.address}" + ) + + +def _classify(dev: Any) -> DeviceMode: + """MaskROM or loader? + + Both stages enumerate under the same VID:PID — on RV1106, ``2207:110c`` — + so the product id cannot be used. The boot ROM leaves the low bit of + ``bcdUSB`` clear where the usbplug sets it, which is the same discriminator + xrock relies on. + """ + return DeviceMode.MASKROM if not (dev.bcdUSB & 0x0001) else DeviceMode.LOADER + + +def find_device(product_id: int | None = None) -> FoundDevice | None: + """Return the first Rockchip device on the bus, or None.""" + usb = _require_usb() + + kwargs: dict[str, Any] = {"idVendor": ROCKCHIP_VID, "find_all": True} + if product_id is not None: + kwargs["idProduct"] = product_id + + for dev in usb.core.find(**kwargs): + return FoundDevice( + mode=_classify(dev), + bus=dev.bus, + address=dev.address, + product_id=dev.idProduct, + handle=dev, + ) + return None + + +async def wait_for_device( + timeout: float = 30.0, + mode: DeviceMode | None = None, + poll_interval: float = 0.25, +) -> FoundDevice: + """Poll until a matching device appears. + + Used both for the initial "power-cycle an erased board and catch it in + MaskROM" step and for the re-enumeration that follows the usbplug upload. + + Args: + timeout: seconds to keep looking. + mode: require this stage; None accepts either. + poll_interval: seconds between scans. + + Raises: + RockusbUsbError: if nothing matching shows up in time. + """ + loop = asyncio.get_event_loop() + deadline = loop.time() + timeout + seen: str | None = None + + while loop.time() < deadline: + found = await asyncio.to_thread(find_device) + if found is not None: + if mode is None or found.mode is mode: + return found + seen = str(found) + await asyncio.sleep(poll_interval) + + want = f" in {mode.value} mode" if mode else "" + if seen: + raise RockusbUsbError( + f"no Rockchip device{want} after {timeout:.0f}s — saw {seen} instead" + ) + raise RockusbUsbError( + f"no Rockchip device{want} after {timeout:.0f}s. " + "Power-cycle the board; an erased flash enters MaskROM on its own." + ) + + +class RockusbDevice: + """An opened Rockchip device: control transfers and the bulk command loop. + + Call :meth:`open` before use and :meth:`close` after, or use it as a + context manager. + """ + + def __init__(self, found: FoundDevice, timeout_ms: int = 5000) -> None: + self._found = found + self._dev = found.handle + self._timeout_ms = timeout_ms + self._interface: Any = None + self._ep_in: Any = None + self._ep_out: Any = None + self._detached = False + + @property + def mode(self) -> DeviceMode: + return self._found.mode + + def __enter__(self) -> RockusbDevice: + self.open() + return self + + def __exit__(self, *exc: object) -> None: + self.close() + + def open(self) -> None: + """Claim the interface and locate the bulk endpoints. + + Endpoints are read from the descriptor rather than hardcoded; the + conventional 0x02/0x81 pair is not guaranteed across usbplug builds. + """ + usb = _require_usb() + dev = self._dev + + try: + if dev.is_kernel_driver_active(0): + dev.detach_kernel_driver(0) + self._detached = True + except (NotImplementedError, usb.core.USBError): + # Not all backends/platforms implement this; only Linux binds a + # kernel driver here in the first place. + pass + + try: + dev.set_configuration() + except usb.core.USBError as e: + # Already configured is fine; anything else is not. + if e.errno not in (16, None): # EBUSY + raise RockusbUsbError(f"cannot configure {self._found}: {e}") from e + + cfg = dev.get_active_configuration() + for intf in cfg: + ep_out = usb.util.find_descriptor( + intf, + custom_match=lambda e: ( + usb.util.endpoint_direction(e.bEndpointAddress) + == usb.util.ENDPOINT_OUT + and usb.util.endpoint_type(e.bmAttributes) + == usb.util.ENDPOINT_TYPE_BULK + ), + ) + ep_in = usb.util.find_descriptor( + intf, + custom_match=lambda e: ( + usb.util.endpoint_direction(e.bEndpointAddress) + == usb.util.ENDPOINT_IN + and usb.util.endpoint_type(e.bmAttributes) + == usb.util.ENDPOINT_TYPE_BULK + ), + ) + if ep_in is not None and ep_out is not None: + self._interface, self._ep_in, self._ep_out = intf, ep_in, ep_out + break + + if self._interface is None: + # MaskROM legitimately has no bulk pair — control transfers only. + if self._found.mode is DeviceMode.MASKROM: + logger.debug("%s: no bulk endpoints, MaskROM control-only", self._found) + return + raise RockusbUsbError( + f"{self._found}: no bulk IN/OUT endpoint pair found" + ) + + try: + usb.util.claim_interface(dev, self._interface.bInterfaceNumber) + except usb.core.USBError as e: + raise RockusbUsbError( + f"cannot claim interface on {self._found}: {e} " + "(need a udev rule for 2207:* or root)" + ) from e + + def close(self) -> None: + usb = _require_usb() + try: + if self._interface is not None: + usb.util.release_interface(self._dev, self._interface.bInterfaceNumber) + usb.util.dispose_resources(self._dev) + if self._detached: + self._dev.attach_kernel_driver(0) + except Exception: # pragma: no cover - teardown is best-effort + logger.debug("cleanup failed for %s", self._found, exc_info=True) + + # -- MaskROM stage ---------------------------------------------------- + + def control_write(self, code: int, payload: bytes) -> int: + """One MaskROM code-upload control transfer.""" + try: + written = self._dev.ctrl_transfer( + bmRequestType=0x40, + bRequest=0x0C, + wValue=0x0000, + wIndex=code, + data_or_wLength=payload, + timeout=self._timeout_ms, + ) + return int(written) + except Exception as e: + raise RockusbUsbError( + f"MaskROM control transfer failed (code {code:#06x}, " + f"{len(payload)} bytes): {e}" + ) from e + + # -- rockusb bulk stage ----------------------------------------------- + + def _require_bulk(self) -> None: + if self._ep_in is None or self._ep_out is None: + raise RockusbUsbError( + f"{self._found}: bulk commands need the usbplug running — " + "device is still in MaskROM, send the loader first" + ) + + def command( + self, + opcode: Opcode | int, + *, + subcode: int = 0, + address: int = 0, + count: int = 0, + data_out: bytes | None = None, + read_length: int = 0, + ) -> bytes: + """Run one CBW / optional data phase / CSW exchange. + + Returns any data read during an IN transfer, otherwise ``b""``. + + Raises: + RockusbUsbError: on a transfer error or a non-zero status. + """ + self._require_bulk() + tag = secrets.randbits(32) + direction_in = read_length > 0 + transfer_length = read_length if direction_in else len(data_out or b"") + + cbw = build_cbw( + tag, + opcode, + subcode=subcode, + address=address, + count=count, + transfer_length=transfer_length, + direction_in=direction_in, + ) + + try: + self._ep_out.write(cbw, self._timeout_ms) + payload = b"" + if direction_in: + payload = bytes(self._ep_in.read(read_length, self._timeout_ms)) + elif data_out: + self._ep_out.write(data_out, self._timeout_ms) + csw = bytes(self._ep_in.read(CSW_LENGTH, self._timeout_ms)) + except Exception as e: + raise RockusbUsbError( + f"rockusb transfer failed (opcode {int(opcode):#04x}): {e}" + ) from e + + _, residue, status = parse_csw(csw, expected_tag=tag) + if status != CommandStatus.OK: + raise RockusbUsbError( + f"rockusb command {int(opcode):#04x} failed " + f"(status {status}, residue {residue})" + ) + return payload diff --git a/src/defib/rockusb/loader.py b/src/defib/rockusb/loader.py new file mode 100644 index 0000000..e8b9201 --- /dev/null +++ b/src/defib/rockusb/loader.py @@ -0,0 +1,153 @@ +"""Parse a Rockchip loader (``MiniLoaderAll.bin``) into its two SRAM blobs. + +The container is an ``RKBOOT`` archive: a fixed header, then tables of entries +pointing at blobs elsewhere in the same file. We only care about the 471 +(DDR init) and 472 (usbplug) tables. + +Not every loader Rockchip publishes has this container. The standalone +``rv1106_ddr_*.bin`` / ``rv1106_usbplug_*.bin`` files in rkbin are bare images +with no ``RKBOOT`` header at all, which is exactly why ``rkdeveloptool db`` +rejects them (rockchip-linux/rkdeveloptool#105). Those are handled by +:func:`raw_blobs`, which skips the container entirely — the same escape hatch +``xrock maskrom `` uses. +""" + +from __future__ import annotations + +import struct +from dataclasses import dataclass, field + +TAG_BOOT = b"BOOT" +TAG_LOADER = b"LDR " + +ENTRY_471 = 1 +ENTRY_472 = 2 +ENTRY_LOADER = 4 + +# dwDataOffset, dwDataSize, dwDataDelay — the last 12 bytes of every entry. +_ENTRY_TRAILER = 12 +_ENTRY_NAME_BYTES = 40 # WCHAR szName[20], UTF-16LE + + +class LoaderFormatError(Exception): + """The file is not a loader we know how to read.""" + + +@dataclass(frozen=True) +class LoaderEntry: + """One blob referenced by the loader's entry table.""" + + name: str + data: bytes + delay_ms: int + + +@dataclass(frozen=True) +class LoaderBlobs: + """What the MaskROM stage needs out of a loader file.""" + + ddr: list[LoaderEntry] = field(default_factory=list) + usbplug: list[LoaderEntry] = field(default_factory=list) + rc4_disabled: bool = True + + @property + def use_rc4(self) -> bool: + """Whether blobs must be RC4'd before upload.""" + return not self.rc4_disabled + + +def raw_blobs(ddr: bytes, usbplug: bytes, *, use_rc4: bool = False) -> LoaderBlobs: + """Wrap two bare images as if they had come out of a container. + + For RV1106 this is the normal path, since rkbin ships its DDR and usbplug + images headerless. + """ + return LoaderBlobs( + ddr=[LoaderEntry(name="ddr", data=ddr, delay_ms=0)], + usbplug=[LoaderEntry(name="usbplug", data=usbplug, delay_ms=0)], + rc4_disabled=not use_rc4, + ) + + +def _parse_entries( + data: bytes, offset: int, entry_size: int, count: int +) -> list[LoaderEntry]: + """Read ``count`` entries of ``entry_size`` bytes starting at ``offset``. + + The three fields we need sit at the *end* of each entry, so they are read + backwards from the entry stride rather than forwards from its start. That + sidesteps the one genuinely ambiguous field in the struct — ``emType`` is a + C enum, so whether it occupies 1 byte or 4 depends on how the producing + toolchain packed it, and getting it wrong would silently shift every + subsequent offset. + """ + if entry_size < _ENTRY_TRAILER + _ENTRY_NAME_BYTES: + raise LoaderFormatError( + f"entry size {entry_size} too small to hold a name plus offsets" + ) + + entries: list[LoaderEntry] = [] + for i in range(count): + base = offset + i * entry_size + if base + entry_size > len(data): + raise LoaderFormatError( + f"entry {i} at {base:#x} runs past end of file ({len(data)} bytes)" + ) + + name_at = base + entry_size - _ENTRY_TRAILER - _ENTRY_NAME_BYTES + name = ( + data[name_at : name_at + _ENTRY_NAME_BYTES] + .decode("utf-16-le", errors="replace") + .rstrip("\x00") + ) + data_offset, data_size, delay_ms = struct.unpack_from( + " len(data): + raise LoaderFormatError( + f"entry {name!r} blob at {data_offset:#x}+{data_size} " + f"runs past end of file ({len(data)} bytes)" + ) + entries.append( + LoaderEntry( + name=name, + data=data[data_offset : data_offset + data_size], + delay_ms=delay_ms, + ) + ) + return entries + + +def parse_loader(data: bytes) -> LoaderBlobs: + """Parse an ``RKBOOT`` container. + + Raises: + LoaderFormatError: if the magic is wrong or the tables do not fit. A + headerless rkbin image lands here — use :func:`raw_blobs` for those. + """ + if len(data) < 64: + raise LoaderFormatError(f"file too short to be a loader ({len(data)} bytes)") + if data[:4] not in (TAG_BOOT, TAG_LOADER): + raise LoaderFormatError( + f"expected {TAG_BOOT!r} or {TAG_LOADER!r} magic, got {data[:4]!r} — " + "a bare rkbin image? those have no container, pass them to raw_blobs()" + ) + + # Header layout up to the entry tables: + # uiTag[4] usSize[2] dwVersion[4] dwMergeVersion[4] + # STRUCT_RKTIME[7] emSupportChip[4] + # then the three (count, offset, size) triples. + table_at = 4 + 2 + 4 + 4 + 7 + 4 + + n471, off471, size471 = struct.unpack_from("device, device recipient) + bRequest = 0x0C + wValue = 0x0000 + wIndex = 0x0471 | 0x0472 + +``0x0471`` carries the DDR init blob, which runs in SRAM, brings up DRAM and +returns to the boot ROM. ``0x0472`` then carries the "usbplug" blob, which +takes over the USB device and starts answering the bulk protocol in +:mod:`defib.rockusb.protocol`. + +This module deliberately holds no USB code — :func:`build_maskrom_chunks` +turns a blob into the exact sequence of control-transfer payloads, so the +framing (which is where the fiddly parts live) is unit-testable with no +hardware and no libusb. +""" + +from __future__ import annotations + +import struct + +from defib.rockusb.codec import rc4, rk_crc16 + +CODE_471 = 0x0471 +CODE_472 = 0x0472 + +# Boot ROM accepts at most this much per control transfer. +CHUNK_SIZE = 4096 + + +def build_maskrom_chunks( + blob: bytes, + *, + use_rc4: bool = False, +) -> list[bytes]: + """Frame ``blob`` into the control-transfer payloads the boot ROM expects. + + The blob is optionally RC4'd, gets a big-endian CRC-16 appended, and is + then split into 4096-byte chunks. Two size-dependent quirks, both copied + from the reference implementations, are folded in here: + + * ``len(blob) % 4096 == 4095`` — a zero byte is appended *before* the CRC + is computed. Without it the two CRC bytes would straddle a chunk + boundary, which the boot ROM does not accept. + * ``len(blob) % 4096 == 4094`` — appending the CRC makes the payload an + exact multiple of the chunk size, leaving no short packet to signal the + end of the transfer. A trailing one-byte chunk is emitted to terminate + it. + + Args: + blob: raw DDR-init or usbplug image. + use_rc4: encrypt before checksumming. Loaders whose header sets the + "RC4 disabled" flag — which includes every RV1106 loader Rockchip + ships — must leave this off. + + Returns: + Payloads to send, in order, each as one control transfer. + """ + payload = bytearray(blob) + + remainder = len(payload) % CHUNK_SIZE + needs_terminator = remainder == CHUNK_SIZE - 2 + if remainder == CHUNK_SIZE - 1: + payload.append(0x00) + + if use_rc4: + # Whole-buffer, matching xrock. rkdeveloptool is widely read as + # encrypting per 4096-byte block instead; the two agree only when the + # blob is under one chunk. Untested here because RV1106 ships RC4-off + # loaders — verify against hardware before trusting this on a part + # that actually needs encryption. + payload = bytearray(rc4(bytes(payload))) + + payload += struct.pack(">H", rk_crc16(payload)) + + chunks = [ + bytes(payload[i : i + CHUNK_SIZE]) + for i in range(0, len(payload), CHUNK_SIZE) + ] + if needs_terminator: + chunks.append(b"\x00") + return chunks diff --git a/src/defib/rockusb/protocol.py b/src/defib/rockusb/protocol.py new file mode 100644 index 0000000..57fdd01 --- /dev/null +++ b/src/defib/rockusb/protocol.py @@ -0,0 +1,172 @@ +"""rockusb bulk stage: CBW/CSW framing and Rockchip opcodes. + +Once the usbplug is running the device speaks a dialect of USB Mass Storage +Bulk-Only Transport: a 31-byte command block wrapper out, an optional data +phase, then a 13-byte status wrapper back. The command descriptor block is +Rockchip's own, and — unlike the wrappers around it — is big-endian. + +As with :mod:`defib.rockusb.maskrom`, this module is pure framing so it can be +tested without libusb. +""" + +from __future__ import annotations + +import struct +from enum import IntEnum + +CBW_SIGNATURE = b"USBC" +CSW_SIGNATURE = b"USBS" + +CBW_LENGTH = 31 +CSW_LENGTH = 13 + +#: The usbplug presents flash as 512-byte logical blocks regardless of the +#: underlying medium; on SPI NAND it runs Rockchip's FTL underneath, so bad +#: blocks, wear levelling and ECC are all handled device-side. +SECTOR_SIZE = 512 + +#: Conservative ceiling for one READ_LBA/WRITE_LBA. The count field is 16-bit +#: so the protocol permits far more, but the usbplug's own buffering is +#: undocumented and the reference tools stay at or below this. +MAX_SECTORS_PER_TRANSFER = 512 + +DIRECTION_IN = 0x80 +DIRECTION_OUT = 0x00 + + +class Opcode(IntEnum): + """Rockchip CDB operation codes (only the ones we actually issue).""" + + TEST_UNIT_READY = 0x00 + READ_FLASH_ID = 0x01 + ERASE_NORMAL = 0x06 + READ_LBA = 0x14 + WRITE_LBA = 0x15 + READ_FLASH_INFO = 0x1A + READ_CHIP_INFO = 0x1B + ERASE_LBA = 0x25 + READ_CAPABILITY = 0xAA + RESET_DEVICE = 0xFF + + +class ResetSubcode(IntEnum): + """Sub-selector for :attr:`Opcode.RESET_DEVICE`.""" + + NORMAL = 0 + RESET_MSC = 1 + POWEROFF = 2 + MASKROM = 3 + DISCONNECT = 4 + + +class CommandStatus(IntEnum): + OK = 0 + FAILED = 1 + + +class RockusbError(Exception): + """A rockusb command failed, or the reply did not frame correctly.""" + + +def build_cbw( + tag: int, + opcode: Opcode | int, + *, + subcode: int = 0, + address: int = 0, + count: int = 0, + transfer_length: int = 0, + direction_in: bool = False, +) -> bytes: + """Build the 31-byte command block wrapper. + + Mind the mixed endianness: the wrapper's ``tag`` and ``length`` are + little-endian, but ``address`` and ``count`` inside the CDB are + big-endian. + + Args: + tag: caller-chosen id, echoed back in the status wrapper. + opcode: Rockchip operation code. + subcode: CDB byte 1 — the reset selector, or the read/write method. + address: starting LBA, for the block opcodes. + count: sector count, for the block opcodes. + transfer_length: bytes in the data phase. Defaults to + ``count * SECTOR_SIZE`` when a count is given. + direction_in: True when the data phase flows device to host. + """ + if transfer_length == 0 and count: + transfer_length = count * SECTOR_SIZE + + cdb = struct.pack( + ">BB I B H 7x", + int(opcode), + subcode, + address, + 0, + count, + ) + assert len(cdb) == 16, f"CDB must be 16 bytes, got {len(cdb)}" + + return ( + CBW_SIGNATURE + + struct.pack(" tuple[int, int, int]: + """Parse the 13-byte command status wrapper. + + Returns: + ``(tag, residue, status)``. + + Raises: + RockusbError: on a short read, a bad signature, or a tag that does not + match ``expected_tag``. A mismatched tag means replies have got out + of step with commands, which is not something to paper over. + """ + if len(data) < CSW_LENGTH: + raise RockusbError( + f"short status wrapper: got {len(data)} bytes, want {CSW_LENGTH}" + ) + if data[:4] != CSW_SIGNATURE: + raise RockusbError( + f"bad status signature {data[:4]!r}, want {CSW_SIGNATURE!r}" + ) + + tag, residue, status = struct.unpack_from(" list[tuple[int, int]]: + """Split a block range into ``(lba, count)`` pairs of at most ``max_sectors``.""" + if total_sectors < 0: + raise ValueError(f"negative sector count: {total_sectors}") + if max_sectors <= 0: + raise ValueError(f"max_sectors must be positive, got {max_sectors}") + + out: list[tuple[int, int]] = [] + lba = start_lba + remaining = total_sectors + while remaining > 0: + count = min(remaining, max_sectors) + out.append((lba, count)) + lba += count + remaining -= count + return out diff --git a/src/defib/rockusb/recovery.py b/src/defib/rockusb/recovery.py new file mode 100644 index 0000000..2521404 --- /dev/null +++ b/src/defib/rockusb/recovery.py @@ -0,0 +1,196 @@ +"""End-to-end Rockchip USB recovery. + +The flow this exists to serve: a board whose SPI NAND was erased or +half-written. Its boot ROM finds no valid IDB, gives up on flash and falls +into MaskROM by itself at power-up — no button, no strap, no UART. So a rig +that can only cut power can still bring the board back: + + power_cycle() -> wait_for_device(MASKROM) -> download_boot() + -> write_image(...) -> reset() + +:meth:`RockchipRecovery.download_boot` is the part that turns a MaskROM device +into one that can actually touch flash; everything after it is ordinary block +writes, because the usbplug runs Rockchip's FTL and presents SPI NAND as flat +512-byte sectors with bad blocks and ECC already handled. +""" + +from __future__ import annotations + +import asyncio +import logging +from typing import Callable + +from defib.recovery.events import ProgressEvent, Stage +from defib.rockusb.device import ( + DeviceMode, + RockusbDevice, + RockusbUsbError, + wait_for_device, +) +from defib.rockusb.loader import LoaderBlobs +from defib.rockusb.maskrom import CODE_471, CODE_472, build_maskrom_chunks +from defib.rockusb.protocol import ( + SECTOR_SIZE, + Opcode, + ResetSubcode, + split_lba_transfers, +) + +logger = logging.getLogger(__name__) + +#: The usbplug needs a moment after the last 472 chunk before it drops off the +#: bus and comes back as a loader-mode device. +USBPLUG_SETTLE = 1.0 + + +def _emit(cb: Callable[[ProgressEvent], None] | None, event: ProgressEvent) -> None: + if cb is not None: + cb(event) + + +class RockchipRecovery: + """Drive one board from MaskROM through to flashed and rebooted.""" + + def __init__(self, device: RockusbDevice) -> None: + self._device = device + + # -- stage 1: get the usbplug running --------------------------------- + + async def download_boot( + self, + blobs: LoaderBlobs, + on_progress: Callable[[ProgressEvent], None] | None = None, + reenumerate_timeout: float = 15.0, + ) -> RockusbDevice: + """Upload DDR init then usbplug, and wait for the device to come back. + + Returns the *new* opened device — the old handle is stale once the + usbplug re-enumerates, so callers must use the returned one. + """ + if self._device.mode is not DeviceMode.MASKROM: + logger.info("device already past MaskROM; skipping loader upload") + return self._device + + for entry in blobs.ddr: + await self._upload( + CODE_471, entry.data, blobs.use_rc4, Stage.DDR_INIT, entry.name, on_progress + ) + if entry.delay_ms: + await asyncio.sleep(entry.delay_ms / 1000.0) + + for entry in blobs.usbplug: + await self._upload( + CODE_472, entry.data, blobs.use_rc4, Stage.USBPLUG, entry.name, on_progress + ) + if entry.delay_ms: + await asyncio.sleep(entry.delay_ms / 1000.0) + + self._device.close() + await asyncio.sleep(USBPLUG_SETTLE) + + found = await wait_for_device( + timeout=reenumerate_timeout, mode=DeviceMode.LOADER + ) + device = RockusbDevice(found) + device.open() + self._device = device + _emit( + on_progress, + ProgressEvent(Stage.USBPLUG, 1, 1, f"usbplug running: {found}"), + ) + return device + + async def _upload( + self, + code: int, + blob: bytes, + use_rc4: bool, + stage: Stage, + name: str, + on_progress: Callable[[ProgressEvent], None] | None, + ) -> None: + chunks = build_maskrom_chunks(blob, use_rc4=use_rc4) + sent = 0 + for chunk in chunks: + await asyncio.to_thread(self._device.control_write, code, chunk) + sent += len(chunk) + _emit( + on_progress, + ProgressEvent(stage, sent, len(blob), f"{name} -> {code:#06x}"), + ) + logger.debug( + "uploaded %s (%d bytes in %d chunks) to %#06x", name, len(blob), len(chunks), code + ) + + # -- stage 2: touch flash --------------------------------------------- + + async def write_image( + self, + start_lba: int, + data: bytes, + on_progress: Callable[[ProgressEvent], None] | None = None, + ) -> None: + """Write ``data`` to flash starting at ``start_lba``. + + Short trailing data is zero-padded up to a sector; the usbplug has no + concept of a partial block. + """ + if len(data) % SECTOR_SIZE: + data = data + bytes(SECTOR_SIZE - (len(data) % SECTOR_SIZE)) + + total = len(data) // SECTOR_SIZE + written = 0 + for lba, count in split_lba_transfers(start_lba, total): + offset = (lba - start_lba) * SECTOR_SIZE + await asyncio.to_thread( + self._device.command, + Opcode.WRITE_LBA, + address=lba, + count=count, + data_out=data[offset : offset + count * SECTOR_SIZE], + ) + written += count + _emit( + on_progress, + ProgressEvent( + Stage.FLASH_WRITE, + written * SECTOR_SIZE, + total * SECTOR_SIZE, + f"lba {lba}", + ), + ) + + async def read_image(self, start_lba: int, sectors: int) -> bytes: + """Read ``sectors`` sectors back, for verification.""" + out = bytearray() + for lba, count in split_lba_transfers(start_lba, sectors): + out += await asyncio.to_thread( + self._device.command, + Opcode.READ_LBA, + address=lba, + count=count, + read_length=count * SECTOR_SIZE, + ) + return bytes(out) + + async def read_flash_id(self) -> bytes: + """Flash ID bytes — a cheap "is the usbplug really alive" probe.""" + return await asyncio.to_thread( + self._device.command, Opcode.READ_FLASH_ID, read_length=5 + ) + + async def reset(self, subcode: ResetSubcode = ResetSubcode.NORMAL) -> None: + """Reset the device. + + ``ResetSubcode.MASKROM`` comes back in MaskROM rather than booting, + which is how you chain several flash operations without needing the + board's power cut in between. + """ + try: + await asyncio.to_thread( + self._device.command, Opcode.RESET_DEVICE, subcode=int(subcode) + ) + except RockusbUsbError as e: + # The device is entitled to drop off the bus before it acknowledges + # its own reset, so a failed status read here is expected. + logger.debug("reset ack not received (device already gone): %s", e) diff --git a/tests/test_rockusb_codec.py b/tests/test_rockusb_codec.py new file mode 100644 index 0000000..b2bb399 --- /dev/null +++ b/tests/test_rockusb_codec.py @@ -0,0 +1,95 @@ +"""Tests for the Rockchip MaskROM wire codecs (CRC-16 and RC4).""" + +import struct + +import pytest + +from defib.rockusb.codec import CRC16_INIT, RK_RC4_KEY, rc4, rk_crc16 + + +def _crc16_bitwise(data: bytes, crc: int = CRC16_INIT) -> int: + """Independent bit-by-bit CRC-CCITT, to check the table-driven version. + + Poly 0x1021, MSB-first, no final XOR. + """ + for byte in data: + crc ^= byte << 8 + for _ in range(8): + crc = ((crc << 1) ^ 0x1021) if (crc & 0x8000) else (crc << 1) + crc &= 0xFFFF + return crc + + +class TestRkCrc16: + @pytest.mark.parametrize( + "data", + [ + b"", + b"\x00", + b"\xff", + b"123456789", + bytes(range(256)), + b"\x00" * 4096, + bytes(range(256)) * 17, + ], + ) + def test_matches_bitwise_reference(self, data): + assert rk_crc16(data) == _crc16_bitwise(data) + + def test_check_vector(self): + # CRC-16/CCITT-FALSE over b"123456789" is the standard 0x29B1 check + # value; this pins the seed and bit order, not just self-consistency. + assert rk_crc16(b"123456789") == 0x29B1 + + def test_empty_returns_seed(self): + assert rk_crc16(b"") == CRC16_INIT + + def test_in_range(self): + assert 0 <= rk_crc16(bytes(range(256))) <= 0xFFFF + + def test_differs_from_hisilicon_variant(self): + """Guard against anyone 'simplifying' this to reuse calc_crc(). + + The HiSilicon helper seeds at 0 and pads with two zero bytes; feeding a + Rockchip blob through it would produce a loader the boot ROM rejects. + """ + from defib.protocol.crc import calc_crc + + data = b"\xde\xad\xbe\xef" + assert rk_crc16(data) != calc_crc(data) + + def test_bytearray_accepted(self): + assert rk_crc16(bytearray(b"abc")) == rk_crc16(b"abc") + + +class TestRc4: + def test_known_vector(self): + # RFC 6229-style vector: key "Key", plaintext "Plaintext". + assert rc4(b"Plaintext", b"Key").hex() == "bbf316e8d940af0ad3" + + def test_second_known_vector(self): + assert rc4(b"pedia", b"Wiki").hex() == "1021bf0420" + + def test_self_inverse(self): + plain = bytes(range(256)) * 3 + assert rc4(rc4(plain)) == plain + + def test_default_key_is_the_rockchip_constant(self): + assert len(RK_RC4_KEY) == 16 + assert RK_RC4_KEY.hex() == "7c4e0304550509072d2c7b38170d1711" + + def test_length_preserved(self): + assert len(rc4(b"\x00" * 1000)) == 1000 + + def test_empty(self): + assert rc4(b"") == b"" + + +class TestCrcAppendOrder: + def test_appended_big_endian(self): + """The boot ROM wants the high byte first.""" + payload = b"\x01\x02\x03" + crc = rk_crc16(payload) + packed = struct.pack(">H", crc) + assert packed[0] == (crc >> 8) & 0xFF + assert packed[1] == crc & 0xFF diff --git a/tests/test_rockusb_loader.py b/tests/test_rockusb_loader.py new file mode 100644 index 0000000..c49bf0e --- /dev/null +++ b/tests/test_rockusb_loader.py @@ -0,0 +1,139 @@ +"""Tests for RKBOOT loader container parsing. + +Entries are read backwards from their stride because ``emType`` is a C enum of +ambiguous width. The parametrised stride tests below are what make that +approach worth having. +""" + +import struct + +import pytest + +from defib.rockusb.loader import ( + LoaderFormatError, + parse_loader, + raw_blobs, +) + +# uiTag[4] usSize[2] dwVersion[4] dwMergeVersion[4] RKTIME[7] emSupportChip[4] +TABLE_AT = 25 + + +def _build_loader( + ddr: bytes = b"\xdd" * 32, + usbplug: bytes = b"\xbb" * 64, + *, + entry_size: int = 57, + rc4_flag: int = 1, + ddr_delay: int = 0, + magic: bytes = b"BOOT", +) -> bytes: + """Synthesise a minimal but structurally honest RKBOOT container.""" + header_len = TABLE_AT + 6 * 3 + 2 + 57 + off471 = header_len + off472 = off471 + entry_size + blob_at = off472 + entry_size + + out = bytearray(blob_at) + out[0:4] = magic + struct.pack_into(" None: + out[base] = entry_size + name_at = base + entry_size - 12 - 40 + out[name_at : name_at + 40] = name.encode("utf-16-le").ljust(40, b"\x00") + struct.pack_into(" bytes: + return b"".join(chunks) + + +class TestChunking: + @pytest.mark.parametrize("size", [1, 100, 4095, 4096, 4097, 8192, 100_000]) + def test_no_chunk_exceeds_limit(self, size): + chunks = build_maskrom_chunks(b"\xa5" * size) + assert all(len(c) <= CHUNK_SIZE for c in chunks) + + def test_small_blob_is_one_chunk(self): + chunks = build_maskrom_chunks(b"\x01\x02\x03") + assert len(chunks) == 1 + assert len(chunks[0]) == 5 # 3 payload + 2 CRC + + def test_crc_appended_big_endian(self): + blob = b"\xde\xad\xbe\xef" + chunks = build_maskrom_chunks(blob) + assert _rejoin(chunks) == blob + struct.pack(">H", rk_crc16(blob)) + + def test_exact_chunk_multiple_spills_crc(self): + blob = b"\x11" * CHUNK_SIZE + chunks = build_maskrom_chunks(blob) + assert len(chunks) == 2 + assert len(chunks[0]) == CHUNK_SIZE + assert len(chunks[1]) == 2 # just the CRC + + +class TestBoundaryQuirks: + def test_4095_pads_before_crc(self): + """A 4095-byte blob gains a zero byte so the CRC stays contiguous.""" + blob = b"\x5a" * (CHUNK_SIZE - 1) + chunks = build_maskrom_chunks(blob) + + padded = blob + b"\x00" + assert _rejoin(chunks) == padded + struct.pack(">H", rk_crc16(padded)) + # Padding matters: the CRC must cover it, not the unpadded blob. + assert rk_crc16(padded) != rk_crc16(blob) + assert len(chunks[0]) == CHUNK_SIZE + assert len(chunks[1]) == 2 + + def test_4094_appends_terminator_packet(self): + """CRC lands the payload on an exact multiple, so a short packet is + needed to close the transfer.""" + blob = b"\x5a" * (CHUNK_SIZE - 2) + chunks = build_maskrom_chunks(blob) + + assert len(chunks) == 2 + assert len(chunks[0]) == CHUNK_SIZE + assert chunks[-1] == b"\x00" + assert chunks[0] == blob + struct.pack(">H", rk_crc16(blob)) + + def test_4094_plus_full_chunk_also_terminated(self): + blob = b"\x5a" * (CHUNK_SIZE + CHUNK_SIZE - 2) + chunks = build_maskrom_chunks(blob) + assert chunks[-1] == b"\x00" + assert sum(len(c) for c in chunks[:-1]) % CHUNK_SIZE == 0 + + @pytest.mark.parametrize("size", [4093, 4096, 4097]) + def test_no_terminator_when_not_needed(self, size): + chunks = build_maskrom_chunks(b"\x5a" * size) + assert chunks[-1] != b"\x00" or len(chunks[-1]) > 1 + + +class TestRc4Path: + def test_rc4_off_by_default(self): + """RV1106 loaders are built RC4-off; defaulting the other way would + silently corrupt every upload.""" + blob = b"\x01" * 64 + assert _rejoin(build_maskrom_chunks(blob)).startswith(blob) + + def test_rc4_on_encrypts_then_checksums(self): + blob = b"\x01" * 64 + chunks = build_maskrom_chunks(blob, use_rc4=True) + + encrypted = rc4(blob) + assert _rejoin(chunks) == encrypted + struct.pack(">H", rk_crc16(encrypted)) + + def test_rc4_changes_output(self): + blob = b"\x01" * 64 + assert build_maskrom_chunks(blob) != build_maskrom_chunks(blob, use_rc4=True) + + +class TestDeterminism: + def test_stable_across_calls(self): + blob = bytes(range(256)) * 40 + assert build_maskrom_chunks(blob) == build_maskrom_chunks(blob) + + def test_input_not_mutated(self): + blob = bytearray(b"\x01\x02\x03") + build_maskrom_chunks(bytes(blob)) + assert blob == bytearray(b"\x01\x02\x03") diff --git a/tests/test_rockusb_protocol.py b/tests/test_rockusb_protocol.py new file mode 100644 index 0000000..a22ed5f --- /dev/null +++ b/tests/test_rockusb_protocol.py @@ -0,0 +1,145 @@ +"""Tests for rockusb CBW/CSW framing. + +The wrapper is little-endian but the command block inside it is big-endian. +Most of these tests exist to pin that down. +""" + +import struct + +import pytest + +from defib.rockusb.protocol import ( + CBW_LENGTH, + CBW_SIGNATURE, + CSW_LENGTH, + CSW_SIGNATURE, + DIRECTION_IN, + DIRECTION_OUT, + MAX_SECTORS_PER_TRANSFER, + SECTOR_SIZE, + Opcode, + ResetSubcode, + RockusbError, + build_cbw, + parse_csw, + split_lba_transfers, +) + + +def _csw(tag: int, residue: int = 0, status: int = 0, signature: bytes = CSW_SIGNATURE): + return signature + struct.pack(" Date: Tue, 18 Aug 2026 19:20:13 +0300 Subject: [PATCH 2/8] profiles: record how a chip is reached when it is dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every profile so far has been HiSilicon UART bytecode, so a chip whose boot ROM only answers on USB could not be described at all. Add a RECOVERY discriminator that defaults to "uart", leaving all 111 existing profiles untouched. The four bytecode fields become optional to make room for that, which on its own would let a UART profile quietly lose its bytecode and only fail as a confusing NoneType deep inside a burn. A validator requires them back whenever RECOVERY is "uart", and the UART-only properties now raise a plain "rv1106 recovers over usb, not UART" instead of returning None. recovery_mode() falls back to "uart" when a chip has no profile at all, which is what keeps the V500 and CV6xx families working — their chip lists live in their protocol modules, not in JSON. Ship rv1106 only. The partition LBAs come from Luckfox's published layout for the Pico Pro/Max; idblock stays at 0x40000, the offset the boot ROM looks for the IDB at, because moving it bricks the board in a way no button recovers. rv1103 is deliberately absent — the Pico Plus/Mini layout was never verified, and a fabricated one would be worse than none. Co-Authored-By: Claude Opus 4.8 --- src/defib/flashdump.py | 5 +- src/defib/profiles/data/rv1106.json | 15 ++++ src/defib/profiles/loader.py | 13 ++++ src/defib/profiles/schema.py | 102 +++++++++++++++++++++++++--- 4 files changed, 124 insertions(+), 11 deletions(-) create mode 100644 src/defib/profiles/data/rv1106.json diff --git a/src/defib/flashdump.py b/src/defib/flashdump.py index 7bae16e..fbeac96 100644 --- a/src/defib/flashdump.py +++ b/src/defib/flashdump.py @@ -95,7 +95,10 @@ def get_ram_staging_addr(chip: str) -> int: try: from defib.profiles.loader import load_profile profile = load_profile(chip_lower) - uboot_addr = int(profile.addresses[2], 16) + # The property raises for USB-recovery chips, which have no load + # addresses at all — the except below then falls through to the + # prefix lookup, which is the right answer for them. + uboot_addr = profile.uboot_address # Derive RAM base from U-Boot address (aligned to 0x40000000 boundary) ram_base = uboot_addr & 0xF0000000 return ram_base + RAM_STAGING_OFFSET diff --git a/src/defib/profiles/data/rv1106.json b/src/defib/profiles/data/rv1106.json new file mode 100644 index 0000000..df7eccd --- /dev/null +++ b/src/defib/profiles/data/rv1106.json @@ -0,0 +1,15 @@ +{ + "name": "rv1106", + "RECOVERY": "usb", + "LOADER_DDR": "rv1106_ddr_924MHz_v1.15.bin", + "LOADER_USBPLUG": "rv1106_usbplug_v1.09.bin", + "PARTITIONS": { + "env": 0, + "idblock": 512, + "uboot": 1024, + "boot": 2048, + "oem": 10240, + "userdata": 71680, + "rootfs": 92160 + } +} diff --git a/src/defib/profiles/loader.py b/src/defib/profiles/loader.py index eb90ba1..cd5e13b 100644 --- a/src/defib/profiles/loader.py +++ b/src/defib/profiles/loader.py @@ -107,6 +107,19 @@ def load_profile(chip_name: str, profiles_dir: Path | None = None) -> SoCProfile raise ValueError(f"Alias chain too deep for chip: {chip_name}") +def recovery_mode(chip_name: str, profiles_dir: Path | None = None) -> str: + """How ``chip_name`` is reached when it is dead: ``"uart"`` or ``"usb"``. + + Defaults to ``"uart"`` for anything without a profile, which covers the + V500 and CV6xx families whose chip lists live in their protocol modules + rather than in JSON. + """ + try: + return load_profile(chip_name, profiles_dir).recovery + except (FileNotFoundError, ValueError): + return "uart" + + def list_variants(chip_name: str, profiles_dir: Path | None = None) -> list[str]: """Return the list of board variants declared for ``chip_name``. diff --git a/src/defib/profiles/schema.py b/src/defib/profiles/schema.py index 8fff775..c517c2f 100644 --- a/src/defib/profiles/schema.py +++ b/src/defib/profiles/schema.py @@ -2,16 +2,30 @@ from __future__ import annotations -from pydantic import BaseModel, Field, PrivateAttr +from typing import Literal + +from pydantic import BaseModel, Field, PrivateAttr, model_validator class SoCProfile(BaseModel): - """A SoC configuration profile for the standard HiSilicon protocol. + """A SoC configuration profile. - Fields match the JSON profile format used by the original burn tool. + Field names match the JSON profile format used by the original burn tool, + which was HiSilicon-only. ``RECOVERY`` widens that: chips whose boot ROM + has no UART download path at all — Rockchip's, which is USB-only — declare + ``"RECOVERY": "usb"`` and supply none of the DDR/SPL bytecode below. """ name: str = Field(description="Internal chip name") + recovery: Literal["uart", "usb"] = Field( + default="uart", alias="RECOVERY", + description=( + "How a dead board is reached. 'uart' drives the boot ROM over " + "serial with the bytecode in this profile. 'usb' means the boot " + "ROM only answers on USB, so the fields below do not apply and " + "the loader blobs are named by LOADER_* instead." + ), + ) prestep0: list[int] | None = Field( default=None, alias="PRESTEP0", description="Pre-DDR init bytecode (sent before DDRSTEP0)", @@ -20,17 +34,19 @@ class SoCProfile(BaseModel): default=None, alias="PRESTEP1", description="DDR training verification bytecode (sent after DDRSTEP0)", ) - ddrstep0: list[int] = Field(alias="DDRSTEP0", description="DDR initialization bytecode") - addresses: list[str] = Field( - alias="ADDRESS", + ddrstep0: list[int] | None = Field( + default=None, alias="DDRSTEP0", description="DDR initialization bytecode" + ) + addresses: list[str] | None = Field( + default=None, alias="ADDRESS", description="Load addresses: [ddr_step, spl, uboot]", ) - file_lengths: list[str] = Field( - alias="FILELEN", + file_lengths: list[str] | None = Field( + default=None, alias="FILELEN", description="Size limits: [ddr_step_max, spl_max]", ) - step_lengths: list[str] = Field( - alias="STEPLEN", + step_lengths: list[str] | None = Field( + default=None, alias="STEPLEN", description="Step frame sizes: [ddr_step, spl]", ) sram_limit: str | None = Field( @@ -56,10 +72,66 @@ class SoCProfile(BaseModel): "`spl_data` property." ), ) + loader_ddr: str | None = Field( + default=None, alias="LOADER_DDR", + description=( + "USB recovery only. Filename of the DDR-init blob the boot ROM " + "expects first (rkbin's rv1106_ddr_*.bin). Not bundled — it is a " + "vendor binary the user supplies." + ), + ) + loader_usbplug: str | None = Field( + default=None, alias="LOADER_USBPLUG", + description=( + "USB recovery only. Filename of the second-stage blob that takes " + "over USB and exposes flash (rkbin's rv1106_usbplug_*.bin)." + ), + ) + partitions: dict[str, int] = Field( + default_factory=dict, alias="PARTITIONS", + description=( + "USB recovery only. Partition name to starting LBA (512-byte " + "sectors), used to place firmware images without the caller " + "computing offsets." + ), + ) + # Bytes of the SPL_BLOB, populated by the loader (not in JSON, not # validated by pydantic). None if `spl_blob` is unset. _spl_data: bytes | None = PrivateAttr(default=None) + @model_validator(mode="after") + def _check_recovery_fields(self) -> SoCProfile: + """Each recovery family must carry its own required fields. + + Without this the UART fields could quietly go missing on a UART chip + and only surface as a confusing ``NoneType`` deep inside a burn. + """ + if self.recovery == "uart": + missing = [ + alias + for alias, value in ( + ("DDRSTEP0", self.ddrstep0), + ("ADDRESS", self.addresses), + ("FILELEN", self.file_lengths), + ("STEPLEN", self.step_lengths), + ) + if value is None + ] + if missing: + raise ValueError( + f"profile '{self.name}' uses UART recovery but is missing " + f"{', '.join(missing)}" + ) + return self + + def _require_uart(self, field: str) -> None: + if self.recovery != "uart": + raise ValueError( + f"'{self.name}' recovers over {self.recovery}, not UART — " + f"{field} is not defined for it" + ) + @property def spl_data(self) -> bytes | None: """Pre-built SPL bytes if the profile declares an `SPL_BLOB`.""" @@ -67,18 +139,26 @@ def spl_data(self) -> bytes | None: @property def ddr_step_address(self) -> int: + self._require_uart("ddr_step_address") + assert self.addresses is not None return int(self.addresses[0], 16) @property def spl_address(self) -> int: + self._require_uart("spl_address") + assert self.addresses is not None return int(self.addresses[1], 16) @property def uboot_address(self) -> int: + self._require_uart("uboot_address") + assert self.addresses is not None return int(self.addresses[2], 16) @property def spl_max_size(self) -> int: + self._require_uart("spl_max_size") + assert self.file_lengths is not None return int(self.file_lengths[1], 16) @property @@ -89,6 +169,8 @@ def spl_sram_limit(self) -> int | None: @property def ddr_step_data(self) -> bytes: + self._require_uart("ddr_step_data") + assert self.ddrstep0 is not None return bytes(self.ddrstep0) @property From ffa27c88a6aeca88526b236f3d1f6dd4aa3cdaa5 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Tue, 18 Aug 2026 19:22:42 +0300 Subject: [PATCH 3/8] cli: route USB-recovery chips through burn and install MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first cut of this put Rockchip behind a `defib rockchip` sub-app, which was wrong: the whole CLI is chip-selected — `-c hi3516ev300`, `-c gk7205v200` — and no vendor name appears anywhere in it. A vendor namespace pushed an internal problem (USB not fitting the serial-shaped verbs) onto the user's fingers. Fix the dispatch instead. defib burn -c rv1106 --ddr --usbplug --power-cycle defib install -c rv1106 --firmware openipc.rv1106-nor-lite.tgz --verify burn stops once the usbplug is running — the USB equivalent of uploading U-Boot into RAM — and install writes images to the partitions the profile declares. -p is simply not consulted for these chips. Both loader forms exist because the blobs Rockchip publishes for RV1106 carry no container header, which is precisely what `rkdeveloptool db` refuses to load; --ddr/--usbplug takes them as-is and --loader takes an RKBOOT container. install refuses rootfs.ubi rather than guessing at it. That image bundles kernel and rootfs as UBI volumes, so no single partition is the right answer, and picking one would bury a real unresolved question about this board's layout. Error text goes through rich.markup.escape() — Rich was eating the "defib[rockchip]" install hint as a style tag. Co-Authored-By: Claude Opus 4.8 --- src/defib/cli/app.py | 319 +++++++++++++++++++++++++++- tests/test_profiles_usb_recovery.py | 136 ++++++++++++ 2 files changed, 447 insertions(+), 8 deletions(-) create mode 100644 tests/test_profiles_usb_recovery.py diff --git a/src/defib/cli/app.py b/src/defib/cli/app.py index 93d7f9e..565470a 100644 --- a/src/defib/cli/app.py +++ b/src/defib/cli/app.py @@ -24,19 +24,28 @@ def burn( poe_port_override: str = typer.Option("", "--poe-port", help="Explicit MikroTik ether port (e.g. ether3) — overrides comment-based auto-discovery. Requires --power-cycle."), output: str = typer.Option("human", "--output", help="Output mode: human, json, quiet"), debug: bool = typer.Option(False, "-d", "--debug", help="Enable debug logging"), + ddr: str = typer.Option("", "--ddr", help="USB-recovery chips: DDR-init blob (rkbin rv1106_ddr_*.bin)"), + usbplug: str = typer.Option("", "--usbplug", help="USB-recovery chips: usbplug blob (rkbin rv1106_usbplug_*.bin)"), + loader: str = typer.Option("", "--loader", help="USB-recovery chips: RKBOOT container (MiniLoaderAll.bin), instead of --ddr/--usbplug"), + wait: float = typer.Option(30.0, "--wait", help="USB-recovery chips: seconds to wait for the board to enumerate"), ) -> None: - """Recover a device by uploading firmware via UART serial. + """Wake a dead device so its flash can be written. - If no firmware file is specified with -f, automatically downloads - the appropriate U-Boot from OpenIPC releases. + For most chips this uploads U-Boot into RAM over UART; if no file is + given with -f the right one is downloaded from OpenIPC. + + Chips whose boot ROM only answers on USB (Rockchip) instead take + --ddr/--usbplug (or --loader) and ignore -p: an erased flash drops them + into MaskROM on its own at power-up, so --power-cycle is all it takes. """ import asyncio - asyncio.run(_burn_async(chip, file, port, send_break, terminal, power_cycle, poe_port_override, output, debug)) + asyncio.run(_burn_async(chip, file, port, send_break, terminal, power_cycle, poe_port_override, output, debug, ddr, usbplug, loader, wait)) async def _burn_async( chip: str, file: str, port: str, send_break: bool, terminal: bool, power_cycle: bool, poe_port_override: str, output: str, debug: bool, + ddr: str = "", usbplug: str = "", loader: str = "", wait: float = 30.0, ) -> None: import json as json_mod import logging @@ -44,6 +53,7 @@ async def _burn_async( from rich.console import Console from rich.progress import Progress, SpinnerColumn, TextColumn, BarColumn, TaskProgressColumn + from defib.profiles.loader import recovery_mode from defib.recovery.events import LogEvent, ProgressEvent from defib.recovery.session import RecoverySession @@ -54,6 +64,10 @@ async def _burn_async( else: logging.basicConfig(level=logging.INFO) + if recovery_mode(chip) == "usb": + await _burn_usb_async(chip, ddr, usbplug, loader, power_cycle, output, wait) + return + # Resolve firmware: local file or auto-download from OpenIPC firmware_path = file if not firmware_path: @@ -2094,17 +2108,26 @@ def install( ), output: str = typer.Option("human", "--output", help="Output mode: human, json"), debug: bool = typer.Option(False, "-d", "--debug", help="Enable debug logging"), + ddr: str = typer.Option("", "--ddr", help="USB-recovery chips: DDR-init blob (rkbin rv1106_ddr_*.bin)"), + usbplug: str = typer.Option("", "--usbplug", help="USB-recovery chips: usbplug blob (rkbin rv1106_usbplug_*.bin)"), + loader: str = typer.Option("", "--loader", help="USB-recovery chips: RKBOOT container (MiniLoaderAll.bin), instead of --ddr/--usbplug"), + wait: float = typer.Option(30.0, "--wait", help="USB-recovery chips: seconds to wait for the board to enumerate"), + verify: bool = typer.Option(False, "--verify", help="USB-recovery chips: read each image back and compare"), ) -> None: - """Install a full OpenIPC firmware (U-Boot + kernel + rootfs) via UART + TFTP. + """Install a full OpenIPC firmware (U-Boot + kernel + rootfs). + + For UART chips: extracts the tarball, burns U-Boot to RAM via the boot + ROM, then TFTPs kernel and rootfs to U-Boot, which flashes them. - Extracts the firmware tarball, burns U-Boot to RAM via boot ROM, - then uses TFTP to transfer kernel and rootfs to U-Boot which - flashes them to NOR or NAND. + For chips whose boot ROM only answers on USB (Rockchip), -p and the TFTP + options do not apply — pass --ddr/--usbplug (or --loader) and the images + are written straight to flash over USB. """ import asyncio asyncio.run(_install_async( chip, firmware, port, power_cycle, poe_port_override, nic, host_ip, device_ip, tftp_port, nor_size, nand, wipe_env, tftp_via, output, debug, + ddr, usbplug, loader, wait, verify, )) @@ -2181,6 +2204,11 @@ async def _install_async( tftp_via: str, output: str, debug: bool, + ddr: str = "", + usbplug: str = "", + loader: str = "", + wait: float = 30.0, + verify: bool = False, ) -> None: import hashlib import json as json_mod @@ -2201,6 +2229,7 @@ async def _install_async( ) from defib.network.ip_manager import list_interfaces, temporary_ip from defib.network.tftp_server import start_tftp_server + from defib.profiles.loader import recovery_mode from defib.recovery.events import LogEvent, ProgressEvent from defib.recovery.session import RecoverySession from defib.transport.serial_platform import create_transport, normalize_port_name @@ -2212,6 +2241,13 @@ async def _install_async( else: logging.basicConfig(level=logging.INFO) + if recovery_mode(chip) == "usb": + await _install_usb_async( + chip, firmware_path, ddr, usbplug, loader, + power_cycle, output, wait, verify, + ) + return + if nand: layout = _NAND_LAYOUT flash_cmd = "nand" @@ -3611,5 +3647,272 @@ async def _replace_in_tftp(name: str, data: bytes) -> None: print(json_mod.dumps({"event": "done", "success": True, "partitions": len(partitions)})) +def _usb_progress_printer(output: str) -> Any: + """Progress callback that emits JSON lines, or nothing in other modes.""" + import json as json_mod + + from defib.recovery.events import ProgressEvent + + def on_progress(event: ProgressEvent) -> None: + if output == "json": + print(json_mod.dumps({ + "event": "progress", "stage": event.stage.value, + "sent": event.bytes_sent, "total": event.bytes_total, + "percent": round(event.percent, 1), + }), flush=True) + + return on_progress + + +def _resolve_usb_loader(chip: str, ddr: str, usbplug: str, loader: str) -> Any: + """Build the MaskROM loader blobs for a USB-recovery chip. + + ``--loader`` takes an RKBOOT container; ``--ddr``/``--usbplug`` take the + two bare images. Both forms exist because the blobs Rockchip publishes for + RV1106 carry no container header — which is exactly what ``rkdeveloptool + db`` refuses to load. + + Neither is bundled: they are vendor binaries, so the profile records only + the filenames it expects and the user supplies them. + """ + from pathlib import Path + + from defib.profiles.loader import load_profile + from defib.rockusb.loader import parse_loader, raw_blobs + + if loader: + return parse_loader(Path(loader).read_bytes()) + if ddr and usbplug: + return raw_blobs(Path(ddr).read_bytes(), Path(usbplug).read_bytes()) + + profile = load_profile(chip) + expected = " and ".join( + n for n in (profile.loader_ddr, profile.loader_usbplug) if n + ) + raise typer.BadParameter( + f"{chip} recovers over USB and needs its vendor loader blobs: pass " + f"--ddr and --usbplug (expected {expected}), or --loader for an " + "RKBOOT container." + ) + + +async def _open_usb_target( + blobs: Any, power_cycle: bool, output: str, wait: float, +) -> Any: + """Power-cycle if asked, catch the board, and get its usbplug running. + + Returns a ``RockchipRecovery`` ready to touch flash. + """ + from rich.console import Console + + from defib.rockusb.device import DeviceMode, RockusbDevice, wait_for_device + from defib.rockusb.recovery import RockchipRecovery + + console = Console() + + if power_cycle: + from defib.power.factory import power_controller_from_env + + controller = power_controller_from_env() + if output == "human": + console.print(f" Power-cycling via {controller.name()}...") + try: + await controller.power_cycle("") + finally: + await controller.close() + + found = await wait_for_device(timeout=wait) + if output == "human": + console.print(f" Found {found}") + + device = RockusbDevice(found) + device.open() + recovery = RockchipRecovery(device) + + if found.mode is DeviceMode.MASKROM: + if output == "human": + console.print(" Uploading DDR init and usbplug...") + await recovery.download_boot(blobs, on_progress=_usb_progress_printer(output)) + + return recovery + + +def _usb_fail(output: str, message: str) -> None: + """Report a USB-recovery failure and exit non-zero.""" + import json as json_mod + + from rich.console import Console + from rich.markup import escape + + if output == "json": + print(json_mod.dumps({"event": "error", "message": message})) + else: + # Escaped: messages carry file paths and the "defib[rockchip]" install + # hint, either of which Rich would read as markup. + Console().print(f"[red]{escape(message)}[/red]") + raise typer.Exit(1) + + +async def _burn_usb_async( + chip: str, ddr: str, usbplug: str, loader: str, + power_cycle: bool, output: str, wait: float, +) -> None: + """``burn`` for chips whose boot ROM only answers on USB. + + The UART path uploads U-Boot into RAM; the USB equivalent is getting the + usbplug running, after which flash is writable. It stops there — writing + images is ``install``'s job. + """ + import json as json_mod + + from rich.console import Console + + from defib.rockusb.protocol import RockusbError + + console = Console() + blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) + + try: + recovery = await _open_usb_target(blobs, power_cycle, output, wait) + flash_id = await recovery.read_flash_id() + except RockusbError as e: + _usb_fail(output, str(e)) + return + + if output == "json": + print(json_mod.dumps({ + "event": "done", "success": True, "flash_id": flash_id.hex(), + })) + elif output != "quiet": + console.print(f" Flash ID: {flash_id.hex()}") + console.print( + "\n[green bold]Device is awake.[/green bold] Flash it with: " + f"defib install -c {chip} --firmware " + ) + + +# Which partition each OpenIPC image belongs in, for USB-recovery chips. +# Keyed on the filename stem the tarball uses before the ``.`` suffix. +_USB_IMAGE_PARTITIONS = { + "zboot.img": "boot", + "rootfs.squashfs": "rootfs", + "uboot.img": "uboot", + "idblock.img": "idblock", +} + + +def _map_usb_images(names: list[str], partitions: dict[str, int]) -> list[tuple[str, str, int]]: + """Match tarball members to partitions. + + Returns ``(member, partition, lba)`` triples. + + Raises: + typer.BadParameter: if a member cannot be placed. The UBI case is + called out by name because it is not a mapping gap but a real + layout question: ``rootfs.ubi`` bundles kernel *and* rootfs as UBI + volumes, so it does not correspond to any single partition in the + vendor table. + """ + mapped: list[tuple[str, str, int]] = [] + for name in names: + stem = name.rsplit(".", 1)[0] if "." in name else name + partition = _USB_IMAGE_PARTITIONS.get(stem) + if partition is None: + if stem.startswith("rootfs.ubi"): + raise typer.BadParameter( + f"{name} is a UBI image holding both kernel and rootfs " + "volumes, so it has no single partition to go in. Use the " + "nor-style tarball (zboot.img + rootfs.squashfs), or " + "decide the UBI region for this board first." + ) + continue + if partition not in partitions: + raise typer.BadParameter( + f"{name} belongs in partition '{partition}', which this " + f"chip's profile does not declare. Known: " + f"{', '.join(sorted(partitions)) or '(none)'}" + ) + mapped.append((name, partition, partitions[partition])) + return mapped + + +async def _install_usb_async( + chip: str, firmware_path: str, ddr: str, usbplug: str, loader: str, + power_cycle: bool, output: str, wait: float, verify: bool, +) -> None: + """``install`` for chips whose boot ROM only answers on USB. + + The usbplug runs Rockchip's FTL, so flash is a flat array of 512-byte + sectors here — bad blocks and ECC are handled device-side and there is no + TFTP or U-Boot console in the path at all. + """ + import json as json_mod + import tarfile + from pathlib import Path + + from rich.console import Console + + from defib.profiles.loader import load_profile + from defib.rockusb.protocol import SECTOR_SIZE, ResetSubcode, RockusbError + + console = Console() + blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) + partitions = load_profile(chip).partitions + + tar_path = Path(firmware_path) + if not tar_path.exists(): + raise typer.BadParameter(f"firmware not found: {firmware_path}") + + with tarfile.open(tar_path) as tar: + members = [m for m in tar.getmembers() if m.isfile()] + names = [m.name for m in members if not m.name.endswith(".md5sum")] + targets = _map_usb_images(names, partitions) + if not targets: + raise typer.BadParameter( + f"nothing in {tar_path.name} maps to a partition " + f"(saw: {', '.join(names) or 'no files'})" + ) + payloads = [] + for name, partition, lba in targets: + handle = tar.extractfile(name) + if handle is None: + raise typer.BadParameter(f"cannot read {name} from {tar_path.name}") + payloads.append((name, partition, lba, handle.read())) + + try: + recovery = await _open_usb_target(blobs, power_cycle, output, wait) + flash_id = await recovery.read_flash_id() + if output == "human": + console.print(f" Flash ID: {flash_id.hex()}") + + for name, partition, lba, data in payloads: + if output == "human": + console.print( + f" Writing {name} -> {partition} " + f"(LBA {lba}, {len(data)} bytes)..." + ) + await recovery.write_image( + lba, data, on_progress=_usb_progress_printer(output) + ) + if verify: + sectors = (len(data) + SECTOR_SIZE - 1) // SECTOR_SIZE + if (await recovery.read_image(lba, sectors))[: len(data)] != data: + _usb_fail(output, f"verify failed for {name} at LBA {lba}") + if output == "human": + console.print(" Verified") + + await recovery.reset(ResetSubcode.NORMAL) + except RockusbError as e: + _usb_fail(output, str(e)) + return + + if output == "json": + print(json_mod.dumps({ + "event": "done", "success": True, "images": len(payloads), + })) + else: + console.print("\n[green bold]Install complete![/green bold] Device is rebooting.") + + def main() -> None: app() diff --git a/tests/test_profiles_usb_recovery.py b/tests/test_profiles_usb_recovery.py new file mode 100644 index 0000000..28d950e --- /dev/null +++ b/tests/test_profiles_usb_recovery.py @@ -0,0 +1,136 @@ +"""Tests for the USB-recovery discriminator on SoC profiles. + +Chips whose boot ROM has no UART download path (Rockchip) declare +``"RECOVERY": "usb"`` and carry none of the DDR/SPL bytecode. These tests pin +down that the two families stay distinguishable and that neither can silently +be missing what it needs. +""" + +import json +from pathlib import Path + +import pytest +from pydantic import ValidationError + +from defib.cli.app import _map_usb_images +from defib.profiles.loader import load_profile, recovery_mode +from defib.profiles.schema import SoCProfile + +PROFILES_DIR = Path(__file__).parent.parent / "src" / "defib" / "profiles" / "data" + + +class TestRecoveryDiscriminator: + def test_uart_is_the_default(self): + """Every pre-existing profile predates the field and must stay UART.""" + assert load_profile("hi3516cv300", PROFILES_DIR).recovery == "uart" + + def test_rv1106_is_usb(self): + assert load_profile("rv1106", PROFILES_DIR).recovery == "usb" + + def test_recovery_mode_helper(self): + assert recovery_mode("hi3516cv300", PROFILES_DIR) == "uart" + assert recovery_mode("rv1106", PROFILES_DIR) == "usb" + + def test_unknown_chip_defaults_to_uart(self): + """V500/CV6xx chips have no JSON profile, so absence must not be + mistaken for USB recovery.""" + assert recovery_mode("hi3516dv500", PROFILES_DIR) == "uart" + assert recovery_mode("no-such-chip-at-all", PROFILES_DIR) == "uart" + + +class TestValidation: + def test_uart_profile_missing_bytecode_is_rejected(self): + with pytest.raises(ValidationError, match="DDRSTEP0"): + SoCProfile.model_validate({"name": "bogus"}) + + def test_uart_error_names_every_missing_field(self): + with pytest.raises(ValidationError) as excinfo: + SoCProfile.model_validate({"name": "bogus", "DDRSTEP0": [1]}) + message = str(excinfo.value) + for alias in ("ADDRESS", "FILELEN", "STEPLEN"): + assert alias in message + + def test_usb_profile_needs_no_bytecode(self): + profile = SoCProfile.model_validate({"name": "x", "RECOVERY": "usb"}) + assert profile.recovery == "usb" + assert profile.ddrstep0 is None + + def test_uart_properties_raise_on_usb_profile(self): + profile = SoCProfile.model_validate({"name": "x", "RECOVERY": "usb"}) + for attr in ( + "ddr_step_address", "spl_address", "uboot_address", + "spl_max_size", "ddr_step_data", + ): + with pytest.raises(ValueError, match="recovers over usb"): + getattr(profile, attr) + + def test_unknown_recovery_value_rejected(self): + with pytest.raises(ValidationError): + SoCProfile.model_validate({"name": "x", "RECOVERY": "jtag"}) + + +class TestRv1106Profile: + def test_declares_its_loader_blobs(self): + profile = load_profile("rv1106", PROFILES_DIR) + assert profile.loader_ddr == "rv1106_ddr_924MHz_v1.15.bin" + assert profile.loader_usbplug == "rv1106_usbplug_v1.09.bin" + + def test_partition_lbas_match_the_vendor_byte_layout(self): + """Luckfox SPI NAND: 256K(env) 256K@256K(idblock) 512K(uboot) 4M(boot) + 30M(oem) 10M(userdata) 80M(rootfs), converted to 512-byte sectors.""" + partitions = load_profile("rv1106", PROFILES_DIR).partitions + expected_bytes = { + "env": 0, + "idblock": 256 * 1024, + "uboot": 512 * 1024, + "boot": 1024 * 1024, + "oem": 5 * 1024 * 1024, + "userdata": 35 * 1024 * 1024, + "rootfs": 45 * 1024 * 1024, + } + assert partitions == {k: v // 512 for k, v in expected_bytes.items()} + + def test_idblock_stays_at_its_fixed_offset(self): + """The boot ROM looks for the IDB at 0x40000; moving it bricks the + board in a way no button recovers.""" + assert load_profile("rv1106", PROFILES_DIR).partitions["idblock"] * 512 == 0x40000 + + def test_profile_json_is_minimal(self): + """A USB profile carrying UART bytecode would mean someone copied the + wrong template.""" + data = json.loads((PROFILES_DIR / "rv1106.json").read_text()) + assert not {"DDRSTEP0", "PRESTEP0", "ADDRESS", "FILELEN"} & set(data) + + +class TestMapUsbImages: + PARTS = {"boot": 2048, "rootfs": 92160, "uboot": 1024, "idblock": 512} + + def test_maps_nor_style_tarball(self): + out = _map_usb_images( + ["zboot.img.rv1106", "rootfs.squashfs.rv1106"], self.PARTS + ) + assert out == [ + ("zboot.img.rv1106", "boot", 2048), + ("rootfs.squashfs.rv1106", "rootfs", 92160), + ] + + def test_ubi_image_refused_with_the_reason(self): + """Not a mapping gap — a UBI bundles kernel and rootfs volumes, so no + single partition is the right answer.""" + import typer + + with pytest.raises(typer.BadParameter, match="kernel and rootfs"): + _map_usb_images(["rootfs.ubi.rv1106"], self.PARTS) + + def test_undeclared_partition_is_an_error_not_a_silent_skip(self): + import typer + + with pytest.raises(typer.BadParameter, match="does not declare"): + _map_usb_images(["zboot.img.rv1106"], {"rootfs": 1}) + + def test_unrecognised_files_are_ignored(self): + assert _map_usb_images(["README", "notes.txt"], self.PARTS) == [] + + def test_checksums_are_not_mistaken_for_images(self): + out = _map_usb_images(["zboot.img.rv1106.md5sum"], self.PARTS) + assert out == [] From 6d1164ddcc84ab1ab43268c7e485504d973d9218 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Tue, 18 Aug 2026 21:33:45 +0300 Subject: [PATCH 4/8] rockusb: close the gaps review found in the USB path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ten findings, all real. The theme running through most of them is a flash that goes wrong without the operator being told. Wrong-board hazard. Every Rockchip board shares one VID:PID and gets a fresh USB address when the usbplug re-enumerates, so "first match wins" could upload to one board and flash another. Identify boards by their physical port path, which is the one thing that survives re-enumeration; refuse to guess when several are attached, and pin the post-upload wait to the port the board was found on. --usb-path picks one explicitly. Unbounded writes. Partitions carried only a starting LBA, so an oversized image ran straight on into whatever followed. They now carry their extent, and an image that would not fit is refused by name and size. The layout is also asserted to tile without gaps or overlap. Idblock ordering. Images were written in tar order, so the IDB could land before the rest of flash was populated — and the IDB is precisely what stops the boot ROM falling into MaskROM. A failure after that point would leave a board that boots a broken image instead of one that can be re-flashed over USB. It is now written last. Silent partial transfers. A device may move less than it was asked to and still report status OK, reporting the shortfall as residue. That was ignored, so an incomplete write could be announced as a finished install. Incomplete and corrupt firmware. An archive holding only a rootfs was written and called a success; require the kernel and rootfs pair, as the UART installer does. The shipped .md5sums were skipped entirely, and --verify cannot stand in for them because it compares flash against the same bytes that were sent — a corrupt download would verify perfectly. Also: detach and reattach the interface actually claimed rather than assuming interface 0; reject loaders declaring no DDR or usbplug entries, which otherwise upload nothing and time out blaming the board; thread --poe-port through so RouterOS addresses the right port; and resolve loader files inside the error handler so a missing file produces a JSON error event instead of a traceback. Co-Authored-By: Claude Opus 4.8 --- src/defib/cli/app.py | 176 +++++++++++++++++----- src/defib/profiles/data/rv1106.json | 14 +- src/defib/profiles/schema.py | 28 +++- src/defib/rockusb/device.py | 121 +++++++++++---- src/defib/rockusb/loader.py | 22 +++ src/defib/rockusb/recovery.py | 9 +- tests/test_profiles_usb_recovery.py | 100 +++++++++++-- tests/test_rockusb_install.py | 222 ++++++++++++++++++++++++++++ tests/test_rockusb_loader.py | 37 +++++ 9 files changed, 635 insertions(+), 94 deletions(-) create mode 100644 tests/test_rockusb_install.py diff --git a/src/defib/cli/app.py b/src/defib/cli/app.py index 565470a..d13a1f2 100644 --- a/src/defib/cli/app.py +++ b/src/defib/cli/app.py @@ -28,6 +28,7 @@ def burn( usbplug: str = typer.Option("", "--usbplug", help="USB-recovery chips: usbplug blob (rkbin rv1106_usbplug_*.bin)"), loader: str = typer.Option("", "--loader", help="USB-recovery chips: RKBOOT container (MiniLoaderAll.bin), instead of --ddr/--usbplug"), wait: float = typer.Option(30.0, "--wait", help="USB-recovery chips: seconds to wait for the board to enumerate"), + usb_path: str = typer.Option("", "--usb-path", help="USB-recovery chips: pin to one physical port path (e.g. 1-4.2); required when several Rockchip boards are attached"), ) -> None: """Wake a dead device so its flash can be written. @@ -39,13 +40,14 @@ def burn( into MaskROM on its own at power-up, so --power-cycle is all it takes. """ import asyncio - asyncio.run(_burn_async(chip, file, port, send_break, terminal, power_cycle, poe_port_override, output, debug, ddr, usbplug, loader, wait)) + asyncio.run(_burn_async(chip, file, port, send_break, terminal, power_cycle, poe_port_override, output, debug, ddr, usbplug, loader, wait, usb_path)) async def _burn_async( chip: str, file: str, port: str, send_break: bool, terminal: bool, power_cycle: bool, poe_port_override: str, output: str, debug: bool, ddr: str = "", usbplug: str = "", loader: str = "", wait: float = 30.0, + usb_path: str = "", ) -> None: import json as json_mod import logging @@ -65,7 +67,10 @@ async def _burn_async( logging.basicConfig(level=logging.INFO) if recovery_mode(chip) == "usb": - await _burn_usb_async(chip, ddr, usbplug, loader, power_cycle, output, wait) + await _burn_usb_async( + chip, ddr, usbplug, loader, power_cycle, output, wait, + poe_port_override, usb_path, + ) return # Resolve firmware: local file or auto-download from OpenIPC @@ -2113,6 +2118,7 @@ def install( loader: str = typer.Option("", "--loader", help="USB-recovery chips: RKBOOT container (MiniLoaderAll.bin), instead of --ddr/--usbplug"), wait: float = typer.Option(30.0, "--wait", help="USB-recovery chips: seconds to wait for the board to enumerate"), verify: bool = typer.Option(False, "--verify", help="USB-recovery chips: read each image back and compare"), + usb_path: str = typer.Option("", "--usb-path", help="USB-recovery chips: pin to one physical port path (e.g. 1-4.2); required when several Rockchip boards are attached"), ) -> None: """Install a full OpenIPC firmware (U-Boot + kernel + rootfs). @@ -2127,7 +2133,7 @@ def install( asyncio.run(_install_async( chip, firmware, port, power_cycle, poe_port_override, nic, host_ip, device_ip, tftp_port, nor_size, nand, wipe_env, tftp_via, output, debug, - ddr, usbplug, loader, wait, verify, + ddr, usbplug, loader, wait, verify, usb_path, )) @@ -2209,6 +2215,7 @@ async def _install_async( loader: str = "", wait: float = 30.0, verify: bool = False, + usb_path: str = "", ) -> None: import hashlib import json as json_mod @@ -2244,7 +2251,7 @@ async def _install_async( if recovery_mode(chip) == "usb": await _install_usb_async( chip, firmware_path, ddr, usbplug, loader, - power_cycle, output, wait, verify, + power_cycle, output, wait, verify, poe_port_override, usb_path, ) return @@ -3698,6 +3705,7 @@ def _resolve_usb_loader(chip: str, ddr: str, usbplug: str, loader: str) -> Any: async def _open_usb_target( blobs: Any, power_cycle: bool, output: str, wait: float, + poe_port_override: str = "", usb_path: str = "", ) -> Any: """Power-cycle if asked, catch the board, and get its usbplug running. @@ -3717,11 +3725,11 @@ async def _open_usb_target( if output == "human": console.print(f" Power-cycling via {controller.name()}...") try: - await controller.power_cycle("") + await controller.power_cycle(poe_port_override) finally: await controller.close() - found = await wait_for_device(timeout=wait) + found = await wait_for_device(timeout=wait, usb_path=usb_path or None) if output == "human": console.print(f" Found {found}") @@ -3732,7 +3740,13 @@ async def _open_usb_target( if found.mode is DeviceMode.MASKROM: if output == "human": console.print(" Uploading DDR init and usbplug...") - await recovery.download_boot(blobs, on_progress=_usb_progress_printer(output)) + # Pin re-enumeration to the port this board is plugged into, so a + # second board appearing mid-upload cannot be adopted instead. + await recovery.download_boot( + blobs, + on_progress=_usb_progress_printer(output), + usb_path=found.usb_path, + ) return recovery @@ -3756,6 +3770,7 @@ def _usb_fail(output: str, message: str) -> None: async def _burn_usb_async( chip: str, ddr: str, usbplug: str, loader: str, power_cycle: bool, output: str, wait: float, + poe_port_override: str = "", usb_path: str = "", ) -> None: """``burn`` for chips whose boot ROM only answers on USB. @@ -3767,15 +3782,21 @@ async def _burn_usb_async( from rich.console import Console + from defib.rockusb.loader import LoaderFormatError from defib.rockusb.protocol import RockusbError console = Console() - blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) + # Loader resolution sits inside the handler: an unreadable file or a + # malformed container would otherwise escape as a traceback, and in JSON + # mode that means no error event at all. try: - recovery = await _open_usb_target(blobs, power_cycle, output, wait) + blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) + recovery = await _open_usb_target( + blobs, power_cycle, output, wait, poe_port_override, usb_path + ) flash_id = await recovery.read_flash_id() - except RockusbError as e: + except (RockusbError, LoaderFormatError, OSError) as e: _usb_fail(output, str(e)) return @@ -3800,11 +3821,23 @@ async def _burn_usb_async( "idblock.img": "idblock", } +# A firmware install is only complete with both of these. Writing a rootfs +# without the kernel beside it leaves an unbootable board that this command +# would otherwise call a success. +_USB_REQUIRED_PARTITIONS = ("boot", "rootfs") + +# Written last, whatever order the tarball lists things in. The boot ROM falls +# into MaskROM precisely because it finds no valid IDB, so committing the +# idblock before everything else is on flash trades the recovery path away: a +# failure after that point leaves a board that boots into a broken image +# instead of one that can be re-flashed over USB. +_USB_WRITE_LAST = ("idblock",) -def _map_usb_images(names: list[str], partitions: dict[str, int]) -> list[tuple[str, str, int]]: - """Match tarball members to partitions. - Returns ``(member, partition, lba)`` triples. +def _map_usb_images(names: list[str], partitions: dict[str, Any]) -> list[tuple[str, str, Any]]: + """Match tarball members to partitions, in a safe write order. + + Returns ``(member, partition, extent)`` triples. Raises: typer.BadParameter: if a member cannot be placed. The UBI case is @@ -3813,7 +3846,7 @@ def _map_usb_images(names: list[str], partitions: dict[str, int]) -> list[tuple[ volumes, so it does not correspond to any single partition in the vendor table. """ - mapped: list[tuple[str, str, int]] = [] + mapped: list[tuple[str, str, Any]] = [] for name in names: stem = name.rsplit(".", 1)[0] if "." in name else name partition = _USB_IMAGE_PARTITIONS.get(stem) @@ -3833,12 +3866,90 @@ def _map_usb_images(names: list[str], partitions: dict[str, int]) -> list[tuple[ f"{', '.join(sorted(partitions)) or '(none)'}" ) mapped.append((name, partition, partitions[partition])) + + mapped.sort(key=lambda item: item[1] in _USB_WRITE_LAST) return mapped +def _check_usb_image_fits(name: str, partition: str, extent: Any, size: int) -> None: + """Reject an image too big for the partition it is bound for. + + Raises: + typer.BadParameter: if it would spill past the end. The usbplug takes + a start sector and a count and writes what it is told, so an + oversized image quietly runs on into whatever follows. + """ + if size > extent.size_bytes: + raise typer.BadParameter( + f"{name} is {size} bytes but partition '{partition}' holds " + f"{extent.size_bytes} — it would overwrite whatever follows" + ) + + +def _read_usb_payloads(tar_path: Any, partitions: dict[str, Any]) -> list[tuple[str, str, Any, bytes]]: + """Extract, checksum and bounds-check every image the tarball places. + + Returns ``(member, partition, extent, data)`` in write order. + """ + import hashlib + import tarfile + + with tarfile.open(tar_path) as tar: + members = [m for m in tar.getmembers() if m.isfile()] + names = [m.name for m in members if not m.name.endswith(".md5sum")] + targets = _map_usb_images(names, partitions) + if not targets: + raise typer.BadParameter( + f"nothing in {tar_path.name} maps to a partition " + f"(saw: {', '.join(names) or 'no files'})" + ) + + placed = {partition for _, partition, _ in targets} + missing = [p for p in _USB_REQUIRED_PARTITIONS if p not in placed] + if missing: + raise typer.BadParameter( + f"{tar_path.name} has no image for {', '.join(missing)} — " + "a firmware install needs both a kernel and a rootfs, and " + "writing one without the other leaves an unbootable board" + ) + + def _member(name: str) -> bytes: + handle = tar.extractfile(name) + if handle is None: + raise typer.BadParameter(f"cannot read {name} from {tar_path.name}") + return handle.read() + + # The tarball ships a .md5sum beside each image. --verify only proves + # flash matches what we sent, so without this a corrupted download + # would be written and confirmed against its own corruption. + expected: dict[str, str] = {} + for member in members: + if not member.name.endswith(".md5sum"): + continue + line = _member(member.name).decode(errors="replace").strip() + if line: + expected[member.name.removesuffix(".md5sum")] = line.split()[0] + + payloads: list[tuple[str, str, Any, bytes]] = [] + for name, partition, extent in targets: + data = _member(name) + digest = expected.get(name) + if digest is not None: + actual = hashlib.md5(data).hexdigest() + if actual != digest: + raise typer.BadParameter( + f"MD5 mismatch for {name}: expected {digest}, got {actual}" + ) + _check_usb_image_fits(name, partition, extent, len(data)) + payloads.append((name, partition, extent, data)) + + return payloads + + async def _install_usb_async( chip: str, firmware_path: str, ddr: str, usbplug: str, loader: str, power_cycle: bool, output: str, wait: float, verify: bool, + poe_port_override: str = "", usb_path: str = "", ) -> None: """``install`` for chips whose boot ROM only answers on USB. @@ -3847,62 +3958,51 @@ async def _install_usb_async( TFTP or U-Boot console in the path at all. """ import json as json_mod - import tarfile from pathlib import Path from rich.console import Console from defib.profiles.loader import load_profile + from defib.rockusb.loader import LoaderFormatError from defib.rockusb.protocol import SECTOR_SIZE, ResetSubcode, RockusbError console = Console() - blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) partitions = load_profile(chip).partitions tar_path = Path(firmware_path) if not tar_path.exists(): raise typer.BadParameter(f"firmware not found: {firmware_path}") - with tarfile.open(tar_path) as tar: - members = [m for m in tar.getmembers() if m.isfile()] - names = [m.name for m in members if not m.name.endswith(".md5sum")] - targets = _map_usb_images(names, partitions) - if not targets: - raise typer.BadParameter( - f"nothing in {tar_path.name} maps to a partition " - f"(saw: {', '.join(names) or 'no files'})" - ) - payloads = [] - for name, partition, lba in targets: - handle = tar.extractfile(name) - if handle is None: - raise typer.BadParameter(f"cannot read {name} from {tar_path.name}") - payloads.append((name, partition, lba, handle.read())) + payloads = _read_usb_payloads(tar_path, partitions) try: - recovery = await _open_usb_target(blobs, power_cycle, output, wait) + blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) + recovery = await _open_usb_target( + blobs, power_cycle, output, wait, poe_port_override, usb_path + ) flash_id = await recovery.read_flash_id() if output == "human": console.print(f" Flash ID: {flash_id.hex()}") - for name, partition, lba, data in payloads: + for name, partition, extent, data in payloads: if output == "human": console.print( f" Writing {name} -> {partition} " - f"(LBA {lba}, {len(data)} bytes)..." + f"(LBA {extent.lba}, {len(data)} bytes)..." ) await recovery.write_image( - lba, data, on_progress=_usb_progress_printer(output) + extent.lba, data, on_progress=_usb_progress_printer(output) ) if verify: sectors = (len(data) + SECTOR_SIZE - 1) // SECTOR_SIZE - if (await recovery.read_image(lba, sectors))[: len(data)] != data: - _usb_fail(output, f"verify failed for {name} at LBA {lba}") + read_back = await recovery.read_image(extent.lba, sectors) + if read_back[: len(data)] != data: + _usb_fail(output, f"verify failed for {name} at LBA {extent.lba}") if output == "human": console.print(" Verified") await recovery.reset(ResetSubcode.NORMAL) - except RockusbError as e: + except (RockusbError, LoaderFormatError, OSError) as e: _usb_fail(output, str(e)) return diff --git a/src/defib/profiles/data/rv1106.json b/src/defib/profiles/data/rv1106.json index df7eccd..84b22ce 100644 --- a/src/defib/profiles/data/rv1106.json +++ b/src/defib/profiles/data/rv1106.json @@ -4,12 +4,12 @@ "LOADER_DDR": "rv1106_ddr_924MHz_v1.15.bin", "LOADER_USBPLUG": "rv1106_usbplug_v1.09.bin", "PARTITIONS": { - "env": 0, - "idblock": 512, - "uboot": 1024, - "boot": 2048, - "oem": 10240, - "userdata": 71680, - "rootfs": 92160 + "env": {"lba": 0, "sectors": 512}, + "idblock": {"lba": 512, "sectors": 512}, + "uboot": {"lba": 1024, "sectors": 1024}, + "boot": {"lba": 2048, "sectors": 8192}, + "oem": {"lba": 10240, "sectors": 61440}, + "userdata": {"lba": 71680, "sectors": 20480}, + "rootfs": {"lba": 92160, "sectors": 163840} } } diff --git a/src/defib/profiles/schema.py b/src/defib/profiles/schema.py index c517c2f..2635305 100644 --- a/src/defib/profiles/schema.py +++ b/src/defib/profiles/schema.py @@ -7,6 +7,26 @@ from pydantic import BaseModel, Field, PrivateAttr, model_validator +class FlashPartition(BaseModel): + """Where a partition lives, in 512-byte sectors. + + The size matters as much as the offset: without it an oversized image + would be written straight through into whatever follows. + """ + + lba: int = Field(description="Starting sector") + sectors: int = Field(description="Length in sectors") + + @property + def end_lba(self) -> int: + """First sector past this partition.""" + return self.lba + self.sectors + + @property + def size_bytes(self) -> int: + return self.sectors * 512 + + class SoCProfile(BaseModel): """A SoC configuration profile. @@ -87,12 +107,12 @@ class SoCProfile(BaseModel): "over USB and exposes flash (rkbin's rv1106_usbplug_*.bin)." ), ) - partitions: dict[str, int] = Field( + partitions: dict[str, FlashPartition] = Field( default_factory=dict, alias="PARTITIONS", description=( - "USB recovery only. Partition name to starting LBA (512-byte " - "sectors), used to place firmware images without the caller " - "computing offsets." + "USB recovery only. Partition name to its extent, used to place " + "firmware images without the caller computing offsets and to " + "reject images that would not fit." ), ) diff --git a/src/defib/rockusb/device.py b/src/defib/rockusb/device.py index 458b190..693d909 100644 --- a/src/defib/rockusb/device.py +++ b/src/defib/rockusb/device.py @@ -72,11 +72,25 @@ class FoundDevice: address: int product_id: int handle: Any # usb.core.Device + port_numbers: tuple[int, ...] = () + + @property + def usb_path(self) -> str: + """Physical topology path, e.g. ``1-4.2``. + + This is the only stable identity a Rockchip board has across the + MaskROM-to-loader transition: the USB address is reassigned when the + usbplug re-enumerates, and every board shares one VID:PID. The port + path is where it is plugged in, so it survives. + """ + if not self.port_numbers: + return f"{self.bus}-?" + return f"{self.bus}-{'.'.join(str(p) for p in self.port_numbers)}" def __str__(self) -> str: return ( f"{self.mode.value} device {ROCKCHIP_VID:04x}:{self.product_id:04x} " - f"at bus {self.bus} addr {self.address}" + f"at {self.usb_path} (bus {self.bus} addr {self.address})" ) @@ -91,49 +105,76 @@ def _classify(dev: Any) -> DeviceMode: return DeviceMode.MASKROM if not (dev.bcdUSB & 0x0001) else DeviceMode.LOADER -def find_device(product_id: int | None = None) -> FoundDevice | None: - """Return the first Rockchip device on the bus, or None.""" +def find_devices(usb_path: str | None = None) -> list[FoundDevice]: + """Every Rockchip device on the bus, optionally filtered to one port path.""" usb = _require_usb() - kwargs: dict[str, Any] = {"idVendor": ROCKCHIP_VID, "find_all": True} - if product_id is not None: - kwargs["idProduct"] = product_id - - for dev in usb.core.find(**kwargs): - return FoundDevice( + out: list[FoundDevice] = [] + for dev in usb.core.find(idVendor=ROCKCHIP_VID, find_all=True): + found = FoundDevice( mode=_classify(dev), bus=dev.bus, address=dev.address, product_id=dev.idProduct, handle=dev, + port_numbers=tuple(getattr(dev, "port_numbers", None) or ()), + ) + if usb_path is None or found.usb_path == usb_path: + out.append(found) + return out + + +def find_device(usb_path: str | None = None) -> FoundDevice | None: + """A single Rockchip device, or None. + + Raises: + RockusbUsbError: if more than one is present and no ``usb_path`` + narrows it down. Picking one arbitrarily would mean flashing + whichever board happened to enumerate first, which on a rack is + how you write firmware to the wrong device. + """ + devices = find_devices(usb_path) + if not devices: + return None + if len(devices) > 1: + paths = ", ".join(sorted(d.usb_path for d in devices)) + raise RockusbUsbError( + f"{len(devices)} Rockchip devices present ({paths}) — " + "narrow it down with --usb-path" ) - return None + return devices[0] async def wait_for_device( timeout: float = 30.0, mode: DeviceMode | None = None, poll_interval: float = 0.25, + usb_path: str | None = None, ) -> FoundDevice: """Poll until a matching device appears. Used both for the initial "power-cycle an erased board and catch it in MaskROM" step and for the re-enumeration that follows the usbplug upload. + Pass ``usb_path`` for the latter: the board keeps its port path across + re-enumeration, so pinning it stops the session hopping to a different + board that happened to appear meanwhile. Args: timeout: seconds to keep looking. mode: require this stage; None accepts either. poll_interval: seconds between scans. + usb_path: restrict to this physical port path (e.g. ``1-4.2``). Raises: - RockusbUsbError: if nothing matching shows up in time. + RockusbUsbError: if nothing matching shows up in time, or if several + candidates are present and ``usb_path`` does not disambiguate. """ loop = asyncio.get_event_loop() deadline = loop.time() + timeout seen: str | None = None while loop.time() < deadline: - found = await asyncio.to_thread(find_device) + found = await asyncio.to_thread(find_device, usb_path) if found is not None: if mode is None or found.mode is mode: return found @@ -141,12 +182,14 @@ async def wait_for_device( await asyncio.sleep(poll_interval) want = f" in {mode.value} mode" if mode else "" + where = f" at {usb_path}" if usb_path else "" if seen: raise RockusbUsbError( - f"no Rockchip device{want} after {timeout:.0f}s — saw {seen} instead" + f"no Rockchip device{want}{where} after {timeout:.0f}s — " + f"saw {seen} instead" ) raise RockusbUsbError( - f"no Rockchip device{want} after {timeout:.0f}s. " + f"no Rockchip device{want}{where} after {timeout:.0f}s. " "Power-cycle the board; an erased flash enters MaskROM on its own." ) @@ -165,7 +208,7 @@ def __init__(self, found: FoundDevice, timeout_ms: int = 5000) -> None: self._interface: Any = None self._ep_in: Any = None self._ep_out: Any = None - self._detached = False + self._detached_interface: int | None = None @property def mode(self) -> DeviceMode: @@ -187,15 +230,6 @@ def open(self) -> None: usb = _require_usb() dev = self._dev - try: - if dev.is_kernel_driver_active(0): - dev.detach_kernel_driver(0) - self._detached = True - except (NotImplementedError, usb.core.USBError): - # Not all backends/platforms implement this; only Linux binds a - # kernel driver here in the first place. - pass - try: dev.set_configuration() except usb.core.USBError as e: @@ -236,11 +270,25 @@ def open(self) -> None: f"{self._found}: no bulk IN/OUT endpoint pair found" ) + # Detach from the interface actually being claimed, not interface 0 — + # the bulk pair is not guaranteed to live on the first one, and + # detaching the wrong interface leaves the right one bound and the + # claim below failing. + number = self._interface.bInterfaceNumber + try: + if dev.is_kernel_driver_active(number): + dev.detach_kernel_driver(number) + self._detached_interface = number + except (NotImplementedError, usb.core.USBError): + # Not all backends/platforms implement this; only Linux binds a + # kernel driver here in the first place. + pass + try: - usb.util.claim_interface(dev, self._interface.bInterfaceNumber) + usb.util.claim_interface(dev, number) except usb.core.USBError as e: raise RockusbUsbError( - f"cannot claim interface on {self._found}: {e} " + f"cannot claim interface {number} on {self._found}: {e} " "(need a udev rule for 2207:* or root)" ) from e @@ -250,8 +298,8 @@ def close(self) -> None: if self._interface is not None: usb.util.release_interface(self._dev, self._interface.bInterfaceNumber) usb.util.dispose_resources(self._dev) - if self._detached: - self._dev.attach_kernel_driver(0) + if self._detached_interface is not None: + self._dev.attach_kernel_driver(self._detached_interface) except Exception: # pragma: no cover - teardown is best-effort logger.debug("cleanup failed for %s", self._found, exc_info=True) @@ -299,7 +347,11 @@ def command( Returns any data read during an IN transfer, otherwise ``b""``. Raises: - RockusbUsbError: on a transfer error or a non-zero status. + RockusbUsbError: on a transfer error, a non-zero status, or a + short transfer. A device is entitled to move less than it was + asked to and still report OK, reporting the shortfall in the + status wrapper's residue; treating that as success is how a + partial write gets announced as a finished install. """ self._require_bulk() tag = secrets.randbits(32) @@ -335,4 +387,15 @@ def command( f"rockusb command {int(opcode):#04x} failed " f"(status {status}, residue {residue})" ) + if residue: + raise RockusbUsbError( + f"rockusb command {int(opcode):#04x} moved " + f"{transfer_length - residue} of {transfer_length} bytes " + f"(residue {residue})" + ) + if direction_in and len(payload) != read_length: + raise RockusbUsbError( + f"rockusb command {int(opcode):#04x} returned " + f"{len(payload)} of {read_length} bytes" + ) return payload diff --git a/src/defib/rockusb/loader.py b/src/defib/rockusb/loader.py index e8b9201..64e0959 100644 --- a/src/defib/rockusb/loader.py +++ b/src/defib/rockusb/loader.py @@ -61,7 +61,14 @@ def raw_blobs(ddr: bytes, usbplug: bytes, *, use_rc4: bool = False) -> LoaderBlo For RV1106 this is the normal path, since rkbin ships its DDR and usbplug images headerless. + + Raises: + LoaderFormatError: if either image is empty — uploading nothing would + look like a board that never came back. """ + empty = [n for n, b in (("ddr", ddr), ("usbplug", usbplug)) if not b] + if empty: + raise LoaderFormatError(f"{' and '.join(empty)} image is empty") return LoaderBlobs( ddr=[LoaderEntry(name="ddr", data=ddr, delay_ms=0)], usbplug=[LoaderEntry(name="usbplug", data=usbplug, delay_ms=0)], @@ -145,6 +152,21 @@ def parse_loader(data: bytes) -> LoaderBlobs: flags_at = table_at + 6 + 6 + 6 rc4_flag = data[flags_at + 1] + # Both stages are mandatory. A container missing either would parse fine + # and then upload nothing, leaving the caller waiting out a re-enumeration + # that was never going to happen — a timeout that blames the board for a + # bad file. + missing = [ + name + for name, count in (("DDR init (471)", n471), ("usbplug (472)", n472)) + if count == 0 + ] + if missing: + raise LoaderFormatError( + f"loader declares no {' or '.join(missing)} entries — " + "it cannot bring a board up" + ) + return LoaderBlobs( ddr=_parse_entries(data, off471, size471, n471), usbplug=_parse_entries(data, off472, size472, n472), diff --git a/src/defib/rockusb/recovery.py b/src/defib/rockusb/recovery.py index 2521404..0bcde4e 100644 --- a/src/defib/rockusb/recovery.py +++ b/src/defib/rockusb/recovery.py @@ -61,11 +61,18 @@ async def download_boot( blobs: LoaderBlobs, on_progress: Callable[[ProgressEvent], None] | None = None, reenumerate_timeout: float = 15.0, + usb_path: str | None = None, ) -> RockusbDevice: """Upload DDR init then usbplug, and wait for the device to come back. Returns the *new* opened device — the old handle is stale once the usbplug re-enumerates, so callers must use the returned one. + + Pass ``usb_path`` to require that the device coming back is the same + one that went away. Every Rockchip board shares a VID:PID and gets a + fresh USB address on re-enumeration, so without it a second board + appearing mid-upload could be adopted by this session and flashed by + mistake. """ if self._device.mode is not DeviceMode.MASKROM: logger.info("device already past MaskROM; skipping loader upload") @@ -89,7 +96,7 @@ async def download_boot( await asyncio.sleep(USBPLUG_SETTLE) found = await wait_for_device( - timeout=reenumerate_timeout, mode=DeviceMode.LOADER + timeout=reenumerate_timeout, mode=DeviceMode.LOADER, usb_path=usb_path ) device = RockusbDevice(found) device.open() diff --git a/tests/test_profiles_usb_recovery.py b/tests/test_profiles_usb_recovery.py index 28d950e..03caf95 100644 --- a/tests/test_profiles_usb_recovery.py +++ b/tests/test_profiles_usb_recovery.py @@ -12,9 +12,9 @@ import pytest from pydantic import ValidationError -from defib.cli.app import _map_usb_images +from defib.cli.app import _check_usb_image_fits, _map_usb_images from defib.profiles.loader import load_profile, recovery_mode -from defib.profiles.schema import SoCProfile +from defib.profiles.schema import FlashPartition, SoCProfile PROFILES_DIR = Path(__file__).parent.parent / "src" / "defib" / "profiles" / "data" @@ -79,21 +79,34 @@ def test_partition_lbas_match_the_vendor_byte_layout(self): """Luckfox SPI NAND: 256K(env) 256K@256K(idblock) 512K(uboot) 4M(boot) 30M(oem) 10M(userdata) 80M(rootfs), converted to 512-byte sectors.""" partitions = load_profile("rv1106", PROFILES_DIR).partitions - expected_bytes = { - "env": 0, - "idblock": 256 * 1024, - "uboot": 512 * 1024, - "boot": 1024 * 1024, - "oem": 5 * 1024 * 1024, - "userdata": 35 * 1024 * 1024, - "rootfs": 45 * 1024 * 1024, + K, M = 1024, 1024 * 1024 + expected = { + "env": (0, 256 * K), + "idblock": (256 * K, 256 * K), + "uboot": (512 * K, 512 * K), + "boot": (1 * M, 4 * M), + "oem": (5 * M, 30 * M), + "userdata": (35 * M, 10 * M), + "rootfs": (45 * M, 80 * M), } - assert partitions == {k: v // 512 for k, v in expected_bytes.items()} + actual = { + name: (p.lba * 512, p.size_bytes) for name, p in partitions.items() + } + assert actual == expected + + def test_partitions_tile_without_gaps_or_overlap(self): + """A gap or an overlap here would show up as one image silently + landing inside another.""" + partitions = load_profile("rv1106", PROFILES_DIR).partitions + extents = sorted(partitions.values(), key=lambda p: p.lba) + for lower, upper in zip(extents, extents[1:]): + assert lower.end_lba == upper.lba def test_idblock_stays_at_its_fixed_offset(self): """The boot ROM looks for the IDB at 0x40000; moving it bricks the board in a way no button recovers.""" - assert load_profile("rv1106", PROFILES_DIR).partitions["idblock"] * 512 == 0x40000 + partitions = load_profile("rv1106", PROFILES_DIR).partitions + assert partitions["idblock"].lba * 512 == 0x40000 def test_profile_json_is_minimal(self): """A USB profile carrying UART bytecode would mean someone copied the @@ -103,17 +116,38 @@ def test_profile_json_is_minimal(self): class TestMapUsbImages: - PARTS = {"boot": 2048, "rootfs": 92160, "uboot": 1024, "idblock": 512} + PARTS = { + "idblock": FlashPartition(lba=512, sectors=512), + "uboot": FlashPartition(lba=1024, sectors=1024), + "boot": FlashPartition(lba=2048, sectors=8192), + "rootfs": FlashPartition(lba=92160, sectors=163840), + } def test_maps_nor_style_tarball(self): out = _map_usb_images( ["zboot.img.rv1106", "rootfs.squashfs.rv1106"], self.PARTS ) - assert out == [ + assert [(name, part, ext.lba) for name, part, ext in out] == [ ("zboot.img.rv1106", "boot", 2048), ("rootfs.squashfs.rv1106", "rootfs", 92160), ] + def test_idblock_is_written_last(self): + """The boot ROM enters MaskROM precisely because it finds no valid + IDB. Committing one before the rest of flash is populated trades that + recovery path away if a later write fails.""" + out = _map_usb_images( + ["idblock.img.rv1106", "zboot.img.rv1106", "rootfs.squashfs.rv1106"], + self.PARTS, + ) + assert [part for _, part, _ in out][-1] == "idblock" + + def test_order_is_otherwise_preserved(self): + out = _map_usb_images( + ["rootfs.squashfs.rv1106", "zboot.img.rv1106"], self.PARTS + ) + assert [part for _, part, _ in out] == ["rootfs", "boot"] + def test_ubi_image_refused_with_the_reason(self): """Not a mapping gap — a UBI bundles kernel and rootfs volumes, so no single partition is the right answer.""" @@ -126,7 +160,10 @@ def test_undeclared_partition_is_an_error_not_a_silent_skip(self): import typer with pytest.raises(typer.BadParameter, match="does not declare"): - _map_usb_images(["zboot.img.rv1106"], {"rootfs": 1}) + _map_usb_images( + ["zboot.img.rv1106"], + {"rootfs": FlashPartition(lba=1, sectors=1)}, + ) def test_unrecognised_files_are_ignored(self): assert _map_usb_images(["README", "notes.txt"], self.PARTS) == [] @@ -134,3 +171,36 @@ def test_unrecognised_files_are_ignored(self): def test_checksums_are_not_mistaken_for_images(self): out = _map_usb_images(["zboot.img.rv1106.md5sum"], self.PARTS) assert out == [] + + +class TestImageFits: + BOOT = FlashPartition(lba=2048, sectors=8192) # 4 MB + + def test_image_within_bounds_passes(self): + _check_usb_image_fits("zboot.img", "boot", self.BOOT, self.BOOT.size_bytes) + + def test_oversized_image_refused(self): + """The usbplug writes what it is told from a start sector, so an + oversized image runs straight on into the next partition.""" + import typer + + with pytest.raises(typer.BadParameter, match="would overwrite"): + _check_usb_image_fits( + "zboot.img", "boot", self.BOOT, self.BOOT.size_bytes + 1 + ) + + def test_error_names_both_sizes(self): + import typer + + with pytest.raises(typer.BadParameter) as excinfo: + _check_usb_image_fits("zboot.img", "boot", self.BOOT, 9_000_000) + assert "9000000" in str(excinfo.value) + assert str(self.BOOT.size_bytes) in str(excinfo.value) + + +class TestFlashPartition: + def test_end_lba(self): + assert FlashPartition(lba=100, sectors=50).end_lba == 150 + + def test_size_bytes(self): + assert FlashPartition(lba=0, sectors=8192).size_bytes == 4 * 1024 * 1024 diff --git a/tests/test_rockusb_install.py b/tests/test_rockusb_install.py new file mode 100644 index 0000000..c76065a --- /dev/null +++ b/tests/test_rockusb_install.py @@ -0,0 +1,222 @@ +"""Tests for USB-recovery tarball handling and device selection. + +These cover the guards that stop a flash going wrong in a way the operator +would not notice: a truncated tarball, a corrupted image, an oversized one, a +partial transfer the device reported as OK, or the wrong board entirely. +""" + +import hashlib +import io +import tarfile + +import pytest +import typer + +from defib.cli.app import _read_usb_payloads +from defib.profiles.schema import FlashPartition +from defib.rockusb.device import DeviceMode, FoundDevice +from defib.rockusb.protocol import CSW_SIGNATURE, Opcode + +PARTS = { + "idblock": FlashPartition(lba=512, sectors=512), + "uboot": FlashPartition(lba=1024, sectors=1024), + "boot": FlashPartition(lba=2048, sectors=8192), + "rootfs": FlashPartition(lba=92160, sectors=163840), +} + + +def _make_tarball(tmp_path, files: dict[str, bytes], *, checksums=True, corrupt=()): + """Build an OpenIPC-shaped tarball, optionally with bad checksums.""" + path = tmp_path / "fw.tgz" + with tarfile.open(path, "w:gz") as tar: + for name, data in files.items(): + info = tarfile.TarInfo(name) + info.size = len(data) + tar.addfile(info, io.BytesIO(data)) + if not checksums: + continue + digest = hashlib.md5(data).hexdigest() + if name in corrupt: + digest = "0" * 32 + line = f"{digest} {name}\n".encode() + sig = tarfile.TarInfo(f"{name}.md5sum") + sig.size = len(line) + tar.addfile(sig, io.BytesIO(line)) + return path + + +def _complete(**overrides) -> dict[str, bytes]: + files = { + "zboot.img.rv1106": b"\xaa" * 2048, + "rootfs.squashfs.rv1106": b"\xbb" * 4096, + } + files.update(overrides) + return files + + +class TestReadUsbPayloads: + def test_reads_a_complete_tarball(self, tmp_path): + payloads = _read_usb_payloads(_make_tarball(tmp_path, _complete()), PARTS) + assert [(n, p) for n, p, _, _ in payloads] == [ + ("zboot.img.rv1106", "boot"), + ("rootfs.squashfs.rv1106", "rootfs"), + ] + + def test_returns_image_bytes(self, tmp_path): + payloads = _read_usb_payloads(_make_tarball(tmp_path, _complete()), PARTS) + assert dict((n, d) for n, _, _, d in payloads)["zboot.img.rv1106"] == b"\xaa" * 2048 + + def test_missing_rootfs_refused(self, tmp_path): + """Half a firmware written and called a success leaves an unbootable + board — the UART installer rejects this too.""" + tar = _make_tarball(tmp_path, {"zboot.img.rv1106": b"\xaa" * 16}) + with pytest.raises(typer.BadParameter, match="no image for rootfs"): + _read_usb_payloads(tar, PARTS) + + def test_missing_kernel_refused(self, tmp_path): + tar = _make_tarball(tmp_path, {"rootfs.squashfs.rv1106": b"\xbb" * 16}) + with pytest.raises(typer.BadParameter, match="no image for boot"): + _read_usb_payloads(tar, PARTS) + + def test_empty_tarball_refused(self, tmp_path): + tar = _make_tarball(tmp_path, {"README": b"nothing here"}) + with pytest.raises(typer.BadParameter, match="maps to a partition"): + _read_usb_payloads(tar, PARTS) + + def test_idblock_ordered_last(self, tmp_path): + files = _complete() + files["idblock.img.rv1106"] = b"\xcc" * 512 + payloads = _read_usb_payloads(_make_tarball(tmp_path, files), PARTS) + assert [p for _, p, _, _ in payloads][-1] == "idblock" + + +class TestChecksums: + def test_corrupted_image_refused(self, tmp_path): + """--verify compares flash against what was sent, so it cannot catch a + bad download; only the shipped md5sum can.""" + tar = _make_tarball(tmp_path, _complete(), corrupt=("zboot.img.rv1106",)) + with pytest.raises(typer.BadParameter, match="MD5 mismatch"): + _read_usb_payloads(tar, PARTS) + + def test_error_names_both_digests(self, tmp_path): + tar = _make_tarball(tmp_path, _complete(), corrupt=("zboot.img.rv1106",)) + with pytest.raises(typer.BadParameter) as excinfo: + _read_usb_payloads(tar, PARTS) + message = str(excinfo.value) + assert "0" * 32 in message + assert hashlib.md5(b"\xaa" * 2048).hexdigest() in message + + def test_tarball_without_checksums_still_works(self, tmp_path): + """Absent checksums are not an error — only mismatching ones are.""" + tar = _make_tarball(tmp_path, _complete(), checksums=False) + assert len(_read_usb_payloads(tar, PARTS)) == 2 + + +class TestBounds: + def test_oversized_image_refused(self, tmp_path): + files = _complete(**{"zboot.img.rv1106": b"\xaa" * (PARTS["boot"].size_bytes + 1)}) + with pytest.raises(typer.BadParameter, match="would overwrite"): + _read_usb_payloads(_make_tarball(tmp_path, files), PARTS) + + def test_exactly_full_partition_accepted(self, tmp_path): + files = _complete(**{"zboot.img.rv1106": b"\xaa" * PARTS["boot"].size_bytes}) + assert len(_read_usb_payloads(_make_tarball(tmp_path, files), PARTS)) == 2 + + +class TestFoundDevice: + def _dev(self, bus=1, ports=(4, 2), mode=DeviceMode.MASKROM): + return FoundDevice( + mode=mode, bus=bus, address=7, product_id=0x110C, + handle=None, port_numbers=ports, + ) + + def test_usb_path_is_the_topology_path(self): + assert self._dev().usb_path == "1-4.2" + + def test_usb_path_survives_readdressing(self): + """Address changes when the usbplug re-enumerates; the port path is + what identifies the same physical board across that.""" + before = self._dev() + after = FoundDevice( + mode=DeviceMode.LOADER, bus=1, address=9, product_id=0x110C, + handle=None, port_numbers=(4, 2), + ) + assert before.usb_path == after.usb_path + assert before.address != after.address + + def test_distinct_ports_are_distinct_paths(self): + assert self._dev(ports=(4, 2)).usb_path != self._dev(ports=(4, 3)).usb_path + + def test_missing_port_numbers_degrade_visibly(self): + assert self._dev(ports=()).usb_path == "1-?" + + def test_str_mentions_the_path(self): + assert "1-4.2" in str(self._dev()) + + +class TestResidueHandling: + """A device may move less than asked and still report status OK.""" + + class _FakeEndpoint: + def __init__(self, replies=None): + self.written = [] + self._replies = list(replies or []) + + def write(self, data, timeout=None): + self.written.append(bytes(data)) + return len(data) + + def read(self, size, timeout=None): + return self._replies.pop(0) + + def _device(self, residue: int, status: int = 0): + from defib.rockusb.device import RockusbDevice + + found = FoundDevice( + mode=DeviceMode.LOADER, bus=1, address=1, product_id=0x110C, + handle=None, port_numbers=(1,), + ) + device = RockusbDevice(found) + device._ep_out = self._FakeEndpoint() + # CSW tag is echoed from the CBW the device just received. + device._ep_in = self._FakeEndpoint() + + import struct + + def read(size, timeout=None): + cbw = device._ep_out.written[0] + tag = struct.unpack_from(" bytes: + data = bytearray(_build_loader()) + data[TABLE_AT] = n471 + data[TABLE_AT + 6] = n472 + return bytes(data) + + def test_no_ddr_entries_rejected(self): + with pytest.raises(LoaderFormatError, match="DDR init"): + parse_loader(self._with_counts(0, 1)) + + def test_no_usbplug_entries_rejected(self): + with pytest.raises(LoaderFormatError, match="usbplug"): + parse_loader(self._with_counts(1, 0)) + + def test_neither_stage_names_both(self): + with pytest.raises(LoaderFormatError) as excinfo: + parse_loader(self._with_counts(0, 0)) + assert "DDR init" in str(excinfo.value) + assert "usbplug" in str(excinfo.value) + + class TestRawBlobs: def test_wraps_bare_images(self): blobs = raw_blobs(b"\x01" * 16, b"\x02" * 32) @@ -137,3 +166,11 @@ def test_defaults_to_rc4_off(self): def test_rc4_can_be_forced_on(self): assert raw_blobs(b"a", b"b", use_rc4=True).use_rc4 is True + + def test_empty_ddr_rejected(self): + with pytest.raises(LoaderFormatError, match="ddr"): + raw_blobs(b"", b"b") + + def test_empty_usbplug_rejected(self): + with pytest.raises(LoaderFormatError, match="usbplug"): + raw_blobs(b"a", b"") From 83ae4aa9c6c428ebc354d947c480e00b507db47a Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Tue, 18 Aug 2026 22:23:12 +0300 Subject: [PATCH 5/8] rockusb: stop short transfers and stray images passing as success MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review pass. All six are variants of the same failure: something did not fully happen, and nothing said so. Foreign SoC images. Member names were matched on their stem and the suffix discarded, so an OpenIPC tarball built for another chip mapped cleanly onto these partitions and installed a kernel this board cannot boot. That suffix is the only thing distinguishing the two, so it is now checked against the chip. Short transfers, three places. pyusb reports how many bytes it actually moved, and all three call sites threw that away: the bulk command wrapper, the bulk payload, and the MaskROM control transfer. A short payload write is a partial flash write wearing the costume of a finished one; a short control transfer is a truncated loader that only surfaces later as a re-enumeration timeout blaming the board. Progress now counts what the wire took rather than what it was handed. Reset failures. reset() swallowed every error, on the reasoning that a board is entitled to vanish while acknowledging its own reset. True of the status read, not of the command itself — a reset that never went out, or came back with an explicit failure, was still reported as a completed install. Only the missing status wrapper is tolerated now, and only when the caller asks for it. Detached driver left behind. A failed interface claim raised without restoring the kernel driver it had just detached, so a failed attempt stranded the interface for whatever owned it. Archive errors in JSON mode. The tarball was opened and validated before the handler that emits structured errors, so a corrupt or incomplete archive produced Typer text or a traceback instead of the error event the JSON contract promises. Human mode keeps Typer's nicer rendering. Co-Authored-By: Claude Opus 4.8 --- src/defib/cli/app.py | 50 ++++++-- src/defib/rockusb/device.py | 88 ++++++++++++-- src/defib/rockusb/recovery.py | 27 +++-- tests/test_rockusb_install.py | 222 ++++++++++++++++++++++++++++++++++ 4 files changed, 357 insertions(+), 30 deletions(-) diff --git a/src/defib/cli/app.py b/src/defib/cli/app.py index d13a1f2..26ed0e1 100644 --- a/src/defib/cli/app.py +++ b/src/defib/cli/app.py @@ -3834,21 +3834,33 @@ async def _burn_usb_async( _USB_WRITE_LAST = ("idblock",) -def _map_usb_images(names: list[str], partitions: dict[str, Any]) -> list[tuple[str, str, Any]]: +def _map_usb_images( + names: list[str], partitions: dict[str, Any], chip: str = "", +) -> list[tuple[str, str, Any]]: """Match tarball members to partitions, in a safe write order. Returns ``(member, partition, extent)`` triples. + Args: + names: tarball member names, e.g. ``zboot.img.rv1106``. + partitions: the chip profile's partition table. + chip: when given, the SoC suffix every member must carry. OpenIPC + names each image for the SoC it was built for, and that suffix is + the only thing separating an RV1106 kernel from one that would + leave this board unbootable. + Raises: - typer.BadParameter: if a member cannot be placed. The UBI case is - called out by name because it is not a mapping gap but a real - layout question: ``rootfs.ubi`` bundles kernel *and* rootfs as UBI - volumes, so it does not correspond to any single partition in the - vendor table. + typer.BadParameter: if a member cannot be placed, or was built for a + different SoC. The UBI case is called out by name because it is + not a mapping gap but a real layout question: ``rootfs.ubi`` + bundles kernel *and* rootfs as UBI volumes, so it does not + correspond to any single partition in the vendor table. """ mapped: list[tuple[str, str, Any]] = [] for name in names: - stem = name.rsplit(".", 1)[0] if "." in name else name + stem, _, suffix = name.rpartition(".") + if not stem: + stem, suffix = name, "" partition = _USB_IMAGE_PARTITIONS.get(stem) if partition is None: if stem.startswith("rootfs.ubi"): @@ -3859,6 +3871,11 @@ def _map_usb_images(names: list[str], partitions: dict[str, Any]) -> list[tuple[ "decide the UBI region for this board first." ) continue + if chip and suffix.lower() != chip.lower(): + raise typer.BadParameter( + f"{name} was built for '{suffix}', not '{chip}' — flashing " + "another SoC's image would leave this board unbootable" + ) if partition not in partitions: raise typer.BadParameter( f"{name} belongs in partition '{partition}', which this " @@ -3886,7 +3903,9 @@ def _check_usb_image_fits(name: str, partition: str, extent: Any, size: int) -> ) -def _read_usb_payloads(tar_path: Any, partitions: dict[str, Any]) -> list[tuple[str, str, Any, bytes]]: +def _read_usb_payloads( + tar_path: Any, partitions: dict[str, Any], chip: str = "", +) -> list[tuple[str, str, Any, bytes]]: """Extract, checksum and bounds-check every image the tarball places. Returns ``(member, partition, extent, data)`` in write order. @@ -3897,7 +3916,7 @@ def _read_usb_payloads(tar_path: Any, partitions: dict[str, Any]) -> list[tuple[ with tarfile.open(tar_path) as tar: members = [m for m in tar.getmembers() if m.isfile()] names = [m.name for m in members if not m.name.endswith(".md5sum")] - targets = _map_usb_images(names, partitions) + targets = _map_usb_images(names, partitions, chip) if not targets: raise typer.BadParameter( f"nothing in {tar_path.name} maps to a partition " @@ -3958,6 +3977,7 @@ async def _install_usb_async( TFTP or U-Boot console in the path at all. """ import json as json_mod + import tarfile from pathlib import Path from rich.console import Console @@ -3973,7 +3993,17 @@ async def _install_usb_async( if not tar_path.exists(): raise typer.BadParameter(f"firmware not found: {firmware_path}") - payloads = _read_usb_payloads(tar_path, partitions) + # In JSON mode every failure has to arrive as a structured event, so + # archive and validation errors are funnelled through _usb_fail rather + # than surfacing as Typer's own text (or, for a corrupt archive, a + # traceback). Human mode keeps Typer's nicer rendering. + try: + payloads = _read_usb_payloads(tar_path, partitions, chip) + except (typer.BadParameter, tarfile.TarError, OSError) as e: + if output != "json": + raise + _usb_fail(output, str(e)) + return try: blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) diff --git a/src/defib/rockusb/device.py b/src/defib/rockusb/device.py index 693d909..28a5ccf 100644 --- a/src/defib/rockusb/device.py +++ b/src/defib/rockusb/device.py @@ -287,36 +287,69 @@ def open(self) -> None: try: usb.util.claim_interface(dev, number) except usb.core.USBError as e: + # Give the kernel driver back before bailing out. Leaving it + # detached would strand the interface for whatever owned it, + # turning a failed attempt into a lasting one. + self._reattach_kernel_driver() raise RockusbUsbError( f"cannot claim interface {number} on {self._found}: {e} " "(need a udev rule for 2207:* or root)" ) from e + def _reattach_kernel_driver(self) -> None: + """Undo a detach, if we did one. Best-effort by nature.""" + if self._detached_interface is None: + return + try: + self._dev.attach_kernel_driver(self._detached_interface) + except Exception: # pragma: no cover - platform dependent + logger.debug( + "could not reattach kernel driver to interface %d", + self._detached_interface, exc_info=True, + ) + finally: + self._detached_interface = None + def close(self) -> None: usb = _require_usb() try: if self._interface is not None: usb.util.release_interface(self._dev, self._interface.bInterfaceNumber) usb.util.dispose_resources(self._dev) - if self._detached_interface is not None: - self._dev.attach_kernel_driver(self._detached_interface) except Exception: # pragma: no cover - teardown is best-effort logger.debug("cleanup failed for %s", self._found, exc_info=True) + self._reattach_kernel_driver() # -- MaskROM stage ---------------------------------------------------- def control_write(self, code: int, payload: bytes) -> int: - """One MaskROM code-upload control transfer.""" + """One MaskROM code-upload control transfer. + + Returns the number of bytes the host actually pushed. + + Raises: + RockusbUsbError: on a transfer error, or a short write. A + truncated loader upload surfaces much later as a + re-enumeration timeout that looks like a dead board, so it is + caught where it happens. + """ try: - written = self._dev.ctrl_transfer( + written = int(self._dev.ctrl_transfer( bmRequestType=0x40, bRequest=0x0C, wValue=0x0000, wIndex=code, data_or_wLength=payload, timeout=self._timeout_ms, - ) - return int(written) + )) + if written != len(payload): + raise RockusbUsbError( + f"MaskROM control transfer short (code {code:#06x}): " + f"wrote {written} of {len(payload)} bytes" + ) + return written + except RockusbUsbError: + raise except Exception as e: raise RockusbUsbError( f"MaskROM control transfer failed (code {code:#06x}, " @@ -325,6 +358,25 @@ def control_write(self, code: int, payload: bytes) -> int: # -- rockusb bulk stage ----------------------------------------------- + def _write_bulk(self, data: bytes, what: str, opcode: Opcode | int) -> None: + """Push ``data`` out the bulk endpoint, insisting all of it lands. + + pyusb returns how many bytes it managed; a short count means the + device took less than we handed it, which for a payload write is a + partial flash write wearing the costume of a successful one. + """ + try: + written = int(self._ep_out.write(data, self._timeout_ms)) + except Exception as e: + raise RockusbUsbError( + f"rockusb {what} write failed (opcode {int(opcode):#04x}): {e}" + ) from e + if written != len(data): + raise RockusbUsbError( + f"rockusb {what} write short (opcode {int(opcode):#04x}): " + f"wrote {written} of {len(data)} bytes" + ) + def _require_bulk(self) -> None: if self._ep_in is None or self._ep_out is None: raise RockusbUsbError( @@ -341,6 +393,7 @@ def command( count: int = 0, data_out: bytes | None = None, read_length: int = 0, + tolerate_disconnect: bool = False, ) -> bytes: """Run one CBW / optional data phase / CSW exchange. @@ -368,17 +421,34 @@ def command( direction_in=direction_in, ) + # The command itself must always get out intact. Only the status + # wrapper may legitimately go missing, and only when the caller is + # expecting the device to drop off the bus — see `tolerate_disconnect`. + self._write_bulk(cbw, "command wrapper", opcode) try: - self._ep_out.write(cbw, self._timeout_ms) payload = b"" if direction_in: payload = bytes(self._ep_in.read(read_length, self._timeout_ms)) elif data_out: - self._ep_out.write(data_out, self._timeout_ms) + self._write_bulk(data_out, "payload", opcode) + except RockusbUsbError: + raise + except Exception as e: + raise RockusbUsbError( + f"rockusb data phase failed (opcode {int(opcode):#04x}): {e}" + ) from e + + try: csw = bytes(self._ep_in.read(CSW_LENGTH, self._timeout_ms)) except Exception as e: + if tolerate_disconnect: + logger.debug( + "no status wrapper for opcode %#04x — device already gone: %s", + int(opcode), e, + ) + return b"" raise RockusbUsbError( - f"rockusb transfer failed (opcode {int(opcode):#04x}): {e}" + f"rockusb status read failed (opcode {int(opcode):#04x}): {e}" ) from e _, residue, status = parse_csw(csw, expected_tag=tag) diff --git a/src/defib/rockusb/recovery.py b/src/defib/rockusb/recovery.py index 0bcde4e..630d375 100644 --- a/src/defib/rockusb/recovery.py +++ b/src/defib/rockusb/recovery.py @@ -24,7 +24,6 @@ from defib.rockusb.device import ( DeviceMode, RockusbDevice, - RockusbUsbError, wait_for_device, ) from defib.rockusb.loader import LoaderBlobs @@ -119,8 +118,10 @@ async def _upload( chunks = build_maskrom_chunks(blob, use_rc4=use_rc4) sent = 0 for chunk in chunks: - await asyncio.to_thread(self._device.control_write, code, chunk) - sent += len(chunk) + # Count what the wire took, not what we handed it. control_write + # already rejects a short transfer; crediting the full chunk here + # anyway would make the progress bar lie about it. + sent += await asyncio.to_thread(self._device.control_write, code, chunk) _emit( on_progress, ProgressEvent(stage, sent, len(blob), f"{name} -> {code:#06x}"), @@ -192,12 +193,16 @@ async def reset(self, subcode: ResetSubcode = ResetSubcode.NORMAL) -> None: ``ResetSubcode.MASKROM`` comes back in MaskROM rather than booting, which is how you chain several flash operations without needing the board's power cut in between. + + A device is entitled to drop off the bus before acknowledging its own + reset, so a missing status wrapper is tolerated — but only that. + Failing to send the command at all, or getting back an explicit + failure, means the board never reset and must not be reported as + though it did. """ - try: - await asyncio.to_thread( - self._device.command, Opcode.RESET_DEVICE, subcode=int(subcode) - ) - except RockusbUsbError as e: - # The device is entitled to drop off the bus before it acknowledges - # its own reset, so a failed status read here is expected. - logger.debug("reset ack not received (device already gone): %s", e) + await asyncio.to_thread( + self._device.command, + Opcode.RESET_DEVICE, + subcode=int(subcode), + tolerate_disconnect=True, + ) diff --git a/tests/test_rockusb_install.py b/tests/test_rockusb_install.py index c76065a..4c4e764 100644 --- a/tests/test_rockusb_install.py +++ b/tests/test_rockusb_install.py @@ -154,6 +154,44 @@ def test_str_mentions_the_path(self): assert "1-4.2" in str(self._dev()) +class TestForeignSocImages: + """The SoC suffix is the only thing distinguishing an image built for this + board from one that would leave it unbootable.""" + + def test_foreign_suffix_refused(self, tmp_path): + files = { + "zboot.img.hi3516ev300": b"\xaa" * 2048, + "rootfs.squashfs.hi3516ev300": b"\xbb" * 4096, + } + with pytest.raises(typer.BadParameter, match="built for 'hi3516ev300'"): + _read_usb_payloads(_make_tarball(tmp_path, files), PARTS, "rv1106") + + def test_error_names_the_expected_chip(self, tmp_path): + files = {"zboot.img.hi3516ev300": b"\xaa" * 16} + with pytest.raises(typer.BadParameter, match="not 'rv1106'"): + _read_usb_payloads(_make_tarball(tmp_path, files), PARTS, "rv1106") + + def test_matching_suffix_accepted(self, tmp_path): + payloads = _read_usb_payloads( + _make_tarball(tmp_path, _complete()), PARTS, "rv1106" + ) + assert len(payloads) == 2 + + def test_suffix_match_is_case_insensitive(self, tmp_path): + payloads = _read_usb_payloads( + _make_tarball(tmp_path, _complete()), PARTS, "RV1106" + ) + assert len(payloads) == 2 + + def test_no_chip_given_skips_the_check(self, tmp_path): + """Callers that genuinely do not know the SoC keep the old behaviour.""" + files = { + "zboot.img.whatever": b"\xaa" * 16, + "rootfs.squashfs.whatever": b"\xbb" * 16, + } + assert len(_read_usb_payloads(_make_tarball(tmp_path, files), PARTS)) == 2 + + class TestResidueHandling: """A device may move less than asked and still report status OK.""" @@ -220,3 +258,187 @@ def test_failure_status_still_wins(self): device = self._device(residue=0, status=1) with pytest.raises(RockusbUsbError, match="failed"): device.command(Opcode.WRITE_LBA, address=0, count=1, data_out=b"\x00" * 512) + + +class TestShortBulkWrites: + """pyusb reports how many bytes it managed; a short count on a payload is + a partial flash write dressed as a successful one.""" + + def _device(self, write_returns): + import struct + + from defib.rockusb.device import RockusbDevice + + found = FoundDevice( + mode=DeviceMode.LOADER, bus=1, address=1, product_id=0x110C, + handle=None, port_numbers=(1,), + ) + device = RockusbDevice(found) + written: list[bytes] = [] + + class Out: + def write(self, data, timeout=None): + written.append(bytes(data)) + return write_returns(bytes(data)) + + class In: + def read(self, size, timeout=None): + tag = struct.unpack_from(" Date: Wed, 19 Aug 2026 17:00:32 +0300 Subject: [PATCH 6/8] rockusb: make it actually work on an RV1106 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Everything here was found by putting a Luckfox Pico Max on the bench. The protocol layer had full green tests and typed clean and still did not move a single byte of flash. Command length. The CDB length field declares 6 for simple commands and 10 for those carrying an address — never the 16 bytes the field occupies on the wire, which is what this sent. A usbplug given 16 ignores the wrapper outright, so the command never lands and the host waits out its timeout with nothing to explain it. Confirmed against xrock, which declares 6 or 10 at every one of its call sites. Mode detection. xrock separates MaskROM from the running usbplug by the low bit of bcdUSB, and this copied that. Measured, both stages report 0x0200, so the test calls a live usbplug MaskROM and the caller waits for a re-enumeration that already happened. What does differ is the string descriptors: the boot ROM ships a bare one, the usbplug names itself RockChip / USB-MSC. Only the descriptor index is read, so this stays cheap. Device selection. A booted Luckfox presents 2207:0019 — an RNDIS+ADB gadget sharing the vendor id, and with bcdUSB 0x0200 it looked like MaskROM too. Matching on vendor id alone meant a healthy running board was a candidate for having a loader uploaded into it. Profiles now declare which product ids mean "waiting to be flashed". Stale input. A recovery tool is routinely pointed at a device some earlier attempt abandoned mid-transaction. The unread status wrapper it left behind gets read as the next command's data phase: 13 bytes into a 5-byte buffer is [Errno 75] Overflow, and every command after it desynchronises. Drain on open, the same way the serial transports open by reading until the line goes quiet. This also makes a wedged device recoverable without power-cycling it. Bulk read sizing. Buffers must be a multiple of the endpoint's max packet size or a full-packet reply overflows them. Residue. Honoured on the LBA path and nowhere else — other opcodes report their transfer length byte-swapped, i.e. "none of it arrived", while the data plainly did. The check stays where a short transfer means a partially written flash. Configuration. Only configure a device that is not configured already; SET_CONFIGURATION resets the data toggles under a running usbplug. Partition table. rootfs is 210M, not the 80M Luckfox's docs give and this shipped with. Read off the board's own U-Boot env and /proc/mtd. Verified on hardware: MaskROM upload, re-enumeration, TEST_UNIT_READY, READ_FLASH_ID ("SNAND"), READ_CAPABILITY, READ_FLASH_INFO, and READ_LBA across env/idblock/uboot/boot — every one byte-identical to a backup taken independently over SSH through mtd. Co-Authored-By: Claude Opus 4.8 --- src/defib/cli/app.py | 17 +- src/defib/profiles/data/rv1106.json | 3 +- src/defib/profiles/schema.py | 10 ++ src/defib/rockusb/device.py | 121 +++++++++++--- src/defib/rockusb/protocol.py | 50 +++++- src/defib/rockusb/recovery.py | 5 +- tests/test_profiles_usb_recovery.py | 14 +- tests/test_rockusb_install.py | 235 ++++++++++++++++++++++++++++ tests/test_rockusb_protocol.py | 31 +++- 9 files changed, 453 insertions(+), 33 deletions(-) diff --git a/src/defib/cli/app.py b/src/defib/cli/app.py index 26ed0e1..b570801 100644 --- a/src/defib/cli/app.py +++ b/src/defib/cli/app.py @@ -3706,6 +3706,7 @@ def _resolve_usb_loader(chip: str, ddr: str, usbplug: str, loader: str) -> Any: async def _open_usb_target( blobs: Any, power_cycle: bool, output: str, wait: float, poe_port_override: str = "", usb_path: str = "", + recovery_ids: Any = None, ) -> Any: """Power-cycle if asked, catch the board, and get its usbplug running. @@ -3729,7 +3730,9 @@ async def _open_usb_target( finally: await controller.close() - found = await wait_for_device(timeout=wait, usb_path=usb_path or None) + found = await wait_for_device( + timeout=wait, usb_path=usb_path or None, recovery_ids=recovery_ids + ) if output == "human": console.print(f" Found {found}") @@ -3746,6 +3749,7 @@ async def _open_usb_target( blobs, on_progress=_usb_progress_printer(output), usb_path=found.usb_path, + recovery_ids=recovery_ids, ) return recovery @@ -3782,10 +3786,12 @@ async def _burn_usb_async( from rich.console import Console + from defib.profiles.loader import load_profile from defib.rockusb.loader import LoaderFormatError from defib.rockusb.protocol import RockusbError console = Console() + recovery_ids = load_profile(chip).usb_recovery_ids # Loader resolution sits inside the handler: an unreadable file or a # malformed container would otherwise escape as a traceback, and in JSON @@ -3793,7 +3799,8 @@ async def _burn_usb_async( try: blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) recovery = await _open_usb_target( - blobs, power_cycle, output, wait, poe_port_override, usb_path + blobs, power_cycle, output, wait, poe_port_override, usb_path, + recovery_ids, ) flash_id = await recovery.read_flash_id() except (RockusbError, LoaderFormatError, OSError) as e: @@ -3987,7 +3994,8 @@ async def _install_usb_async( from defib.rockusb.protocol import SECTOR_SIZE, ResetSubcode, RockusbError console = Console() - partitions = load_profile(chip).partitions + profile = load_profile(chip) + partitions = profile.partitions tar_path = Path(firmware_path) if not tar_path.exists(): @@ -4008,7 +4016,8 @@ async def _install_usb_async( try: blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) recovery = await _open_usb_target( - blobs, power_cycle, output, wait, poe_port_override, usb_path + blobs, power_cycle, output, wait, poe_port_override, usb_path, + profile.usb_recovery_ids, ) flash_id = await recovery.read_flash_id() if output == "human": diff --git a/src/defib/profiles/data/rv1106.json b/src/defib/profiles/data/rv1106.json index 84b22ce..5cc7b32 100644 --- a/src/defib/profiles/data/rv1106.json +++ b/src/defib/profiles/data/rv1106.json @@ -1,6 +1,7 @@ { "name": "rv1106", "RECOVERY": "usb", + "USB_RECOVERY_IDS": [4364], "LOADER_DDR": "rv1106_ddr_924MHz_v1.15.bin", "LOADER_USBPLUG": "rv1106_usbplug_v1.09.bin", "PARTITIONS": { @@ -10,6 +11,6 @@ "boot": {"lba": 2048, "sectors": 8192}, "oem": {"lba": 10240, "sectors": 61440}, "userdata": {"lba": 71680, "sectors": 20480}, - "rootfs": {"lba": 92160, "sectors": 163840} + "rootfs": {"lba": 92160, "sectors": 430080} } } diff --git a/src/defib/profiles/schema.py b/src/defib/profiles/schema.py index 2635305..5240cf6 100644 --- a/src/defib/profiles/schema.py +++ b/src/defib/profiles/schema.py @@ -92,6 +92,16 @@ class SoCProfile(BaseModel): "`spl_data` property." ), ) + usb_recovery_ids: list[int] = Field( + default_factory=list, alias="USB_RECOVERY_IDS", + description=( + "USB recovery only. Product ids the SoC presents while it is " + "actually in a recovery mode. Needed because a running board can " + "expose an unrelated gadget under the same vendor id — a Luckfox " + "booted into Linux shows 2207:0019 (RNDIS+ADB), which must never " + "be mistaken for something waiting to be flashed." + ), + ) loader_ddr: str | None = Field( default=None, alias="LOADER_DDR", description=( diff --git a/src/defib/rockusb/device.py b/src/defib/rockusb/device.py index 28a5ccf..00e7230 100644 --- a/src/defib/rockusb/device.py +++ b/src/defib/rockusb/device.py @@ -15,6 +15,7 @@ import asyncio import logging import secrets +from collections.abc import Sequence from dataclasses import dataclass from enum import Enum from typing import Any @@ -26,6 +27,7 @@ RockusbError, build_cbw, parse_csw, + residue_is_meaningful, ) logger = logging.getLogger(__name__) @@ -98,19 +100,50 @@ def _classify(dev: Any) -> DeviceMode: """MaskROM or loader? Both stages enumerate under the same VID:PID — on RV1106, ``2207:110c`` — - so the product id cannot be used. The boot ROM leaves the low bit of - ``bcdUSB`` clear where the usbplug sets it, which is the same discriminator - xrock relies on. + so the product id cannot separate them. + + xrock uses the low bit of ``bcdUSB`` for this. Measured on an RV1106 that + does not hold: the boot ROM and the running usbplug both report 0x0200, + so that test calls the usbplug MaskROM and the caller waits out a + re-enumeration that already happened. + + What does differ is the string descriptors. The boot ROM ships a bare + descriptor with no strings at all; the usbplug names itself:: + + MaskROM iManufacturer=0 iProduct=0 + usbplug iManufacturer=1 "RockChip" iProduct=2 "USB-MSC" + + Only the index is read here, which lives in the device descriptor — no + string fetch, so this stays cheap and cannot fail on a device that + refuses string reads. """ - return DeviceMode.MASKROM if not (dev.bcdUSB & 0x0001) else DeviceMode.LOADER + named = bool(dev.iProduct) or bool(dev.iManufacturer) + return DeviceMode.LOADER if named else DeviceMode.MASKROM -def find_devices(usb_path: str | None = None) -> list[FoundDevice]: - """Every Rockchip device on the bus, optionally filtered to one port path.""" +def find_devices( + usb_path: str | None = None, + recovery_ids: Sequence[int] | None = None, +) -> list[FoundDevice]: + """Every Rockchip device in a recovery mode, optionally pinned to a port. + + ``recovery_ids`` restricts the search to product ids that mean "waiting to + be flashed". Without it the vendor id alone is far too broad: a Luckfox + booted into Linux presents 2207:0019, an RNDIS+ADB gadget that shares the + vendor id and — because its bcdUSB is 0x0200 — even looks like MaskROM to + the bcdUSB test. Uploading a loader into a healthy running board is not a + mistake worth leaving reachable. + """ usb = _require_usb() out: list[FoundDevice] = [] for dev in usb.core.find(idVendor=ROCKCHIP_VID, find_all=True): + if recovery_ids and dev.idProduct not in recovery_ids: + logger.debug( + "ignoring %04x:%04x — not a recovery product id", + ROCKCHIP_VID, dev.idProduct, + ) + continue found = FoundDevice( mode=_classify(dev), bus=dev.bus, @@ -124,8 +157,11 @@ def find_devices(usb_path: str | None = None) -> list[FoundDevice]: return out -def find_device(usb_path: str | None = None) -> FoundDevice | None: - """A single Rockchip device, or None. +def find_device( + usb_path: str | None = None, + recovery_ids: Sequence[int] | None = None, +) -> FoundDevice | None: + """A single Rockchip device in a recovery mode, or None. Raises: RockusbUsbError: if more than one is present and no ``usb_path`` @@ -133,7 +169,7 @@ def find_device(usb_path: str | None = None) -> FoundDevice | None: whichever board happened to enumerate first, which on a rack is how you write firmware to the wrong device. """ - devices = find_devices(usb_path) + devices = find_devices(usb_path, recovery_ids) if not devices: return None if len(devices) > 1: @@ -150,6 +186,7 @@ async def wait_for_device( mode: DeviceMode | None = None, poll_interval: float = 0.25, usb_path: str | None = None, + recovery_ids: Sequence[int] | None = None, ) -> FoundDevice: """Poll until a matching device appears. @@ -174,7 +211,7 @@ async def wait_for_device( seen: str | None = None while loop.time() < deadline: - found = await asyncio.to_thread(find_device, usb_path) + found = await asyncio.to_thread(find_device, usb_path, recovery_ids) if found is not None: if mode is None or found.mode is mode: return found @@ -230,14 +267,21 @@ def open(self) -> None: usb = _require_usb() dev = self._dev + # Only configure a device that is not configured already. Issuing + # SET_CONFIGURATION to a running usbplug resets the endpoint data + # toggles underneath it, after which the next read comes back + # [Errno 75] Overflow — the device answers out of step with what the + # host asked for. Costly to diagnose, trivial to avoid. try: - dev.set_configuration() - except usb.core.USBError as e: - # Already configured is fine; anything else is not. - if e.errno not in (16, None): # EBUSY - raise RockusbUsbError(f"cannot configure {self._found}: {e}") from e - - cfg = dev.get_active_configuration() + cfg = dev.get_active_configuration() + except usb.core.USBError: + try: + dev.set_configuration() + except usb.core.USBError as e: + raise RockusbUsbError( + f"cannot configure {self._found}: {e}" + ) from e + cfg = dev.get_active_configuration() for intf in cfg: ep_out = usb.util.find_descriptor( intf, @@ -286,6 +330,7 @@ def open(self) -> None: try: usb.util.claim_interface(dev, number) + self._drain_stale_input() except usb.core.USBError as e: # Give the kernel driver back before bailing out. Leaving it # detached would strand the interface for whatever owned it, @@ -358,6 +403,44 @@ def control_write(self, code: int, payload: bytes) -> int: # -- rockusb bulk stage ----------------------------------------------- + def _drain_stale_input(self) -> None: + """Discard anything the device still has queued from a previous run. + + A recovery tool is routinely pointed at a device some earlier attempt + abandoned mid-transaction, leaving an unread status wrapper in the + pipe. The next command then reads that stale wrapper as its data + phase — a 13-byte CSW landing in a 5-byte buffer surfaces as + ``[Errno 75] Overflow`` and every command after it desynchronises. + + The serial transports already open by reading until the line goes + quiet; this is the same idea one layer down. + """ + if self._ep_in is None: + return + mps = getattr(self._ep_in, "wMaxPacketSize", 512) or 512 + for _ in range(8): + try: + stale = self._ep_in.read(mps, 50) + except Exception: + return # nothing waiting: a timeout here is the good outcome + if not stale: + return + logger.debug("discarded %d stale bytes from a previous session", len(stale)) + + def _read_bulk(self, length: int) -> bytes: + """Read ``length`` bytes from the bulk IN endpoint. + + The request is rounded up to the endpoint's max packet size and the + result trimmed. A bulk IN transfer whose buffer is not a multiple of + that size fails with ``[Errno 75] Overflow`` the moment the device + answers with a full packet — which the usbplug does for short replies + like READ_FLASH_ID's five bytes. + """ + mps = getattr(self._ep_in, "wMaxPacketSize", 512) or 512 + rounded = ((length + mps - 1) // mps) * mps + data = bytes(self._ep_in.read(rounded, self._timeout_ms)) + return data[:length] + def _write_bulk(self, data: bytes, what: str, opcode: Opcode | int) -> None: """Push ``data`` out the bulk endpoint, insisting all of it lands. @@ -428,7 +511,7 @@ def command( try: payload = b"" if direction_in: - payload = bytes(self._ep_in.read(read_length, self._timeout_ms)) + payload = self._read_bulk(read_length) elif data_out: self._write_bulk(data_out, "payload", opcode) except RockusbUsbError: @@ -457,7 +540,7 @@ def command( f"rockusb command {int(opcode):#04x} failed " f"(status {status}, residue {residue})" ) - if residue: + if residue and residue_is_meaningful(opcode): raise RockusbUsbError( f"rockusb command {int(opcode):#04x} moved " f"{transfer_length - residue} of {transfer_length} bytes " diff --git a/src/defib/rockusb/protocol.py b/src/defib/rockusb/protocol.py index 57fdd01..ea434d5 100644 --- a/src/defib/rockusb/protocol.py +++ b/src/defib/rockusb/protocol.py @@ -49,6 +49,54 @@ class Opcode(IntEnum): RESET_DEVICE = 0xFF +#: Declared CDB length, which is *not* the 16 bytes the field occupies on the +#: wire. Commands carrying an address declare 10; the rest declare 6. Sending +#: 16 makes the usbplug ignore the wrapper outright — the command never lands +#: and the host waits out its timeout with no error to explain it. +CDB_LENGTH_SHORT = 6 +CDB_LENGTH_ADDRESSED = 10 + +_ADDRESSED_OPCODES = frozenset({ + Opcode.READ_LBA, + Opcode.WRITE_LBA, + Opcode.ERASE_LBA, +}) + + +def cdb_length(opcode: Opcode | int) -> int: + """How long this opcode's command block claims to be.""" + return ( + CDB_LENGTH_ADDRESSED + if opcode in _ADDRESSED_OPCODES + else CDB_LENGTH_SHORT + ) + + +def residue_is_meaningful(opcode: Opcode | int) -> bool: + """Whether this opcode's status wrapper reports a usable residue. + + Mass Storage says residue is a little-endian count of bytes *not* + transferred, and on the LBA path an RV1106 usbplug honours that — a + full-sector read comes back with residue 0. + + Everything else reports nonsense. Measured:: + + TEST_UNIT_READY transfer 0 residue 0x06000000 + READ_FLASH_ID transfer 5 residue 0x05000000 + READ_CAPABILITY transfer 8 residue 0x08000000 + READ_FLASH_INFO transfer 11 residue 0x0B000000 + READ_LBA transfer 512 residue 0 + + Those are the transfer lengths written big-endian, i.e. "none of it + arrived" — while the data plainly did arrive and is correct. xrock never + looks at residue at all, which is presumably why nobody noticed. + + So the check is kept exactly where it earns its keep: the block path, the + one place a short transfer means a partially written flash. + """ + return opcode in _ADDRESSED_OPCODES + + class ResetSubcode(IntEnum): """Sub-selector for :attr:`Opcode.RESET_DEVICE`.""" @@ -114,7 +162,7 @@ def build_cbw( [ DIRECTION_IN if direction_in else DIRECTION_OUT, 0x00, # LUN - len(cdb), + cdb_length(opcode), ] ) + cdb diff --git a/src/defib/rockusb/recovery.py b/src/defib/rockusb/recovery.py index 630d375..3d616fc 100644 --- a/src/defib/rockusb/recovery.py +++ b/src/defib/rockusb/recovery.py @@ -18,6 +18,7 @@ import asyncio import logging +from collections.abc import Sequence from typing import Callable from defib.recovery.events import ProgressEvent, Stage @@ -61,6 +62,7 @@ async def download_boot( on_progress: Callable[[ProgressEvent], None] | None = None, reenumerate_timeout: float = 15.0, usb_path: str | None = None, + recovery_ids: Sequence[int] | None = None, ) -> RockusbDevice: """Upload DDR init then usbplug, and wait for the device to come back. @@ -95,7 +97,8 @@ async def download_boot( await asyncio.sleep(USBPLUG_SETTLE) found = await wait_for_device( - timeout=reenumerate_timeout, mode=DeviceMode.LOADER, usb_path=usb_path + timeout=reenumerate_timeout, mode=DeviceMode.LOADER, + usb_path=usb_path, recovery_ids=recovery_ids, ) device = RockusbDevice(found) device.open() diff --git a/tests/test_profiles_usb_recovery.py b/tests/test_profiles_usb_recovery.py index 03caf95..f3330bd 100644 --- a/tests/test_profiles_usb_recovery.py +++ b/tests/test_profiles_usb_recovery.py @@ -76,8 +76,16 @@ def test_declares_its_loader_blobs(self): assert profile.loader_usbplug == "rv1106_usbplug_v1.09.bin" def test_partition_lbas_match_the_vendor_byte_layout(self): - """Luckfox SPI NAND: 256K(env) 256K@256K(idblock) 512K(uboot) 4M(boot) - 30M(oem) 10M(userdata) 80M(rootfs), converted to 512-byte sectors.""" + """Read off a real Luckfox Pico Max, not the docs. + + Its U-Boot env and /proc/mtd both give:: + + spi-nand0:256K(env),256K@256K(idblock),512K(uboot),4M(boot), + 30M(oem),10M(userdata),210M(rootfs) + + Note rootfs is 210M. Luckfox's published layout says 80M, which is + what this profile shipped with until hardware contradicted it. + """ partitions = load_profile("rv1106", PROFILES_DIR).partitions K, M = 1024, 1024 * 1024 expected = { @@ -87,7 +95,7 @@ def test_partition_lbas_match_the_vendor_byte_layout(self): "boot": (1 * M, 4 * M), "oem": (5 * M, 30 * M), "userdata": (35 * M, 10 * M), - "rootfs": (45 * M, 80 * M), + "rootfs": (45 * M, 210 * M), } actual = { name: (p.lba * 512, p.size_bytes) for name, p in partitions.items() diff --git a/tests/test_rockusb_install.py b/tests/test_rockusb_install.py index 4c4e764..802e23c 100644 --- a/tests/test_rockusb_install.py +++ b/tests/test_rockusb_install.py @@ -192,6 +192,241 @@ def test_no_chip_given_skips_the_check(self, tmp_path): assert len(_read_usb_payloads(_make_tarball(tmp_path, files), PARTS)) == 2 +class TestRecoveryIdFilter: + """A running Luckfox presents 2207:0019 — an RNDIS+ADB gadget sharing the + vendor id, whose bcdUSB of 0x0200 makes it look like MaskROM to the + bcdUSB test. Found on real hardware: without a product-id filter, defib + would have uploaded a loader into a healthy booted board. + """ + + class _FakeUsbDevice: + def __init__(self, pid, bcd=0x0200, strings=False): + self.idProduct = pid + self.bcdUSB = bcd + # MaskROM ships no string descriptors; anything past it names + # itself. Measured on an RV1106: bcdUSB is 0x0200 for both, so + # these indices are the only thing separating the two stages. + self.iManufacturer = 1 if strings else 0 + self.iProduct = 2 if strings else 0 + self.bus = 7 + self.address = 3 + self.port_numbers = (1,) + + def _patch(self, monkeypatch, devices): + import defib.rockusb.device as mod + + class FakeCore: + @staticmethod + def find(**kwargs): + return list(devices) + + monkeypatch.setattr(mod, "_require_usb", lambda: type("U", (), {"core": FakeCore})) + + def test_runtime_adb_gadget_ignored(self, monkeypatch): + from defib.rockusb.device import find_devices + + self._patch(monkeypatch, [self._FakeUsbDevice(0x0019)]) + assert find_devices(recovery_ids=[0x110C]) == [] + + def test_recovery_device_still_found(self, monkeypatch): + from defib.rockusb.device import find_devices + + self._patch(monkeypatch, [self._FakeUsbDevice(0x110C)]) + found = find_devices(recovery_ids=[0x110C]) + assert len(found) == 1 + assert found[0].product_id == 0x110C + + def test_recovery_device_picked_out_of_a_mixed_bus(self, monkeypatch): + from defib.rockusb.device import find_devices + + self._patch( + monkeypatch, + [self._FakeUsbDevice(0x0019), self._FakeUsbDevice(0x110C)], + ) + found = find_devices(recovery_ids=[0x110C]) + assert [d.product_id for d in found] == [0x110C] + + def test_no_ids_means_no_filter(self, monkeypatch): + """Back-compat: callers that pass nothing keep the old broad search.""" + from defib.rockusb.device import find_devices + + self._patch(monkeypatch, [self._FakeUsbDevice(0x0019)]) + assert len(find_devices()) == 1 + + def test_rv1106_profile_declares_the_recovery_id(self): + from defib.profiles.loader import load_profile + + assert load_profile("rv1106").usb_recovery_ids == [0x110C] + + +class TestModeClassification: + """Measured on an RV1106: both stages report bcdUSB 0x0200, so xrock's + low-bit test calls a running usbplug MaskROM. Only the string descriptors + tell them apart. + """ + + def _dev(self, *, strings): + return TestRecoveryIdFilter._FakeUsbDevice(0x110C, strings=strings) + + def test_bare_descriptor_is_maskrom(self): + from defib.rockusb.device import DeviceMode, _classify + + assert _classify(self._dev(strings=False)) is DeviceMode.MASKROM + + def test_named_device_is_loader(self): + from defib.rockusb.device import DeviceMode, _classify + + assert _classify(self._dev(strings=True)) is DeviceMode.LOADER + + def test_bcdusb_is_not_consulted(self): + """Both real stages report 0x0200; keying on it regresses the bug.""" + from defib.rockusb.device import DeviceMode, _classify + + maskrom = TestRecoveryIdFilter._FakeUsbDevice(0x110C, bcd=0x0200, strings=False) + loader = TestRecoveryIdFilter._FakeUsbDevice(0x110C, bcd=0x0200, strings=True) + assert _classify(maskrom) is DeviceMode.MASKROM + assert _classify(loader) is DeviceMode.LOADER + + def test_manufacturer_alone_is_enough(self): + from defib.rockusb.device import DeviceMode, _classify + + dev = TestRecoveryIdFilter._FakeUsbDevice(0x110C, strings=False) + dev.iManufacturer = 1 + assert _classify(dev) is DeviceMode.LOADER + + +class TestResidueScope: + """Measured on an RV1106 usbplug: residue is only honoured on the LBA + path. Everything else reports its transfer length written big-endian — + i.e. "none of it arrived" — while the data plainly did arrive. + """ + + def test_lba_opcodes_are_checked(self): + from defib.rockusb.protocol import Opcode, residue_is_meaningful + + for op in (Opcode.READ_LBA, Opcode.WRITE_LBA, Opcode.ERASE_LBA): + assert residue_is_meaningful(op) + + def test_other_opcodes_are_not(self): + from defib.rockusb.protocol import Opcode, residue_is_meaningful + + for op in ( + Opcode.TEST_UNIT_READY, Opcode.READ_FLASH_ID, + Opcode.READ_CAPABILITY, Opcode.READ_CHIP_INFO, + Opcode.READ_FLASH_INFO, Opcode.RESET_DEVICE, + ): + assert not residue_is_meaningful(op) + + def test_observed_garbage_residues_would_be_ignored(self): + """The literal values seen on the bench, each the transfer length + byte-swapped. Enforcing residue here would break every probe.""" + from defib.rockusb.protocol import Opcode, residue_is_meaningful + + observed = { + Opcode.TEST_UNIT_READY: 0x06000000, + Opcode.READ_FLASH_ID: 0x05000000, + Opcode.READ_CAPABILITY: 0x08000000, + Opcode.READ_FLASH_INFO: 0x0B000000, + } + for op, residue in observed.items(): + assert residue != 0 and not residue_is_meaningful(op) + + +class TestStaleInputDrain: + """A recovery tool is routinely pointed at a device an earlier attempt + abandoned mid-transaction. The unread status wrapper it left behind gets + read as the next command's data phase — 13 bytes into a 5-byte buffer is + [Errno 75] Overflow, and everything after it desynchronises. + """ + + class _Ep: + wMaxPacketSize = 512 + + def __init__(self, queued): + self.queued = list(queued) + self.reads = 0 + + def read(self, size, timeout=None): + self.reads += 1 + if not self.queued: + raise TimeoutError("nothing waiting") + return self.queued.pop(0) + + def _device(self, queued): + from defib.rockusb.device import RockusbDevice + + found = FoundDevice( + mode=DeviceMode.LOADER, bus=1, address=1, product_id=0x110C, + handle=None, port_numbers=(1,), + ) + d = RockusbDevice(found) + d._ep_in = self._Ep(queued) + return d + + def test_stale_bytes_are_discarded(self): + d = self._device([b"USBS" + b"\x00" * 9]) + d._drain_stale_input() + assert d._ep_in.queued == [] + + def test_drain_stops_at_the_first_timeout(self): + d = self._device([]) + d._drain_stale_input() + assert d._ep_in.reads == 1 + + def test_drain_is_bounded(self): + """A device stuck streaming must not hang the open.""" + d = self._device([b"x" * 512] * 100) + d._drain_stale_input() + assert d._ep_in.reads <= 8 + + def test_no_endpoint_is_harmless(self): + """MaskROM is reached before endpoints are known.""" + d = self._device([]) + d._ep_in = None + d._drain_stale_input() + + +class TestBulkReadRounding: + """Bulk IN buffers must be a multiple of the endpoint's max packet size.""" + + class _Ep: + wMaxPacketSize = 512 + + def __init__(self): + self.requested = None + + def read(self, size, timeout=None): + self.requested = size + return b"\x5a" * size + + def _device(self): + from defib.rockusb.device import RockusbDevice + + found = FoundDevice( + mode=DeviceMode.LOADER, bus=1, address=1, product_id=0x110C, + handle=None, port_numbers=(1,), + ) + d = RockusbDevice(found) + d._ep_in = self._Ep() + return d + + def test_short_read_is_rounded_up(self): + d = self._device() + out = d._read_bulk(5) + assert d._ep_in.requested == 512 + assert len(out) == 5 + + def test_exact_multiple_is_unchanged(self): + d = self._device() + d._read_bulk(1024) + assert d._ep_in.requested == 1024 + + def test_partial_sector_rounds_to_next_packet(self): + d = self._device() + d._read_bulk(513) + assert d._ep_in.requested == 1024 + + class TestResidueHandling: """A device may move less than asked and still report status OK.""" diff --git a/tests/test_rockusb_protocol.py b/tests/test_rockusb_protocol.py index a22ed5f..00cc939 100644 --- a/tests/test_rockusb_protocol.py +++ b/tests/test_rockusb_protocol.py @@ -55,10 +55,33 @@ def test_direction_flag(self): assert build_cbw(1, Opcode.READ_LBA, count=1, direction_in=True)[12] == DIRECTION_IN assert build_cbw(1, Opcode.WRITE_LBA, count=1)[12] == DIRECTION_OUT - def test_lun_and_cdb_length(self): - cbw = build_cbw(1, Opcode.TEST_UNIT_READY) - assert cbw[13] == 0 # LUN - assert cbw[14] == 16 # CDB length + def test_lun(self): + assert build_cbw(1, Opcode.TEST_UNIT_READY)[13] == 0 + + @pytest.mark.parametrize( + "opcode,expected", + [ + (Opcode.TEST_UNIT_READY, 6), + (Opcode.READ_FLASH_ID, 6), + (Opcode.READ_CAPABILITY, 6), + (Opcode.READ_CHIP_INFO, 6), + (Opcode.READ_FLASH_INFO, 6), + (Opcode.RESET_DEVICE, 6), + (Opcode.READ_LBA, 10), + (Opcode.WRITE_LBA, 10), + (Opcode.ERASE_LBA, 10), + ], + ) + def test_declared_cdb_length(self, opcode, expected): + """Commands carrying an address declare 10, the rest 6 — never the 16 + bytes the field actually occupies. Measured against a real usbplug: + declaring 16 makes it ignore the wrapper and the host just times out. + """ + assert build_cbw(1, opcode, count=1)[14] == expected + + def test_cdb_block_is_still_16_bytes_on_the_wire(self): + """The declared length shrinks; the wrapper does not.""" + assert len(build_cbw(1, Opcode.TEST_UNIT_READY)) == CBW_LENGTH def test_opcode_and_subcode_placement(self): cbw = build_cbw(1, Opcode.RESET_DEVICE, subcode=int(ResetSubcode.MASKROM)) From 854230adff406527c24b226cfa5f53ba2a46e99b Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Wed, 19 Aug 2026 17:44:48 +0300 Subject: [PATCH 7/8] cli: hold back USB install until it can produce a booting board MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The read path is proven on hardware; writing flash is not, and on the one board this was developed against `install` could not succeed even if every byte landed correctly. That board's rootfs is UBI — mtd6, with `ubi.mtd=6 root=ubi0:rootfs rootfstype=ubifs`. The nand tarball ships rootfs.ubi, which the image mapper refuses by design. The nor tarball ships rootfs.squashfs, which would be written raw into mtd6, destroying the UBI while the U-Boot environment still asks for it. Making that work means rewriting sys_bootargs for a squashfs root while preserving rk_dma_heap_cma=66M, without which the media stack has no buffers at all. That is a design question, not an oversight, and it deserves its own change. Meanwhile WRITE_LBA, the multi-transfer split, and residue-on-write have never run against silicon. The read path had seven bugs that only hardware found, in code that was fully green and typed clean; there is no reason to believe the write path is better off, and the failure mode there is somebody's flash rather than a timeout. So `install -c ` now says so and exits, instead of offering options that look finished. `burn` is unaffected: it wakes a board over USB and is verified end to end on an RV1106. The image-mapping helpers go with it, along with their tests. The partition table stays — it was read off real hardware, it is what the follow-up will need, and it documents the chip either way. Co-Authored-By: Claude Opus 4.8 --- src/defib/cli/app.py | 270 +----------------- src/defib/rockusb/device.py | 24 ++ tests/test_profiles_usb_recovery.py | 84 ------ ...kusb_install.py => test_rockusb_device.py} | 159 +---------- 4 files changed, 41 insertions(+), 496 deletions(-) rename tests/{test_rockusb_install.py => test_rockusb_device.py} (74%) diff --git a/src/defib/cli/app.py b/src/defib/cli/app.py index b570801..15e9e05 100644 --- a/src/defib/cli/app.py +++ b/src/defib/cli/app.py @@ -2113,27 +2113,17 @@ def install( ), output: str = typer.Option("human", "--output", help="Output mode: human, json"), debug: bool = typer.Option(False, "-d", "--debug", help="Enable debug logging"), - ddr: str = typer.Option("", "--ddr", help="USB-recovery chips: DDR-init blob (rkbin rv1106_ddr_*.bin)"), - usbplug: str = typer.Option("", "--usbplug", help="USB-recovery chips: usbplug blob (rkbin rv1106_usbplug_*.bin)"), - loader: str = typer.Option("", "--loader", help="USB-recovery chips: RKBOOT container (MiniLoaderAll.bin), instead of --ddr/--usbplug"), - wait: float = typer.Option(30.0, "--wait", help="USB-recovery chips: seconds to wait for the board to enumerate"), - verify: bool = typer.Option(False, "--verify", help="USB-recovery chips: read each image back and compare"), - usb_path: str = typer.Option("", "--usb-path", help="USB-recovery chips: pin to one physical port path (e.g. 1-4.2); required when several Rockchip boards are attached"), ) -> None: - """Install a full OpenIPC firmware (U-Boot + kernel + rootfs). - - For UART chips: extracts the tarball, burns U-Boot to RAM via the boot - ROM, then TFTPs kernel and rootfs to U-Boot, which flashes them. + """Install a full OpenIPC firmware (U-Boot + kernel + rootfs) via UART + TFTP. - For chips whose boot ROM only answers on USB (Rockchip), -p and the TFTP - options do not apply — pass --ddr/--usbplug (or --loader) and the images - are written straight to flash over USB. + Extracts the firmware tarball, burns U-Boot to RAM via boot ROM, + then uses TFTP to transfer kernel and rootfs to U-Boot which + flashes them to NOR or NAND. """ import asyncio asyncio.run(_install_async( chip, firmware, port, power_cycle, poe_port_override, nic, host_ip, device_ip, tftp_port, nor_size, nand, wipe_env, tftp_via, output, debug, - ddr, usbplug, loader, wait, verify, usb_path, )) @@ -2210,12 +2200,6 @@ async def _install_async( tftp_via: str, output: str, debug: bool, - ddr: str = "", - usbplug: str = "", - loader: str = "", - wait: float = 30.0, - verify: bool = False, - usb_path: str = "", ) -> None: import hashlib import json as json_mod @@ -2249,11 +2233,15 @@ async def _install_async( logging.basicConfig(level=logging.INFO) if recovery_mode(chip) == "usb": - await _install_usb_async( - chip, firmware_path, ddr, usbplug, loader, - power_cycle, output, wait, verify, poe_port_override, usb_path, + # Writing flash over USB is not landed yet: this chip's images have + # nowhere correct to go until the UBI-vs-raw rootfs question is + # settled, and the write path has never run against hardware. Wake + # the board with `burn` and flash it by other means meanwhile. + console.print( + f"[red]{chip} recovers over USB, and `install` does not support " + "that yet — use `defib burn` to bring the board up.[/red]" ) - return + raise typer.Exit(1) if nand: layout = _NAND_LAYOUT @@ -3819,239 +3807,5 @@ async def _burn_usb_async( ) -# Which partition each OpenIPC image belongs in, for USB-recovery chips. -# Keyed on the filename stem the tarball uses before the ``.`` suffix. -_USB_IMAGE_PARTITIONS = { - "zboot.img": "boot", - "rootfs.squashfs": "rootfs", - "uboot.img": "uboot", - "idblock.img": "idblock", -} - -# A firmware install is only complete with both of these. Writing a rootfs -# without the kernel beside it leaves an unbootable board that this command -# would otherwise call a success. -_USB_REQUIRED_PARTITIONS = ("boot", "rootfs") - -# Written last, whatever order the tarball lists things in. The boot ROM falls -# into MaskROM precisely because it finds no valid IDB, so committing the -# idblock before everything else is on flash trades the recovery path away: a -# failure after that point leaves a board that boots into a broken image -# instead of one that can be re-flashed over USB. -_USB_WRITE_LAST = ("idblock",) - - -def _map_usb_images( - names: list[str], partitions: dict[str, Any], chip: str = "", -) -> list[tuple[str, str, Any]]: - """Match tarball members to partitions, in a safe write order. - - Returns ``(member, partition, extent)`` triples. - - Args: - names: tarball member names, e.g. ``zboot.img.rv1106``. - partitions: the chip profile's partition table. - chip: when given, the SoC suffix every member must carry. OpenIPC - names each image for the SoC it was built for, and that suffix is - the only thing separating an RV1106 kernel from one that would - leave this board unbootable. - - Raises: - typer.BadParameter: if a member cannot be placed, or was built for a - different SoC. The UBI case is called out by name because it is - not a mapping gap but a real layout question: ``rootfs.ubi`` - bundles kernel *and* rootfs as UBI volumes, so it does not - correspond to any single partition in the vendor table. - """ - mapped: list[tuple[str, str, Any]] = [] - for name in names: - stem, _, suffix = name.rpartition(".") - if not stem: - stem, suffix = name, "" - partition = _USB_IMAGE_PARTITIONS.get(stem) - if partition is None: - if stem.startswith("rootfs.ubi"): - raise typer.BadParameter( - f"{name} is a UBI image holding both kernel and rootfs " - "volumes, so it has no single partition to go in. Use the " - "nor-style tarball (zboot.img + rootfs.squashfs), or " - "decide the UBI region for this board first." - ) - continue - if chip and suffix.lower() != chip.lower(): - raise typer.BadParameter( - f"{name} was built for '{suffix}', not '{chip}' — flashing " - "another SoC's image would leave this board unbootable" - ) - if partition not in partitions: - raise typer.BadParameter( - f"{name} belongs in partition '{partition}', which this " - f"chip's profile does not declare. Known: " - f"{', '.join(sorted(partitions)) or '(none)'}" - ) - mapped.append((name, partition, partitions[partition])) - - mapped.sort(key=lambda item: item[1] in _USB_WRITE_LAST) - return mapped - - -def _check_usb_image_fits(name: str, partition: str, extent: Any, size: int) -> None: - """Reject an image too big for the partition it is bound for. - - Raises: - typer.BadParameter: if it would spill past the end. The usbplug takes - a start sector and a count and writes what it is told, so an - oversized image quietly runs on into whatever follows. - """ - if size > extent.size_bytes: - raise typer.BadParameter( - f"{name} is {size} bytes but partition '{partition}' holds " - f"{extent.size_bytes} — it would overwrite whatever follows" - ) - - -def _read_usb_payloads( - tar_path: Any, partitions: dict[str, Any], chip: str = "", -) -> list[tuple[str, str, Any, bytes]]: - """Extract, checksum and bounds-check every image the tarball places. - - Returns ``(member, partition, extent, data)`` in write order. - """ - import hashlib - import tarfile - - with tarfile.open(tar_path) as tar: - members = [m for m in tar.getmembers() if m.isfile()] - names = [m.name for m in members if not m.name.endswith(".md5sum")] - targets = _map_usb_images(names, partitions, chip) - if not targets: - raise typer.BadParameter( - f"nothing in {tar_path.name} maps to a partition " - f"(saw: {', '.join(names) or 'no files'})" - ) - - placed = {partition for _, partition, _ in targets} - missing = [p for p in _USB_REQUIRED_PARTITIONS if p not in placed] - if missing: - raise typer.BadParameter( - f"{tar_path.name} has no image for {', '.join(missing)} — " - "a firmware install needs both a kernel and a rootfs, and " - "writing one without the other leaves an unbootable board" - ) - - def _member(name: str) -> bytes: - handle = tar.extractfile(name) - if handle is None: - raise typer.BadParameter(f"cannot read {name} from {tar_path.name}") - return handle.read() - - # The tarball ships a .md5sum beside each image. --verify only proves - # flash matches what we sent, so without this a corrupted download - # would be written and confirmed against its own corruption. - expected: dict[str, str] = {} - for member in members: - if not member.name.endswith(".md5sum"): - continue - line = _member(member.name).decode(errors="replace").strip() - if line: - expected[member.name.removesuffix(".md5sum")] = line.split()[0] - - payloads: list[tuple[str, str, Any, bytes]] = [] - for name, partition, extent in targets: - data = _member(name) - digest = expected.get(name) - if digest is not None: - actual = hashlib.md5(data).hexdigest() - if actual != digest: - raise typer.BadParameter( - f"MD5 mismatch for {name}: expected {digest}, got {actual}" - ) - _check_usb_image_fits(name, partition, extent, len(data)) - payloads.append((name, partition, extent, data)) - - return payloads - - -async def _install_usb_async( - chip: str, firmware_path: str, ddr: str, usbplug: str, loader: str, - power_cycle: bool, output: str, wait: float, verify: bool, - poe_port_override: str = "", usb_path: str = "", -) -> None: - """``install`` for chips whose boot ROM only answers on USB. - - The usbplug runs Rockchip's FTL, so flash is a flat array of 512-byte - sectors here — bad blocks and ECC are handled device-side and there is no - TFTP or U-Boot console in the path at all. - """ - import json as json_mod - import tarfile - from pathlib import Path - - from rich.console import Console - - from defib.profiles.loader import load_profile - from defib.rockusb.loader import LoaderFormatError - from defib.rockusb.protocol import SECTOR_SIZE, ResetSubcode, RockusbError - - console = Console() - profile = load_profile(chip) - partitions = profile.partitions - - tar_path = Path(firmware_path) - if not tar_path.exists(): - raise typer.BadParameter(f"firmware not found: {firmware_path}") - - # In JSON mode every failure has to arrive as a structured event, so - # archive and validation errors are funnelled through _usb_fail rather - # than surfacing as Typer's own text (or, for a corrupt archive, a - # traceback). Human mode keeps Typer's nicer rendering. - try: - payloads = _read_usb_payloads(tar_path, partitions, chip) - except (typer.BadParameter, tarfile.TarError, OSError) as e: - if output != "json": - raise - _usb_fail(output, str(e)) - return - - try: - blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) - recovery = await _open_usb_target( - blobs, power_cycle, output, wait, poe_port_override, usb_path, - profile.usb_recovery_ids, - ) - flash_id = await recovery.read_flash_id() - if output == "human": - console.print(f" Flash ID: {flash_id.hex()}") - - for name, partition, extent, data in payloads: - if output == "human": - console.print( - f" Writing {name} -> {partition} " - f"(LBA {extent.lba}, {len(data)} bytes)..." - ) - await recovery.write_image( - extent.lba, data, on_progress=_usb_progress_printer(output) - ) - if verify: - sectors = (len(data) + SECTOR_SIZE - 1) // SECTOR_SIZE - read_back = await recovery.read_image(extent.lba, sectors) - if read_back[: len(data)] != data: - _usb_fail(output, f"verify failed for {name} at LBA {extent.lba}") - if output == "human": - console.print(" Verified") - - await recovery.reset(ResetSubcode.NORMAL) - except (RockusbError, LoaderFormatError, OSError) as e: - _usb_fail(output, str(e)) - return - - if output == "json": - print(json_mod.dumps({ - "event": "done", "success": True, "images": len(payloads), - })) - else: - console.print("\n[green bold]Install complete![/green bold] Device is rebooting.") - - def main() -> None: app() diff --git a/src/defib/rockusb/device.py b/src/defib/rockusb/device.py index 00e7230..df1a3e3 100644 --- a/src/defib/rockusb/device.py +++ b/src/defib/rockusb/device.py @@ -330,6 +330,7 @@ def open(self) -> None: try: usb.util.claim_interface(dev, number) + self._clear_stalls() self._drain_stale_input() except usb.core.USBError as e: # Give the kernel driver back before bailing out. Leaving it @@ -403,6 +404,29 @@ def control_write(self, code: int, payload: bytes) -> int: # -- rockusb bulk stage ----------------------------------------------- + def _clear_stalls(self) -> None: + """Clear a halt condition left on either endpoint. + + The other half of inheriting a device from an abandoned attempt: a + transfer that failed part-way can leave an endpoint halted, and every + subsequent transfer on it then times out — including the very first + command wrapper, which makes the device look dead rather than merely + out of step. + + Best-effort. A device that has genuinely gone away will fail here and + that is fine; the caller finds out on the next real transfer. + """ + for ep in (self._ep_out, self._ep_in): + if ep is None: + continue + try: + ep.clear_halt() + except Exception: + logger.debug( + "could not clear halt on endpoint %#04x", + ep.bEndpointAddress, exc_info=True, + ) + def _drain_stale_input(self) -> None: """Discard anything the device still has queued from a previous run. diff --git a/tests/test_profiles_usb_recovery.py b/tests/test_profiles_usb_recovery.py index f3330bd..8140d4d 100644 --- a/tests/test_profiles_usb_recovery.py +++ b/tests/test_profiles_usb_recovery.py @@ -12,7 +12,6 @@ import pytest from pydantic import ValidationError -from defib.cli.app import _check_usb_image_fits, _map_usb_images from defib.profiles.loader import load_profile, recovery_mode from defib.profiles.schema import FlashPartition, SoCProfile @@ -123,89 +122,6 @@ def test_profile_json_is_minimal(self): assert not {"DDRSTEP0", "PRESTEP0", "ADDRESS", "FILELEN"} & set(data) -class TestMapUsbImages: - PARTS = { - "idblock": FlashPartition(lba=512, sectors=512), - "uboot": FlashPartition(lba=1024, sectors=1024), - "boot": FlashPartition(lba=2048, sectors=8192), - "rootfs": FlashPartition(lba=92160, sectors=163840), - } - - def test_maps_nor_style_tarball(self): - out = _map_usb_images( - ["zboot.img.rv1106", "rootfs.squashfs.rv1106"], self.PARTS - ) - assert [(name, part, ext.lba) for name, part, ext in out] == [ - ("zboot.img.rv1106", "boot", 2048), - ("rootfs.squashfs.rv1106", "rootfs", 92160), - ] - - def test_idblock_is_written_last(self): - """The boot ROM enters MaskROM precisely because it finds no valid - IDB. Committing one before the rest of flash is populated trades that - recovery path away if a later write fails.""" - out = _map_usb_images( - ["idblock.img.rv1106", "zboot.img.rv1106", "rootfs.squashfs.rv1106"], - self.PARTS, - ) - assert [part for _, part, _ in out][-1] == "idblock" - - def test_order_is_otherwise_preserved(self): - out = _map_usb_images( - ["rootfs.squashfs.rv1106", "zboot.img.rv1106"], self.PARTS - ) - assert [part for _, part, _ in out] == ["rootfs", "boot"] - - def test_ubi_image_refused_with_the_reason(self): - """Not a mapping gap — a UBI bundles kernel and rootfs volumes, so no - single partition is the right answer.""" - import typer - - with pytest.raises(typer.BadParameter, match="kernel and rootfs"): - _map_usb_images(["rootfs.ubi.rv1106"], self.PARTS) - - def test_undeclared_partition_is_an_error_not_a_silent_skip(self): - import typer - - with pytest.raises(typer.BadParameter, match="does not declare"): - _map_usb_images( - ["zboot.img.rv1106"], - {"rootfs": FlashPartition(lba=1, sectors=1)}, - ) - - def test_unrecognised_files_are_ignored(self): - assert _map_usb_images(["README", "notes.txt"], self.PARTS) == [] - - def test_checksums_are_not_mistaken_for_images(self): - out = _map_usb_images(["zboot.img.rv1106.md5sum"], self.PARTS) - assert out == [] - - -class TestImageFits: - BOOT = FlashPartition(lba=2048, sectors=8192) # 4 MB - - def test_image_within_bounds_passes(self): - _check_usb_image_fits("zboot.img", "boot", self.BOOT, self.BOOT.size_bytes) - - def test_oversized_image_refused(self): - """The usbplug writes what it is told from a start sector, so an - oversized image runs straight on into the next partition.""" - import typer - - with pytest.raises(typer.BadParameter, match="would overwrite"): - _check_usb_image_fits( - "zboot.img", "boot", self.BOOT, self.BOOT.size_bytes + 1 - ) - - def test_error_names_both_sizes(self): - import typer - - with pytest.raises(typer.BadParameter) as excinfo: - _check_usb_image_fits("zboot.img", "boot", self.BOOT, 9_000_000) - assert "9000000" in str(excinfo.value) - assert str(self.BOOT.size_bytes) in str(excinfo.value) - - class TestFlashPartition: def test_end_lba(self): assert FlashPartition(lba=100, sectors=50).end_lba == 150 diff --git a/tests/test_rockusb_install.py b/tests/test_rockusb_device.py similarity index 74% rename from tests/test_rockusb_install.py rename to tests/test_rockusb_device.py index 802e23c..100662a 100644 --- a/tests/test_rockusb_install.py +++ b/tests/test_rockusb_device.py @@ -1,127 +1,16 @@ -"""Tests for USB-recovery tarball handling and device selection. +"""Tests for Rockchip USB device selection and transport behaviour. -These cover the guards that stop a flash going wrong in a way the operator -would not notice: a truncated tarball, a corrupted image, an oversized one, a -partial transfer the device reported as OK, or the wrong board entirely. +Most of what is pinned here was learnt from a Luckfox Pico Max on the bench: +which device on the bus is actually a recovery target, which stage it is in, +and the several ways an inherited or half-finished transaction makes a +healthy board look dead. """ -import hashlib -import io -import tarfile - import pytest -import typer -from defib.cli.app import _read_usb_payloads -from defib.profiles.schema import FlashPartition from defib.rockusb.device import DeviceMode, FoundDevice from defib.rockusb.protocol import CSW_SIGNATURE, Opcode -PARTS = { - "idblock": FlashPartition(lba=512, sectors=512), - "uboot": FlashPartition(lba=1024, sectors=1024), - "boot": FlashPartition(lba=2048, sectors=8192), - "rootfs": FlashPartition(lba=92160, sectors=163840), -} - - -def _make_tarball(tmp_path, files: dict[str, bytes], *, checksums=True, corrupt=()): - """Build an OpenIPC-shaped tarball, optionally with bad checksums.""" - path = tmp_path / "fw.tgz" - with tarfile.open(path, "w:gz") as tar: - for name, data in files.items(): - info = tarfile.TarInfo(name) - info.size = len(data) - tar.addfile(info, io.BytesIO(data)) - if not checksums: - continue - digest = hashlib.md5(data).hexdigest() - if name in corrupt: - digest = "0" * 32 - line = f"{digest} {name}\n".encode() - sig = tarfile.TarInfo(f"{name}.md5sum") - sig.size = len(line) - tar.addfile(sig, io.BytesIO(line)) - return path - - -def _complete(**overrides) -> dict[str, bytes]: - files = { - "zboot.img.rv1106": b"\xaa" * 2048, - "rootfs.squashfs.rv1106": b"\xbb" * 4096, - } - files.update(overrides) - return files - - -class TestReadUsbPayloads: - def test_reads_a_complete_tarball(self, tmp_path): - payloads = _read_usb_payloads(_make_tarball(tmp_path, _complete()), PARTS) - assert [(n, p) for n, p, _, _ in payloads] == [ - ("zboot.img.rv1106", "boot"), - ("rootfs.squashfs.rv1106", "rootfs"), - ] - - def test_returns_image_bytes(self, tmp_path): - payloads = _read_usb_payloads(_make_tarball(tmp_path, _complete()), PARTS) - assert dict((n, d) for n, _, _, d in payloads)["zboot.img.rv1106"] == b"\xaa" * 2048 - - def test_missing_rootfs_refused(self, tmp_path): - """Half a firmware written and called a success leaves an unbootable - board — the UART installer rejects this too.""" - tar = _make_tarball(tmp_path, {"zboot.img.rv1106": b"\xaa" * 16}) - with pytest.raises(typer.BadParameter, match="no image for rootfs"): - _read_usb_payloads(tar, PARTS) - - def test_missing_kernel_refused(self, tmp_path): - tar = _make_tarball(tmp_path, {"rootfs.squashfs.rv1106": b"\xbb" * 16}) - with pytest.raises(typer.BadParameter, match="no image for boot"): - _read_usb_payloads(tar, PARTS) - - def test_empty_tarball_refused(self, tmp_path): - tar = _make_tarball(tmp_path, {"README": b"nothing here"}) - with pytest.raises(typer.BadParameter, match="maps to a partition"): - _read_usb_payloads(tar, PARTS) - - def test_idblock_ordered_last(self, tmp_path): - files = _complete() - files["idblock.img.rv1106"] = b"\xcc" * 512 - payloads = _read_usb_payloads(_make_tarball(tmp_path, files), PARTS) - assert [p for _, p, _, _ in payloads][-1] == "idblock" - - -class TestChecksums: - def test_corrupted_image_refused(self, tmp_path): - """--verify compares flash against what was sent, so it cannot catch a - bad download; only the shipped md5sum can.""" - tar = _make_tarball(tmp_path, _complete(), corrupt=("zboot.img.rv1106",)) - with pytest.raises(typer.BadParameter, match="MD5 mismatch"): - _read_usb_payloads(tar, PARTS) - - def test_error_names_both_digests(self, tmp_path): - tar = _make_tarball(tmp_path, _complete(), corrupt=("zboot.img.rv1106",)) - with pytest.raises(typer.BadParameter) as excinfo: - _read_usb_payloads(tar, PARTS) - message = str(excinfo.value) - assert "0" * 32 in message - assert hashlib.md5(b"\xaa" * 2048).hexdigest() in message - - def test_tarball_without_checksums_still_works(self, tmp_path): - """Absent checksums are not an error — only mismatching ones are.""" - tar = _make_tarball(tmp_path, _complete(), checksums=False) - assert len(_read_usb_payloads(tar, PARTS)) == 2 - - -class TestBounds: - def test_oversized_image_refused(self, tmp_path): - files = _complete(**{"zboot.img.rv1106": b"\xaa" * (PARTS["boot"].size_bytes + 1)}) - with pytest.raises(typer.BadParameter, match="would overwrite"): - _read_usb_payloads(_make_tarball(tmp_path, files), PARTS) - - def test_exactly_full_partition_accepted(self, tmp_path): - files = _complete(**{"zboot.img.rv1106": b"\xaa" * PARTS["boot"].size_bytes}) - assert len(_read_usb_payloads(_make_tarball(tmp_path, files), PARTS)) == 2 - class TestFoundDevice: def _dev(self, bus=1, ports=(4, 2), mode=DeviceMode.MASKROM): @@ -154,44 +43,6 @@ def test_str_mentions_the_path(self): assert "1-4.2" in str(self._dev()) -class TestForeignSocImages: - """The SoC suffix is the only thing distinguishing an image built for this - board from one that would leave it unbootable.""" - - def test_foreign_suffix_refused(self, tmp_path): - files = { - "zboot.img.hi3516ev300": b"\xaa" * 2048, - "rootfs.squashfs.hi3516ev300": b"\xbb" * 4096, - } - with pytest.raises(typer.BadParameter, match="built for 'hi3516ev300'"): - _read_usb_payloads(_make_tarball(tmp_path, files), PARTS, "rv1106") - - def test_error_names_the_expected_chip(self, tmp_path): - files = {"zboot.img.hi3516ev300": b"\xaa" * 16} - with pytest.raises(typer.BadParameter, match="not 'rv1106'"): - _read_usb_payloads(_make_tarball(tmp_path, files), PARTS, "rv1106") - - def test_matching_suffix_accepted(self, tmp_path): - payloads = _read_usb_payloads( - _make_tarball(tmp_path, _complete()), PARTS, "rv1106" - ) - assert len(payloads) == 2 - - def test_suffix_match_is_case_insensitive(self, tmp_path): - payloads = _read_usb_payloads( - _make_tarball(tmp_path, _complete()), PARTS, "RV1106" - ) - assert len(payloads) == 2 - - def test_no_chip_given_skips_the_check(self, tmp_path): - """Callers that genuinely do not know the SoC keep the old behaviour.""" - files = { - "zboot.img.whatever": b"\xaa" * 16, - "rootfs.squashfs.whatever": b"\xbb" * 16, - } - assert len(_read_usb_payloads(_make_tarball(tmp_path, files), PARTS)) == 2 - - class TestRecoveryIdFilter: """A running Luckfox presents 2207:0019 — an RNDIS+ADB gadget sharing the vendor id, whose bcdUSB of 0x0200 makes it look like MaskROM to the From 504886f4fdf0df243d118b988a4ac227cc6dbf24 Mon Sep 17 00:00:00 2001 From: Dmitry Ilyin <6576495+widgetii@users.noreply.github.com> Date: Thu, 20 Aug 2026 08:16:51 +0300 Subject: [PATCH 8/8] rockusb: fix what the review found in the narrowed PR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six findings, four of them introduced by the two commits just before this one — which is the argument for re-reviewing after a rewrite rather than assuming the earlier pass still applies. Worst of them: `burn` finished by telling the user to run `defib install -c rv1106`, a command the previous commit had just made refuse that chip. Every successfully recovered board would have been pointed straight at a guaranteed failure. It now says what is true — the board is awake and in loader mode. The USB rejection in `install` printed Rich text unconditionally, so `--output json` got prose where automation expects an event. Same for `burn` when the loader arguments are missing: that path raises typer.BadParameter, which the handler did not catch. `recovery_mode()` swallowed every ValueError and called the result UART. So `rv1106:typo` quietly entered the serial workflow and failed later complaining about a serial port, rather than saying the variant does not exist. It now defaults to UART only when the chip has no profile at all — the case that fallback was for — and profile problems surface as themselves. `burn` never released the device. A claimed interface outliving the command makes the next attempt unable to open the board, which is indistinguishable from hardware that has stopped answering; given how much of this session was spent on boards that appeared wedged, that is not a leak worth keeping. And upload progress compared real bytes sent against the unframed blob length, so a 3-byte blob reported 5 of 3. Framing adds a CRC and sometimes a terminator; both ends now measure framed bytes. Co-Authored-By: Claude Opus 4.8 --- src/defib/cli/app.py | 52 +++++++++++++++++++++-------- src/defib/profiles/loader.py | 14 +++++--- src/defib/rockusb/recovery.py | 15 ++++++++- tests/test_profiles_usb_recovery.py | 27 +++++++++++++++ tests/test_rockusb_device.py | 29 ++++++++++++++++ 5 files changed, 118 insertions(+), 19 deletions(-) diff --git a/src/defib/cli/app.py b/src/defib/cli/app.py index 15e9e05..2c2c4cf 100644 --- a/src/defib/cli/app.py +++ b/src/defib/cli/app.py @@ -55,7 +55,6 @@ async def _burn_async( from rich.console import Console from rich.progress import Progress, SpinnerColumn, TextColumn, BarColumn, TaskProgressColumn - from defib.profiles.loader import recovery_mode from defib.recovery.events import LogEvent, ProgressEvent from defib.recovery.session import RecoverySession @@ -66,7 +65,7 @@ async def _burn_async( else: logging.basicConfig(level=logging.INFO) - if recovery_mode(chip) == "usb": + if _recovery_mode_or_exit(chip, output) == "usb": await _burn_usb_async( chip, ddr, usbplug, loader, power_cycle, output, wait, poe_port_override, usb_path, @@ -2220,7 +2219,6 @@ async def _install_async( ) from defib.network.ip_manager import list_interfaces, temporary_ip from defib.network.tftp_server import start_tftp_server - from defib.profiles.loader import recovery_mode from defib.recovery.events import LogEvent, ProgressEvent from defib.recovery.session import RecoverySession from defib.transport.serial_platform import create_transport, normalize_port_name @@ -2232,16 +2230,18 @@ async def _install_async( else: logging.basicConfig(level=logging.INFO) - if recovery_mode(chip) == "usb": + if _recovery_mode_or_exit(chip, output) == "usb": # Writing flash over USB is not landed yet: this chip's images have # nowhere correct to go until the UBI-vs-raw rootfs question is # settled, and the write path has never run against hardware. Wake # the board with `burn` and flash it by other means meanwhile. - console.print( - f"[red]{chip} recovers over USB, and `install` does not support " - "that yet — use `defib burn` to bring the board up.[/red]" + # Routed through _usb_fail so --output json still gets an event. + _usb_fail( + output, + f"{chip} recovers over USB, and `install` does not support that " + "yet — use `defib burn` to bring the board up.", ) - raise typer.Exit(1) + return if nand: layout = _NAND_LAYOUT @@ -3642,6 +3642,22 @@ async def _replace_in_tftp(name: str, data: bytes) -> None: print(json_mod.dumps({"event": "done", "success": True, "partitions": len(partitions)})) +def _recovery_mode_or_exit(chip: str, output: str) -> str: + """Resolve how ``chip`` is recovered, reporting profile problems cleanly. + + ``recovery_mode`` deliberately lets a bad variant or malformed profile + propagate rather than defaulting to UART, so this turns that into a + message rather than a traceback. + """ + from defib.profiles.loader import recovery_mode + + try: + return recovery_mode(chip) + except ValueError as e: + _usb_fail(output, str(e)) + raise AssertionError("unreachable") from e # pragma: no cover + + def _usb_progress_printer(output: str) -> Any: """Progress callback that emits JSON lines, or nothing in other modes.""" import json as json_mod @@ -3781,9 +3797,11 @@ async def _burn_usb_async( console = Console() recovery_ids = load_profile(chip).usb_recovery_ids - # Loader resolution sits inside the handler: an unreadable file or a - # malformed container would otherwise escape as a traceback, and in JSON - # mode that means no error event at all. + # Loader resolution sits inside the handler: an unreadable file, a + # malformed container or missing arguments would otherwise escape as + # Typer text or a traceback, and in JSON mode that means no error event + # at all. + recovery = None try: blobs = _resolve_usb_loader(chip, ddr, usbplug, loader) recovery = await _open_usb_target( @@ -3791,9 +3809,15 @@ async def _burn_usb_async( recovery_ids, ) flash_id = await recovery.read_flash_id() - except (RockusbError, LoaderFormatError, OSError) as e: + except (RockusbError, LoaderFormatError, OSError, typer.BadParameter) as e: _usb_fail(output, str(e)) return + finally: + # Release the interface either way. A claimed handle outliving the + # command leaves the next attempt unable to open the board, which is + # indistinguishable from a device that has stopped responding. + if recovery is not None: + recovery.close() if output == "json": print(json_mod.dumps({ @@ -3802,8 +3826,8 @@ async def _burn_usb_async( elif output != "quiet": console.print(f" Flash ID: {flash_id.hex()}") console.print( - "\n[green bold]Device is awake.[/green bold] Flash it with: " - f"defib install -c {chip} --firmware " + "\n[green bold]Device is awake[/green bold] and in loader mode, " + "ready for a flashing tool." ) diff --git a/src/defib/profiles/loader.py b/src/defib/profiles/loader.py index cd5e13b..748fe70 100644 --- a/src/defib/profiles/loader.py +++ b/src/defib/profiles/loader.py @@ -110,13 +110,19 @@ def load_profile(chip_name: str, profiles_dir: Path | None = None) -> SoCProfile def recovery_mode(chip_name: str, profiles_dir: Path | None = None) -> str: """How ``chip_name`` is reached when it is dead: ``"uart"`` or ``"usb"``. - Defaults to ``"uart"`` for anything without a profile, which covers the - V500 and CV6xx families whose chip lists live in their protocol modules - rather than in JSON. + Defaults to ``"uart"`` only when the chip has no profile at all, which + covers the V500 and CV6xx families whose chip lists live in their + protocol modules rather than in JSON. + + Anything else — an unknown variant, an alias loop, malformed JSON, a + profile that fails validation — propagates. Treating those as UART would + quietly route a USB-recovery chip into the serial workflow and report the + wrong problem: ``rv1106:typo`` should say the variant is unknown, not + fail later trying to open a serial port. """ try: return load_profile(chip_name, profiles_dir).recovery - except (FileNotFoundError, ValueError): + except FileNotFoundError: return "uart" diff --git a/src/defib/rockusb/recovery.py b/src/defib/rockusb/recovery.py index 3d616fc..cc061b6 100644 --- a/src/defib/rockusb/recovery.py +++ b/src/defib/rockusb/recovery.py @@ -119,6 +119,10 @@ async def _upload( on_progress: Callable[[ProgressEvent], None] | None, ) -> None: chunks = build_maskrom_chunks(blob, use_rc4=use_rc4) + # Total is the framed size, not the blob's: framing appends a CRC and + # sometimes a terminator packet, so measuring real bytes sent against + # the raw length reports more than 100%. + total = sum(len(c) for c in chunks) sent = 0 for chunk in chunks: # Count what the wire took, not what we handed it. control_write @@ -127,7 +131,7 @@ async def _upload( sent += await asyncio.to_thread(self._device.control_write, code, chunk) _emit( on_progress, - ProgressEvent(stage, sent, len(blob), f"{name} -> {code:#06x}"), + ProgressEvent(stage, sent, total, f"{name} -> {code:#06x}"), ) logger.debug( "uploaded %s (%d bytes in %d chunks) to %#06x", name, len(blob), len(chunks), code @@ -190,6 +194,15 @@ async def read_flash_id(self) -> bytes: self._device.command, Opcode.READ_FLASH_ID, read_length=5 ) + def close(self) -> None: + """Release the underlying device. + + Whoever drove the recovery owns the handle; leaving it claimed makes + the next attempt fail to open the board, which looks exactly like + hardware that has stopped answering. + """ + self._device.close() + async def reset(self, subcode: ResetSubcode = ResetSubcode.NORMAL) -> None: """Reset the device. diff --git a/tests/test_profiles_usb_recovery.py b/tests/test_profiles_usb_recovery.py index 8140d4d..2fe2f70 100644 --- a/tests/test_profiles_usb_recovery.py +++ b/tests/test_profiles_usb_recovery.py @@ -128,3 +128,30 @@ def test_end_lba(self): def test_size_bytes(self): assert FlashPartition(lba=0, sectors=8192).size_bytes == 4 * 1024 * 1024 + + +class TestRecoveryModeErrorHandling: + """recovery_mode() must default to UART only for a chip with no profile. + + Swallowing every ValueError would route a mistyped USB chip into the + serial workflow and then report the wrong problem entirely — a failure to + open a serial port, rather than "that variant does not exist". + """ + + def test_missing_profile_defaults_to_uart(self): + assert recovery_mode("no-such-chip-at-all", PROFILES_DIR) == "uart" + + def test_unknown_variant_propagates(self): + with pytest.raises(ValueError, match="Unknown variant"): + recovery_mode("rv1106:typo", PROFILES_DIR) + + def test_unknown_variant_on_uart_chip_also_propagates(self): + with pytest.raises(ValueError, match="Unknown variant"): + recovery_mode("hi3516cv300:nope", PROFILES_DIR) + + def test_declared_variant_still_resolves(self): + from defib.profiles.loader import list_variants + + for chip in ("hi3516cv300", "rv1106"): + for variant in list_variants(chip, PROFILES_DIR): + assert recovery_mode(f"{chip}:{variant}", PROFILES_DIR) in ("uart", "usb") diff --git a/tests/test_rockusb_device.py b/tests/test_rockusb_device.py index 100662a..cd9e596 100644 --- a/tests/test_rockusb_device.py +++ b/tests/test_rockusb_device.py @@ -528,3 +528,32 @@ def test_nothing_detached_means_nothing_restored(self): device._detached_interface = None device._reattach_kernel_driver() assert events == [] + + +class TestUploadProgress: + """Progress is measured in framed bytes at both ends. + + Framing appends a CRC and sometimes a terminator packet, so counting real + bytes sent against the raw blob length reports over 100% — a 3-byte blob + would announce 5 of 3. + """ + + def test_framed_total_covers_the_crc(self): + from defib.rockusb.maskrom import build_maskrom_chunks + + blob = b"\x01\x02\x03" + total = sum(len(c) for c in build_maskrom_chunks(blob)) + assert total == len(blob) + 2 + assert total > len(blob) + + def test_progress_never_exceeds_total(self): + from defib.rockusb.maskrom import build_maskrom_chunks + + for size in (1, 3, 4094, 4095, 4096, 10000): + chunks = build_maskrom_chunks(b"\xa5" * size) + total = sum(len(c) for c in chunks) + sent = 0 + for c in chunks: + sent += len(c) + assert sent <= total + assert sent == total