feat(eink): add idle hook for refresh-wait#16
Draft
jeremydk wants to merge 3 commits into
Draft
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-idle-hook
branch
from
9657256 to
90c076f
Compare
jeremydk
added a commit
to jeremydk/community-sdk
that referenced
this pull request
May 25, 2026
Doxygen comment on the abstract virtual at Panel.h:135-140 understated the X3 behaviour: it said panels "pick a default" if `lut` is null, which is true for X4 but misses that X3 ignores `lut` regardless of nullability. c740771 fixed the implementation-side comment on X3Panel::displayGrayBuffer but the API-doc surface anyone reading Panel.h would land on first still implied `lut` mattered universally. Spotted by claude-jeremy-perf during the reader-side migration onto PR crosspoint-reader#16; landed here so it'll be in the SDK when the perf branch rebases off PR crosspoint-reader#16's eventual merge.
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).
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.
jeremydk
added a commit
to jeremydk/community-sdk
that referenced
this pull request
May 25, 2026
Two build-time flags (default 1) that let downstream firmware drop a panel from the binary at compile time. Default behaviour is unchanged — both panels compile in and the resulting binary is byte-identical to the PR crosspoint-reader#16 tip. Mechanism: Panel.h defines EINK_PANEL_X3 and EINK_PANEL_X4 with defaults of 1 and #error if both are 0. Each panel's TU (XnPanel.cpp, XnLuts.cpp) and companion header (XnPanel.h, XnLuts.h, XnConstants.h) is whole-file guarded. EInkDisplay's friend declarations, the deprecated setDisplayX3() shim, and the X4-only setRamArea/ writeRamBuffer privates are guarded too. The legacy default constructor now goes through a makeDefaultPanel() factory that picks whichever panel is available (X4 preferred for backcompat). Also drops one latent cross-panel coupling surfaced by the single-X3 build: X3Panel::displayGrayBuffer reached at the X4 lut_factory_fast extern just to tag a log line, and the tag was misleading anyway — both branches loaded the same X3 _full bank. Removed the differentiation and the cross-panel reference; log line now reads X3_GRAY_MODE=factory regardless. Size impact on crosspoint-reader (.bin, post-clean-rebuild): default (both panels): 5,886,272 B baseline single-X3 (-DEINK_PANEL_X4=0): 5,881,520 B -4,752 B single-X4 (-DEINK_PANEL_X3=0): 5,879,936 B -6,336 B Modest at N=2 because -ffunction-sections + --gc-sections already prunes much of the unreachable X3/X4 surface even when both link in. The structural win is that adding panel N never requires editing EInkDisplay.cpp's dispatch — the new TU drops in next to XnPanel.cpp and single-panel builds compile out everything they don't ship. Downstream pattern: any firmware-side caller of a panel-specific symbol (e.g. crosspoint-reader's HalDisplay::begin() call to setDisplayX3()) must mirror the flag. The SDK can't see those sites.
jeremydk
added a commit
to jeremydk/community-sdk
that referenced
this pull request
May 25, 2026
Doxygen comment on the abstract virtual at Panel.h:135-140 understated the X3 behaviour: it said panels "pick a default" if `lut` is null, which is true for X4 but misses that X3 ignores `lut` regardless of nullability. c740771 fixed the implementation-side comment on X3Panel::displayGrayBuffer but the API-doc surface anyone reading Panel.h would land on first still implied `lut` mattered universally. Spotted by claude-jeremy-perf during the reader-side migration onto PR crosspoint-reader#16; landed here so it'll be in the SDK when the perf branch rebases off PR crosspoint-reader#16's eventual merge.
jeremydk
force-pushed
the
feat/panel-abstraction-idle-hook
branch
from
90c076f to
3c488a2
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.
What
Adds an idle hook the SDK invokes during the BUSY-poll loop on every e-ink refresh, so consumers can amortize background work into the 400-700ms window that's otherwise CPU-idle per page turn.
using IdleHook = void ()(void ctx);
static void EInkDisplay::setIdleHook(IdleHook hook, void* ctx);
The polling loop (in each Panel's pollBusy) invokes the hook after each delay(1). Function pointer rather than std::function for binary-size discipline. Single static slot for the whole SDK; installing a second hook replaces the first.
Why
waitForRefresh and waitWhileBusy spend 400-700ms per typical page turn polling the BUSY line in a 1ms delay(1) loop. The CPU is idle that entire time. The reader has work it would otherwise be forced to run after displayBuffer returns; running it during the wait makes the page turn fully cover that latency instead of stacking it.
Caller contract
The tickIdleHook static helper exists so Panel implementations don't need to reach into hook-storage details; one pointer compare per delay(1), no call overhead when no hook is installed.
Dependencies
Stacked on:
Filed as draft until both land. Once they merge, this can be marked ready and the diff stays a clean single-commit add.
Testing
Hardware-verified on both panels with no hook installed (the consumer-side install will come from the reader's chapter-prebuild work).
X4: half=1554ms, fast=418/419ms, EpubReader render cycle clean.
X3: full=926ms, post-full settle=417ms (PR #14 fix firing through the new dispatch), conditional resync=455ms, grayscale (gc bank)=2346ms, fast=378ms.
Timings byte-identical to pre-hook on both panels. Zero ERR/assert/abort.
Image footprint: +60 bytes (the static slot, the setter, three tickIdleHook() call sites). No measurable runtime cost for the null-hook case.