Power button short press when sleeping refreshes TRMNL#4
Open
mcmphd wants to merge 8 commits into
Open
Conversation
… place Short-pressing the power button while the device is deep-asleep with the TRMNL sleep screen active now re-fetches and re-renders the TRMNL image and goes straight back to sleep, instead of either silently doing nothing (the previous IGNORE default) or fully booting into Home/Reader. A long press continues to wake normally, unchanged. Root cause / design: SHORT_PWRBTN (the existing configurable "what does a short press do" enum, e.g. FORCE_REFRESH) only dispatches from the main loop, once the device has *already* fully booted -- it never fires while still asleep. What actually gates a press while asleep is HalGPIO::verifyPowerButtonWakeup(), called from setup() before any activity is pushed. Wiring this feature through SHORT_PWRBTN as originally scoped would still flash Home/Reader before refreshing; the correct hook is the wake gate itself. verifyPowerButtonWakeup is refactored (behavior-preserving) to delegate its calibrated press-duration polling to a new HalGPIO::measurePowerButtonPressWasShort(), which the new TRMNL path also calls -- always allowing the boot to continue (shortPressAllowed=true) when sleepScreen==TRMNL, then measuring short vs. long against the existing long-press threshold to decide refresh-in-place vs. normal wake. The refresh itself reuses enterDeepSleep() -> activityManager.goToSleep() -> SleepActivity::renderTrmnlSleepScreen(), the same call the periodic TRMNL refresh already uses, rather than duplicating it. enterDeepSleep() gained a preserveLastSleepFromReader parameter: called this early (before Home/Reader/Boot is ever pushed), activityManager.isReaderActivity() would wrongly report false and corrupt APP_STATE.lastSleepFromReader, breaking reader-resume on the next real wake. All other call sites are unaffected (default false). A new "Refreshing" status string (STR_REFRESHING) is shown in place of the "BOOTING" splash during this path, which skips the splash entirely. Not gated behind a new SHORT_PWRBTN option: that enum doesn't govern this code path at all (see above), so adding one wouldn't be reachable here. Instead this activates automatically whenever sleepScreen==TRMNL, which is itself an explicit, existing user choice. Verification: pio run -e simulator builds clean. The simulator's wake path (external crosspoint-simulator dependency, not part of this repo) cannot measure press duration at all -- confirmed via .pio/libdeps/simulator/simulator/src/HalGPIO.cpp, where verifyPowerButtonWakeup is a no-op and any keypress triggers an immediate reboot regardless of duration -- so measurePowerButtonPressWasShort is only compiled for real hardware (#ifndef SIMULATOR) and the short/long distinction itself needs a real device flash to confirm. The rest of the new logic (wake routing, display setup, "Refreshing" render, TRMNL fetch with graceful fallback on missing config, SleepActivity entry, clean re-sleep, and the lastSleepFromReader preservation fix) was verified by directly driving the simulator's WakeupReason::PowerButton path via CROSSPOINT_SIM_WAKE_REASON=power against a local settings file with sleepScreen=TRMNL, temporarily forcing the refresh branch on to confirm it runs end-to-end with no crashes, and confirming lastSleepFromReader survives the refresh when set to true beforehand. The temporary override is not part of this commit. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The short-press-refreshes-TRMNL-in-place behavior from the previous commit was unconditional whenever sleepScreen==TRMNL, with no way to opt out. Add Settings > System > TRMNL Settings > Refresh on Power Button, off by default (matching the Extended Wi-Fi Timeout toggle's convention of preserving prior behavior unless explicitly enabled). - CrossPointSettings: new trmnlRefreshOnPowerButton field, registered in getSettingsList() for JSON persistence (same pattern as trmnlOrientation and trmnlExtendedWifiTimeout). - TrmnlSettingsActivity: new list item, toggled and saved the same way as the other TRMNL settings. - main.cpp: trmnlSleepScreenActive now also requires the toggle, which gates both the wake-gate fast-path (shortPressAllowed) and the press-duration measurement -- off means byte-for-byte the pre-feature wake behavior. Verified: pio run -e simulator builds clean. Confirmed via CROSSPOINT_SIM_WAKE_REASON=power against a local settings file with sleepScreen=TRMNL that shortAllowed logs 0 with the new field absent/0 (default) and 1 once set to 1, matching the gate's intent. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…fresh
Two text tweaks to the refresh-in-place flow, requested after trying it
conceptually: the transient full-screen status shown at the very start
now reads as a usage hint rather than a bare status word, and the "Going
to sleep" popup that SleepActivity always shows on the way down now says
"Refreshing" specifically for this path (unchanged for every other sleep
trigger/mode).
- showTrmnlRefreshingStatus() now shows two lines ("Short press power to
refresh" / "Long press power to wake up") instead of just "Refreshing".
- SleepActivity gains an isPowerButtonRefresh constructor flag (threaded
through ActivityManager::goToSleep() and enterDeepSleep(), replacing
the narrower preserveLastSleepFromReader flag added previously -- this
is the more accurate name for what that flag actually represents, and
now drives two behaviors instead of one). onEnter() swaps its "Going to
sleep" popup for "Refreshing" only when this flag is set; every other
sleep-screen mode and sleep trigger (timeout, manual sleep, etc.) is
unaffected.
- STR_REFRESHING moves from the BOOTING-style all-caps splash convention
to the ENTERING_SLEEP-style sentence-case popup convention, since it's
no longer used for the former.
Verified: pio run -e simulator builds clean. Re-ran the same
CROSSPOINT_SIM_WAKE_REASON=power + temporarily-forced-refresh-branch
check used for the original feature commit; confirmed the same
crash-free activity sequence (status screen -> SleepActivity -> TRMNL
fetch fallback -> deep sleep) with the new strings compiled in. Exact
on-device text rendering (line wrap, popup sizing) still needs a real
device flash to see visually -- not verifiable in this simulator per
the original commit's limitations.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The refresh-in-place status screen (showTrmnlRefreshingStatus) explains what the buttons do but didn't say where the power button physically is. Add a simple up-pointing chevron-and-shaft arrow in the top-left corner of the screen, matching the X4's physical power button placement (top edge, offset left) as confirmed by the user. Reuses the existing stacked-widening-lines chevron technique already used for BaseTheme's list scroll arrows (src/components/themes/ BaseTheme.cpp) rather than introducing a new drawing primitive or a bitmap asset -- there were no existing button-hint diagrams or arrow assets anywhere in the repo to reuse (confirmed by search). Text casing left unchanged (sentence-style, matching Apple HIG guidance: capitalize only the first word, no title-case/all-caps for generic hardware terms like "power"). Verified: pio run -e simulator builds clean. Re-ran the same forced- refresh-branch trace used for prior commits on this branch to confirm the arrow-drawing code executes without crashing. Pixel-level visual verification (arrow proportions, exact on-screen position) was not possible in this environment -- the simulator binary isn't a registered macOS app bundle, so GUI screenshot tooling can't target its window, and hand-decoding the raw rotated 1bpp framebuffer risked a wrong read more than it was worth for a cosmetic check. Geometric placement was sanity- checked against the known 480x800 logical portrait canvas (arrow head spans roughly x=33-47, y=24-32; shaft to y=52; well clear of the text centered around y=400, no clipping or overlap). Real device flash will be the actual visual confirmation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The previous commit placed the arrow at the top-left corner based on an initial rough description. Corrected per an actual physical measurement: the power button is on the device's right edge, about 20% of the way down from the top (~22mm). Replaces the up-pointing chevron with a right-pointing arrow (shaft plus a two-line diagonal head, mirroring BaseTheme's keyboard backspace arrow rotated 180 degrees) positioned at the right edge, y = pageHeight/5. Verified: pio run -e simulator builds clean. Re-ran the same forced- refresh-branch trace as prior commits to confirm no crash. Geometry sanity-checked against the 480x800 logical portrait canvas: arrow tip at x=456, head spans y=152-168, shaft to x=432 -- within bounds, clear of the text block centered around y=400. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Reviewer's GuideImplements an optional feature where a short power-button press while the TRMNL sleep screen is active refreshes the screen in place and immediately re-enters deep sleep, including input-duration measurement on real hardware, new settings plumbing, and sleep/boot UI updates, while preserving existing long-press wake behavior. Sequence diagram for short power-button TRMNL refresh-in-place boot pathsequenceDiagram
actor User
participant HalGPIO as HalGPIO
participant Main as setup
participant Renderer as GfxRenderer
participant ActivityMgr as ActivityManager
participant SleepAct as SleepActivity
User->>HalGPIO: Power button press (device in TRMNL sleep)
HalGPIO-->>Main: getWakeupReason() == PowerButton
Main->>HalGPIO: verifyPowerButtonWakeup(getPowerButtonWakeDuration, shortPressAllowed)
Note over Main,HalGPIO: Short press allowed when TRMNL sleep screen active
Main->>HalGPIO: measurePowerButtonPressWasShort(getPowerButtonLongPressDuration)
HalGPIO-->>Main: true (short press)
Main->>Main: powerButtonWakeIsTrmnlRefresh = true
Main->>Renderer: setupDisplayAndFonts(seamless=true)
Main->>Renderer: showTrmnlRefreshingStatus()
Main->>ActivityMgr: enterDeepSleep(fromTimeout=false, isPowerButtonRefresh=true)
ActivityMgr->>SleepAct: SleepActivity(..., fromTimeout=false, isPowerButtonRefresh=true)
SleepAct->>Renderer: GUI.drawPopup(STR_REFRESHING)
SleepAct->>SleepAct: renderTrmnlSleepScreen()
SleepAct->>HalGPIO: esp_deep_sleep_start() (via enterDeepSleep)
HalGPIO-->>User: Device returns to deep sleep with refreshed TRMNL screen
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
TrmnlSettingsActivity, the hard-coded index4for the new "Refresh on Power Button" option is brittle; consider introducing a named constant or deriving indices from themenuNamesarray to keep the selection/render logic in sync if items are reordered. HalGPIO::measurePowerButtonPressWasShortis now used with both the wake-duration and the long-press duration thresholds; it would be clearer to document this explicitly or split into two helpers (e.g.,isShortPressForWakevsisShortPressForLongPressDetection) so the differing semantics are obvious at call sites.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `TrmnlSettingsActivity`, the hard-coded index `4` for the new "Refresh on Power Button" option is brittle; consider introducing a named constant or deriving indices from the `menuNames` array to keep the selection/render logic in sync if items are reordered.
- `HalGPIO::measurePowerButtonPressWasShort` is now used with both the wake-duration and the long-press duration thresholds; it would be clearer to document this explicitly or split into two helpers (e.g., `isShortPressForWake` vs `isShortPressForLongPressDetection`) so the differing semantics are obvious at call sites.
## Individual Comments
### Comment 1
<location path="lib/hal/HalGPIO.cpp" line_range="252" />
<code_context>
+void HalGPIO::verifyPowerButtonWakeup(uint16_t requiredDurationMs, bool shortPressAllowed) {
</code_context>
<issue_to_address>
**issue (bug_risk):** Short presses now always force deep sleep, ignoring `shortPressAllowed` and changing `verifyPowerButtonWakeup` semantics.
Previously this function distinguished between short presses that should be rejected and short presses that were explicitly allowed (via `shortPressAllowed`, e.g. `SHORT_PWRBTN::SLEEP`). With the new `measurePowerButtonPressWasShort(requiredDurationMs)` call and unconditional `startDeepSleep()` on `true`, all short presses now trigger deep sleep, ignoring `shortPressAllowed`.
This means:
- Configurations that allow short presses will still be put back to sleep, breaking the existing API contract.
- TRMNL refresh short presses are likely to be consumed here before `setup()` can handle them, making `powerButtonWakeIsTrmnlRefresh` hard or impossible to reach.
To preserve behavior while reusing the timing logic, you could:
- Use `measurePowerButtonPressWasShort` only to obtain `bool isShort`, then apply the previous decision tree using `shortPressAllowed`, or
- Keep `verifyPowerButtonWakeup` as the wake gatekeeper and move the TRMNL-refresh decision (using `measurePowerButtonPressWasShort`) into `setup()` only.
Without such changes, this is likely to regress power‑button behavior and conflict with the new refresh-in-place feature.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
- TrmnlSettingsActivity: replace the hard-coded index 4 with a named
MENU_INDEX_REFRESH_ON_POWER_BUTTON constant, matching the review's
suggestion. Keeps handleSelection()/render() in sync if items are ever
reordered.
- HalGPIO::measurePowerButtonPressWasShort is a single generic duration-
vs-threshold primitive deliberately reused from two call sites with
different thresholds and meanings (noise-filter gate inside
verifyPowerButtonWakeup vs. short/long classification in main.cpp's
TRMNL routing). Rather than splitting into two identically-implemented
wrapper functions, documented this explicitly at the declaration and at
both call sites -- the review offered either as an acceptable fix, and
the docs option avoids adding two functions whose only difference is
their name. Renamed the parameter from longPressDurationMs to the more
accurate durationMs since it isn't always a long-press duration.
Investigated but did NOT act on the review's third comment (bug_risk on
HalGPIO.cpp:252): it claimed verifyPowerButtonWakeup now ignores
shortPressAllowed and always forces deep sleep on a short press,
breaking the TRMNL refresh path. That's not what the code does -- the
`if (shortPressAllowed) { return; }` guard is unchanged from before this
feature (confirmed against the original commit's diff, which only
touched code after that guard) and still short-circuits before
measurePowerButtonPressWasShort/startDeepSleep are ever reached. Since
shortPressAllowed is true precisely when the TRMNL refresh path is
active, the claimed regression can't occur. Restructuring working code
to fix a bug that doesn't exist would add risk for no benefit, so left
the logic as-is and instead strengthened the guard's comment (see above)
so the same misread doesn't happen again on a future review.
Verified: pio run -e simulator builds clean. Re-ran the same forced-
refresh-branch trace used for every commit on this branch to confirm
behavior is unchanged (doc/naming-only change, no logic touched).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Short-pressing the power button while the device is deep-asleep with the TRMNL
sleep screen active did nothing; a long press fully booted into
Home/Reader — there was no way to refresh the TRMNL sleep screen and immediately go back to sleep.
Design
SHORT_PWRBTN(the existing "what does a short press do" enum) only dispatchesonce the device has already fully booted — it never fires while still asleep.
What actually gates a press while asleep is
HalGPIO::verifyPowerButtonWakeup(),called from
setup()before any activity is pushed. This feature hooks in thereinstead, reusing
enterDeepSleep()→SleepActivity::renderTrmnlSleepScreen()(the same call the periodic TRMNL refresh already uses) rather than duplicating it.
Off by default:
Settings > System > TRMNL Settings > Refresh on Power Button.Long-press wake is completely unchanged.
While refreshing, the screen shows a hint ("Short press power to refresh" / "Long
press power to wake up") with an arrow pointing at the physical power button, and
the usual "Going to sleep" popup reads "Refreshing" for this path specifically —
every other sleep trigger/mode is unaffected.
Verification
pio run -e simulatorbuilds clean. The simulator can't measure press durationat all (confirmed via the external
crosspoint-simulatordependency's stubbedverifyPowerButtonWakeup), someasurePowerButtonPressWasShortis compiled onlyfor real hardware (
#ifndef SIMULATOR). Wake routing, the settings toggle, thepopup/hint text swap, the arrow, and a
lastSleepFromReaderpreservation fixthis path needed were verified in the simulator by directly driving
WakeupReason::PowerButtonviaCROSSPOINT_SIM_WAKE_REASON=powerandtemporarily forcing the refresh branch on to confirm no crashes end-to-end.
Confirmed on a real X4 device:
reader
default, only takes effect once enabled)
Summary by Sourcery
Enable short power-button presses during TRMNL sleep to refresh the TRMNL screen in place and immediately return to sleep, gated by a new user setting.
New Features:
Bug Fixes:
Enhancements:
Documentation: