stm32wl(rak3172): implement cpuDeepSleep()/shutdown() via STM32LowPower#10993
stm32wl(rak3172): implement cpuDeepSleep()/shutdown() via STM32LowPower#10993ndoo wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesSTM32WL now initializes STM32LowPower and implements RTC-aware timed or indefinite deep sleep. The common shutdown path also turns off indicators before deep sleep, and the RAK3172 configuration pins the required low-power library. STM32WL low-power integration
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Power
participant STM32WL
participant LowPower
Power->>STM32WL: Request deep sleep
STM32WL->>STM32WL: Check RTC availability
STM32WL->>LowPower: Shutdown with timeout or indefinitely
LowPower-->>STM32WL: Return unexpectedly
STM32WL->>STM32WL: Force system reset
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
⚡ Try this PR in the Web FlasherNote Building this pull request… the flash button, badges and supported-board |
|
@coderabbitai review Only commit 79d126a should be reviewed (IMHO) because the commits preceding it were reviewed in #10961 |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 10993 --repo meshtastic/firmware --json commits --jq '.commits[] | "\(.oid) \(.messageHeadline)"'
echo "---"
gh api repos/meshtastic/firmware/commits/79d126add6de98827fe47182d0749cb50dc30f9b --jq '.files[].filename'
echo "---diff---"
gh api repos/meshtastic/firmware/commits/79d126add6de98827fe47182d0749cb50dc30f9b --jq '.files[].patch'Length of output: 3050 🏁 Script executed: #!/bin/bash
set -euo pipefail
git clone --depth 60 https://github.com/meshtastic/firmware.git repo 2>&1 | tail -5
cd repo
git fetch origin pull/10993/head:pr10993 2>&1 | tail -5
git checkout pr10993 2>&1 | tail -5
sed -n '860,960p' src/Power.cpp
echo "----main-stm32wl.cpp----"
sed -n '1,150p' src/platform/stm32wl/main-stm32wl.cpp
echo "----callers of cpuDeepSleep----"
rg -n "cpuDeepSleep" -C3
echo "----portMAX_DELAY usage----"
rg -n "portMAX_DELAY" src/platform/stm32wlLength of output: 8988 🏁 Script executed: #!/bin/bash
set -uo pipefail
cd repo 2>/dev/null || true
echo "----callers of cpuDeepSleep----"
rg -n "cpuDeepSleep" -C3 || true
echo "----portMAX_DELAY usage----"
rg -n "portMAX_DELAY" src/platform/stm32wl || true
echo "----DELAY_FOREVER def----"
rg -n "DELAY_FOREVER" || true
echo "----doDeepSleep def----"
rg -n "void doDeepSleep|doDeepSleep\(" -C5 || trueLength of output: 530 🏁 Script executed: #!/bin/bash
set -uo pipefail
pwd
ls
if [ -d repo ]; then
cd repo
git status | head -5
echo "---"
git grep -n "cpuDeepSleep" || true
echo "---doDeepSleep---"
git grep -n "doDeepSleep" || true
echo "---DELAY_FOREVER---"
git grep -n "DELAY_FOREVER" || true
else
echo "no repo dir, re-cloning"
git clone --depth 60 https://github.com/meshtastic/firmware.git repo
cd repo
git fetch origin pull/10993/head:pr10993
git checkout pr10993
git grep -n "cpuDeepSleep" || true
echo "---doDeepSleep---"
git grep -n "doDeepSleep" || true
echo "---DELAY_FOREVER---"
git grep -n "DELAY_FOREVER" || true
fiLength of output: 4540 🏁 Script executed: #!/bin/bash
cd repo
sed -n '200,260p' src/sleep.cpp
echo "---sleep.cpp near 380-410---"
sed -n '370,410p' src/sleep.cpp
echo "---esp32 cpuDeepSleep---"
sed -n '280,380p' src/platform/esp32/main-esp32.cpp
echo "---nrf52 cpuDeepSleep---"
sed -n '420,470p' src/platform/nrf52/main-nrf52.cppLength of output: 9233
Overall this commit looks solid:
One thing worth a second look, in if (!stm32wlRtcAvailable())
return;If the LSE crystal fails to lock at boot (e.g., missing/faulty crystal on a given This isn't a regression (the previous empty stub was equally silent), and it's an edge case gated behind a boot-time hardware fault, so it's not blocking. A small improvement would be to add a if (!stm32wlRtcAvailable()) {
LOG_WARN("STM32WL: hardware RTC unavailable, cannot deep sleep/shutdown");
return;
}✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Power.cpp (1)
906-919: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake failed STM32WL sleep attempts observable.
If LSE initialization fails, the STM32WL deep-sleep implementation can return early when the RTC is unavailable, leaving the device running after shutdown was requested. Emit a warning at the attempted sleep/shutdown point, not only during boot, so this failure is diagnosable.
🤖 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 `@src/Power.cpp` around lines 906 - 919, Update the STM32WL shutdown path surrounding doDeepSleep to emit a warning when the deep-sleep attempt cannot proceed because the RTC/LSE is unavailable. Keep the warning at the sleep/shutdown call site so failed attempts are observable, while preserving the existing LED shutdown behavior and sleep flow for successful attempts.src/platform/stm32wl/main-stm32wl.cpp (1)
137-138: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winLog skipped deep-sleep requests.
If LSE initialization failed, this return makes shutdown/deep sleep a no-op; the boot-time warning does not identify the skipped sleep attempt. Emit
LOG_WARNbefore returning.Proposed change
if (!stm32wlRtcAvailable()) + LOG_WARN("STM32WL: cannot enter deep sleep; hardware RTC unavailable"); + if (!stm32wlRtcAvailable()) return;As per coding guidelines, prefer
LOG_WARNfor logging.🤖 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 `@src/platform/stm32wl/main-stm32wl.cpp` around lines 137 - 138, In the shutdown/deep-sleep path guarded by stm32wlRtcAvailable(), emit a LOG_WARN describing that the sleep request is being skipped before returning when the RTC is unavailable. Keep the existing early-return behavior unchanged.Source: Coding guidelines
🤖 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 `@src/platform/stm32wl/main-stm32wl.cpp`:
- Around line 137-138: In the shutdown/deep-sleep path guarded by
stm32wlRtcAvailable(), emit a LOG_WARN describing that the sleep request is
being skipped before returning when the RTC is unavailable. Keep the existing
early-return behavior unchanged.
In `@src/Power.cpp`:
- Around line 906-919: Update the STM32WL shutdown path surrounding doDeepSleep
to emit a warning when the deep-sleep attempt cannot proceed because the RTC/LSE
is unavailable. Keep the warning at the sleep/shutdown call site so failed
attempts are observable, while preserving the existing LED shutdown behavior and
sleep flow for successful attempts.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 54007e5c-9872-42ba-b69f-c56331efcbb9
📒 Files selected for processing (54)
src/Power.cppsrc/RedirectablePrint.cppsrc/gps/GPS.cppsrc/gps/NMEAWPL.cppsrc/gps/RTC.cppsrc/gps/RTC.hsrc/graphics/SharedUIDisplay.cppsrc/graphics/draw/UIRenderer.cppsrc/graphics/niche/InkHUD/Applet.cppsrc/graphics/niche/InkHUD/Applets/Bases/NodeList/NodeListApplet.cppsrc/graphics/niche/InkHUD/Applets/System/Menu/MenuApplet.cppsrc/graphics/niche/InkHUD/Applets/System/Notification/NotificationApplet.cppsrc/graphics/niche/InkHUD/Applets/User/Heard/HeardApplet.cppsrc/graphics/niche/InkHUD/Applets/User/RecentsList/RecentsListApplet.cppsrc/graphics/niche/InkHUD/Applets/User/ThreadedMessage/ThreadedMessageApplet.cppsrc/graphics/niche/InkHUD/Events.cppsrc/main.cppsrc/main.hsrc/mesh/MeshService.cppsrc/mesh/NodeDB.cppsrc/mesh/PhoneAPI.cppsrc/mesh/Router.cppsrc/mesh/StreamAPI.cppsrc/mesh/TransmitHistory.cppsrc/mesh/eth/ethClient.cppsrc/mesh/wifi/WiFiAPClient.cppsrc/modules/AdminModule.cppsrc/modules/ExternalNotificationModule.cppsrc/modules/KeyVerificationModule.cppsrc/modules/MeshBeaconModule.cppsrc/modules/NeighborInfoModule.cppsrc/modules/NodeInfoModule.cppsrc/modules/PositionModule.cppsrc/modules/PowerStressModule.cppsrc/modules/RangeTestModule.cppsrc/modules/RemoteHardwareModule.cppsrc/modules/SerialModule.cppsrc/modules/StoreForwardModule.cppsrc/modules/Telemetry/AirQualityTelemetry.cppsrc/modules/Telemetry/DeviceTelemetry.cppsrc/modules/Telemetry/EnvironmentTelemetry.cppsrc/modules/Telemetry/HealthTelemetry.cppsrc/modules/Telemetry/PowerTelemetry.cppsrc/modules/Telemetry/Sensor/PMSA003ISensor.hsrc/modules/Telemetry/Sensor/SCD4XSensor.hsrc/modules/Telemetry/Sensor/SEN5XSensor.hsrc/modules/Telemetry/Sensor/SFA30Sensor.hsrc/modules/esp32/AudioModule.cppsrc/mqtt/MQTT.cppsrc/platform/stm32wl/architecture.hsrc/platform/stm32wl/main-stm32wl.cppsrc/security/EncryptedStorage.cppvariants/stm32/rak3172/platformio.inivariants/stm32/rak3172/variant.h
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 10993 --repo meshtastic/firmware --json commits --jq '.commits[-3:] | .[] | "\(.oid) \(.messageHeadline)"'
echo "---latest diff for main-stm32wl.cpp---"
gh pr diff 10993 --repo meshtastic/firmware -- src/platform/stm32wl/main-stm32wl.cppLength of output: 959 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 10993 --repo meshtastic/firmware --json commits --jq '.commits[] | "\(.oid) \(.messageHeadline)"'Length of output: 863 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 10993 --repo meshtastic/firmware --json headRefOid,updatedAt --jq '.'
echo "---"
gh api repos/meshtastic/firmware/commits/79d126add6de98827fe47182d0749cb50dc30f9b --jq '.commit.author.date'
echo "---file at head---"
gh api repos/meshtastic/firmware/contents/src/platform/stm32wl/main-stm32wl.cpp?ref=pr10993 2>/dev/null || trueLength of output: 773 🏁 Script executed: #!/bin/bash
set -euo pipefail
gh api repos/meshtastic/firmware/contents/src/platform/stm32wl/main-stm32wl.cpp?ref=79d126add6de98827fe47182d0749cb50dc30f9b --jq '.content' | base64 -d | sed -n '1,200p'Length of output: 6808 🏁 Script executed: #!/bin/bash
set -euo pipefail
rm -rf repo
git clone --depth 100 https://github.com/meshtastic/firmware.git repo 2>&1 | tail -3
cd repo
git fetch origin pull/10993/head 2>&1 | tail -5
git rev-parse FETCH_HEAD
git log --oneline -5 FETCH_HEADLength of output: 953 Confirmed: the current head of the PR is still void cpuDeepSleep(uint32_t msecToWake)
{
`#if` HAS_LSE
if (!stm32wlRtcAvailable())
return;
if (Serial)
Serial.end();
...No Could you push the commit with these changes? Once it's on the branch I'll take another look at the updated ✅ Action performedReview finished.
|
|
Pushed @coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 10993 --repo meshtastic/firmware --json headRefOid,commits --jq '.headRefOid, (.commits[-3:] | .[] | "\(.oid) \(.messageHeadline)")'
echo "---diff for main-stm32wl.cpp---"
gh pr diff 10993 --repo meshtastic/firmware -- src/platform/stm32wl/main-stm32wl.cppLength of output: 1027 🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf repo
git clone --depth 100 https://github.com/meshtastic/firmware.git repo 2>&1 | tail -5
cd repo
git fetch origin pull/10993/head 2>&1 | tail -5
git checkout FETCH_HEAD 2>&1 | tail -5
echo "---full file---"
cat -n src/platform/stm32wl/main-stm32wl.cpp | sed -n '1,220p'Length of output: 9501
The three changes you mentioned are all there and look good:
Two small nits, neither blocking:
Nice work — this addresses the concern well. [resolve] ᐟ(•ㅅ•)ᐟ |
bfe8c29 to
21878ea
Compare
Force-pushed for a few fixes:
@coderabbitai review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
gh pr view 10993 --repo meshtastic/firmware --json headRefOid,commits --jq '.headRefOid, (.commits[-3:] | .[] | "\(.oid) \(.messageHeadline)")'
echo "---diff for main-stm32wl.cpp---"
gh pr diff 10993 --repo meshtastic/firmware -- src/platform/stm32wl/main-stm32wl.cpp
echo "---full file at head---"
HEAD=$(gh pr view 10993 --repo meshtastic/firmware --json headRefOid --jq '.headRefOid')
gh api repos/meshtastic/firmware/contents/src/platform/stm32wl/main-stm32wl.cpp?ref=$HEAD --jq '.content' | base64 -d | cat -n | sed -n '1,220p'Length of output: 1292 🏁 Script executed: #!/bin/bash
set -euo pipefail
HEAD="21878ea3c36909c8e76213825b7d2772c4d2d2e2"
echo "---diff for main-stm32wl.cpp---"
gh pr diff 10993 --repo meshtastic/firmware --patch | awk '/^diff --git a\/src\/platform\/stm32wl\/main-stm32wl.cpp/,/^diff --git a\/[^s]/' | head -300
echo "---full file at head---"
gh api repos/meshtastic/firmware/contents/src/platform/stm32wl/main-stm32wl.cpp?ref=$HEAD --jq '.content' | base64 -d | cat -n | sed -n '1,220p'Length of output: 12519 🏁 Script executed: #!/bin/bash
set -euo pipefail
HEAD="21878ea3c36909c8e76213825b7d2772c4d2d2e2"
echo "---full file at head---"
gh api repos/meshtastic/firmware/contents/src/platform/stm32wl/main-stm32wl.cpp?ref=$HEAD --jq '.content' | base64 -d | cat -nLength of output: 11232
This looks good to me now — nice work on [resolve] |
|
Please fix the merge conflicts |
#10961 must merge before this PR btw, so I will fix the conflicts there too first |
21878ea to
c5023e4
Compare
fixed the merge conflicts in #10961 then rebased this PR on that cleanly |
cpuDeepSleep() was an empty stub, so the SDS deep-sleep PowerFSM state and low-battery shutdown did nothing but leave the CPU running at full power. Power::shutdown() also didn't include STM32WL in its arch list, so an explicit shutdown command just logged a FIXME warning. Adds STM32LowPower as a lib_dep (rak3172 only, mirroring STM32RTC) and implements cpuDeepSleep() using it. Standby mode is used for both the finite-wake (SDS/low battery) and forever (shutdown) paths, via LowPower.shutdown(), with or without an RTC alarm. If the LSE-backed hardware RTC never came up (stm32wlRtcAvailable() false), this is a no-op - safer than sleeping without a confirmed wake source. LowPower.shutdown() resets the MCU on wake and should never return; if it somehow does, force a reset via HAL_NVIC_SystemReset() rather than hanging the device silently forever. Gated behind the existing HAS_LSE flag, so this is inert on every variant but rak3172. Assisted-by: Claude Sonnet 5 <noreply@anthropic.com> Signed-off-by: Andrew Yong <me@ndoo.sg>
This is to prevent leaving the firmware catanonic when firmware has run its shutdown routine but doesn't actually shutdown. Signed-off-by: Andrew Yong <me@ndoo.sg>
c5023e4 to
01297c7
Compare
|
rebased this on HEAD of develop now that #10961 has been merged |
2 participants
Summary
Implements deep-sleep/shutdown for STM32WL (
rak3172) using theSTM32LowPowerArduino library, building on #10961'sHAS_LSE/STM32RTCinfrastructure.cpuDeepSleep()(src/platform/stm32wl/main-stm32wl.cpp) was an empty stub, so the SDS deep-sleep PowerFSM state and low-battery shutdown never actually slept.Power::shutdown()didn't include STM32WL in its arch list, so an explicit shutdown command just logged a FIXME warning.Both gaps are closed for
rak3172, gated behind the existingHAS_LSEflag (default 0, only set forrak3172). The main reason to only implement it in RAK3172 is because the remaining FLASH memory is too tight on other variants, including the RTC and LowPower libraries overflow on most other STM32WL variants.In future, this can (and should) be rolled out to all STM32WL variants that have a hardware LSE crystal (most of them, it seems).
Dependency
This PR is based on #10961 (open, not yet merged).
stm32wl-hardware-rtc(its head branch) only exists on themesh-malaysiafork, not upstream, so this PR targetsdevelopdirectly — the diff currently includes #10961's commits too, until #10961 merges.Implementation
Both
cpuDeepSleep()paths useLowPower.shutdown()(Standby mode) — with an RTC alarm for a finite wake (SDS/low-battery), without one for a forever shutdown (Power::shutdown()).Flash budget
rak3172beforerak3172afterwio-e5before/after🤝 Attestations
Tested on a RAK3172-T unit, with PR #10964's TCXO fixes cherry-picked in for that testing session (not included in this PR — a RAK3172-T needs #10964 to boot at all, but a base RAK3172 does not, and this PR's changes don't depend on it). Confirmed on hardware:
Power::shutdown(), Standby mode, no alarm): device saves state, sleeps the radio, enters Standby, and stays silent until manually reset.low_voltage_counterdebounce firesEVENT_LOW_BATTERY, device enters deep sleep, then resets and rejoins the mesh on its own oncesds_secselapses.Build-verified no regression on
wio-e5,russell,CDEBYTE_E77-MBL(HAS_LSEdefaults to0, all new code compiled out).milesight_gs301fails to build ondevelopindependent of this PR (pre-existingNANOLIB_FLOAT_PRINTF/debug-print config issue, confirmed via a clean checkout).Summary by CodeRabbit