From 4c820bdf79de879bf5f29f8a19e7516ee5274f49 Mon Sep 17 00:00:00 2001 From: MCKero Date: Fri, 28 Aug 2026 04:36:30 +0100 Subject: [PATCH] Add a frame grabber that reads the LCD over QMP memsave memsave, not pmemsave. The plan specified pmemsave on the strength of a timing measurement that never checked the contents; it turns out pmemsave takes a *physical* address and silently returns zeros for gFrameBuffer's virtual address. No error, no warning -- just a permanently blank screen. Caught by rendering a real frame and finding 0 lit pixels where the gdb path reported 1693. With memsave the count matches exactly, and the image reads correctly: both VFOs at 18.00000 MHz, PS/DWR/CL status bar. Two tests guard the decisions rather than the current text: the stub client raises if pmemsave is ever used, and a source check rejects subprocess/popen so frame reads cannot regress onto gdb, which would halt the guest. Measured 1.35 ms per frame with the guest still reporting status running. --- tools/test_uvk5_lcd.py | 95 +++++++++++++++++++++++++++++++++++++++++- tools/uvk5_lcd.py | 42 +++++++++++++++++++ 2 files changed, 136 insertions(+), 1 deletion(-) diff --git a/tools/test_uvk5_lcd.py b/tools/test_uvk5_lcd.py index 931b246..326e1c4 100644 --- a/tools/test_uvk5_lcd.py +++ b/tools/test_uvk5_lcd.py @@ -1,10 +1,13 @@ #!/usr/bin/env python3 """Unit tests for LCD unpacking and PNG encoding. No emulator needed.""" +import os import struct +import tempfile import unittest import zlib -from uvk5_lcd import LCD_HEIGHT, LCD_WIDTH, encode_png, unpack +from uvk5_lcd import (FRAME_BYTES, LCD_HEIGHT, LCD_WIDTH, STATUS_BYTES, + FrameGrabber, encode_png, unpack) class TestUnpack(unittest.TestCase): @@ -78,5 +81,95 @@ class TestEncodePng(unittest.TestCase): self.assertEqual(raw[2], 0xFF) # neighbour -> white +class StubClient: + """Stands in for QmpClient; writes the files memsave would write. + + Rejects pmemsave on purpose. pmemsave takes a *physical* address and returns + zeros for the framebuffer's virtual address -- a blank screen with no error + reported anywhere, which is exactly the bug this stub is here to catch. + """ + + def __init__(self): + self.calls = [] + + def command(self, name, **args): + self.calls.append((name, args)) + if name == "pmemsave": + raise AssertionError( + "pmemsave reads physical addresses and silently returns zeros " + "for gFrameBuffer; use memsave") + if name != "memsave": + raise AssertionError(f"unexpected command {name}") + with open(args["filename"], "wb") as fh: + fh.write(bytes(args["size"])) + return {} + + +class TestFrameGrabber(unittest.TestCase): + def test_uses_memsave_and_returns_png(self): + client = StubClient() + tmp = tempfile.mkdtemp() + grabber = FrameGrabber(client, frame_addr=0x200013DC, + status_addr=0x2000175C, spool_dir=tmp) + png = grabber.png(scale=2) + + self.assertTrue(png.startswith(b"\x89PNG\r\n\x1a\n")) + self.assertEqual([c[0] for c in client.calls], ["memsave", "memsave"]) + + def test_reads_the_right_addresses_and_sizes(self): + client = StubClient() + grabber = FrameGrabber(client, frame_addr=0x200013DC, + status_addr=0x2000175C, + spool_dir=tempfile.mkdtemp()) + grabber.raw() + + by_addr = {args["val"]: args["size"] for _, args in client.calls} + self.assertEqual(by_addr[0x200013DC], FRAME_BYTES) + self.assertEqual(by_addr[0x2000175C], STATUS_BYTES) + + def test_raw_returns_status_then_frame(self): + client = StubClient() + grabber = FrameGrabber(client, frame_addr=0x1000, status_addr=0x2000, + spool_dir=tempfile.mkdtemp()) + status, frame = grabber.raw() + self.assertEqual(len(status), STATUS_BYTES) + self.assertEqual(len(frame), FRAME_BYTES) + + def test_never_uses_pmemsave(self): + """Regression guard: pmemsave silently reads the wrong memory. + + The framebuffer symbols are CPU virtual addresses. pmemsave treats its + argument as physical and returns zeros, so the screen renders blank with + no error raised. Verified against a live emulator: pmemsave gave 0 lit + bits, memsave gave 1693, matching the gdb path exactly. + """ + source = open(os.path.join(os.path.dirname(os.path.abspath(__file__)), + "uvk5_lcd.py")).read() + code = "\n".join(line for line in source.splitlines() + if not line.lstrip().startswith("#") + and not line.lstrip().startswith("*")) + # Allow the word inside the explanatory docstring, forbid a real call. + self.assertNotIn('command("pmemsave"', code) + + def test_does_not_invoke_gdb(self): + """Guard the design decision, not just the current behaviour. + + A gdb attach halts the guest, which would both stutter a live stream and + perturb key debounce timing (see AGENTS.md). pmemsave leaves the guest + running -- measured ~1.3 ms per frame with status still "running". + + Checks for the machinery needed to shell out, not for the word "gdb": + the module mentions gdb in prose precisely to explain why it is avoided. + """ + source = open(os.path.join(os.path.dirname(os.path.abspath(__file__)), + "uvk5_lcd.py")).read() + code = "\n".join(line for line in source.splitlines() + if not line.lstrip().startswith("#")) + for forbidden in ("subprocess", "os.system", "popen", "gdb-multiarch"): + self.assertNotIn(forbidden, code.lower(), + f"{forbidden} must not appear: reading frames has " + "to stay on the QMP path") + + if __name__ == "__main__": unittest.main() diff --git a/tools/uvk5_lcd.py b/tools/uvk5_lcd.py index ca2a424..df26e53 100644 --- a/tools/uvk5_lcd.py +++ b/tools/uvk5_lcd.py @@ -6,6 +6,7 @@ The firmware keeps the display in gStatusLine (page 0) and gFrameBuffer the layout the ST7565 expects. Extracted from tools/screenshot.py so the web UI and the CLI screenshotter cannot drift apart. """ +import os import struct import zlib @@ -56,3 +57,44 @@ def encode_png(pixels, scale: int = 4) -> bytes: + chunk(b"IHDR", struct.pack(">IIBBBBB", width, height, 8, 0, 0, 0, 0)) + chunk(b"IDAT", zlib.compress(bytes(raw), 6)) + chunk(b"IEND", b"")) + + +class FrameGrabber: + """Reads the LCD out of guest memory over QMP. + + memsave, not pmemsave. The framebuffer symbols are CPU virtual addresses; + pmemsave interprets its argument as a *physical* address and silently returns + a block of zeros for these, which renders as a blank screen with no error + anywhere. memsave takes the virtual address and returns the real contents -- + verified against the gdb path, both reporting 1693 lit bits on the same frame. + + QMP, not gdb: measured ~1.35 ms per frame with the guest still reporting + status "running". The gdb path used by screenshot.py halts the guest on every + attach, which is unusable for a live stream and also perturbs key debounce + timing (see AGENTS.md). Do not reintroduce gdb here. + """ + + def __init__(self, client, frame_addr: int, status_addr: int, + spool_dir: str = "/dev/shm"): + self._client = client + self._frame_addr = frame_addr + self._status_addr = status_addr + # pmemsave writes to a path, so a tmpfs avoids disk I/O every frame. + self._frame_path = os.path.join(spool_dir, "uvk5-frame.bin") + self._status_path = os.path.join(spool_dir, "uvk5-status.bin") + + def raw(self) -> tuple[bytes, bytes]: + """Return (status, frame) exactly as the firmware holds them.""" + self._client.command("memsave", val=self._frame_addr, + size=FRAME_BYTES, filename=self._frame_path) + self._client.command("memsave", val=self._status_addr, + size=STATUS_BYTES, filename=self._status_path) + with open(self._frame_path, "rb") as fh: + frame = fh.read(FRAME_BYTES) + with open(self._status_path, "rb") as fh: + status = fh.read(STATUS_BYTES) + return status, frame + + def png(self, scale: int = 4) -> bytes: + status, frame = self.raw() + return encode_png(unpack(status, frame), scale)