Skip to content

feat: optional hooks around long e-ink BUSY waits#21

Open
BrianPugh wants to merge 1 commit into
crosspoint-reader:mainfrom
BrianPugh:feature/busy-wait-hooks
Open

feat: optional hooks around long e-ink BUSY waits#21
BrianPugh wants to merge 1 commit into
crosspoint-reader:mainfrom
BrianPugh:feature/busy-wait-hooks

Conversation

@BrianPugh

Copy link
Copy Markdown

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 adds setBusyWaitHooks(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).

  • Fully backwards compatible: both hooks default to nullptr; behavior is unchanged unless installed. The only added cost is a null-check inside the 1 ms poll loop.
  • The 20 ms threshold means short command waits never fire the hooks, so hosts don't pay two clock switches on trivial waits. The end hook fires only if the begin hook did.
  • Covers both the X4 (active-HIGH) and X3 (active-LOW, two-phase) wait paths, including the X3 early-return path.

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

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 <[email protected]>
@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

Adds a setBusyWaitHooks API to EInkDisplay for registering optional begin/end callback function pointers, along with a threshold constant. pollBusy is updated to invoke these hooks during long BUSY-pin waits, and X3 polling logic is reworked into separate LOW-transition and HIGH-completion phases.

Changes

Busy-Wait Hook Support

Layer / File(s) Summary
Hook registration API
libs/display/EInkDisplay/include/EInkDisplay.h
Adds setBusyWaitHooks method, BUSY_WAIT_HOOK_THRESHOLD_MS constant, and private begin/end hook pointer members.
pollBusy hook integration and X3 rework
libs/display/EInkDisplay/src/EInkDisplay.cpp
Wires begin/end hook invocation into X4 polling based on a threshold, and splits X3 polling into a LOW-transition wait followed by a HIGH-completion wait, suppressing completion logging when LOW is never observed.

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
Loading
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: optional hooks for long e-ink BUSY waits.
Description check ✅ Passed The description is directly related to the patch and accurately explains the new busy-wait hooks and their behavior.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

BrianPugh added a commit to BrianPugh/crosspoint-reader that referenced this pull request Jul 2, 2026
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 <[email protected]>
@BrianPugh

BrianPugh commented Jul 2, 2026

Copy link
Copy Markdown
Author

Consumer PR with the full context and measurements: crosspoint-reader/crosspoint-reader#2525

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
libs/display/EInkDisplay/include/EInkDisplay.h (1)

130-133: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clarify the begin/end pairing invariant.

beginHook and endHook are accepted independently. If a caller installs a beginHook (e.g. a downclock) but leaves endHook as nullptr, 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

📥 Commits

Reviewing files that changed from the base of the PR and between 198ad26 and 0a899b1.

📒 Files selected for processing (2)
  • libs/display/EInkDisplay/include/EInkDisplay.h
  • libs/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 win

Verify 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 on delay(1) and millis() for both the BUSY_WAIT_HOOK_THRESHOLD_MS check and the 30 s safety timeout. On platforms where these derive from the core clock (e.g. AVR Timer0-based millis), 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!

@BrianPugh BrianPugh closed this Jul 2, 2026
@BrianPugh

Copy link
Copy Markdown
Author

Sorry, not ready yet

BrianPugh added a commit to BrianPugh/crosspoint-reader that referenced this pull request Jul 2, 2026
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 <[email protected]>
@BrianPugh BrianPugh reopened this Jul 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant