From de388249f58ac356a32354172bc378d4af3da453 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Thu, 9 Jul 2026 22:27:25 -0400 Subject: [PATCH 1/6] v2 SPI tweaks --- docs/spi_lcd_timing_pi3.md | 213 ++++++++++++++++++ ...pi_lcd_timing.md => spi_lcd_timing_pi5.md} | 85 ++----- emulator/hardware_base.py | 2 +- pistomp/lcd320x240.py | 12 +- pistomp/pistompcore.py | 4 +- pistomp/pistomptre.py | 6 +- tests/test_spi_timing.py | 96 ++++++++ uilib/lcd_ili9341.py | 26 ++- uilib/spi_timing.py | 54 ++++- 9 files changed, 415 insertions(+), 83 deletions(-) create mode 100644 docs/spi_lcd_timing_pi3.md rename docs/{spi_lcd_timing.md => spi_lcd_timing_pi5.md} (81%) create mode 100644 tests/test_spi_timing.py diff --git a/docs/spi_lcd_timing_pi3.md b/docs/spi_lcd_timing_pi3.md new file mode 100644 index 000000000..3ae0da923 --- /dev/null +++ b/docs/spi_lcd_timing_pi3.md @@ -0,0 +1,213 @@ +# SPI LCD Timing — Pi 3A+ / BCM2837 (v2 hardware) + +Pushing a 320×240 16-bit frame to the ILI9341 over SPI on the v2 pi-stomp +(Raspberry Pi 3A+, BCM2837B0). + +For the v3 hardware (Pi 5 / RP1), see [`spi_lcd_timing_pi5.md`](spi_lcd_timing_pi5.md). +That doc also covers the device-agnostic options for getting the blocking +`write()` off the poll loop (background thread, `panel-mipi-dbi-spi`). + +All numbers here are measured on-device (`6.18.36-rpi-v8-rt`, `core_freq=400`, +`spidev.bufsiz=163840`) by timing the pixel-payload `os.write()` inside +`LcdIli9341._block_fast` and deriving the clock from bits/second. + +`uilib/spi_timing.py` models this: `actual_spi_hz()` applies the divisor rule for +the running host (detected from `/proc/device-tree/compatible`), and +`transfer_ms()` estimates push cost from the *actual* clock, never the requested +one. + +--- + +## Hardware architecture + +The SPI peripheral is **on-die** — there is no PCIe hop, and therefore none of +the Pi 5's PCIe DMA stall. The Linux driver is `spi-bcm2835`, not `spi_dw_mmio`. + +``` +BCM2837B0 +┌───────────────────────────────────┐ +│ 4× Cortex-A53 @ 1.2–1.4 GHz │ +│ 512 MB SDRAM │ +│ bcm2835-dma ──► reads SDRAM │ +│ ↓ (no PCIe) │ +│ spi0 (spi-bcm2835) ◄─ VPU 400MHz │ +│ ├─ ILI9341 LCD (CE0) │ +│ └─ MCP3008 ADC (CE1) │ +└───────────────────────────────────┘ +``` + +**Device nodes**: `/dev/spidev0.0` (LCD, CE0), `/dev/spidev0.1` (ADC, CE1). +The ADC is opened separately by `hardware.init_spi()` (`spidev.SpiDev().open(0, 1)`, +1 MHz), so it has its own fd and cannot clobber the LCD's clock. + +--- + +## The clock divisor rule + +`spi-bcm2835.c` computes: + +```c +cdiv = DIV_ROUND_UP(clk_hz, spi_hz); /* clk_hz = 400 MHz */ +cdiv += cdiv % 2; /* round *up to even*, not to a power of 2 */ +``` + +**CDIV is rounded up to an even number, not to a power of two.** The +power-of-2 rule is legacy folklore — it appears in the BCM2835 datasheet and in +the pre-2015 downstream driver — and it is wrong for mainline. Verified +empirically across 20 requested speeds: every measured clock lands on +`400 / even`, including 40 MHz (CDIV=10), 28.57 MHz (CDIV=14) and 66.67 MHz +(CDIV=6), none of which a power-of-2 rule can produce. + +Because the divisor is rounded *up*, the actual clock is always ≤ the requested +clock. Requesting one hertz below an exact divisor point costs a full step: + +``` +66_666_666 → CDIV=8 → 50.00 MHz +66_666_667 → CDIV=6 → 66.67 MHz +``` + +This is why `Lcd` takes `spi_speed_hz` (an exact integer) rather than +`spi_speed_mhz`. + +The VPU core clock is pinned at 400 MHz. Setting `core_freq` (or `force_turbo`) +moves the SPI clock with it. + +--- + +## Achievable speeds + +Full-frame (320×240 = 76,800 px, 153,600 B) medians, service stopped. +"Total" is `pack + address-window + payload write`; MB/s is payload/wire. + +| Requested | CDIV | **Actual** | Wire | Total | Throughput | Full-frame ceiling | +|-----------|------|-----------|---------|---------|----------|-----| +| 12.5 MHz | 32 | 12.50 MHz | 98.77 ms | 102.87 ms | 1.56 MB/s | 9.7 fps | +| 20 MHz | 20 | 20.00 MHz | 61.90 ms | 65.93 ms | 2.48 MB/s | 15.2 fps | +| 25–28.5 MHz | 16 | 25.00 MHz | 49.63 ms | 53.70 ms | 3.09 MB/s | 18.6 fps | +| 33.3 MHz | 14 | 28.57 MHz | 43.49 ms | 47.63 ms | 3.53 MB/s | 21.0 fps | +| 40–44.4 MHz | 10 | 40.00 MHz | 31.18 ms | 35.53 ms | 4.93 MB/s | 28.1 fps | +| 50–66.66 MHz | 8 | 50.00 MHz | 25.05 ms | 29.23 ms | 6.13 MB/s | 34.2 fps | +| **66,666,667–99.9 MHz** | **6** | **66.67 MHz** | **18.94 ms** | **23.30 ms** | **8.11 MB/s** | **42.9 fps** | +| ≥ 100 MHz | 4 | 100 MHz | 12.3 ms | — | — | **display garbled** | + +**66.67 MHz is the practical maximum.** It sits at the ILI9341's 66.7 MHz +write-cycle limit, and the next step up (CDIV=4) is the known-garbled 100 MHz. +`pistompcore.py` requests `66_666_667`. + +This is the one axis on which **v2 out-runs v3**: the Pi 5's 200 MHz source +cannot produce 66.67 MHz (it needs an odd divisor), so v3 is stuck at 50 MHz +while v2 pushes a full frame 24% faster. + +> **Not electronically verified.** MISO is not wired on v2 — `RDDID` (0x04) +> returns all zeros — so the panel's GRAM cannot be read back to confirm pixel +> integrity at 66.67 MHz. The panel accepts the writes without stalling, but +> correctness at this clock rests on visual inspection. + +--- + +## Measured per-call overhead + +Wire time is `px × 16 bits / clock`; `BITS_PER_PIXEL` fits to **16.00**, i.e. +there is no framing overhead. Everything else is fixed or CPU-bound: + +| Term | Cost | Scales with | +|------|------|-------------| +| Payload `write()` overhead (syscall + DMA setup) | **0.48 ms** | nothing — flat across all 7 clocks | +| Address window (5 small writes + DC/CS toggles) | **0.50 ms** | nothing | +| RGB565 pack, idle | `0.36 ms + 4.23e-5 × px` | pixels (3.61 ms full frame) | +| RGB565 pack, under service load | `0.41 ms + 6.56e-5 × px` | pixels (5.45 ms full frame) | + +The payload-write overhead being constant (0.466–0.505 ms) from 12.5 MHz all the +way to 66.67 MHz is also the proof that `core_freq` is pinned at 400 MHz; a +scaling VPU clock would show up as clock-dependent drift here. + +Fixed cost per push is therefore **~1.0 ms** at the Python boundary, not the +~15 µs of kernel-side register writes. Ten small dirty rects cost 10 ms of pure +overhead before a single pixel moves. + +**`spidev.bufsiz`** matters exactly as on the Pi 5: the 4096 B default means 38 +writes per frame. `spidev.bufsiz=163840` is already set on the kernel cmdline. + +--- + +## Where the time actually goes + +Live profile of the idle main panel (`PISTOMP_PROFILE=1`, 1 s windows). Note +that `profiling.maybe_start()` is only called by the tuner and NAM panels, so +reproducing this requires starting the reporter explicitly. + +| Stage | Mean | % of one core | +|---|---|---| +| `poll_lcd_updates` | 7.23 ms | 33% | +| ├ `lcd.update:_block` (SPI wire) | 1.94 ms | 11% | +| ├ `widget.do_draw` | 1.32 ms | 7% | +| ├ `lcd.update:pack` | 1.26 ms | 7% | +| ├ `panelstack.recompose` | 0.65 ms | 4% | +| └ unaccounted | ~2.0 ms | — | +| `poll_controls` | 0.83 ms | 8% | + +The whole pi-stomp process sits at ~55% of one core, and the main loop achieves +**92 Hz**, not the nominal 100. + +**The SPI transfer is only 27% of an LCD tick.** The push is bandwidth-bound +only for large rects; for the small dirty rects of normal operation it is +compute-bound. Raising the clock from 50 → 66.67 MHz shaves ~0.5 ms off a +7.2 ms idle tick (~7%), but cuts a *full-frame* push from 25.05 ms to 18.94 ms — +so it pays off on panel and pedalboard transitions, not on idle animation. + +`uilib/spi_timing.py`'s constants were fit on a Pi 5 and already predict Pi 3A+ +totals to within ~5% (mean 4.6%). A Pi 3A+ refit gives `FIXED_MS=0.95`, +`PIPELINE_MS_PER_PX=4.81e-5`, `BITS_PER_PIXEL=16.0` (mean error 2.2%) and moves +the 8 ms inline-push gate from 19,244 px to 19,144 px — a 0.5% shift, not worth +per-device constants. + +### Two environmental drags + +- **Thermal soft limit.** `vcgencmd get_throttled` returns `0x80008` (bit 3, + "soft temp limit active") above 60 °C. `arm_freq=1400` and `cpufreq` sysfs + both report 1.4 GHz, but `vcgencmd measure_clock arm` reports **1.2 GHz** — + the firmware cap is invisible to the standard telemetry. +- **Memory pressure.** 463 MB usable, ~86 MB available, with ~150 MB compressed + into zram. Steady-state `si`/`so` are ~0, so it is not thrashing, but fresh + allocations (opening a menu, building a panel, loading a pedalboard) can stall + the main thread on decompression. + +--- + +## Coalescing cuts both ways + +`PanelStack._pending_lcd_clip` merges pending pushes with `Box.union`, which is a +**bounding box**. Because wire time is linear in pixels, merging disjoint rects +can cost far more than pushing them separately: + +| | Separate | Coalesced to bounding box | +|---|---|---| +| 4 × (32×131) rects, disjoint | 9.8 ms | 29.3 ms (full screen) | + +Coalescing wins on per-push overhead (~1.0 ms each) and on genuine overlap; it +loses whenever the union area exceeds the summed areas by more than that. +`PanelStack.INLINE_BUDGET_MS = 8.0` defers anything estimated over 8 ms, which is +a latency guard for the 10 ms tick, not a throughput optimization. + +Dirty-rect **culling** — pushing fewer pixels — remains the primary lever, as on +the Pi 5. + +--- + +## Summary + +| What | Value | +|------|-------| +| SPI driver | `spi-bcm2835` | +| LCD device node | `/dev/spidev0.0` (CE0) | +| ADC device node | `/dev/spidev0.1` (CE1, own fd, 1 MHz) | +| Clock source | VPU core clock = 400 MHz (pinned) | +| Divisor rule | **Round up to even** (`cdiv += cdiv % 2`) | +| Practical max speed | **66.67 MHz** (request ≥ `66_666_667`) | +| Configured in `pistompcore.py` | `spi_speed_hz=66_666_667` | +| Peak payload throughput | **8.11 MB/s** measured (8.33 MB/s theoretical) | +| Full-frame wire time | 25.05 ms @ 50 MHz → **18.94 ms @ 66.67 MHz** | +| PCIe DMA stall | None (on-die DMA) | +| Fixed overhead/push | ~1.0 ms (0.48 payload write + 0.50 address window) | +| SPI share of an idle LCD tick | ~27% (the rest is CPU) | +| Primary optimization lever | Dirty-rect culling | diff --git a/docs/spi_lcd_timing.md b/docs/spi_lcd_timing_pi5.md similarity index 81% rename from docs/spi_lcd_timing.md rename to docs/spi_lcd_timing_pi5.md index ebc016af4..613cc442d 100644 --- a/docs/spi_lcd_timing.md +++ b/docs/spi_lcd_timing_pi5.md @@ -1,9 +1,16 @@ -# SPI LCD Timing Analysis — Pi 5 / RP1 +# SPI LCD Timing — Pi 5 / RP1 (v3 hardware) Investigation of the full kernel path for pushing a 320×240 16-bit frame to the ILI9341 display over SPI on Raspberry Pi 5. All measurements were taken on-device with a PREEMPT_RT kernel (`linux 6.18.33-3-rpi-rt-v8-rt`). +For the v2 hardware (Pi 3A+), see [`spi_lcd_timing_pi3.md`](spi_lcd_timing_pi3.md). +The two boards differ enough — different SPI controller, different source clock, +different achievable speeds — that they no longer share a document. + +`uilib/spi_timing.py` models both: `actual_spi_hz()` applies the divisor rule for +the running host, and `transfer_ms()` estimates push cost from the *actual* clock. + --- ## Hardware architecture @@ -66,6 +73,9 @@ and renders garbage. 66.7 MHz is not achievable: it would require BAUDR=3 (odd), which the hardware forbids. The safe maximum is **50 MHz** (any request in 56–99 MHz). +This is the one axis where the older v2 board wins: its 400 MHz source divides +evenly by 6, so a Pi 3A+ *can* run the panel at 66.67 MHz. See the Pi 3 doc. + --- ## Full write() path — one full-frame push @@ -133,6 +143,8 @@ The ~25–26 ms that `write()` blocks Python is the dominant cost for the poll loop. Wire time is hardware-fixed, but it can be hidden by decoupling the LCD push from the main thread. +This section applies equally to the Pi 3A+. + ### Background thread Wrap `LcdIli9341.update()` in a daemon thread. The poll loop calls a @@ -209,10 +221,11 @@ runs in a DRM worker thread. The stock `linux-rpi` kernel includes it; `linux-rpi-rt` depends on the base ALARM config and should be checked before building. -5. **`INIT_STAMP` / splash handoff**: `LcdIli9341.has_system_splash` reads - `/run/lcd.init` to detect whether the display was already initialised this - boot. With a kernel driver the init sequence runs at module load, not via - lcd-splash; the stamp mechanism would need redesigning. +5. **`INIT_STAMP` / splash handoff**: `has_system_splash()` reads `/run/lcd.init` + to detect whether the display was already initialised this boot; callers pass + `LcdIli9341` a reset pin (or `None`) accordingly. With a kernel driver the + init sequence runs at module load, not via lcd-splash; the stamp mechanism + would need redesigning. **Python-side changes**: Replace `LcdIli9341` with a thin writer that opens `/dev/fb0`, packs RGB565, and calls `write()`. Rotation must be pre-applied @@ -270,68 +283,8 @@ optimization and requires no kernel or boot changes. | ≥ 100 MHz requested → actual | **100 MHz** (BAUDR=2) — **display garbled** (ILI9341 max 66.7 MHz) | | 66.7 MHz achievable? | **No** — requires BAUDR=3 (odd, forbidden) | | Safe maximum setting | Any value 56–99 MHz (all give 50 MHz actual) | +| Configured in `pistomptre.py` | `spi_speed_hz=50_000_000` | | Full-frame time @ 50 MHz | ~25 ms (24.6 ms wire + ~0 PCIe stall + ~0.4 ms overhead) | | DMA/IRQ crossover | 128 bytes (FIFO depth × 2-byte words) | | Optimal `spidev.bufsiz` | ≥153,600 (set to 163,840 in `pistomp-arch/files/cmdline.txt`) | | Primary optimization lever | Dirty-rect culling (linear in pixel count) | - ---- - -## Appendix: Pi 3A+ (v2 hardware) - -The v2 pi-stomp uses a Raspberry Pi 3A+ (BCM2837B0). The SPI peripheral is -**on-die** — there is no PCIe hop. The Linux driver is `spi-bcm2835`, not -`spi_dw_mmio`. - -### Clock source and divisor rule - -| | Pi 5 (RP1) | Pi 3A+ (BCM2837B0) | -|---|---|---| -| SPI clock source | `RP1_CLK_SYS` = 200 MHz | VPU core clock = 400 MHz | -| Divisor constraint | Even only (`ALIGN(…, 2)`) | **Power of 2 only** (`roundup_pow_of_two()`) | - -`spi-bcm2835.c` computes `cdiv = roundup_pow_of_two(DIV_ROUND_UP(400_000_000, speed_hz))`. -At 400 MHz VPU the BCM2835 SPI CDIV register only works reliably with power-of-2 -divisors (confirmed hardware constraint, raspberrypi/linux #2286). Setting -`core_freq=250` in `config.txt` restores arbitrary even divisors but slows the -SDRAM interface and is not recommended. - -### Achievable speeds near the ILI9341 limit - -| Requested | CDIV | **Actual speed** | Full-frame wire time | -|-----------|------|------------------|----------------------| -| 51–99 MHz | 8 | **50 MHz** | 24.6 ms | -| ≥ 100 MHz | 4 | **100 MHz** | 12.3 ms — **display garbled** | -| ≤ 50 MHz | 8–16 | 50 or 25 MHz | 24.6–49.2 ms | - -`DIV_ROUND_UP(400, 99) = 5` → `roundup_pow_of_two(5) = 8` → 50 MHz. -`DIV_ROUND_UP(400, 100) = 4` → already pow2 → 100 MHz (garbled, same as Pi 5). - -66.7 MHz is not achievable: it requires CDIV=6 (not a power of 2). -**Safe maximum is 50 MHz** (any request 51–99 MHz), identical to Pi 5. - -### Differences from Pi 5 - -**No PCIe stall.** BCM2835 DMA accesses SDRAM directly. The 31% overhead -observed at 100 MHz on Pi 5 does not apply. At 50 MHz this was near-zero on -Pi 5 too, so wire time is identical. - -**Lower fixed overhead per transfer.** CS/BAUDR/DMA-kick register writes hit -on-die MMIO directly (~1 ns) rather than over PCIe (~200 ns per write). -Fixed overhead per frame is ~10–15 µs vs ~100 µs on Pi 5. - -**`spidev.bufsiz` matters identically.** Default 4096 B → 38 writes per -frame → ~1.5 ms wasted overhead. Same `spidev.bufsiz=163840` fix applies. - -### Summary (Pi 3A+) - -| What | Value | -|------|-------| -| SPI driver | `spi-bcm2835` | -| Clock source | VPU core clock = 400 MHz | -| Divisor rule | Power of 2 only | -| Safe max speed | **50 MHz** | -| Full-frame wire time | **~24.6 ms** (identical to Pi 5) | -| PCIe DMA stall | None (on-die DMA) | -| Fixed overhead/frame | ~15 µs | -| Primary optimization lever | Dirty-rect culling (same as Pi 5) | diff --git a/emulator/hardware_base.py b/emulator/hardware_base.py index 2a9884964..a23485705 100644 --- a/emulator/hardware_base.py +++ b/emulator/hardware_base.py @@ -54,7 +54,7 @@ def init_lcd(self): self.lcd_pygame = LcdPygame(320, 240, spi_hz=50_000_000) self.handler.add_lcd( - Lcd.Lcd(self.handler.homedir, self.handler, flip=self.lcd_flip, display=self.lcd_pygame, spi_speed_mhz=50) + Lcd.Lcd(self.handler.homedir, self.handler, flip=self.lcd_flip, display=self.lcd_pygame, spi_speed_hz=50_000_000) ) def init_footswitches(self): diff --git a/pistomp/lcd320x240.py b/pistomp/lcd320x240.py index 50ff387b2..19d085e43 100644 --- a/pistomp/lcd320x240.py +++ b/pistomp/lcd320x240.py @@ -113,29 +113,31 @@ def set_description(self, text: str) -> None: class Lcd: CAPTURE_SOCKET_PATH = "/tmp/pistomp-lcd.sock" - def __init__(self, cwd, handler: "Modhandler", flip=False, display=None, spi_speed_mhz=50): + def __init__(self, cwd, handler: "Modhandler", flip=False, display=None, + spi_speed_hz=50_000_000, reset=True): self.cwd = cwd self.imagedir = os.path.join(cwd, "images") Config(os.path.join(cwd, 'ui', 'config.json')) self.handler: "Modhandler" = handler self.flip = flip - self.spi_speed_mhz = spi_speed_mhz + self.spi_speed_hz = spi_speed_hz self._capture_socket = None self._capture_check_tick = 0 # UI poll cadence scales with SPI speed: slower clock → more ticks per frame push. - self.poll_divisor = 8 if spi_speed_mhz <= 24 else 2 + self.poll_divisor = 8 if spi_speed_hz <= 24_000_000 else 2 # TODO would be good to decouple the actual LCD hardware. This file should work for any 320x240 display if display is None: import board import digitalio + reset_pin = digitalio.DigitalInOut(board.D5) if reset else None display = LcdIli9341(board.SPI(), digitalio.DigitalInOut(board.CE0), digitalio.DigitalInOut(board.D6), - digitalio.DigitalInOut(board.D5), - spi_speed_mhz * 1_000_000, + reset_pin, + spi_speed_hz, flip) # Colors diff --git a/pistomp/pistompcore.py b/pistomp/pistompcore.py index 7221ae2b6..a919413ed 100755 --- a/pistomp/pistompcore.py +++ b/pistomp/pistompcore.py @@ -68,7 +68,9 @@ def __init__(self, cfg, mod, midiout, refresh_callback): # self.reinit(None) def init_lcd(self): - self.mod.add_lcd(Lcd.Lcd(self.mod.homedir, self.mod, flip=True, spi_speed_mhz=50)) + # 66_666_667 = lowest request selecting CDIV=6; one Hz less gives 50 MHz. + self.mod.add_lcd(Lcd.Lcd(self.mod.homedir, self.mod, flip=True, + spi_speed_hz=66_666_667, reset=True)) def init_encoders(self): top_enc = EncoderController.EncoderController( diff --git a/pistomp/pistomptre.py b/pistomp/pistomptre.py index fee1318cf..abf6d5bd4 100755 --- a/pistomp/pistomptre.py +++ b/pistomp/pistomptre.py @@ -23,6 +23,7 @@ import pistomp.ledstrip as Ledstrip import pistomp.lcd320x240 as Lcd +from uilib.lcd_ili9341 import has_system_splash # This subclass defines hardware specific to pi-Stomp Tre # 4 Encoders (one for Navigation, 3 for tweaking) @@ -83,7 +84,10 @@ def __init__(self, cfg, handler, midiout, refresh_callback): #self.reinit(None) # TODO do we still need this? Maybe after pb load? mappings? def init_lcd(self): - self.handler.add_lcd(Lcd.Lcd(self.handler.homedir, self.handler, flip=False, spi_speed_mhz=50)) + # RP1's 200 MHz source can't reach 66.67 MHz (odd divisor); 50 is the ceiling. + self.handler.add_lcd(Lcd.Lcd(self.handler.homedir, self.handler, flip=False, + spi_speed_hz=50_000_000, + reset=not has_system_splash())) def add_encoder(self, id, type, callback, longpress_callback, midi_channel, midi_cc): enc_pins = Util.DICT_GET(ENC, id) diff --git a/tests/test_spi_timing.py b/tests/test_spi_timing.py new file mode 100644 index 000000000..3b8a92165 --- /dev/null +++ b/tests/test_spi_timing.py @@ -0,0 +1,96 @@ +import pytest + +from uilib.spi_timing import DEFAULT_SOURCE_HZ, actual_spi_hz, spi_source_hz, transfer_ms + +PI3 = 400_000_000 # v2 — BCM2837 VPU core clock +PI5 = 200_000_000 # v3 — RP1 CLK_SYS + + +@pytest.mark.parametrize( + "requested, expected_hz", + [ + # Measured on a Pi 3A+ (docs/spi_lcd_timing_pi3.md): every achievable + # clock is 400 MHz / even divisor. + (12_500_000, 12_500_000), # cdiv 32 + (20_000_000, 20_000_000), # cdiv 20 + (25_000_000, 25_000_000), # cdiv 16 + (28_571_428, 25_000_000), # cdiv 15 -> 16 + (33_333_333, 400_000_000 / 14), # cdiv 13 -> 14 + (40_000_000, 40_000_000), # cdiv 10 + (44_444_444, 40_000_000), # cdiv 9 -> 10 + (50_000_000, 50_000_000), # cdiv 8 (production) + (57_142_857, 50_000_000), # cdiv 7 -> 8 + (80_000_000, 400_000_000 / 6), # cdiv 5 -> 6 + (99_000_000, 400_000_000 / 6), # cdiv 5 -> 6 + (100_000_000, 100_000_000), # cdiv 4 — garbles the ILI9341 + ], +) +def test_pi3_divisor_rule(requested, expected_hz): + assert actual_spi_hz(requested, PI3) == pytest.approx(expected_hz) + + +def test_pi3_one_hertz_below_a_divisor_point_costs_a_full_step(): + # 400e6 / 6 == 66_666_666.67, so 66_666_666 rounds the divisor to 7 -> 8. + assert actual_spi_hz(66_666_666, PI3) == pytest.approx(50_000_000) + assert actual_spi_hz(66_666_667, PI3) == pytest.approx(400_000_000 / 6) + + +@pytest.mark.parametrize( + "requested, expected_hz", + [ + (81_000_000, 50_000_000), # BAUDR 3 -> 4 + (50_000_000, 50_000_000), # BAUDR 4 + (99_000_000, 50_000_000), # BAUDR 3 -> 4 + (100_000_000, 100_000_000), # BAUDR 2 + ], +) +def test_pi5_divisor_rule(requested, expected_hz): + assert actual_spi_hz(requested, PI5) == pytest.approx(expected_hz) + + +def test_pi5_cannot_reach_66mhz_but_pi3_can(): + # 200/6 needs BAUDR 3 (odd); 400/6 is even, so v2 out-runs v3 here. + assert actual_spi_hz(70_000_000, PI5) == pytest.approx(50_000_000) + assert actual_spi_hz(70_000_000, PI3) == pytest.approx(400_000_000 / 6) + + +def test_actual_never_exceeds_request(): + for source in (PI3, PI5): + for requested in range(1_000_000, 100_000_000, 997_000): + assert actual_spi_hz(requested, source) <= requested + + +def test_divisor_floors_at_two(): + assert actual_spi_hz(10_000_000_000, PI3) == pytest.approx(PI3 / 2) + + +def test_rejects_nonpositive(): + with pytest.raises(ValueError): + actual_spi_hz(0, PI3) + + +def test_source_from_device_tree(monkeypatch, tmp_path): + dt = tmp_path / "compatible" + dt.write_bytes(b"raspberrypi,3-model-b-plus\0brcm,bcm2837\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + assert spi_source_hz() == PI3 + + +def test_source_defaults_when_device_tree_absent(monkeypatch, tmp_path): + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", tmp_path / "nope") + assert spi_source_hz() == DEFAULT_SOURCE_HZ + + +def test_source_defaults_on_unknown_soc(monkeypatch, tmp_path): + dt = tmp_path / "compatible" + dt.write_bytes(b"acme,widget\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + assert spi_source_hz() == DEFAULT_SOURCE_HZ + + +def test_transfer_ms_scales_with_clock(): + slow = transfer_ms(76800, 50_000_000) + fast = transfer_ms(76800, 400_000_000 / 6) + assert slow > fast + # Wire time dominates a full frame: 66.67 MHz should be meaningfully cheaper. + assert fast / slow == pytest.approx(0.79, abs=0.03) diff --git a/uilib/lcd_ili9341.py b/uilib/lcd_ili9341.py index d3b6029c9..159810f8f 100644 --- a/uilib/lcd_ili9341.py +++ b/uilib/lcd_ili9341.py @@ -17,6 +17,7 @@ import numpy as np from uilib.panel import LcdBase, Box +from uilib.spi_timing import actual_spi_hz from uilib.spi_timing import transfer_ms as spi_transfer_ms from uilib import profiling import logging @@ -26,6 +27,11 @@ INIT_STAMP = "/run/lcd.init" +def has_system_splash() -> bool: + """True if lcd-splash already initialised the panel this boot.""" + return os.path.exists(INIT_STAMP) + + try: with open("/sys/module/spidev/parameters/bufsiz", "r") as _f: SPIDEV_BUFSIZ = int(_f.read().strip()) @@ -38,13 +44,19 @@ class LcdIli9341(LcdBase): def __init__(self, spi, cs_pin, dc_pin, reset_pin, baudrate, flip=True): import adafruit_rgb_display.ili9341 as ili9341 - is_v2 = flip - needs_init = is_v2 or not self.has_system_splash - rst = reset_pin if needs_init else None + # reset_pin=None adopts the panel as lcd-splash left it. + needs_reset = reset_pin is not None - self.disp = ili9341.ILI9341(spi, cs=cs_pin, dc=dc_pin, rst=rst, baudrate=baudrate) + self.disp = ili9341.ILI9341(spi, cs=cs_pin, dc=dc_pin, rst=reset_pin, baudrate=baudrate) self.disp._block = self._block_fast - self.baudrate = baudrate + + # transfer_ms gates inline pushes, so it must model the achieved clock. + self.requested_baudrate = baudrate + self.baudrate = actual_spi_hz(baudrate) + if self.baudrate != baudrate: + logging.info( + "SPI %.2f MHz requested, %.2f MHz actual", baudrate / 1e6, self.baudrate / 1e6 + ) self.lock = threading.Lock() @@ -52,7 +64,7 @@ def __init__(self, spi, cs_pin, dc_pin, reset_pin, baudrate, flip=True): # it's pretty clear we need to fork adafruit_rgb_display... # idea: maybe we can query the display's current state and only run init() if it's uninitialized? - if is_v2 or not self.has_system_splash: + if needs_reset: self.clear() # full-panel black while still in Adafruit's portrait MADCTL self._set_stamp() @@ -131,7 +143,7 @@ def _block_fast(self, x0, y0, x1, y1, data=None): @property def has_system_splash(self) -> bool: - return os.path.exists(INIT_STAMP) + return has_system_splash() def _set_stamp(self): try: diff --git a/uilib/spi_timing.py b/uilib/spi_timing.py index be0ec6a36..a57467404 100644 --- a/uilib/spi_timing.py +++ b/uilib/spi_timing.py @@ -18,10 +18,60 @@ One source of truth for "how long does it take to push N pixels at this SPI clock", used by the LCD drivers' transfer_ms (the inline-push gate), the emulator's transfer-time simulation, and lcd320x240's poll_divisor. + +Callers must pass the *actual* clock, not the requested one — see actual_spi_hz. """ -# Device target: BAUDR=4 → 50.00 MHz actual (Pi 4/5; see pistomp-arch firstboot.sh). -DEVICE_SPI_HZ: float = 50_000_000.0 +import math +from pathlib import Path +from typing import Optional + +_DT_COMPATIBLE = Path("/proc/device-tree/compatible") + +# SPI controller source clock by SoC. spi-bcm2835 (Pi <=4) divides the VPU core +# clock; spi-dw (Pi 5, inside RP1) divides RP1_CLK_SYS. +SPI_SOURCE_HZ: dict[str, int] = { + "brcm,bcm2837": 400_000_000, # Pi 3 (v2) + "brcm,bcm2711": 500_000_000, # Pi 4 + "brcm,bcm2712": 200_000_000, # Pi 5 (v3) +} + +# Off-device (tests, emulator). +DEFAULT_SOURCE_HZ: int = 200_000_000 + + +def spi_source_hz() -> int: + """SPI controller source clock for the running host.""" + try: + raw = _DT_COMPATIBLE.read_bytes() + except OSError: + return DEFAULT_SOURCE_HZ + for entry in raw.decode("ascii", errors="replace").split("\0"): + source = SPI_SOURCE_HZ.get(entry) + if source is not None: + return source + return DEFAULT_SOURCE_HZ + + +def actual_spi_hz(requested_hz: float, source_hz: Optional[int] = None) -> float: + """The clock the SPI controller will really run at for `requested_hz`. + + Both drivers take ceil(source / requested) then round that divisor up to an + even number, so the achieved clock lands on source/even and never exceeds + the request. One hertz below an exact divisor point costs a whole step: on a + Pi 3, 66_666_666 gives 50 MHz and 66_666_667 gives 66.67 MHz. + """ + if requested_hz <= 0: + raise ValueError(f"requested_hz must be positive, got {requested_hz}") + source = spi_source_hz() if source_hz is None else source_hz + + # Integer DIV_ROUND_UP; float division rounds the wrong way at divisor points. + divisor = -(-source // math.floor(requested_hz)) + if divisor < 2: + divisor = 2 + elif divisor % 2: + divisor += 1 + return source / divisor # Constants fit from on-device timing of LcdIli9341.update() at 20 MHz, 33.3 MHz, # and 50 MHz actual (Pi 5 / Python 3.14, tools/bench_lcd_device.py). From 628be88ca466a9a993dc86eb28abc4b523775e13 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Thu, 9 Jul 2026 23:42:24 -0400 Subject: [PATCH 2/6] Do not wake up websocket unnecessarily --- modalapi/websocket_bridge.py | 34 +++++++++++++- tests/test_websocket_bridge.py | 81 ++++++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 2 deletions(-) diff --git a/modalapi/websocket_bridge.py b/modalapi/websocket_bridge.py index 9902f7bb1..a464dedda 100644 --- a/modalapi/websocket_bridge.py +++ b/modalapi/websocket_bridge.py @@ -55,6 +55,7 @@ def __init__( self.ws = None self._loop: Optional[asyncio.AbstractEventLoop] = None self._stop_event: asyncio.Event = asyncio.Event() + self._wakeup: asyncio.Event = asyncio.Event() # Metrics self.messages_sent = 0 @@ -80,10 +81,21 @@ def signal_stop(self): if self._loop is None: return self._loop.call_soon_threadsafe(self._stop_event.set) + self._loop.call_soon_threadsafe(self._wakeup.set) ws = self.ws if ws is not None: asyncio.run_coroutine_threadsafe(ws.close(), self._loop) + def notify(self): + """Thread-safe: wake the send loop after a message is enqueued.""" + loop = self._loop + if loop is None: + return # worker not started; connect-time flush covers these + try: + loop.call_soon_threadsafe(self._wakeup.set) + except RuntimeError: + pass # loop closed during shutdown + async def _interruptible_sleep(self, delay: float) -> bool: """Sleep for delay seconds; returns True if stop was signaled before the delay elapsed.""" try: @@ -124,7 +136,19 @@ async def _async_worker(self): if flushed: logging.info(f"Flushed {flushed} stale messages from queue after reconnect") - await asyncio.gather(self._process_queue(ws), self._receive_messages(ws)) + # FIRST_COMPLETED, not gather: the send loop parks on _wakeup and + # cannot notice a closed socket on its own. Whichever loop exits + # first cancels the other so we fall through to reconnect. + tasks = { + asyncio.create_task(self._process_queue(ws)), + asyncio.create_task(self._receive_messages(ws)), + } + done, pending = await asyncio.wait(tasks, return_when=asyncio.FIRST_COMPLETED) + for task in pending: + task.cancel() + await asyncio.gather(*pending, return_exceptions=True) + for task in done: + task.result() # re-raise so the reconnect handler sees it except (websockets.exceptions.WebSocketException, OSError, ConnectionRefusedError) as e: logging.error(f"WebSocket connection error: {e}") @@ -156,7 +180,11 @@ async def _process_queue(self, ws): try: msg = self.command_queue.get_nowait() except queue.Empty: - await asyncio.sleep(0.001) # 1ms yield + # Clear before re-checking: a producer that enqueues between the + # failed get and the clear would otherwise have its wakeup erased. + self._wakeup.clear() + if self.command_queue.empty(): + await self._wakeup.wait() continue await ws.send(msg) @@ -263,6 +291,7 @@ def send_bpm(self, bpm: float) -> bool: if self._worker.backpressure_active: return False self.command_queue.put_nowait(f"transport-bpm {bpm}") + self._worker.notify() return True def send_parameter(self, instance_id: str, symbol: str, value: float) -> bool: @@ -271,6 +300,7 @@ def send_parameter(self, instance_id: str, symbol: str, value: float) -> bool: if self._worker.backpressure_active: return False self.command_queue.put_nowait(f"param_set /graph/{instance_id}/{symbol} {value}") + self._worker.notify() return True def get_received_messages(self) -> list: diff --git a/tests/test_websocket_bridge.py b/tests/test_websocket_bridge.py index ff10395fa..5ea828ab6 100644 --- a/tests/test_websocket_bridge.py +++ b/tests/test_websocket_bridge.py @@ -3,6 +3,8 @@ import asyncio import queue +import pytest + from modalapi.websocket_bridge import AsyncWebSocketBridge, WebSocketWorker @@ -231,3 +233,82 @@ def test_multiple_sends_preserve_order(): "transport-bpm 60", "param_set /graph/b/y 2.0", ] + + +# --------------------------------------------------------------------------- +# _process_queue: parks on _wakeup rather than polling +# --------------------------------------------------------------------------- + + +class _SendWs: + """WebSocket stand-in that records sends. No transport => buffer size reads as 0.""" + + def __init__(self): + self.sent: list[str] = [] + + async def send(self, msg: str) -> None: + self.sent.append(msg) + + +async def _started_worker() -> tuple[WebSocketWorker, _SendWs, asyncio.Task]: + worker = _make_worker() + worker.running = True + worker._loop = asyncio.get_running_loop() + ws = _SendWs() + task = asyncio.create_task(worker._process_queue(ws)) + await asyncio.sleep(0.02) # let it reach the park + return worker, ws, task + + +def test_idle_send_loop_parks_instead_of_polling(): + async def go(): + worker, ws, task = await _started_worker() + assert not task.done() + assert ws.sent == [] + # Parked: the event was consumed, so the loop is suspended, not spinning. + assert not worker._wakeup.is_set() + + worker.running = False + worker.signal_stop() + await asyncio.wait_for(task, timeout=1.0) + + asyncio.run(go()) + + +def test_notify_wakes_parked_send_loop(): + async def go(): + worker, ws, task = await _started_worker() + + worker.command_queue.put_nowait("param_set /graph/a/x 1.0") + worker.notify() + await asyncio.sleep(0.02) + assert ws.sent == ["param_set /graph/a/x 1.0"] + + worker.running = False + worker.signal_stop() + await asyncio.wait_for(task, timeout=1.0) + + asyncio.run(go()) + + +def test_parked_send_loop_is_cancellable(): + """_async_worker cancels the sender when the receive loop sees the socket close. + + Without this, a disconnect while idle would hang the bridge forever: the sender + never touches the socket, so it cannot notice the close on its own. + """ + + async def go(): + _worker, _ws, task = await _started_worker() + task.cancel() + with pytest.raises(asyncio.CancelledError): + await task + + asyncio.run(go()) + + +def test_notify_before_worker_starts_is_a_noop(): + # No event loop yet; notify() must not raise. Queued messages are flushed on connect. + bridge = _make_bridge() + bridge.send_parameter("a", "x", 1.0) + assert bridge.get_queue_depth() == 1 From 73f0dad78a7b74d470e609ca6b9479d0708d90cf Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 10 Jul 2026 00:05:53 -0400 Subject: [PATCH 3/6] Put dpkg-verify on a background thread --- modalapi/modhandler.py | 31 ++++++------ modalapi/version_check.py | 71 ++++++++++++++++++++++++++ pistomp/lcd320x240.py | 2 +- tests/test_lcd320x240.py | 1 + tests/test_menu_scrolling.py | 1 + tests/test_version_check.py | 98 ++++++++++++++++++++++++++++++++++++ tests/v2/test_system_menu.py | 38 ++++++++------ tests/v3/test_system_menu.py | 38 ++++++++------ util/modify_version.sh | 9 ++-- 9 files changed, 239 insertions(+), 50 deletions(-) create mode 100644 modalapi/version_check.py create mode 100644 tests/test_version_check.py diff --git a/modalapi/modhandler.py b/modalapi/modhandler.py index 2f5ea7c61..c7e511539 100755 --- a/modalapi/modhandler.py +++ b/modalapi/modhandler.py @@ -65,6 +65,7 @@ WebSocketMessage, ) from modalapi.pedalboard_monitor import FileChangeMonitor, read_pedalboard_bundle +from modalapi.version_check import DpkgDriftCheck from pistomp.controller_manager import ControllerManager from pistomp.current import Current @@ -105,6 +106,9 @@ def __init__(self, audiocard: Audiocard, homedir, data_dir="/home/pistomp/data") self.build_version = "User build" self.build_file = "/home/pistomp/.osbuild" + # background task(s) + self._drift_check = DpkgDriftCheck() + self.pedalboards = {} self.pedalboard_list = [] # TODO LAME to have two lists self.plugin_dict = {} @@ -1076,21 +1080,8 @@ def system_info_load(self): output = subprocess.check_output(["dpkg-query", "--showformat=${Version}", "--show", "pi-stomp"]) self.software_version = output.decode().strip() logging.info("pi-Stomp Software Version (pkg): %s" % self.software_version) - # dpkg equivalent of `git describe --dirty=*`: append an - # asterisk when on-disk package contents have drifted from - # the .deb's recorded md5sums (e.g. after ./deploy.sh or - # manual edits). Skipped silently on non-dpkg systems. - try: - verify = subprocess.run( - ["dpkg", "--verify", "pi-stomp"], - capture_output=True, - text=True, - check=False, - ) - if verify.stdout.strip() or verify.stderr.strip(): - self.software_version += "*" - except (FileNotFoundError, OSError): - pass + # dpkg equivalent of `git describe --dirty=*` + self._drift_check.start() except subprocess.CalledProcessError: logging.error("Cannot obtain software version info") @@ -1118,6 +1109,16 @@ def system_info_load(self): self.bypass_right = self.audiocard.get_bypass_right() self.lcd.update_bypass(self.bypass_left, self.bypass_right) + def get_software_version(self) -> str: + """Software version with optional '*' suffix when on-disk files have + drifted from the installed .deb. Blocks on the background `dpkg --verify` + if it hasn't finished — fine, because opening the System Info menu is + never the user's first move, and a few seconds there is invisible + compared to the same wait on every boot.""" + self._drift_check.join() + base = self.software_version or "" + return base + ("*" if self._drift_check.drifted else "") + @cached_property def recovery_available(self) -> bool: return ( diff --git a/modalapi/version_check.py b/modalapi/version_check.py new file mode 100644 index 000000000..56b86c853 --- /dev/null +++ b/modalapi/version_check.py @@ -0,0 +1,71 @@ +# This file is part of pi-stomp. +# +# pi-stomp is free software: you can redistribute it and/or modify +# it under the terms of the GNU General Public License as published by +# the Free Software Foundation, either version 3 of the License, or +# (at your option) any later version. +# +# pi-stomp is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +# GNU General Public License for more details. +# +# You should have received a copy of the GNU General Public License +# along with pi-stomp. If not, see . + +from __future__ import annotations + +import logging +import subprocess +import threading +from typing import Optional + + +class DpkgDriftCheck: + """Run `dpkg --verify pi-stomp` on a background thread.""" + + def __init__(self) -> None: + self._thread: Optional[threading.Thread] = None + self._drifted: Optional[bool] = None + + def start(self) -> None: + """Spawn the verification thread. Idempotent.""" + if self._thread is not None: + return + self._thread = threading.Thread(target=self._run, daemon=True, name="dpkg-drift-check") + self._thread.start() + + def join(self, timeout: Optional[float] = None) -> bool: + """Block until the check finishes (or `timeout` elapses). Returns + True if the check is done, False if `join` timed out.""" + thread = self._thread + if thread is None: + return True + thread.join(timeout=timeout) + return not thread.is_alive() + + @property + def done(self) -> bool: + return self._drifted is not None + + @property + def drifted(self) -> bool: + """True if drift was detected, False otherwise (including pre-start + and on any failure — the indicator is best-effort).""" + return bool(self._drifted) + + def _run(self) -> None: + try: + verify = subprocess.run( + ["dpkg", "--verify", "pi-stomp"], + capture_output=True, + text=True, + check=False, + ) + self._drifted = bool(verify.stdout.strip() or verify.stderr.strip()) + except (FileNotFoundError, OSError) as e: + logging.debug(f"dpkg --verify unavailable: {e}") + self._drifted = False + except Exception as e: + logging.warning(f"dpkg --verify failed: {e}") + self._drifted = False diff --git a/pistomp/lcd320x240.py b/pistomp/lcd320x240.py index 19d085e43..296656f5b 100644 --- a/pistomp/lcd320x240.py +++ b/pistomp/lcd320x240.py @@ -889,7 +889,7 @@ def update_sample_pedalboards(self, arg): def draw_system_info_dialog(self, arg): msg="Software:{}\nBuild:{}\nSystemState:{}\nTemperature:{}\nThrottled:{}".format( - self.handler.software_version, + self.handler.get_software_version(), self.handler.build_version, self.handler.SystemState, self.handler.temperature, diff --git a/tests/test_lcd320x240.py b/tests/test_lcd320x240.py index a7d984633..ef2649e0f 100644 --- a/tests/test_lcd320x240.py +++ b/tests/test_lcd320x240.py @@ -51,6 +51,7 @@ def mock_handler(): handler.get_num_footswitches.return_value = 4 handler.hardware.version = 3 handler.software_version = "1.0.0" + handler.get_software_version.return_value = "1.0.0" handler.build_version = "20231027" handler.SystemState = "Running" handler.temperature = "45C" diff --git a/tests/test_menu_scrolling.py b/tests/test_menu_scrolling.py index 63daf363c..230ddd844 100644 --- a/tests/test_menu_scrolling.py +++ b/tests/test_menu_scrolling.py @@ -28,6 +28,7 @@ def long_menu(fake_lcd): handler.get_num_footswitches.return_value = 4 handler.hardware.version = 3 handler.software_version = "1.0.0" + handler.get_software_version.return_value = "1.0.0" handler.build_version = "20231027" handler.SystemState = "Running" handler.temperature = "45C" diff --git a/tests/test_version_check.py b/tests/test_version_check.py new file mode 100644 index 000000000..68bce73a8 --- /dev/null +++ b/tests/test_version_check.py @@ -0,0 +1,98 @@ +"""Unit tests for modalapi.version_check.DpkgDriftCheck.""" + +import threading +from unittest.mock import MagicMock, patch + +from modalapi.version_check import DpkgDriftCheck + + +def _completed(stdout: str = "", stderr: str = "") -> MagicMock: + cp = MagicMock() + cp.stdout = stdout + cp.stderr = stderr + return cp + + +def test_pre_start_state(): + check = DpkgDriftCheck() + assert check.done is False + assert check.drifted is False + # join() before start() is a no-op that returns "done". + assert check.join() is True + + +def test_clean_verify(): + check = DpkgDriftCheck() + with patch("subprocess.run", return_value=_completed("", "")): + check.start() + check.join() + assert check.done is True + assert check.drifted is False + + +def test_drift_detected(): + drift_out = "??5?????? /opt/pistomp/pi-stomp/modalapi/modhandler.py\n" + check = DpkgDriftCheck() + with patch("subprocess.run", return_value=_completed(drift_out, "")): + check.start() + check.join() + assert check.done is True + assert check.drifted is True + + +def test_stderr_only_counts_as_drift(): + check = DpkgDriftCheck() + with patch("subprocess.run", return_value=_completed("", "dpkg: warnings\n")): + check.start() + check.join() + assert check.drifted is True + + +def test_dpkg_missing_treated_as_no_drift(): + """If dpkg isn't installed (e.g. macOS dev box), the indicator is just off.""" + check = DpkgDriftCheck() + with patch("subprocess.run", side_effect=FileNotFoundError("dpkg")): + check.start() + check.join() + assert check.done is True + assert check.drifted is False + + +def test_unexpected_failure_treated_as_no_drift(): + check = DpkgDriftCheck() + with patch("subprocess.run", side_effect=RuntimeError("boom")): + check.start() + check.join() + assert check.done is True + assert check.drifted is False + + +def test_start_is_idempotent(): + """Calling start() twice must not spawn a second thread.""" + check = DpkgDriftCheck() + with patch("subprocess.run", return_value=_completed("", "")): + check.start() + first = check._thread + check.start() + second = check._thread + assert first is second + + +def test_join_with_timeout_when_not_done(monkeypatch): + """join(timeout) returns False when the thread is still running.""" + gate = threading.Event() + call_started = threading.Event() + + def slow_run(*args, **kwargs): + call_started.set() + gate.wait(timeout=2.0) + return _completed("", "") + + check = DpkgDriftCheck() + with patch("subprocess.run", side_effect=slow_run): + check.start() + call_started.wait(timeout=1.0) + assert check.join(timeout=0.05) is False + gate.set() + check.join() + assert check.done is True diff --git a/tests/v2/test_system_menu.py b/tests/v2/test_system_menu.py index d30350368..80775d294 100644 --- a/tests/v2/test_system_menu.py +++ b/tests/v2/test_system_menu.py @@ -35,9 +35,11 @@ def test_system_info_load(v2_system: SystemFixture): handler.audiocard = MagicMock() handler.audiocard.get_switch_parameter.return_value = True - with _patch_expanded(handler, False), \ - patch("subprocess.check_output", return_value=b"v1.0.0-abc\n"), \ - patch("subprocess.run", return_value=_fake_completed("", "")): + with ( + _patch_expanded(handler, False), + patch("subprocess.check_output", return_value=b"v1.0.0-abc\n"), + patch("subprocess.run", return_value=_fake_completed("", "")), + ): handler.system_info_load() assert handler.software_version == "v1.0.0-abc" @@ -56,12 +58,16 @@ def test_system_info_load_dpkg_clean(v2_system: SystemFixture): handler.audiocard = MagicMock() handler.audiocard.get_switch_parameter.return_value = True - with _patch_expanded(handler, False), \ - patch("subprocess.check_output", return_value=b"1.2.3\n"), \ - patch("subprocess.run", return_value=_fake_completed("", "")) as mock_run: + with ( + _patch_expanded(handler, False), + patch("subprocess.check_output", return_value=b"1.2.3\n"), + patch("subprocess.run", return_value=_fake_completed("", "")) as mock_run, + ): handler.system_info_load() + handler._drift_check.join() assert handler.software_version == "1.2.3" + assert handler.get_software_version() == "1.2.3" verify_calls = [c for c in mock_run.call_args_list if c.args and c.args[0][:3] == ["dpkg", "--verify", "pi-stomp"]] assert len(verify_calls) == 1 @@ -77,12 +83,16 @@ def test_system_info_load_dpkg_drifted(v2_system: SystemFixture): verify_out = "??5?????? /opt/pistomp/pi-stomp/modalapi/modhandler.py\n" - with _patch_expanded(handler, False), \ - patch("subprocess.check_output", return_value=b"1.2.3\n"), \ - patch("subprocess.run", return_value=_fake_completed(verify_out, "")): + with ( + _patch_expanded(handler, False), + patch("subprocess.check_output", return_value=b"1.2.3\n"), + patch("subprocess.run", return_value=_fake_completed(verify_out, "")), + ): handler.system_info_load() - assert handler.software_version == "1.2.3*" + handler._drift_check.join() + assert handler.software_version == "1.2.3" + assert handler.get_software_version() == "1.2.3*" def test_system_info_load_git_clean(v2_system: SystemFixture): @@ -94,8 +104,7 @@ def test_system_info_load_git_clean(v2_system: SystemFixture): handler.audiocard = MagicMock() handler.audiocard.get_switch_parameter.return_value = True - with _patch_expanded(handler, True), \ - patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1\n"): + with _patch_expanded(handler, True), patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1\n"): handler.system_info_load() assert handler.software_version == "v3.0.4-224-gd392af1\n" @@ -110,8 +119,7 @@ def test_system_info_load_git_dirty(v2_system: SystemFixture): handler.audiocard = MagicMock() handler.audiocard.get_switch_parameter.return_value = True - with _patch_expanded(handler, True), \ - patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1*\n"): + with _patch_expanded(handler, True), patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1*\n"): handler.system_info_load() - assert handler.software_version == "v3.0.4-224-gd392af1*\n" \ No newline at end of file + assert handler.software_version == "v3.0.4-224-gd392af1*\n" diff --git a/tests/v3/test_system_menu.py b/tests/v3/test_system_menu.py index 1a4d254f0..7c0843e66 100644 --- a/tests/v3/test_system_menu.py +++ b/tests/v3/test_system_menu.py @@ -46,9 +46,11 @@ def test_system_info_load(v3_system: SystemFixture): handler = v3_system.handler _setup(handler) - with _patch_expanded(handler, False), \ - patch("subprocess.check_output", return_value=b"v1.0.0-abc\n"), \ - patch("subprocess.run", return_value=_fake_completed("", "")): + with ( + _patch_expanded(handler, False), + patch("subprocess.check_output", return_value=b"v1.0.0-abc\n"), + patch("subprocess.run", return_value=_fake_completed("", "")), + ): handler.system_info_load() assert handler.software_version == "v1.0.0-abc" @@ -62,12 +64,16 @@ def test_system_info_load_dpkg_clean(v3_system: SystemFixture): handler = v3_system.handler _setup(handler) - with _patch_expanded(handler, False), \ - patch("subprocess.check_output", return_value=b"1.2.3\n"), \ - patch("subprocess.run", return_value=_fake_completed("", "")) as mock_run: + with ( + _patch_expanded(handler, False), + patch("subprocess.check_output", return_value=b"1.2.3\n"), + patch("subprocess.run", return_value=_fake_completed("", "")) as mock_run, + ): handler.system_info_load() + handler._drift_check.join() assert handler.software_version == "1.2.3" + assert handler.get_software_version() == "1.2.3" verify_calls = [c for c in mock_run.call_args_list if c.args and c.args[0][:3] == ["dpkg", "--verify", "pi-stomp"]] assert len(verify_calls) == 1 @@ -79,12 +85,16 @@ def test_system_info_load_dpkg_drifted(v3_system: SystemFixture): verify_out = "??5?????? /opt/pistomp/pi-stomp/modalapi/modhandler.py\n" - with _patch_expanded(handler, False), \ - patch("subprocess.check_output", return_value=b"1.2.3\n"), \ - patch("subprocess.run", return_value=_fake_completed(verify_out, "")): + with ( + _patch_expanded(handler, False), + patch("subprocess.check_output", return_value=b"1.2.3\n"), + patch("subprocess.run", return_value=_fake_completed(verify_out, "")), + ): handler.system_info_load() - assert handler.software_version == "1.2.3*" + handler._drift_check.join() + assert handler.software_version == "1.2.3" + assert handler.get_software_version() == "1.2.3*" def test_system_info_load_git_clean(v3_system: SystemFixture): @@ -92,8 +102,7 @@ def test_system_info_load_git_clean(v3_system: SystemFixture): handler = v3_system.handler _setup(handler) - with _patch_expanded(handler, True), \ - patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1\n"): + with _patch_expanded(handler, True), patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1\n"): handler.system_info_load() assert handler.software_version == "v3.0.4-224-gd392af1\n" @@ -104,8 +113,7 @@ def test_system_info_load_git_dirty(v3_system: SystemFixture): handler = v3_system.handler _setup(handler) - with _patch_expanded(handler, True), \ - patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1*\n"): + with _patch_expanded(handler, True), patch("subprocess.check_output", return_value=b"v3.0.4-224-gd392af1*\n"): handler.system_info_load() - assert handler.software_version == "v3.0.4-224-gd392af1*\n" \ No newline at end of file + assert handler.software_version == "v3.0.4-224-gd392af1*\n" diff --git a/util/modify_version.sh b/util/modify_version.sh index 2e53217b5..fd163a29a 100755 --- a/util/modify_version.sh +++ b/util/modify_version.sh @@ -22,7 +22,6 @@ if [ -z "$1" ] exit fi -jackdrc_file="/etc/jackdrc" config_dir="$HOME/data/config" config_file="$config_dir/default_config.yml" hwfile="$config_dir/hardware-descriptor.json" @@ -38,23 +37,25 @@ mkdir -p $config_dir cp $default_hwfile $hwfile +# JACK settings are not touched here. The period comes from JACK_PERIOD in +# /etc/default/jack (written by firstboot from pistomp.conf) and the port cap is +# derived from the board model by jackdrc. The sed that used to rewrite /etc/jackdrc +# stopped matching when the period became a variable, and jackdrc has since moved to +# /usr/lib/pistomp/ (shipped by jack2-pistomp). if awk "BEGIN {exit !($1 >= 3.0 )}"; then cp $pistomp_tre_config_file $config_file - sudo sed -i 's/-p [0-9]\+/-p 128/' $jackdrc_file if ! git -C "$pb_dir" checkout master 2>/dev/null && ! git -C "$pb_dir" checkout main 2>/dev/null; then echo "Git checkout failed (tried master and main)" exit 1 fi elif awk "BEGIN {exit !($1 >= 2.0 )}"; then cp $pistomp_core_config_file $config_file - sudo sed -i 's/-p [0-9]\+/-p 256/' $jackdrc_file if ! git -C "$pb_dir" checkout v2; then echo "Git checkout failed" exit 1 fi elif awk "BEGIN {exit !($1 >= 1.0 )}"; then cp $pistomp_orig_config_file $config_file - sudo sed -i 's/-p [0-9]\+/-p 256/' $jackdrc_file if ! git -C "$pb_dir" checkout v1; then echo "Git checkout failed" exit 1 From 7ce3a874f8204bcd851958a09e55881fa05710dd Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 10 Jul 2026 02:36:09 -0400 Subject: [PATCH 4/6] Seems to be fine skipping reset now?? --- pistomp/pistompcore.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/pistomp/pistompcore.py b/pistomp/pistompcore.py index a919413ed..0c1c86289 100755 --- a/pistomp/pistompcore.py +++ b/pistomp/pistompcore.py @@ -27,6 +27,7 @@ import pistomp.relay as Relay import pistomp.lcd320x240 as Lcd +from uilib.lcd_ili9341 import has_system_splash # Pins (Unless the hardware has been changed, these should not be altered) TOP_ENC_PIN_D = 17 @@ -69,12 +70,14 @@ def __init__(self, cfg, mod, midiout, refresh_callback): def init_lcd(self): # 66_666_667 = lowest request selecting CDIV=6; one Hz less gives 50 MHz. - self.mod.add_lcd(Lcd.Lcd(self.mod.homedir, self.mod, flip=True, - spi_speed_hz=66_666_667, reset=True)) + self.mod.add_lcd( + Lcd.Lcd(self.mod.homedir, self.mod, flip=True, spi_speed_hz=66_666_667, reset=not has_system_splash()) + ) def init_encoders(self): top_enc = EncoderController.EncoderController( - TOP_ENC_PIN_D, TOP_ENC_PIN_CLK, + TOP_ENC_PIN_D, + TOP_ENC_PIN_CLK, type=Token.NAV, sw_pin=1, ) From 7bbf8ca148f8de99a572f21fecac3197b7a334f0 Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Fri, 10 Jul 2026 02:41:42 -0400 Subject: [PATCH 5/6] Back out modify version changes --- util/modify_version.sh | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/util/modify_version.sh b/util/modify_version.sh index fd163a29a..2e53217b5 100755 --- a/util/modify_version.sh +++ b/util/modify_version.sh @@ -22,6 +22,7 @@ if [ -z "$1" ] exit fi +jackdrc_file="/etc/jackdrc" config_dir="$HOME/data/config" config_file="$config_dir/default_config.yml" hwfile="$config_dir/hardware-descriptor.json" @@ -37,25 +38,23 @@ mkdir -p $config_dir cp $default_hwfile $hwfile -# JACK settings are not touched here. The period comes from JACK_PERIOD in -# /etc/default/jack (written by firstboot from pistomp.conf) and the port cap is -# derived from the board model by jackdrc. The sed that used to rewrite /etc/jackdrc -# stopped matching when the period became a variable, and jackdrc has since moved to -# /usr/lib/pistomp/ (shipped by jack2-pistomp). if awk "BEGIN {exit !($1 >= 3.0 )}"; then cp $pistomp_tre_config_file $config_file + sudo sed -i 's/-p [0-9]\+/-p 128/' $jackdrc_file if ! git -C "$pb_dir" checkout master 2>/dev/null && ! git -C "$pb_dir" checkout main 2>/dev/null; then echo "Git checkout failed (tried master and main)" exit 1 fi elif awk "BEGIN {exit !($1 >= 2.0 )}"; then cp $pistomp_core_config_file $config_file + sudo sed -i 's/-p [0-9]\+/-p 256/' $jackdrc_file if ! git -C "$pb_dir" checkout v2; then echo "Git checkout failed" exit 1 fi elif awk "BEGIN {exit !($1 >= 1.0 )}"; then cp $pistomp_orig_config_file $config_file + sudo sed -i 's/-p [0-9]\+/-p 256/' $jackdrc_file if ! git -C "$pb_dir" checkout v1; then echo "Git checkout failed" exit 1 From cc66ecf7115b63f4b2df90bea7371225b5b6c2ea Mon Sep 17 00:00:00 2001 From: Cam Gorrie Date: Mon, 13 Jul 2026 18:10:25 -0400 Subject: [PATCH 6/6] Faster blitting and better Pi3/Pi5 SPI constants and tools --- GUIDE.md | 2 + deploy.sh | 22 +++- docs/lcd_worker_thread.md | 129 ++++++++++++++++++++++ docs/spi_lcd_timing_pi3.md | 103 ++++++++++++----- docs/spi_lcd_timing_pi5.md | 36 +++++- tests/test_spi_timing.py | 53 ++++++++- tools/bench_adc_contention.py | 202 ++++++++++++++++++++++++++++++++++ tools/bench_lcd_device.py | 10 +- tools/bench_pack_variants.py | 138 +++++++++++++++-------- uilib/lcd_ili9341.py | 32 +++--- uilib/panel.py | 2 - uilib/spi_timing.py | 82 +++++++++----- 12 files changed, 687 insertions(+), 124 deletions(-) create mode 100644 docs/lcd_worker_thread.md create mode 100644 tools/bench_adc_contention.py diff --git a/GUIDE.md b/GUIDE.md index e96de4839..eff11c186 100644 --- a/GUIDE.md +++ b/GUIDE.md @@ -254,6 +254,8 @@ Types: `KNOB` and `EXPRESSION` (config-driven). When `autosync: true`, `initiali **Surface formats** — Never create a bare `pygame.Surface((w,h))`: it inherits the display format, which is opaque RGB headless (device/tests) but ARGB under a real window driver (the cocoa emulator). That stray alpha channel silently breaks SRCALPHA compositing (e.g. glyph pastes drop their fill). Always be explicit: `pygame.SRCALPHA` for alpha surfaces, or `depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)` for opaque RGB ones (bit-identical to the device's headless 32-bit XRGB; `depth=24` differs in AA rounding). +**`PanelStack`'s root surface must stay opaque.** `LcdIli9341.update` quantises it to RGB565 with an SDL convert-blit; give that blit an `SRCALPHA` source and SDL silently switches to its per-pixel *alpha-blending* blitter — ~7x slower, and it lands on every LCD push. The root is a blend *destination*, so it needs 32-bit for blend precision but gains nothing from a dest alpha channel (a dimmer over black yields the same `(128,128,128)` either way). Panel surfaces (`ShroudedPanel`, `RoundedPanel`) are blit *sources* and do still need `RGBA`. Benchmark the pack path with `tools/bench_pack_variants.py`. + ### WebSocket Bridge (`modalapi/websocket_bridge.py`, `modalapi/ws_protocol.py`) `AsyncWebSocketBridge` manages a background daemon thread that owns the WebSocket connection. Queues: diff --git a/deploy.sh b/deploy.sh index 058a97665..66a9f157c 100755 --- a/deploy.sh +++ b/deploy.sh @@ -10,15 +10,35 @@ REMOTE_DIR="/home/pistomp/pi-stomp" echo "==> Deploying to ${TARGET}" -rsync -az --delete --exclude='__pycache__' --exclude='*.pyc' \ +rsync -az --delete \ + --exclude='.venv' --exclude='__pycache__' --exclude='*.pyc' \ + --exclude='.pytest_cache' --exclude='.ruff_cache' --exclude='.coverage' \ + --exclude='.DS_Store' --exclude='.claude' --exclude='.github' \ + --exclude='typings' \ --exclude='.git' --exclude='.gitignore' --exclude='tests' \ --exclude='setup' --exclude='*.md' --exclude='*.yml' --exclude='*.yaml' \ --exclude='*.json' --exclude='*.toml' --exclude='*.lock' \ --exclude='*.png' --exclude='*.jpg' --exclude='*.svg' \ + --filter='protect .git-meta' \ ./ \ "${TARGET}:${REMOTE_DIR}/" echo "==> Restarting mod-ala-pi-stomp" ssh "${TARGET}" 'sudo systemctl restart mod-ala-pi-stomp' +echo "==> Service starting, showing logs..." +echo "----------------------------------------" +ssh "${TARGET}" "timeout 2 sudo journalctl -u mod-ala-pi-stomp -f --since '1 second ago' 2>/dev/null || true" +echo "----------------------------------------" + +if ssh "${TARGET}" 'sudo systemctl is-active --quiet mod-ala-pi-stomp'; then + echo "==> Service started successfully." +else + echo "ERROR: Service failed to start!" + echo "----------------------------------------" + echo "Service status:" + ssh "${TARGET}" 'sudo systemctl status mod-ala-pi-stomp' + exit 1 +fi + echo "==> Done" diff --git a/docs/lcd_worker_thread.md b/docs/lcd_worker_thread.md new file mode 100644 index 000000000..20082bbba --- /dev/null +++ b/docs/lcd_worker_thread.md @@ -0,0 +1,129 @@ +# Move the LCD push off the poll loop + +## Why + +A full-frame push blocks the UI thread for **21.4 ms on v2** and **25.2 ms on v3**, +against a 10 ms tick. Nothing mitigates this today. + +`PanelStack.propagate_dirty` gates pushes on `transfer_ms(clip) <= INLINE_BUDGET_MS` +(8 ms), which *looks* like it protects the poll loop. It doesn't. The deferred path +— `poll_lcd_updates()` → `flush()` → `lcd.update()` — runs on the **same UI thread** +(`modalapistomp.py:224`). There is no worker anywhere. Deferring a push doesn't +offload it; it moves it to a later tick, where it blocks just as long. The gate only +chooses *which* tick eats the stall. + +The win is available because the cost is almost entirely **wire time**, and wire time +is the part that touches no UI state: + +| | stays on UI thread (compose + 565 pack) | movable (address window + `os.write`) | +|---|---|---| +| v3 @ 50 MHz | 0.29 ms | 24.9 ms | +| v2 @ 66.67 MHz | 1.82 ms | 19.3 ms | + +`LcdIli9341.update()` already materialises `pixels_bytes` (an immutable `bytes`) and +hands it to `_block_fast(x0, y0, x1, y1, data)`, which touches only the SPI fd and the +DC/CS pins. **The seam is already there.** No UI code moves, no surface is shared, and +no double-buffer is needed — `tobytes()` is the copy. `os.write` releases the GIL, so +the UI thread genuinely runs during the transfer. + +Estimated UI-thread stall on a full frame: **25.2 → ~0.3 ms (v3)**, **21.4 → ~1.8 ms (v2)**. + +## Hazards + +Both were found by measurement, and both are invisible from the cost model. Neither is +a reason not to do this — but a naive "wrap `update()` in a thread" ships a regression. + +### 1. The ADC shares the SPI bus, and a full frame is one kernel message + +The MCP3008 is on the same master (CE1, raw `spidev` @1 MHz); `poll_controls()` reads it +every tick on the UI thread. Today LCD and ADC **cannot** overlap — both are on the UI +thread — so the kernel sees one transfer at a time. A worker breaks that invariant, and +with `spidev.bufsiz=163840` the whole 153,600 B payload is a *single* SPI message. The +spi core serialises messages per master, so an ADC read issued mid-frame waits for the +entire frame to clock out. + +Measured on v2 @ 66.67 MHz (`tools/bench_adc_contention.py`), ADC `xfer2` latency: + +| | median | p95 | max | reads over the 10 ms tick | +|---|---|---|---|---| +| LCD idle (today) | 0.18 ms | 0.30 ms | 1.08 ms | **0%** | +| worker, one 153.6 kB write | **11.30 ms** | 12.86 ms | 18.14 ms | **100%** | +| worker, 64 kB chunks | 3.24 ms | 9.12 ms | 10.51 ms | 1% | +| worker, 16 kB chunks | 1.98 ms | 3.19 ms | 4.04 ms | **0%** | +| worker, 4 kB chunks | 0.82 ms | 1.61 ms | 2.91 ms | 0% | + +Unchunked, **every** ADC read blocks past the tick. That trades a 21 ms stall in +`poll_lcd_updates` for an 11 ms stall in `poll_controls` — knobs and expression pedals +judder instead of the display. Strictly worse than today. + +**16 kB is the sweet spot.** ADC max 4.04 ms, zero reads over tick. It costs full-frame +throughput (48.8 → 39 fps, −20%), which is free: `lcd_poll_divisor` gates pushes far +below that anyway. + +### 2. LCD chip-select is a GPIO held low across the whole block write + +`_block_fast` drives CS manually (`cs.value = ...`) and holds it asserted for the entire +call. **Any SPI traffic the kernel interleaves while that line is low is clocked into the +ILI9341 as pixel data.** So releasing the kernel lock between chunks is not sufficient — +chunking must genuinely *deassert CS* per chunk. + +And once CS drops mid-GRAM-write, resuming needs the ILI9341's **`Write_Memory_Continue` +(0x3C)**. The Adafruit driver only defines `_RAM_WRITE = 0x2C` (`ili9341.py:49`), and +re-issuing *that* resets the address pointer to the window origin. + +> **This is the trap.** It is invisible in the timing data — the bytes and the bus +> occupancy are identical whether or not the panel interprets them correctly. The +> chunked rows in the table above were produced by a prototype that did **not** send +> 0x3C, so its *timings are valid but its pixels are unverified*. MISO is not wired on +> v2, so GRAM cannot be read back; this needs a human looking at the screen. + +## Proposed architecture + +1. **Worker owns `_block_fast` only.** Poll loop composes, packs to 565, and submits + `(bytes, x0, y0, x1, y1)` to a single-slot queue. Non-blocking. If a push is already + in flight, coalesce or drop — `LcdIli9341.self.lock` already exists for this. +2. **Chunk the payload at ~16 kB**, deasserting CS between chunks so the ADC can + interleave. +3. **Add `Write_Memory_Continue` (0x3C)** for every chunk after the first. Without it, + (2) corrupts the frame. + +Do (2) and (3) together or not at all — (2) alone is a silent corruption bug, and (1) +alone is a regression. + +### Knock-on: the cost model gets less load-bearing + +`transfer_ms` exists to gate inline pushes onto the UI thread. Once pushes don't block +the UI thread, a bad estimate stops being dangerous, and the per-SoC `PUSH_PROFILE` in +`uilib/spi_timing.py` (v2's per-pixel cost is 6.2× v3's — the boards genuinely diverge) +is demoted from safety-critical to an optimisation. It's still needed for +`lcd320x240.poll_divisor` and the emulator, which both need an estimate before any push +has happened. + +### Unrelated, and worth more than it looks + +`PanelStack._pending_lcd_clip` coalesces with `Box.union` — a **bounding box**, +unconditionally. Wire cost is linear in pixels, so merging disjoint rects is close to +pure loss: the 4×(32×131) case in [`spi_lcd_timing_pi3.md`](spi_lcd_timing_pi3.md) costs +**9.20 ms pushed separately, 21.36 ms coalesced** (2.3× worse). On v2 a merge only pays +while the union adds < ~4,575 px (6% of the screen) — the point where the added pixels +outweigh the 1.20 ms fixed cost saved. + +We built a cost model and then don't consult it for the one decision where it would pay. +Comparing `cost(union)` against `cost(a) + cost(b)` before merging is small, self- +contained, and independent of everything above. + +## Status + +⚠️ **Proposed. Hazards measured, architecture not built.** + +Evidence: `tools/bench_adc_contention.py` (v2 numbers above; **not yet run on v3** — the +Pi 5's DMA path arbitrates differently and its ADC sits behind RP1 rather than on-die). +Push costs: `tools/bench_lcd_device.py`, fitted into `spi_timing.PUSH_PROFILE`. + +Verify 0x3C **visually on hardware** before trusting the chunked path. The one thing the +instruments cannot tell you here is whether the picture is right. + +> Bench scripts live in `tools/` and must `sys.path.insert` the repo root — otherwise +> `import uilib` resolves to the copy in the venv's site-packages and you will silently +> benchmark the *packaged* driver instead of your working tree. This has already cost +> one round of bad constants. diff --git a/docs/spi_lcd_timing_pi3.md b/docs/spi_lcd_timing_pi3.md index 3ae0da923..e4e4e3e66 100644 --- a/docs/spi_lcd_timing_pi3.md +++ b/docs/spi_lcd_timing_pi3.md @@ -76,20 +76,26 @@ moves the SPI clock with it. ## Achievable speeds -Full-frame (320×240 = 76,800 px, 153,600 B) medians, service stopped. +Full-frame (320×240 = 76,800 px, 153,600 B), service stopped. "Total" is `pack + address-window + payload write`; MB/s is payload/wire. +Wire time is clock-derived and unaffected by the pack. **Total** is from the +current SDL convert-blit pack (see "The pack" below): measured at 20, 40 and +66.67 MHz, and model-derived from `spi_timing.PUSH_PROFILE` at the rest. + | Requested | CDIV | **Actual** | Wire | Total | Throughput | Full-frame ceiling | |-----------|------|-----------|---------|---------|----------|-----| -| 12.5 MHz | 32 | 12.50 MHz | 98.77 ms | 102.87 ms | 1.56 MB/s | 9.7 fps | -| 20 MHz | 20 | 20.00 MHz | 61.90 ms | 65.93 ms | 2.48 MB/s | 15.2 fps | -| 25–28.5 MHz | 16 | 25.00 MHz | 49.63 ms | 53.70 ms | 3.09 MB/s | 18.6 fps | -| 33.3 MHz | 14 | 28.57 MHz | 43.49 ms | 47.63 ms | 3.53 MB/s | 21.0 fps | -| 40–44.4 MHz | 10 | 40.00 MHz | 31.18 ms | 35.53 ms | 4.93 MB/s | 28.1 fps | -| 50–66.66 MHz | 8 | 50.00 MHz | 25.05 ms | 29.23 ms | 6.13 MB/s | 34.2 fps | -| **66,666,667–99.9 MHz** | **6** | **66.67 MHz** | **18.94 ms** | **23.30 ms** | **8.11 MB/s** | **42.9 fps** | +| 12.5 MHz | 32 | 12.50 MHz | 97.83 ms | 100.85 ms | 1.57 MB/s | 9.9 fps | +| 20 MHz | 20 | 20.00 MHz | 61.14 ms | **63.98 ms** ᵐ | 2.51 MB/s | 15.6 fps | +| 25–28.5 MHz | 16 | 25.00 MHz | 48.92 ms | 51.93 ms | 3.14 MB/s | 19.3 fps | +| 33.3 MHz | 14 | 28.57 MHz | 42.80 ms | 45.82 ms | 3.59 MB/s | 21.8 fps | +| 40–44.4 MHz | 10 | 40.00 MHz | 30.57 ms | **33.66 ms** ᵐ | 5.02 MB/s | 29.8 fps | +| 50–66.66 MHz | 8 | 50.00 MHz | 24.46 ms | 27.47 ms | 6.28 MB/s | 36.4 fps | +| **66,666,667–99.9 MHz** | **6** | **66.67 MHz** | **18.34 ms** | **21.31 ms** ᵐ | **8.37 MB/s** | **46.8 fps** | | ≥ 100 MHz | 4 | 100 MHz | 12.3 ms | — | — | **display garbled** | +ᵐ = measured; the rest are the fitted model. + **66.67 MHz is the practical maximum.** It sits at the ILI9341's 66.7 MHz write-cycle limit, and the next step up (CDIV=4) is the known-garbled 100 MHz. `pistompcore.py` requests `66_666_667`. @@ -107,24 +113,39 @@ while v2 pushes a full frame 24% faster. ## Measured per-call overhead -Wire time is `px × 16 bits / clock`; `BITS_PER_PIXEL` fits to **16.00**, i.e. -there is no framing overhead. Everything else is fixed or CPU-bound: +Wire time is `px × 16 bits / clock`; the fitted `bits_per_pixel` is **15.92**, +i.e. there is no framing overhead. Everything else is fixed or CPU-bound: | Term | Cost | Scales with | |------|------|-------------| | Payload `write()` overhead (syscall + DMA setup) | **0.48 ms** | nothing — flat across all 7 clocks | | Address window (5 small writes + DC/CS toggles) | **0.50 ms** | nothing | -| RGB565 pack, idle | `0.36 ms + 4.23e-5 × px` | pixels (3.61 ms full frame) | -| RGB565 pack, under service load | `0.41 ms + 6.56e-5 × px` | pixels (5.45 ms full frame) | +| RGB565 pack + driver (`pipeline_ms_per_px`) | `2.36e-5 × px` | pixels (**1.82 ms** full frame) | The payload-write overhead being constant (0.466–0.505 ms) from 12.5 MHz all the way to 66.67 MHz is also the proof that `core_freq` is pinned at 400 MHz; a scaling VPU clock would show up as clock-dependent drift here. -Fixed cost per push is therefore **~1.0 ms** at the Python boundary, not the -~15 µs of kernel-side register writes. Ten small dirty rects cost 10 ms of pure +Fixed cost per push is **1.20 ms** (`fixed_ms`) at the Python boundary, not the +~15 µs of kernel-side register writes. Ten small dirty rects cost 12 ms of pure overhead before a single pixel moves. +### The pack + +`LcdIli9341.update()` quantises to RGB565 with a **single SDL convert-blit** into a +preallocated 16-bit staging surface, not a numpy channel-mask pipeline. Measured +on this board, end-to-end through `_block` at 66.67 MHz: + +| Full frame | numpy pack | SDL convert-blit | +|---|---|---| +| 320×240 | 23.91 ms | **21.31 ms** (−2.59 ms) | + +The blit source **must be opaque** — `PanelStack`'s root is XRGB for exactly this +reason. Blitting an `SRCALPHA` surface silently takes SDL's per-pixel +alpha-blending path instead of a format convert, which on this A53 is *~8× slower +than the numpy pack it replaced*. See CLAUDE.md "Traps" and +`tools/bench_pack_variants.py`. + **`spidev.bufsiz`** matters exactly as on the Pi 5: the 4096 B default means 38 writes per frame. `spidev.bufsiz=163840` is already set on the kernel cmdline. @@ -136,30 +157,51 @@ Live profile of the idle main panel (`PISTOMP_PROFILE=1`, 1 s windows). Note that `profiling.maybe_start()` is only called by the tuner and NAM panels, so reproducing this requires starting the reporter explicitly. +> **Captured with the old numpy pack.** The `lcd.update:pack` row is the one the +> SDL convert-blit changed; the others stand. Not re-profiled live. + | Stage | Mean | % of one core | |---|---|---| | `poll_lcd_updates` | 7.23 ms | 33% | | ├ `lcd.update:_block` (SPI wire) | 1.94 ms | 11% | | ├ `widget.do_draw` | 1.32 ms | 7% | -| ├ `lcd.update:pack` | 1.26 ms | 7% | +| ├ `lcd.update:pack` | 1.26 ms → **~0.5 ms** | 7% → ~3% | | ├ `panelstack.recompose` | 0.65 ms | 4% | | └ unaccounted | ~2.0 ms | — | | `poll_controls` | 0.83 ms | 8% | -The whole pi-stomp process sits at ~55% of one core, and the main loop achieves -**92 Hz**, not the nominal 100. +The whole pi-stomp process sat at ~55% of one core with a **92 Hz** main loop, not +the nominal 100. The pack is now ~2.6× cheaper, so expect an idle tick nearer +6.5 ms — but that is derived from the pack benchmark, not a fresh live profile. -**The SPI transfer is only 27% of an LCD tick.** The push is bandwidth-bound +**The SPI transfer is only ~27% of an LCD tick.** The push is bandwidth-bound only for large rects; for the small dirty rects of normal operation it is -compute-bound. Raising the clock from 50 → 66.67 MHz shaves ~0.5 ms off a -7.2 ms idle tick (~7%), but cuts a *full-frame* push from 25.05 ms to 18.94 ms — -so it pays off on panel and pedalboard transitions, not on idle animation. +compute-bound. Raising the clock from 50 → 66.67 MHz shaves ~0.5 ms off an idle +tick, but cuts a *full-frame* push from 24.46 ms to 18.34 ms of wire — so it pays +off on panel and pedalboard transitions, not on idle animation. + +### Why the cost model is per-SoC + +A single global constant set used to be defensible: with the numpy pack, a Pi 3A+ +refit gave `PIPELINE_MS_PER_PX=4.81e-5` against the Pi 5's `5.86e-5` — near enough +that sharing one value moved the 8 ms gate by 0.5%. + +**The SDL convert-blit ended that.** It is a far bigger win on the A76 than on the +A53, so the two boards diverged: + +| | `fixed_ms` | `pipeline_ms_per_px` | full-frame pack | +|---|---|---|---| +| v3 (Pi 5, BCM2712) | 0.2727 | 3.83e-6 | 0.29 ms | +| v2 (Pi 3A+, BCM2837) | 1.2008 | **2.36e-5 (6.2×)** | 1.82 ms | + +Both terms are CPU-bound, and `transfer_ms` gates **inline pushes onto the UI +thread** (`PanelStack.INLINE_BUDGET_MS`). Sharing v3's numbers would let the gate +admit a 31,691 px clip inline on v2 that actually costs **9.56 ms** — against an +8 ms budget on a 10 ms tick, consuming essentially all the slack. Underestimating +is the unsafe direction, so `spi_timing.PUSH_PROFILE` keys the profile off the +device-tree SoC, and an unrecognised board gets the *slower* profile. -`uilib/spi_timing.py`'s constants were fit on a Pi 5 and already predict Pi 3A+ -totals to within ~5% (mean 4.6%). A Pi 3A+ refit gives `FIXED_MS=0.95`, -`PIPELINE_MS_PER_PX=4.81e-5`, `BITS_PER_PIXEL=16.0` (mean error 2.2%) and moves -the 8 ms inline-push gate from 19,244 px to 19,144 px — a 0.5% shift, not worth -per-device constants. +Refit with `tools/bench_lcd_device.py` **on each board** whenever the pack changes. ### Two environmental drags @@ -205,9 +247,12 @@ the Pi 5. | Divisor rule | **Round up to even** (`cdiv += cdiv % 2`) | | Practical max speed | **66.67 MHz** (request ≥ `66_666_667`) | | Configured in `pistompcore.py` | `spi_speed_hz=66_666_667` | -| Peak payload throughput | **8.11 MB/s** measured (8.33 MB/s theoretical) | -| Full-frame wire time | 25.05 ms @ 50 MHz → **18.94 ms @ 66.67 MHz** | +| Peak payload throughput | **8.37 MB/s** measured (8.33 MB/s theoretical) | +| Full-frame wire time | 24.46 ms @ 50 MHz → **18.34 ms @ 66.67 MHz** | +| Full-frame total | **21.31 ms** @ 66.67 MHz (was 23.91 ms with the numpy pack) | | PCIe DMA stall | None (on-die DMA) | -| Fixed overhead/push | ~1.0 ms (0.48 payload write + 0.50 address window) | +| RGB565 pack | SDL convert-blit; source **must be opaque** | +| Fixed overhead/push | 1.20 ms (`PUSH_PROFILE[bcm2837].fixed_ms`) | +| Per-pixel pipeline | 2.36e-5 ms/px — **6.2× the Pi 5's**, hence per-SoC constants | | SPI share of an idle LCD tick | ~27% (the rest is CPU) | | Primary optimization lever | Dirty-rect culling | diff --git a/docs/spi_lcd_timing_pi5.md b/docs/spi_lcd_timing_pi5.md index 613cc442d..afae55e2d 100644 --- a/docs/spi_lcd_timing_pi5.md +++ b/docs/spi_lcd_timing_pi5.md @@ -102,6 +102,31 @@ dominates and the PCIe stall is near-zero at 50 MHz. --- +## The pack + +`LcdIli9341.update()` quantises the composed surface to RGB565 with a **single SDL +convert-blit** into a preallocated 16-bit staging surface — not a numpy +channel-mask pipeline. Measured full-frame on this board: + +| Term | Value | +|---|---| +| `fixed_ms` | 0.2727 ms | +| `pipeline_ms_per_px` | 3.83e-6 ms/px (**0.29 ms** full frame) | +| `bits_per_pixel` | 16.03 — no framing overhead | + +The blit source **must be opaque**; `PanelStack`'s root is XRGB for exactly this +reason. An `SRCALPHA` source silently takes SDL's per-pixel alpha-blending path +instead of a format convert and is *slower than the numpy pack it replaced*. See +CLAUDE.md "Traps" and `tools/bench_pack_variants.py`. + +At 50 MHz the pack is now **~1%** of a full-frame push (0.29 ms of 25 ms) — wire +time is overwhelmingly the cost, and dirty-rect culling remains the only lever +that matters. The pack is *not* negligible on v2, where it is 6.2× dearer; the +cost model is per-SoC for that reason. See +[`spi_lcd_timing_pi3.md`](spi_lcd_timing_pi3.md). + +--- + ## The PCIe DMA stall At 100 MHz SPI clock (BAUDR=2), the TX FIFO drains 16 bytes in **1.28 µs**. @@ -159,7 +184,7 @@ poll loop (10 ms cadence) _do_update() # runs on worker thread lock.acquire() - numpy pack + rot90 (~1 ms) + SDL 565 convert-blit (~0.3 ms) # no rot90: panel is landscape-native os.write(spidev_fd, data) # blocks ~25 ms on worker thread lock.release() ``` @@ -228,9 +253,8 @@ runs in a DRM worker thread. would need redesigning. **Python-side changes**: Replace `LcdIli9341` with a thin writer that opens -`/dev/fb0`, packs RGB565, and calls `write()`. Rotation must be pre-applied -in numpy (same as today) or configured via `MADCTL` in the firmware blob to -make the panel landscape-native (eliminating the `rot90` step entirely). +`/dev/fb0`, packs RGB565, and calls `write()`. Rotation needs no work — the +driver already sets `MADCTL` in `__init__` so the panel is landscape-native. **When to prefer this over the background thread**: If DRM integration is wanted for other reasons (console on the display, compositor, hardware @@ -284,7 +308,9 @@ optimization and requires no kernel or boot changes. | 66.7 MHz achievable? | **No** — requires BAUDR=3 (odd, forbidden) | | Safe maximum setting | Any value 56–99 MHz (all give 50 MHz actual) | | Configured in `pistomptre.py` | `spi_speed_hz=50_000_000` | -| Full-frame time @ 50 MHz | ~25 ms (24.6 ms wire + ~0 PCIe stall + ~0.4 ms overhead) | +| Full-frame time @ 50 MHz | **25.19 ms** measured (24.6 ms wire + ~0 PCIe stall + 0.56 ms pack/overhead) | +| RGB565 pack | SDL convert-blit; source **must be opaque**. 0.29 ms/full frame | +| Cost-model constants | `spi_timing.PUSH_PROFILE["brcm,bcm2712"]` — per-SoC; v2's differ 6.2× | | DMA/IRQ crossover | 128 bytes (FIFO depth × 2-byte words) | | Optimal `spidev.bufsiz` | ≥153,600 (set to 163,840 in `pistomp-arch/files/cmdline.txt`) | | Primary optimization lever | Dirty-rect culling (linear in pixel count) | diff --git a/tests/test_spi_timing.py b/tests/test_spi_timing.py index 3b8a92165..af64e8be0 100644 --- a/tests/test_spi_timing.py +++ b/tests/test_spi_timing.py @@ -1,6 +1,15 @@ import pytest -from uilib.spi_timing import DEFAULT_SOURCE_HZ, actual_spi_hz, spi_source_hz, transfer_ms +from uilib.spi_timing import ( + DEFAULT_PROFILE, + DEFAULT_SOURCE_HZ, + PUSH_PROFILE, + SPI_SOURCE_HZ, + actual_spi_hz, + push_profile, + spi_source_hz, + transfer_ms, +) PI3 = 400_000_000 # v2 — BCM2837 VPU core clock PI5 = 200_000_000 # v3 — RP1 CLK_SYS @@ -88,9 +97,49 @@ def test_source_defaults_on_unknown_soc(monkeypatch, tmp_path): assert spi_source_hz() == DEFAULT_SOURCE_HZ +def test_every_known_soc_has_a_push_profile(): + # push_profile() indexes PUSH_PROFILE with whatever soc_key() returns, and + # soc_key() answers from SPI_SOURCE_HZ. A SoC in one dict but not the other + # is a KeyError on that board and nowhere else. + assert SPI_SOURCE_HZ.keys() == PUSH_PROFILE.keys() + + +def test_pi3_push_costs_more_than_pi5(tmp_path, monkeypatch): + # The CPU-bound terms are why the profile is per-SoC at all: an A53 pack is + # ~7x an A76's, and treating them alike over-admits inline pushes on v2. + dt = tmp_path / "compatible" + + dt.write_bytes(b"brcm,bcm2837\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + pi3 = push_profile() + + dt.write_bytes(b"brcm,bcm2712\0") + pi5 = push_profile() + + assert pi3.pipeline_ms_per_px > 5 * pi5.pipeline_ms_per_px + assert pi3.fixed_ms > pi5.fixed_ms + + +def test_unknown_soc_on_a_real_board_gets_the_slow_profile(tmp_path, monkeypatch): + # A Pi 2, or a Pi 6 we've never benched: guessing Pi 5's costs here would + # over-admit inline pushes and stall the poll loop. + dt = tmp_path / "compatible" + dt.write_bytes(b"brcm,bcm9999\0") + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", dt) + assert push_profile() == PUSH_PROFILE["brcm,bcm2837"] + + +def test_off_device_gets_the_default_profile(tmp_path, monkeypatch): + # No device tree at all (mac, emulator, CI) — pair with DEFAULT_SOURCE_HZ. + monkeypatch.setattr("uilib.spi_timing._DT_COMPATIBLE", tmp_path / "absent") + assert push_profile() == DEFAULT_PROFILE + + def test_transfer_ms_scales_with_clock(): slow = transfer_ms(76800, 50_000_000) fast = transfer_ms(76800, 400_000_000 / 6) assert slow > fast # Wire time dominates a full frame: 66.67 MHz should be meaningfully cheaper. - assert fast / slow == pytest.approx(0.79, abs=0.03) + # The floor is the pure wire ratio, 50/66.67 = 0.75; the CPU-bound terms are + # what hold it above that, so this tracks the host's push profile. + assert fast / slow == pytest.approx(0.76, abs=0.03) diff --git a/tools/bench_adc_contention.py b/tools/bench_adc_contention.py new file mode 100644 index 000000000..d79a01eee --- /dev/null +++ b/tools/bench_adc_contention.py @@ -0,0 +1,202 @@ +#!/usr/bin/env python3 +"""Does an in-flight LCD push stall the ADC on the shared SPI bus? + +The LCD (CE0, Blinka) and the MCP3008 (CE1, raw spidev @1MHz) sit on the same +SPI master. Today they never overlap: poll_lcd_updates() and poll_controls() +both run on the UI thread, so the kernel sees one transfer at a time. + +Moving the LCD push to a worker thread (docs/spi_lcd_timing_pi5.md) would remove +a 19-25ms UI-thread stall -- but only if it doesn't simply relocate that stall +into poll_controls(). With spidev.bufsiz=163840 a full frame is ONE kernel SPI +message, and the spi core serializes messages per master: an ADC read issued +mid-frame may wait for the whole payload to clock out. + +This measures ADC xfer2 latency with the LCD idle vs. with a worker thread +pushing frames continuously, and sweeps the LCD payload chunk size -- chunking +releases the bus between writes, letting the ADC interleave, at the cost of more +syscalls. + +Verdict this is looking for: p95/max ADC latency under contention. The UI tick is +10ms; an ADC read that blocks for ~19ms is a worse bug than the one we set out to +fix. + + sudo systemctl stop mod-ala-pi-stomp + PYTHONPATH=/opt/pistomp/pi-stomp python tools/bench_adc_contention.py [baud] +""" + +from __future__ import annotations + +import os +import statistics +import sys +import threading +import time +from pathlib import Path + +# tools/ lands on sys.path[0], not the repo root -- without this, `import uilib` +# finds the copy in the venv's site-packages and benches the packaged driver. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) + +os.environ.setdefault("SDL_VIDEODRIVER", "dummy") + +import pygame # noqa: E402 + +pygame.init() +pygame.display.set_mode((1, 1)) + +import board # noqa: E402 +import digitalio # noqa: E402 +import spidev # noqa: E402 + +from uilib.box import Box # noqa: E402 +from uilib.lcd_ili9341 import LcdIli9341 # noqa: E402 + +BAUD = int(sys.argv[1]) if len(sys.argv) > 1 else 66_666_667 +ADC_CHANNEL = 0 + + +def adc_open() -> spidev.SpiDev: + """Exactly what hardware.init_spi() does.""" + spi = spidev.SpiDev() + spi.open(0, 1) # bus 0, CE1 + spi.max_speed_hz = 1_000_000 + return spi + + +def adc_read(spi: spidev.SpiDev) -> int: + """One MCP3008 single-ended read -- analogcontrol.readChannel().""" + adc = spi.xfer2([1, (8 + ADC_CHANNEL) << 4, 0]) + return ((adc[1] & 3) << 8) + adc[2] + + +def sample_adc(spi: spidev.SpiDev, duration_s: float) -> list[float]: + """Poll the ADC at the real 10ms tick cadence for `duration_s`.""" + out: list[float] = [] + deadline = time.perf_counter() + duration_s + while time.perf_counter() < deadline: + t0 = time.perf_counter_ns() + adc_read(spi) + out.append((time.perf_counter_ns() - t0) / 1e6) + time.sleep(0.010) + return out + + +def report(label: str, samples: list[float]) -> None: + s = sorted(samples) + p = lambda q: s[min(len(s) - 1, int(q * len(s)))] # noqa: E731 + over = sum(1 for v in s if v > 10.0) + print( + f" {label:<34} n={len(s):4d} med={statistics.median(s):6.3f} " + f"p95={p(0.95):7.3f} p99={p(0.99):7.3f} max={max(s):7.3f} ms" + f" >10ms: {over} ({100*over/len(s):.1f}%)" + ) + + +def main() -> int: + lcd = LcdIli9341( + board.SPI(), + digitalio.DigitalInOut(board.CE0), + digitalio.DigitalInOut(board.D6), + digitalio.DigitalInOut(board.D5), + BAUD, + True, + ) + surf = pygame.Surface((320, 240), depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)) + for y in range(240): + pygame.draw.line(surf, (y, 255 - y, (y * 3) % 256), (0, y), (319, y)) + full = Box(0, 0, 320, 240) + + adc = adc_open() + + print(f"LCD @ {lcd.baudrate/1e6:.2f} MHz actual | ADC @ 1 MHz, CE1 | tick = 10 ms\n") + + print("ADC latency, LCD idle (baseline):") + report("idle", sample_adc(adc, 2.0)) + + # --- LCD pushing continuously on a worker thread --- + stop = threading.Event() + frames = [0] + + def pusher(): + while not stop.is_set(): + lcd.update(surf, full) + frames[0] += 1 + + print("\nADC latency, worker thread pushing full frames continuously:") + t = threading.Thread(target=pusher, daemon=True) + t.start() + time.sleep(0.2) # let it get going + contended = sample_adc(adc, 4.0) + stop.set() + t.join(timeout=5) + report("contended (one 153.6kB write)", contended) + print(f" (worker pushed {frames[0]} frames)") + + # --- same, but chunk the payload so the bus is released between writes --- + orig_block = lcd.disp._block + + for chunk_kb in (64, 16, 4): + chunk = chunk_kb * 1024 + + def chunked(x0, y0, x1, y1, data=None, _c=chunk): + if data is None: + return orig_block(x0, y0, x1, y1, data) + for i in range(0, len(data), _c): + # Only the first call sets the address window; the panel + # auto-increments GRAM across subsequent payload writes. + if i == 0: + orig_block(x0, y0, x1, y1, data[i : i + _c]) + else: + _raw_payload(lcd, data[i : i + _c]) + return None + + lcd.disp._block = chunked + stop = threading.Event() + frames = [0] + + def pusher2(): + while not stop.is_set(): + lcd.update(surf, full) + frames[0] += 1 + + t = threading.Thread(target=pusher2, daemon=True) + t.start() + time.sleep(0.2) + s = sample_adc(adc, 3.0) + stop.set() + t.join(timeout=5) + report(f"contended ({chunk_kb}kB chunks)", s) + print(f" (worker pushed {frames[0]} frames)") + + lcd.disp._block = orig_block + adc.close() + return 0 + + +def _raw_payload(lcd: LcdIli9341, data: bytes) -> None: + """Continuation payload write: no address window, DC high, CS asserted.""" + disp = lcd.disp + spi_dev = disp.spi_device + spi = spi_dev.spi + cs = spi_dev.chip_select + dc = disp.dc_pin + fd = spi._spi._spi.handle + + while not spi.try_lock(): + time.sleep(0) + try: + spi.configure( + baudrate=spi_dev.baudrate, polarity=spi_dev.polarity, phase=spi_dev.phase + ) + if cs: + cs.value = spi_dev.cs_active_value + dc.value = 1 + os.write(fd, data) + finally: + if cs: + cs.value = not spi_dev.cs_active_value + spi.unlock() + + +if __name__ == "__main__": + sys.exit(main()) diff --git a/tools/bench_lcd_device.py b/tools/bench_lcd_device.py index 09e7c504e..f5d9f53b7 100644 --- a/tools/bench_lcd_device.py +++ b/tools/bench_lcd_device.py @@ -12,6 +12,12 @@ import sys import time import statistics +from pathlib import Path + +# Running as tools/bench_lcd_device.py puts tools/ on sys.path[0], not the repo +# root -- so `import uilib` finds the copy installed in the venv's site-packages +# and silently benches the *packaged* driver instead of the working tree. +sys.path.insert(0, str(Path(__file__).resolve().parent.parent)) os.environ.setdefault("SDL_VIDEODRIVER", "dummy") @@ -35,7 +41,9 @@ True, ) -surf = pygame.Surface((320, 240)) +# Opaque XRGB, matching PanelStack's root. update()'s 565 convert-blit is ~7x +# slower off an SRCALPHA source, so the fit is only valid on an opaque one. +surf = pygame.Surface((320, 240), depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)) for y in range(240): pygame.draw.line(surf, (y, 255 - y, (y * 3) % 256), (0, y), (319, y)) diff --git a/tools/bench_pack_variants.py b/tools/bench_pack_variants.py index 32cb6dd6a..efef28bff 100644 --- a/tools/bench_pack_variants.py +++ b/tools/bench_pack_variants.py @@ -1,13 +1,23 @@ #!/usr/bin/env python3 """Benchmark RGB565 pack variants for the LCD update path. -Compares the current pygame.image.tobytes() approach against pygame.surfarray -alternatives (pixels3d / array3d) at clip sizes the tuner strobe actually -generates: one stripe edge (~5px), one stripe span (~53px), the unioned strobe -region (~270px), and the full strobe widget (320px). +Compares the production SDL staging-surface blit (LcdIli9341.update) against the +numpy packs it replaced, at clip sizes the tuner strobe actually generates: one +stripe edge (~5px), one stripe span (~53px), the unioned strobe region (~270px), +and the full strobe widget (320px). -Source surface is RGBA (the PanelStack surface format when use_dimming=True). -Both reading and output buffer allocation strategies are tested. +Each size is run against both source formats, because that is the whole question +for the SDL path: + + opaque RGB source (PanelStack's root) — a straight convert blit. ~2.5x faster + than the best numpy pack. + + RGBA source — blitting this into a 565 surface hits SDL's per-pixel + *alpha-blending* blitter, not a format convert, and is ~7x slower than the + opaque blit. This is why the root must stay opaque; see CLAUDE.md "Traps". + +SDL output is native little-endian; the ILI9341 wants high byte first, so the +byteswap is part of the measured cost, not an optimisation to factor out. Run: uv run python tools/bench_pack_variants.py @@ -98,6 +108,35 @@ def pack_pixels3d_colmajor(sub: pygame.Surface, out_col: np.ndarray) -> bytes: return pix.tobytes() # wrong byte order for LCD — for timing only +_staging: dict[tuple[int, int], pygame.Surface] = {} + + +def _stage_565(w: int, h: int) -> pygame.Surface: + """Preallocated RGB565 staging surface, as the driver would hold.""" + s = _staging.get((w, h)) + if s is None: + # masks spelled out — never a bare Surface. See CLAUDE.md "Traps". + s = pygame.Surface((w, h), depth=16, masks=(0xF800, 0x07E0, 0x001F, 0)) + _staging[(w, h)] = s + return s + + +def pack_sdl_blit(sub: pygame.Surface, out: np.ndarray) -> bytes: + """Production path: blit into a 565 staging surface, byteswap its pixels. + + Slicing the sub-rect out of the staging array sidesteps SDL's 4-byte row + padding, which an odd width would otherwise hit. Surface pixels are native + little-endian; the panel wants high byte first. + """ + sw, sh = sub.get_size() + s565 = _stage_565(sw, sh) + s565.blit(sub, (0, 0)) + staged = pygame.surfarray.pixels2d(s565).T + out_bytes = staged[:sh, :sw].byteswap().tobytes() + del staged # unlock the surface + return out_bytes + + def pack_pixels2d(sub: pygame.Surface, out: np.ndarray) -> bytes: """pixels2d: lock the surface as a (width, height) array of mapped pixels. @@ -125,30 +164,27 @@ def pack_pixels2d(sub: pygame.Surface, out: np.ndarray) -> bytes: def _check_all_match() -> None: - rng = np.random.default_rng(7) - # Use a size that's not a power-of-2 to catch stride bugs + # Odd width, non-power-of-2 height: catches stride bugs and the SDL row pad. w, h = 13, 47 - rgba = rng.integers(0, 256, (h, w, 4), dtype=np.uint8) - # Build an RGBA surface from random data - surf = pygame.Surface((w, h), pygame.SRCALPHA) - pygame.surfarray.blit_array(surf, rgba[:, :, :3].transpose(1, 0, 2).copy()) - out = np.empty((h, w, 2), dtype=np.uint8) + for label, surf in (("RGBA", make_rgba_surface(w, h)), ("opaque", make_opaque_surface(w, h))): + out = np.empty((h, w, 2), dtype=np.uint8) + ref = pack_tobytes(surf, out.copy()) - ref = pack_tobytes(surf, out.copy()) - - variants = [ - ("pixels3d_transpose", pack_pixels3d_transpose(surf, out.copy())), - ("pixels3d_contig", pack_pixels3d_contig(surf, out.copy())), - ("array3d", pack_array3d(surf, out.copy())), - ("pixels2d", pack_pixels2d(surf, out.copy())), - ] - for name, result in variants: - assert result == ref, ( - f"{name} mismatch at ({w}x{h}):\n" - f" ref[0:8]={list(ref[:8])}\n got[0:8]={list(result[:8])}" - ) - print("Correctness check passed: all variants produce identical bytes.\n") + variants = [ + ("pixels3d_transpose", pack_pixels3d_transpose(surf, out.copy())), + ("pixels3d_contig", pack_pixels3d_contig(surf, out.copy())), + ("array3d", pack_array3d(surf, out.copy())), + ("pixels2d", pack_pixels2d(surf, out.copy())), + ("sdl_blit", pack_sdl_blit(surf, out.copy())), + ] + for name, result in variants: + assert result == ref, ( + f"{name} mismatch on {label} source at ({w}x{h}):\n" + f" ref[0:8]={list(ref[:8])}\n got[0:8]={list(result[:8])}" + ) + print("Correctness check passed: all variants produce identical bytes " + "(both source formats, odd width).\n") # --------------------------------------------------------------------------- @@ -208,16 +244,26 @@ def _print(r: Result, baseline_us: float | None = None) -> None: # --------------------------------------------------------------------------- -def make_rgba_surface(w: int, h: int) -> pygame.Surface: - """RGBA surface with random pixel data (matches PanelStack format).""" +def _fill_random(surf: pygame.Surface, w: int, h: int) -> pygame.Surface: rng = np.random.default_rng(42) - surf = pygame.Surface((w, h), pygame.SRCALPHA) - data = rng.integers(0, 256, (h, w, 4), dtype=np.uint8) - # blit_array expects (w, h, 3) for RGB - pygame.surfarray.blit_array(surf, data[:, :, :3].transpose(1, 0, 2).copy()) + data = rng.integers(0, 256, (h, w, 3), dtype=np.uint8) + pygame.surfarray.blit_array(surf, data.transpose(1, 0, 2).copy()) return surf +def make_rgba_surface(w: int, h: int) -> pygame.Surface: + """Today's PanelStack root format (use_dimming=True).""" + return _fill_random(pygame.Surface((w, h), pygame.SRCALPHA), w, h) + + +def make_opaque_surface(w: int, h: int) -> pygame.Surface: + """Opaque RGB root: what the SDL blit path would need. Same 8-bit blend + precision as RGBA — a dest alpha channel buys nothing for alpha blending.""" + return _fill_random( + pygame.Surface((w, h), depth=32, masks=(0xFF0000, 0xFF00, 0xFF, 0)), w, h + ) + + def run(sizes: list[tuple[int, int]], iters: int) -> None: MAX_W = max(w for w, h in sizes) MAX_H = max(h for w, h in sizes) @@ -225,19 +271,23 @@ def run(sizes: list[tuple[int, int]], iters: int) -> None: out_col = np.empty((MAX_W, MAX_H, 2), dtype=np.uint8) # col-major (w,h,2) for w, h in sizes: - surf = make_rgba_surface(w, h) - px = w * h - print(f"\n{'='*72}") + px = w * h + print(f"\n{'='*78}") print(f" {w}x{h} ({px:,} px, {px*2:,} B RGB565)") - print(f"{'='*72}") + print(f"{'='*78}") + + rgba = make_rgba_surface(w, h) + opaque = make_opaque_surface(w, h) results = [ - bench("tobytes (baseline)", lambda s=surf: pack_tobytes(s, out), iters), - bench("pixels3d + transpose", lambda s=surf: pack_pixels3d_transpose(s, out), iters), - bench("pixels3d + ascontiguousarray", lambda s=surf: pack_pixels3d_contig(s, out), iters), - bench("array3d + transpose", lambda s=surf: pack_array3d(s, out), iters), - bench("pixels2d + transpose", lambda s=surf: pack_pixels2d(s, out), iters), - bench("pixels3d no-transpose [WRONG]",lambda s=surf: pack_pixels3d_colmajor(s, out_col), iters), + bench("pixels3d + transpose (was PROD)", lambda s=rgba: pack_pixels3d_transpose(s, out), iters), + bench("tobytes", lambda s=rgba: pack_tobytes(s, out), iters), + bench("pixels3d + ascontiguousarray", lambda s=rgba: pack_pixels3d_contig(s, out), iters), + bench("array3d + transpose", lambda s=rgba: pack_array3d(s, out), iters), + bench("pixels2d + transpose", lambda s=rgba: pack_pixels2d(s, out), iters), + bench("sdl blit [RGBA src, as doc]", lambda s=rgba: pack_sdl_blit(s, out), iters), + bench("sdl blit [opaque src] (PROD)", lambda s=opaque: pack_sdl_blit(s, out), iters), + bench("pixels3d no-transpose [WRONG]",lambda s=rgba: pack_pixels3d_colmajor(s, out_col), iters), ] baseline = results[0].median_us @@ -245,7 +295,7 @@ def run(sizes: list[tuple[int, int]], iters: int) -> None: _print(r, baseline_us=None if r is results[0] else baseline) print() - print(f" Speedup summary vs tobytes baseline ({baseline:.1f} µs median):") + print(f" Speedup summary vs the old numpy pack ({baseline:.1f} µs median):") for r in results[1:]: delta = baseline - r.median_us sign = "faster" if delta > 0 else "SLOWER" diff --git a/uilib/lcd_ili9341.py b/uilib/lcd_ili9341.py index 159810f8f..ef61446d0 100644 --- a/uilib/lcd_ili9341.py +++ b/uilib/lcd_ili9341.py @@ -14,7 +14,6 @@ # along with pi-stomp. If not, see . import pygame -import numpy as np from uilib.panel import LcdBase, Box from uilib.spi_timing import actual_spi_hz @@ -79,7 +78,10 @@ def __init__(self, spi, cs_pin, dc_pin, reset_pin, baudrate, flip=True): # Landscape dimensions presented to the UI (matches the panel post-MADCTL). self.width = self.disp.height # 320 self.height = self.disp.width # 240 - self._pixels = np.empty((self.height, self.width, 2), dtype=np.uint8) + # 565 staging surface: SDL converts RGB888->RGB565 in one C blit, far + # cheaper than packing it in numpy. Masks spelled out -- a bare Surface + # inherits the display format. See CLAUDE.md "Traps". + self._565 = pygame.Surface((self.width, self.height), depth=16, masks=(0xF800, 0x07E0, 0x001F, 0)) def _block_fast(self, x0, y0, x1, y1, data=None): """Bypass adafruit_rgb_display's write method to perform a block write @@ -170,9 +172,13 @@ def clear(self): def update(self, image: pygame.Surface, box=None): """Push (a sub-rect of) the composed pygame surface to the LCD. - Converts surface → RGB888 bytes → packed RGB565, writing via Display._block - to bypass PIL. The panel runs landscape-native (MADCTL set in __init__) so - no rotation is needed.""" + Quantises to RGB565 via an SDL convert-blit, writing via Display._block to + bypass PIL. The panel runs landscape-native (MADCTL set in __init__) so no + rotation is needed. + + `image` must be opaque: blitting an SRCALPHA source takes SDL's per-pixel + alpha-blending blitter instead of a straight convert, and is ~7x slower + than the numpy pack this replaced. PanelStack's root is opaque XRGB.""" if self.lock.locked(): logging.debug("LCD update was locked by another thread") self.lock.acquire() @@ -193,14 +199,14 @@ def update(self, image: pygame.Surface, box=None): # Landscape-native: surface coords map straight to the panel address # window, so the RGB565 sub-rect ships row-major with no rotation. sw, sh = sub.get_size() - with profiling.measure("lcd.update:pack"): - arr = pygame.surfarray.pixels3d(sub).transpose(1, 0, 2) - - pix = self._pixels[:sh, :sw] - g = arr[:, :, 1] - pix[:, :, 0] = (arr[:, :, 0] & 0xF8) | (g >> 5) - pix[:, :, 1] = ((g & 0x1C) << 3) | (arr[:, :, 2] >> 3) - pixels_bytes = pix.tobytes() + self._565.blit(sub, (0, 0)) + + # Staging is full-width, so slicing the sub-rect out of it sidesteps + # SDL's 4-byte row padding, which an odd-width rect would otherwise hit. + # Surface pixels are native little-endian; the panel wants high byte first. + staged = pygame.surfarray.pixels2d(self._565).T + pixels_bytes = staged[:sh, :sw].byteswap().tobytes() + del staged # unlock the surface with profiling.measure("lcd.update:_block(SPI)"): self.disp._block(x1, y1, x1 + sw - 1, y1 + sh - 1, pixels_bytes) diff --git a/uilib/panel.py b/uilib/panel.py index 146c61e63..fd80a11a0 100644 --- a/uilib/panel.py +++ b/uilib/panel.py @@ -355,8 +355,6 @@ def __init__( box = Box((0, 0), lcd.dimensions()) if image_format is None: image_format = lcd.default_format() - if use_dimming: - image_format = "RGBA" trace(self, "Panel stack initializing with box=", box) super(PanelStack, self).__init__(box=box, image_format=image_format) diff --git a/uilib/spi_timing.py b/uilib/spi_timing.py index a57467404..78031d83f 100644 --- a/uilib/spi_timing.py +++ b/uilib/spi_timing.py @@ -24,7 +24,7 @@ import math from pathlib import Path -from typing import Optional +from typing import NamedTuple, Optional _DT_COMPATIBLE = Path("/proc/device-tree/compatible") @@ -40,17 +40,22 @@ DEFAULT_SOURCE_HZ: int = 200_000_000 -def spi_source_hz() -> int: - """SPI controller source clock for the running host.""" +def soc_key() -> Optional[str]: + """Device-tree `compatible` entry naming the running SoC, if we know it.""" try: raw = _DT_COMPATIBLE.read_bytes() except OSError: - return DEFAULT_SOURCE_HZ + return None for entry in raw.decode("ascii", errors="replace").split("\0"): - source = SPI_SOURCE_HZ.get(entry) - if source is not None: - return source - return DEFAULT_SOURCE_HZ + if entry in SPI_SOURCE_HZ: + return entry + return None + + +def spi_source_hz() -> int: + """SPI controller source clock for the running host.""" + soc = soc_key() + return DEFAULT_SOURCE_HZ if soc is None else SPI_SOURCE_HZ[soc] def actual_spi_hz(requested_hz: float, source_hz: Optional[int] = None) -> float: @@ -73,32 +78,55 @@ def actual_spi_hz(requested_hz: float, source_hz: Optional[int] = None) -> float divisor += 1 return source / divisor -# Constants fit from on-device timing of LcdIli9341.update() at 20 MHz, 33.3 MHz, -# and 50 MHz actual (Pi 5 / Python 3.14, tools/bench_lcd_device.py). -# -# DW APB SSI BAUDR rounding means only these speeds are achievable: -# request 20 MHz (BAUDR=10) → 20.00 MHz actual -# request 34–49 MHz (BAUDR=6) → 33.33 MHz actual -# request 50–99 MHz (BAUDR=4) → 50.00 MHz actual -# + # The push cost is affine: # fixed per-call + clock-independent per-pixel + bits-on-the-wire / clock # -# Fit holds within ~1% across all three clocks and the full size range. +# fixed and per-pixel are CPU-bound, so they are *per-SoC* -- an A53 at 1.2 GHz is +# nothing like an A76 at 2.4 GHz, and v2's per-pixel cost is 6.2x v3's. Sharing one +# global would let v3's cheap per-pixel cost admit a 31.7k-px clip inline on v2 that +# really costs 9.6 ms, against an 8 ms budget on a 10 ms tick. Underestimating is the +# unsafe direction -- transfer_ms gates inline pushes onto the UI thread +# (panel.py INLINE_BUDGET_MS). +# +# Refit with tools/bench_lcd_device.py, on each board, whenever the pack path changes. + + +class PushProfile(NamedTuple): + fixed_ms: float # address-window commands + Python/driver call overhead + pipeline_ms_per_px: float # 565 convert-blit + driver; no cheaper on a faster clock + bits_per_pixel: float # ~16 (RGB565); the fit converging there means no framing -# Wire bits per pixel: 16 (RGB565). The fit converges very close to 16, -# confirming negligible framing overhead at these clock speeds. -BITS_PER_PIXEL = 16.0071 -# Clock-independent per-pixel cost: numpy 565 packing + driver. Doesn't get -# cheaper with a faster clock, so it floors how fast large pushes can go. -PIPELINE_MS_PER_PX = 5.856e-05 +# Fit from on-device timing of LcdIli9341.update() across three clocks per board +# (Python 3.13, SDL convert-blit pack). Errors: v3 rms 0.4%, v2 rms 3.1%. +_PI3 = PushProfile(1.2008, 2.3647e-05, 15.923) # Pi 3A+ (v2), fit @ 20/40/66.7 MHz + +PUSH_PROFILE: dict[str, PushProfile] = { + "brcm,bcm2837": _PI3, + "brcm,bcm2711": _PI3, # Pi 4 unmeasured; the slower board is the safe guess + "brcm,bcm2712": PushProfile(0.2727, 3.8290e-06, 16.032), # Pi 5 (v3) @ 20/33.3/50 MHz +} -# Fixed per-call cost: address-window commands + Python/driver call overhead. -FIXED_MS = 0.7117 +# Off-device (tests, emulator) — matches DEFAULT_SOURCE_HZ's Pi 5 assumption. +DEFAULT_PROFILE = PUSH_PROFILE["brcm,bcm2712"] + + +def push_profile() -> PushProfile: + """Affine push-cost model for the running host. + + An unrecognised SoC on a *real* board (a Pi 2, a future Pi 6) gets the slowest + profile we have, not the default: overestimating only defers a push to the + worker, while underestimating stalls the poll loop. + """ + soc = soc_key() + if soc is not None: + return PUSH_PROFILE[soc] + return DEFAULT_PROFILE if not _DT_COMPATIBLE.exists() else _PI3 def transfer_ms(pixels: int, spi_hz: float) -> float: """Estimated milliseconds to push `pixels` to an SPI display at `spi_hz`.""" - wire_ms = pixels * BITS_PER_PIXEL / spi_hz * 1000 - return FIXED_MS + pixels * PIPELINE_MS_PER_PX + wire_ms + p = push_profile() + wire_ms = pixels * p.bits_per_pixel / spi_hz * 1000 + return p.fixed_ms + pixels * p.pipeline_ms_per_px + wire_ms