feat: add SDL support - #227
Conversation
|
facing this issue as well: lvgl/lvgl#9237 |
|
does this enable iterating on the UI without flashing a device ? |
yes, but that can be done with native/X11, too |
|
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:
📝 WalkthroughWalkthroughLVGL is upgraded to 9.5 with expanded configuration, SDL display support is added, platform-specific timing is revised, and TFT UI styling is updated across maps, nodes, messages, connectivity indicators, and chat controls. ChangesLVGL display updates
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DisplayDriverFactory
participant SDLDriver
participant DisplayDriver
participant LVGL
DisplayDriverFactory->>SDLDriver: create(width, height)
SDLDriver->>DisplayDriver: initialize base driver
SDLDriver->>LVGL: create SDL display and input devices
SDLDriver->>DisplayDriver: run display task handler
SDLDriver->>LVGL: advance LVGL ticks
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 6
🧹 Nitpick comments (1)
library.json (1)
24-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the LVGL version banner in
include/lv_conf.h
library.jsonpins LVGLv9.5.0, but the header still saysv9.2.2. Keep the comment aligned with the dependency.🤖 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 `@library.json` at line 24, Update the LVGL version banner comment in lv_conf.h to state v9.5.0, matching the version pinned by the lvgl/lvgl dependency in library.json.
🤖 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 `@include/graphics/driver/SDLDriver.h`:
- Around line 4-9: Update the documentation comment above the SDL driver class
to reference SDL2 linking, replacing the misleading -lSDL flag with -lSDL2 or
the project’s established SDL2 build configuration. Keep the existing usage and
platform guidance unchanged.
In `@include/lv_conf.h`:
- Around line 288-292: Update the SDL and X11 driver preprocessor guards to use
value-based checks, replacing `#ifdef/`#if defined(...) checks with `#if` USE_SDL or
`#if` USE_X11 as appropriate. Ensure USE_SDL=0 and USE_X11=0 disable their
respective paths while nonzero values enable them, including the related guards
in include/lv_conf.h.
In `@source/graphics/driver/X11Driver.cpp`:
- Around line 32-49: Update X11Driver::task_handler to remove both lv_tick_inc()
calls while preserving the existing duration-based sleep behavior in both
branches.
In `@source/graphics/TFT/TFTView_320x240.cpp`:
- Around line 7144-7146: Replace the unsafe strcpy call in the no-new-messages
branch with a bounded copy using snprintf or strlcpy, ensuring the translated
string is truncated safely and buf remains null-terminated within its 64-byte
capacity.
- Around line 5185-5187: Replace the sprintf call formatting the MAC address in
the Bluetooth label update with snprintf, passing the size of buf so writes are
bounded while preserving the existing output format and lv_label_set_text call.
- Line 244: Rename the LVGL style setter calls throughout the TFT view
implementation from the deprecated `bg_img` spelling to the LVGL 9 `bg_image`
spelling, including `lv_obj_set_style_bg_img_recolor` and all other `_img_`
style setters in this file. Preserve each call’s existing arguments and
behavior.
---
Nitpick comments:
In `@library.json`:
- Line 24: Update the LVGL version banner comment in lv_conf.h to state v9.5.0,
matching the version pinned by the lvgl/lvgl dependency in library.json.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c1b4c862-46f2-4655-a716-8bdfe1bc0464
📒 Files selected for processing (10)
include/graphics/driver/SDLDriver.hinclude/graphics/driver/TFTDriver.hinclude/graphics/driver/X11Driver.hinclude/lv_conf.hlibrary.jsonsource/graphics/DeviceGUI.cppsource/graphics/TFT/TFTView_320x240.cppsource/graphics/driver/DisplayDriverFactory.cppsource/graphics/driver/SDLDriver.cppsource/graphics/driver/X11Driver.cpp
💤 Files with no reviewable changes (1)
- source/graphics/DeviceGUI.cpp
| /** | ||
| * @brief For simulation on pc/raspberry | ||
| * This class provides an SDL GUI on the local desktop; dimensions are defined | ||
| * in lv_drv_conf.h Usage: define USE_SDL=1 for the rasbian/portduino target and | ||
| * link with -lSDL | ||
| */ |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -- '-lSDL(2)?\b|sdl2-config|pkg-config.*sdl2|SDL2::SDL2' .Repository: meshtastic/device-ui
Length of output: 217
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SDLDriver.h =="
cat -n include/graphics/driver/SDLDriver.h
echo
echo "== SDL-related files =="
git ls-files | rg '(^|/)(CMakeLists\.txt|.*\.cmake|.*\.(h|hpp|c|cc|cpp))$' | rg 'SDL|sdl|graphics/driver'
echo
echo "== SDL includes/usages =="
rg -n --glob '!**/build/**' --glob '!**/dist/**' --glob '!**/node_modules/**' 'SDL2?|sdl2-config|pkg-config.*sdl2|SDL_' include src .github CMakeLists.txt *.cmake
echo
echo "== Build/link config mentions =="
rg -n --glob '!**/build/**' --glob '!**/dist/**' --glob '!**/node_modules/**' '-lSDL(2)?\b|SDL2::SDL2|PkgConfig::.*SDL2|find_package\(SDL2|find_package\(SDL' .Repository: meshtastic/device-ui
Length of output: 3592
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== SDL driver implementation =="
cat -n source/graphics/driver/SDLDriver.cpp
echo
echo "== SDL2 and linker config mentions =="
rg -n --glob '!**/build/**' --glob '!**/dist/**' --glob '!**/node_modules/**' \
'SDL2|sdl2-config|pkg-config.*sdl2|find_package\(SDL|find_package\(SDL2|SDL2::SDL2|PkgConfig::.*SDL2|SDL_INCLUDE|SDL_LIB|USE_SDL|LV_USE_SDL' \
include source .
echo
echo "== Build files =="
git ls-files | rg '(^|/)(CMakeLists\.txt|.*\.cmake|Makefile|GNUmakefile|Kconfig|platformio\.ini)$'Repository: meshtastic/device-ui
Length of output: 4303
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== CMakeLists.txt =="
cat -n CMakeLists.txt
echo
echo "== cmake/Portduino.cmake =="
cat -n cmake/Portduino.cmake
echo
echo "== cmake/LVGL.cmake =="
cat -n cmake/LVGL.cmake
echo
echo "== SDL build references =="
rg -n --glob '!**/build/**' --glob '!**/dist/**' --glob '!**/node_modules/**' \
'SDL2|SDL|sdl2-config|pkg-config|target_link_libraries|find_package' CMakeLists.txt cmake include sourceRepository: meshtastic/device-ui
Length of output: 10711
Document the SDL2 linker flag
This code uses SDL2 (<SDL2/SDL.h> and LV_SDL_INCLUDE_PATH), so -lSDL is misleading. Update the note to point to SDL2 linking (-lSDL2 or the project’s SDL2 build config).
🤖 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 `@include/graphics/driver/SDLDriver.h` around lines 4 - 9, Update the
documentation comment above the SDL driver class to reference SDL2 linking,
replacing the misleading -lSDL flag with -lSDL2 or the project’s established
SDL2 build configuration. Keep the existing usage and platform guidance
unchanged.
| /** | ||
| * Measure how long it takes to call DisplayDriver::task_handler(). | ||
| * Then tell the lvgl library how long it took via lv_tick_inc(). | ||
| */ | ||
| void X11Driver::task_handler(void) | ||
| { | ||
| const int ms = 10; | ||
| auto start = std::chrono::high_resolution_clock::now(); | ||
| DisplayDriver::task_handler(); | ||
| auto stop = std::chrono::high_resolution_clock::now(); | ||
| auto duration = std::chrono::duration_cast<std::chrono::milliseconds>(stop - start); | ||
| if (duration.count() < ms) { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(ms - duration.count())); | ||
| lv_tick_inc(ms); | ||
| } else { | ||
| std::this_thread::sleep_for(std::chrono::milliseconds(1)); | ||
| lv_tick_inc(duration.count() + 1); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
curl -fsSL \
https://raw.githubusercontent.com/lvgl/lvgl/v9.5.0/src/drivers/x11/lv_x11_display.c |
rg -n -C3 'x11_tick_thread|lv_tick_inc'Repository: meshtastic/device-ui
Length of output: 605
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the driver and its LVGL/X11 integration points
git ls-files 'source/graphics/driver/*' 'source/graphics/*' 'src/*' 'include/*' | sed -n '1,200p'
echo '--- X11Driver.cpp ---'
cat -n source/graphics/driver/X11Driver.cpp | sed -n '1,220p'
echo '--- search for X11 backend / lv_tick_inc / lv_timer_handler usage ---'
rg -n 'lv_tick_inc|lv_timer_handler|x11|LV_USE_X11|lv_x11' source include srcRepository: meshtastic/device-ui
Length of output: 9173
Remove the extra X11 tick updates
LVGL’s X11 backend already advances time on its own 5 ms thread, so these lv_tick_inc() calls double-count time and make timers/animations run too fast. Keep the sleep, but remove both increments.
🤖 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 `@source/graphics/driver/X11Driver.cpp` around lines 32 - 49, Update
X11Driver::task_handler to remove both lv_tick_inc() calls while preserving the
existing duration-based sleep behavior in both branches.
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
|
Caution Review failedAn error occurred during the review process. Please try again later. 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 |
|
lvgl 9.5.0 crashes in lv_draw_sdl.c (see lvgl/lvgl@master...mverch67:lvgl:fixes-9.5.0) |
2 participants
requires lvgl 9.4.0
Summary by CodeRabbit