Show a confirmed shutdown screen before cutting power#555
Conversation
Add DisplayDevice::waitForFlushComplete() (default no-op) so callers can block until a frame handed to the display has physically finished drawing, and use it in the Launcher's power-off handler to show a plain "Tactility OS / Powered off" screen before cutting power. Displays that draw synchronously within their flush callback are unaffected. Displays with an asynchronous refresh pipeline (e.g. e-paper, where a full refresh can take seconds) should override this so a caller doing something irreversible — like cutting power — can be sure the screen physically shows what was just drawn, rather than freezing mid-refresh. On e-paper this also doubles as a real signal that the device shut down cleanly rather than crashed or hung, since the panel keeps showing it after power cuts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis change adds 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 309989ed-f8ac-45e2-9172-2e1fe3c178b0
📒 Files selected for processing (2)
Tactility/Include/Tactility/hal/display/DisplayDevice.hTactility/Source/app/launcher/Launcher.cpp
| lv_obj_set_style_bg_color(screen, lv_color_white(), 0); | ||
| lv_obj_set_flex_flow(screen, LV_FLEX_FLOW_COLUMN); | ||
| lv_obj_set_flex_align(screen, LV_FLEX_ALIGN_CENTER, LV_FLEX_ALIGN_CENTER, LV_FLEX_ALIGN_CENTER); | ||
|
|
||
| auto* title = lv_label_create(screen); | ||
| lv_label_set_text(title, "Tactility OS"); | ||
| lv_obj_set_style_text_font(title, lvgl_get_text_font(FONT_SIZE_LARGE), 0); | ||
|
|
||
| auto* subtitle = lv_label_create(screen); | ||
| lv_label_set_text(subtitle, "Powered off"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Launcher.cpp around target lines ---'
sed -n '1,180p' Tactility/Source/app/launcher/Launcher.cpp
echo
echo '--- Search for text color styling in launcher/app files ---'
rg -n "lv_obj_set_style_text_color|lv_label_set_text_color|lv_obj_set_style_bg_color|Powered off|Tactility OS" Tactility/Source -SRepository: TactilityProject/Tactility
Length of output: 14329
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Search for theme/default style setup ---'
rg -n "lv_theme|default_init|text color|text_color|set_style_text_color|LV_COLOR" Tactility/Source -S
echo
echo '--- Inspect style/theme related files ---'
for f in Tactility/Source/lvgl/Style.cpp Tactility/Source/service/gui/GuiService.cpp Tactility/Source/app/launcher/Launcher.cpp; do
echo "### $f"
wc -l "$f"
done
echo
echo '--- Style.cpp ---'
sed -n '1,220p' Tactility/Source/lvgl/Style.cpp
echo
echo '--- GuiService.cpp around initialization ---'
sed -n '1,200p' Tactility/Source/service/gui/GuiService.cppRepository: TactilityProject/Tactility
Length of output: 14053
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Search for theme initialization / mode selection ---'
rg -n "lv_theme_default_init|lv_theme_set|lv_display_set_theme|LV_THEME|lv_theme_get_color" Tactility/Source -S
echo
echo '--- Search for label text color inheritance utilities or defaults ---'
rg -n "lv_label_create\\(|lv_obj_set_style_text_color\\(.*label|lv_obj_set_style_text_color\\(.*screen" Tactility/Source/app Tactility/Source/lvgl -S
echo
echo '--- Inspect launcher shutdown context lines ---'
sed -n '88,110p' Tactility/Source/app/launcher/Launcher.cppRepository: TactilityProject/Tactility
Length of output: 22946
🌐 Web query:
LVGL label default text color theme inherited from parent background white default theme text color
💡 Result:
In LVGL, text color properties (such as text_color) are inheritable from parent widgets [1][2][3]. If a label does not have a text color explicitly set, it will search up the object hierarchy to find a parent that has a text color style defined [1][2]. Key aspects of this behavior include: 1. Inheritance Mechanism: Inheritance applies only if the property is not explicitly set in the label's own styles (even in the default state) [1][2][3]. If the label has no local style for text color, it will continue to look at its parents until a value is found [1][2]. 2. Theme Influence: LVGL themes apply default styles to widgets upon creation [1][4]. These default styles often include predefined text colors [5][6]. If you want a label to inherit a specific color from a parent (e.g., a container with a white background), ensure the label itself does not have a local text color style, and that the parent or a higher ancestor has the desired text color set [7]. 3. Precedence: Local styles set on the label will always override inherited values [1][2]. If your labels are defaulting to a color that conflicts with your white background, you should either update the parent's style or explicitly add a style to the label that defines the correct text color [8][9]. 4. Troubleshooting: If a label is not inheriting correctly, it is often because an intermediate parent has a style set that defines a text color, preventing the search from reaching the grandparent or intended container [7]. In such cases, removing the style from the intermediate parent or explicitly applying the desired color to the label is necessary [7][10]. For consistent application of colors, you can create a custom style and add it to your labels, or update the active theme's styles [8][6]. If you need to force a color change when a theme changes, ensure your style is linked to the primary or secondary color palette provided by the theme [8].
Citations:
- 1: https://lvgl.io/docs/open/9.3/details/common-widget-features/styles/styles
- 2: https://lvgl.io/docs/open/9.2/overview/style
- 3: https://docs.lvgl.io/8.0/overview/style.html?highlight=set_style_bg_color
- 4: https://lvgl.io/docs/open/common-widget-features/styles/themes
- 5: [6.1.1] Default theme color and fonts support lvgl/lvgl#1425
- 6: https://forum.lvgl.io/t/is-there-a-default-style-that-can-be-redefined/18400
- 7: Add 'inherit' and 'initial' CSS properties lvgl/lvgl#3086
- 8: https://forum.lvgl.io/t/how-to-change-label-colors-by-changing-theme/3904
- 9: Colouring issue lvgl/lvgl#862
- 10: When the group is focused on the button，how to show the label "text" of the button lvgl/lvgl#318
🌐 Web query:
LVGL default label text color theme defaults light text on white background label inherits style documentation
💡 Result:
In LVGL, the default appearance of a label is determined by the active theme, which is initialized automatically if the default theme is enabled in your configuration (LV_USE_THEME_DEFAULT) [1]. When using this default theme, text color defaults are predefined based on whether the theme is set to light or dark mode [2][1]. Regarding your specific query on text inheritance and styling: 1. Style Inheritance: LVGL supports style inheritance for text-related properties (such as text color and font) [3][4]. If a property is not explicitly set for a specific widget (including a label), LVGL will search up the object hierarchy (parent to child) until it finds a widget that specifies a value for that property [3][5]. 2. Theme Defaults: When the default theme is active, it applies styles to various widgets. If you require a specific behavior—such as white text on a white background—it is generally recommended to explicitly set the style for that label or its parent container, as the default theme aims for contrast rather than monochromatic rendering [1]. 3. How to Override: - To change text color locally: You can use lv_obj_set_style_text_color(obj, color, selector) to apply a specific color to a label [1]. - To change inheritance: By setting a style on a parent container, all child labels that do not have their own explicit text color will inherit the parent's color [3][4]. For detailed implementation, consult the LVGL Styles Overview documentation, which outlines how properties cascade, how inheritance functions, and how to use local styles to override theme defaults [3][6].
Citations:
- 1: https://docs.lvgl.io/8.0/overview/style.html?highlight=set_style_bg_color
- 2: https://lvgl.io/docs/open/9.5/API/themes/default/lv_theme_default_h.html
- 3: https://lvgl.io/docs/open/common-widget-features/styles/overview
- 4: https://lvgl.github.io/open-docs/HTML/9.5/common-widget-features/styles/overview.html
- 5: https://lvgl.io/docs/open/9.1/overview/style.html
- 6: https://lvgl.io/docs/open/9.5/common-widget-features/styles/overview
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- Search for LVGL theme configuration / dark mode settings ---'
rg -n "LV_USE_THEME_DEFAULT|LV_THEME_DEFAULT|DARK|LIGHT|theme.*dark|theme.*light|lv_theme_default_init|lv_display_set_theme|lv_theme_set" -S .
echo
echo '--- Search config-like files for LVGL theme flags ---'
fd -t f 'lv_conf.h|sdkconfig|config.*|*.conf|*.ini|*.json|*.yaml|*.yml' . | sed -n '1,200p'Repository: TactilityProject/Tactility
Length of output: 7299
Set the shutdown labels to a dark color. The default dark theme can leave these labels light on the white powered-off screen, making them hard to read.
| if (display != nullptr) { | ||
| auto* lvgl_display = display->getLvglDisplay(); | ||
| if (lvgl_display != nullptr) { | ||
| lv_refr_now(lvgl_display); | ||
| } | ||
| display->waitForFlushComplete(); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and nearby symbols first.
ast-grep outline Tactility/Source/app/launcher/Launcher.cpp --view expanded || true
# Read the relevant section with line numbers.
sed -n '1,220p' Tactility/Source/app/launcher/Launcher.cpp | cat -n
# Find the display-device API and the launcher call sites.
rg -n "findFirstDevice<DisplayDevice>|getLvglDisplay|waitForFlushComplete|lv_refr_now|lv_obj_get_display|lv_event_get_target_obj|showPoweredOffScreenAndWait" Tactility/Source -S
# Inspect the display device implementation and LVGL ownership helpers.
fd -a "DisplayDevice" Tactility/SourceRepository: TactilityProject/Tactility
Length of output: 15898
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the display-device interface and implementation.
fd -a "DisplayDevice" Tactility/Source | sed -n '1,20p'
ast-grep outline Tactility/Source/hal/display/DisplayDevice.h --view expanded || true
ast-grep outline Tactility/Source/hal/display/DisplayDevice.cpp --view expanded || true
# Inspect the relevant parts of the files if they exist.
for f in \
Tactility/Source/hal/display/DisplayDevice.h \
Tactility/Source/hal/display/DisplayDevice.cpp \
Tactility/Source/lvgl/Lvgl.cpp
do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
fi
done
# Show the display-device declarations/definitions and LVGL startup wiring.
for f in \
Tactility/Source/hal/display/DisplayDevice.h \
Tactility/Source/hal/display/DisplayDevice.cpp \
Tactility/Source/lvgl/Lvgl.cpp
do
if [ -f "$f" ]; then
echo "===== $f ====="
sed -n '1,260p' "$f" | cat -n
fi
doneRepository: TactilityProject/Tactility
Length of output: 8428
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the display device interface and the device registry semantics.
rg -n "class DisplayDevice|struct DisplayDevice|findFirstDevice<|findDevices<|supportsLvgl|getLvglDisplay|waitForFlushComplete|startLvgl\\(" Tactility/Source -S
# Find where the launcher screen is created/attached to a display.
rg -n "Launcher|showPoweredOffScreenAndWait|onPowerOffPressed|lv_screen_load|lv_obj_get_display\\(parent\\)|lv_obj_get_display\\(.*launcher|power off" Tactility/Source/app Tactility/Source/lvgl -S
# Inspect the device registry implementation if present.
fd -a "Device" Tactility/SourceRepository: TactilityProject/Tactility
Length of output: 7157
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the launcher's power-button setup and the surrounding display selection logic.
sed -n '200,280p' Tactility/Source/app/launcher/Launcher.cpp | cat -n
# Check whether the launcher or nearby code explicitly selects a display from the LVGL object tree.
rg -n "lv_obj_get_display\\(|lv_screen_load\\(|power_button|onPowerOffPressed|onShow\\(" Tactility/Source/app/launcher/Launcher.cpp Tactility/Source/app/boot/Boot.cpp Tactility/Source/app/display/Display.cpp -S
# Read the boot/display app code that picks the display device.
sed -n '1,120p' Tactility/Source/app/display/Display.cpp | cat -n
sed -n '1,260p' Tactility/Source/app/boot/Boot.cpp | cat -nRepository: TactilityProject/Tactility
Length of output: 23251
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the device registry and any helpers that define first-device selection order.
sed -n '1,220p' Tactility/Source/hal/Hal.cpp | cat -n
# Inspect the app/display service that creates visible screens, if present.
rg -n "lv_screen_load|lv_obj_get_screen|lv_display_get_default|findFirstDevice<hal::display::DisplayDevice>|redraw|AppContext|create<.*App>|startService\\(\"Gui\"\\)" Tactility/Source/service Tactility/Source/app Tactility/Source/hal -S
# Read any compact file that appears to manage app-to-screen attachment.
fd -a "Gui" Tactility/Source/service Tactility/Source/app Tactility/Source/halRepository: TactilityProject/Tactility
Length of output: 22189
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect GUI service redraw/app instantiation to see whether apps are per-display or shared.
sed -n '1,360p' Tactility/Source/service/gui/GuiService.cpp | cat -n
# Inspect AppContext / LoaderService ownership if needed.
sed -n '1,360p' Tactility/Source/service/loader/Loader.cpp | cat -n
sed -n '1,240p' Tactility/Source/app/App.cpp | cat -nRepository: TactilityProject/Tactility
Length of output: 26223
Refresh the launcher’s own display before power-off. findFirstDevice<DisplayDevice>() can return a different panel than the one hosting the power button, so lv_refr_now() and waitForFlushComplete() may run on the wrong display or be skipped. Use the event target’s lv_display_t and match the corresponding DisplayDevice before cutting power.
The power-off button was one accidental tap away from cutting power
with no way back. Show an alert dialog ("Power off?" / Cancel) and
only proceed to the shutdown screen + PowerDevice::powerOff() once the
user picks "Power off".
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
It's a great idea for a feature, but the way it's implemented doesn't work well as it puts unnecessary functionality in the Launcher which should only be responsible about basic launcher tasks and not power off logic: I'd rather see this as a separate application. It can even be a full screen app (set flag in the app manifest to hide the system top bar) and have a text with "Power off?" and buttons "Yes" and "No" underneath. Once "Yes" is pressed, e-paper behaviour can be put in place if the display supports it. |
showPoweredOffScreenAndWait() is called from App::onResult, which runs on the loader dispatcher thread — not the LVGL thread. Every other piece of code in this project that touches LVGL objects from a non-LVGL context (e.g. DisplayIdleService's screensaver overlay) acquires the LVGL lock first; this call site didn't, so it was racing the LVGL render task. Confirmed on hardware: without the lock, the shutdown screen sometimes failed to render correctly before power cut. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed a follow-up fix: while testing the confirmation dialog on hardware, I found `showPoweredOffScreenAndWait()` (added in this PR) was calling LVGL functions without acquiring the LVGL lock. It's invoked from `App::onResult`, which runs on the loader dispatcher thread rather than the LVGL thread, so it was racing the LVGL render task — every other place in the codebase that touches LVGL from a non-LVGL context (e.g. `DisplayIdleService`'s screensaver overlay) already does this. Confirmed on hardware: without the lock, the shutdown screen intermittently failed to render correctly before the board's power rail cut. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Tactility/Source/app/launcher/Launcher.cpp (1)
92-151: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftConsider moving the shutdown flow out of Launcher.
The launcher now owns confirmation UI, final-screen rendering, display-specific flush waiting, and power HAL orchestration. A dedicated full-screen shutdown app would keep Launcher focused and let the manifest hide system chrome for the confirmation/final screen. Based on the PR comments summary recommending a separate full-screen power-off app.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3d6fff74-bc99-440c-a4a0-2ffc697c9f6b
📒 Files selected for processing (1)
Tactility/Source/app/launcher/Launcher.cpp
| static void showPoweredOffScreenAndWait(hal::display::DisplayDevice* display) { | ||
| if (!lvgl::lock(lvgl::defaultLockTime)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Do not cut power when the shutdown screen was skipped.
If lvgl::lock() times out, showPoweredOffScreenAndWait() returns silently and performPowerOff() still calls powerOff(), so the confirmed shutdown can cut power without drawing/flushing the final screen.
🐛 Proposed fix
- static void showPoweredOffScreenAndWait(hal::display::DisplayDevice* display) {
+ static bool showPoweredOffScreenAndWait(hal::display::DisplayDevice* display) {
if (!lvgl::lock(lvgl::defaultLockTime)) {
- return;
+ return false;
}
@@
lvgl::unlock();
+ return true;
}
@@
auto display = hal::findFirstDevice<hal::display::DisplayDevice>(hal::Device::Type::Display);
- showPoweredOffScreenAndWait(display.get());
+ if (!showPoweredOffScreenAndWait(display.get())) {
+ return;
+ }
power->powerOff();Also applies to: 137-140
Launcher was doing too much (Ken review, PR TactilityProject#555): the confirmation dialog, the 'powered off' shutdown screen, and the power-off call all lived in LauncherApp. Move that into a standalone PowerOff app (HideStatusBar + Hidden) with its own Yes/No buttons; Launcher now just starts it. As a side effect, the LVGL-lock workaround from the previous commit is no longer needed: the power-off flow now runs from an LVGL button click callback (already on the LVGL thread) instead of from App::onResult (loader dispatcher thread), so it can call LVGL functions directly.
CodeRabbit flagged this on the original PR: the screen forces a white background, but the title/subtitle labels had no explicit text color, so the default theme's (potentially light) text color could render unreadable on it.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Tactility/Source/app/poweroff/PowerOff.cpp (1)
23-59: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDisplay used for the shutdown screen may not be the display resolved for flush-wait.
lv_obj_create(nullptr)(line 24) attaches the new screen to LVGL's default display, whiledisplay(used forlv_refr_now/waitForFlushComplete) is independently resolved viahal::findFirstDevice<hal::display::DisplayDevice>()at line 55. In a multi-display setup these can diverge, so the flush-wait could target/skip the wrong panel — the same concern previously raised (and unresolved) when this logic lived inLauncher.cpp, now carried over into this new file.Consider resolving the
DisplayDevicefrom the actuallv_display_tthe screen was loaded on (e.g. vialv_obj_get_display(screen)and matching it against registeredDisplayDevice::getLvglDisplay()instances) rather than blindly taking the first registered display device.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 66e68510-3a23-4018-8b8b-bb3eb12ee20b
📒 Files selected for processing (3)
Tactility/Source/Tactility.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/poweroff/PowerOff.cpp
Per further discussion of Ken's PR feedback: the power button itself shouldn't live in the Launcher UI either, not just the power-off logic. Drop the power icon from Launcher entirely and list PowerOff as a regular Settings entry (Category::Settings, own icon), the same way the existing Power (battery info) app is reached.
2 participants
Small, standalone core change — no board-specific code, doesn't depend on any of my other open PRs.
Motivation
Today,
Launcher.cpp's power-off button callsPowerDevice::powerOff()directly, with no visual feedback. On a normal display that's fine (the screen goes dark with the power rail). On an e-paper display, though, the panel keeps showing whatever was drawn last — so if power cuts mid-refresh (or while still showing the app the user was in), there's no way to tell "the device shut down cleanly" from "the device crashed or hung."I hit this while building an e-paper board (T-Deck Max, #539/#552/#553): those PRs'
GDEQ031T10driver refreshes asynchronously on a background task (needed to keep input responsive during multi-second e-paper refreshes), so simply drawing a message isn't enough — the caller needs to know the panel has physically finished drawing before the power rail dies, or the message itself could get cut off mid-refresh.What this adds
DisplayDevice::waitForFlushComplete()— a new virtual, default no-op. Displays that draw synchronously in their flush callback (i.e. every current upstream display) are completely unaffected. Displays with an async refresh pipeline can override it to block until any pending frame has physically finished drawing.Launcher.cpp's power-off handler now shows a plain "Tactility OS / Powered off" screen, forces a synchronous LVGL redraw (lv_refr_now), callswaitForFlushComplete(), and only then callspowerOff().No existing driver needs changes — the new virtual defaults to a no-op everywhere, so this is a pure API addition plus a small UX improvement for every board with a power-off-capable device.
Verification
GDEQ031T10driver + awaitForFlushComplete()override): pressed the power-off button, the "Tactility OS / Powered off" screen rendered fully before the board's power rail cut, and the panel kept showing it after power-off (no garbled/partial refresh). Device powered back on cleanly when USB was reconnected.🤖 Generated with Claude Code
Summary by CodeRabbit