Add display PM locks for auto light sleep - #12
Conversation
…display-locks # Conflicts: # libs/display/FreeInkDisplay/src/FreeInkDisplay.cpp
|
@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
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
|
@itsthisjustin remerged main due to conflict |
|
@KnicKnic sorry another conflict after recent merges |
obey-agent
left a comment
There was a problem hiding this comment.
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
- CS held across
waitBusy—Ed2208M5Driver::powerOn/powerOffstill keep aTransaction(CS low + APB lock) open whilewaitBusycan 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). - Nested lock types — Facade
NO_LIGHT_SLEEP+ busNO_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
waitRefreshCompleteremoves 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.
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. |
|
@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.
|
Summary
CONFIG_PM_ENABLEgated PM locks: display operations use no-light-sleep locks around panel work that must not be interrupted by automatic light sleep, andEpdBususes anESP_PM_APB_FREQ_MAXlock around Arduino SPI setup/transactions.EpdBus::Transactionreturned by publicEpdBus::beginTxn()for grouped workflows.beginTxn()preserves the existing grouped transaction select ordering, selects the panel, and returns a transaction that ownsSPI.beginTransaction()/SPI.endTransaction()plus the APB PM lock. Grouped callers write throughtxn.cmd(),txn.data(), andtxn.writeBytes().EpdBus::cmd(),data(), andcmdData()use the privatetransaction()helper and preserve the original standalone pin ordering from the merge-base implementation rather than routing through publicbeginTxn().origin/main(421d75d) into this branch at SDK commite6daae5. The merge kept main's async no-shadow/deferred-refresh support and retained the display PM lock around async refresh workflows.cmd/data/cmdDatapaths intentionally do not perform the grouped_coCshandling frombeginTxn(), matching the original branch behavior. The APB lock is still held through the private transaction wrapper.Additional Context
SPIClass, which goes through Arduino HAL register transactions instead of the ESP-IDFspi_masterdriver path that would create SPI PM locks automatically.SPI.begin()andSPI.beginTransaction(), so this PR holds APB max around both the bus bring-up calculation and every display transaction.EpdBusso 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.c55d425e, which references FreeInk SDK commite6daae5.PYTHONIOENCODING=utf-8 platformio run -e default --jobs 1 --disable-auto-cleanfrom the CrossPoint checkout. Result: success, 22:12 for the latest main-merge revision.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