Restore the termination guarantee in the sleep brightness step-down - #2468
Open
fluffyspace wants to merge 1 commit into
Open
Restore the termination guarantee in the sleep brightness step-down#2468fluffyspace wants to merge 1 commit into
fluffyspace wants to merge 1 commit into
Conversation
The GoToSleep/GoToAOD handler steps the backlight down before sleeping:
while (brightnessController.Level() != Levels::Low) {
brightnessController.Lower();
vTaskDelay(100);
}
This was safe for about three years. It originally targeted Levels::Off --
the bottom of the ladder -- and Lower() walks monotonically downwards to
Off, so the loop terminated from every level.
3dca742 ("aod: PPI/RTC-based backlight brightness") changed the target from
Off to Low and, in the same commit, inserted AlwaysOn into the enum between
Off and Low:
enum class Levels { Off, Low, Medium, High }; // before
enum class Levels { Off, AlwaysOn, Low, Medium, High }; // after
Lower() is a no-op at both Off and AlwaysOn. The target moved from the
bottom of the ladder to the middle while two levels that cannot be lowered
were placed beneath it, so the loop can no longer terminate if the handler
is entered at either -- two of the five levels. AlwaysOn is also the level
that AOD itself sets.
Step down only from above Low, then set the level outright.
If the handler is ever entered that way the failure mode is a bad one. The
loop delays rather than spins, so nothing is starved: SystemTask keeps
running and keeps kicking the watchdog, so there is no reset. DisplayApp
never returns from Refresh(), never sends OnDisplayTaskSleeping and never
redraws, so SystemTask stays in GoingToSleep indefinitely. The result is a
dark, button-deaf watch that is nonetheless fully alive -- still advertising
and still serving BLE -- recoverable only by holding the button to force a
watchdog reset.
A second unbounded loop in the same handler, `while (!lv_task_handler()) {}`
(added by 2625ed3 for the go-to-clock-on-sleep path), has the same shape
and the same consequence if LVGL never reports work done. Bounded here too.
I have not found a reachable path that enters this handler below Low on
current main, so this is not backed by a reproducer. It restores an
invariant that was lost, rather than fixing an observed hang.
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
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.
The regression
The
GoToSleep/GoToAODhandler steps the backlight down before sleeping:This was safe for about three years. It originally targeted
Levels::Off— the bottom of the ladder — andLower()walks monotonically downwards toOff, so it terminated from every level.3dca742b("aod: PPI/RTC-based backlight brightness") changed the target fromOfftoLowand, in the same commit, insertedAlwaysOninto the enum betweenOffandLow:Lower()is a no-op at bothOffandAlwaysOn. So the target moved from the bottom of the ladder to the middle, while two levels that cannot be lowered were placed beneath it. The loop can no longer terminate if the handler is entered at either — two of the five levels.AlwaysOnis also the level AOD itself sets.You can confirm the whole story from history:
Why the failure mode is worth caring about
The loop delays rather than spins, so nothing is starved. SystemTask keeps running and keeps kicking the watchdog, so there is no reset to recover things. DisplayApp never returns from
Refresh(), never sendsOnDisplayTaskSleepingand never redraws, so SystemTask stays inGoingToSleepindefinitely.The result is a watch that is dark and deaf to the button while being fully alive — still advertising, still serving BLE reads, uptime still climbing. It looks exactly like a dead battery or dead firmware, and the only way out is holding the button to force a watchdog reset. That is a very expensive thing to debug from the outside; it cost me two evenings before I instrumented the watch and could see SystemTask parked in
GoingToSleep.The change
Step down only from above
Low, then set the level outright. A second unbounded loop in the same handler,while (!lv_task_handler()) {};(added by2625ed39for the go-to-clock-on-sleep path), has the same shape and the same consequence if LVGL never reports work done — bounded here too.What I have and have not verified
Verified: the regression is plain from git history, and the fix builds clean and runs on a PineTime (bootloader 1.0.1, 1.16.0) with normal sleep/wake behaviour — brightness stepping down, screen waking on the button,
GoingToSleep → Sleepingcompleting.Not verified: I have not found a reachable path that enters this handler below
Lowon currentmain, so I cannot offer a reproducer. To be straight about how I got here — I was chasing a wedge on my own watch with exactly the signature above, found this loop while reading the sleep path, and only discovered the regression afterwards. My watch has AOD off, soAlwaysOnwas never set on it and this loop is unlikely to have been my actual culprit. I am still hunting that separately.So this is offered as restoring an invariant that a specific commit removed, not as the fix for an observed hang. Neither loop has a safe reason to be unbounded, and the cost of being wrong about reachability is a watch that has to be force-reset.
Related: #2165 fixed a different stale-message bug in this same handler, and noted the state handling here is "quite overcomplicated". Possible minor conflict with the stale #1870, which also touches
DisplayApp.cppandBrightnessController.