From e61023e087044423a3f787324ad2fb6aa587a0f1 Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Sun, 1 Feb 2026 21:47:58 +0100 Subject: [PATCH] PR feedback fixes --- Devices/simulator/Source/hal/SdlDisplay.h | 4 +- .../Include/tactility/lvgl_module.h | 2 +- Modules/lvgl-module/Source/arch/lvgl_esp32.c | 3 +- Modules/lvgl-module/Source/arch/lvgl_posix.c | 58 ++++++++++++------- Platforms/PlatformPosix/Source/freertos.c | 7 +++ Tactility/Source/Tactility.cpp | 2 +- Tactility/Source/app/App.cpp | 2 +- Tactility/Source/service/gui/GuiService.cpp | 17 +++--- 8 files changed, 59 insertions(+), 36 deletions(-) diff --git a/Devices/simulator/Source/hal/SdlDisplay.h b/Devices/simulator/Source/hal/SdlDisplay.h index c380d469..99f8aa65 100644 --- a/Devices/simulator/Source/hal/SdlDisplay.h +++ b/Devices/simulator/Source/hal/SdlDisplay.h @@ -6,7 +6,7 @@ class SdlDisplay final : public tt::hal::display::DisplayDevice { - lv_disp_t* displayHandle; + lv_disp_t* displayHandle = nullptr; public: @@ -20,12 +20,14 @@ public: bool supportsLvgl() const override { return true; } bool startLvgl() override { + if (displayHandle) return true; // already started displayHandle = lv_sdl_window_create(320, 240); lv_sdl_window_set_title(displayHandle, "Tactility"); return displayHandle != nullptr; } bool stopLvgl() override { + if (!displayHandle) return true; lv_display_delete(displayHandle); displayHandle = nullptr; return true; diff --git a/Modules/lvgl-module/Include/tactility/lvgl_module.h b/Modules/lvgl-module/Include/tactility/lvgl_module.h index bc818b68..e18e3a53 100644 --- a/Modules/lvgl-module/Include/tactility/lvgl_module.h +++ b/Modules/lvgl-module/Include/tactility/lvgl_module.h @@ -69,7 +69,7 @@ bool lvgl_lock(void); /** * @brief Tries to lock the LVGL mutex with a timeout. * - * @param timeout Timeout in milliseconds. + * @param timeout Timeout in ticks * @return true if the lock was acquired, false otherwise. */ bool lvgl_try_lock_timed(uint32_t timeout); diff --git a/Modules/lvgl-module/Source/arch/lvgl_esp32.c b/Modules/lvgl-module/Source/arch/lvgl_esp32.c index 09b667cf..9c26a6f6 100644 --- a/Modules/lvgl-module/Source/arch/lvgl_esp32.c +++ b/Modules/lvgl-module/Source/arch/lvgl_esp32.c @@ -3,6 +3,7 @@ #include +#include #include #include @@ -17,7 +18,7 @@ bool lvgl_lock(void) { bool lvgl_try_lock_timed(uint32_t timeout) { if (!initialized) return false; - return lvgl_port_lock(timeout); + return lvgl_port_lock(millis_to_ticks(timeout)); } void lvgl_unlock(void) { diff --git a/Modules/lvgl-module/Source/arch/lvgl_posix.c b/Modules/lvgl-module/Source/arch/lvgl_posix.c index 9ce8dbc2..71e4122d 100644 --- a/Modules/lvgl-module/Source/arch/lvgl_posix.c +++ b/Modules/lvgl-module/Source/arch/lvgl_posix.c @@ -5,8 +5,8 @@ #include #include #include - #include +#include #include @@ -28,42 +28,34 @@ static bool lvgl_task_interrupt_requested = false; #define LVGL_STOP_TIMEOUT 5000 static void task_lock(void) { - if (!task_mutex_initialised) { - recursive_mutex_construct(&task_mutex); - task_mutex_initialised = true; - } + if (!task_mutex_initialised) return; recursive_mutex_lock(&task_mutex); } static void task_unlock(void) { + if (!task_mutex_initialised) return; recursive_mutex_unlock(&task_mutex); } bool lvgl_lock(void) { - if (!lvgl_mutex_initialised) { - recursive_mutex_construct(&lvgl_mutex); - lvgl_mutex_initialised = true; - } + if (!lvgl_mutex_initialised) return false; recursive_mutex_lock(&lvgl_mutex); return true; } bool lvgl_try_lock_timed(uint32_t timeout) { - if (!lvgl_mutex_initialised) { - recursive_mutex_construct(&lvgl_mutex); - lvgl_mutex_initialised = true; - } - - return recursive_mutex_try_lock_timed(&lvgl_mutex, timeout); + if (!lvgl_mutex_initialised) return false; + return recursive_mutex_try_lock_timed(&lvgl_mutex, millis_to_ticks(timeout)); } void lvgl_unlock(void) { + if (!lvgl_mutex_initialised) return; recursive_mutex_unlock(&lvgl_mutex); } -static void lvgl_task_interrupt() { +static void lvgl_task_set_interrupted(bool interrupted) { task_lock(); - lvgl_task_interrupt_requested = false; // interrupt task with boolean as flag + lvgl_task_interrupt_requested = interrupted; task_unlock(); } @@ -79,7 +71,7 @@ static void lvgl_task(void* arg) { check(!lvgl_task_is_interrupt_requested()); - // on_start must be called from the task, otherwhise the display doesn't work + // on_start must be called from the task, otherwise the display doesn't work if (lvgl_module_config.on_start) lvgl_module_config.on_start(); while (!lvgl_task_is_interrupt_requested()) { @@ -97,10 +89,26 @@ static void lvgl_task(void* arg) { if (lvgl_module_config.on_stop) lvgl_module_config.on_stop(); + task_lock(); + lvgl_task_handle = nullptr; + task_unlock(); + vTaskDelete(NULL); } error_t lvgl_arch_start() { + if (!lvgl_mutex_initialised) { + recursive_mutex_construct(&lvgl_mutex); + lvgl_mutex_initialised = true; + } + + if (!task_mutex_initialised) { + recursive_mutex_construct(&task_mutex); + task_mutex_initialised = true; + } + + lvgl_task_set_interrupted(false); + lv_init(); // Create the main app loop, like ESP-IDF @@ -108,9 +116,9 @@ error_t lvgl_arch_start() { lvgl_task, "lvgl", lvgl_module_config.task_stack_size, - &lvgl_task_handle, + NULL, lvgl_module_config.task_priority, - NULL + &lvgl_task_handle ); return (task_result == pdTRUE) ? ERROR_NONE : ERROR_RESOURCE; @@ -118,9 +126,15 @@ error_t lvgl_arch_start() { error_t lvgl_arch_stop() { TickType_t start_ticks = get_ticks(); - lvgl_task_interrupt(); - while (lvgl_task_handle != NULL) { // TODO: make thread-safe + lvgl_task_set_interrupted(true); + while (true) { + task_lock(); + bool done = (lvgl_task_handle == nullptr); + task_unlock(); + if (done) break; + delay_ticks(LVGL_STOP_POLL_INTERVAL); + if (get_ticks() - start_ticks > LVGL_STOP_TIMEOUT) { return ERROR_TIMEOUT; } diff --git a/Platforms/PlatformPosix/Source/freertos.c b/Platforms/PlatformPosix/Source/freertos.c index 8a6775c7..b99b1c7b 100644 --- a/Platforms/PlatformPosix/Source/freertos.c +++ b/Platforms/PlatformPosix/Source/freertos.c @@ -1,4 +1,7 @@ +#include +#if defined(__GLIBC__) #include +#endif #include #include @@ -9,5 +12,9 @@ */ void vAssertCalled(unsigned long line, const char* const file) { LOG_E("Assert triggered at {}:{}", file, line); +#if defined(__GLIBC__) __assert_fail("assert failed", file, line, ""); +#else + abort(); +#endif } diff --git a/Tactility/Source/Tactility.cpp b/Tactility/Source/Tactility.cpp index 3cc3ef99..1445fb91 100644 --- a/Tactility/Source/Tactility.cpp +++ b/Tactility/Source/Tactility.cpp @@ -371,7 +371,7 @@ void run(const Configuration& config, Module* platformModule, Module* deviceModu .task_affinity = getCpuAffinityConfiguration().graphics #endif }); - module_set_parent(&lvgl_module, &tactility_module_parent); + check(module_set_parent(&lvgl_module, &tactility_module_parent) == ERROR_NONE); lvgl::start(); registerAndStartSecondaryServices(); diff --git a/Tactility/Source/app/App.cpp b/Tactility/Source/app/App.cpp index 5bb5839e..598cbe56 100644 --- a/Tactility/Source/app/App.cpp +++ b/Tactility/Source/app/App.cpp @@ -43,7 +43,7 @@ std::shared_ptr getCurrentAppContext() { std::shared_ptr getCurrentApp() { const auto app_context = getCurrentAppContext(); - return (app_context != nullptr) ? app_context->getApp() : nullptr; + return (app_context != nullptr) ? app_context->getApp() : nullptr; } } diff --git a/Tactility/Source/service/gui/GuiService.cpp b/Tactility/Source/service/gui/GuiService.cpp index c4f10d52..24dfce79 100644 --- a/Tactility/Source/service/gui/GuiService.cpp +++ b/Tactility/Source/service/gui/GuiService.cpp @@ -59,10 +59,6 @@ int32_t GuiService::guiMain() { lv_obj_set_style_border_width(screen_root, 0, LV_STATE_DEFAULT); lv_obj_set_style_pad_all(screen_root, 0, LV_STATE_DEFAULT); - service->keyboardGroup = lv_group_create(); - lv_obj_set_style_border_width(screen_root, 0, LV_STATE_DEFAULT); - lv_obj_set_style_pad_all(screen_root, 0, LV_STATE_DEFAULT); - lv_obj_t* vertical_container = lv_obj_create(screen_root); lv_obj_set_size(vertical_container, LV_PCT(100), LV_PCT(100)); lv_obj_set_flex_flow(vertical_container, LV_FLEX_FLOW_COLUMN); @@ -214,12 +210,15 @@ void GuiService::onStop(ServiceContext& service) { unlock(); thread->join(); - lvgl::lock(); - if (keyboardGroup != nullptr) { - lv_group_delete(keyboardGroup); - keyboardGroup = nullptr; + if (lvgl::lock()) { + if (keyboardGroup != nullptr) { + lv_group_delete(keyboardGroup); + keyboardGroup = nullptr; + } + lvgl::unlock(); + } else { + LOGGER.error("Failed to unlock LVGL during GUI stop"); } - lvgl::unlock(); delete thread; }