From f18c4ed221aef236f301600d2e9e2e811b8a05d1 Mon Sep 17 00:00:00 2001 From: Brad Barnett Date: Mon, 21 Sep 2026 21:51:13 -0500 Subject: [PATCH] firmware flash: run the partition-layout check from the image itself, and say what differs The layout check took the table it expects from the build directory's partition_table/partition-table.bin, so it silently skipped itself whenever that directory was not there -- which is every flash of a saved .bin with --artifact, not just the ROM-download-port case the issue reported. That is also the flash people reach for when something has already gone wrong. A whole-flash image carries its own table at 0x8000, so take it from there when the build directory is gone. Reading the device's table needed one more thing: esptool was only ever asked with --before default-reset, which is right for a board sitting at a REPL and wrong for one already in ROM download mode, where toggling DTR/RTS can knock it back out. Try no-reset as well. When the tables differ, decode both and name the partition, the field and the two values instead of saying only that they differ. "Enable Erase if the board misbehaves" was the wrong advice for the failure this exists to prevent: the T-Embed's files were intact 64 KB further on, and an erase would have destroyed them. Say what moved, and why flashing anyway leaves a filesystem where the new table does not expect it -- the board then sits in inisetup.fs_corrupted() before USB starts, with no panic and nothing on the bus. Add mpftp firmware ptable , which prints an image's table and diffs it against another image or against an attached board. That is the command that would have found the T-Embed's fault in seconds. Proven on a Waveshare ESP32-S3-Touch-LCD-4.3, with a copy of its own firmware whose vfs entry was moved to 0x390000. Refused on the REPL port and again through the ROM download port after machine.bootloader() (COM73 at 303A:4003 re-enumerated as COM13 at 303A:1001), both times reporting "vfs: offset 0x3a0000 on the device, 0x390000 in the image". The unmodified image flashed through both ports in the same session, reporting "partition layout matches", so the check is not simply refusing everything. --- cli/src/mpftp/cli.py | 16 ++ cli/src/mpftp/firmware.py | 328 +++++++++++++++++++++++++----- cli/tests/test_partition_table.py | 147 +++++++++++++ 3 files changed, 440 insertions(+), 51 deletions(-) create mode 100644 cli/tests/test_partition_table.py diff --git a/cli/src/mpftp/cli.py b/cli/src/mpftp/cli.py index 9d77949..1e97584 100755 --- a/cli/src/mpftp/cli.py +++ b/cli/src/mpftp/cli.py @@ -1630,6 +1630,14 @@ def cmd_firmware(ns: argparse.Namespace) -> None: if sub == "artifact": out(_engine_json("artifact", _sel_args(ns))) return + if sub == "ptable": + extra = [ns.image] + if getattr(ns, "compare", ""): + extra += ["--compare", ns.compare] + if getattr(ns, "device", ""): + extra += ["--device", ns.device] + out(_engine_json("ptable", extra)) + return if sub == "build": extra = _sel_args(ns) if ns.clean: @@ -2130,6 +2138,14 @@ def build_parser() -> argparse.ArgumentParser: func=cmd_firmware ) + fwp = fwsub.add_parser( + "ptable", help="Print a firmware image's partition table; diff two, or a board's" + ) + fwp.add_argument("image", help="Firmware .bin (whole-flash image)") + fwp.add_argument("--compare", default="", help="Second image to diff against") + fwp.add_argument("--device", default="", help="Also read and diff this board's table") + fwp.set_defaults(func=cmd_firmware) + fwf = fwsub.add_parser("flash", parents=[fw_sel, device_opts], help="Flash a built or downloaded artifact") fwf.add_argument("--artifact", help="Explicit firmware file (else last build)") fwf.add_argument("--family", default="", help="MCU family for flash offset (download mode)") diff --git a/cli/src/mpftp/firmware.py b/cli/src/mpftp/firmware.py index 9c7147d..e0a4b88 100644 --- a/cli/src/mpftp/firmware.py +++ b/cli/src/mpftp/firmware.py @@ -34,6 +34,7 @@ import os import re import shutil +import struct import subprocess import sys import time @@ -1309,6 +1310,123 @@ def _esptool_cmd(ns: argparse.Namespace) -> list[str]: # Standard esp32 partition-table offset (CONFIG_PARTITION_TABLE_OFFSET). _PARTITION_TABLE_OFFSET = 0x8000 +# A binary partition table is 32-byte entries from _PARTITION_TABLE_OFFSET. +# Real entries start with the magic AA 50; the trailing checksum entry starts +# EB EB; the rest of the 0xC00 region is erase padding. +_PT_ENTRY_MAGIC = b"\xaa\x50" +_PT_MD5_MAGIC = b"\xeb\xeb" +_PT_ENTRY_SIZE = 32 +_PT_REGION_SIZE = 0xC00 + +_PT_TYPES = {0: "app", 1: "data"} +_PT_SUBTYPES = { + (0, 0x00): "factory", + (0, 0x10): "ota_0", + (0, 0x11): "ota_1", + (0, 0x20): "test", + (1, 0x00): "ota", + (1, 0x01): "phy", + (1, 0x02): "nvs", + (1, 0x03): "coredump", + (1, 0x04): "nvs_keys", + (1, 0x05): "efuse", + (1, 0x06): "undefined", + (1, 0x80): "esphttpd", + (1, 0x81): "fat", + (1, 0x82): "spiffs", + (1, 0x83): "littlefs", +} + + +def parse_partition_table(blob: bytes) -> list[dict[str, Any]]: + """Decode a binary esp32 partition table into rows. + + Stops at the checksum entry or at the first thing that is not an entry, so + it is safe to hand the whole 0xC00 region including its erase padding. + """ + rows: list[dict[str, Any]] = [] + for off in range(0, max(0, len(blob) - _PT_ENTRY_SIZE + 1), _PT_ENTRY_SIZE): + entry = blob[off : off + _PT_ENTRY_SIZE] + if entry[:2] != _PT_ENTRY_MAGIC: + break # EB EB checksum row, or 0xFF padding: the table ends here + ptype, subtype = entry[2], entry[3] + p_offset, p_size = struct.unpack(" list[str]: + """Say, in sentences, every way the device's table differs from the image's. + + An empty list means identical. The point is to name the partition, the + field and both values: "the tables differ" is what cost a night on a + T-Embed whose vfs had moved 64 KB. + """ + out: list[str] = [] + want_by_name = {r["name"]: r for r in want} + got_by_name = {r["name"]: r for r in got} + for row in want: + name = row["name"] + other = got_by_name.get(name) + if other is None: + out.append(f"{name}: in the image, absent on the device") + continue + for field in ("offset", "size"): + if row[field] != other[field]: + out.append( + f"{name}: {field} {hex(other[field])} on the device, " + f"{hex(row[field])} in the image" + ) + for field in ("type", "subtype"): + if row[field] != other[field]: + out.append( + f"{name}: {field} {other[field]} on the device, " + f"{row[field]} in the image" + ) + for row in got: + if row["name"] not in want_by_name: + out.append(f"{row['name']}: on the device, absent from the image") + return out + + +def partition_table_from_image(artifact: Path, offset: Any = 0) -> Optional[bytes]: + """Slice the partition table out of a whole-flash image, or None. + + A full esp32 image written at 0x0 carries its own table at 0x8000, so the + layout can be checked from the ``.bin`` alone. That matters because the + build directory that produced it is usually gone -- flashing a saved image + with ``--artifact`` is exactly the case where the sibling + ``partition_table/partition-table.bin`` does not exist, and where the check + used to be skipped. + """ + try: + if int(str(offset), 0) != 0: + return None # a partial image does not contain the table + except (TypeError, ValueError): + return None + try: + with artifact.open("rb") as fh: + fh.seek(_PARTITION_TABLE_OFFSET) + blob = fh.read(_PT_REGION_SIZE) + except OSError: + return None + if len(blob) < _PT_ENTRY_SIZE or blob[:2] != _PT_ENTRY_MAGIC: + return None + return blob + def _esptool_reset_mode(value: str, default: str) -> str: """Normalize esptool v5 reset-mode names (hyphens; accept legacy underscores).""" @@ -1321,54 +1439,93 @@ def _read_device_partition_table(base: list[str], nbytes: int) -> Optional[bytes The read runs through the same esptool as the flash (Windows esptool under WSL), writing to a temp file whose path is translated for that host. + + Tried twice: ``default-reset`` first, which is what a board sitting at a + REPL needs, then ``no-reset`` for a board already in ROM download mode, + where toggling DTR/RTS can knock it back out. Reading the table is the + whole point of flashing through the ROM port -- that is the route with no + REPL to ask, so it is the one where a wrong layout goes unnoticed. """ import tempfile tmp = Path(tempfile.gettempdir()) / f"mpftp_pt_{os.getpid()}.bin" out_arg = _wslpath_w(str(tmp)) if HOST == "wsl" else str(tmp) - cmd = base + [ - "--before", - "default-reset", - "--after", - "no-reset", - "read-flash", - hex(_PARTITION_TABLE_OFFSET), - hex(nbytes), - out_arg, - ] - try: - r = subprocess.run( - cmd, capture_output=True, text=True, timeout=60, **_no_window_kwargs() - ) - if r.returncode != 0 or not tmp.is_file(): - return None - return tmp.read_bytes() - except Exception: - return None - finally: + for before in ("default-reset", "no-reset"): + cmd = base + [ + "--before", + before, + "--after", + "no-reset", + "read-flash", + hex(_PARTITION_TABLE_OFFSET), + hex(nbytes), + out_arg, + ] try: - tmp.unlink() + r = subprocess.run( + cmd, capture_output=True, text=True, timeout=60, **_no_window_kwargs() + ) + if r.returncode == 0 and tmp.is_file(): + return tmp.read_bytes() except Exception: pass + finally: + try: + tmp.unlink() + except Exception: + pass + return None -def _esp32_layout_changed(base: list[str], artifact: Path) -> Optional[bool]: - """Does the device's partition table differ from the one we're about to flash? +def _expected_partition_table(artifact: Path, offset: Any = 0) -> tuple[Optional[bytes], str]: + """The table this flash will install, and where it was found. - Returns True if it changed (user must confirm a full erase — a moved - vfs/storage offset otherwise leaves a stale filesystem), False if identical, - or None if it couldn't be determined. + The build directory's ``partition_table/partition-table.bin`` is preferred + because it is exactly the bytes the build produced; a whole-flash image + carries the same table at 0x8000 and is the fallback, which is what makes + the check work for a saved ``.bin`` flashed with ``--artifact``. """ - new_pt = artifact.parent / "partition_table" / "partition-table.bin" - if not new_pt.is_file(): - return None - try: - want = new_pt.read_bytes() - except Exception: - return None - got = _read_device_partition_table(base, len(want)) - if got is None: - return None - return got[: len(want)] != want + sibling = artifact.parent / "partition_table" / "partition-table.bin" + if sibling.is_file(): + try: + return sibling.read_bytes(), str(sibling) + except OSError: + pass + blob = partition_table_from_image(artifact, offset) + if blob is not None: + return blob, f"{artifact.name} @ {hex(_PARTITION_TABLE_OFFSET)}" + return None, "" + + +def _esp32_layout_check( + base: list[str], artifact: Path, offset: Any = 0 +) -> dict[str, Any]: + """Compare the device's partition table with the one about to be flashed. + + Returns ``determined``, and when determined, ``changed`` plus a list of + ``differences`` naming partition, field and both values. A moved + vfs/storage offset leaves a stale filesystem that boots corrupt, and the + board then sits in ``inisetup.fs_corrupted()`` before USB starts -- no + panic, no console, nothing on the bus. Saying which partition moved is the + difference between a minute and a night. + """ + want_blob, source = _expected_partition_table(artifact, offset) + if want_blob is None: + return { + "determined": False, + "reason": "no partition table in the image or beside it", + } + got_blob = _read_device_partition_table(base, len(want_blob)) + if got_blob is None: + return {"determined": False, "reason": "could not read the device's table"} + want = parse_partition_table(want_blob) + got = parse_partition_table(got_blob[: len(want_blob)]) + differences = diff_partition_tables(want, got) + return { + "determined": True, + "changed": bool(differences) or got_blob[: len(want_blob)] != want_blob, + "differences": differences, + "source": source, + } def flash_esp32(ns: argparse.Namespace, mp: Optional[Path], artifact: Path) -> None: @@ -1388,34 +1545,48 @@ def flash_esp32(ns: argparse.Namespace, mp: Optional[Path], artifact: Path) -> N if not erase: # A moved vfs/storage offset leaves a stale filesystem that boots corrupt. # Never auto-erase: warn and require an explicit erase + second Flash. - changed = _esp32_layout_changed(base, artifact) - if changed: + check = _esp32_layout_check(base, artifact, offset) + if check.get("determined") and check.get("changed"): + differences = check.get("differences") or [] + detail = "; ".join(differences) if differences else ( + "the tables differ byte for byte but name the same partitions" + ) emit_log( - "[mpftp] partition layout on the device differs from this firmware" + "[mpftp] partition layout on the device differs from this firmware: " + + detail ) emit_result( False, error=( - "Partition table on the device differs from this build. " - "Enable “Erase flash before writing” and click Flash again. " - "A full erase wipes the filesystem (vfs/storage) partition — " - "all board files will be lost." + "Partition table on the device differs from this build: " + f"{detail}. Flashing anyway would leave the old filesystem " + "where the new table does not expect it, and the board can " + "boot into fs_corrupted() with nothing on the USB bus. " + "Re-flash with erase to apply the new table — that wipes the " + "filesystem (vfs/storage) partition, so copy anything you " + "want off the board first." ), needEraseConfirm={ "reason": "partition_layout_changed", "message": ( - "The on-device partition table does not match this firmware. " - "Re-flashing with erase will apply the new table but wipe the " - "filesystem (vfs/storage) partition — all files on the board " - "will be lost." + "The on-device partition table does not match this firmware " + f"({detail}). Re-flashing with erase will apply the new table " + "but wipe the filesystem (vfs/storage) partition — all files " + "on the board will be lost." ), + "differences": differences, }, ) return - if changed is None: + if check.get("determined"): emit_log( - "[mpftp] could not read on-device partition table; " - "skipping layout check (enable Erase if the board misbehaves)" + "[mpftp] partition layout matches (compared against " + f"{check.get('source')})" + ) + else: + emit_log( + "[mpftp] could not check the partition layout: " + f"{check.get('reason')}" ) if erase: @@ -1934,6 +2105,53 @@ def _autosize_regenerate_override( return new_size +def do_ptable(ns: argparse.Namespace) -> None: + """Print a firmware image's partition table, and diff it on request. + + The table is 32-byte entries from 0x8000, magic AA 50. Having this as one + command is what would have caught a T-Embed image whose vfs sat 64 KB off + in seconds instead of several trips to the BOOT button. + """ + image = Path(ns.image).expanduser() + blob = partition_table_from_image(image, 0) + if blob is None: + print_json( + { + "error": f"no partition table at {hex(_PARTITION_TABLE_OFFSET)} in {image}", + "hint": "ptable wants a whole-flash image (the .bin written at 0x0)", + } + ) + raise SystemExit(1) + result: dict[str, Any] = {"image": str(image), "rows": parse_partition_table(blob)} + + if ns.compare: + other_path = Path(ns.compare).expanduser() + other = partition_table_from_image(other_path, 0) + if other is None: + result["compareError"] = f"no partition table in {other_path}" + else: + other_rows = parse_partition_table(other) + result["compare"] = str(other_path) + result["compareRows"] = other_rows + result["differences"] = diff_partition_tables( + result["rows"], other_rows + ) + + if ns.device: + base = _esptool_cmd(ns) + ["-b", str(ns.baud or 460800), "-p", ns.device] + got = _read_device_partition_table(base, len(blob)) + if got is None: + result["deviceError"] = "could not read the device's table" + else: + device_rows = parse_partition_table(got[: len(blob)]) + result["device"] = ns.device + result["deviceRows"] = device_rows + result["deviceDifferences"] = diff_partition_tables( + result["rows"], device_rows + ) + print_json(result) + + def do_partitions(ns: argparse.Namespace) -> None: mp = Path(ns.mp).expanduser().resolve() workspace = workspace_of(mp) @@ -2744,6 +2962,14 @@ def add_mp(sp: argparse.ArgumentParser, required: bool = False) -> None: help="JSON of MicroPython interpreter hints (optional enrichment)") dt.set_defaults(func=do_detect) + ptb = sub.add_parser("ptable", help="Print an image's partition table; diff it") + ptb.add_argument("image", help="Firmware .bin (whole-flash image)") + ptb.add_argument("--compare", default="", help="Second image to diff against") + ptb.add_argument("--device", default="", help="Also read and diff this board's table") + ptb.add_argument("--baud", type=int, default=460800) + ptb.add_argument("--esptool", default="") + ptb.set_defaults(func=do_ptable) + pt = sub.add_parser("partitions") add_mp(pt, required=True) pt.add_argument("action", choices=["get", "set", "reset", "candidates", "split"]) diff --git a/cli/tests/test_partition_table.py b/cli/tests/test_partition_table.py new file mode 100644 index 0000000..ee4ef47 --- /dev/null +++ b/cli/tests/test_partition_table.py @@ -0,0 +1,147 @@ +"""Binary partition tables: decode, diff, and find one inside a firmware image. + +The check these back exists because a T-Embed S3 was flashed through its ROM +download port with an image whose ``vfs`` sat at 0x390000 where every image that +board had run has it at 0x3a0000. esptool verified the write; the board then +never enumerated, because MicroPython's ``_boot.py`` found a first block that +was neither blank nor mountable and sat in ``inisetup.fs_corrupted()`` before +the runtime USB device starts. No panic, no core dump, nothing on the bus. + +So each test here plants the fault it is meant to catch, rather than asserting +that a good table looks good -- a comparison that cannot fail is not evidence. +""" + +from __future__ import annotations + +import struct +import tempfile +import unittest +from pathlib import Path + +from mpftp.firmware import ( + _PARTITION_TABLE_OFFSET, + diff_partition_tables, + parse_partition_table, + partition_table_from_image, +) + + +def entry(name: str, ptype: int, subtype: int, offset: int, size: int) -> bytes: + return ( + b"\xaa\x50" + + bytes([ptype, subtype]) + + struct.pack(" Path: + tmp = Path(tempfile.mkdtemp()) / "firmware.bin" + blob = bytearray(b"\x00" * (_PARTITION_TABLE_OFFSET + 0xC00)) + blob[_PARTITION_TABLE_OFFSET : _PARTITION_TABLE_OFFSET + len(table)] = table + for i in range(_PARTITION_TABLE_OFFSET + len(table), len(blob)): + blob[i] = 0xFF + tmp.write_bytes(bytes(blob)) + return tmp + + def test_it_finds_the_table_a_whole_flash_image_carries(self): + # This is the case that used to skip the check entirely: a saved .bin + # flashed with --artifact has no sibling partition_table/ directory. + rows = parse_partition_table(partition_table_from_image(self._image(GOOD), 0)) + self.assertEqual("vfs", rows[-1]["name"]) + + def test_it_declines_a_partial_image(self): + # Written at a non-zero offset, the file is not a whole-flash image and + # 0x8000 into it is application code, not a table. + self.assertIsNone(partition_table_from_image(self._image(GOOD), 0x10000)) + + def test_it_declines_a_file_with_no_table_where_one_should_be(self): + tmp = Path(tempfile.mkdtemp()) / "notfirmware.bin" + tmp.write_bytes(b"\x00" * (_PARTITION_TABLE_OFFSET + 0xC00)) + self.assertIsNone(partition_table_from_image(tmp, 0)) + + def test_it_declines_a_file_too_short_to_reach_the_table(self): + tmp = Path(tempfile.mkdtemp()) / "short.bin" + tmp.write_bytes(b"\x00" * 16) + self.assertIsNone(partition_table_from_image(tmp, 0)) + + +if __name__ == "__main__": + unittest.main()