feat: optional hooks around long e-ink BUSY waits#21
Conversation
A refresh takes ~0.3-2 s during which pollBusy() only polls the BUSY pin at full CPU clock. Add a pair of optional plain-function-pointer hooks (setBusyWaitHooks) fired when a wait exceeds 20 ms and when it completes, so host firmware can apply its own power policy (e.g. reduce the CPU clock) for the wait window. No behavior change unless hooks are installed; short command waits never trigger them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a ChangesBusy-Wait Hook Support
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant EInkDisplay
participant BusyPin
participant HookCallbacks
Caller->>EInkDisplay: setBusyWaitHooks(beginHook, endHook)
Caller->>EInkDisplay: pollBusy()
EInkDisplay->>BusyPin: read BUSY state
alt X4 threshold exceeded
EInkDisplay->>HookCallbacks: _busyWaitBeginHook()
else X3 LOW observed
EInkDisplay->>BusyPin: wait for LOW (up to 1s)
EInkDisplay->>HookCallbacks: _busyWaitBeginHook()
EInkDisplay->>BusyPin: wait for HIGH (up to 30s)
end
alt hook fired
EInkDisplay->>HookCallbacks: _busyWaitEndHook()
end
EInkDisplay-->>Caller: return polling result
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
Temporarily references the fork commit backing crosspoint-reader/community-sdk#21; will be re-bumped to the upstream commit once that PR merges. Until then submodule fetch (and therefore CI) cannot resolve the SHA from the upstream URL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Consumer PR with the full context and measurements: crosspoint-reader/crosspoint-reader#2525 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/display/EInkDisplay/include/EInkDisplay.h (1)
130-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the begin/end pairing invariant.
beginHookandendHookare accepted independently. If a caller installs abeginHook(e.g. a downclock) but leavesendHookasnullptr, the begin side effect fires and is never reversed, leaving the host in the reduced-power state indefinitely. The doc comment says both "default to disabled" but doesn't state that a begin hook without a matching end hook is unsafe. Consider documenting that the two must be provided as a pair (or asserting it) to prevent this footgun.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/display/EInkDisplay/include/EInkDisplay.h` around lines 130 - 133, The busy-wait hook setter currently allows beginHook and endHook to be installed independently, which makes it easy to leave the host in an unrecovered state. Update setBusyWaitHooks in EInkDisplay to clearly enforce or document that _busyWaitBeginHook and _busyWaitEndHook must be provided as a pair, and consider asserting or rejecting a beginHook when endHook is nullptr. Also update the nearby API comment for setBusyWaitHooks to state the pairing invariant so callers know the hooks are not meant to be used separately.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/display/EInkDisplay/include/EInkDisplay.h`:
- Around line 130-133: The busy-wait hook setter currently allows beginHook and
endHook to be installed independently, which makes it easy to leave the host in
an unrecovered state. Update setBusyWaitHooks in EInkDisplay to clearly enforce
or document that _busyWaitBeginHook and _busyWaitEndHook must be provided as a
pair, and consider asserting or rejecting a beginHook when endHook is nullptr.
Also update the nearby API comment for setBusyWaitHooks to state the pairing
invariant so callers know the hooks are not meant to be used separately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 01e55239-8b88-4e83-86d8-7f3c28d97810
📒 Files selected for processing (2)
libs/display/EInkDisplay/include/EInkDisplay.hlibs/display/EInkDisplay/src/EInkDisplay.cpp
📜 Review details
🔇 Additional comments (3)
libs/display/EInkDisplay/include/EInkDisplay.h (1)
147-151: LGTM!libs/display/EInkDisplay/src/EInkDisplay.cpp (2)
586-597: 🩺 Stability & Availability | ⚡ Quick winVerify timekeeping stays clock-independent when the begin hook downclocks the CPU.
The intended use is a CPU-clock reduction inside
_busyWaitBeginHook(). After that hook runs, the loop still relies ondelay(1)andmillis()for both theBUSY_WAIT_HOOK_THRESHOLD_MScheck and the 30 s safety timeout. On platforms where these derive from the core clock (e.g. AVR Timer0-basedmillis), a downclock hook would stretch/skew the 30 s cutoff and the poll cadence; on RTC-driven cores (nRF52) it's unaffected. Confirm the supported targets use a clock-independent time source, otherwise the safety timeout guarantee weakens after the hook fires. The same applies to the X3 LOW-phase loop below.
612-633: LGTM!
|
Sorry, not ready yet |
Temporarily references the fork commit backing crosspoint-reader/community-sdk#21; will be re-bumped to the upstream commit once that PR merges. Until then submodule fetch (and therefore CI) cannot resolve the SHA from the upstream URL. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
An e-ink refresh takes ~0.3–2 s, during which
pollBusy()spins on the BUSY pin at whatever clock the host is running. This PR addssetBusyWaitHooks(begin, end): plain function pointers fired when a wait exceeds 20 ms and again when it completes, letting host firmware apply its own power policy for the wait window (CrossPoint uses it to drop the ESP32-C3 to 10 MHz there).nullptr; behavior is unchanged unless installed. The only added cost is a null-check inside the 1 ms poll loop.Measured on an X3 with a Nordic PPK2 at the battery terminals: with CrossPoint installing a 10 MHz downclock hook, a fast page turn drops from ~38 mC to ~29 mC (−24%), with fast/full/grayscale refreshes visually unchanged on device.
Consumer PR with full context and measurements: crosspoint-reader/crosspoint-reader (linked in a comment once opened).
🤖 Generated with Claude Code