From 112453671482acec86de327d15972d40a979e73d Mon Sep 17 00:00:00 2001 From: Brad Barnett <127794626+bdbarnett@users.noreply.github.com> Date: Wed, 21 Oct 2026 14:33:10 -0500 Subject: [PATCH] get: ask again for a chunk that arrives with extra bytes on it mpremote reads a file 256 bytes at a time as print(repr(r(256))) and parses the line with ast.literal_eval. A P4 DEV-KIT on its native USB sometimes sends more than it printed: after the closing quote come the zero padding of the string's memory and, now and then, the 16-byte header of the string object next to it (a type pointer, hash, length and data pointer), whose first byte is 0x04 and ends the reply early. Reading a 2.8 MB model failed that way on every run ("source code string cannot contain null bytes"; the instrumented runs failed 47 KB and 750 KB in, and one printed loop without the model failed the same way), with the board running nothing else. The bytes inside the quotes were right each time. Serial transports now read a file with an explicit seek per chunk, parse only the first line, check the length, and read the chunk again (after a clean return to the raw REPL prompt) when it is wrong. --- cli/src/mpftp/rawpaste.py | 130 ++++++++++++++++++++++++ cli/tests/test_read_file_checked.py | 152 ++++++++++++++++++++++++++++ docs/agent-guide.md | 1 + 3 files changed, 283 insertions(+) create mode 100644 cli/tests/test_read_file_checked.py diff --git a/cli/src/mpftp/rawpaste.py b/cli/src/mpftp/rawpaste.py index 535a1ac..d9ef917 100644 --- a/cli/src/mpftp/rawpaste.py +++ b/cli/src/mpftp/rawpaste.py @@ -79,6 +79,133 @@ def paced_raw_paste_write(transport: Any, command_bytes: bytes) -> None: raise TransportError("could not complete raw paste: {!r}".format(data)) +def _repr_line(out: bytes) -> bytes: + """The printed ``repr`` in a board's reply: its first line, with the zeros + some boards append to it taken off. A ``repr`` never holds a raw zero, CR + or LF, so whatever follows its closing quote is not content.""" + line = out.split(b"\r", 1)[0].split(b"\n", 1)[0] + return line.rstrip(b"\x00 ") + + +def read_file_checked( + transport: Any, + src: str, + chunk_size: int = 256, + progress_callback: Optional[Callable[[int, int], None]] = None, + attempts: int = 8, +) -> bytearray: + """mpremote's ``fs_readfile``, except that a chunk that arrives wrong is + asked for again. + + mpremote reads a file 256 bytes at a time as ``print(repr(r(256)))`` and + parses the line it gets back. The P4 DEV-KIT's native USB sometimes sends + more than it was given: after the closing quote come the zero padding of + the string's memory and, now and then, the 16-byte header of the string + object next to it, a byte of which can be the ``\\x04`` that ends the + reply. A 2.8 MB model failed that way twice in two attempts ("source code + string cannot contain null bytes"), and a third run lost the framing too. + The bytes of the file inside the quotes were right every time, so each + chunk is read with an explicit ``seek``, parsed from the first line only, + and checked against the length it should have; a chunk that fails either + is read again, up to ``attempts`` times. + """ + import ast + + size = None + for _ in range(attempts): + try: + size = transport.fs_stat(src).st_size + break + except OSError: + raise + except Exception as e: # noqa: BLE001 - a stat reply with noise in it + if _is_board_error(e): + raise _convert_fs_error(transport, e, src) from None + _resync(transport) + if size is None: + raise OSError("could not stat %s" % src) + contents = bytearray() + try: + transport.exec("f=open('%s','rb')\nr=f.read\ns=f.seek" % src) + except Exception as e: # noqa: BLE001 - same conversion mpremote does + raise _convert_fs_error(transport, e, src) from None + try: + while len(contents) < size: + want = min(chunk_size, size - len(contents)) + chunk = None + last: Optional[BaseException] = None + for _ in range(attempts): + try: + out = transport.exec( + "s(%d)\nprint(repr(r(%d)))" % (len(contents), want) + ) + value = ast.literal_eval(_repr_line(out).decode("ascii")) + except (ValueError, SyntaxError, UnicodeDecodeError, MemoryError) as e: + last = e + _resync(transport) + continue + except Exception as e: # noqa: BLE001 + # A TransportExecError whose "error" is the spurious bytes + # (its text is decoded from them) or a framing timeout + # that the next command's wait for the prompt recovers. + if _is_board_error(e): + raise _convert_fs_error(transport, e, src) from None + last = e + _resync(transport) + continue + if isinstance(value, (bytes, bytearray)) and len(value) == want: + chunk = bytes(value) + break + last = ValueError( + "read %d bytes where %d were asked for" + % (len(value) if hasattr(value, "__len__") else -1, want) + ) + _resync(transport) + if chunk is None: + raise OSError( + "reading %s at byte %d failed %d times: %s" + % (src, len(contents), attempts, last) + ) + contents.extend(chunk) + if progress_callback: + progress_callback(len(contents), size) + finally: + try: + transport.exec("f.close()") + except Exception: # noqa: BLE001 - the read's own error is the one to show + pass + return contents + + +def _resync(transport: Any) -> None: + """Back to a clean raw-REPL prompt after a reply that did not parse. The + spurious bytes can include a ``\\x04``, which ends the reply early and leaves + the real end marker and prompt unread. Entering raw REPL again (no soft + reset, so the open file stays open) throws all of that away.""" + enter = getattr(transport, "enter_raw_repl", None) + if enter is not None: + try: + enter(soft_reset=False) + except Exception: # noqa: BLE001 - the retry will say if it did not work + pass + + +def _is_board_error(exc: BaseException) -> bool: + """True for an error the board reported about the file (a Python + traceback), not for noise in the reply.""" + text = str(getattr(exc, "error_output", "") or "") + return "Traceback" in text + + +def _convert_fs_error(transport: Any, exc: BaseException, src: str) -> BaseException: + try: + from mpremote.transport import _convert_filesystem_error + + return _convert_filesystem_error(exc, src) or exc + except Exception: # noqa: BLE001 + return exc + + def _transport_class() -> Any: global _TRANSPORT_CLASS if _TRANSPORT_CLASS is not None: @@ -155,6 +282,9 @@ def raw_paste_write(self, command_bytes: bytes) -> None: else: super().raw_paste_write(command_bytes) + def fs_readfile(self, src, chunk_size=256, progress_callback=None): + return read_file_checked(self, src, chunk_size, progress_callback) + _TRANSPORT_CLASS = PacedSerialTransport return _TRANSPORT_CLASS diff --git a/cli/tests/test_read_file_checked.py b/cli/tests/test_read_file_checked.py new file mode 100644 index 0000000..97a98a5 --- /dev/null +++ b/cli/tests/test_read_file_checked.py @@ -0,0 +1,152 @@ +"""``get`` survives a board that sends more than it was asked to print. + +The P4 DEV-KIT, on its native USB, sometimes sends bytes after the closing +quote of a printed ``repr``: the zero padding of the string's memory, and now +and then the 16-byte header of the string object beside it. A byte of that +header can be ``\\x04``, which ends the reply early. mpremote parsed the line +with ``ast.literal_eval`` and failed on the zeros ("source code string cannot +contain null bytes"), once in a few hundred thousand bytes. These tests give +``read_file_checked`` a stand-in board that does the same and check that the +file comes out byte for byte. +""" + +from __future__ import annotations + +import contextlib +import io +import random +import tempfile +import unittest +from pathlib import Path +from types import SimpleNamespace + +from mpftp import rawpaste + +try: + from mpremote.transport import TransportExecError + + HAVE_MPREMOTE = True +except ImportError: + HAVE_MPREMOTE = False + + class TransportExecError(Exception): # type: ignore[no-redef] + def __init__(self, status_code, error_output): + self.status_code = status_code + self.error_output = error_output + super().__init__(error_output) + + +def blob() -> bytes: + rng = random.Random(7) + return bytes(range(256)) + bytes(700) + bytes(rng.randrange(256) for _ in range(4000)) + b"\x04\x00" + + +class FaultyBoard: + """Runs the commands under CPython; ``fault(n, out)`` may change reply n.""" + + def __init__(self, path: str, fault=None) -> None: + self.path = path + self.ns: dict = {} + self.fault = fault + self.calls = 0 + self.resyncs = 0 + + def fs_stat(self, src): + return SimpleNamespace(st_size=Path(self.path).stat().st_size) + + def enter_raw_repl(self, soft_reset=True): + assert soft_reset is False, "a soft reset would close the open file" + self.resyncs += 1 + + def exec(self, command, data_consumer=None): + command = command.replace(repr(self.path)[1:-1], self.path) + buf = io.StringIO() + with contextlib.redirect_stdout(buf): + exec(compile(command, "", "exec"), self.ns) + out = buf.getvalue().replace("\n", "\r\n").encode() + if "print(repr(r(" in command: + self.calls += 1 + if self.fault is not None: + return self.fault(self.calls, out) + return out + + +@unittest.skipUnless(HAVE_MPREMOTE, "mpremote not installed") +class ReadFileCheckedTests(unittest.TestCase): + def setUp(self) -> None: + self.tmp = tempfile.TemporaryDirectory() + self.addCleanup(self.tmp.cleanup) + self.path = str(Path(self.tmp.name) / "net.espdl") + self.data = blob() + Path(self.path).write_bytes(self.data) + + def read(self, fault=None, **kw): + board = FaultyBoard(self.path, fault) + return board, rawpaste.read_file_checked(board, self.path, **kw) + + def test_a_clean_board_gives_the_file(self): + board, got = self.read() + self.assertEqual(bytes(got), self.data) + self.assertEqual(board.resyncs, 0) + + def test_zeros_after_the_closing_quote(self): + # What the board sent: the repr, 16 zeros, then the newline. + def fault(n, out): + return out[:-2] + bytes(16) + b"\r\n" if n % 5 == 0 else out + + board, got = self.read(fault) + self.assertEqual(bytes(got), self.data) + + def test_zeros_and_the_next_objects_header_with_its_04(self): + # type pointer 0x406b3704, hash, length, data pointer: the stream then + # carries \x04 early, mpremote ends the reply there and reads the rest + # as the error text. + header = b"\x047k@=|\x00\x00\xc4\x03\x00\x00\xe0\x93\x00H" + + def fault(n, out): + if n == 3: + raise TransportExecError(out[:-2] + bytes(12), header[1:].decode("latin-1") + "\r\n") + return out + + board, got = self.read(fault) + self.assertEqual(bytes(got), self.data) + self.assertEqual(board.resyncs, 1) + + def test_a_reply_cut_short_is_read_again(self): + def fault(n, out): + return out[:40] + b"'\r\n" if n == 4 else out + + board, got = self.read(fault) + self.assertEqual(bytes(got), self.data) + + def test_a_reply_that_never_parses_gives_up_and_says_where(self): + def fault(n, out): + return b"b'\\x00" + bytes(8) if n >= 4 else out + + with self.assertRaises(OSError) as cm: + self.read(fault, attempts=3) + self.assertIn("at byte 768", str(cm.exception)) + + def test_a_board_error_is_not_retried(self): + board = FaultyBoard(self.path) + Path(self.path).unlink() + board.fs_stat = lambda src: SimpleNamespace(st_size=10) + with self.assertRaises(Exception) as cm: + rawpaste.read_file_checked(board, self.path) + self.assertEqual(board.calls, 0) + self.assertIsInstance(cm.exception, (OSError, FileNotFoundError)) + + def test_the_file_is_not_a_multiple_of_the_chunk(self): + Path(self.path).write_bytes(self.data[:1000]) + _, got = self.read() + self.assertEqual(bytes(got), self.data[:1000]) + Path(self.path).write_bytes(b"") + _, got = self.read() + self.assertEqual(bytes(got), b"") + + def test_repr_line_drops_the_padding_only(self): + self.assertEqual(rawpaste._repr_line(b"b'a\\x00'\x00\x00\x00\r\n"), b"b'a\\x00'") + + +if __name__ == "__main__": + unittest.main() diff --git a/docs/agent-guide.md b/docs/agent-guide.md index 912e75b..bd60dce 100644 --- a/docs/agent-guide.md +++ b/docs/agent-guide.md @@ -956,6 +956,7 @@ where it hung. | Two editors on the *same* workspace folder — CLI reaches an unexpected session | One-entry-per-path registry; most recent connect wins — was tracked in [PyDevices/mpftp#21](https://github.com/PyDevices/mpftp/issues/21), fixed: no longer silent. `mpftp status` → check `editor`/`pid`/`updatedAt` on the resolved entry | | `Access is denied` / `transport_dead` after hung `exec`/`run` | Sidecar releases the COM handle automatically (was tracked in [PyDevices/mpftp#3](https://github.com/PyDevices/mpftp/issues/3), fixed via a bounded serial write-timeout); `disconnect` then `resume`/`connect`. If still busy: reload extension window, then replug USB only as last resort | | `busy or locked` on a board's native USB port (`303A`, `239A`, `2E8A`) with no tool holding it | The board's program is stuck in a callback that never returns, so its USB stops answering and Windows refuses the open ([mpftp#79](https://github.com/PyDevices/mpftp/issues/79)). A killed mpftp command lets go of the port within about 10 s; past that, reset the board through its UART port (`hard-reset -d `) or its RESET button | +| `get` failed with "source code string cannot contain null bytes" (older mpftp) | The board's native USB sent a few extra bytes after a printed chunk: zero padding, sometimes the next object's header, which can hold a `\x04` that ends the reply early. The file's own bytes were right. A serial `get`/`cp` now reads each chunk with an explicit `seek`, checks its length and asks again when it is wrong. Seen on the P4 DEV-KIT (ESP32-P4, TinyUSB CDC); a failed chunk that never comes right fails with the byte offset | | `timeout waiting for first EOF` | Board still running (UI loop). Use `run` without `--follow` / `exec --no-follow`, then `interrupt` or `soft-reset` | | Soft-reset left UI dead after deploy | Expected: soft-reset skips `main.py`. Use `soft-reboot` or `hard-reset` to run startup | | Dual USB (UART + native CDC) | `mpftp ports` shows `role` (`repl` vs `cdc_debug`); control on UART, `monitor`/`debug-tee` on CDC |