refactor: introduce Panel hierarchy, retire _x3Mode (additive)#15
Draft
jeremydk wants to merge 2 commits into
Draft
refactor: introduce Panel hierarchy, retire _x3Mode (additive)#15jeremydk wants to merge 2 commits into
jeremydk wants to merge 2 commits into
Conversation
On X3 (UC81xx) with anti-aliasing off, the first fast/half differential after a full refresh garbled, bleeding the prior content: opening the reader menu or the first page turn after entering a book, and the first turn after the periodic full-refresh cadence. The controller's post-full state corrupts the next differential; it is not a DTM1-content problem (promoting that op to a half did not help, a full did). After any X3 full, spend that corrupt slot with a no-op fast of the just-displayed frame. DTM1 and DTM2 both hold it, so nothing visibly changes, but the controller is left in the post-fast state and the caller's next differential renders clean.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jeremydk
force-pushed
the
feat/panel-abstraction
branch
from
4fe0057 to
52170b7
Compare
jeremydk
added a commit
to jeremydk/community-sdk
that referenced
this pull request
May 25, 2026
Port of the original feat/eink-idle-hook commit (9df4199) onto the Panel-virtualized SDK. Same public API and contract; the call sites moved because pollBusy is no longer a single method on EInkDisplay. waitForRefresh / waitWhileBusy poll the BUSY line in a 1 ms delay loop for 400-700 ms per typical page turn. That window is CPU-idle and a natural slot for amortizing background work the reader is otherwise forced to run after displayBuffer returns (chunked next-chapter indexing, for example). setIdleHook installs a C-style void(void*) callback + context; the polling loops invoke it after each 1 ms poll. The hook is a function pointer (not std::function) to keep binary cost flat across SDK consumers, and fires on whatever task drove the wait into the SDK (the render task in the existing reader architecture). Caller contract: the hook must return within ~50-100 ms (it delays BUSY detection by exactly that much), and must be safe to call from the calling task. Returning immediately when there is no work to do (one cheap pointer check) is the common case. Differences vs the original commit: * Storage stays on EInkDisplay (single static slot for the whole SDK — installing a second hook replaces the first; same as pre-refactor). * setIdleHook / tickIdleHook are public static methods on EInkDisplay. tickIdleHook is the per-loop tick point that each panel's pollBusy invokes; it's `static inline` so the null-hook check is one pointer compare with no call overhead. * X4Panel::pollBusy gets 1 tick call (single-phase HIGH-wait). X3Panel::pollBusy gets 2 tick calls (two-phase HIGH→LOW→HIGH wait). Total 3 call sites vs 3 in the pre-refactor monolithic pollBusy — identical poll cadence. Built on top of the Panel-abstraction branch (PR crosspoint-reader#15). Image +60 bytes for the 3 tick call sites, the static slot, and the setter body. Hardware verified both panels: * X4: half=1554ms, fast=418/419ms, EpubReader render cycle clean. Timings identical to pre-hook (the null hook check is a no-op). * X3: full=926ms, post-full settle=417ms, conditional resync 455ms, grayscale (gc bank) 2346ms, fast=378ms. Same. Zero ERR/assert/abort on either device.
EInkDisplay was a runtime-switched monolith — a `_x3Mode` bool plus a fistful of `if (_x3Mode) ...` branches scattered across init, refresh, display, grayscale, and busy-poll code paths. Adding a third panel would have meant editing every one of those branches. This refactor introduces an abstract `Panel` base class with concrete X3Panel and X4Panel subclasses; EInkDisplay owns a `unique_ptr<Panel>` and dispatches device-specific work through 18 virtual methods. What's in this commit: - **Panel hierarchy**: new `Panel.h` with abstract base + X4Panel declaration; `X3Panel.h` for the X3-specific helper surface that's too large to inline. Concrete impls live in `X3Panel.cpp` / `X4Panel.cpp` alongside their controller-specific LUT banks (`X3Luts.cpp` / `X4Luts.cpp`) and command opcodes (`X3Constants.h` / `X4Constants.h`). - **Panel selection at construction**: new `EInkDisplay(unique_ptr<Panel>, sclk, mosi, cs, dc, rst, busy)` ctor. Legacy pin-only ctor still works (defaults to X4Panel for backcompat); `setDisplayX3()` is preserved as a `[[deprecated]]` shim that swaps the panel in place so existing firmware can migrate at its own pace. - **Friend-based access**: each Panel subclass is a `friend class` of EInkDisplay so device-specific code can reach into SPI/pin state directly. The alternative (a DisplayContext interface) was evaluated and rejected — see the design note at the top of `Panel.h` for the four-point rationale (hot-path perf without whole-program LTO, bounded panel set, same-author maintenance, EInkDisplay and Panel co-evolve). - **Scoped-enum API**: `RefreshMode` and `GrayPlane` promoted from C-style `enum` to `enum class : uint8_t`. The underlying-type pin locks any cache structure holding these enums at one byte (vs the 4-byte `int` default). Use sites qualify as `RefreshMode::FULL_REFRESH` / `GrayPlane::GRAY_PLANE_LSB` etc. Value names kept (no rename to `Full` / `Half` / `Fast`) to minimize churn at call sites; cosmetic rename can be a separate later pass. - **X3 state machine on X3Panel**: the resync flags (`_x3RedRamSynced`, `_x3InitialFullSyncsRemaining`, `_x3ForceFullSyncNext`, `_x3ForcedConditionPassesNext`, `_x3GrayState`) move to X3Panel so X4 builds carry none of it. `requestResync()` / `skipInitialResync()` on EInkDisplay become thin forwarders to `_panel->...`. Net: EInkDisplay shrinks from 1813 lines to ~390. Adding a third panel from here is a new TU + a friend declaration; no edits to EInkDisplay's dispatch. Hardware-verified on both X3 (port 101) and X4 (port 2101).
jeremydk
force-pushed
the
feat/panel-abstraction
branch
from
52170b7 to
5d532e8
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
EInkDisplay carries per-device behavior in 130+ scattered if (_x3Mode) branches plus six X3-only state members on the base class. The X3-only SPI helpers (sendCommandDataX3, loadLutBankX3*,
etc.) live alongside X4-only methods. Adding a third panel today requires editing EInkDisplay at ~50 sites. The flag-based dispatch pattern doesn't scale.
Approach
Introduce a Panel hierarchy that owns all per-device knowledge. EInkDisplay becomes a coordinator (framebuffer, SPI bus, pins, Panel delegate). Concrete panels (X4Panel, X3Panel) implement 22 virtuals covering geometry, lifecycle, refresh dispatch, grayscale paths, LUT loading, state-machine hooks, and busy-line polling.
Panel subclasses reach into EInkDisplay's SPI/pin/state privates via a friend class declaration rather than going through a DisplayContext interface. Rationale documented at the top of Panel.h: vtable-elision concerns on the C3 dominate for SPI-bus and busy-poll hot paths, the panel set is bounded (2 today, maybe a small handful ever), same-author maintenance, EInkDisplay and Panel co-evolve. Open to pushback if a reviewer wants the explicit-interface form.
What's in this PR
Panel hierarchy with these virtual concerns:
setCustomLUT
Concrete panels:
Data co-located with consumers:
EInkDisplay becomes:
Migration shim for downstream consumers:
Dropped:
What's NOT in this PR (deliberately deferred)
Panel-aware shape
Dependencies
Built on top of #14 (X3 entry-ghost fix). Slice 0 of this branch is 6e89dcc. The post-full settle pass introduced in #14 is preserved as part of X3Panel::displayBuffer. When #14 lands, the
base auto-rebases to master and the diff stays clean.
Filed as a draft while #14 is in review. Ready to mark ready-for-review once #14 merges.
Testing
Hardware-verified on both X3 and X4 throughout. Every behavior migration was flashed to both panels and the boot + refresh + EpubReader render flow exercised before moving to the next.
X4:
X3:
Zero ERR/assert/abort/panic on either device. Timings byte-identical to pre-refactor at every checkpoint.
Reviewer suggested reading order
The opcode/LUT headers (X3Constants.h, X4Constants.h, X3Luts.h, X4Luts.h) are pure data moves from EInkDisplay.cpp; not interesting to read line-by-line.