feat: lower CPU freq on idle, add HalPowerManager#852
feat: lower CPU freq on idle, add HalPowerManager#852osteotek merged 21 commits intocrosspoint-reader:masterfrom
Conversation
|
Does it have any noticeable difference in reading mode? Like increased time to turn pages? |
|
@osteotek No I don't see any additional delays, the frequency change is instantly |
|
Found a quite nice article which confirms my research: https://robdobson.com/2023/11/investigating-esp32-c3-power-management/ TL;DR: delay loop 10ms consumes 20.1mA, and 10MHz bring it all the way down to 7.4mA The CPU is now x16 times slower, and since our loop is not an empty loop, the delay of 50ms is worth keeping in this case, as it allows the CPU to go to lower-power mode (the WFI) for most of the time |
|
Testing this out today on my device. So far, my initial impressions are that I see no noticeable change in the on-device activities (which is GREAT 😄). If I notice anything throughout the day, I'll post back in here. Otherwise, seems great so far 👍 |
Ugh, had I waited 5 more minutes to post this... In the first time section file creation of any EPUB, if the chapter is large (I have a book with a chapter of 279 pages), low-power mode will be entered part way through and cause a significant slow down in processing the rest of the section. Here's the relevant logs: The EPUB I was testing with came from here: #757 (comment) but any EPUB with a large number of pages in a chapter will likely cause this. |
|
Hmm right, fixing it now, should not be too complicated.
|
|
@jdk2pq sorry I just pushed one more fix, should be good now (testing on my side too, so far so good) |
Haven't noticed any issues! I didn't notice a drain in battery % while in sleep before this change, and with this change, my battery % is still at what it was last night before I put the device to sleep, so it seems like it's working well to me! |
📝 WalkthroughWalkthroughPower management was migrated from HalGPIO to a new HalPowerManager. HalGPIO had deep-sleep and battery APIs removed. HalPowerManager implements CPU frequency scaling, deep-sleep coordination, battery reading, an RAII Lock, and is integrated across main and activity render loops. Changes
Sequence Diagram(s)mermaid Main->>PowerMgr: begin() Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Fix all issues with AI agents
In `@lib/hal/HalPowerManager.cpp`:
- Around line 31-33: Fix the typo in the comment that references the wrong
symbol name: change "currentLockMod" to "currentLockMode" in the comment above
the read of currentLockMode (the line referencing currentLockMod next to the
declaration const LockMode mode = currentLockMode). Ensure the comment matches
the actual variable name currentLockMode and rebuild.
- Around line 72-83: When acquiring the lock in HalPowerManager::Lock::Lock(),
after setting powerManager.currentLockMode = NormalSpeed and valid = true, also
ensure the CPU frequency is restored immediately by calling
setPowerSaving(false) if powerManager.isLowPower is true; perform this call
while holding the same modeMutex (i.e., before xSemaphoreGive) so the transition
is immediate and synchronized with currentLockMode to prevent races.
In `@lib/hal/HalPowerManager.h`:
- Around line 42-49: The Lock RAII class can be copied/moved which risks
double-release; make it non-copyable and non-movable by deleting the copy
constructor, copy assignment operator, move constructor and move assignment
operator in the Lock class declaration (class Lock) so instances cannot be
copied or moved; keep HalPowerManager friend as-is and ensure only the explicit
Lock() and ~Lock() remain public.
In `@src/activities/home/HomeActivity.cpp`:
- Line 6: Remove the unused include of HalPowerManager.h from HomeActivity.cpp:
locate the line containing `#include` <HalPowerManager.h> and delete it
(HomeActivity.cpp related to class HomeActivity), ensuring no other references
to HalPowerManager exist in HomeActivity.cpp or HomeActivity.h; run a quick
compile to confirm no missing symbols after removal.
In `@src/activities/reader/EpubReaderActivity.cpp`:
- Line 6: The HalPowerManager.h include is unused or the render() method's heavy
work (notably createSectionFile()) needs protection; either remove the
HalPowerManager include or wrap the heavy processing in render() with a
HalPowerManager::Lock to prevent CPU frequency scaling during EPUB section
creation. Locate the render() method and around the block that calls
createSectionFile() (and other expensive operations) instantiate a
HalPowerManager::Lock scoped object before the work and let it release on scope
exit, or if you choose not to use power management remove the HalPowerManager.h
include and any references to it to avoid unused headers.
In `@src/activities/reader/XtcReaderActivity.cpp`:
- Line 12: Remove the unused include: delete the line that includes
HalPowerManager.h since renderPage() does pixel rendering and the file does not
reference powerManager or HalPowerManager::Lock; ensure no other symbols from
HalPowerManager (e.g., powerManager, HalPowerManager::Lock) are referenced in
this file so the build remains clean.
🧹 Nitpick comments (2)
src/activities/settings/OtaUpdateActivity.h (1)
36-37: GateskipLoopDelay()to active update states to avoid idle power drain.Line 37 forces no delay even when the user is just selecting Wi‑Fi or waiting for confirmation. Consider mirroring the
preventAutoSleep()condition so idle UI states still benefit from power saving.💡 Suggested change
- bool skipLoopDelay() override { return true; } // Prevent power-saving mode + bool skipLoopDelay() override { + return state == CHECKING_FOR_UPDATE || state == UPDATE_IN_PROGRESS; + } // Prevent power-saving mode during active OTAlib/hal/HalPowerManager.h (1)
12-13: Duplicateexterndeclaration.The
extern HalPowerManager powerManager;declaration appears twice (lines 13 and 52). Remove one of them to avoid redundancy.♻️ Suggested fix
class HalPowerManager; -extern HalPowerManager powerManager; // Singleton class HalPowerManager {Keep only the declaration at line 52 (after the class definition).
Also applies to: 52-52
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
lib/hal/HalGPIO.cpplib/hal/HalGPIO.hlib/hal/HalPowerManager.cpplib/hal/HalPowerManager.hsrc/activities/home/HomeActivity.cppsrc/activities/reader/EpubReaderActivity.cppsrc/activities/reader/TxtReaderActivity.cppsrc/activities/reader/XtcReaderActivity.cppsrc/activities/settings/ClearCacheActivity.hsrc/activities/settings/OtaUpdateActivity.hsrc/main.cpp
💤 Files with no reviewable changes (2)
- lib/hal/HalGPIO.h
- lib/hal/HalGPIO.cpp
🧰 Additional context used
🧬 Code graph analysis (3)
src/activities/settings/ClearCacheActivity.h (1)
src/activities/settings/OtaUpdateActivity.h (1)
skipLoopDelay(37-37)
lib/hal/HalPowerManager.cpp (1)
lib/hal/HalPowerManager.h (1)
Lock(42-49)
lib/hal/HalPowerManager.h (1)
lib/hal/HalPowerManager.cpp (10)
begin(13-18)begin(13-13)setPowerSaving(20-53)setPowerSaving(20-20)startDeepSleep(55-65)startDeepSleep(55-55)getBatteryPercentage(67-70)getBatteryPercentage(67-67)Lock(72-83)Lock(85-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🔇 Additional comments (8)
src/activities/settings/ClearCacheActivity.h (1)
16-16: LGTM!The addition of
skipLoopDelay()follows the established pattern fromOtaUpdateActivityand appropriately prevents power-saving mode during cache clearing operations. This avoids potential slowdowns from CPU frequency reduction during file system I/O, which aligns with the fixes discussed in the PR for similar processing-intensive activities.src/main.cpp (3)
6-6: Good power management integration.The integration of
HalPowerManagerinmain.cppfollows a clean pattern:
- Initialized early in
setup()viapowerManager.begin()- Power saving disabled on user activity
- Power saving enabled after
IDLE_POWER_SAVING_MSof inactivity- Properly disabled during active operations (
skipLoopDelay)Also applies to: 287-287
396-398: LGTM: User activity resets power saving.Correctly disables power saving on any button press/release or when an activity prevents auto-sleep. This ensures responsive behavior during user interaction.
431-443: LGTM: Power saving control in loop delay logic.The logic properly:
- Disables power saving when
skipLoopDelay()is true (e.g., webserver active)- Enables power saving only after extended idle (
IDLE_POWER_SAVING_MS)- Uses longer delay (50ms) in low-power mode vs short delay (10ms) for responsiveness
lib/hal/HalPowerManager.cpp (3)
13-18: LGTM: Initialization logic.Properly initializes the battery GPIO, captures the baseline CPU frequency, and creates the mutex with an assertion for failure detection.
55-65: LGTM: Deep sleep implementation.Correctly waits for power button release before arming the GPIO wakeup trigger, preventing immediate wake-up if the button is still held.
67-70: Code compiles correctly, const usage is safe.
readPercentage()is a const method onBatteryMonitor(external library from open-x4-sdk). The code successfully compiles, which proves the const usage is safe and not problematic.
BAT_GPIO0initialization is properly sequenced:HalPowerManager::begin()callspinMode(BAT_GPIO0, INPUT)during application setup (insrc/main.cpp), before any battery reads occur.No changes needed.
Likely an incorrect or invalid review comment.
src/activities/reader/TxtReaderActivity.cpp (1)
4-4: Remove unused includeHalPowerManager.h.
#include <HalPowerManager.h>is included but never used in this file. There are no calls topowerManagerorHalPowerManager::Lockanywhere in TxtReaderActivity.cpp, despite the similar include appearing in other reader activity files. Remove the include.
|
Alright, since #774 is merged, I did a final clean up pass, should be good now. Testing on my side, still working as expected (CPU is kept on performance mode during long epub parsing) |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/hal/HalPowerManager.h (1)
12-13: Duplicate extern declaration.
extern HalPowerManager powerManager;appears at both line 13 and line 58. While not harmful, only one declaration is needed. Consider keeping just one, typically after the class definition.🧹 Suggested fix
class HalPowerManager; -extern HalPowerManager powerManager; // Singleton class HalPowerManager {Also applies to: 58-58
lib/hal/HalPowerManager.cpp (1)
37-40: Consider LOG_WARN for frequency change failures.Failed CPU frequency changes are logged at DEBUG level. Since these failures could indicate a real problem (e.g., invalid frequency value), consider using LOG_WARN to ensure visibility in production.
Also applies to: 45-48
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/hal/HalPowerManager.cpplib/hal/HalPowerManager.h
🧰 Additional context used
🧬 Code graph analysis (1)
lib/hal/HalPowerManager.cpp (1)
lib/hal/HalPowerManager.h (1)
Lock(42-55)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cppcheck
- GitHub Check: build
🔇 Additional comments (5)
lib/hal/HalPowerManager.h (1)
42-55: LGTM!The
Lockclass properly implements RAII semantics with deleted copy/move operations as addressed from previous review feedback.lib/hal/HalPowerManager.cpp (4)
13-18: LGTM!Initialization correctly stores the baseline CPU frequency and creates the mutex. The assert provides a fail-fast mechanism for debugging.
55-65: LGTM!The deep sleep sequence correctly waits for button release before arming the wakeup trigger, preventing immediate wake after entering sleep.
72-95: LGTM!The Lock implementation correctly addresses past review feedback by immediately restoring normal CPU frequency when acquired. The mutex protection and mode transitions are properly handled.
67-70: No issues found. The code compiles in a production environment andreadPercentage()is called on thestatic constBatteryMonitor object in multiple const method contexts, confirming thatBatteryMonitor::readPercentage()is properly const-qualified.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lib/hal/HalPowerManager.h (1)
42-48: Guard againstLockusage beforebegin()initializes the mutex.
If aLockis constructed beforebegin(),modeMutexwill be null and the constructor will dereference it. Consider a defensive check or assert in the constructor to prevent a hard crash.🛡️ Example defensive guard (apply in
lib/hal/HalPowerManager.cpp)HalPowerManager::Lock::Lock() { if (powerManager.modeMutex == nullptr) { LOG_ERR("PWR", "Lock used before begin()"); valid = false; return; } // existing logic... }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
lib/hal/HalPowerManager.h
🧰 Additional context used
🧬 Code graph analysis (1)
lib/hal/HalPowerManager.h (1)
lib/hal/HalPowerManager.cpp (10)
begin(13-18)begin(13-13)setPowerSaving(20-53)setPowerSaving(20-20)startDeepSleep(55-65)startDeepSleep(55-55)getBatteryPercentage(67-70)getBatteryPercentage(67-67)Lock(72-87)Lock(89-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: cppcheck
🔇 Additional comments (1)
lib/hal/HalPowerManager.h (1)
15-37: Clean API surface and constants.
The public API and the idle/low‑power constants are clear and appropriately scoped.
|
@ngxson there are other activities requiring wifi (KoReaderSyncActivity, KoReaderAuthActivity, WifiSelectionActivity, CalibreConnectActivity, CrosspointWebServerActivity, possibly others), should they also use skipLoopDelay? |
|
@osteotek if wifi is turned on, the power saving will automatically be disabled (checking via |
|
Tested in various activities, seems to not to degrade performance. Haven't tested battery efficiency claims |
) ## Summary Continue my experiment from crosspoint-reader#801 This PR add the ability to lower the CPU frequency on extended idle period (currently set to 3 seconds). By default, the esp32c3 CPU is set to 160MHz, and now on idle, we can reduce it to just 10MHz. Note that while this functionality is already provided by [esp power management](https://docs.espressif.com/projects/esp-idf/en/v4.3/esp32c3/api-reference/system/power_management.html), the current Arduino build lacks of this, and enabling it is just too complicated (not worth the effort compared to this PR) Update: more info in crosspoint-reader#852 (comment) ## Testing Pre-condition for each test case: the battery is charged to 100%, and is left plugged in after fully charged for an extra 1 hour. The table below shows how much battery is **used** for a given duration: | case / duration | 6 hrs | 12 hrs | | --- | --- | --- | | `delay(10)` | 26% | 48% | | `delay(50)`, PR crosspoint-reader#801 | 20% | Not tested | | `delay(50)` + low CPU freq (This PR) | Not tested | 25% | | `delay(10)` + low CPU freq (1) | Not tested | Not tested | (1) I decided not to test this case because it may not make sense. The problem is that CPU frequency vs power consumption do not follow a linear relationship, see [this](https://www.arrow.com/en/research-and-events/articles/esp32-power-consumption-can-be-reduced-with-sleep-modes) as an example. So, tight loop (10ms) + lower CPU freq significantly impact battery life, because the active CPU time is now much higher compared to the wall time. **So in conclusion, this PR improves ~150% to ~200% battery use time per charge.** The projected battery life is now: ~36-48 hrs of reading time (normal reading, no wifi) --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **NO**
) ## Summary Continue my experiment from crosspoint-reader#801 This PR add the ability to lower the CPU frequency on extended idle period (currently set to 3 seconds). By default, the esp32c3 CPU is set to 160MHz, and now on idle, we can reduce it to just 10MHz. Note that while this functionality is already provided by [esp power management](https://docs.espressif.com/projects/esp-idf/en/v4.3/esp32c3/api-reference/system/power_management.html), the current Arduino build lacks of this, and enabling it is just too complicated (not worth the effort compared to this PR) Update: more info in crosspoint-reader#852 (comment) ## Testing Pre-condition for each test case: the battery is charged to 100%, and is left plugged in after fully charged for an extra 1 hour. The table below shows how much battery is **used** for a given duration: | case / duration | 6 hrs | 12 hrs | | --- | --- | --- | | `delay(10)` | 26% | 48% | | `delay(50)`, PR crosspoint-reader#801 | 20% | Not tested | | `delay(50)` + low CPU freq (This PR) | Not tested | 25% | | `delay(10)` + low CPU freq (1) | Not tested | Not tested | (1) I decided not to test this case because it may not make sense. The problem is that CPU frequency vs power consumption do not follow a linear relationship, see [this](https://www.arrow.com/en/research-and-events/articles/esp32-power-consumption-can-be-reduced-with-sleep-modes) as an example. So, tight loop (10ms) + lower CPU freq significantly impact battery life, because the active CPU time is now much higher compared to the wall time. **So in conclusion, this PR improves ~150% to ~200% battery use time per charge.** The projected battery life is now: ~36-48 hrs of reading time (normal reading, no wifi) --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **NO**
) ## Summary Continue my experiment from crosspoint-reader#801 This PR add the ability to lower the CPU frequency on extended idle period (currently set to 3 seconds). By default, the esp32c3 CPU is set to 160MHz, and now on idle, we can reduce it to just 10MHz. Note that while this functionality is already provided by [esp power management](https://docs.espressif.com/projects/esp-idf/en/v4.3/esp32c3/api-reference/system/power_management.html), the current Arduino build lacks of this, and enabling it is just too complicated (not worth the effort compared to this PR) Update: more info in crosspoint-reader#852 (comment) ## Testing Pre-condition for each test case: the battery is charged to 100%, and is left plugged in after fully charged for an extra 1 hour. The table below shows how much battery is **used** for a given duration: | case / duration | 6 hrs | 12 hrs | | --- | --- | --- | | `delay(10)` | 26% | 48% | | `delay(50)`, PR crosspoint-reader#801 | 20% | Not tested | | `delay(50)` + low CPU freq (This PR) | Not tested | 25% | | `delay(10)` + low CPU freq (1) | Not tested | Not tested | (1) I decided not to test this case because it may not make sense. The problem is that CPU frequency vs power consumption do not follow a linear relationship, see [this](https://www.arrow.com/en/research-and-events/articles/esp32-power-consumption-can-be-reduced-with-sleep-modes) as an example. So, tight loop (10ms) + lower CPU freq significantly impact battery life, because the active CPU time is now much higher compared to the wall time. **So in conclusion, this PR improves ~150% to ~200% battery use time per charge.** The projected battery life is now: ~36-48 hrs of reading time (normal reading, no wifi) --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **NO**
) ## Summary Continue my experiment from crosspoint-reader#801 This PR add the ability to lower the CPU frequency on extended idle period (currently set to 3 seconds). By default, the esp32c3 CPU is set to 160MHz, and now on idle, we can reduce it to just 10MHz. Note that while this functionality is already provided by [esp power management](https://docs.espressif.com/projects/esp-idf/en/v4.3/esp32c3/api-reference/system/power_management.html), the current Arduino build lacks of this, and enabling it is just too complicated (not worth the effort compared to this PR) Update: more info in crosspoint-reader#852 (comment) ## Testing Pre-condition for each test case: the battery is charged to 100%, and is left plugged in after fully charged for an extra 1 hour. The table below shows how much battery is **used** for a given duration: | case / duration | 6 hrs | 12 hrs | | --- | --- | --- | | `delay(10)` | 26% | 48% | | `delay(50)`, PR crosspoint-reader#801 | 20% | Not tested | | `delay(50)` + low CPU freq (This PR) | Not tested | 25% | | `delay(10)` + low CPU freq (1) | Not tested | Not tested | (1) I decided not to test this case because it may not make sense. The problem is that CPU frequency vs power consumption do not follow a linear relationship, see [this](https://www.arrow.com/en/research-and-events/articles/esp32-power-consumption-can-be-reduced-with-sleep-modes) as an example. So, tight loop (10ms) + lower CPU freq significantly impact battery life, because the active CPU time is now much higher compared to the wall time. **So in conclusion, this PR improves ~150% to ~200% battery use time per charge.** The projected battery life is now: ~36-48 hrs of reading time (normal reading, no wifi) --- ### AI Usage While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it helps set the right context for reviewers. Did you use AI tools to help write this code? **NO**
Summary
Continue my experiment from #801
This PR add the ability to lower the CPU frequency on extended idle period (currently set to 3 seconds). By default, the esp32c3 CPU is set to 160MHz, and now on idle, we can reduce it to just 10MHz.
Note that while this functionality is already provided by esp power management, the current Arduino build lacks of this, and enabling it is just too complicated (not worth the effort compared to this PR)
Update: more info in #852 (comment)
Testing
Pre-condition for each test case: the battery is charged to 100%, and is left plugged in after fully charged for an extra 1 hour.
The table below shows how much battery is used for a given duration:
delay(10)delay(50), PR #801delay(50)+ low CPU freq (This PR)delay(10)+ low CPU freq (1)(1) I decided not to test this case because it may not make sense. The problem is that CPU frequency vs power consumption do not follow a linear relationship, see this as an example. So, tight loop (10ms) + lower CPU freq significantly impact battery life, because the active CPU time is now much higher compared to the wall time.
So in conclusion, this PR improves ~150% to ~200% battery use time per charge.
The projected battery life is now: ~36-48 hrs of reading time (normal reading, no wifi)
AI Usage
While CrossPoint doesn't have restrictions on AI tools in contributing, please be transparent about their usage as it
helps set the right context for reviewers.
Did you use AI tools to help write this code? NO