feat: Add autowake actions#11
Conversation
|
@CodeRabbit-ai review |
|
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis change introduces scheduled timer-based device wakeups with optional task execution. It extends the hardware abstraction layer to detect and arm timer wakeups during deep sleep, adds configurable settings for wake time and tasks, implements a task runner for NTP synchronization and image downloading, and integrates timer wake handling into the main application loop. Changes
Sequence DiagramsequenceDiagram
participant Device as Device<br/>(Main)
participant HAL as Hardware<br/>(GPIO/Power)
participant Clock as Clock<br/>(NTP/Sync)
participant Network as Network<br/>(WiFi/HTTP)
Device->>Device: calculateTimerWakeupUs()
Device->>HAL: startDeepSleep(timerUs)
HAL->>HAL: arm ESP timer
HAL->>HAL: esp_deep_sleep_start()
Note over HAL: Timer expires, device wakes
HAL->>HAL: detect TimerWake
HAL-->>Device: getWakeupReason() = TimerWake
Device->>Clock: restore clock state
Device->>Device: ScheduledTaskRunner::run()
alt scheduledWakeTaskNtp enabled
Device->>Network: connect WiFi
Network-->>Device: WiFi connected
Device->>Clock: syncNtp()
Clock-->>Device: NTP synced
end
alt scheduledWakeTaskImg enabled
Device->>Network: download sleep image
Network-->>Device: image downloaded
Device->>Device: rename to /sleep.bmp
end
opt WiFi was used
Device->>Network: wifiOff()
end
Device->>Clock: save clock state
Device->>HAL: startDeepSleep(timerUs)
HAL->>HAL: arm ESP timer again
HAL->>HAL: esp_deep_sleep_start()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/util/ScheduledTaskRunner.cpp (1)
99-100: Only require WiFi for tasks that will actually execute.Right now, WiFi is brought up when
scheduledWakeTaskImgis enabled even ifscheduledWakeImageUrlis empty (the task then no-ops on Line 62). This adds avoidable battery drain.♻️ Proposed refactor
- bool needWifi = SETTINGS.scheduledWakeTaskNtp || SETTINGS.scheduledWakeTaskImg; + const bool shouldRunImageTask = SETTINGS.scheduledWakeTaskImg && SETTINGS.scheduledWakeImageUrl[0] != '\0'; + bool needWifi = SETTINGS.scheduledWakeTaskNtp || shouldRunImageTask; bool wifiConnected = false; @@ - if (SETTINGS.scheduledWakeTaskImg) { + if (shouldRunImageTask) { taskDownloadSleepImage(); }Also applies to: 114-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/util/ScheduledTaskRunner.cpp` around lines 99 - 100, The code currently sets needWifi to true whenever SETTINGS.scheduledWakeTaskImg is enabled even if SETTINGS.scheduledWakeImageUrl is empty; update the logic so WiFi is required only when the Img task will actually run (i.e., scheduledWakeTaskImg is true AND scheduledWakeImageUrl is non-empty) while still requiring WiFi for scheduledWakeTaskNtp; change the expression that sets needWifi (and the analogous check around lines handling wifi connection) to use this stricter condition referencing SETTINGS.scheduledWakeTaskImg, SETTINGS.scheduledWakeImageUrl, and SETTINGS.scheduledWakeTaskNtp so the Img branch no longer forces WiFi when there is no URL.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/hal/HalPowerManager.cpp`:
- Around line 81-84: The call to esp_sleep_enable_timer_wakeup(timerWakeupUs) is
ignoring its return value; update the HalPowerManager code around the
esp_sleep_enable_timer_wakeup invocation to check the returned esp_err_t, and if
it indicates failure log an error (e.g., via LOG_ERR or similar) including the
error code and timerWakeupUs, and avoid treating the wakeup as armed (do not
emit the current LOG_DBG success message on failure); ensure the success path
still logs the existing debug message and consider returning/setting an error
flag so callers know the timer wakeup was not armed.
In `@src/main.cpp`:
- Around line 174-176: The abort path is re-arming the timer before the
RTC/clock is restored, so calculateTimerWakeupUs() may see HalClock::isSynced()
== false and skip re-arming; move the clock-restore step to run before
calculateTimerWakeupUs() (i.e., restore the hardware clock/RTC first so
HalClock::isSynced() is true), then call calculateTimerWakeupUs() and finally
powerManager.startDeepSleep(gpio, SETTINGS.useClock, timerUs).
In `@src/util/ScheduledTaskRunner.cpp`:
- Around line 66-67: Replace the direct logging of
SETTINGS.scheduledWakeImageUrl in the LOG_INF call so secrets in the URL aren't
emitted; instead redact or canonicalize the URL (e.g., remove query string and
userinfo) before logging, or log only the hostname/path or a generic message
like "Downloading sleep image" plus a safe identifier. Update the place where
LOG_INF("SCHED", "Downloading sleep image from %s",
SETTINGS.scheduledWakeImageUrl) is called in ScheduledTaskRunner.cpp to compute
a redactedUrl (strip ?query and any user@host credentials) and pass that to
LOG_INF, or remove the URL from the log entirely and log a non-sensitive status
message.
---
Nitpick comments:
In `@src/util/ScheduledTaskRunner.cpp`:
- Around line 99-100: The code currently sets needWifi to true whenever
SETTINGS.scheduledWakeTaskImg is enabled even if SETTINGS.scheduledWakeImageUrl
is empty; update the logic so WiFi is required only when the Img task will
actually run (i.e., scheduledWakeTaskImg is true AND scheduledWakeImageUrl is
non-empty) while still requiring WiFi for scheduledWakeTaskNtp; change the
expression that sets needWifi (and the analogous check around lines handling
wifi connection) to use this stricter condition referencing
SETTINGS.scheduledWakeTaskImg, SETTINGS.scheduledWakeImageUrl, and
SETTINGS.scheduledWakeTaskNtp so the Img branch no longer forces WiFi when there
is no URL.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 95ded1f3-7b66-419e-8ced-eb7aeede9019
📒 Files selected for processing (10)
lib/I18n/translations/english.yamllib/hal/HalGPIO.cpplib/hal/HalGPIO.hlib/hal/HalPowerManager.cpplib/hal/HalPowerManager.hsrc/CrossPointSettings.hsrc/SettingsList.hsrc/main.cppsrc/util/ScheduledTaskRunner.cppsrc/util/ScheduledTaskRunner.h
| if (timerWakeupUs > 0) { | ||
| esp_sleep_enable_timer_wakeup(timerWakeupUs); | ||
| LOG_DBG("PWR", "Timer wakeup armed for %llu us (~%llu min)", timerWakeupUs, timerWakeupUs / 60000000ULL); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the file
find . -name "HalPowerManager.cpp" -type fRepository: jpirnay/crosspoint-reader
Length of output: 98
🏁 Script executed:
# Search for the specific code pattern to verify line numbers and context
rg -n "esp_sleep_enable_timer_wakeup" lib/hal/HalPowerManager.cpp -B 2 -A 3Repository: jpirnay/crosspoint-reader
Length of output: 356
🏁 Script executed:
# Check if there are other usages of esp_sleep_enable_timer_wakeup in the codebase
rg "esp_sleep_enable_timer_wakeup" --type cppRepository: jpirnay/crosspoint-reader
Length of output: 146
🏁 Script executed:
# Look for error handling patterns with ESP APIs in the codebase
rg "esp_err_t|ESP_OK" lib/hal/ --type cpp -A 2 | head -50Repository: jpirnay/crosspoint-reader
Length of output: 51
🏁 Script executed:
# Read the entire HalPowerManager.cpp file to understand context and error handling patterns
wc -l lib/hal/HalPowerManager.cppRepository: jpirnay/crosspoint-reader
Length of output: 100
🏁 Script executed:
# View the file to understand error handling patterns and context
cat -n lib/hal/HalPowerManager.cppRepository: jpirnay/crosspoint-reader
Length of output: 5027
🏁 Script executed:
# Search for other ESP API calls in the file that might handle errors
rg "esp_" lib/hal/HalPowerManager.cpp -nRepository: jpirnay/crosspoint-reader
Length of output: 319
🏁 Script executed:
# Look for error handling patterns with LOG_ERR or similar in lib/hal/
rg "LOG_ERR|return.*ESP_" lib/hal/ --type cpp -B 1Repository: jpirnay/crosspoint-reader
Length of output: 854
🏁 Script executed:
# Check if esp_sleep.h is in the repository or if it comes from ESP-IDF
find . -name "esp_sleep.h" -type fRepository: jpirnay/crosspoint-reader
Length of output: 51
🏁 Script executed:
# Search for any documentation or comments about ESP API return values in the codebase
rg "esp_err_t|esp_sleep_enable_timer_wakeup" --type cpp -B 2 -A 2Repository: jpirnay/crosspoint-reader
Length of output: 4238
🏁 Script executed:
# Check if there are any error checks for similar ESP APIs elsewhere in the codebase
rg "esp_deep_sleep|esp_sleep" --type cpp -B 1 -A 1Repository: jpirnay/crosspoint-reader
Length of output: 1676
Check return value from esp_sleep_enable_timer_wakeup() on line 82.
The return value is being ignored. If the timer wakeup fails to arm, the code still logs success, resulting in silent failure of scheduled wakeup functionality.
🔧 Proposed fix
// Optionally arm a timer wakeup (for scheduled tasks)
if (timerWakeupUs > 0) {
- esp_sleep_enable_timer_wakeup(timerWakeupUs);
- LOG_DBG("PWR", "Timer wakeup armed for %llu us (~%llu min)", timerWakeupUs, timerWakeupUs / 60000000ULL);
+ const esp_err_t err = esp_sleep_enable_timer_wakeup(timerWakeupUs);
+ if (err != ESP_OK) {
+ LOG_ERR("PWR", "Failed to arm timer wakeup: %d", static_cast<int>(err));
+ } else {
+ LOG_DBG("PWR", "Timer wakeup armed for %llu us (~%llu min)", timerWakeupUs, timerWakeupUs / 60000000ULL);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (timerWakeupUs > 0) { | |
| esp_sleep_enable_timer_wakeup(timerWakeupUs); | |
| LOG_DBG("PWR", "Timer wakeup armed for %llu us (~%llu min)", timerWakeupUs, timerWakeupUs / 60000000ULL); | |
| } | |
| if (timerWakeupUs > 0) { | |
| const esp_err_t err = esp_sleep_enable_timer_wakeup(timerWakeupUs); | |
| if (err != ESP_OK) { | |
| LOG_ERR("PWR", "Failed to arm timer wakeup: %d", static_cast<int>(err)); | |
| } else { | |
| LOG_DBG("PWR", "Timer wakeup armed for %llu us (~%llu min)", timerWakeupUs, timerWakeupUs / 60000000ULL); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/hal/HalPowerManager.cpp` around lines 81 - 84, The call to
esp_sleep_enable_timer_wakeup(timerWakeupUs) is ignoring its return value;
update the HalPowerManager code around the esp_sleep_enable_timer_wakeup
invocation to check the returned esp_err_t, and if it indicates failure log an
error (e.g., via LOG_ERR or similar) including the error code and timerWakeupUs,
and avoid treating the wakeup as armed (do not emit the current LOG_DBG success
message on failure); ensure the success path still logs the existing debug
message and consider returning/setting an error flag so callers know the timer
wakeup was not armed.
| // IMPORTANT: Re-arm the wakeup trigger (and scheduled timer) before sleeping again | ||
| uint64_t timerUs = calculateTimerWakeupUs(); | ||
| powerManager.startDeepSleep(gpio, SETTINGS.useClock, timerUs); |
There was a problem hiding this comment.
Restore clock before re-arming timer in abort path.
At Line 175, calculateTimerWakeupUs() can run before clock restore, so HalClock::isSynced() may be false and the timer may not be re-armed after an aborted wake.
🔧 Proposed fix
if (abort) {
// Button released too early. Returning to sleep.
// IMPORTANT: Re-arm the wakeup trigger (and scheduled timer) before sleeping again
+ HalClock::restore();
uint64_t timerUs = calculateTimerWakeupUs();
+ HalClock::saveBeforeSleep(SETTINGS.useClock);
powerManager.startDeepSleep(gpio, SETTINGS.useClock, timerUs);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // IMPORTANT: Re-arm the wakeup trigger (and scheduled timer) before sleeping again | |
| uint64_t timerUs = calculateTimerWakeupUs(); | |
| powerManager.startDeepSleep(gpio, SETTINGS.useClock, timerUs); | |
| // IMPORTANT: Re-arm the wakeup trigger (and scheduled timer) before sleeping again | |
| HalClock::restore(); | |
| uint64_t timerUs = calculateTimerWakeupUs(); | |
| HalClock::saveBeforeSleep(SETTINGS.useClock); | |
| powerManager.startDeepSleep(gpio, SETTINGS.useClock, timerUs); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.cpp` around lines 174 - 176, The abort path is re-arming the timer
before the RTC/clock is restored, so calculateTimerWakeupUs() may see
HalClock::isSynced() == false and skip re-arming; move the clock-restore step to
run before calculateTimerWakeupUs() (i.e., restore the hardware clock/RTC first
so HalClock::isSynced() is true), then call calculateTimerWakeupUs() and finally
powerManager.startDeepSleep(gpio, SETTINGS.useClock, timerUs).
| LOG_INF("SCHED", "Downloading sleep image from %s", SETTINGS.scheduledWakeImageUrl); | ||
|
|
There was a problem hiding this comment.
Avoid logging full download URLs (possible secret leakage).
On Line 66, logging the raw URL can expose embedded credentials or signed query tokens in device logs.
🔒 Proposed fix
- LOG_INF("SCHED", "Downloading sleep image from %s", SETTINGS.scheduledWakeImageUrl);
+ LOG_INF("SCHED", "Downloading configured sleep image");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| LOG_INF("SCHED", "Downloading sleep image from %s", SETTINGS.scheduledWakeImageUrl); | |
| LOG_INF("SCHED", "Downloading configured sleep image"); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/util/ScheduledTaskRunner.cpp` around lines 66 - 67, Replace the direct
logging of SETTINGS.scheduledWakeImageUrl in the LOG_INF call so secrets in the
URL aren't emitted; instead redact or canonicalize the URL (e.g., remove query
string and userinfo) before logging, or log only the hostname/path or a generic
message like "Downloading sleep image" plus a safe identifier. Update the place
where LOG_INF("SCHED", "Downloading sleep image from %s",
SETTINGS.scheduledWakeImageUrl) is called in ScheduledTaskRunner.cpp to compute
a redactedUrl (strip ?query and any user@host credentials) and pass that to
LOG_INF, or remove the URL from the log entirely and log a non-sensitive status
message.
1 participant
Summary by CodeRabbit