get: ask again for a chunk that arrives with extra bytes on it

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
 

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, "<board>", "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()

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 <UART>`) 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 |