Skip to content

Add display PM locks for auto light sleep - #12

Open
KnicKnic wants to merge 14 commits into
Free-Ink:mainfrom
KnicKnic:knicknic/issue-11-pm-display-locks
Open

Add display PM locks for auto light sleep#12
KnicKnic wants to merge 14 commits into
Free-Ink:mainfrom
KnicKnic:knicknic/issue-11-pm-display-locks

Conversation

@KnicKnic

@KnicKnic KnicKnic commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • What is the goal of this PR? Implements the FreeInk SDK side of Optimize code for power management & Auto light sleep #11 by preventing automatic light sleep while display operations are active and by keeping APB at max during EPD SPI setup/transfers.
  • What changes are included? Adds Arduino + CONFIG_PM_ENABLE gated PM locks: display operations use no-light-sleep locks around panel work that must not be interrupted by automatic light sleep, and EpdBus uses an ESP_PM_APB_FREQ_MAX lock around Arduino SPI setup/transactions.
  • API change: Adds a move-only RAII EpdBus::Transaction returned by public EpdBus::beginTxn() for grouped workflows. beginTxn() preserves the existing grouped transaction select ordering, selects the panel, and returns a transaction that owns SPI.beginTransaction() / SPI.endTransaction() plus the APB PM lock. Grouped callers write through txn.cmd(), txn.data(), and txn.writeBytes().
  • Standalone command/data behavior: EpdBus::cmd(), data(), and cmdData() use the private transaction() helper and preserve the original standalone pin ordering from the merge-base implementation rather than routing through public beginTxn().
  • Latest update: Merged current origin/main (421d75d) into this branch at SDK commit e6daae5. The merge kept main's async no-shadow/deferred-refresh support and retained the display PM lock around async refresh workflows.
  • Latest correction: The standalone cmd/data/cmdData paths intentionally do not perform the grouped _coCs handling from beginTxn(), matching the original branch behavior. The APB lock is still held through the private transaction wrapper.

Additional Context

  • The APB lock is SDK-owned because the X3 display path uses Arduino SPIClass, which goes through Arduino HAL register transactions instead of the ESP-IDF spi_master driver path that would create SPI PM locks automatically.
  • Arduino SPI computes dividers in SPI.begin() and SPI.beginTransaction(), so this PR holds APB max around both the bus bring-up calculation and every display transaction.
  • Long e-paper BUSY waits intentionally do not hold the APB max lock once the SPI transaction has ended. That should be reviewed per wait site: in some places this is desired because the panel is driving itself, but call sites should make it clear when dropping APB max during a wait is acceptable.
  • Future cleanup worth considering: rearchitect EpdBus so it is easier to see when a PM lock is being taken, and so callers naturally chain sequential bus operations inside one transaction when those operations should keep the APB lock continuously held.
  • This PR preserves the existing grouped transaction-select ordering: co-resident CS high, then SPI transaction begin, then panel CS low. If we decide it is safe to change that ordering, more of the CS enablement could move directly into the transaction constructor, but I did not want to risk changing panel/bus behavior in this PR.
  • Tested against CrossPoint integration branch: https://github.com/KnicKnic/crosspoint-reader/tree/knicknic/test-pm-autosleep at commit c55d425e, which references FreeInk SDK commit e6daae5.
  • Validation run: PYTHONIOENCODING=utf-8 platformio run -e default --jobs 1 --disable-auto-clean from the CrossPoint checkout. Result: success, 22:12 for the latest main-merge revision.
  • Fixes Optimize code for power management & Auto light sleep #11.

AI Usage

While FreeInk does not currently include a PR template, I am mirroring the CrossPoint disclosure format for transparency.

Did you use AI tools to help write this code? YES

I leveraged AI assistance with Codex to draft and iterate on the PM lock placement, RAII transaction refactor, merge resolution, and PR notes; I reviewed and tested the resulting changes locally.

here is an image of a test build I created with this change, the locks appear at the last big chunk at the bottom, showing render on my x3 roughly 1/8 uses apb max frequency lock

IMG_0548

@KnicKnic
KnicKnic marked this pull request as draft July 13, 2026 04:32
@KnicKnic
KnicKnic marked this pull request as ready for review July 13, 2026 15:59
KnicKnic added 2 commits July 13, 2026 09:25
…display-locks

# Conflicts:
#	libs/display/FreeInkDisplay/src/FreeInkDisplay.cpp
@itsthisjustin

Copy link
Copy Markdown
Contributor

@KnicKnic there's some conflicts here after a recent PR. Also wanted to ask if this is applicable to x4? I only saw x3 in your notes

…display-locks

# Conflicts:
#	libs/display/FreeInkDisplay/src/FreeInkDisplay.cpp
#	libs/display/FreeInkDisplay/src/bus/EpdBus.cpp
@KnicKnic

KnicKnic commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@KnicKnic there's some conflicts here after a recent PR. Also wanted to ask if this is applicable to x4? I only saw x3 in your notes

I only have an x3 to use. The code should be x4 compatible. I put the branch I forked to test with, so it is available if you want to test with x4. It should be compatible with esp32-s3 devices as well. The biggest difference is they are dual core devices. Depending on the x4pro ship date & thickness I'll probably get one of those .

Other than concerns around multithreading and locking. My assumption was that most of the display code is not reentrant and that the callers should apply appropriate memory fences. Is there guidance around this?

…display-locks

# Conflicts:
#	libs/display/FreeInkDisplay/src/FreeInkDisplay.cpp
@KnicKnic

Copy link
Copy Markdown
Contributor Author

@itsthisjustin remerged main due to conflict

@itsthisjustin

Copy link
Copy Markdown
Contributor

@KnicKnic sorry another conflict after recent merges

@obey-agent obey-agent 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.

Review (obey-agent)

RAII EpdBus::Transaction + dual PM locks (ESP_PM_APB_FREQ_MAX for SPI, ESP_PM_NO_LIGHT_SLEEP for rail/BUSY windows) is the right shape for DFS/light-sleep safety. Facade-level DisplayPmLock covers the higher-level display ops that don’t always go through a single bus helper. Move semantics on Transaction/PmLockGuard look correct (moved-from does not double-release).

Findings

No definite bugs relative to the prior beginTxn/endTxn model.

Suggestions

  1. CS held across waitBusyEd2208M5Driver::powerOn/powerOff still keep a Transaction (CS low + APB lock) open while waitBusy can run for a long time. Pre-existing pattern, but with co-resident SPI devices (_coCs) and power goals this is worth eventually ending the txn before the busy wait (cmd only needs a short CS pulse).
  2. Nested lock types — Facade NO_LIGHT_SLEEP + bus NO_LIGHT_SLEEP + SPI APB locks are separate handles (refcounted); fine, just be aware refreshes hold multiple locks for the full op duration.

Nits

  • Comment stripping in waitRefreshComplete removes useful edge-case documentation; consider restoring the light-sleep/ISR hazard note even if the code path is unchanged.

Verdict: COMMENT — good structural fix for issue #11; no REQUEST_CHANGES.

@KnicKnic

KnicKnic commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@KnicKnic sorry another conflict after recent merges

I'll try to get it updated in the next 24 hours. On the plus side I upgraded my x3 for an x4pro, all the code works. I'll fix the comment removal as well.

@KnicKnic

KnicKnic commented Aug 5, 2026

Copy link
Copy Markdown
Contributor Author

@itsthisjustin fixed the merge. I also picked up 8279 which need a fix (glad I moved operations to force compile breaks).

Ideally what operations are exposed on bus would be slightly reconfigured.

  • Return objects which represent the lifetime of the lock and expose existing operations on those to encourage and allow the user to collapse the number of locks taken.
    • This optimization of minimizing the number of power management locks that are taken is an exercise left for a future exercise.
    • It has no impact to everyone that is not currently using power management, and I currently don't care about those cycles.

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.

Optimize code for power management & Auto light sleep

3 participants