Display-only: no-controller auto-sleep + battery indicator (LilyGo T-RGB)#773
Display-only: no-controller auto-sleep + battery indicator (LilyGo T-RGB)#773dmq1219 wants to merge 7 commits into
Conversation
…only) Display firmware only (LilyGo T-RGB, env:display). No controller, coffee-control (heater/pump/valve/pressure/temperature), profile, BLE-command, bootloader, partition or flash-security changes. Auto-sleep: deep-sleep the screen (wake on touch) after 120s with no BLE link to the controller. Cancels on reconnect; suppressed during OTA/update/Wi-Fi-setup; sleeps sooner when the battery is critical. Toggle + timeout in Settings (default on / 2 min). Implemented as a pure-logic AutoSleepManager. Battery: read the single-cell Li-ion voltage via Driver::getBatteryMilliVolts() (LilyGo panel.getBattVoltage(), GPIO4, handles the 1/2 divider). Sampled every 30s; BatteryMonitor maps it to an APPROXIMATE percent (piecewise LUT) shown with the raw voltage on a top-layer overlay. USB-plugged readings reflect charging voltage, not true charge (documented). Simulator: SdlDriver mocks the battery (GM_SIM_BATTERY_MV) and logs a sleep event instead of exiting. Verified: pio run -e display, pio run -e display-sim. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds two display-firmware features: (1) deep sleep when no BLE controller is detected for a configurable timeout, with touch wake and suppression during OTA/AP/autotune, and (2) a battery voltage/percentage overlay. Introduces ChangesAuto-Sleep and Battery Indicator
Sequence Diagram(s)sequenceDiagram
participant BLE as BLE Event
participant DefaultUI as DefaultUI
participant AutoSleep as AutoSleepManager
participant Battery as BatteryMonitor
participant Driver as Driver (LilyGo/Sdl)
participant Settings as Settings
BLE->>DefaultUI: onControllerDisconnected
DefaultUI->>AutoSleep: onControllerDisconnected(millis())
loop Every loop tick
DefaultUI->>Driver: getBatteryMilliVolts()
Driver-->>DefaultUI: voltageMv
DefaultUI->>Battery: update(voltageMv, now)
Battery-->>DefaultUI: percent, isCritical, isLow
DefaultUI->>AutoSleep: setForceShortTimeout(critical && !connected)
DefaultUI->>Settings: isAutoSleepNoController(), getNoControllerSleepTimeout()
DefaultUI->>AutoSleep: setEnabled(), setTimeoutMs(), setSuppressed()
DefaultUI->>AutoSleep: evaluate(now)
AutoSleep-->>DefaultUI: SleepReason
alt SleepReason == NoController
DefaultUI->>Driver: setBrightness(0)
DefaultUI->>Driver: sleep()
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
…eenshots TOP_RIGHT placed the indicator in the round T-RGB's dead corner; move it to top-centre under the status-icon row so it's visible. Add simulator screenshots (normal + critical battery) and reference them in the docs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
![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
🤖 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.
Inline comments:
In `@sim/driver/SdlDriver.h`:
- Around line 26-29: The getBatteryMilliVolts() method directly casts the result
of atoi() to uint16_t without validating the input, which causes negative values
to wrap around (e.g., -1 becomes 65535). Modify the method to validate and clamp
the integer value returned by atoi() to ensure it falls within a valid range for
uint16_t before performing the cast, treating invalid or out-of-range values as
the default 3900 millivolts instead.
In `@src/display/core/AutoSleepManager.h`:
- Line 40: The code uses 0 as a sentinel value to indicate "not counting" for
the controllerDisconnectedSince_ variable, but coerces it to 1 on lines 40 and
54 to avoid sentinel issues. However, line 69 performs unsigned subtraction (now
- controllerDisconnectedSince_), which causes an underflow when now equals 0,
resulting in a very large value that immediately satisfies the timeout check and
triggers unintended deep sleep at boot/disconnect. Fix this by separating the
"counting active" state from the timestamp value itself—use a distinct boolean
flag to track whether the controller disconnection timer is actively counting,
rather than relying on sentinel values in the timestamp field. Update lines 40,
54, and 69 to check this flag instead of relying on the 0-to-1 coercion and
unsigned math behavior.
In `@src/display/core/Settings.cpp`:
- Line 99: The noControllerSleepTimeout values read via preferences.getInt() are
not being validated or clamped to acceptable ranges before flowing into
AutoSleepManager::setTimeoutMs(uint32_t). Negative, zero, or corrupt values can
cause wrap-around or trigger unintended immediate sleep behavior. Clamp the
noControllerSleepTimeout values at both the point where they are read from
preferences and where they are set from incoming values to ensure they fall
within a valid timeout range before being passed to the AutoSleepManager method.
🪄 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: dc28be7d-b903-47f5-9c75-9fb2f4e0c47f
📒 Files selected for processing (12)
docs/DISPLAY_BATTERY_AUTOSLEEP.mdsim/driver/SdlDriver.hsrc/display/core/AutoSleepManager.hsrc/display/core/BatteryMonitor.hsrc/display/core/Settings.cppsrc/display/core/Settings.hsrc/display/core/constants.hsrc/display/drivers/Driver.hsrc/display/drivers/LilyGoDriver.hsrc/display/plugins/WebUIPlugin.cppsrc/display/ui/default/DefaultUI.cppsrc/display/ui/default/DefaultUI.h
| uint16_t getBatteryMilliVolts() override { | ||
| const char *mv = getenv("GM_SIM_BATTERY_MV"); | ||
| return static_cast<uint16_t>(mv ? atoi(mv) : 3900); | ||
| } |
There was a problem hiding this comment.
Validate and clamp GM_SIM_BATTERY_MV before casting.
atoi() + direct uint16_t cast can wrap invalid inputs (e.g., -1 becomes 65535), which distorts simulated battery percentage and auto-sleep behavior.
Suggested fix
uint16_t getBatteryMilliVolts() override {
- const char *mv = getenv("GM_SIM_BATTERY_MV");
- return static_cast<uint16_t>(mv ? atoi(mv) : 3900);
+ const char *mv = std::getenv("GM_SIM_BATTERY_MV");
+ if (mv == nullptr) {
+ return 3900;
+ }
+ char *end = nullptr;
+ const long parsed = std::strtol(mv, &end, 10);
+ if (end == mv || *end != '\0' || parsed < 0 || parsed > 5000) {
+ return 3900;
+ }
+ return static_cast<uint16_t>(parsed);
}📝 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.
| uint16_t getBatteryMilliVolts() override { | |
| const char *mv = getenv("GM_SIM_BATTERY_MV"); | |
| return static_cast<uint16_t>(mv ? atoi(mv) : 3900); | |
| } | |
| uint16_t getBatteryMilliVolts() override { | |
| const char *mv = std::getenv("GM_SIM_BATTERY_MV"); | |
| if (mv == nullptr) { | |
| return 3900; | |
| } | |
| char *end = nullptr; | |
| const long parsed = std::strtol(mv, &end, 10); | |
| if (end == mv || *end != '\0' || parsed < 0 || parsed > 5000) { | |
| return 3900; | |
| } | |
| return static_cast<uint16_t>(parsed); | |
| } |
🤖 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 `@sim/driver/SdlDriver.h` around lines 26 - 29, The getBatteryMilliVolts()
method directly casts the result of atoi() to uint16_t without validating the
input, which causes negative values to wrap around (e.g., -1 becomes 65535).
Modify the method to validate and clamp the integer value returned by atoi() to
ensure it falls within a valid range for uint16_t before performing the cast,
treating invalid or out-of-range values as the default 3900 millivolts instead.
| // Begin with no connection yet (start the countdown at boot). | ||
| void begin(uint32_t now) { | ||
| controllerConnected_ = false; | ||
| controllerDisconnectedSince_ = now ? now : 1; // 0 means "not counting" |
There was a problem hiding this comment.
Fix immediate-sleep underflow when timestamp is 0.
Line 40 and Line 54 coerce now == 0 to 1, but Line 69 computes now - controllerDisconnectedSince_ as unsigned math. At now == 0, that underflows and can instantly satisfy timeout, causing an unintended deep sleep right at boot/disconnect edge.
💡 Suggested fix (separate “counting active” state from timestamp value)
class AutoSleepManager {
public:
@@
void begin(uint32_t now) {
controllerConnected_ = false;
- controllerDisconnectedSince_ = now ? now : 1; // 0 means "not counting"
+ disconnectCountdownActive_ = true;
+ controllerDisconnectedSince_ = now;
lastUserInteractionAt_ = now;
}
@@
void onControllerConnected(uint32_t now) {
controllerConnected_ = true;
lastControllerConnectedAt_ = now;
- controllerDisconnectedSince_ = 0; // cancel the countdown
+ disconnectCountdownActive_ = false; // cancel the countdown
+ controllerDisconnectedSince_ = 0;
}
@@
void onControllerDisconnected(uint32_t now) {
@@
- if (controllerConnected_ || controllerDisconnectedSince_ == 0) {
- controllerDisconnectedSince_ = now ? now : 1;
+ if (controllerConnected_ || !disconnectCountdownActive_) {
+ controllerDisconnectedSince_ = now;
+ disconnectCountdownActive_ = true;
}
controllerConnected_ = false;
}
@@
SleepReason evaluate(uint32_t now) const {
- if (!enabled_ || suppressed_ || controllerConnected_ || controllerDisconnectedSince_ == 0) {
+ if (!enabled_ || suppressed_ || controllerConnected_ || !disconnectCountdownActive_) {
return SleepReason::None;
}
const uint32_t timeout = forceShortTimeout_ ? (timeoutMs_ / 4) : timeoutMs_;
if (now - controllerDisconnectedSince_ >= timeout) {
return SleepReason::NoController;
}
return SleepReason::None;
}
@@
private:
@@
+ bool disconnectCountdownActive_ = false;
uint32_t controllerDisconnectedSince_ = 0;Also applies to: 54-54, 69-69
🤖 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/display/core/AutoSleepManager.h` at line 40, The code uses 0 as a
sentinel value to indicate "not counting" for the controllerDisconnectedSince_
variable, but coerces it to 1 on lines 40 and 54 to avoid sentinel issues.
However, line 69 performs unsigned subtraction (now -
controllerDisconnectedSince_), which causes an underflow when now equals 0,
resulting in a very large value that immediately satisfies the timeout check and
triggers unintended deep sleep at boot/disconnect. Fix this by separating the
"counting active" state from the timestamp value itself—use a distinct boolean
flag to track whether the controller disconnection timer is actively counting,
rather than relying on sentinel values in the timestamp field. Update lines 40,
54, and 69 to check this flag instead of relying on the 0-to-1 coercion and
unsigned math behavior.
| standbyBrightnessTimeout = preferences.getInt("standby_bt", 60000); | ||
| // [display-auto-sleep] display-only | ||
| autoSleepNoController = preferences.getBool("as_noctl", DEFAULT_AUTO_SLEEP_NO_CONTROLLER); | ||
| noControllerSleepTimeout = preferences.getInt("as_noctl_t", DEFAULT_NO_CONTROLLER_SLEEP_TIMEOUT_MS); |
There was a problem hiding this comment.
Clamp persisted and incoming noControllerSleepTimeout values.
Line [99] and Line [381] accept raw int timeout values; those flow into AutoSleepManager::setTimeoutMs(uint32_t) (see src/display/ui/default/DefaultUI.cpp Line [362]). Negative/zero/corrupt values can wrap or trigger unintended immediate sleep behavior.
🔧 Proposed fix
- noControllerSleepTimeout = preferences.getInt("as_noctl_t", DEFAULT_NO_CONTROLLER_SLEEP_TIMEOUT_MS);
+ noControllerSleepTimeout =
+ std::clamp(preferences.getInt("as_noctl_t", DEFAULT_NO_CONTROLLER_SLEEP_TIMEOUT_MS),
+ NO_CONTROLLER_SLEEP_CHECK_INTERVAL_MS, 24 * 60 * 60 * 1000);
void Settings::setNoControllerSleepTimeout(int timeout_ms) {
- noControllerSleepTimeout = timeout_ms;
+ noControllerSleepTimeout =
+ std::clamp(timeout_ms, NO_CONTROLLER_SLEEP_CHECK_INTERVAL_MS, 24 * 60 * 60 * 1000);
save();
}Also applies to: 380-383
🤖 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/display/core/Settings.cpp` at line 99, The noControllerSleepTimeout
values read via preferences.getInt() are not being validated or clamped to
acceptable ranges before flowing into AutoSleepManager::setTimeoutMs(uint32_t).
Negative, zero, or corrupt values can cause wrap-around or trigger unintended
immediate sleep behavior. Clamp the noControllerSleepTimeout values at both the
point where they are read from preferences and where they are set from incoming
values to ensure they fall within a valid timeout range before being passed to
the AutoSleepManager method.
Adds the Display Settings → Auto Sleep entry (enable toggle + timeout) in web/src/pages/Settings/index.jsx so it's user-configurable, not just API-settable. Fix the backend to use the hasArg presence convention for the checkbox (matching clock24hFormat/autowakeupEnabled). Document the web entry and a hardware test record template. Verified: pio run -e display, web `vite build` + eslint (0 errors). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
Follow-up commits:
|
…ched' test item Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
…ent only The T-RGB has no charge-status pin, so when USB is plugged the ADC reads the charging voltage (~4.2V) and the overlay used to show a misleading "100%". Now voltage >= BATTERY_CHARGING_MV (4200) is treated as on-USB/charging and the overlay shows "⚡ USB" (green) instead of a percentage. On battery it shows the approximate percentage only (voltage text removed, per request). Screenshots + docs updated. Verified: pio run -e display, pio run -e display-sim (normal / charging / low). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
…h wake Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
Charging detection has no percentage of its own; show the (charging-voltage based) percentage next to the bolt so the level is visible while plugged in, instead of just '⚡ USB'. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
We require contributors to sign our Contributor License Agreement, and we don't have yours on file. In order for us to review and merge your code, please contact @jniebuhr (mdwasp) on Discord to get yourself added. |
|
1 participant
Summary
Two display-only features for the LilyGo T-RGB screen (
env:display):Branched from
master.Scope / guardrails
src/display/**,sim/**,docs/**.9 files changed (+190), plus 2 pure-logic headers and 1 doc.
How it works
AutoSleepManager(pure logic): countdown from boot/disconnect, cancels on reconnect; touch resets the UI idle timer but does not block the no-controller countdown; suppressed during OTA/update/Wi-Fi-setup/autotune; sleeps after ¼ timeout when battery is critical. Toggle + timeout inSettings(default on / 2 min), also via the web-config API.BatteryMonitor(pure logic):Driver::getBatteryMilliVolts()→ on T-RGBpanel.getBattVoltage()(GPIO4, averages ~20 reads, handles the 1/2 divider). Sampled every 30 s; coarse Li-ion LUT → percent.SdlDrivermocks the battery (GM_SIM_BATTERY_MV) and logs a sleep event instead of exiting.Caveats
docs/DISPLAY_BATTERY_AUTOSLEEP.md.Testing
pio run -e display✅ ·pio run -e display-sim✅.pio check -e display: no real defects added (only pre-existing cppcheck line-0 macro bailouts).🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
New Features
Updates
Documentation