port to Waveshare S3 Touch AMOLED 2.06"#525
Conversation
📝 WalkthroughWalkthroughAdds full support for the Waveshare ESP32-S3 Touch AMOLED 2.06, including BSP, display/touch/audio/power drivers and device configs. Refactors ESP32 I2C to master-bus API and extends sensors/power drivers. Introduces Audio and DialogBox apps, RTC time-sync service, launcher charging notifications, statusbar time/NT P dialogs, and enhanced display idle behavior. Updates LVGL config, fonts, and stop logic. Adds build dependencies, sdkconfig, toolchain flags, linker/gdb helpers, and comprehensive flash args/templates. Also adds a minimal Waveshare 1.8 device slice and documentation for LVGL v9 desync fixes. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Tactility/Source/app/timezone/TimeZone.cpp (1)
132-138:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
fgetslength to matchlinebuffer inTimeZone.cpp(lines 132-138)
lineischar line[128], but the loop reads withfgets(line, 96, file), so longer entries are still truncated to 95 chars.💡 Suggested fix
- while (fgets(line, 96, file)) { + while (fgets(line, sizeof(line), file)) {bootloader-prefix/tmp/bootloader-mkdirs.cmake (1)
1-28:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winStop committing generated CMake bootloader build artifacts (remove and ignore
bootloader-prefix/)
bootloader-prefix/tmp/bootloader-mkdirs.cmakeis tracked in git and contains developer-specific absolute paths (/Users/oneillchampagnie/...), so it should not be committed.- Also tracked:
bootloader-prefix/tmp/bootloader-cfgcmd.txtandbootloader-prefix/src/bootloader-stamp/{bootloader-patch-info.txt,bootloader-source_dirinfo.txt,bootloader-update-info.txt}.- Add
.gitignorerules forbootloader-prefix/(at leastbootloader-prefix/tmp/andbootloader-prefix/src/bootloader-stamp/) to prevent these regenerated files from being reintroduced.
🟠 Major comments (31)
Devices/waveshare-s3-touch-amoled-18/Source/devices/Display.cpp-64-85 (1)
64-85:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake display lifecycle symmetric and idempotent.
Resources are created in
start()/startLvgl()butstop()andstopLvgl()currently do nothing. Re-entering startup can leave stale LVGL registrations and repeat side effects (e.g., SD mount), which is a reliability risk.Also applies to: 122-139, 146-150, 155-157
Devices/waveshare-s3-touch-amoled-18/Source/devices/InternalFlash.cpp.disabled-6-7 (1)
6-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResolve linkage mismatch for
mounted_wl_handle/is_mountedin InternalFlash.
InternalFlash.h.disableddeclaresextern wl_handle_t mounted_wl_handle;andextern bool is_mounted;, butInternalFlash.cpp.disableddefines them asstatic, which is inconsistent linkage and will fail once the file is enabled (and can break any TUs including the header).Suggested fix
-static wl_handle_t mounted_wl_handle = WL_INVALID_HANDLE; -static bool is_mounted = false; +wl_handle_t mounted_wl_handle = WL_INVALID_HANDLE; +bool is_mounted = false;Devices/waveshare-s3-touch-amoled-206/Source/devices/AudioDevice.cpp-54-95 (1)
54-95:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd rollback in
initI2S()failure paths.Partial init failures currently return early without cleaning up created/enabled channels/interface, leaving leaked state for later retries.
Devices/waveshare-s3-touch-amoled-206/Source/devices/Display.cpp-150-155 (1)
150-155:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
stop()doesn’t release hardware resources.
panelHandle/ioHandleare kept alive and never nulled or deinitialized, so repeated start/stop cycles can leak resources and leave stale handles.Devices/waveshare-s3-touch-amoled-206/Source/devices/AudioDevice.cpp-343-355 (1)
343-355:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback speaker open if microphone open fails.
If Line 351 fails, speaker remains open and the function returns false with a partial audio state.
Proposed fix
if (micHandle) { esp_err_t err = esp_codec_dev_open(micHandle, &DEFAULT_FS); if (err != ESP_OK) { LOG_E(TAG, "Failed to open microphone: %d", err); + if (spkHandle) { + esp_codec_dev_close(spkHandle); + } return false; } }Devices/waveshare-s3-touch-amoled-206/CMakeLists.txt-1-3 (1)
1-3:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
SOURCE_FILESgets overwritten, dropping.csources.Line 3 replaces the result from Line 1, so any C files are excluded from
SRCS. Keep a single glob (or append) so both C and C++ files are compiled.Proposed fix
-file(GLOB_RECURSE SOURCE_FILES Source/*.c*) - -file(GLOB_RECURSE SOURCE_FILES "Source/*.cpp") +file(GLOB_RECURSE SOURCE_FILES + "Source/*.c" + "Source/*.cc" + "Source/*.cxx" + "Source/*.cpp" +)Devices/waveshare-s3-touch-amoled-206/Source/Configuration.cpp-165-177 (1)
165-177:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle
initAudioCodecsfailure before registering devices.If codec init fails, speaker/microphone objects may still be pushed as valid handles but unusable runtime devices. Gate registration on successful initialization.
Proposed fix
- if (speaker || microphone) { - waveshare::audio::initAudioCodecs(speaker, microphone); - } + bool audioReady = true; + if (speaker || microphone) { + audioReady = waveshare::audio::initAudioCodecs(speaker, microphone); + } - if (speaker && speaker->isValid()) { + if (audioReady && speaker && speaker->isValid()) { speaker->setVolume(audioSettings.volume); speaker->setMuted(audioSettings.muted); devices.push_back(speaker); } - if (microphone && microphone->isValid()) { + if (audioReady && microphone && microphone->isValid()) { devices.push_back(microphone); }Devices/waveshare-s3-touch-amoled-206/Source/devices/Display.cpp-118-145 (1)
118-145:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
lvgl_port_remove_dispinstead oflv_display_deleteforlvgl_port_add_dispdisplays
InDevices/waveshare-s3-touch-amoled-206/Source/devices/Display.cpp(Co5300Display::stopLvgl()),lvglDisplayis created vialvgl_port_add_disp(&disp_cfg), but it’s destroyed withlv_display_delete(lvglDisplay). Switch tolvgl_port_remove_disp(lvglDisplay)(then setlvglDisplay = nullptr) so the esp_lvgl_port layer can do its port-level cleanup; other displays in this repo already follow this pattern.Devices/waveshare-s3-touch-amoled-206/Source/Configuration.cpp-127-132 (1)
127-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix ES7210 I2C probe:
0x80is invalid for ESP-IDF 7-bit addressing
ESP-IDF I2C master in 7-bit mode expectsdevice_addressto be the 7-bit value0x00–0x7F(no R/W bit).0x80is outside that range, so probing at0x80cannot succeed. Update the probe argument to the ES7210’s correct 7-bit slave address (based on its AD1/AD0 configuration) instead of0x80.Devices/waveshare-s3-touch-amoled-206/Source/devices/Touch.cpp-142-149 (1)
142-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDelete
touchHandlebeforeioHandleinCo5300Touch::stop()
touchHandleis created withesp_lcd_touch_new_i2c_ft5x06(ioHandle, ...), butstop()currently callsesp_lcd_panel_io_del(ioHandle)beforeesp_lcd_touch_del(touchHandle)—swap the order to avoid touch teardown using freed/deinitialized IO resources.Proposed fix
- if (ioHandle != nullptr) { - esp_lcd_panel_io_del(ioHandle); - ioHandle = nullptr; - } - if (touchHandle != nullptr) { esp_lcd_touch_del(touchHandle); touchHandle = nullptr; } + + if (ioHandle != nullptr) { + esp_lcd_panel_io_del(ioHandle); + ioHandle = nullptr; + }Devices/waveshare-s3-touch-amoled-206/Source/devices/Touch.cpp-176-193 (1)
176-193:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
lvgl_port_remove_touch()inCo5300Touch::stopLvgl()for correct LVGL touch teardown/ownership
stopLvgl()manually unregisters the interrupt callback,free(lvglTouchCtx), andlv_indev_delete(lvglTouchIndev). That duplicatesesp_lvgl_port’s internallvgl_port_remove_touch()cleanup (including freeing the touch context obtained vialv_indev_get_driver_data()), so it can break with allocator/ownership changes.
Diff (teardown via port API):
Proposed fix
bool Co5300Touch::stopLvgl() { if (lvglTouchIndev == nullptr) { return false; } - - // Unregister interrupt callback (same as lvgl_port_remove_touch internals) - if (touchHandle != nullptr && touchHandle->config.int_gpio_num != GPIO_NUM_NC) { - esp_lcd_touch_register_interrupt_callback(touchHandle, NULL); - } - - // Free allocated touch context - if (lvglTouchCtx != nullptr) { - free(lvglTouchCtx); - lvglTouchCtx = nullptr; - } - - lv_indev_delete(lvglTouchIndev); + lvgl_port_remove_touch(lvglTouchIndev); lvglTouchIndev = nullptr; + lvglTouchCtx = nullptr; return true; }In
Co5300Touch::stop(),esp_lcd_panel_io_del(ioHandle)runs beforeesp_lcd_touch_del(touchHandle). Prefer deleting the touch handle first when both share the same panel IO transport.Drivers/AXP2101/Source/Axp2101.cpp-42-42 (1)
42-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCharging enable write is inverted.
isChargingEnabled()treats bit0b10as enabled, but Line 42 writes0x00whenenabled == true. This flips behavior for callers likesetAllowedToCharge(true).Proposed fix
bool Axp2101::setChargingEnabled(bool enabled) const { - return writeRegister8(0x18, enabled ? 0x00 : 0x02); + return writeRegister8(0x18, enabled ? 0x02 : 0x00); }Drivers/AXP2101/Source/Axp2101Power.cpp-36-43 (1)
36-43:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t trust
100%when the sanity-check read fails.If
getBatteryVoltage()fails, the current logic still returns100%. That bypasses the fallback estimate and can overreport charge.Proposed fix
if (percentage == 100) { float vbatMillis = 0.0f; - if (axpDevice->getBatteryVoltage(vbatMillis) && vbatMillis < 4150.0f) { - // fall through to voltage calculation below - } else { + if (axpDevice->getBatteryVoltage(vbatMillis)) { + if (vbatMillis >= 4150.0f) { + data.valueAsUint8 = percentage; + return true; + } + // fall through to voltage calculation below + } else { + // fall through to voltage calculation below + } + } else { data.valueAsUint8 = percentage; return true; - } - } else { - data.valueAsUint8 = percentage; - return true; }Modules/lvgl-module/source/arch/lvgl_esp32.c-55-63 (1)
55-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRecovery path leaves
initializedin inconsistent state.If
lvgl_port_deinit()fails andon_start()is called to recover,initializedremainsfalse. This means subsequentlvgl_lock()calls will return fake locks even though LVGL has been restarted, potentially causing concurrent access issues.🐛 Proposed fix
// Set initialized to false BEFORE deinit to prevent concurrent // lock calls from accessing the mutex after it's destroyed. initialized = false; if (lvgl_port_deinit() != ESP_OK) { // Call on_start again to recover - if (lvgl_module_config.on_start) lvgl_module_config.on_start(); + if (lvgl_module_config.on_start) { + lvgl_module_config.on_start(); + initialized = true; // Restore state after recovery + } return ERROR_RESOURCE; }Tactility/Source/PartitionsEsp.cpp-151-170 (1)
151-170:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate partition mount/registration failures instead of always returning success.
Line 169 always returns
ESP_OK, even when mount fails (Lines 152/161) or filesystem registration fails (Lines 157/166). That can leave startup in a partial FS state while reporting success.Suggested fix
- err = esp_vfs_fat_spiflash_mount_rw_wl("/system", "system", &mount_config, &system_wl_handle); - if (err != ESP_OK) { + esp_err_t system_err = esp_vfs_fat_spiflash_mount_rw_wl("/system", "system", &mount_config, &system_wl_handle); + if (system_err != ESP_OK) { LOGGER.error("Failed to mount system partition: %s", esp_err_to_name(err)); } else { LOGGER.info("System partition mounted at /system"); - system_mounted = true; system_fs = file_system_add(&system_fs_api, nullptr); + system_mounted = (system_fs != nullptr); + if (!system_mounted) { + LOGGER.error("Failed to register system filesystem"); + system_err = ESP_FAIL; + } } - err = esp_vfs_fat_spiflash_mount_rw_wl("/data", "data", &mount_config, &data_wl_handle); - if (err != ESP_OK) { + esp_err_t data_err = esp_vfs_fat_spiflash_mount_rw_wl("/data", "data", &mount_config, &data_wl_handle); + if (data_err != ESP_OK) { LOGGER.error("Failed to mount data partition: %s", esp_err_to_name(err)); } else { LOGGER.info("Data partition mounted at /data"); - data_mounted = true; data_fs = file_system_add(&data_fs_api, nullptr); + data_mounted = (data_fs != nullptr); + if (!data_mounted) { + LOGGER.error("Failed to register data filesystem"); + data_err = ESP_FAIL; + } } - return ESP_OK; + return (system_err == ESP_OK && data_err == ESP_OK) ? ESP_OK : ESP_FAIL;Tactility/Source/TactilityEsp.cpp-30-31 (1)
30-31:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap
esp_netif_init()withESP_ERROR_CHECK()to handle init failures.
Tactility/Source/TactilityEsp.cpp:31callsesp_netif_init()without checking its returnedesp_err_t, so network init failures can be lost and surface later. Replace withESP_ERROR_CHECK(esp_netif_init());.Suggested fix
ESP_ERROR_CHECK(esp_event_loop_create_default()); - esp_netif_init(); + ESP_ERROR_CHECK(esp_netif_init());Tactility/Source/network/Ntp.cpp-27-28 (1)
27-28:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThread resource leak on shutdown.
rtcSyncThreadis allocated and started but never joined or freed. When the module is deinitialized or on restart, this leaks resources. Add a cleanup function to stop and free the thread.Proposed approach
Add a
deinit()function or ensure cleanup in an existing shutdown path:void deinit() { stopRtcSync = true; if (rtcSyncThread) { thread_join(rtcSyncThread); thread_free(rtcSyncThread); rtcSyncThread = nullptr; } }Also applies to: 120-132
Tactility/Source/hal/sdcard/SpiSdCardDevice.cpp-52-75 (1)
52-75:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftHardcoded GPIO pins reduce reusability.
The SPI1 bus initialization hardcodes GPIO pins (MOSI=1, MISO=3, SCLK=2), but this is a generic SD card device class. These values should come from the configuration struct, similar to how other SPI properties are configured.
Proposed approach
Add GPIO pin fields to the configuration struct and use them here:
if (config->spiHost == SPI1_HOST) { spi_bus_config_t buscfg = { - .mosi_io_num = GPIO_NUM_1, - .miso_io_num = GPIO_NUM_3, - .sclk_io_num = GPIO_NUM_2, + .mosi_io_num = config->spiPinMosi, + .miso_io_num = config->spiPinMiso, + .sclk_io_num = config->spiPinSclk, .quadwp_io_num = -1, .quadhd_io_num = -1, .max_transfer_sz = 4092, .flags = 0, .intr_flags = 0 };bootloader-prefix/src/bootloader-stamp/bootloader-source_dirinfo.txt-7-7 (1)
7-7:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove hardcoded user-specific absolute path.
Line 7 contains a hardcoded absolute path to a specific user's home directory (
/Users/oneillchampagnie/...). This build artifact should not be committed to version control as it:
- Will not work for other developers with different usernames or directory structures
- Exposes developer-specific environment details
- Is typically regenerated during build
Add
bootloader-prefix/to.gitignoreto prevent committing machine-specific build artifacts.bootloader-prefix/tmp/bootloader-cfgcmd.txt-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove build artifact with multiple hardcoded paths.
This file contains a CMake command string with numerous hardcoded absolute paths specific to a developer's machine (multiple
/Users/oneillchampagnie/...references). This is a generated build artifact that:
- Must be regenerated for each developer's environment
- Exposes environment-specific details unnecessarily
- Should be excluded via
.gitignore(addbootloader-prefix/pattern)Same root cause as the issue in
bootloader-source_dirinfo.txt.components/waveshare__esp32_s3_touch_amoled_2_06/esp32_s3_touch_amoled_2_06.c-338-341 (1)
338-341:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate brightness init errors instead of always returning
ESP_OK.Line 340 currently discards failure from
bsp_display_brightness_set(100).Suggested fix
esp_err_t bsp_display_brightness_init(void) { - bsp_display_brightness_set(100); - return ESP_OK; + return bsp_display_brightness_set(100); }components/waveshare__esp32_s3_touch_amoled_2_06/esp32_s3_touch_amoled_2_06.c-365-365 (1)
365-365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCheck DCS brightness write result.
esp_lcd_panel_io_tx_param(...)is unchecked, so I/O failures are silently reported as success.Suggested fix
- esp_lcd_panel_io_tx_param(io_handle, lcd_cmd, ¶m, 1); - - return ESP_OK; + return esp_lcd_panel_io_tx_param(io_handle, lcd_cmd, ¶m, 1);components/waveshare__esp32_s3_touch_amoled_2_06/esp32_s3_touch_amoled_2_06.c-236-244 (1)
236-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear global I2S handles on init failure.
After
i2s_del_channel(...), the globals remain non-null. A retry can falsely pass theif (i2s_tx_chan && i2s_rx_chan)guard and leave audio in an invalid state.Suggested fix
err: if (i2s_tx_chan) { i2s_del_channel(i2s_tx_chan); + i2s_tx_chan = NULL; } if (i2s_rx_chan) { i2s_del_channel(i2s_rx_chan); + i2s_rx_chan = NULL; } + i2s_data_if = NULL; return ret;components/waveshare__esp32_s3_touch_amoled_2_06/esp32_s3_touch_amoled_2_06.c-426-470 (1)
426-470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop aborting in
bsp_display_new; propagateesp_err_tfailures instead (major)
bsp_display_new()returnsesp_err_tand callers handlebsp_display_new(...)’s return value, but it currently usesESP_ERROR_CHECK(...)(e.g.,spi_bus_initialize,esp_lcd_new_panel_io_spi,esp_lcd_new_panel_sh8601), which aborts the application instead of returning the error.esp_lcd_panel_reset/init/set_gap/disp_on_offalso returnesp_err_t, but their results are ignored.Suggested fix
- ESP_ERROR_CHECK(spi_bus_initialize(BSP_LCD_SPI_NUM, &buscfg, SPI_DMA_CH_AUTO)); + ESP_RETURN_ON_ERROR(spi_bus_initialize(BSP_LCD_SPI_NUM, &buscfg, SPI_DMA_CH_AUTO), TAG, "spi_bus_initialize failed"); @@ - ESP_ERROR_CHECK(esp_lcd_new_panel_io_spi((esp_lcd_spi_bus_handle_t)BSP_LCD_SPI_NUM, &io_config, &io_handle)); + ESP_RETURN_ON_ERROR(esp_lcd_new_panel_io_spi((esp_lcd_spi_bus_handle_t)BSP_LCD_SPI_NUM, &io_config, &io_handle), TAG, "new_panel_io_spi failed"); @@ - ESP_ERROR_CHECK(esp_lcd_new_panel_sh8601(io_handle, &panel_config, &panel_handle)); - esp_lcd_panel_reset(panel_handle); - esp_lcd_panel_init(panel_handle); - esp_lcd_panel_set_gap(panel_handle, 0x16, 0); - esp_lcd_panel_disp_on_off(panel_handle, true); + ESP_RETURN_ON_ERROR(esp_lcd_new_panel_sh8601(io_handle, &panel_config, &panel_handle), TAG, "new_panel_sh8601 failed"); + ESP_RETURN_ON_ERROR(esp_lcd_panel_reset(panel_handle), TAG, "panel_reset failed"); + ESP_RETURN_ON_ERROR(esp_lcd_panel_init(panel_handle), TAG, "panel_init failed"); + ESP_RETURN_ON_ERROR(esp_lcd_panel_set_gap(panel_handle, 0x16, 0), TAG, "panel_set_gap failed"); + ESP_RETURN_ON_ERROR(esp_lcd_panel_disp_on_off(panel_handle, true), TAG, "panel_disp_on_off failed");config.env-2-11 (1)
2-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove machine-specific paths and user-identifying data from committed config.
Lines 2-11 embed absolute local paths (for example under
/Users/oneillchampagnie/...). This leaks workstation-specific info and makes the file non-portable in CI/other dev environments. Please keep this file generated locally (or templated with env vars) and out of source control.esp-idf/Tactility/data-flash_args.in-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid hardcoding flash mode/frequency/size in shared flash-args templates.
esp-idf/Tactility/data-flash_args.in:1,esp-idf/Tactility/system-flash_args.in:1, andesp-idf/bootloader/bootloader-flash_args.in:1all hardcode the same values:--flash_mode dout --flash_freq 80m --flash_size 32MBMeanwhile
Devices/**/device.propertiesdeclares varyingflashSizeandesptoolFlashFreqacross devices and even includes differentflashModevalues (e.g.,flashMode=DIO), so these shared template defaults can diverge from device-specific settings and lead to incorrect flashing parameters for boards that don’t match the hardcoded profile.esp-idf/esptool_py/app-flash_args.in-1-1 (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop hardcoding esptool flash parameters in shared esp-idf templates.
Hardcoded
dout/80m/32MBis present not only inesp-idf/esptool_py/app-flash_args.inbut also inesp-idf/esptool_py/flasher_args.json.in(e.g.,write_flash_args/flash_mode|flash_size|flash_freq). Parameterize these values per target instead of embedding fixed strings.Suggested direction
---flash_mode dout --flash_freq 80m --flash_size 32MB +--flash_mode `@ESPTOOLPY_FLASHMODE`@ --flash_freq `@ESPTOOLPY_FLASHFREQ`@ --flash_size `@ESPTOOLPY_FLASHSIZE`@gdbinit/symbols-19-20 (1)
19-20:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded absolute path with relative path.
Line 20 contains a hardcoded absolute path that breaks portability for other developers.
🔧 Proposed fix using relative path
# Load application symbols -file /Users/oneillchampagnie/Tactility/Tactility.elf +file Tactility.elfGDB will resolve this relative to the working directory (typically the project root).
gdbinit/symbols-14-17 (1)
14-17:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded absolute path with relative path.
Line 16 contains a hardcoded absolute path that breaks portability for other developers.
🔧 Proposed fix using relative path
# Load bootloader symbols set confirm off - add-symbol-file /Users/oneillchampagnie/Tactility/bootloader/bootloader.elf + add-symbol-file bootloader/bootloader.elf set confirm onGDB will resolve this relative to the working directory (typically the project root).
gdbinit/gdbinit-1-2 (1)
1-2:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded absolute paths with relative paths.
The hardcoded absolute path
/Users/oneillchampagnie/Tactility/prevents other developers from using these GDB scripts and breaks portability.🔧 Proposed fix using relative paths
-source /Users/oneillchampagnie/Tactility/gdbinit/symbols -source /Users/oneillchampagnie/Tactility/gdbinit/connect +source gdbinit/symbols +source gdbinit/connectGDB will resolve these relative to the working directory (typically the project root). Alternatively, if these paths need to be dynamic, consider using environment variables or generating this file as part of the build process.
gdbinit/symbols-2-11 (1)
2-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace hardcoded absolute path with portable solution.
Line 6 contains a hardcoded absolute path
/Users/oneillchampagnie/.espressif/...that prevents other developers from using this debug configuration.🔧 Proposed fix using environment variable
define target hookpost-remote set confirm off # if $_streq((char *) 0x3ff194ad, "Mar 1 2021") if (*(int*) 0x3ff194ad) == 0x2072614d && (*(int*) 0x3ff194b1) == 0x32203120 && (*(int*) 0x3ff194b5) == 0x313230 - add-symbol-file /Users/oneillchampagnie/.espressif/tools/esp-rom-elfs/20241011esp32s3_rev0_rom.elf + python gdb.execute("add-symbol-file " + os.path.expandvars("$HOME/.espressif/tools/esp-rom-elfs/20241011esp32s3_rev0_rom.elf")) else echo Warning: Unknown esp32s3 ROM revision.\n endAlternatively, if IDF_PATH is set, you could reference that environment variable, or generate this file as part of the build process with the correct paths injected.
🟡 Minor comments (13)
Devices/waveshare-s3-touch-amoled-18/CMakeLists.txt-1-3 (1)
1-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix
SOURCE_FILESoverwrite in globbing.Line 3 overwrites the list built at Line 1, so
.csources are excluded fromSRCS. Merge these into one glob assignment.Suggested fix
-file(GLOB_RECURSE SOURCE_FILES Source/*.c*) - -file(GLOB_RECURSE SOURCE_FILES "Source/*.cpp") +file(GLOB_RECURSE SOURCE_FILES + "Source/*.c" + "Source/*.cpp" +)Firmware/idf_component.yml-42-49 (1)
42-49:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPin dependency versions (avoid
version: "*") for the Waveshare touch + CO5300 LCD drivers
Firmware/idf_component.ymlsetsversion: "*"forwaveshare/esp32_s3_touch_amoled_2_06andespressif/esp_lcd_co5300, while other components in the same file use fixed versions.*can change what gets pulled over time and cause unexpected build/runtime issues when upstream releases are updated. Attempts to fetch version metadata via the earlier registry URL pattern return404here, so pick concrete versions based on what you’ve tested (or via the IDF Component Manager).
- Replace
version: "*"with specific version numbers for both components.Tactility/Source/app/audio/Audio.cpp-52-55 (1)
52-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReset the dirty flag across app lifecycle.
audioSettingsUpdatedis never cleared after load/save, so later hides can re-save unchanged settings.💡 Suggested fix
void onShow(AppContext& app, lv_obj_t* parent) override { audioSettings = settings::audio::loadOrGetDefault(); + audioSettingsUpdated = false; auto ui_density = lvgl_get_ui_density(); @@ void onHide(AppContext& app) override { if (audioSettingsUpdated) { const settings::audio::AudioSettings settings_to_save = audioSettings; getMainDispatcher().dispatch([settings_to_save] { settings::audio::save(settings_to_save); }); } + audioSettingsUpdated = false; }Also applies to: 104-110
Tactility/Source/app/dialogbox/DialogBox.cpp-22-27 (1)
22-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGuard timeout narrowing before storing into
Bundle.
Tactility/Source/app/dialogbox/DialogBox.cpp(lines 22-27 and 34-38) castsuint32_t timeoutMstoint32_tforbundle->putInt32(...).DialogBoxApp::onShowreads it back asint32_tand only starts the timer whentimeoutMs > 0, so values >INT32_MAXwrap negative and disable auto-dismiss.Suggested fix
LaunchId start(const std::string& title, const std::string& message, uint32_t timeoutMs) { auto bundle = std::make_shared<Bundle>(); + const auto safeTimeoutMs = static_cast<int32_t>(std::min<uint32_t>(timeoutMs, INT32_MAX)); bundle->putString(PARAMETER_BUNDLE_KEY_TITLE, title); bundle->putString(PARAMETER_BUNDLE_KEY_MESSAGE, message); - bundle->putInt32(PARAMETER_BUNDLE_KEY_TIMEOUT, (int32_t)timeoutMs); + bundle->putInt32(PARAMETER_BUNDLE_KEY_TIMEOUT, safeTimeoutMs); return app::start(manifest.appId, bundle); } @@ LaunchId start(const std::string& message, uint32_t timeoutMs) { auto bundle = std::make_shared<Bundle>(); + const auto safeTimeoutMs = static_cast<int32_t>(std::min<uint32_t>(timeoutMs, INT32_MAX)); bundle->putString(PARAMETER_BUNDLE_KEY_MESSAGE, message); - bundle->putInt32(PARAMETER_BUNDLE_KEY_TIMEOUT, (int32_t)timeoutMs); + bundle->putInt32(PARAMETER_BUNDLE_KEY_TIMEOUT, safeTimeoutMs); return app::start(manifest.appId, bundle); }Tactility/Source/hal/sdcard/SpiSdCardDevice.cpp-67-67 (1)
67-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winIncorrect format specifier for Logger.
The Logger class uses fmt-style formatting with
{}, not printf-style%s.Proposed fix
- LOGGER.error("Failed to init SPI1 bus: %s", esp_err_to_name(ret)); + LOGGER.error("Failed to init SPI1 bus: {}", esp_err_to_name(ret));Tactility/Source/service/rtctime/RtcTimeService.cpp-67-73 (1)
67-73:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
localtime()is not thread-safe.Use
localtime_r()for thread-safe conversion.Proposed fix
time_t now = time(nullptr); - tm* tm_struct = localtime(&now); - if (!tm_struct) { + tm tm_buf; + tm* tm_struct = localtime_r(&now, &tm_buf); + if (!tm_struct) {Tactility/Source/service/rtctime/RtcTimeService.cpp-110-118 (1)
110-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPublishing Time event triggers redundant RTC write.
After setting system time from RTC in
onStart(), publishingSystemEvent::Timeimmediately triggersonTimeChanged(), which writes the same time back to the RTC. Consider either skipping the event publish when time comes from RTC, or adding a guard to avoid the redundant write.Proposed fix option
Add a flag to skip the first event after reading from RTC:
+ bool skipNextTimeEvent = false; + void onTimeChanged(kernel::SystemEvent event) { if (event == kernel::SystemEvent::Time) { + if (skipNextTimeEvent) { + skipNextTimeEvent = false; + return; + } Device* rtc = findRtcDevice(); if (rtc) { writeRtcFromSystemTime(rtc); } } } // In onStart(): if (setSystemTimeFromRtc(rtc)) { + skipNextTimeEvent = true; kernel::publishSystemEvent(kernel::SystemEvent::Time); }Tactility/Source/network/Ntp.cpp-36-40 (1)
36-40:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
localtime()is not thread-safe.Use
localtime_r()for thread-safe time conversion in background threads.Proposed fix
time_t now = time(nullptr); - tm* tm_struct = localtime(&now); - if (!tm_struct) { - return; - } + tm tm_struct; + if (localtime_r(&now, &tm_struct) == nullptr) { + return; + }Then use
tm_struct.instead oftm_struct->below.Tactility/Source/lvgl/Statusbar.cpp-64-93 (1)
64-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
localtime()is not thread-safe.
localtime()returns a pointer to static internal storage that may be overwritten by subsequent calls from other threads. Consider usinglocaltime_r()for thread-safe conversion.Proposed fix
static std::string formatDateTimeString() { time_t now = ::time(nullptr); - tm* tm_struct = localtime(&now); + tm tm_struct; + localtime_r(&now, &tm_struct); settings::SystemSettings sysSettings; settings::loadSystemSettings(sysSettings); - int month = tm_struct->tm_mon + 1; - int day = tm_struct->tm_mday; - int year = tm_struct->tm_year + 1900; + int month = tm_struct.tm_mon + 1; + int day = tm_struct.tm_mday; + int year = tm_struct.tm_year + 1900; // ... and similar changes for tm_hour, tm_minTactility/Source/settings/AudioSettings.cpp-20-26 (1)
20-26:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat malformed persisted volume as invalid input.
Line 23 currently turns non-numeric values into
0, which can silently mute audio instead of using a safe default.🔧 Proposed fix
auto volume_entry = map.find(SETTINGS_KEY_VOLUME); int volume = 70; if (volume_entry != map.end()) { - volume = atoi(volume_entry->second.c_str()); - if (volume < 0) volume = 0; - if (volume > 100) volume = 100; + const auto& raw = volume_entry->second; + if (raw.empty() || raw.find_first_not_of("0123456789") != std::string::npos) { + volume = 70; + } else { + volume = atoi(raw.c_str()); + if (volume < 0) volume = 0; + if (volume > 100) volume = 100; + } }components/waveshare__esp32_s3_touch_amoled_2_06/include/bsp/display.h-62-63 (1)
62-63:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate backlight docs to match implementation path.
These comments still describe PWM pin control, but implementation uses panel DCS brightness commands. The API docs should reflect that to avoid integration confusion.
Also applies to: 73-74, 88-89, 100-101
components/waveshare__esp32_s3_touch_amoled_2_06/idf_component.yml-6-12 (1)
6-12:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace wildcard
*dependency versions with bounded semver ranges for reproducible buildsIn
components/waveshare__esp32_s3_touch_amoled_2_06/idf_component.yml(lines 6-12),*is an open-ended wildcard that can match “anything”, so dependency resolution can drift over time. Use bounded ranges instead:
esp_lcd_touch_ft5x06:^1.0.0(or~1)waveshare/esp_lcd_sh8601:^1.0.2(or~1)toolchain/toolchain-esp32s3.cmake-2-2 (1)
2-2:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd an explicit
IDF_PATHguard before the ESP-IDF toolchain include.
toolchain/toolchain-esp32s3.cmakecurrently does an unguarded:include($ENV{IDF_PATH}/tools/cmake/toolchain.cmake)Add a fail-fast
FATAL_ERRORwhenIDF_PATHis missing/empty to make misconfiguration easier to diagnose.Proposed change
set(_CMAKE_TOOLCHAIN_PREFIX xtensa-esp32s3-elf-) -include($ENV{IDF_PATH}/tools/cmake/toolchain.cmake) +if(NOT DEFINED ENV{IDF_PATH} OR "$ENV{IDF_PATH}" STREQUAL "") + message(FATAL_ERROR "IDF_PATH is not set. Please source ESP-IDF export script first.") +endif() +include("$ENV{IDF_PATH}/tools/cmake/toolchain.cmake")
🧹 Nitpick comments (7)
Devices/waveshare-s3-touch-amoled-18/devicetree.yaml (1)
3-3: ⚡ Quick winUse a 1.8-specific DTS filename for this device slice.
Line 3 references a
...206...DTS from the...-18target, which makes board identity ambiguous and fragile for maintenance/tooling.Suggested fix
-dts: waveshare,s3-touch-amoled-206.dts +dts: waveshare,s3-touch-amoled-18.dtsDevices/waveshare-s3-touch-amoled-18/Source/devices/InternalFlash.h.disabled (1)
15-23: ⚡ Quick winMove
extern mounted_wl_handle/extern is_mountedaboveInternalFlashDevice
InternalFlashDeviceinline methods referencemounted_wl_handlebefore it’s declared (lines 15 and 18–19 vs extern at 22). Reorder the extern declarations so enabling the header won’t hit name-lookup/compile errors; this*.h.disabledfile isn’t referenced by non-disabled sources right now.Suggested fix
`#include` <memory> `#include` <esp_vfs_fat.h> +extern wl_handle_t mounted_wl_handle; +extern bool is_mounted; + class InternalFlashDevice { public: @@ -extern wl_handle_t mounted_wl_handle; -extern bool is_mounted; - extern wl_handle_t getInternalFlashWlHandle(); extern bool isInternalFlashMounted(); extern std::shared_ptr<InternalFlashDevice> createInternalFlash();Tactility/Source/app/localesettings/LocaleSettings.cpp (1)
109-111: ⚡ Quick winRemove the duplicate
textResources.load()call inonShow.It is already loaded at the beginning of
onShow, so this second load is redundant.💡 Suggested fix
auto* region_label = lv_label_create(region_wrapper); - textResources.load(); lv_label_set_text(region_label, textResources[i18n::Text::REGION].c_str());Tactility/Source/app/wifimanage/WifiManage.cpp (1)
7-7: 💤 Low valueUnused include.
DialogBox.his included but not used in this file. Remove it to keep includes clean.Tactility/Source/network/Ntp.cpp (1)
67-71: 💤 Low valueExact time equality check may cause repeated syncs.
Comparing
rtc_time == nowfor exact equality is fragile due to timing differences between reading system time and RTC. Consider allowing a small tolerance (e.g., ±2 seconds).Tactility/Source/service/displayidle/DisplayIdle.cpp (1)
31-65: ⚡ Quick winDuplicate IMU device lookups per tick.
findQmi8658()is called separately in bothcheckMotionWake()andcheckFaceDown(), resulting in two device lookups per tick when both are called. Consider caching the device pointer or combining the checks into a single read.Proposed approach
Create a combined check function that reads once and returns both results:
struct MotionState { bool motionWake = false; bool faceDown = false; }; static MotionState checkMotionState() { MotionState state; Device* imu = findQmi8658(); if (!imu) return state; Qmi8658Data data; if (qmi8658_read(imu, &data) != ERROR_NONE) return state; float magnitude = std::sqrt(data.ax * data.ax + data.ay * data.ay + data.az * data.az); state.motionWake = std::abs(magnitude - 1.0f) > 0.35f; state.faceDown = data.az > 0.9f; return state; }Tactility/Source/service/statusbar/Statusbar.cpp (1)
240-240: ⚖️ Poor tradeoff30-second update interval may delay status changes.
Increasing the statusbar update from 1 second to 30 seconds significantly delays UI feedback for battery level changes, charging state transitions, WiFi signal strength, and SD card mount/unmount events. Consider a shorter interval (e.g., 5-10 seconds) or event-driven updates for immediate state changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 380d9139-4e83-4809-8bd8-593148df5849
⛔ Files ignored due to path filters (1)
partitions-32mb.csvis excluded by!**/*.csv
📒 Files selected for processing (147)
.clangd.opencode/plans/lvgl9-desync-fix.mdBuildscripts/sdkconfig/default.propertiesData/user/app/ImuBall/manifest.propertiesDevices/waveshare-s3-touch-amoled-18/CMakeLists.txtDevices/waveshare-s3-touch-amoled-18/Source/Configuration.cppDevices/waveshare-s3-touch-amoled-18/Source/devices/Display.cppDevices/waveshare-s3-touch-amoled-18/Source/devices/Display.hDevices/waveshare-s3-touch-amoled-18/Source/devices/InternalFlash.cpp.disabledDevices/waveshare-s3-touch-amoled-18/Source/devices/InternalFlash.h.disabledDevices/waveshare-s3-touch-amoled-18/Source/devices/Sdcard.cppDevices/waveshare-s3-touch-amoled-18/Source/devices/Sdcard.hDevices/waveshare-s3-touch-amoled-18/Source/module.cppDevices/waveshare-s3-touch-amoled-18/device.propertiesDevices/waveshare-s3-touch-amoled-18/devicetree.yamlDevices/waveshare-s3-touch-amoled-18/waveshare,s3-touch-amoled-206.dtsDevices/waveshare-s3-touch-amoled-206/CMakeLists.txtDevices/waveshare-s3-touch-amoled-206/Source/Configuration.cppDevices/waveshare-s3-touch-amoled-206/Source/devices/AudioDevice.cppDevices/waveshare-s3-touch-amoled-206/Source/devices/AudioDevice.hDevices/waveshare-s3-touch-amoled-206/Source/devices/Display.cppDevices/waveshare-s3-touch-amoled-206/Source/devices/Display.hDevices/waveshare-s3-touch-amoled-206/Source/devices/Power.cppDevices/waveshare-s3-touch-amoled-206/Source/devices/Power.hDevices/waveshare-s3-touch-amoled-206/Source/devices/Sdcard.cppDevices/waveshare-s3-touch-amoled-206/Source/devices/Sdcard.hDevices/waveshare-s3-touch-amoled-206/Source/devices/Touch.cppDevices/waveshare-s3-touch-amoled-206/Source/devices/Touch.hDevices/waveshare-s3-touch-amoled-206/Source/module.cppDevices/waveshare-s3-touch-amoled-206/device.propertiesDevices/waveshare-s3-touch-amoled-206/devicetree.yamlDevices/waveshare-s3-touch-amoled-206/waveshare,s3-touch-amoled-206.dtsDrivers/AXP2101/Source/Axp2101.cppDrivers/AXP2101/Source/Axp2101.hDrivers/AXP2101/Source/Axp2101Power.cppDrivers/AXP2101/bindings/axp2101.hDrivers/AXP2101/devicetree.yamlDrivers/bm8563-module/bindings/nxp,pcf85063.yamlDrivers/bm8563-module/bindings/nxp,pcf8563.yamlDrivers/bm8563-module/source/bm8563.cppDrivers/qmi8658-module/source/qmi8658.cppFirmware/idf_component.ymlModules/hal-device-module/include/tactility/hal/Device.hModules/lvgl-module/source/arch/lvgl_esp32.cPlatforms/platform-esp32/CMakeLists.txtPlatforms/platform-esp32/include/tactility/drivers/esp32_i2c.hPlatforms/platform-esp32/source/drivers/esp32_i2c.cppTactility/CMakeLists.txtTactility/Include/Tactility/app/dialogbox/DialogBox.hTactility/Include/Tactility/hal/audio/AudioDevice.hTactility/Include/Tactility/service/displayidle/DisplayIdleService.hTactility/Include/Tactility/settings/AudioSettings.hTactility/Private/Tactility/app/files/SupportedFiles.hTactility/Source/PartitionsEsp.cppTactility/Source/Tactility.cppTactility/Source/TactilityEsp.cppTactility/Source/app/audio/Audio.cppTactility/Source/app/dialogbox/DialogBox.cppTactility/Source/app/files/SupportedFiles.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/miscsettings/MiscSettings.cppTactility/Source/app/power/Power.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/wifimanage/WifiManage.cppTactility/Source/hal/sdcard/SpiSdCardDevice.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/network/Ntp.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/rtctime/RtcTimeService.cppTactility/Source/service/statusbar/Statusbar.cppTactility/Source/service/wifi/WifiEsp.cppTactility/Source/settings/AudioSettings.cppTactility/Source/settings/DisplaySettings.cppTactilityKernel/include/tactility/bindings/bm8563.hTactilityKernel/include/tactility/bindings/qmi8658.hTactilityKernel/include/tactility/concurrent/thread.hTactilityKernel/include/tactility/drivers/bm8563.hTactilityKernel/include/tactility/drivers/gpio.hTactilityKernel/include/tactility/drivers/qmi8658.hTactilityKernel/source/concurrent/thread.cppTactilityKernel/source/kernel_symbols.capp-flash_argsbootloader-flash_argsbootloader-prefix/src/bootloader-stamp/bootloader-patch-info.txtbootloader-prefix/src/bootloader-stamp/bootloader-source_dirinfo.txtbootloader-prefix/src/bootloader-stamp/bootloader-update-info.txtbootloader-prefix/tmp/bootloader-cfgcmd.txtbootloader-prefix/tmp/bootloader-mkdirs.cmakebuild.ninjacompile_commands.jsoncomponent_manager_run_counter.43315components/waveshare__esp32_s3_touch_amoled_2_06/.component_hashcomponents/waveshare__esp32_s3_touch_amoled_2_06/CHECKSUMS.jsoncomponents/waveshare__esp32_s3_touch_amoled_2_06/CMakeLists.txtcomponents/waveshare__esp32_s3_touch_amoled_2_06/Kconfigcomponents/waveshare__esp32_s3_touch_amoled_2_06/LICENSEcomponents/waveshare__esp32_s3_touch_amoled_2_06/README.mdcomponents/waveshare__esp32_s3_touch_amoled_2_06/esp32_s3_touch_amoled_2_06.ccomponents/waveshare__esp32_s3_touch_amoled_2_06/idf_component.ymlcomponents/waveshare__esp32_s3_touch_amoled_2_06/include/bsp/config.hcomponents/waveshare__esp32_s3_touch_amoled_2_06/include/bsp/display.hcomponents/waveshare__esp32_s3_touch_amoled_2_06/include/bsp/esp-bsp.hcomponents/waveshare__esp32_s3_touch_amoled_2_06/include/bsp/esp32_s3_touch_amoled_2_06.hcomponents/waveshare__esp32_s3_touch_amoled_2_06/include/bsp/touch.hcomponents/waveshare__esp32_s3_touch_amoled_2_06/priv_include/bsp_err_check.hconfig.envconfig/kconfig_menus.jsonconfig/sdkconfig.cmakeconfig/sdkconfig.hconfig/sdkconfig.jsondata-flash_argsdevice.pyesp-idf/Tactility/data-flash_args.inesp-idf/Tactility/system-flash_args.inesp-idf/bootloader/bootloader-flash_args.inesp-idf/esp_system/ld/linker_script_generator.cmakeesp-idf/esptool_py/app-flash_args.inesp-idf/esptool_py/flasher_args.json.inesp-idf/partition_table/partition-table-flash_args.inesp_idf_project_configuration.jsonflash_app_argsflash_argsflash_args.inflash_bootloader_argsflash_project_argsflasher_args.jsongdbinit/connectgdbinit/gdbinitgdbinit/prefix_mapgdbinit/py_extensionsgdbinit/symbolskconfigs.inkconfigs_projbuild.inldgen_librariesldgen_libraries.inlv_conf.hmain/idf_component.ymlpartition-table-flash_argsproject_description.jsonsession-ses_28c2.mdsystem-flash_argstoolchain/asmflagstoolchain/cflagstoolchain/cxxflagstoolchain/ldflagstoolchain/toolchain-esp32s3.cmake
| @@ -0,0 +1 @@ | |||
| 0 No newline at end of file | |||
There was a problem hiding this comment.
Build artifact should not be committed to version control.
This is an ESP-IDF component manager runtime counter file (note the process-ID-like suffix .43315). Such files are generated during the build process and should be excluded from version control.
| assert(cfg != NULL); | ||
| BSP_ERROR_CHECK_RETURN_NULL(lvgl_port_init(&cfg->lvgl_port_cfg)); | ||
|
|
||
| BSP_NULL_CHECK(disp = bsp_display_lcd_init(cfg), NULL); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Show declaration and call sites for bsp_display_lcd_init
rg -n 'static lv_display_t \*bsp_display_lcd_init\(|bsp_display_lcd_init\(' \
components/waveshare__esp32_s3_touch_amoled_2_06/esp32_s3_touch_amoled_2_06.c -C2Repository: TactilityProject/Tactility
Length of output: 410
Fix argument mismatch at Line 623 (compile blocker).
bsp_display_lcd_init is declared as static lv_display_t *bsp_display_lcd_init() (no parameters) but is called as bsp_display_lcd_init(cfg), causing a signature mismatch.
Minimal compile fix
- BSP_NULL_CHECK(disp = bsp_display_lcd_init(cfg), NULL);
+ BSP_NULL_CHECK(disp = bsp_display_lcd_init(), NULL);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| BSP_NULL_CHECK(disp = bsp_display_lcd_init(cfg), NULL); | |
| BSP_NULL_CHECK(disp = bsp_display_lcd_init(), NULL); |
| struct Module waveshare_s3_touch_amoled_206_module = { | ||
| .name = "waveshare-s3-touch-amoled-206", | ||
| .start = start, |
There was a problem hiding this comment.
Fix module identity to match the 1.8 target.
In Devices/waveshare-s3-touch-amoled-18, Line 15 and Line 16 still use the ...206... identity. With a real 206 module also present in this PR, this risks duplicate symbols and incorrect module registration.
Suggested fix
-struct Module waveshare_s3_touch_amoled_206_module = {
- .name = "waveshare-s3-touch-amoled-206",
+struct Module waveshare_s3_touch_amoled_18_module = {
+ .name = "waveshare-s3-touch-amoled-18",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| struct Module waveshare_s3_touch_amoled_206_module = { | |
| .name = "waveshare-s3-touch-amoled-206", | |
| .start = start, | |
| struct Module waveshare_s3_touch_amoled_18_module = { | |
| .name = "waveshare-s3-touch-amoled-18", | |
| .start = start, |
| static void IRAM_ATTR gpio0_reset_isr(void* arg) { | ||
| // Stop the top app, or restart if no app is running | ||
| tt::app::stop(); | ||
| } |
There was a problem hiding this comment.
Don’t call app-stop logic directly from ISR context.
tt::app::stop() in gpio0_reset_isr can execute non-ISR-safe code. Use ISR to signal/defer work to a task (queue/event flag), then stop the app from task context.
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/driver/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/efuse/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp-tls/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_adc/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_coex/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_common/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_ana_cmpr/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_bitscrambler/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_dac/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_gpio/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_gptimer/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_i2c/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_i2s/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_ledc/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_mcpwm/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_parlio/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_pcnt/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_rmt/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_sdm/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_spi/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_tsens/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_twai/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_uart/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_driver_usb_serial_jtag/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_event/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_http_client/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_http_server/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_hw_support/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_lcd/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_mm/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_netif/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_partition/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_phy/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_pm/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_psram/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_ringbuf/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_rom/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_security/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_system/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_timer/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/esp_wifi/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/fatfs/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/freertos/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/hal/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/heap/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/log/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/lwip/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/mbedtls/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/newlib/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/nvs_flash/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/pthread/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/soc/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/spi_flash/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/spiffs/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/tcp_transport/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/usb/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/vfs/Kconfig" | ||
| source "/Users/oneillchampagnie/.espressif/v5.5.4/esp-idf/components/wear_levelling/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/Firmware/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/Libraries/elf_loader/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/components/waveshare__esp32_s3_touch_amoled_2_06/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/managed_components/espressif__cmake_utilities/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/managed_components/espressif__esp_codec_dev/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/managed_components/espressif__esp_lcd_touch/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/managed_components/espressif__esp_tinyusb/Kconfig" | ||
| source "/Users/oneillchampagnie/Tactility/managed_components/lvgl__lvgl/Kconfig" No newline at end of file |
There was a problem hiding this comment.
Remove machine-local Kconfig paths from committed sources.
Line 1 through Line 66 hardcode /Users/... absolute paths. This makes Kconfig resolution non-portable (breaks on CI/other developers) and leaks local user path information. These files should be generated per-environment or rewritten to project/IDF-root-relative references before committing.
| i2c_device_config_t dev_cfg = { | ||
| .dev_addr_length = I2C_ADDR_BIT_LEN_7, | ||
| .device_address = address, | ||
| .scl_speed_hz = driver_data->clock_frequency_hz, | ||
| }; | ||
| i2c_master_dev_handle_t dev_handle = nullptr; | ||
| esp_err_t err = i2c_master_bus_add_device(driver_data->bus_handle, &dev_cfg, &dev_handle); | ||
| if (err == ESP_OK) { | ||
| err = i2c_master_receive(dev_handle, data, data_size, ticks_to_timeout_ms(timeout)); | ||
| i2c_master_bus_rm_device(dev_handle); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ESP-IDF i2c_master_bus_add_device performance overhead
💡 Result:
The function i2c_master_bus_add_device() in the ESP-IDF I2C master driver is primarily an initialization and resource allocation function, not a routine performed during high-frequency data transmission [1]. Consequently, its performance overhead is typically negligible in application contexts because it is designed to be called once per device during the system setup phase [1]. Regarding the broader performance of the ESP-IDF I2C driver (introduced in v5.2 and now the standard, as the legacy driver is EOL) [2][3]: 1. Architectural Overhead: The driver utilizes a bus-device model [1]. While this abstraction provides better thread safety and resource management, it introduces some overhead compared to legacy implementations due to internal semaphore management and interrupt handling [4][5][6]. 2. Transaction Latency: Developers have reported that transaction times can be affected by interrupt handling and task context switching [5][6]. Specifically, gaps between I2C master transmit and receive operations can occur due to task scheduling or the way the driver handles synchronization [5]. 3. Optimizations: - Synchronous transactions in newer ESP-IDF versions have been optimized to avoid unnecessary busy-waiting, improving efficiency by allowing tasks to yield while waiting for completion [6]. - If extremely low latency is required, users have successfully reduced overhead by minimizing interrupts or tuning task priorities and synchronization methods (e.g., using task notifications instead of semaphores) [5][6]. - Configuration settings like CONFIG_I2C_ISR_IRAM_SAFE can be enabled to keep I2C interrupts functional when the cache is disabled, though this increases IRAM consumption [4][7]. In summary, i2c_master_bus_add_device() itself does not create a performance bottleneck for runtime operations. The primary performance considerations for the I2C driver relate to transaction scheduling, interrupt latency, and proper configuration for real-time constraints [4][5][6].
Citations:
- 1: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/api-reference/peripherals/i2c.html
- 2: https://github.com/nopnop2002/esp-idf-i2c-benchmark
- 3: https://docs.espressif.com/projects/esp-idf/en/stable/esp32/migration-guides/release-6.x/6.0/peripherals.html
- 4: https://docs.espressif.com/projects/esp-idf/en/v5.2/esp32/api-reference/peripherals/i2c.html
- 5: 35us time gaps in i2c_master_transmit_receive() signal (IDFGH-14976) espressif/esp-idf#15675
- 6: fix(i2c_master): Do not busy wait on sync transactions (IDFGH-12280) espressif/esp-idf#13322
- 7: https://github.com/espressif/esp-idf/blob/v5.2/docs/en/api-reference/peripherals/i2c.rst
Avoid per-transaction i2c_master_bus_add_device() / i2c_master_bus_rm_device(); cache device handles instead.
i2c_master_bus_add_device() is an initialization/resource-allocation path intended to be called once per device during setup; creating/removing the device handle for every transaction adds avoidable overhead. Create the device handle once (or lazily cache by address) and reuse it for subsequent reads/writes; remove it when the driver stops.
1 participant
Checklist
Description
initialized Waveshare S3 Touch AMOLED 2.06" port
Summary by CodeRabbit
Release Notes
New Features
Improvements