Trim dirty regions to cut I2C traffic and visible tearing#67
Trim dirty regions to cut I2C traffic and visible tearing#67cafedomingo wants to merge 11 commits into
Conversation
DiffRegions previously emitted full-width row strips, so any change in a row resent all 160 columns. Regions now carry X/W: each strip is trimmed to its dirty columns, and clean gaps big enough to pay for another region's command overhead (gap area >= 32px) split the strip, which separates the two-column screen layouts naturally. drawChanged copies non-full-width regions into a contiguous buffer for SendRegion.
drawChanged copied each trimmed region into a contiguous pixel slice (with a full-width fast path) only for the driver to copy it again into a byte buffer. SendRegion/SendFull now take a Region and the framebuffer directly, and the driver serializes the rectangle row by row straight to RGB565 bytes, so the framebuffer layout knowledge stays in the st7735 package, the intermediate copy disappears, and drawChanged collapses to one send per region.
There was a problem hiding this comment.
Pull request overview
This PR reduces I2C traffic (and therefore visible progressive redraw/tearing) by changing the dirty-region diffing and display send path so that only the minimal changed rectangles are serialized and sent to the ST7735 bridge.
Changes:
- Extend dirty regions from full-width row strips to trimmed/splittable rectangles with an x-extent (
Region{X,Y,W,H}) and updateDiffRegionsto emit these rectangles. - Update the ST7735 display interface to send a
Regionplus a framebuffer, moving rectangle serialization into thest7735driver. - Update screen rendering and tests to use the new region-based send path and validate column splitting/serialization.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/st7735/README.md | Updates hardware/protocol/timing docs and documents the new trimmed/splittable dirty-rectangle approach. |
| internal/st7735/framebuffer.go | Adds a shared bytesPerPixel constant used by serialization/tests. |
| internal/st7735/driver.go | Changes the Display interface and implements region-based framebuffer serialization (regionToBytes). |
| internal/st7735/driver_test.go | Updates framing test for the new SendRegion signature and replaces pixelsToBytes tests with regionToBytes coverage. |
| internal/st7735/diff.go | Reworks Region and DiffRegions to compute trimmed rectangles and split on worthwhile clean gaps. |
| internal/st7735/diff_test.go | Expands diff tests to assert trimming, splitting, and union behavior of the new rectangle diffing. |
| internal/screen/screen.go | Collapses drawChanged to send one SendRegion call per diffed region using the back framebuffer. |
| cmd/display/main.go | Updates full-screen blanking to call SendFull with a framebuffer pointer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
After running for awhile, the screen eventually got into an inconsistent state. Seems like we have a bug somewhere. 
|
cafedomingo
left a comment
cafedomingo
left a comment
There was a problem hiding this comment.
I found one blocking reliability issue that lines up with the reported long-run panel desync, plus a focused test gap around the new partial-region send path.
Local verification: git diff --check origin/main...HEAD, go test ./..., the CI coverage test command, arm64 display build, screenshot build/run, screenshot diff check, and shellcheck install.sh passed. golangci-lint run did not complete locally because this machine's installed linter failed while loading Go's unsafe package.
The bridge can misapply a write without surfacing an I2C error, and a diffed send then leaves the panel permanently out of sync with the software front buffer (seen on hardware after ~24h). Redraw() resends the full back buffer and realigns front; the main loop invokes it hourly, bounding how long silent desync can persist for one ~1.3s progressive repaint per interval. Also covers the partial-rectangle send path in the framing test: non-zero X window bytes, payload size, short final chunk, row-strided order, and no leakage of pixels left of the region.
|
Investigated: the ~24h log shows zero I2C write errors, so the bridge misapplied at least one write silently: trimmed regions exercise arbitrary address windows (non-zero x, narrow widths) the old full-width code never sent, and full-width strips had accidental self-healing that trimming removed. Mitigation in 4e2edb0: hourly full-frame Redraw() bounds any silent desync to one hour. A hardware geometry sweep (4,080 draw/erase region ops across widths 1-78 at odd/even x offsets) is on the panel now awaiting inspection; if it pinpoints a trigger geometry a targeted workaround follows. |
|
Forensics complete. Overnight geometry soak: 2.55M trimmed region sends over 12h22m with self-identifying colors, zero I2C errors, zero misplaced pixels: the host send path is exonerated at ~150x the corrupted run's region volume, and the bridge mis-latch did not reproduce under continuous traffic (condition-dependent, not geometric). Read probe: the bridge never ACKs I2C reads (ETIMEDOUT), and the vendor C driver is write-only, so readback-based desync detection is impossible. The hourly Redraw() resync remains the mitigation: the fault is real, silent, and unfixable host-side. |
Redraws were visibly progressive, worst on the sparkline screen: the display bridge tops out around 19 kB/s effective (the MCU clock-stretches ~26µs/byte on top of the 400kHz transfer), so every byte we avoid sending is the only lever. DiffRegions previously emitted full-width row strips, resending all 160 columns for any change in a row.
Regions now carry an x-extent: each strip of dirty rows is trimmed to its dirty columns, and a clean gap splits the strip into side-by-side regions when the gap area is worth the extra command overhead (threshold validated against measured per-region cost of ~2.35ms). The two-column screen layouts split naturally at the x=78..81 gutter.
SendRegion/SendFull now take a Region and the framebuffer, and the driver serializes the rectangle row by row straight to RGB565 bytes. This keeps framebuffer layout knowledge in the st7735 package, avoids an intermediate contiguous copy in the screen layer, and drawChanged collapses to one send per diffed region.
Simulated over 60 ticks of changing sparkline values: average tick drops from ~13.6 kB to ~7.4 kB, roughly 710ms to 400ms of bus time. Verified on the physical panel.
Two dead ends worth recording: the bridge does not latch pixel data on the sync register (deferring all syncs changes nothing visually, so atomic flushes are impossible), and the 450µs inter-chunk delay cannot be tightened (the historical tuning went through time.Sleep overshoot, so ~1.1ms actual gap is the proven floor).