From 020aa471e2690196fe60c57090ebb38bff0ce8be Mon Sep 17 00:00:00 2001 From: Ken Van Hoeylandt Date: Fri, 28 Aug 2026 01:06:53 +0200 Subject: [PATCH] File locking refactored (#631) Remove FileMutex and wire locking into driver subsystems. --- .claude/rules/hardware-abstraction-layer.md | 7 + Documentation/ideas.md | 2 - .../esp-epaper-module/source/esp_epaper.cpp | 14 ++ Drivers/gc9a01-module/source/gc9a01.cpp | 57 +++++- .../gdeq031t10-module/source/gdeq031t10.cpp | 37 +++- Drivers/hx8357-module/source/hx8357.cpp | 16 ++ Drivers/ili9341-module/source/ili9341.cpp | 65 ++++-- Drivers/ili9488-module/source/ili9488.cpp | 59 +++++- Drivers/jd9853-module/source/jd9853.cpp | 57 +++++- Drivers/st7735-module/source/st7735.cpp | 59 +++++- Drivers/st7789-module/source/st7789.cpp | 59 +++++- Drivers/st7796-module/source/st7796.cpp | 59 +++++- Drivers/xpt2046-module/source/xpt2046.cpp | 32 ++- .../source/app_esp32_loader_service.cpp | 9 - Modules/app-module/private/app/private/fs.h | 57 +----- Modules/app-module/source/app_install.cpp | 92 +++++++-- .../source/app_metadata_parsing.cpp | 8 - Modules/gps-module/source/gps_settings.cpp | 29 --- .../include/tactility/drivers/esp32_sdcard.h | 22 ++ .../source/drivers/esp32_sdcard.cpp | 77 +++++++ .../source/drivers/esp32_sdspi.cpp | 19 ++ Tactility/Include/Tactility/file/File.h | 31 --- Tactility/Include/Tactility/file/ObjectFile.h | 10 +- Tactility/Source/Tactility.cpp | 37 +--- Tactility/Source/app/apphub/AppHubEntry.cpp | 2 - .../app/crashdiagnostics/CrashDiagnostics.cpp | 1 - Tactility/Source/app/files/View.cpp | 135 ++++--------- Tactility/Source/app/notes/Notes.cpp | 14 +- Tactility/Source/app/setup/Setup.cpp | 2 - Tactility/Source/file/File.cpp | 12 -- Tactility/Source/file/FileMutexLvgl.cpp | 135 ------------- Tactility/Source/lvgl/LabelUtils.cpp | 7 +- Tactility/Source/network/Http.cpp | 3 +- Tactility/Source/network/HttpdReq.cpp | 17 +- .../Source/service/webserver/AssetVersion.cpp | 78 ++++--- .../service/webserver/WebServerService.cpp | 7 - .../tactility/drivers/spi_controller.h | 13 ++ .../include/tactility/filesystem/file_mutex.h | 63 ------ .../include/tactility/properties_file.h | 2 - .../source/drivers/spi_controller.cpp | 16 ++ .../source/filesystem/file_mutex.cpp | 123 ------------ TactilityKernel/source/properties_file.cpp | 15 -- TactilityKernel/source/symbols.c | 10 +- .../tests/source/file_mutex_test.cpp | 190 ------------------ 44 files changed, 766 insertions(+), 993 deletions(-) delete mode 100644 Tactility/Source/file/FileMutexLvgl.cpp delete mode 100644 TactilityKernel/include/tactility/filesystem/file_mutex.h delete mode 100644 TactilityKernel/source/filesystem/file_mutex.cpp delete mode 100644 TactilityKernel/tests/source/file_mutex_test.cpp diff --git a/.claude/rules/hardware-abstraction-layer.md b/.claude/rules/hardware-abstraction-layer.md index 6c50f168..bff3d4a2 100644 --- a/.claude/rules/hardware-abstraction-layer.md +++ b/.claude/rules/hardware-abstraction-layer.md @@ -10,6 +10,13 @@ A driver generally consists of: Drivers are part of a kernel module. +A driver whose device sits on a shared SPI controller (parent device type `SPI_CONTROLLER_TYPE`) +must bracket its own bus access with `spi_controller_lock()`/`spi_controller_unlock()` +(`spi_controller_lock_bus_of()`/`spi_controller_unlock_bus_of()` for the common case of a direct +child), at the logical-operation level rather than per primitive. This is not done for you by the +kernel's generic device-type wrappers (`display.cpp`, `pointer.cpp`, etc.) - only the driver knows +for certain which of its calls touch the bus. + Modules with drivers can be stored in: - TactilityKernel - A subproject in `Platforms` folder diff --git a/Documentation/ideas.md b/Documentation/ideas.md index 21d35ee4..4100a9cc 100644 --- a/Documentation/ideas.md +++ b/Documentation/ideas.md @@ -12,7 +12,6 @@ ## Higher Priority - CrashDiagnostics shouldn't show a QR when there's no callstack -- lvgl file lock won't work with display vs sdcard when lvgl is stopped (external app bug risk) - Apps should be able to specify stack size in their manifest, per architecture. Use thread_get_stack_space() to find out the unused bytes - Apps currently have a `Context` object with an `appInstanceId` in it, purely for being able to close the app. @@ -24,7 +23,6 @@ - Get rid of WiFi service (Wifi.cpp/h) in Tactility.cpp - Make it more clear to end-users that an SD card is required to run Tactility - Make it possible to override stack size for an app via config file (loaded at boot), and make it possible to set preferred memory location (e.g. internal/external) -- Wrap file operations like fopen/fclose with file_mutex - Add bold fonts for e-ink readability improvement - Httpd.cpp: warn if running on same CPU core (or task) as UI/LVGL/window manager. - Improve Setup: Show "Step done" screen diff --git a/Drivers/esp-epaper-module/source/esp_epaper.cpp b/Drivers/esp-epaper-module/source/esp_epaper.cpp index 8bff9eea..0fc5506c 100644 --- a/Drivers/esp-epaper-module/source/esp_epaper.cpp +++ b/Drivers/esp-epaper-module/source/esp_epaper.cpp @@ -27,6 +27,7 @@ constexpr auto* TAG = "esp_epaper"; constexpr uint16_t MAX_PANEL_DIMENSION = 2048; struct EspEpaperInternal { + Device* spi_controller; /** Opaque esp_epaper device, owns the panel's pins and SPI device. */ epd_handle_t epd; epd_panel_info_t info; @@ -77,7 +78,9 @@ static error_t esp_epaper_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); xSemaphoreTake(internal->panel_mutex, portMAX_DELAY); // epd_wake() toggles the reset pin and re-runs the full init sequence. + spi_controller_lock(internal->spi_controller); const esp_err_t ret = epd_wake(internal->epd); + spi_controller_unlock(internal->spi_controller); // epd_wake() re-inits the panel, so it is awake (and drawable) again. if (ret == ESP_OK) { internal->display_on = true; @@ -89,7 +92,9 @@ static error_t esp_epaper_reset(Device* device) { static error_t esp_epaper_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); xSemaphoreTake(internal->panel_mutex, portMAX_DELAY); + spi_controller_lock(internal->spi_controller); const esp_err_t ret = epd_wake(internal->epd); + spi_controller_unlock(internal->spi_controller); if (ret == ESP_OK) { internal->display_on = true; } @@ -133,7 +138,9 @@ static error_t esp_epaper_draw_bitmap(Device* device, int32_t x_start, int32_t y source = internal->rotate_buffer; } + spi_controller_lock(internal->spi_controller); const esp_err_t ret = epd_update(internal->epd, source, config->update_mode); + spi_controller_unlock(internal->spi_controller); xSemaphoreGive(internal->panel_mutex); if (ret != ESP_OK) { LOG_E(TAG, "epd_update failed: %s", esp_err_to_name(ret)); @@ -152,6 +159,7 @@ static error_t esp_epaper_disp_on_off(Device* device, bool on_off) { } bool ok = true; + spi_controller_lock(internal->spi_controller); if (on_off) { if (epd_wake(internal->epd) != ESP_OK) { LOG_E(TAG, "epd_wake failed"); @@ -163,6 +171,7 @@ static error_t esp_epaper_disp_on_off(Device* device, bool on_off) { ok = false; } } + spi_controller_unlock(internal->spi_controller); if (ok) { internal->display_on = on_off; @@ -220,7 +229,9 @@ static const DisplayApi esp_epaper_display_api = { static void free_internal(EspEpaperInternal* internal) { if (internal->epd != nullptr) { + spi_controller_lock(internal->spi_controller); epd_deinit(internal->epd); + spi_controller_unlock(internal->spi_controller); } if (internal->rotate_buffer != nullptr) { free(internal->rotate_buffer); @@ -284,6 +295,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->epd = epd; internal->panel_mutex = xSemaphoreCreateMutex(); if (internal->panel_mutex == nullptr) { @@ -331,7 +343,9 @@ static error_t stop(Device* device) { xSemaphoreTake(internal->panel_mutex, portMAX_DELAY); if (internal->display_on) { // Leave the panel in deep sleep. + spi_controller_lock(internal->spi_controller); epd_sleep(internal->epd); + spi_controller_unlock(internal->spi_controller); internal->display_on = false; } xSemaphoreGive(internal->panel_mutex); diff --git a/Drivers/gc9a01-module/source/gc9a01.cpp b/Drivers/gc9a01-module/source/gc9a01.cpp index bd1b1632..aa4fbbda 100644 --- a/Drivers/gc9a01-module/source/gc9a01.cpp +++ b/Drivers/gc9a01-module/source/gc9a01.cpp @@ -25,6 +25,7 @@ constexpr auto* TAG = "GC9A01"; #define GET_CONFIG(device) (static_cast((device)->config)) struct Gc9a01Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // See st7796-module's identical field for why this exists: draw_bitmap() must block until @@ -63,6 +64,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -123,6 +125,7 @@ static error_t start(Device* device) { // Bring-up sequence, order matches the deprecated HAL's Gc9a01Display (proven correct on real // Waveshare S3 Touch LCD 1.28 hardware). Every failure path below must clean up fully: unlike // stop_device, this is never retried by the kernel if start_device fails. + spi_controller_lock(internal->spi_controller); bool ok = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK; @@ -134,6 +137,7 @@ static error_t start(Device* device) { ok = ok && ((!config->mirror_x && !config->mirror_y) || esp_lcd_panel_mirror(internal->panel_handle, config->mirror_x, config->mirror_y) == ESP_OK); ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -151,9 +155,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -162,10 +168,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); vSemaphoreDelete(internal->draw_done_semaphore); free(internal); @@ -179,12 +187,18 @@ static error_t stop(Device* device) { static error_t gc9a01_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t gc9a01_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t gc9a01_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -192,22 +206,35 @@ static error_t gc9a01_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } + // Hold the bus lock across the wait too, not just the queueing call: the command/data phases + // aren't wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could + // otherwise interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t gc9a01_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t gc9a01_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static bool gc9a01_get_swap_xy(Device* device) { @@ -224,7 +251,10 @@ static bool gc9a01_get_mirror_y(Device* device) { static error_t gc9a01_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static int32_t gc9a01_get_gap_x(Device* device) { @@ -237,17 +267,26 @@ static int32_t gc9a01_get_gap_y(Device* device) { static error_t gc9a01_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t gc9a01_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t gc9a01_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // The deprecated HAL's Gc9a01Display always set swap_bytes=true on its lvgl_port config diff --git a/Drivers/gdeq031t10-module/source/gdeq031t10.cpp b/Drivers/gdeq031t10-module/source/gdeq031t10.cpp index 3fd5309d..e7d90246 100644 --- a/Drivers/gdeq031t10-module/source/gdeq031t10.cpp +++ b/Drivers/gdeq031t10-module/source/gdeq031t10.cpp @@ -51,6 +51,7 @@ static constexpr uint8_t DEEP_SLEEP_CHECK_CODE = 0xA5; extern "C" { struct Gdeq031t10Internal { + Device* spi_controller; spi_device_handle_t spi_device; struct GpioDescriptor* dc; struct GpioDescriptor* reset; // optional @@ -123,12 +124,14 @@ static bool wait_while_busy(Gdeq031t10Internal* internal) { } static void hardware_reset(Gdeq031t10Internal* internal) { + spi_controller_lock(internal->spi_controller); if (internal->reset != nullptr) { gpio_descriptor_set_level(internal->reset, false); delay_millis(10); gpio_descriptor_set_level(internal->reset, true); delay_millis(10); } + spi_controller_unlock(internal->spi_controller); } static bool init_full(Gdeq031t10Internal* internal, bool mirror_180) { @@ -137,10 +140,12 @@ static bool init_full(Gdeq031t10Internal* internal, bool mirror_180) { // mode change needs partial mode's lingering VCOM/data-interval setting cleared, and // waking from deep sleep is only possible by toggling RST. hardware_reset(internal); + spi_controller_lock(internal->spi_controller); bool ok = write_command(internal, CMD_PANEL_SETTING); ok = ok && write_data_byte(internal, mirror_180 ? 0x13 : 0x1F); ok = ok && write_command(internal, CMD_POWER_ON); ok = ok && wait_while_busy(internal); + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Full init failed"); return false; @@ -164,6 +169,7 @@ static bool init_with_fast_lut(Gdeq031t10Internal* internal, bool mirror_180, en case GDEQ031T10_REFRESH_PARTIAL: timing = 0x79; break; default: return true; // GDEQ031T10_REFRESH_FULL: init_full() already did everything } + spi_controller_lock(internal->spi_controller); bool ok = write_command(internal, CMD_FAST_MODE_ENABLE); ok = ok && write_data_byte(internal, 0x02); ok = ok && write_command(internal, CMD_FAST_MODE_TIMING); @@ -172,6 +178,7 @@ static bool init_with_fast_lut(Gdeq031t10Internal* internal, bool mirror_180, en ok = write_command(internal, CMD_VCOM_DATA_INTERVAL); ok = ok && write_data_byte(internal, 0xD7); } + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Mode init failed"); return false; @@ -185,7 +192,10 @@ static bool ensure_panel_ready(Gdeq031t10Internal* internal, bool mirror_180, en return init_with_fast_lut(internal, mirror_180, mode); } else if (!internal->panel_power_on) { // Registers still hold the mode; only the charge pump was idled. - if (!write_command(internal, CMD_POWER_ON) || !wait_while_busy(internal)) { + spi_controller_lock(internal->spi_controller); + bool ok = write_command(internal, CMD_POWER_ON) && wait_while_busy(internal); + spi_controller_unlock(internal->spi_controller); + if (!ok) { LOG_E(TAG, "Panel did not become ready after power-on"); return false; } @@ -198,11 +208,13 @@ static bool panel_power_off(Gdeq031t10Internal* internal) { // Command the panel off regardless of whether BUSY confirms it: retrying // forever here would just as likely hang, and a stuck-BUSY panel is // already unusable either way. + spi_controller_lock(internal->spi_controller); bool ok = write_command(internal, CMD_POWER_ON_OFF); // 0x02 standalone = power off if (!ok || !wait_while_busy(internal)) { LOG_E(TAG, "Panel did not confirm power-off"); ok = false; } + spi_controller_unlock(internal->spi_controller); internal->panel_power_on = false; return ok; } @@ -222,12 +234,18 @@ static void refresh_full(Gdeq031t10Internal* internal, bool mirror_180, enum Gde return; } + // Single logical bus operation spanning old/new frame data and the refresh trigger: an SD + // transaction slipping in between any of these writes would corrupt the sequence the panel + // is mid-way through parsing. + spi_controller_lock(internal->spi_controller); + // shadow_framebuffer holds the panel's actual current content (matches the vendor's tracked // oldData[] buffer) - send it as "old" data so the controller computes correct per-pixel // transitions. LOG_I(TAG, "write old data from shadow_buffer"); if (!write_command(internal, CMD_DATA_START_OLD) || !write_data(internal, internal->shadow_framebuffer, FRAMEBUFFER_SIZE)) { LOG_E(TAG, "Failed to send old frame data"); + spi_controller_unlock(internal->spi_controller); return; } @@ -235,11 +253,13 @@ static void refresh_full(Gdeq031t10Internal* internal, bool mirror_180, enum Gde // is showing right now until this refresh is confirmed below. if (!write_command(internal, CMD_DATA_START_NEW) || !write_data(internal, render_bitmap, FRAMEBUFFER_SIZE)) { LOG_E(TAG, "Failed to send new frame data"); + spi_controller_unlock(internal->spi_controller); return; } if (!write_command(internal, CMD_DISPLAY_REFRESH)) { LOG_E(TAG, "Failed to trigger display refresh"); + spi_controller_unlock(internal->spi_controller); return; } delay_millis(1); // datasheet requires >=200us settle before polling BUSY @@ -252,6 +272,7 @@ static void refresh_full(Gdeq031t10Internal* internal, bool mirror_180, enum Gde // content the panel never displayed, and hide the difference from the change scan. LOG_E(TAG, "Full refresh did not complete"); } + spi_controller_unlock(internal->spi_controller); // EPD_DeepSleep(): power off, then actually deep-sleep rather than just idling the charge // pump. panel_mode_valid=false forces the next refresh_full() call back through a fresh @@ -277,6 +298,11 @@ static void refresh_window(Gdeq031t10Internal* internal, bool mirror_180, const const uint16_t y = static_cast(first_row); const uint16_t ye = static_cast(last_row); + // Single logical bus operation spanning the window setup, old/new region data and the + // refresh trigger: an SD transaction slipping in between any of these writes would corrupt + // the sequence the panel is mid-way through parsing. + spi_controller_lock(internal->spi_controller); + // Set the partial RAM window (GxEPD2 GDEQ031T10 sequence). bool ok = write_command(internal, CMD_PARTIAL_IN); ok = ok && write_command(internal, CMD_PARTIAL_WINDOW); @@ -290,6 +316,7 @@ static void refresh_window(Gdeq031t10Internal* internal, bool mirror_180, const if (!ok) { LOG_E(TAG, "Failed to set partial refresh window"); write_command(internal, CMD_PARTIAL_OUT); // best-effort: leave partial-window mode + spi_controller_unlock(internal->spi_controller); return; } @@ -303,6 +330,7 @@ static void refresh_window(Gdeq031t10Internal* internal, bool mirror_180, const if (!write_command(internal, CMD_DATA_START_OLD) || !write_data(internal, internal->region_buffer, n)) { LOG_E(TAG, "Failed to send old window data"); write_command(internal, CMD_PARTIAL_OUT); + spi_controller_unlock(internal->spi_controller); return; } @@ -317,6 +345,7 @@ static void refresh_window(Gdeq031t10Internal* internal, bool mirror_180, const if (!write_command(internal, CMD_DATA_START_NEW) || !write_data(internal, internal->region_buffer, n)) { LOG_E(TAG, "Failed to send new window data"); write_command(internal, CMD_PARTIAL_OUT); + spi_controller_unlock(internal->spi_controller); return; } @@ -340,6 +369,7 @@ static void refresh_window(Gdeq031t10Internal* internal, bool mirror_180, const } } write_command(internal, CMD_PARTIAL_OUT); + spi_controller_unlock(internal->spi_controller); } // endregion @@ -467,8 +497,10 @@ static error_t gdeq031t10_disp_on_off(Device* device, bool on_off) { panel_power_off(internal); } delay_millis(100); + spi_controller_lock(internal->spi_controller); write_command(internal, CMD_DEEP_SLEEP); write_data_byte(internal, DEEP_SLEEP_CHECK_CODE); + spi_controller_unlock(internal->spi_controller); // Deep sleep needs a reset to wake, which restores register defaults. internal->panel_mode_valid = false; } @@ -566,6 +598,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->dc = gpio_descriptor_acquire( config->pin_dc.gpio_controller, config->pin_dc.pin, @@ -651,8 +684,10 @@ static error_t stop(Device* device) { panel_power_off(internal); } delay_millis(100); + spi_controller_lock(internal->spi_controller); write_command(internal, CMD_DEEP_SLEEP); write_data_byte(internal, DEEP_SLEEP_CHECK_CODE); + spi_controller_unlock(internal->spi_controller); internal->display_on = false; } xSemaphoreGive(internal->panel_mutex); diff --git a/Drivers/hx8357-module/source/hx8357.cpp b/Drivers/hx8357-module/source/hx8357.cpp index a89fe0a3..ff0c140e 100644 --- a/Drivers/hx8357-module/source/hx8357.cpp +++ b/Drivers/hx8357-module/source/hx8357.cpp @@ -111,6 +111,7 @@ constexpr uint8_t INIT_CMDS[] = { } // namespace struct Hx8357Internal { + Device* spi_controller; spi_device_handle_t spi_handle; gpio_num_t dc_pin; size_t max_transfer_size; @@ -210,6 +211,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->dc_pin = static_cast(pin_or_unused(config->pin_dc)); // Clamped below the bus's configured max_transfer_size: that value only bounds the DMA // buffer/descriptor allocation, not the SPI peripheral's own per-transaction bit-length @@ -275,9 +277,11 @@ static error_t start(Device* device) { internal->mirror_x = config->mirror_x; internal->mirror_y = config->mirror_y; + spi_controller_lock(internal->spi_controller); run_init_cmds(internal); send_madctl(internal); send_cmd(internal, config->invert_color ? HX8357_INVON : HX8357_INVOFF); + spi_controller_unlock(internal->spi_controller); device_set_driver_data(device, internal); return ERROR_NONE; @@ -329,6 +333,7 @@ static error_t hx8357_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta static_cast((y2 >> 8) & 0xFF), static_cast(y2 & 0xFF), }; + spi_controller_lock(internal->spi_controller); send_cmd(internal, HX8357_CASET); send_data(internal, xb, 4); send_cmd(internal, HX8357_PASET); @@ -337,6 +342,7 @@ static error_t hx8357_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta const size_t pixel_count = static_cast(x_end - x_start) * static_cast(y_end - y_start); send_data(internal, static_cast(color_data), pixel_count * 3); // RGB888 = 3 bytes/pixel + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } @@ -345,14 +351,18 @@ static error_t hx8357_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); internal->mirror_x = x_axis; internal->mirror_y = y_axis; + spi_controller_lock(internal->spi_controller); send_madctl(internal); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t hx8357_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); internal->swap_xy = swap_axes; + spi_controller_lock(internal->spi_controller); send_madctl(internal); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } @@ -373,19 +383,25 @@ static bool hx8357_get_mirror_y(Device* device) { static error_t hx8357_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); send_cmd(internal, invert_color_data ? HX8357_INVON : HX8357_INVOFF); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t hx8357_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); send_cmd(internal, on_off ? HX8357_DISPON : 0x28 /* HX8357_DISPOFF */); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t hx8357_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); send_cmd(internal, sleep ? 0x10 /* HX8357_SLPIN */ : HX8357_SLPOUT); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } diff --git a/Drivers/ili9341-module/source/ili9341.cpp b/Drivers/ili9341-module/source/ili9341.cpp index d8de08a2..3b30b133 100644 --- a/Drivers/ili9341-module/source/ili9341.cpp +++ b/Drivers/ili9341-module/source/ili9341.cpp @@ -34,6 +34,7 @@ static const uint8_t GAMMA_CURVE_VALUES[4] = { 0x01, 0x04, 0x02, 0x08 }; struct Ili9341Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // Given from ISR context by on_color_trans_done() once a queued SPI transfer physically @@ -108,6 +109,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -187,8 +189,10 @@ static error_t start(Device* device) { // Bring-up sequence, order matches EspLcdDisplayV2::applyConfiguration (proven correct on real ILI9341 panels). // Every failure path below must clean up fully: unlike stop_device, this is never retried by the kernel // if start_device fails (see device_start() in TactilityKernel), so a partial failure here would leak. - bool ok = - pulse_reset(internal->reset_descriptor) == ERROR_NONE && + bool ok = pulse_reset(internal->reset_descriptor) == ERROR_NONE; + + spi_controller_lock(internal->spi_controller); + ok = ok && esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); @@ -203,6 +207,7 @@ static error_t start(Device* device) { ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && (config->gamma_curve >= 4 || esp_lcd_panel_io_tx_param(internal->io_handle, LCD_CMD_GAMSET, &GAMMA_CURVE_VALUES[config->gamma_curve], 1) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -223,9 +228,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -234,10 +241,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); if (internal->reset_descriptor != nullptr) { gpio_descriptor_release(internal->reset_descriptor); @@ -256,16 +265,23 @@ static error_t stop(Device* device) { static error_t ili9341_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); error_t error = pulse_reset(internal->reset_descriptor); if (error != ERROR_NONE) { + spi_controller_unlock(internal->spi_controller); return error; } - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9341_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9341_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -276,26 +292,39 @@ static error_t ili9341_draw_bitmap(Device* device, int32_t x_start, int32_t y_st // satisfied by this draw's own transfer completing. xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } // Block until the SPI transfer physically completes: DisplayApi's draw_bitmap is a synchronous // contract (see lvgl_display.c), so the caller must be able to safely reuse/overwrite // color_data as soon as this call returns. esp_lcd_panel_draw_bitmap() only queues the - // transfer and returns once it's handed to the SPI peripheral, not once it's finished. + // transfer and returns once it's handed to the SPI peripheral, not once it's finished. Hold the + // bus lock across the wait too, not just the queueing call: the command/data phases aren't + // wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could otherwise + // interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t ili9341_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9341_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state: swap_xy()/mirror() calls made after @@ -314,7 +343,10 @@ static bool ili9341_get_mirror_y(Device* device) { static error_t ili9341_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state - see DisplayApi::get_gap_x(). @@ -334,17 +366,26 @@ static int32_t ili9341_get_gap_y(Device* device) { static error_t ili9341_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9341_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9341_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // bgr_order only selects the panel controller's rgb_ele_order (applied in start(), below) so the diff --git a/Drivers/ili9488-module/source/ili9488.cpp b/Drivers/ili9488-module/source/ili9488.cpp index a23731ac..4d1a707e 100644 --- a/Drivers/ili9488-module/source/ili9488.cpp +++ b/Drivers/ili9488-module/source/ili9488.cpp @@ -26,6 +26,7 @@ #define GET_CONFIG(device) (static_cast((device)->config)) struct Ili9488Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // Given from ISR context by on_color_trans_done() once a queued SPI transfer physically @@ -68,6 +69,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -131,6 +133,7 @@ static error_t start(Device* device) { // Bring-up sequence, order matches EspLcdDisplayV2::applyConfiguration (proven correct on real ILI9488 panels). // Every failure path below must clean up fully: unlike stop_device, this is never retried by the kernel // if start_device fails (see device_start() in TactilityKernel), so a partial failure here would leak. + spi_controller_lock(internal->spi_controller); bool ok = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK && @@ -145,6 +148,7 @@ static error_t start(Device* device) { ok = ok && ((!config->mirror_x && !config->mirror_y) || esp_lcd_panel_mirror(internal->panel_handle, config->mirror_x, config->mirror_y) == ESP_OK); ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -162,9 +166,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -173,10 +179,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); vSemaphoreDelete(internal->draw_done_semaphore); free(internal); @@ -190,12 +198,18 @@ static error_t stop(Device* device) { static error_t ili9488_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9488_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9488_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -206,26 +220,39 @@ static error_t ili9488_draw_bitmap(Device* device, int32_t x_start, int32_t y_st // satisfied by this draw's own transfer completing. xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } // Block until the SPI transfer physically completes: DisplayApi's draw_bitmap is a synchronous // contract (see lvgl_display.c), so the caller must be able to safely reuse/overwrite // color_data as soon as this call returns. esp_lcd_panel_draw_bitmap() only queues the - // transfer and returns once it's handed to the SPI peripheral, not once it's finished. + // transfer and returns once it's handed to the SPI peripheral, not once it's finished. Hold the + // bus lock across the wait too, not just the queueing call: the command/data phases aren't + // wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could otherwise + // interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t ili9488_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9488_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state: swap_xy()/mirror() calls made after @@ -244,7 +271,10 @@ static bool ili9488_get_mirror_y(Device* device) { static error_t ili9488_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state - see DisplayApi::get_gap_x(). @@ -264,17 +294,26 @@ static int32_t ili9488_get_gap_y(Device* device) { static error_t ili9488_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9488_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t ili9488_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // bgr_order only selects the panel controller's rgb_ele_order (applied in start(), above) so the diff --git a/Drivers/jd9853-module/source/jd9853.cpp b/Drivers/jd9853-module/source/jd9853.cpp index 28a364cb..a1d59d2c 100644 --- a/Drivers/jd9853-module/source/jd9853.cpp +++ b/Drivers/jd9853-module/source/jd9853.cpp @@ -31,6 +31,7 @@ constexpr auto* TAG = "JD9853"; static const uint8_t GAMMA_CURVE_VALUES[4] = { 0x01, 0x04, 0x02, 0x08 }; struct Jd9853Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // See st7796-module's identical field for why this exists: draw_bitmap() must block until @@ -69,6 +70,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -134,6 +136,7 @@ static error_t start(Device* device) { // Bring-up sequence, order matches the deprecated HAL's Jd9853Display (proven correct on real // Waveshare S3 Touch LCD 1.47 hardware). Every failure path below must clean up fully: unlike // stop_device, this is never retried by the kernel if start_device fails. + spi_controller_lock(internal->spi_controller); bool ok = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK && @@ -147,6 +150,7 @@ static error_t start(Device* device) { ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && (config->gamma_curve >= 4 || esp_lcd_panel_io_tx_param(internal->io_handle, LCD_CMD_GAMSET, &GAMMA_CURVE_VALUES[config->gamma_curve], 1) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -164,9 +168,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -175,10 +181,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); vSemaphoreDelete(internal->draw_done_semaphore); free(internal); @@ -192,12 +200,18 @@ static error_t stop(Device* device) { static error_t jd9853_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t jd9853_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t jd9853_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -205,22 +219,35 @@ static error_t jd9853_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } + // Hold the bus lock across the wait too, not just the queueing call: the command/data phases + // aren't wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could + // otherwise interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t jd9853_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t jd9853_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static bool jd9853_get_swap_xy(Device* device) { @@ -237,7 +264,10 @@ static bool jd9853_get_mirror_y(Device* device) { static error_t jd9853_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static int32_t jd9853_get_gap_x(Device* device) { @@ -250,17 +280,26 @@ static int32_t jd9853_get_gap_y(Device* device) { static error_t jd9853_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t jd9853_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t jd9853_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // The deprecated HAL's Jd9853Display always set swap_bytes=true on its lvgl_port config diff --git a/Drivers/st7735-module/source/st7735.cpp b/Drivers/st7735-module/source/st7735.cpp index 65ff21cf..bacaf6b1 100644 --- a/Drivers/st7735-module/source/st7735.cpp +++ b/Drivers/st7735-module/source/st7735.cpp @@ -32,6 +32,7 @@ static const uint8_t GAMMA_CURVE_VALUES[4] = { 0x01, 0x04, 0x02, 0x08 }; struct St7735Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // Given from ISR context by on_color_trans_done() once a queued SPI transfer physically @@ -74,6 +75,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -134,6 +136,7 @@ static error_t start(Device* device) { // Bring-up sequence, order matches st7789-module (proven correct on real panels). // Every failure path below must clean up fully: unlike stop_device, this is never retried by the kernel // if start_device fails (see device_start() in TactilityKernel), so a partial failure here would leak. + spi_controller_lock(internal->spi_controller); bool ok = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK && @@ -152,6 +155,7 @@ static error_t start(Device* device) { ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && (config->gamma_curve >= 4 || esp_lcd_panel_io_tx_param(internal->io_handle, LCD_CMD_GAMSET, &GAMMA_CURVE_VALUES[config->gamma_curve], 1) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -169,9 +173,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -180,10 +186,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); vSemaphoreDelete(internal->draw_done_semaphore); free(internal); @@ -197,12 +205,18 @@ static error_t stop(Device* device) { static error_t st7735_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7735_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7735_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -213,26 +227,39 @@ static error_t st7735_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta // satisfied by this draw's own transfer completing. xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } // Block until the SPI transfer physically completes: DisplayApi's draw_bitmap is a synchronous // contract (see lvgl_display.c), so the caller must be able to safely reuse/overwrite // color_data as soon as this call returns. esp_lcd_panel_draw_bitmap() only queues the - // transfer and returns once it's handed to the SPI peripheral, not once it's finished. + // transfer and returns once it's handed to the SPI peripheral, not once it's finished. Hold the + // bus lock across the wait too, not just the queueing call: the command/data phases aren't + // wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could otherwise + // interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t st7735_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7735_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state: swap_xy()/mirror() calls made after @@ -251,7 +278,10 @@ static bool st7735_get_mirror_y(Device* device) { static error_t st7735_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state - see DisplayApi::get_gap_x(). @@ -271,17 +301,26 @@ static int32_t st7735_get_gap_y(Device* device) { static error_t st7735_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7735_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7735_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static enum DisplayColorFormat st7735_get_color_format(Device* device) { diff --git a/Drivers/st7789-module/source/st7789.cpp b/Drivers/st7789-module/source/st7789.cpp index b7466ff2..801b5f12 100644 --- a/Drivers/st7789-module/source/st7789.cpp +++ b/Drivers/st7789-module/source/st7789.cpp @@ -32,6 +32,7 @@ static const uint8_t GAMMA_CURVE_VALUES[4] = { 0x01, 0x04, 0x02, 0x08 }; struct St7789Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // Given from ISR context by on_color_trans_done() once a queued SPI transfer physically @@ -74,6 +75,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -134,6 +136,7 @@ static error_t start(Device* device) { // Bring-up sequence, order matches EspLcdDisplayV2::applyConfiguration (proven correct on real ST7789 panels). // Every failure path below must clean up fully: unlike stop_device, this is never retried by the kernel // if start_device fails (see device_start() in TactilityKernel), so a partial failure here would leak. + spi_controller_lock(internal->spi_controller); bool ok = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK && @@ -152,6 +155,7 @@ static error_t start(Device* device) { ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && (config->gamma_curve >= 4 || esp_lcd_panel_io_tx_param(internal->io_handle, LCD_CMD_GAMSET, &GAMMA_CURVE_VALUES[config->gamma_curve], 1) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -169,9 +173,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -180,10 +186,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); vSemaphoreDelete(internal->draw_done_semaphore); free(internal); @@ -197,12 +205,18 @@ static error_t stop(Device* device) { static error_t st7789_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7789_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7789_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -213,26 +227,39 @@ static error_t st7789_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta // satisfied by this draw's own transfer completing. xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } // Block until the SPI transfer physically completes: DisplayApi's draw_bitmap is a synchronous // contract (see lvgl_display.c), so the caller must be able to safely reuse/overwrite // color_data as soon as this call returns. esp_lcd_panel_draw_bitmap() only queues the - // transfer and returns once it's handed to the SPI peripheral, not once it's finished. + // transfer and returns once it's handed to the SPI peripheral, not once it's finished. Hold the + // bus lock across the wait too, not just the queueing call: the command/data phases aren't + // wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could otherwise + // interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t st7789_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7789_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state: swap_xy()/mirror() calls made after @@ -251,7 +278,10 @@ static bool st7789_get_mirror_y(Device* device) { static error_t st7789_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state - see DisplayApi::get_gap_x(). @@ -271,17 +301,26 @@ static int32_t st7789_get_gap_y(Device* device) { static error_t st7789_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7789_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7789_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static enum DisplayColorFormat st7789_get_color_format(Device* device) { diff --git a/Drivers/st7796-module/source/st7796.cpp b/Drivers/st7796-module/source/st7796.cpp index c8ba2603..e070bfb1 100644 --- a/Drivers/st7796-module/source/st7796.cpp +++ b/Drivers/st7796-module/source/st7796.cpp @@ -32,6 +32,7 @@ static const uint8_t GAMMA_CURVE_VALUES[4] = { 0x01, 0x04, 0x02, 0x08 }; struct St7796Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_panel_handle_t panel_handle; // Given from ISR context by on_color_trans_done() once a queued SPI transfer physically @@ -74,6 +75,7 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; internal->draw_done_semaphore = xSemaphoreCreateBinary(); if (internal->draw_done_semaphore == nullptr) { free(internal); @@ -134,6 +136,7 @@ static error_t start(Device* device) { // Bring-up sequence, order matches EspLcdDisplayV2::applyConfiguration (proven correct on real ST7796 panels). // Every failure path below must clean up fully: unlike stop_device, this is never retried by the kernel // if start_device fails (see device_start() in TactilityKernel), so a partial failure here would leak. + spi_controller_lock(internal->spi_controller); bool ok = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK && esp_lcd_panel_init(internal->panel_handle) == ESP_OK && @@ -151,6 +154,7 @@ static error_t start(Device* device) { ok = ok && (!config->invert_color || esp_lcd_panel_invert_color(internal->panel_handle, true) == ESP_OK); ok = ok && (config->gamma_curve >= 4 || esp_lcd_panel_io_tx_param(internal->io_handle, LCD_CMD_GAMSET, &GAMMA_CURVE_VALUES[config->gamma_curve], 1) == ESP_OK); ok = ok && esp_lcd_panel_disp_on_off(internal->panel_handle, true) == ESP_OK; + spi_controller_unlock(internal->spi_controller); if (!ok) { LOG_E(TAG, "Failed to bring up panel"); @@ -168,9 +172,11 @@ static error_t start(Device* device) { static error_t stop(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); + spi_controller_lock(internal->spi_controller); if (internal->panel_handle != nullptr) { if (esp_lcd_panel_del(internal->panel_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->panel_handle = nullptr; @@ -179,10 +185,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); vSemaphoreDelete(internal->draw_done_semaphore); free(internal); @@ -196,12 +204,18 @@ static error_t stop(Device* device) { static error_t st7796_reset(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_reset(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7796_init(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_init(internal->panel_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7796_draw_bitmap(Device* device, int32_t x_start, int32_t y_start, int32_t x_end, int32_t y_end, const void* color_data) { @@ -212,26 +226,39 @@ static error_t st7796_draw_bitmap(Device* device, int32_t x_start, int32_t y_sta // satisfied by this draw's own transfer completing. xSemaphoreTake(internal->draw_done_semaphore, 0); - if (esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data) != ESP_OK) { + spi_controller_lock(internal->spi_controller); + esp_err_t ret = esp_lcd_panel_draw_bitmap(internal->panel_handle, x_start, y_start, x_end, y_end, color_data); + if (ret != ESP_OK) { + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } // Block until the SPI transfer physically completes: DisplayApi's draw_bitmap is a synchronous // contract (see lvgl_display.c), so the caller must be able to safely reuse/overwrite // color_data as soon as this call returns. esp_lcd_panel_draw_bitmap() only queues the - // transfer and returns once it's handed to the SPI peripheral, not once it's finished. + // transfer and returns once it's handed to the SPI peripheral, not once it's finished. Hold the + // bus lock across the wait too, not just the queueing call: the command/data phases aren't + // wrapped in their own acquire_bus by esp_lcd_panel_io_spi, so another bus user could otherwise + // interleave with this transfer while it's still in flight. xSemaphoreTake(internal->draw_done_semaphore, portMAX_DELAY); + spi_controller_unlock(internal->spi_controller); return ERROR_NONE; } static error_t st7796_mirror(Device* device, bool x_axis, bool y_axis) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_mirror(internal->panel_handle, x_axis, y_axis) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7796_swap_xy(Device* device, bool swap_axes) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_swap_xy(internal->panel_handle, swap_axes) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state: swap_xy()/mirror() calls made after @@ -250,7 +277,10 @@ static bool st7796_get_mirror_y(Device* device) { static error_t st7796_set_gap(Device* device, int32_t x_gap, int32_t y_gap) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_set_gap(internal->panel_handle, x_gap, y_gap) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // Reads the devicetree-configured baseline, not live hardware state - see DisplayApi::get_gap_x(). @@ -264,17 +294,26 @@ static int32_t st7796_get_gap_y(Device* device) { static error_t st7796_invert_color(Device* device, bool invert_color_data) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_invert_color(internal->panel_handle, invert_color_data) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7796_disp_on_off(Device* device, bool on_off) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_on_off(internal->panel_handle, on_off) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t st7796_disp_sleep(Device* device, bool sleep) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_panel_disp_sleep(internal->panel_handle, sleep) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } // bgr_order only selects the panel controller's rgb_ele_order (applied in start(), above) so the diff --git a/Drivers/xpt2046-module/source/xpt2046.cpp b/Drivers/xpt2046-module/source/xpt2046.cpp index 4bd1d958..55f770ac 100644 --- a/Drivers/xpt2046-module/source/xpt2046.cpp +++ b/Drivers/xpt2046-module/source/xpt2046.cpp @@ -28,6 +28,7 @@ #define POWER_SUPPLY_MIN_MV 3200 struct Xpt2046Internal { + Device* spi_controller; esp_lcd_panel_io_handle_t io_handle; esp_lcd_touch_handle_t touch_handle; Device* power_supply_device; @@ -74,7 +75,9 @@ static error_t ps_get_property(Device* device, PowerSupplyProperty property, Pow auto* parent_internal = static_cast(device_get_driver_data(parent)); int battery_mv; + spi_controller_lock(parent_internal->spi_controller); error_t error = read_battery_mv(parent_internal->touch_handle, &battery_mv); + spi_controller_unlock(parent_internal->spi_controller); if (error != ERROR_NONE) { return error; } @@ -181,8 +184,12 @@ static error_t start(Device* device) { return ERROR_OUT_OF_MEMORY; } + internal->spi_controller = parent; + const esp_lcd_panel_io_spi_config_t io_config = ESP_LCD_TOUCH_IO_SPI_XPT2046_CONFIG(cs_pin.pin); + spi_controller_lock(internal->spi_controller); esp_err_t ret = esp_lcd_new_panel_io_spi((esp_lcd_spi_bus_handle_t)spi_config->host, &io_config, &internal->io_handle); + spi_controller_unlock(internal->spi_controller); if (ret != ESP_OK) { LOG_E(TAG, "Failed to create panel IO: %s", esp_err_to_name(ret)); free(internal); @@ -209,10 +216,14 @@ static error_t start(Device* device) { .driver_data = nullptr, }; + spi_controller_lock(internal->spi_controller); ret = esp_lcd_touch_new_spi_xpt2046(internal->io_handle, &touch_config, &internal->touch_handle); + spi_controller_unlock(internal->spi_controller); if (ret != ESP_OK) { LOG_E(TAG, "Failed to create touch handle: %s", esp_err_to_name(ret)); + spi_controller_lock(internal->spi_controller); esp_lcd_panel_io_del(internal->io_handle); + spi_controller_unlock(internal->spi_controller); free(internal); return ERROR_RESOURCE; } @@ -224,8 +235,10 @@ static error_t start(Device* device) { error_t error = create_power_supply_child(device, internal->power_supply_device); if (error != ERROR_NONE) { LOG_E(TAG, "Failed to create power-supply device"); + spi_controller_lock(internal->spi_controller); esp_lcd_touch_del(internal->touch_handle); esp_lcd_panel_io_del(internal->io_handle); + spi_controller_unlock(internal->spi_controller); free(internal); return error; } @@ -244,9 +257,11 @@ static error_t stop(Device* device) { // esp_lcd_touch_del() only releases the touch-side resources; the panel IO handle is owned // separately and needs its own deletion. + spi_controller_lock(internal->spi_controller); if (internal->touch_handle != nullptr) { if (esp_lcd_touch_del(internal->touch_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete touch handle"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->touch_handle = nullptr; @@ -255,10 +270,12 @@ static error_t stop(Device* device) { if (internal->io_handle != nullptr) { if (esp_lcd_panel_io_del(internal->io_handle) != ESP_OK) { LOG_E(TAG, "Failed to delete panel IO handle"); + spi_controller_unlock(internal->spi_controller); return ERROR_RESOURCE; } internal->io_handle = nullptr; } + spi_controller_unlock(internal->spi_controller); free(internal); device_set_driver_data(device, nullptr); @@ -271,18 +288,27 @@ static error_t stop(Device* device) { static error_t xpt2046_enter_sleep(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_touch_enter_sleep(internal->touch_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_touch_enter_sleep(internal->touch_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t xpt2046_exit_sleep(Device* device) { auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_touch_exit_sleep(internal->touch_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_touch_exit_sleep(internal->touch_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static error_t xpt2046_read_data(Device* device, TickType_t timeout) { (void)timeout; // esp_lcd_touch_read_data() has no timeout parameter auto* internal = static_cast(device_get_driver_data(device)); - return esp_lcd_touch_read_data(internal->touch_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_lock(internal->spi_controller); + error_t result = esp_lcd_touch_read_data(internal->touch_handle) == ESP_OK ? ERROR_NONE : ERROR_RESOURCE; + spi_controller_unlock(internal->spi_controller); + return result; } static bool xpt2046_get_touched_points(Device* device, uint16_t* x, uint16_t* y, uint16_t* strength, uint8_t* point_count, uint8_t max_point_count) { diff --git a/Modules/app-esp32-module/source/app_esp32_loader_service.cpp b/Modules/app-esp32-module/source/app_esp32_loader_service.cpp index ab800982..4f96855f 100644 --- a/Modules/app-esp32-module/source/app_esp32_loader_service.cpp +++ b/Modules/app-esp32-module/source/app_esp32_loader_service.cpp @@ -8,7 +8,6 @@ #include #include -#include #include #include @@ -31,14 +30,9 @@ struct Esp32AppRuntime { }; error_t read_file(const char* path, uint8_t** out_data, size_t* out_size) { - FileMutex mutex; - file_mutex_get(&mutex, path); - file_mutex_lock(&mutex); - FILE* file = fopen(path, "rb"); if (file == nullptr) { LOG_E(TAG, "Failed to open %s", path); - file_mutex_unlock(&mutex); return ERROR_NOT_FOUND; } @@ -47,20 +41,17 @@ error_t read_file(const char* path, uint8_t** out_data, size_t* out_size) { fseek(file, 0, SEEK_SET); if (size <= 0) { fclose(file); - file_mutex_unlock(&mutex); return ERROR_RESOURCE; } auto* data = static_cast(malloc(static_cast(size))); if (data == nullptr) { fclose(file); - file_mutex_unlock(&mutex); return ERROR_OUT_OF_MEMORY; } size_t read = fread(data, 1, static_cast(size), file); fclose(file); - file_mutex_unlock(&mutex); if (read != static_cast(size)) { free(data); diff --git a/Modules/app-module/private/app/private/fs.h b/Modules/app-module/private/app/private/fs.h index b76445ba..553422a2 100644 --- a/Modules/app-module/private/app/private/fs.h +++ b/Modules/app-module/private/app/private/fs.h @@ -1,12 +1,7 @@ // SPDX-License-Identifier: Apache-2.0 #pragma once -// Minimal filesystem helpers shared by app-module internals that need to look at on-disk app -// directories (app_install.cpp, manager.cpp's install-path scan) - app-module may not depend -// upward on Tactility::file, so this is a small local re-implementation (see -// app_metadata_parsing.cpp for the same constraint applied to properties-file loading). - -#include +// Minimal filesystem helpers shared by app-module internals. #include #include @@ -21,22 +16,12 @@ inline bool app_fs_is_directory(const std::string& path) { struct stat result {}; - FileMutex file_mutex; - file_mutex_get(&file_mutex, path.c_str()); - file_mutex_lock(&file_mutex); - auto is_dir = stat(path.c_str(), &result) == 0 && S_ISDIR(result.st_mode); - file_mutex_unlock(&file_mutex); - return is_dir; + return stat(path.c_str(), &result) == 0 && S_ISDIR(result.st_mode); } inline bool app_fs_is_file(const std::string& path) { - FileMutex file_mutex; - file_mutex_get(&file_mutex, path.c_str()); - file_mutex_lock(&file_mutex); struct stat result {}; - auto retval = stat(path.c_str(), &result) == 0 && S_ISREG(result.st_mode); - file_mutex_unlock(&file_mutex); - return retval; + return stat(path.c_str(), &result) == 0 && S_ISREG(result.st_mode); } // Appends the full path of every direct subdirectory of @a path to @a out. @@ -53,15 +38,11 @@ inline bool app_fs_delete_recursively(const std::string& path) { // ESP-IDF newlib has no lstat(); ESP32 filesystems (FAT/SPIFFS) don't // support symlinks, so stat() is equivalent there. struct stat st {}; - FileMutex file_mutex; - file_mutex_get(&file_mutex, path.c_str()); - file_mutex_lock(&file_mutex); #ifdef ESP_PLATFORM int rc = stat(path.c_str(), &st); #else int rc = lstat(path.c_str(), &st); #endif - file_mutex_unlock(&file_mutex); if (rc != 0) { return false; @@ -70,24 +51,15 @@ inline bool app_fs_delete_recursively(const std::string& path) { #ifndef ESP_PLATFORM if (S_ISLNK(st.st_mode)) { // Symlink — remove as a leaf regardless of its target. - file_mutex_lock(&file_mutex); - bool result = unlink(path.c_str()) == 0; - file_mutex_unlock(&file_mutex); - return result; + return unlink(path.c_str()) == 0; } #endif if (S_ISDIR(st.st_mode)) { - // Collect child names while locked, then release before recursing — - // child paths can resolve to the same mount mutex (see - // app_fs_list_direct_subdirectories comment), so holding the parent - // lock across the recursive call would self-deadlock. std::vector children; - file_mutex_lock(&file_mutex); DIR* dir = opendir(path.c_str()); if (dir == nullptr) { - file_mutex_unlock(&file_mutex); return false; } @@ -99,7 +71,6 @@ inline bool app_fs_delete_recursively(const std::string& path) { children.push_back(path + "/" + entry->d_name); } closedir(dir); - file_mutex_unlock(&file_mutex); bool success = true; for (const auto& child : children) { @@ -109,33 +80,18 @@ inline bool app_fs_delete_recursively(const std::string& path) { } } - file_mutex_lock(&file_mutex); - bool result = rmdir(path.c_str()) == 0; - file_mutex_unlock(&file_mutex); - return result; + return rmdir(path.c_str()) == 0; } // Regular file or other — unlink. - file_mutex_lock(&file_mutex); - bool result = unlink(path.c_str()) == 0; - file_mutex_unlock(&file_mutex); - return result; + return unlink(path.c_str()) == 0; } inline void app_fs_list_direct_subdirectories(const std::string& path, std::vector& out) { - // Collect child names while the directory lock is held, then release it before classifying - // each one with app_fs_is_directory() - that function looks up and locks a FileMutex too, - // and file_mutex_get() resolves a child path to the same registered mutex as its parent - // mount. Calling it while still holding the directory's own lock would be a nested - // acquisition of that same (possibly non-recursive) mutex, and could self-deadlock. std::vector children; - FileMutex file_mutex; - file_mutex_get(&file_mutex, path.c_str()); - file_mutex_lock(&file_mutex); DIR* dir = opendir(path.c_str()); if (dir == nullptr) { - file_mutex_unlock(&file_mutex); return; } @@ -148,7 +104,6 @@ inline void app_fs_list_direct_subdirectories(const std::string& path, std::vect } closedir(dir); - file_mutex_unlock(&file_mutex); for (const auto& child_path : children) { if (app_fs_is_directory(child_path)) { diff --git a/Modules/app-module/source/app_install.cpp b/Modules/app-module/source/app_install.cpp index 8f9ea37f..74482653 100644 --- a/Modules/app-module/source/app_install.cpp +++ b/Modules/app-module/source/app_install.cpp @@ -8,7 +8,6 @@ #include #include -#include #include #include @@ -44,11 +43,7 @@ bool ensure_directory(const std::string& path) { return true; } - FileMutex mutex {}; - file_mutex_get(&mutex, path.c_str()); - file_mutex_lock(&mutex); bool created = mkdir(path.c_str(), 0777) == 0 || errno == EEXIST; - file_mutex_unlock(&mutex); if (!created) { return false; } @@ -140,6 +135,66 @@ bool untar(const std::string& tar_path, const std::string& destination_path) { // endregion +// region Staging-path lock: at most one caller may clean up/populate a given staging_path at a +// time, keyed by source basename. The HTTP server and app tasks can call app_install() +// concurrently, e.g. two uploads sharing a source basename - without this, one call's cleanup +// can delete or overwrite the staging directory another call is still extracting into. + +struct StagingLock { + Mutex mutex {}; + int refcount = 0; +}; + +struct StagingLockTable { + std::unordered_map> locks; + Mutex table_mutex {}; + + StagingLockTable() { mutex_construct(&table_mutex); } +}; + +StagingLockTable& staging_lock_table() { + static StagingLockTable table; + return table; +} + +// Blocks until any other caller staging @a path has released it, then locks it for this caller. +// Must be paired with exactly one release_staging_lock(path) call. +void acquire_staging_lock(const std::string& path) { + auto& table = staging_lock_table(); + mutex_lock(&table.table_mutex); + auto iterator = table.locks.find(path); + if (iterator == table.locks.end()) { + auto lock = std::make_unique(); + mutex_construct(&lock->mutex); + iterator = table.locks.emplace(path, std::move(lock)).first; + } + StagingLock* lock = iterator->second.get(); + lock->refcount++; + mutex_unlock(&table.table_mutex); + + mutex_lock(&lock->mutex); +} + +// Erases the table entry once nothing references it anymore, so the table doesn't grow forever +// across installs with distinct basenames (e.g. unique upload temp names). +void release_staging_lock(const std::string& path) { + auto& table = staging_lock_table(); + mutex_lock(&table.table_mutex); + auto iterator = table.locks.find(path); + if (iterator == table.locks.end()) { + mutex_unlock(&table.table_mutex); + return; + } + StagingLock* lock = iterator->second.get(); + mutex_unlock(&lock->mutex); + if (--lock->refcount == 0) { + table.locks.erase(iterator); + } + mutex_unlock(&table.table_mutex); +} + +// endregion + // region Installed-app registry: owns the AppManifest (and its id/name/path strings) that // app_manager's ledger only keeps a non-owning pointer to (see app_manager_add()'s contract). @@ -282,22 +337,14 @@ error_t app_install(const char* source_path) { } auto staging_path = app_parent_path + "/" + last_path_segment(source_path); + acquire_staging_lock(staging_path); + delete_recursively(staging_path); - FileMutex target_mutex {}; - file_mutex_get(&target_mutex, app_parent_path.c_str()); - FileMutex source_mutex {}; - file_mutex_get(&source_mutex, source_path); - - file_mutex_lock(&target_mutex); - file_mutex_lock(&source_mutex); - bool untar_success = untar(source_path, staging_path); - file_mutex_unlock(&source_mutex); - file_mutex_unlock(&target_mutex); - - if (!untar_success) { + if (!untar(source_path, staging_path)) { LOG_E(TAG, "Failed to extract %s", source_path); delete_recursively(staging_path); + release_staging_lock(staging_path); return ERROR_NOT_FOUND; } @@ -305,6 +352,7 @@ error_t app_install(const char* source_path) { if (!app_fs_is_file(manifest_path)) { LOG_E(TAG, "Manifest not found at %s", manifest_path.c_str()); delete_recursively(staging_path); + release_staging_lock(staging_path); return ERROR_INVALID_ARGUMENT; } @@ -312,6 +360,7 @@ error_t app_install(const char* source_path) { if (app_metadata_parse(manifest_path.c_str(), &metadata) != ERROR_NONE) { LOG_E(TAG, "Install failed: invalid manifest"); delete_recursively(staging_path); + release_staging_lock(staging_path); return ERROR_INVALID_ARGUMENT; } @@ -331,22 +380,21 @@ error_t app_install(const char* source_path) { LOG_E(TAG, "Install failed: failed to remove existing installation"); mutex_unlock(®istry.mutex); delete_recursively(staging_path); + release_staging_lock(staging_path); return ERROR_RESOURCE; } auto final_path = app_parent_path + "/" + metadata.app_id; delete_recursively(final_path); - file_mutex_lock(&target_mutex); - bool rename_success = rename(staging_path.c_str(), final_path.c_str()) == 0; - file_mutex_unlock(&target_mutex); - - if (!rename_success) { + if (rename(staging_path.c_str(), final_path.c_str()) != 0) { LOG_E(TAG, "Failed to rename \"%s\" to \"%s\"", staging_path.c_str(), final_path.c_str()); delete_recursively(staging_path); + release_staging_lock(staging_path); mutex_unlock(®istry.mutex); return ERROR_NOT_FOUND; } + release_staging_lock(staging_path); // Only remaining failure mode is a duplicate id - can't happen, uninstall_locked() above // already removed any previous registration for this exact id. diff --git a/Modules/app-module/source/app_metadata_parsing.cpp b/Modules/app-module/source/app_metadata_parsing.cpp index 54947d3e..2b44afd7 100644 --- a/Modules/app-module/source/app_metadata_parsing.cpp +++ b/Modules/app-module/source/app_metadata_parsing.cpp @@ -1,6 +1,4 @@ // SPDX-License-Identifier: Apache-2.0 -#include - #include #include @@ -62,13 +60,8 @@ bool validate_csv_list(const std::string& value, bool (*is_valid_item)(const std * minimal re-implementation rather than depending on Tactility's file::loadPropertiesFile() - * app-module (like every other kernel module) may not depend upward on the Tactility layer. */ bool load_properties(const std::string& path, std::map& out_properties, std::string& out_first_line) { - FileMutex mutex; - file_mutex_get(&mutex, path.c_str()); - file_mutex_lock(&mutex); - std::ifstream file(path); if (!file.is_open()) { - file_mutex_unlock(&mutex); return false; } @@ -103,7 +96,6 @@ bool load_properties(const std::string& path, std::map out_properties[key] = value; } - file_mutex_unlock(&mutex); return true; } diff --git a/Modules/gps-module/source/gps_settings.cpp b/Modules/gps-module/source/gps_settings.cpp index 7f9729d9..2b706eb0 100644 --- a/Modules/gps-module/source/gps_settings.cpp +++ b/Modules/gps-module/source/gps_settings.cpp @@ -4,7 +4,6 @@ #include -#include #include #include @@ -42,37 +41,12 @@ static bool get_configuration_path(char* out_path, size_t out_path_size) { return service_paths_get_user_data_path(GPS_SETTINGS_STORAGE_ID, "config.bin", out_path, out_path_size) == ERROR_NONE; } -// Holds the lock (if any) that `path` needs for the lifetime of the guard - see file_find_lock(). -class FileLockGuard { - FileMutex mutex; - bool locked; -public: - explicit FileLockGuard(const char* path) { - file_mutex_get(&mutex, path); - file_mutex_lock(&mutex); - locked = true; - } - - ~FileLockGuard() { - unlock(); - } - - void unlock() { - if (locked) { - file_mutex_unlock(&mutex); - locked = false; - } - } -}; - void gps_settings_for_each_configuration(void* context, void (*on_configuration)(const GpsConfiguration* configuration, size_t index, void* context)) { char path[224]; if (!get_configuration_path(path, sizeof(path))) { return; } - FileLockGuard lock(path); - FILE* file = fopen(path, "rb"); if (file == nullptr) { return; // No configurations saved yet @@ -107,8 +81,6 @@ static error_t write_configurations(const std::vector& configu return ERROR_RESOURCE; } - FileLockGuard lock(path); - ensure_directory_exists(directory); FILE* file = fopen(path, "wb"); @@ -130,7 +102,6 @@ static error_t write_configurations(const std::vector& configu return ERROR_RESOURCE; } - lock.unlock(); gps_ledger_sync(); return ERROR_NONE; diff --git a/Platforms/platform-esp32/include/tactility/drivers/esp32_sdcard.h b/Platforms/platform-esp32/include/tactility/drivers/esp32_sdcard.h index 6405c893..0a7ae80b 100644 --- a/Platforms/platform-esp32/include/tactility/drivers/esp32_sdcard.h +++ b/Platforms/platform-esp32/include/tactility/drivers/esp32_sdcard.h @@ -2,6 +2,7 @@ #pragma once #include +#include #ifdef __cplusplus extern "C" { @@ -16,6 +17,27 @@ struct Device; */ sdmmc_card_t* esp32_sdcard_get_card(struct Device* device); +/** + * @brief Wraps the card's do_transaction function pointer so every SD command takes the parent + * SPI controller's bus lock. + * @param[in] device the SD card device + * @param[in] card the card returned by the mount call, passed directly rather than re-resolved + * via esp32_sdcard_get_card(): this is called from within the driver's own start_device + * callback, before device_is_ready() is true, so the readiness-gated getters can't be used + * here. + * @retval ERROR_NONE on success, or when the device's parent is not a SPI controller (no-op, + * e.g. SDMMC cards, which have no shared bus to arbitrate) + */ +error_t esp32_sdcard_install_bus_lock(struct Device* device, sdmmc_card_t* card); + +/** + * @brief Removes the bus lock installed by esp32_sdcard_install_bus_lock(), restoring the card's + * original do_transaction. No-op if no lock was installed for this device. Must be called before + * the card handle is freed. + * @param[in] device the SD card device + */ +void esp32_sdcard_remove_bus_lock(struct Device* device); + #ifdef __cplusplus } #endif diff --git a/Platforms/platform-esp32/source/drivers/esp32_sdcard.cpp b/Platforms/platform-esp32/source/drivers/esp32_sdcard.cpp index 0ca4eed1..1dd91f05 100644 --- a/Platforms/platform-esp32/source/drivers/esp32_sdcard.cpp +++ b/Platforms/platform-esp32/source/drivers/esp32_sdcard.cpp @@ -3,6 +3,9 @@ #include #include #include +#include + +#include #include #if SOC_SDMMC_HOST_SUPPORTED @@ -24,4 +27,78 @@ sdmmc_card_t* esp32_sdcard_get_card(Device* device) { return nullptr; } +// No board in the tree has more than one SD-over-SPI card. +static constexpr size_t MAX_BUS_LOCKS = 2; + +struct SdcardBusLock { + Device* sdcard = nullptr; // owner, for removal + Device* controller = nullptr; // parent SPI controller + int slot = 0; // sdspi_dev_handle_t, read back from card->host.slot + esp_err_t (*inner)(int, sdmmc_command_t*) = nullptr; +}; + +static SdcardBusLock bus_locks[MAX_BUS_LOCKS]; + +static SdcardBusLock* find_by_slot(int slot) { + for (auto& entry : bus_locks) { + if (entry.sdcard != nullptr && entry.slot == slot) { + return &entry; + } + } + return nullptr; +} + +static esp_err_t locked_do_transaction(int slot, sdmmc_command_t* cmd) { + const SdcardBusLock* entry = find_by_slot(slot); + if (entry == nullptr) { + return ESP_ERR_INVALID_STATE; + } + spi_controller_lock(entry->controller); + esp_err_t result = entry->inner(slot, cmd); + spi_controller_unlock(entry->controller); + return result; +} + +error_t esp32_sdcard_install_bus_lock(Device* device, sdmmc_card_t* card) { + auto* parent = device_get_parent(device); + if (parent == nullptr || device_get_type(parent) != &SPI_CONTROLLER_TYPE) { + return ERROR_NONE; + } + + if (card == nullptr) { + return ERROR_INVALID_STATE; + } + + SdcardBusLock* slot_entry = nullptr; + for (auto& entry : bus_locks) { + if (entry.sdcard == nullptr) { + slot_entry = &entry; + break; + } + } + if (slot_entry == nullptr) { + return ERROR_OUT_OF_MEMORY; + } + + slot_entry->sdcard = device; + slot_entry->controller = parent; + slot_entry->slot = card->host.slot; + slot_entry->inner = card->host.do_transaction; + card->host.do_transaction = locked_do_transaction; + return ERROR_NONE; +} + +void esp32_sdcard_remove_bus_lock(Device* device) { + for (auto& entry : bus_locks) { + if (entry.sdcard == device) { + auto* card = esp32_sdcard_get_card(device); + if (card != nullptr) { + card->host.do_transaction = entry.inner; + } + entry = SdcardBusLock{}; + return; + } + } +} + } diff --git a/Platforms/platform-esp32/source/drivers/esp32_sdspi.cpp b/Platforms/platform-esp32/source/drivers/esp32_sdspi.cpp index f7d23248..fb5bf0bf 100644 --- a/Platforms/platform-esp32/source/drivers/esp32_sdspi.cpp +++ b/Platforms/platform-esp32/source/drivers/esp32_sdspi.cpp @@ -5,6 +5,7 @@ #include #include #include +#include #include #include #include @@ -92,6 +93,12 @@ static error_t start(Device* device) { auto* spi_config = static_cast(parent->config); + // Card init below runs through the SD host driver's own do_transaction, which is not yet + // patched to take the bus lock (that only happens once esp32_sdcard_install_bus_lock() runs, + // after a successful mount) - so this whole window, from CS bit-banging through mount, needs + // the coarse controller lock instead. + spi_controller_lock(parent); + // Lower all CS pins esp32_spi_deselect_all_cs(parent); // Manually set the CS pin fo @@ -100,6 +107,7 @@ static error_t start(Device* device) { data->fs_handle = esp32_sdspi_fs_alloc(config, spi_config->host, cs_pin_spec.pin, "/sdcard"); if (!data->fs_handle) { + spi_controller_unlock(parent); data->cleanup_pins(); device_set_driver_data(device, nullptr); data->unlock(); @@ -111,8 +119,18 @@ static error_t start(Device* device) { file_system_set_owner(data->file_system, device); if (file_system_mount(data->file_system) != ERROR_NONE) { LOG_E(TAG, "Failed to mount SD card filesystem"); + } else { + // Pass the card straight from the just-succeeded mount, not via esp32_sdcard_get_card(): + // that getter gates on device_is_ready(), which is still false here -- device_start() + // only flips it after this start_device callback returns. Going through the gated getter + // silently no-ops the lock install here every time, leaving the card's do_transaction + // unwrapped and racing the display for the device's entire lifetime. + if (esp32_sdcard_install_bus_lock(device, esp32_sdspi_fs_get_card(data->fs_handle)) != ERROR_NONE) { + LOG_E(TAG, "Failed to install SD card bus lock"); + } } + spi_controller_unlock(parent); data->unlock(); return ERROR_NONE; } @@ -130,6 +148,7 @@ static error_t stop(Device* device) { data->unlock(); return ERROR_RESOURCE; } + esp32_sdcard_remove_bus_lock(device); } file_system_remove(data->file_system); diff --git a/Tactility/Include/Tactility/file/File.h b/Tactility/Include/Tactility/file/File.h index feacbd8a..71c80cd6 100644 --- a/Tactility/Include/Tactility/file/File.h +++ b/Tactility/Include/Tactility/file/File.h @@ -1,13 +1,7 @@ -/** - * All functions in this file can be safely called without manually applying file locks. - * For calls to C stdlib APIs such as fopen(), always lock with file::FileMutexGuard(path) first! - */ #pragma once #include -#include - #include #include #include @@ -16,10 +10,6 @@ #include #include -/** - * @warning SD card access requires a locking mechanism: - * @warning When using this in the Tactility main project, use `file::FileMutexGuard` - */ namespace tt::file { /** File types for `dirent`'s `d_type`. */ @@ -49,27 +39,6 @@ struct FileCloser { } }; -/** - * RAII lock over TactilityKernel's file_mutex.h for the file system mount that owns `path`. - * Locks in the constructor, unlocks in the destructor - no heap allocation, no virtual dispatch. - */ -class FileMutexGuard final { - FileMutex mutex {}; - -public: - explicit FileMutexGuard(const std::string& path) { - file_mutex_get(&mutex, path.c_str()); - file_mutex_lock(&mutex); - } - - ~FileMutexGuard() { - file_mutex_unlock(&mutex); - } - - FileMutexGuard(const FileMutexGuard&) = delete; - FileMutexGuard& operator=(const FileMutexGuard&) = delete; -}; - long getSize(FILE* file); /** Read a file and return its data. diff --git a/Tactility/Include/Tactility/file/ObjectFile.h b/Tactility/Include/Tactility/file/ObjectFile.h index 661e17d7..9e88df93 100644 --- a/Tactility/Include/Tactility/file/ObjectFile.h +++ b/Tactility/Include/Tactility/file/ObjectFile.h @@ -2,13 +2,8 @@ #include -#include - #include -/** - * @warning The functionality below does NOT safely acquire file locks. Use file::FileMutexGuard when using the functionality below. - */ namespace tt::file { class ObjectFileReader { @@ -45,7 +40,6 @@ class ObjectFileWriter { const uint32_t recordSize; const uint32_t recordVersion; const bool append; - FileMutex mutex {}; std::unique_ptr file; uint32_t recordsWritten = 0; @@ -57,9 +51,7 @@ public: recordSize(recordSize), recordVersion(recordVersion), append(append) - { - file_mutex_get(&mutex, this->filePath.c_str()); - } + {} ~ObjectFileWriter() { diff --git a/Tactility/Source/Tactility.cpp b/Tactility/Source/Tactility.cpp index 6e4acaa7..5803a225 100644 --- a/Tactility/Source/Tactility.cpp +++ b/Tactility/Source/Tactility.cpp @@ -61,7 +61,6 @@ #include #include #include -#include #include #include #include @@ -73,9 +72,6 @@ constexpr auto* TAG = "Tactility"; static DispatcherHandle_t mainDispatcherHandle = dispatcher_alloc(); -void initFileMutexForLvgl(); -void deinitFileMutexForLvgl(); - namespace { void mainDispatcherTrampoline(void* context) { @@ -321,19 +317,12 @@ void createTempDirectory() { auto data_path = getDataPath(); auto temp_path = std::format("{}/tmp", data_path); if (!file::isDirectory(temp_path)) { - FileMutex mutex; - file_mutex_get(&mutex, data_path.c_str()); - if (file_mutex_try_lock(&mutex, 1000 / portTICK_PERIOD_MS)) { - if (!file::findOrCreateParentDirectory(temp_path, 0777)) { - LOG_E(TAG, "Failed to create %s", data_path.c_str()); - } else if (mkdir(temp_path.c_str(), 0777) == 0) { - LOG_I(TAG, "Created %s", temp_path.c_str()); - } else { - LOG_E(TAG, "Failed to create %s", temp_path.c_str()); - } - file_mutex_unlock(&mutex); + if (!file::findOrCreateParentDirectory(temp_path, 0777)) { + LOG_E(TAG, "Failed to create %s", data_path.c_str()); + } else if (mkdir(temp_path.c_str(), 0777) == 0) { + LOG_I(TAG, "Created %s", temp_path.c_str()); } else { - LOG_E(TAG, LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, data_path.c_str()); + LOG_E(TAG, "Failed to create %s", temp_path.c_str()); } } else { LOG_I(TAG, "Found existing %s", temp_path.c_str()); @@ -419,8 +408,6 @@ static void applySavedTouchCalibration() { #endif // CONFIG_TT_TOUCH_CALIBRATION_SUPPORTED static void onLvglStarted() { - initFileMutexForLvgl(); - window_manager_configure(windowManagerScreenInit); check(module_ensure_started(&lvgl_window_manager_module) == ERROR_NONE); @@ -451,14 +438,6 @@ static void onLvglStarted() { } static void onLvglStopped() { - deinitFileMutexForLvgl(); - - if (softwareKeyboard.object != nullptr) { - lvgl_software_keyboard_destruct(&softwareKeyboard); - } - - module_stop(&lvgl_window_manager_module); - lvgl::stopKeyboardDeviceListener(); lvgl::stopUsbHidInput(); @@ -474,6 +453,12 @@ static void onLvglStopped() { check(service::removeService(service::memorychecker::manifest.id)); check(service::removeService(service::statusbar::manifest.id)); + if (softwareKeyboard.object != nullptr) { + lvgl_software_keyboard_destruct(&softwareKeyboard); + } + + module_stop(&lvgl_window_manager_module); + memory_print_stats(); } diff --git a/Tactility/Source/app/apphub/AppHubEntry.cpp b/Tactility/Source/app/apphub/AppHubEntry.cpp index 441dbe6b..57b0e626 100644 --- a/Tactility/Source/app/apphub/AppHubEntry.cpp +++ b/Tactility/Source/app/apphub/AppHubEntry.cpp @@ -21,8 +21,6 @@ static bool parseEntry(const cJSON* object, AppHubEntry& entry) { } bool parseJson(const std::string& filePath, AppHubEntryList& entries) { - file::FileMutexGuard guard(filePath); - auto data = file::readString(filePath); if (data == nullptr) { LOG_E(TAG, "Failed to read %s", filePath.c_str()); diff --git a/Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp b/Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp index 02d43146..4172af50 100644 --- a/Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp +++ b/Tactility/Source/app/crashdiagnostics/CrashDiagnostics.cpp @@ -110,7 +110,6 @@ void writeCrashLogFile(const CrashData& crashData) { } std::string path = std::string(root) + "/crash.txt"; - file::FileMutexGuard guard(path); if (!file::writeString(path, formatCrashData(crashData))) { LOG_E(TAG, "Failed to write %s", path.c_str()); } diff --git a/Tactility/Source/app/files/View.cpp b/Tactility/Source/app/files/View.cpp index 58292d10..51d6ca3d 100644 --- a/Tactility/Source/app/files/View.cpp +++ b/Tactility/Source/app/files/View.cpp @@ -13,17 +13,17 @@ #include #include #include -#include #include #include #include -#include #include #include +#include #include #include +#include #include namespace tt::app::files { @@ -106,31 +106,13 @@ static void onPastePressedCallback(lv_event_t* event) { // region File helpers static bool copyFileContents(const std::string& src, const std::string& dst) { - FileMutex src_mutex; - file_mutex_get(&src_mutex, src.c_str()); - FileMutex dst_mutex; - file_mutex_get(&dst_mutex, dst.c_str()); - const bool same_lock = (src_mutex.lock == dst_mutex.lock && - src_mutex.try_lock == dst_mutex.try_lock && - src_mutex.unlock == dst_mutex.unlock); - - auto unlock_all = [&] { - if (!same_lock) file_mutex_unlock(&dst_mutex); - file_mutex_unlock(&src_mutex); - }; - - file_mutex_lock(&src_mutex); - if (!same_lock) file_mutex_lock(&dst_mutex); - FILE* in = fopen(src.c_str(), "rb"); if (in == nullptr) { - unlock_all(); return false; } FILE* out = fopen(dst.c_str(), "wb"); if (out == nullptr) { fclose(in); - unlock_all(); return false; } uint8_t buf[512]; @@ -152,7 +134,6 @@ static bool copyFileContents(const std::string& src, const std::string& dst) { if (!success) { remove(dst.c_str()); } - unlock_all(); return success; } @@ -162,33 +143,23 @@ static bool copyRecursive(const std::string& src, const std::string& dst) { return false; } - // Process one entry at a time: release the device lock between iterations - // so other SPI bus users aren't starved, and stop immediately on failure. - FileMutex mutex; - file_mutex_get(&mutex, src.c_str()); - file_mutex_lock(&mutex); DIR* dir = opendir(src.c_str()); if (!dir) { - file_mutex_unlock(&mutex); file::deleteRecursively(dst); return false; } bool success = true; while (success) { - struct dirent* entry = readdir(dir); + dirent* entry = readdir(dir); if (!entry) break; if (strcmp(entry->d_name, ".") == 0 || strcmp(entry->d_name, "..") == 0) continue; std::string name = entry->d_name; // copy before releasing lock - file_mutex_unlock(&mutex); success = copyRecursive(file::getChildPath(src, name), file::getChildPath(dst, name)); - - file_mutex_lock(&mutex); } closedir(dir); - file_mutex_unlock(&mutex); if (!success) { file::deleteRecursively(dst); @@ -608,7 +579,6 @@ void View::onResult(uint32_t launchId, int32_t result) { LOG_W(TAG, "Failed to delete %s", filepath.c_str()); } } else if (file::isFile(filepath)) { - file::FileMutexGuard guard(filepath); if (remove(filepath.c_str()) != 0) { LOG_W(TAG, "Failed to delete %s", filepath.c_str()); } @@ -623,20 +593,17 @@ void View::onResult(uint32_t launchId, int32_t result) { std::string new_name = resultText; if (!new_name.empty() && new_name != state->getSelectedChildEntry()) { std::string rename_to = file::getChildPath(state->getCurrentPath(), new_name); - { - file::FileMutexGuard guard(filepath); - struct stat st; - if (stat(rename_to.c_str(), &st) == 0) { - LOG_W(TAG, "Rename: destination already exists: \"%s\"", rename_to.c_str()); - state->setPendingAction(State::ActionNone); - alertdialog::start(appInstanceId, "Rename failed", "\"" + new_name + "\" already exists."); - break; - } - if (rename(filepath.c_str(), rename_to.c_str()) == 0) { - LOG_I(TAG, "Renamed \"%s\" to \"%s\"", filepath.c_str(), rename_to.c_str()); - } else { - LOG_E(TAG, "Failed to rename \"%s\" to \"%s\"", filepath.c_str(), rename_to.c_str()); - } + struct stat st; + if (stat(rename_to.c_str(), &st) == 0) { + LOG_W(TAG, "Rename: destination already exists: \"%s\"", rename_to.c_str()); + state->setPendingAction(State::ActionNone); + alertdialog::start(appInstanceId, "Rename failed", "\"" + new_name + "\" already exists."); + break; + } + if (rename(filepath.c_str(), rename_to.c_str()) == 0) { + LOG_I(TAG, "Renamed \"%s\" to \"%s\"", filepath.c_str(), rename_to.c_str()); + } else { + LOG_E(TAG, "Failed to rename \"%s\" to \"%s\"", filepath.c_str(), rename_to.c_str()); } state->setEntriesForPath(state->getCurrentPath()); @@ -649,22 +616,21 @@ void View::onResult(uint32_t launchId, int32_t result) { if (!filename.empty()) { std::string new_file_path = file::getChildPath(state->getCurrentPath(), filename); - { - file::FileMutexGuard guard(new_file_path); - - struct stat st; - if (stat(new_file_path.c_str(), &st) == 0) { - LOG_W(TAG, "File already exists: \"%s\"", new_file_path.c_str()); - break; - } - - FILE* new_file = fopen(new_file_path.c_str(), "w"); + // O_CREAT | O_EXCL makes creation+existence-check one atomic operation, unlike a separate stat() before fopen() + int fd = open(new_file_path.c_str(), O_CREAT | O_EXCL | O_WRONLY, 0644); + if (fd >= 0) { + FILE* new_file = fdopen(fd, "w"); if (new_file) { fclose(new_file); - LOG_I(TAG, "Created file \"%s\"", new_file_path.c_str()); } else { - LOG_E(TAG, "Failed to create file \"%s\"", new_file_path.c_str()); + close(fd); } + LOG_I(TAG, "Created file \"%s\"", new_file_path.c_str()); + } else if (errno == EEXIST) { + LOG_W(TAG, "File already exists: \"%s\"", new_file_path.c_str()); + break; + } else { + LOG_E(TAG, "Failed to create file \"%s\"", new_file_path.c_str()); } state->setEntriesForPath(state->getCurrentPath()); @@ -677,20 +643,16 @@ void View::onResult(uint32_t launchId, int32_t result) { if (!foldername.empty()) { std::string new_folder_path = file::getChildPath(state->getCurrentPath(), foldername); - { - file::FileMutexGuard guard(new_folder_path); + struct stat st; + if (stat(new_folder_path.c_str(), &st) == 0) { + LOG_W(TAG, "Folder already exists: \"%s\"", new_folder_path.c_str()); + break; + } - struct stat st; - if (stat(new_folder_path.c_str(), &st) == 0) { - LOG_W(TAG, "Folder already exists: \"%s\"", new_folder_path.c_str()); - break; - } - - if (mkdir(new_folder_path.c_str(), 0755) == 0) { - LOG_I(TAG, "Created folder \"%s\"", new_folder_path.c_str()); - } else { - LOG_E(TAG, "Failed to create folder \"%s\"", new_folder_path.c_str()); - } + if (mkdir(new_folder_path.c_str(), 0755) == 0) { + LOG_I(TAG, "Created folder \"%s\"", new_folder_path.c_str()); + } else { + LOG_E(TAG, "Failed to create folder \"%s\"", new_folder_path.c_str()); } state->setEntriesForPath(state->getCurrentPath()); @@ -709,12 +671,9 @@ void View::onResult(uint32_t launchId, int32_t result) { // Revalidate right before the destructive delete so we only ever // remove the exact file the user agreed to overwrite. bool dst_unchanged; - { - file::FileMutexGuard guard(dst); - struct stat current_stat {}; - dst_unchanged = (stat(dst.c_str(), ¤t_stat) == 0) && - state->pendingPasteDstMatches(current_stat); - } + struct stat current_stat {}; + dst_unchanged = (stat(dst.c_str(), ¤t_stat) == 0) && + state->pendingPasteDstMatches(current_stat); state->clearPendingPasteDstStat(); if (!dst_unchanged) { @@ -780,13 +739,6 @@ void View::onPastePressed() { std::string entry_name = file::getLastPathSegment(src); std::string dst = file::getChildPath(state->getCurrentPath(), entry_name); - // Note: FileMutexGuard(src) guards the source path; the existence check below is - // against dst, so there is a TOCTOU gap between this check and the write inside - // doPaste. When dst exists, the overwrite-confirm path below re-validates dst's - // stat immediately before the destructive delete (see ActionPaste in onResult), - // closing the window that matters (the dialog being open). When dst does not - // exist here, doPaste's write can still race a concurrent creator; acceptable on - // a single-user embedded device. if (src == dst) { LOG_I(TAG, "Paste: source and destination are the same path, skipping"); return; @@ -794,12 +746,9 @@ void View::onPastePressed() { bool dst_exists; struct stat dst_stat {}; - { - file::FileMutexGuard guard(src); - dst_exists = (stat(dst.c_str(), &dst_stat) == 0); - } - if (dst_exists) { + // If dst exists... + if (stat(dst.c_str(), &dst_stat) == 0) { state->setPendingPasteDst(dst); state->setPendingPasteDstStat(dst_stat); state->setPendingAction(State::ActionPaste); @@ -815,11 +764,7 @@ void View::doPaste(const std::string& src, bool is_cut, const std::string& dst) bool success = false; bool src_delete_failed = false; if (is_cut) { - { - file::FileMutexGuard guard(src); - success = (rename(src.c_str(), dst.c_str()) == 0); - } - if (!success) { + if (rename(src.c_str(), dst.c_str()) != 0) { // Fallback for cross-filesystem moves: copy then delete. // Only mark success if both halves succeed — if the source removal // fails we leave success=false so the clipboard is preserved and diff --git a/Tactility/Source/app/notes/Notes.cpp b/Tactility/Source/app/notes/Notes.cpp index 94d6b5d2..8bf5bed5 100644 --- a/Tactility/Source/app/notes/Notes.cpp +++ b/Tactility/Source/app/notes/Notes.cpp @@ -46,8 +46,6 @@ void resetFileContent(Context* ctx) { } void openFile(Context* ctx, const std::string& path) { - // We might be reading from the SD card, which could share a SPI bus with other devices (display) - file::FileMutexGuard guard(path); auto data = file::readString(path); if (data != nullptr) { lvgl_lock(); @@ -60,15 +58,11 @@ void openFile(Context* ctx, const std::string& path) { } bool saveFile(Context* ctx, const std::string& path) { - // We might be writing to SD card, which could share a SPI bus with other devices (display) bool result = false; - { - file::FileMutexGuard guard(path); - if (file::writeString(path, ctx->saveBuffer.c_str())) { - LOG_I(TAG, "Saved to %s", path.c_str()); - ctx->filePath = path; - result = true; - } + if (file::writeString(path, ctx->saveBuffer.c_str())) { + LOG_I(TAG, "Saved to %s", path.c_str()); + ctx->filePath = path; + result = true; } return result; } diff --git a/Tactility/Source/app/setup/Setup.cpp b/Tactility/Source/app/setup/Setup.cpp index cc209553..b06e2831 100644 --- a/Tactility/Source/app/setup/Setup.cpp +++ b/Tactility/Source/app/setup/Setup.cpp @@ -58,7 +58,6 @@ bool isCompleted() { LOG_E(TAG, "Setup path not found"); return false; } - file::FileMutexGuard guard(path); return file::isFile(path); } @@ -69,7 +68,6 @@ void markCompleted() { if (!getCompletedMarkerPath(path)) { return; } - file::FileMutexGuard guard(path); file::writeString(path, ""); } diff --git a/Tactility/Source/file/File.cpp b/Tactility/Source/file/File.cpp index 99705053..614cda93 100644 --- a/Tactility/Source/file/File.cpp +++ b/Tactility/Source/file/File.cpp @@ -42,8 +42,6 @@ bool listDirectory( const std::string& path, std::function onEntry ) { - FileMutexGuard guard(path); - LOG_I(TAG, "listDir start %s", path.c_str()); DIR* dir = opendir(path.c_str()); if (dir == nullptr) { @@ -68,8 +66,6 @@ int scandir( ScandirFilter filterMethod, ScandirSort sortMethod ) { - FileMutexGuard guard(path); - LOG_I(TAG, "scandir start"); DIR* dir = opendir(path.c_str()); if (dir == nullptr) { @@ -193,8 +189,6 @@ bool writeString(const std::string& filepath, const std::string& content) { } static bool findOrCreateDirectoryInternal(std::string path, mode_t mode) { - FileMutexGuard guard(path); - struct stat dir_stat; if (mkdir(path.c_str(), mode) == 0) { return true; @@ -310,29 +304,23 @@ bool deleteRecursively(const std::string& path) { } bool deleteFile(const std::string& path) { - FileMutexGuard guard(path); return remove(path.c_str()) == 0; } bool deleteDirectory(const std::string& path) { - FileMutexGuard guard(path); return rmdir(path.c_str()) == 0; } bool isFile(const std::string& path) { - FileMutexGuard guard(path); return access(path.c_str(), F_OK) == 0; } bool isDirectory(const std::string& path) { - FileMutexGuard guard(path); struct stat stat_result; return stat(path.c_str(), &stat_result) == 0 && S_ISDIR(stat_result.st_mode); } bool readLines(const std::string& filePath, bool stripNewLine, std::function callback) { - FileMutexGuard guard(filePath); - auto* file = fopen(filePath.c_str(), "r"); if (file == nullptr) { return false; diff --git a/Tactility/Source/file/FileMutexLvgl.cpp b/Tactility/Source/file/FileMutexLvgl.cpp deleted file mode 100644 index 3d4eeffc..00000000 --- a/Tactility/Source/file/FileMutexLvgl.cpp +++ /dev/null @@ -1,135 +0,0 @@ -#include -#include -#include -#include -#include -#include -#include - -#include - -constexpr auto* TAG = "file_mutex_lvgl"; - -struct Device; - -namespace { - -std::vector registered_ids; - -void wrapped_lvgl_lock() { - if (!lvgl_is_running()) return; - lvgl_lock(); -} - -bool wrapped_lvgl_try_lock(uint32_t timeout) { - // Return lock success, so the file operation can continue when LVGL is not running - // lvgl_try_lock() fails to lock if LVGL is not running - if (!lvgl_is_running()) return true; - return lvgl_try_lock(timeout); -} -void wrapped_lvgl_unlock() { - if (!lvgl_is_running()) return; - lvgl_unlock(); -} - -const FileMutex lvgl_mutex = { - .lock = wrapped_lvgl_lock, - .try_lock = wrapped_lvgl_try_lock, - .unlock = wrapped_lvgl_unlock, -}; - -} - -namespace tt { - -/** - * Finds file systems with a device (e.g. sd card) that is owned by a SPI controller. - * If the SPI controller has a display on the bus, we create an LVGL lock for the file system path. - */ -void initFileMutexForLvgl() { - file_system_for_each(®istered_ids, [](FileSystem* fs, void* context) { - char mount_path[64]; - if (file_system_get_path(fs, mount_path, sizeof(mount_path)) != ERROR_NONE) { - return true; - } - LOG_D(TAG, "Mount path %s", mount_path); - - // We only care about file system with a Device (owner) - auto* owner = file_system_get_owner(fs); - if (owner == nullptr) { - LOG_D(TAG, "Owner: none"); - return true; - } - - LOG_D(TAG, "Owner: %s", owner->name); - - // Ignore devices without a parent (root) - auto* parent = device_get_parent(owner); - if (parent == nullptr) { - LOG_D(TAG, "Owner: no parent"); - return true; - } - - LOG_D(TAG, "Owner: parent %s", parent->name); - - // If the FileSystem is on a SPI bus and there's more than 1 device, we assume the other one is the display. - auto* type = device_get_type(parent); - if (type != &SPI_CONTROLLER_TYPE || device_get_child_count(parent) <= 1) { - LOG_D(TAG, "Owner parent not SPI controller or not enough children"); - return true; - } - - struct Context { - const char* mountPath; - std::vector* registeredIds; - }; - Context ctx = { .mountPath = mount_path, .registeredIds = static_cast*>(context) }; - - device_for_each_child(parent, &ctx, [](Device* child, void* context) -> bool { - Context* ctx = static_cast(context); - if (device_get_type(child) == &DISPLAY_TYPE) { - LOG_I(TAG, "Adding file mutex for %s as it shares a bus with a display", ctx->mountPath); - ctx->registeredIds->push_back(file_mutex_add(&lvgl_mutex, ctx->mountPath)); - return false; - } else { - LOG_D(TAG, "child of parent, %s: not DISPLAY_TYPE", child->name); - } - return true; - }); - - return true; - }); - - // SDMMC-backed SD cards aren't parented under SPI_CONTROLLER_TYPE, so the pass above never - // sees them - but on some chips (classic ESP32) SDMMC and SPI still contend for DMA/bus - // access. Lock every SD card mount if a display exists anywhere, regardless of bus topology. - if (!device_exists_of_type(&DISPLAY_TYPE)) { - return; - } - - file_system_for_each(®istered_ids, [](FileSystem* fs, void* context) { - char mount_path[64]; - if (file_system_get_path(fs, mount_path, sizeof(mount_path)) != ERROR_NONE) { - return true; - } - - auto* owner = file_system_get_owner(fs); - if (owner == nullptr || device_get_type(owner) != &SDCARD_TYPE) { - return true; - } - - LOG_I(TAG, "Adding file mutex for %s (SD card) - a display is present and may contend for bus/DMA resources", mount_path); - auto* ids = static_cast*>(context); - ids->push_back(file_mutex_add(&lvgl_mutex, mount_path)); - return true; - }); -} - -void deinitFileMutexForLvgl() { - for (FileMutexId id : registered_ids) { - file_mutex_remove(id); - } - registered_ids.clear(); -} - -} diff --git a/Tactility/Source/lvgl/LabelUtils.cpp b/Tactility/Source/lvgl/LabelUtils.cpp index ccd8a862..28a5845a 100644 --- a/Tactility/Source/lvgl/LabelUtils.cpp +++ b/Tactility/Source/lvgl/LabelUtils.cpp @@ -4,12 +4,7 @@ namespace tt::lvgl { bool label_set_text_file(lv_obj_t* label, const char* filepath) { - std::unique_ptr text; - { - file::FileMutexGuard guard(filepath); - text = file::readString(filepath); - } - + std::unique_ptr text = file::readString(filepath); if (text != nullptr) { lv_label_set_text(label, reinterpret_cast(text.get())); return true; diff --git a/Tactility/Source/network/Http.cpp b/Tactility/Source/network/Http.cpp index c5e3025b..8431dd7a 100644 --- a/Tactility/Source/network/Http.cpp +++ b/Tactility/Source/network/Http.cpp @@ -67,8 +67,7 @@ void download( auto bytes_left = client->getContentLength(); - file::FileMutexGuard guard(downloadFilePath); - LOG_I(TAG, "opening %s", downloadFilePath.c_str()); + LOG_I(TAG, "Opening %s", downloadFilePath.c_str()); auto* file = fopen(downloadFilePath.c_str(), "wb"); if (file == nullptr) { onError("Failed to open file"); diff --git a/Tactility/Source/network/HttpdReq.cpp b/Tactility/Source/network/HttpdReq.cpp index 3569480f..dfcf4734 100644 --- a/Tactility/Source/network/HttpdReq.cpp +++ b/Tactility/Source/network/HttpdReq.cpp @@ -2,7 +2,6 @@ #include #include -#include #include #include @@ -186,16 +185,7 @@ size_t receiveFile(httpd_req_t* request, size_t length, const std::string& fileP char buffer[BUFFER_SIZE]; size_t bytes_received = 0; - // Locked only around each actual disk I/O call below, not across the httpd_req_recv() waits - // in between - this file's mutex may resolve to lvgl_lock() (see FileMutexLvgl.cpp), and - // holding that for the whole (potentially multi-second) network transfer starves LVGL's own - // task for the entire upload instead of just for each brief write. - FileMutex mutex {}; - file_mutex_get(&mutex, filePath.c_str()); - - file_mutex_lock(&mutex); auto* file = fopen(filePath.c_str(), "wb"); - file_mutex_unlock(&mutex); if (file == nullptr) { LOG_E(TAG, "Failed to open file for writing: %s", filePath.c_str()); return 0; @@ -226,19 +216,14 @@ size_t receiveFile(httpd_req_t* request, size_t length, const std::string& fileP timeout_retries = 0; size_t receive_chunk_size = (size_t)received; - file_mutex_lock(&mutex); - bool write_ok = fwrite(buffer, 1, receive_chunk_size, file) == receive_chunk_size; - file_mutex_unlock(&mutex); - if (!write_ok) { + if (fwrite(buffer, 1, receive_chunk_size, file) != receive_chunk_size) { LOG_E(TAG, "Failed to write all bytes"); break; } bytes_received += receive_chunk_size; } - file_mutex_lock(&mutex); fclose(file); - file_mutex_unlock(&mutex); return bytes_received; } diff --git a/Tactility/Source/service/webserver/AssetVersion.cpp b/Tactility/Source/service/webserver/AssetVersion.cpp index fc94677c..55300826 100644 --- a/Tactility/Source/service/webserver/AssetVersion.cpp +++ b/Tactility/Source/service/webserver/AssetVersion.cpp @@ -29,32 +29,28 @@ static bool loadVersionFromFile(const char* path, AssetVersion& version) { // Read file content std::string content; - { - file::FileMutexGuard guard(path); - - FILE* fp = fopen(path, "r"); - if (!fp) { - LOG_E(TAG, "Failed to open version file: %s", path); - return false; - } - - char buffer[256]; - size_t bytesRead = fread(buffer, 1, sizeof(buffer) - 1, fp); - bool readError = ferror(fp) != 0; - fclose(fp); - - if (readError) { - LOG_E(TAG, "Error reading version file: %s", path); - return false; - } - if (bytesRead == 0) { - LOG_E(TAG, "Version file is empty: %s", path); - return false; - } - buffer[bytesRead] = '\0'; - content = buffer; + FILE* fp = fopen(path, "r"); + if (!fp) { + LOG_E(TAG, "Failed to open version file: %s", path); + return false; } + char buffer[256]; + size_t bytesRead = fread(buffer, 1, sizeof(buffer) - 1, fp); + bool readError = ferror(fp) != 0; + fclose(fp); + + if (readError) { + LOG_E(TAG, "Error reading version file: %s", path); + return false; + } + if (bytesRead == 0) { + LOG_E(TAG, "Version file is empty: %s", path); + return false; + } + buffer[bytesRead] = '\0'; + content = buffer; + // Parse JSON cJSON* json = cJSON_Parse(content.c_str()); if (json == nullptr) { @@ -113,30 +109,26 @@ static bool saveVersionToFile(const char* path, const AssetVersion& version) { // Write to file bool success = false; - { - file::FileMutexGuard guard(path); - - FILE* fp = fopen(path, "w"); - if (fp) { - size_t len = strlen(jsonString); - size_t written = fwrite(jsonString, 1, len, fp); - success = (written == len); - if (success) { - if (fflush(fp) != 0) { - LOG_E(TAG, "Failed to flush version file: %s", path); + FILE* fp = fopen(path, "w"); + if (fp) { + size_t len = strlen(jsonString); + size_t written = fwrite(jsonString, 1, len, fp); + success = (written == len); + if (success) { + if (fflush(fp) != 0) { + LOG_E(TAG, "Failed to flush version file: %s", path); + success = false; + } else { + int fd = fileno(fp); + if (fd >= 0 && fsync(fd) != 0) { + LOG_E(TAG, "Failed to fsync version file: %s", path); success = false; - } else { - int fd = fileno(fp); - if (fd >= 0 && fsync(fd) != 0) { - LOG_E(TAG, "Failed to fsync version file: %s", path); - success = false; - } } } - fclose(fp); } + fclose(fp); } - + cJSON_free(jsonString); cJSON_Delete(json); diff --git a/Tactility/Source/service/webserver/WebServerService.cpp b/Tactility/Source/service/webserver/WebServerService.cpp index 69ab1c77..e3af7ca3 100644 --- a/Tactility/Source/service/webserver/WebServerService.cpp +++ b/Tactility/Source/service/webserver/WebServerService.cpp @@ -1700,8 +1700,6 @@ esp_err_t WebServerService::handleAssets(httpd_req_t* request) { httpd_resp_set_type(request, "image/png"); httpd_resp_set_hdr(request, "Cache-Control", "public, max-age=86400"); - file::FileMutexGuard guard(faviconPath); - FILE* fp = fopen(faviconPath, "rb"); if (fp) { char buffer[512]; @@ -1743,9 +1741,6 @@ esp_err_t WebServerService::handleAssets(httpd_req_t* request) { // Try to serve from Data partition first if (file::isFile(dataPath.c_str())) { httpd_resp_set_type(request, getContentType(dataPath)); - - // Read and send file using standard C FILE* operations - file::FileMutexGuard guard(dataPath); FILE* fp = fopen(dataPath.c_str(), "rb"); if (fp) { @@ -1769,8 +1764,6 @@ esp_err_t WebServerService::handleAssets(httpd_req_t* request) { std::string sdPath = std::string("/sdcard/tactility/webserver") + requestedPath; if (file::isFile(sdPath.c_str())) { httpd_resp_set_type(request, getContentType(sdPath)); - - file::FileMutexGuard guard(sdPath); FILE* fp = fopen(sdPath.c_str(), "rb"); if (fp) { diff --git a/TactilityKernel/include/tactility/drivers/spi_controller.h b/TactilityKernel/include/tactility/drivers/spi_controller.h index 11c4d0d8..d5a1bd1f 100644 --- a/TactilityKernel/include/tactility/drivers/spi_controller.h +++ b/TactilityKernel/include/tactility/drivers/spi_controller.h @@ -65,6 +65,19 @@ error_t spi_controller_try_lock(struct Device* device, TickType_t timeout); */ error_t spi_controller_unlock(struct Device* device); +/** + * @brief Locks device's parent bus when the parent is a SPI controller. + * @param[in] device the device whose parent may be a SPI controller + * @retval ERROR_NONE when the operation was successful, or the parent is not a SPI controller + */ +error_t spi_controller_lock_bus_of(struct Device* device); + +/** + * @brief Unlocks a device's parent bus. + * @param[in] device the device whose parent may be a SPI controller + */ +void spi_controller_unlock_bus_of(struct Device* device); + extern const struct DeviceType SPI_CONTROLLER_TYPE; #ifdef __cplusplus diff --git a/TactilityKernel/include/tactility/filesystem/file_mutex.h b/TactilityKernel/include/tactility/filesystem/file_mutex.h deleted file mode 100644 index c0a165b9..00000000 --- a/TactilityKernel/include/tactility/filesystem/file_mutex.h +++ /dev/null @@ -1,63 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -#pragma once - -#include - -#include - -#ifdef __cplusplus -extern "C" { -#endif - -/** - * @brief Set of lock/try_lock/unlock callbacks backing a filesystem mount's mutex. - * Any field left null is treated as a no-op by file_mutex_lock/try_lock/unlock. - */ -struct FileMutex { - void (*lock)(); - bool (*try_lock)(uint32_t timeout); - void (*unlock)(); -}; - -typedef uint32_t FileMutexId; - -#define FILE_MUTEX_ID_INVALID ((FileMutexId)0) - -/** - * @brief Registers a mutex for a mount path (e.g. "/sdcard") and its descendants. - * @param[in] mutex callbacks to associate with the path; a copy is stored - * @param[in] path mount path this mutex serializes access to - * @return the id of the new entry, or the id of the existing entry if path is already registered - * @note If a mutex is already registered for this exact path, no new entry is created and the - * existing entry's id is returned; the existing callbacks are left unchanged. - */ -FileMutexId file_mutex_add(const struct FileMutex* mutex, const char* path); - -/** - * @brief Removes a previously added mutex registration. - * @param[in] id id returned by file_mutex_add(); a stale or unknown id is a no-op - */ -void file_mutex_remove(FileMutexId id); - -/** - * @brief Looks up the mutex registered for path or one of its ancestor mount paths. - * @param[out] mutex receives the matching mutex, or an all-null (no-op) mutex if none matches - * @param[in] path file or directory path to look up - */ -void file_mutex_get(struct FileMutex* mutex, const char* path); - -/** @brief Locks mutex. No-op if mutex->lock is null. */ -void file_mutex_lock(const struct FileMutex* mutex); - -/** - * @brief Attempts to lock mutex within timeout. - * @return true if locked (or mutex->try_lock is null), false on timeout - */ -bool file_mutex_try_lock(const struct FileMutex* mutex, TickType_t timeout); - -/** @brief Unlocks mutex. No-op if mutex->unlock is null. */ -void file_mutex_unlock(const struct FileMutex* mutex); - -#ifdef __cplusplus -} -#endif diff --git a/TactilityKernel/include/tactility/properties_file.h b/TactilityKernel/include/tactility/properties_file.h index 3c65ebc9..f96c06db 100644 --- a/TactilityKernel/include/tactility/properties_file.h +++ b/TactilityKernel/include/tactility/properties_file.h @@ -2,8 +2,6 @@ /** * @brief Generic string key-value ".properties" file. - * @note Safely acquires/releases the filesystem mutex registered for the file's path (see - * tactility/filesystem/file_mutex.h) - manual locking isn't needed. */ #pragma once diff --git a/TactilityKernel/source/drivers/spi_controller.cpp b/TactilityKernel/source/drivers/spi_controller.cpp index dde7b06e..a095816c 100644 --- a/TactilityKernel/source/drivers/spi_controller.cpp +++ b/TactilityKernel/source/drivers/spi_controller.cpp @@ -22,6 +22,22 @@ error_t spi_controller_unlock(Device* device) { return SPI_DRIVER_API(driver)->unlock(device); } +error_t spi_controller_lock_bus_of(Device* device) { + Device* parent = device_get_parent(device); + if (parent == nullptr || device_get_type(parent) != &SPI_CONTROLLER_TYPE) { + return ERROR_NONE; + } + return spi_controller_lock(parent); +} + +void spi_controller_unlock_bus_of(Device* device) { + Device* parent = device_get_parent(device); + if (parent == nullptr || device_get_type(parent) != &SPI_CONTROLLER_TYPE) { + return; + } + spi_controller_unlock(parent); +} + const DeviceType SPI_CONTROLLER_TYPE { .name = "spi-controller" }; diff --git a/TactilityKernel/source/filesystem/file_mutex.cpp b/TactilityKernel/source/filesystem/file_mutex.cpp deleted file mode 100644 index 72f648d1..00000000 --- a/TactilityKernel/source/filesystem/file_mutex.cpp +++ /dev/null @@ -1,123 +0,0 @@ -// SPDX-License-Identifier: Apache-2.0 -#include -#include - -#include -#include -#include -#include - -static const FileMutex no_mutex = { - .lock = nullptr, - .try_lock = nullptr, - .unlock = nullptr, -}; - -struct FileMutexEntry { - FileMutexId id; - std::string path; - FileMutex mutex; -}; - -// Guards mutex_entries against concurrent add/get/remove; unrelated to whether a FileMutex's own -// lock/unlock is currently held (file_mutex_get() hands out a copy that stays valid regardless of -// later registry changes - see file_mutex_remove()). -struct FileMutexLedger { - std::vector entries; - FileMutexId next_id = 1; - Mutex mutex {}; - - FileMutexLedger() { mutex_construct(&mutex); } - ~FileMutexLedger() { mutex_destruct(&mutex); } - - void lock() { mutex_lock(&mutex); } - void unlock() { mutex_unlock(&mutex); } -}; - -static FileMutexLedger& get_ledger() { - static FileMutexLedger ledger; - return ledger; -} - -extern "C" { - -FileMutexId file_mutex_add(const FileMutex* mutex, const char* path) { - auto& ledger = get_ledger(); - ledger.lock(); - - for (auto& entry : ledger.entries) { - if (entry.path == path) { - FileMutexId existing_id = entry.id; - ledger.unlock(); - return existing_id; - } - } - - FileMutexId new_id = ledger.next_id++; - ledger.entries.push_back({ - .id = new_id, - .path = path, - .mutex = *mutex - }); - - ledger.unlock(); - return new_id; -} - -void file_mutex_remove(FileMutexId id) { - auto& ledger = get_ledger(); - ledger.lock(); - - const auto iterator = std::ranges::find_if(ledger.entries, [id](const FileMutexEntry& entry) { - return entry.id == id; - }); - if (iterator != ledger.entries.end()) { - // Plain erase, not swap-and-pop: file_mutex_get() matches first-registered-wins, so - // removal must preserve the relative order of the remaining entries. - ledger.entries.erase(iterator); - } - - ledger.unlock(); -} - -void file_mutex_get(FileMutex* mutex, const char* path) { - auto& ledger = get_ledger(); - std::string path_string = path; - - ledger.lock(); - for (auto& entry : ledger.entries) { - // Match the mount path itself, or a descendant (e.g. "/sdcard" registered, "/sdcard/config.json" requested). - bool is_match = path_string == entry.path || - (entry.path == "/" && !path_string.empty() && path_string[0] == '/') || - (path_string.rfind(entry.path, 0) == 0 && path_string[entry.path.size()] == '/'); - if (is_match) { - memcpy(mutex, &entry.mutex, sizeof(FileMutex)); - ledger.unlock(); - return; - } - } - ledger.unlock(); - - *mutex = no_mutex; -} - -void file_mutex_lock(const FileMutex* mutex) { - if (mutex->lock) { - mutex->lock(); - } -} - -bool file_mutex_try_lock(const FileMutex* mutex, TickType_t timeout) { - if (mutex->try_lock) { - return mutex->try_lock(timeout); - } - return true; -} - -void file_mutex_unlock(const FileMutex* mutex) { - if (mutex->unlock) { - mutex->unlock(); - } -} - -} diff --git a/TactilityKernel/source/properties_file.cpp b/TactilityKernel/source/properties_file.cpp index e736cd0f..a5e95d08 100644 --- a/TactilityKernel/source/properties_file.cpp +++ b/TactilityKernel/source/properties_file.cpp @@ -1,6 +1,5 @@ // SPDX-License-Identifier: Apache-2.0 #include -#include #include #include @@ -53,14 +52,9 @@ namespace { // (fgetc()'s EOF return doesn't by itself distinguish clean end-of-file from a read error - // ferror() after the loop does); true otherwise, including for a missing file (ENOENT). bool load_from_file(PropertiesFile* file) { - FileMutex mutex {}; - file_mutex_get(&mutex, file->path.c_str()); - file_mutex_lock(&mutex); - FILE* handle = std::fopen(file->path.c_str(), "r"); if (handle == nullptr) { const int open_error = errno; - file_mutex_unlock(&mutex); if (open_error == ENOENT) { return true; } @@ -105,7 +99,6 @@ bool load_from_file(PropertiesFile* file) { bool read_ok = std::ferror(handle) == 0; std::fclose(handle); - file_mutex_unlock(&mutex); if (!read_ok) { LOG_E(TAG, "Failed to read %s", file->path.c_str()); @@ -121,16 +114,11 @@ bool load_from_file(PropertiesFile* file) { // @return true if the backing file was fully replaced with the current entries; false (leaving // the previous on-disk content untouched) if any step failed. bool save_to_file(const PropertiesFile* file) { - FileMutex mutex {}; - file_mutex_get(&mutex, file->path.c_str()); - file_mutex_lock(&mutex); - std::string temp_path = file->path + ".tmp"; FILE* handle = std::fopen(temp_path.c_str(), "w"); if (handle == nullptr) { LOG_E(TAG, "Failed to open %s", temp_path.c_str()); - file_mutex_unlock(&mutex); return false; } @@ -146,7 +134,6 @@ bool save_to_file(const PropertiesFile* file) { if (!write_ok || !flush_ok || !close_ok) { LOG_E(TAG, "Failed to write %s", temp_path.c_str()); std::remove(temp_path.c_str()); - file_mutex_unlock(&mutex); return false; } @@ -157,11 +144,9 @@ bool save_to_file(const PropertiesFile* file) { if (std::rename(temp_path.c_str(), file->path.c_str()) != 0) { LOG_E(TAG, "Failed to replace %s", file->path.c_str()); std::remove(temp_path.c_str()); - file_mutex_unlock(&mutex); return false; } - file_mutex_unlock(&mutex); return true; } diff --git a/TactilityKernel/source/symbols.c b/TactilityKernel/source/symbols.c index d478fdc6..7b756f16 100644 --- a/TactilityKernel/source/symbols.c +++ b/TactilityKernel/source/symbols.c @@ -44,7 +44,6 @@ #include #include #include -#include #include #include #include @@ -170,13 +169,6 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = { DEFINE_MODULE_SYMBOL(display_get_frame_buffer_count), DEFINE_MODULE_SYMBOL(display_get_backlight), DEFINE_MODULE_SYMBOL(DISPLAY_TYPE), - // file_mutex - DEFINE_MODULE_SYMBOL(file_mutex_add), - DEFINE_MODULE_SYMBOL(file_mutex_remove), - DEFINE_MODULE_SYMBOL(file_mutex_get), - DEFINE_MODULE_SYMBOL(file_mutex_lock), - DEFINE_MODULE_SYMBOL(file_mutex_try_lock), - DEFINE_MODULE_SYMBOL(file_mutex_unlock), // file system DEFINE_MODULE_SYMBOL(file_system_mount), DEFINE_MODULE_SYMBOL(file_system_unmount), @@ -306,6 +298,8 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = { DEFINE_MODULE_SYMBOL(spi_controller_lock), DEFINE_MODULE_SYMBOL(spi_controller_try_lock), DEFINE_MODULE_SYMBOL(spi_controller_unlock), + DEFINE_MODULE_SYMBOL(spi_controller_lock_bus_of), + DEFINE_MODULE_SYMBOL(spi_controller_unlock_bus_of), DEFINE_MODULE_SYMBOL(SPI_CONTROLLER_TYPE), // drivers/trackball DEFINE_MODULE_SYMBOL(trackball_read_delta), diff --git a/TactilityKernel/tests/source/file_mutex_test.cpp b/TactilityKernel/tests/source/file_mutex_test.cpp deleted file mode 100644 index a67f14c5..00000000 --- a/TactilityKernel/tests/source/file_mutex_test.cpp +++ /dev/null @@ -1,190 +0,0 @@ -#include "doctest.h" -#include - -namespace { - -int lock_calls = 0; -int unlock_calls = 0; -int try_lock_calls = 0; -bool try_lock_result = true; -uint32_t try_lock_timeout_seen = 0; - -void mock_lock() { lock_calls++; } -void mock_unlock() { unlock_calls++; } -bool mock_try_lock(uint32_t timeout) { - try_lock_calls++; - try_lock_timeout_seen = timeout; - return try_lock_result; -} - -int lock_a_calls = 0; -int lock_b_calls = 0; -void mock_lock_a() { lock_a_calls++; } -void mock_lock_b() { lock_b_calls++; } - -void reset_mocks() { - lock_calls = 0; - unlock_calls = 0; - try_lock_calls = 0; - try_lock_result = true; - try_lock_timeout_seen = 0; - lock_a_calls = 0; - lock_b_calls = 0; -} - -} // namespace - -TEST_CASE("file_mutex_get with zero registrations returns a no-op mutex") { - FileMutex mutex; - file_mutex_get(&mutex, "/nowhere/file.txt"); - - CHECK_EQ(mutex.lock, nullptr); - CHECK_EQ(mutex.try_lock, nullptr); - CHECK_EQ(mutex.unlock, nullptr); - - // Calling through a no-op mutex must be safe, and try_lock must report success. - file_mutex_lock(&mutex); - CHECK_EQ(file_mutex_try_lock(&mutex, 123), true); - file_mutex_unlock(&mutex); -} - -TEST_CASE("file_mutex_add/get with a single registration") { - reset_mocks(); - FileMutex registered = { .lock = mock_lock, .try_lock = mock_try_lock, .unlock = mock_unlock }; - file_mutex_add(®istered, "/mock1"); - - FileMutex mutex; - - // Exact mount path match. - file_mutex_get(&mutex, "/mock1"); - CHECK_EQ(mutex.lock, mock_lock); - CHECK_EQ(mutex.try_lock, mock_try_lock); - CHECK_EQ(mutex.unlock, mock_unlock); - - // Descendant path match. - file_mutex_get(&mutex, "/mock1/nested/file.txt"); - CHECK_EQ(mutex.lock, mock_lock); - - // Unrelated path falls back to no-op. - FileMutex unrelated; - file_mutex_get(&unrelated, "/other/file.txt"); - CHECK_EQ(unrelated.lock, nullptr); - - // Prefix-but-not-descendant path (e.g. "/mock1x") must not match "/mock1". - FileMutex prefix_only; - file_mutex_get(&prefix_only, "/mock1x/file.txt"); - CHECK_EQ(prefix_only.lock, nullptr); - - // Exercise the resolved callbacks. - file_mutex_get(&mutex, "/mock1"); - file_mutex_lock(&mutex); - CHECK_EQ(lock_calls, 1); - CHECK_EQ(file_mutex_try_lock(&mutex, 42), true); - CHECK_EQ(try_lock_calls, 1); - CHECK_EQ(try_lock_timeout_seen, 42); - file_mutex_unlock(&mutex); - CHECK_EQ(unlock_calls, 1); - - // Re-registering the same path is a no-op: original callbacks remain in place. - FileMutex replacement = { .lock = nullptr, .try_lock = nullptr, .unlock = nullptr }; - file_mutex_add(&replacement, "/mock1"); - file_mutex_get(&mutex, "/mock1"); - CHECK_EQ(mutex.lock, mock_lock); -} - -TEST_CASE("file_mutex_add/get with two registrations resolves to the matching path") { - reset_mocks(); - - FileMutex mutex_a = { .lock = mock_lock_a, .try_lock = nullptr, .unlock = nullptr }; - FileMutex mutex_b = { .lock = mock_lock_b, .try_lock = nullptr, .unlock = nullptr }; - - file_mutex_add(&mutex_a, "/mock2a"); - file_mutex_add(&mutex_b, "/mock2b"); - - FileMutex resolved; - - file_mutex_get(&resolved, "/mock2a/file.txt"); - CHECK_EQ(resolved.lock, mock_lock_a); - - file_mutex_get(&resolved, "/mock2b/file.txt"); - CHECK_EQ(resolved.lock, mock_lock_b); - - // Path matching neither registration falls back to no-op. - file_mutex_get(&resolved, "/mock2c/file.txt"); - CHECK_EQ(resolved.lock, nullptr); - - // Registration order matters: the first matching entry wins, not the longest - // prefix. A mount nested under an earlier one is shadowed by it. - FileMutex mutex_nested = { .lock = nullptr, .try_lock = nullptr, .unlock = nullptr }; - file_mutex_add(&mutex_nested, "/mock2a/nested"); - file_mutex_get(&resolved, "/mock2a/nested/file.txt"); - CHECK_EQ(resolved.lock, mock_lock_a); // still /mock2a, registered first -} - -TEST_CASE("file_mutex_add returns a valid id") { - reset_mocks(); - FileMutex registered = { .lock = mock_lock, .try_lock = nullptr, .unlock = nullptr }; - FileMutexId id = file_mutex_add(®istered, "/mockA"); - CHECK_NE(id, FILE_MUTEX_ID_INVALID); -} - -TEST_CASE("file_mutex_add with a duplicate path returns the existing id") { - reset_mocks(); - FileMutex mutex_1 = { .lock = mock_lock, .try_lock = nullptr, .unlock = nullptr }; - FileMutex mutex_2 = { .lock = mock_lock_a, .try_lock = nullptr, .unlock = nullptr }; - - FileMutexId id_1 = file_mutex_add(&mutex_1, "/mockB"); - FileMutexId id_2 = file_mutex_add(&mutex_2, "/mockB"); - CHECK_EQ(id_1, id_2); - - FileMutex resolved; - file_mutex_get(&resolved, "/mockB"); - CHECK_EQ(resolved.lock, mock_lock); // first registration's callbacks win, unchanged -} - -TEST_CASE("file_mutex_remove removes a registration") { - reset_mocks(); - FileMutex registered = { .lock = mock_lock, .try_lock = nullptr, .unlock = nullptr }; - FileMutexId id = file_mutex_add(®istered, "/mockC"); - - FileMutex before; - file_mutex_get(&before, "/mockC"); - CHECK_EQ(before.lock, mock_lock); - - file_mutex_remove(id); - - FileMutex after; - file_mutex_get(&after, "/mockC"); - CHECK_EQ(after.lock, nullptr); -} - -TEST_CASE("file_mutex_remove with an unknown id is a safe no-op") { - reset_mocks(); - FileMutex registered = { .lock = mock_lock, .try_lock = nullptr, .unlock = nullptr }; - file_mutex_add(®istered, "/mockD"); - - file_mutex_remove(999999); // never issued - file_mutex_remove(FILE_MUTEX_ID_INVALID); - - FileMutex resolved; - file_mutex_get(&resolved, "/mockD"); - CHECK_EQ(resolved.lock, mock_lock); // untouched -} - -TEST_CASE("file_mutex_remove of one registration leaves others intact") { - reset_mocks(); - FileMutex mutex_a = { .lock = mock_lock_a, .try_lock = nullptr, .unlock = nullptr }; - FileMutex mutex_b = { .lock = mock_lock_b, .try_lock = nullptr, .unlock = nullptr }; - FileMutexId id_a = file_mutex_add(&mutex_a, "/mockE1"); - file_mutex_add(&mutex_b, "/mockE2"); - - file_mutex_remove(id_a); - - FileMutex resolved_a; - file_mutex_get(&resolved_a, "/mockE1"); - CHECK_EQ(resolved_a.lock, nullptr); - - FileMutex resolved_b; - file_mutex_get(&resolved_b, "/mockE2"); - CHECK_EQ(resolved_b.lock, mock_lock_b); -}