Feature additions #434
Feature additions #434
Conversation
KeyboardBacklight driver Trackball driver Display sleep/wake Files - create file/folder Keyboard settings Time & Date settings tweaks Locale settings tweaks Maybe missed some things in the list. Maybe more to come. A few notes: Something weird is happening with the language dropdown menu. It appears to have ghost entries. Touch wake could do with some kinda of block on wake first touch to prevent UI elements being hit when waking device with touch. Same with encoder/trackball press i guess.
|
Warning Rate limit exceeded@Shadowtrance has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 31 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds KeyboardBacklight and Trackball drivers, device implementations, and CMake component registrations; introduces a Keyboard Settings app and persistent keyboard settings (including backlight timeout). Adds a DisplayIdle service to dim/restore display and keyboard backlights on inactivity. Extends SystemSettings with dateFormat and region and provides timezone/date‑format defaults. Adds display backlight timeout fields and UI, file-manager create-file/create-folder actions and UI, and a large SystemInfo UI refactor (memory/PSRAM/storage/tasks). Improves properties parsing/logging, preserves WiFi during ESP‑NOW init, doubles a memory-check timer interval, and adds tt::hal::Device::Type::Other. Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/service/espnow/EspNowWifi.cpp (1)
36-80: Verify WiFi mode when already active and add mode validation for ESP-NOW reliability.The code preserves an existing WiFi connection by skipping reinitialization when WiFi is already active. However,
esp_wifi_set_mode(mode)(line 71) is only called whenwifi::getRadioState() == wifi::RadioState::Off. If WiFi is already running but not inConnectionActivestate (e.g., connecting, or in an intermediate state), the WiFi mode will not be updated to match the required mode.According to ESP-IDF documentation, ESP-NOW RX is reliably supported only in STA mode. Using AP or AP+STA modes results in unstable or missing ESP-NOW RX functionality. The current code assumes that preserving an active connection means STA mode is already set, but this assumption is not validated. A mismatch between the current WiFi mode and the required mode could cause ESP-NOW initialization to appear successful while silently degrading ESP-NOW RX reliability.
Add a call to query and verify the current WiFi mode when WiFi is already active (line 59), and either enforce the required mode or fail fast with a clear error if the modes are incompatible.
Additionally,
deinitWifi()(line 115) contains unreachable code—the secondreturn true;is unreachable after the first return on line 113. Remove the duplicate.
🧹 Nitpick comments (19)
Tactility/Source/app/files/View.cpp (2)
204-220: Consider consolidating the duplicate size threshold checks.The size threshold comparisons are performed twice: once to determine the suffix (lines 204-213) and again to choose the format specifier (lines 216-220). This could be slightly simplified.
🔎 Proposed simplification
- if (st.st_size < 1024) { - size = st.st_size; - size_suffix = " B"; - } else if (st.st_size < 1024 * 1024) { - size = st.st_size / 1024.0; - size_suffix = " KB"; - } else { - size = st.st_size / (1024.0 * 1024.0); - size_suffix = " MB"; - } - - char size_str[32]; - if (st.st_size < 1024) { - snprintf(size_str, sizeof(size_str), " (%d%s)", (int)size, size_suffix); - } else { - snprintf(size_str, sizeof(size_str), " (%.1f%s)", size, size_suffix); - } + char size_str[32]; + if (st.st_size < 1024) { + snprintf(size_str, sizeof(size_str), " (%lld B)", (long long)st.st_size); + } else if (st.st_size < 1024 * 1024) { + snprintf(size_str, sizeof(size_str), " (%.1f KB)", st.st_size / 1024.0); + } else { + snprintf(size_str, sizeof(size_str), " (%.1f MB)", st.st_size / (1024.0 * 1024.0)); + }
320-321: New button member variables are assigned but not used elsewhere.The
new_file_buttonandnew_folder_buttonpointers are stored as class members but aren't referenced after assignment. If they're intended for future visibility toggling (similar tonavigate_up_buttonon lines 305-308), this is fine as forward-looking. Otherwise, local variables would suffice.Tactility/Source/app/systeminfo/SystemInfo.cpp (2)
371-374: Consider extracting the PSRAM-first allocation pattern.This allocation pattern (try PSRAM, fallback to internal RAM) is duplicated from
updateRtosTasks. A small helper would reduce duplication:static TaskStatus_t* allocateTaskStatusArray(UBaseType_t count) { auto* tasks = (TaskStatus_t*)heap_caps_malloc(sizeof(TaskStatus_t) * count, MALLOC_CAP_SPIRAM | MALLOC_CAP_8BIT); if (!tasks) tasks = (TaskStatus_t*)malloc(sizeof(TaskStatus_t) * count); return tasks; }
414-420: Consider caching UI widgets instead of rebuilding on each update.
updatePsramdestroys and recreates all labels every 10 seconds. For mostly static content (configuration info doesn't change), consider:
- Creating labels once in
onShow- Only updating the dynamic values (usage, peak, min free, largest block) via
lv_label_set_textThis reduces memory churn and improves efficiency, though the current approach is acceptable for a 10-second interval.
Tactility/Source/app/display/Display.cpp (1)
81-91: Consider extracting the timeout mapping logic to reduce duplication.The timeout value mapping appears in two places:
- Line 86: Forward mapping (dropdown index → milliseconds) using a static array
- Line 219: Reverse mapping (milliseconds → dropdown index) using an if-else chain
If the dropdown options change, both locations require updates, increasing maintenance burden and error risk.
🔎 Suggested refactor to extract mapping logic
Create a helper struct at class or file scope:
struct TimeoutOption { uint32_t ms; const char* label; }; static constexpr TimeoutOption TIMEOUT_OPTIONS[] = { {15000, "15 seconds"}, {30000, "30 seconds"}, {60000, "1 minute"}, {120000, "2 minutes"}, {300000, "5 minutes"}, {0, "Never"} }; static constexpr size_t TIMEOUT_OPTION_COUNT = sizeof(TIMEOUT_OPTIONS) / sizeof(TIMEOUT_OPTIONS[0]);Then use helper functions:
static uint32_t msFromIndex(uint32_t idx) { return idx < TIMEOUT_OPTION_COUNT ? TIMEOUT_OPTIONS[idx].ms : 60000; } static uint32_t indexFromMs(uint32_t ms) { for (size_t i = 0; i < TIMEOUT_OPTION_COUNT; ++i) { if (TIMEOUT_OPTIONS[i].ms == ms) return i; } return 2; // default to 1 minute }This centralizes the mapping and makes it easier to add or modify timeout options.
Also applies to: 217-220
Tactility/Source/app/keyboard/KeyboardSettings.cpp (2)
9-16: Consider including headers instead of forward declarations.Forward declaring driver functions works but creates a fragile dependency. If the driver API changes (e.g., return type or parameters), this won't catch mismatches at compile time.
🔎 Alternative approach
If headers exist for these drivers, prefer including them:
-// Forward declare driver functions -namespace driver::keyboardbacklight { - bool setBrightness(uint8_t brightness); -} - -namespace driver::trackball { - void setEnabled(bool enabled); -} +#include <Drivers/KeyboardBacklight/Include/KeyboardBacklight.h> +#include <Drivers/Trackball/Include/Trackball.h>
174-178: Clamped timeout value not synced back to settings.If the loaded
backlightTimeoutMsis outside the 5-600 second range, the slider displays the clamped value butkbSettings.backlightTimeoutMsretains the original out-of-range value. This could cause confusion when saved.🔎 Proposed fix
lv_slider_set_range(sliderTimeoutSeconds, 5, 600); // 5s to 10 minutes if (timeoutSeconds < 5) timeoutSeconds = 5; if (timeoutSeconds > 600) timeoutSeconds = 600; + kbSettings.backlightTimeoutMs = timeoutSeconds * 1000; // Sync clamped value lv_slider_set_value(sliderTimeoutSeconds, (int32_t)timeoutSeconds, LV_ANIM_OFF);Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (1)
34-46: Duplicate date format definitions - consider extracting to a constant.The
dateFormatsarray is defined here and the same strings appear inlv_dropdown_set_options(line 97). If these get out of sync, the wrong format could be saved.🔎 Proposed refactor
+namespace { + constexpr const char* DATE_FORMATS[] = {"MM/DD/YYYY", "DD/MM/YYYY", "YYYY-MM-DD", "YYYY/MM/DD"}; + constexpr const char* DATE_FORMAT_OPTIONS = "MM/DD/YYYY\nDD/MM/YYYY\nYYYY-MM-DD\nYYYY/MM/DD"; +} static void onDateFormatChanged(lv_event_t* event) { auto* dropdown = static_cast<lv_obj_t*>(lv_event_get_target(event)); auto index = lv_dropdown_get_selected(dropdown); - const char* dateFormats[] = {"MM/DD/YYYY", "DD/MM/YYYY", "YYYY-MM-DD", "YYYY/MM/DD"}; - std::string selected_format = dateFormats[index]; + std::string selected_format = DATE_FORMATS[index];Tactility/Source/service/displayidle/DisplayIdle.cpp (1)
40-43: UnusedreloadSettings()method.This method is defined but never called. The TODO comment on lines 103-104 mentions needing events for dynamic updates -
reloadSettings()could be the handler for such events.Either wire this up to a settings-changed event, or remove the dead code:
- void reloadSettings() { - cachedDisplaySettings = settings::display::loadOrGetDefault(); - cachedKeyboardSettings = settings::keyboard::loadOrGetDefault(); - }Tactility/Source/app/localesettings/LocaleSettings.cpp (1)
151-159: Consider LVGL lock when accessing widget inonHide.
lv_textarea_get_text(regionTextArea)accesses an LVGL widget. WhileonHideis typically called in UI context, adding LVGL lock would ensure thread safety, consistent with patterns seen inTimeDateSettings.cppline 143.🔎 Proposed fix
void onHide(TT_UNUSED AppContext& app) override { if (settingsUpdated && regionTextArea) { + std::string regionText; + if (lvgl::lock(100 / portTICK_PERIOD_MS)) { + regionText = lv_textarea_get_text(regionTextArea); + lvgl::unlock(); + } settings::SystemSettings sysSettings; if (settings::loadSystemSettings(sysSettings)) { - sysSettings.region = lv_textarea_get_text(regionTextArea); + sysSettings.region = regionText; settings::saveSystemSettings(sysSettings); } } }Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (1)
68-71: Document the minimum threshold rationale.The minimum threshold of 30 for default brightness is not explained. Consider documenting whether this is a hardware limitation or a design decision to ensure visibility.
Devices/lilygo-tdeck/Source/devices/TdeckKeyboard.cpp (1)
47-64: Consider blocking first keypress after display wake.The PR description notes that "Display wake may require a block on the first touch to avoid accidental UI interaction; similar behavior may be needed for encoder/trackball/keyboard presses." This logic restores the display and keyboard backlight on keypress but does not prevent the keypress from triggering UI actions immediately after wake.
The brightness restoration on every keypress should not cause I2C spam due to the caching logic in the driver (lines 36-38 in KeyboardBacklight.cpp).
Would you like me to help implement debouncing logic to ignore the first keypress after wake, similar to what may be needed for touch/encoder/trackball?
Devices/lilygo-tdeck/Source/Init.cpp (1)
86-99: Clarify error handling in keyboardInit().The function captures the return value from
setBrightness()in theresultvariable but only uses it to log a warning. The function always returnstrueregardless of whether brightness setting succeeded. If brightness setting failure should be non-fatal, consider removing the unusedresultvariable. Otherwise, propagate the failure.🔎 Proposed refactor
If brightness failure is non-fatal (current behavior):
static bool keyboardInit() { auto kbSettings = tt::settings::keyboard::loadOrGetDefault(); auto kbBacklight = tt::hal::findDevice("Keyboard Backlight"); - bool result = driver::keyboardbacklight::setBrightness(kbSettings.backlightEnabled ? kbSettings.backlightBrightness : 0); - - if (!result) { + if (!driver::keyboardbacklight::setBrightness(kbSettings.backlightEnabled ? kbSettings.backlightBrightness : 0)) { TT_LOG_W(TAG, "Failed to set keyboard backlight brightness"); } driver::trackball::setEnabled(kbSettings.trackballEnabled); return true; }Drivers/KeyboardBacklight/Source/KeyboardBacklight.h (3)
8-14: Consider adding validation and error handling to init().The
init()function always returnstrue(per the implementation), but doesn't validate that the I2C port is valid or already initialized. Consider:
- Adding validation for
i2cPort < I2C_NUM_MAX- Checking if the I2C bus is already initialized
- Returning
falseif preconditions aren't metThis would provide better error feedback to callers.
16-28: Document brightness range inconsistency.The documentation states that
setBrightness()accepts 0-255, whilesetDefaultBrightness()clamps to 30-255. This inconsistency should be clarified:
- Why does default brightness have a minimum of 30?
- Should
setBrightness()also have a minimum threshold?- Update documentation to explain the rationale for the different ranges.
1-36: Consider adding a deinit() function.The API provides
init()but no correspondingdeinit()or cleanup function. While the keyboard backlight may not require explicit cleanup, providing a deinit function would:
- Allow proper resource cleanup if needed in the future
- Provide symmetry with the Trackball driver API (which has
deinit())- Enable proper stop/restart cycles
Drivers/Trackball/Source/Trackball.cpp (2)
16-16: Unused variable: g_encDiff.The global variable
g_encDiffis declared but never read or written. Consider removing it to reduce confusion.🔎 Proposed fix to remove unused variable
static bool g_lastState[5] = {false, false, false, false, false}; -// Encoder diff accumulator for navigation -static int16_t g_encDiff = 0;
75-124: Consider partial initialization rollback on GPIO config failure.Lines 103-107 configure GPIO pins in a loop, but if
gpio_config()fails partway through, the function doesn't roll back already-configured pins. While this is unlikely to cause issues in practice, it could lead to inconsistent hardware state on error.Consider either:
- Checking return values and rolling back on error
- Documenting that partial configuration is acceptable
For a production implementation, option 1 is preferred for robustness.
Drivers/Trackball/Source/Trackball.h (1)
20-42: LGTM with minor documentation suggestion.The API is well-structured. Consider documenting:
- That
init()returnsnullptron failure- Thread safety expectations (whether calls must be synchronized)
- Whether
deinit()is safe to call multiple times
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (37)
Data/data/settings/system.propertiesDevices/lilygo-tdeck/CMakeLists.txtDevices/lilygo-tdeck/Source/Configuration.cppDevices/lilygo-tdeck/Source/Init.cppDevices/lilygo-tdeck/Source/devices/KeyboardBacklight.cppDevices/lilygo-tdeck/Source/devices/KeyboardBacklight.hDevices/lilygo-tdeck/Source/devices/TdeckKeyboard.cppDevices/lilygo-tdeck/Source/devices/TrackballDevice.cppDevices/lilygo-tdeck/Source/devices/TrackballDevice.hDrivers/KeyboardBacklight/CMakeLists.txtDrivers/KeyboardBacklight/Source/KeyboardBacklight.cppDrivers/KeyboardBacklight/Source/KeyboardBacklight.hDrivers/Trackball/CMakeLists.txtDrivers/Trackball/Source/Trackball.cppDrivers/Trackball/Source/Trackball.hTactility/CMakeLists.txtTactility/Include/Tactility/settings/DisplaySettings.hTactility/Include/Tactility/settings/KeyboardSettings.hTactility/Include/Tactility/settings/SystemSettings.hTactility/Private/Tactility/app/files/State.hTactility/Private/Tactility/app/files/View.hTactility/Source/Tactility.cppTactility/Source/app/display/Display.cppTactility/Source/app/files/View.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timedatesettings/TimeDateSettings.cppTactility/Source/file/PropertiesFile.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/espnow/EspNowWifi.cppTactility/Source/service/loader/Loader.cppTactility/Source/service/memorychecker/MemoryCheckerService.cppTactility/Source/settings/DisplaySettings.cppTactility/Source/settings/KeyboardSettings.cppTactility/Source/settings/SystemSettings.cppTactility/Source/settings/Time.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/app/files/View.cppDevices/lilygo-tdeck/Source/Configuration.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/localesettings/LocaleSettings.cpp
📚 Learning: 2025-10-28T19:44:35.263Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Drivers/ST7789-i8080/Source/St7789i8080Display.cpp:326-329
Timestamp: 2025-10-28T19:44:35.263Z
Learning: In the LilyGo T-Display S3 ST7789 i8080 display driver (Drivers/ST7789-i8080/Source/St7789i8080Display.cpp), both callback registrations are required: setting on_color_trans_done during panel IO creation in configurePanelIO() and calling esp_lcd_panel_io_register_event_callbacks() in initializeLvgl() after the LVGL display is created. This dual registration is intentional and necessary for proper operation on this hardware.
Applied to files:
Tactility/Source/service/displayidle/DisplayIdle.cppDevices/lilygo-tdeck/Source/Configuration.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (16)
Devices/lilygo-tdeck/Source/devices/TrackballDevice.h (1)
Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp (4)
start(4-26)start(4-4)stop(28-35)stop(28-28)
Tactility/Source/app/display/Display.cpp (2)
Tactility/Source/app/settings/Settings.cpp (2)
app(28-45)app(28-28)Tactility/Source/app/screenshot/Screenshot.cpp (1)
app(42-42)
Tactility/Source/file/PropertiesFile.cpp (1)
TactilityCore/Source/file/File.cpp (2)
readLines(361-387)readLines(361-361)
Tactility/Source/service/displayidle/DisplayIdle.cpp (6)
Tactility/Source/settings/DisplaySettings.cpp (2)
loadOrGetDefault(126-132)loadOrGetDefault(126-126)Tactility/Source/settings/KeyboardSettings.cpp (2)
loadOrGetDefault(47-53)loadOrGetDefault(47-47)Tactility/Source/app/boot/Boot.cpp (1)
display(152-157)Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (2)
setBrightness(24-29)setBrightness(24-24)Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (2)
setBrightness(29-60)setBrightness(29-29)Tactility/Source/app/keyboard/KeyboardSettings.cpp (1)
setBrightness(11-11)
Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp (1)
Drivers/Trackball/Source/Trackball.cpp (2)
init(75-124)init(75-75)
Tactility/Source/settings/SystemSettings.cpp (1)
Tactility/Source/file/PropertiesFile.cpp (2)
savePropertiesFile(56-73)savePropertiesFile(56-56)
Drivers/Trackball/Source/Trackball.h (1)
Drivers/Trackball/Source/Trackball.cpp (8)
init(75-124)init(75-75)deinit(126-133)deinit(126-126)setMovementStep(135-140)setMovementStep(135-135)setEnabled(142-145)setEnabled(142-142)
Tactility/Include/Tactility/settings/KeyboardSettings.h (3)
Tactility/Include/Tactility/settings/SystemSettings.h (1)
tt(5-18)Tactility/Source/settings/DisplaySettings.cpp (8)
load(68-114)load(68-68)loadOrGetDefault(126-132)loadOrGetDefault(126-126)getDefault(116-124)getDefault(116-116)save(134-142)save(134-134)Tactility/Source/settings/KeyboardSettings.cpp (8)
load(16-35)load(16-16)loadOrGetDefault(47-53)loadOrGetDefault(47-47)getDefault(37-45)getDefault(37-37)save(55-63)save(55-55)
Tactility/Private/Tactility/app/files/View.h (1)
Tactility/Source/app/files/View.cpp (4)
onNewFilePressed(258-262)onNewFilePressed(258-258)onNewFolderPressed(264-268)onNewFolderPressed(264-264)
Drivers/KeyboardBacklight/Source/KeyboardBacklight.h (2)
Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (8)
init(16-27)init(16-16)setBrightness(29-60)setBrightness(29-29)setDefaultBrightness(62-90)setDefaultBrightness(62-62)getBrightness(92-94)getBrightness(92-92)Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (4)
setBrightness(24-29)setBrightness(24-24)getBrightness(31-36)getBrightness(31-31)
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.h (2)
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (8)
start(5-13)start(5-5)stop(15-22)stop(15-15)setBrightness(24-29)setBrightness(24-24)getBrightness(31-36)getBrightness(31-31)Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (4)
setBrightness(29-60)setBrightness(29-29)getBrightness(92-94)getBrightness(92-92)
Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (3)
Tactility/Source/settings/Time.cpp (6)
setTimeFormat24Hour(71-79)setTimeFormat24Hour(71-71)getTimeZoneName(42-50)getTimeZoneName(42-42)setTimeZone(29-40)setTimeZone(29-29)Tactility/Source/app/timezone/TimeZone.cpp (7)
start(237-239)start(237-237)app(74-77)app(186-220)app(186-186)app(222-226)app(222-222)Tactility/Source/settings/SystemSettings.cpp (4)
loadSystemSettings(66-76)loadSystemSettings(66-66)saveSystemSettings(78-95)saveSystemSettings(78-78)
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (1)
Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (6)
init(16-27)init(16-16)setBrightness(29-60)setBrightness(29-29)getBrightness(92-94)getBrightness(92-92)
Tactility/Source/Tactility.cpp (4)
Tactility/Source/app/apphub/AppHubApp.cpp (2)
manifest(73-76)manifest(73-73)Tactility/Source/app/applist/AppList.cpp (2)
manifest(19-23)manifest(19-19)Tactility/Source/app/appsettings/AppSettings.cpp (2)
manifest(20-24)manifest(20-20)Tactility/Source/service/ServiceRegistration.cpp (4)
addService(23-41)addService(23-23)addService(43-45)addService(43-43)
Tactility/Source/app/keyboard/KeyboardSettings.cpp (5)
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (2)
setBrightness(24-29)setBrightness(24-24)Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (2)
setBrightness(29-60)setBrightness(29-29)Drivers/Trackball/Source/Trackball.cpp (2)
setEnabled(142-145)setEnabled(142-142)Tactility/Source/settings/KeyboardSettings.cpp (4)
loadOrGetDefault(47-53)loadOrGetDefault(47-47)save(55-63)save(55-55)Tactility/Source/Tactility.cpp (2)
getMainDispatcher(335-337)getMainDispatcher(335-335)
Tactility/Source/app/localesettings/LocaleSettings.cpp (1)
Tactility/Source/settings/SystemSettings.cpp (4)
loadSystemSettings(66-76)loadSystemSettings(66-66)saveSystemSettings(78-95)saveSystemSettings(78-78)
🔇 Additional comments (45)
Tactility/Source/service/memorychecker/MemoryCheckerService.cpp (1)
11-11: LGTM! Reasonable optimization.Doubling the memory check interval from 1 to 2 seconds reduces monitoring overhead while remaining responsive for gradual memory degradation scenarios.
Tactility/Private/Tactility/app/files/View.h (1)
18-19: LGTM!The new UI element declarations and handler method signatures follow the established patterns in this class. The nullptr initialization for the button pointers is consistent with the existing
navigate_up_buttonmember.Also applies to: 43-44
Tactility/Private/Tactility/app/files/State.h (1)
15-21: LGTM!The new
ActionCreateFileandActionCreateFolderenum values extend the state machine cleanly and follow the existing naming convention.Tactility/Source/app/files/View.cpp (1)
435-451: LGTM!The folder creation logic properly handles errors via
mkdirreturn value and updates the view after successful creation. The 0755 permissions are appropriate for directories.Tactility/Source/service/espnow/EspNowWifi.cpp (1)
100-102: LGTM!The diagnostic logging clearly indicates whether an existing WiFi connection was preserved, which will be helpful for debugging coexistence scenarios.
Tactility/Source/service/loader/Loader.cpp (1)
11-11: Clear documentation of the include purpose.The comment clearly explains why
malloc.his included. However, the availability and effectiveness ofmalloc_trim()on the ESP32 platform should be verified (see comment on lines 267-272).Tactility/Source/app/systeminfo/SystemInfo.cpp (5)
55-69: LGTM!The new PSRAM helper functions follow the established pattern of the existing heap functions with proper platform guards and sensible fallback values.
221-251: LGTM!Good defensive allocation strategy with PSRAM preference and internal RAM fallback. The error handling for allocation failure is appropriate, and sorting tasks by CPU usage when showing percentages improves readability.
688-697: LGTM!Good timer lifecycle management. Starting timers after initial updates and stopping them in
onHideensures resources aren't wasted when the app isn't visible.
279-285: LGTM!Safe pattern for timer callbacks to access the app instance. The null checks and manifest ID comparison prevent use-after-hide issues.
123-154: LGTM!Clean encapsulation of bar and label widgets. The UI scale-aware styling is a good touch for different display configurations.
Tactility/Include/Tactility/settings/DisplaySettings.h (1)
19-20: LGTM!The new timeout settings fields are clearly defined and well-documented. The comment clarifies that 0 means "Never", which aligns with the UI implementation in Display.cpp.
Tactility/Source/file/PropertiesFile.cpp (2)
22-24: LGTM!Changing the log level to debug for frequent operations reduces console noise, which improves readability during normal operations.
31-32: LGTM!Enhanced whitespace trimming to handle Windows-style CRLF line endings improves cross-platform compatibility. The updated error handling that continues processing after malformed lines makes the parser more resilient to partial file corruption.
Also applies to: 42-44
Tactility/Source/settings/Time.cpp (1)
48-48: LGTM!Providing concrete default values for timezone name and code is a good improvement over returning empty strings. This ensures the system has valid timezone configuration even when preferences are missing.
Note: There's a minor inconsistency with the timezone default in
Data/data/settings/system.properties(America/New_York vs. America/Los_Angeles), which was flagged in the review for that file.Also applies to: 58-58
Tactility/Source/settings/SystemSettings.cpp (2)
43-60: LGTM!The loading logic correctly handles missing or empty
dateFormatandregionproperties with sensible defaults, providing good backward compatibility with older configuration files. The informative logging helps with debugging configuration issues.
83-84: LGTM!The new fields are correctly persisted to the properties file, and the cache is properly updated after saving to maintain consistency between in-memory and on-disk state.
Also applies to: 91-93
Tactility/Include/Tactility/settings/SystemSettings.h (1)
10-11: LGTM!The new fields are clearly defined with helpful comments indicating the expected format and examples. This extends the public API cleanly without breaking existing code.
Tactility/Source/app/display/Display.cpp (2)
66-79: LGTM!The switch handler correctly updates the
backlightTimeoutEnabledsetting and synchronizes the dropdown's enabled/disabled state accordingly.
183-223: LGTM!The UI construction for the timeout controls is well-structured, properly aligned, and correctly initializes the dropdown selection and enabled state from the loaded settings.
Tactility/Source/Tactility.cpp (2)
54-54: LGTM!The new service and app manifest declarations follow the existing patterns and are correctly placed in their respective namespaces.
Also applies to: 87-87
129-129: LGTM!The registration calls for the new keyboard settings app and display idle service are correctly placed in the appropriate registration functions and follow the established patterns.
Also applies to: 233-233
Tactility/Include/Tactility/settings/KeyboardSettings.h (1)
1-23: LGTM!The header is clean and follows the established settings API pattern used by
DisplaySettingsandSystemSettings. The struct members are appropriately typed and documented.Tactility/Source/app/keyboard/KeyboardSettings.cpp (1)
181-186: Good use of dispatcher for async save.Saving settings via
getMainDispatcher().dispatch()ensures file I/O doesn't block the UI thread. Nice pattern.Tactility/Source/settings/DisplaySettings.cpp (1)
95-111: LGTM!The timeout settings loading follows the established pattern in this file. Boolean parsing is consistent, and the default of 60 seconds is reasonable for a display timeout.
Tactility/Source/settings/KeyboardSettings.cpp (1)
55-63: LGTM!The
save()function correctly serializes all settings fields to the properties file.Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (2)
135-149: LGTM!The
onResulthandler properly validates the result, updates settings, and safely updates the UI with LVGL lock protection.
142-147: No changes needed—code is correct.The timeout calculation
100 / portTICK_PERIOD_MSis the correct approach. The function signaturebool tt_lvgl_lock(TickType timeout)expects ticks, not milliseconds, so converting milliseconds to ticks with this division is proper. While some other parts of the codebase use raw tick values (e.g., DisplayIdle.cpp line 51), the code at lines 142-147 uses the portable and correct pattern.Tactility/Source/service/displayidle/DisplayIdle.cpp (2)
45-94: Well-structured idle detection logic.The tick logic properly handles:
- Single LVGL lock acquisition for both checks
- Graceful handling when timeout is disabled
- Proper state tracking to avoid redundant brightness changes
The 100ms activity threshold combined with 250ms timer interval provides responsive wake behavior (~350ms worst case).
96-110: LGTM on startup logic.Settings are loaded once at startup to avoid file I/O in the timer callback, which prevents watchdog timeouts. The TODO for dynamic settings updates is noted and reasonable to defer.
Tactility/Source/app/localesettings/LocaleSettings.cpp (1)
81-84: Region change tracking looks correct.The event handler appropriately marks
settingsUpdatedwithout performing any heavy operations, deferring the actual save toonHide.Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.h (1)
1-31: LGTM!The
KeyboardBacklightDeviceclass follows the established device pattern with clear lifecycle methods and well-documented API. The implementation properly inherits fromtt::hal::Deviceand provides appropriate type, name, and description overrides.Drivers/KeyboardBacklight/CMakeLists.txt (1)
1-5: LGTM!The CMake configuration properly registers the KeyboardBacklight component with appropriate source directories and dependencies.
Devices/lilygo-tdeck/Source/Configuration.cpp (1)
2-2: LGTM!The new keyboard backlight and trackball devices are properly integrated into the device creation flow, following the established pattern.
Also applies to: 6-6, 20-21
Tactility/CMakeLists.txt (1)
13-13: LGTM!The KeyboardBacklight dependency is correctly added to the ESP-IDF component requirements list.
Drivers/Trackball/CMakeLists.txt (1)
1-5: LGTM!The Trackball component is properly registered with appropriate dependencies including lvgl for input device integration.
Devices/lilygo-tdeck/CMakeLists.txt (1)
6-6: LGTM!The component dependencies are correctly updated to include the new KeyboardBacklight and Trackball drivers.
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (3)
15-22: LGTM!Properly turns off the backlight on shutdown and clears the initialization state.
24-36: LGTM!Both methods correctly check the
initializedflag before delegating to the driver, preventing operations on an uninitialized device.
5-13: The I2C initialization order and slave address are correct.I2C_NUM_0 is initialized by
hal::init()(in Tactility/Source/hal/Hal.cpp) early in the boot sequence, withInitMode::ByTactilityconfigured for the T-Deck. The KeyboardBacklightDevice starts 500ms later via the deferred timer ininitI2cDevices(), ensuring the I2C bus is ready. The default slave address 0x55 is correct for the T-Deck keyboard backlight controller and is consistently used throughout the codebase (Drivers/KeyboardBacklight and TdeckKeyboard).Devices/lilygo-tdeck/Source/devices/TrackballDevice.h (2)
9-16: LGTM!The public API surface is well-defined with appropriate accessors and lifecycle methods.
18-20: LGTM!The private member initialization is correct.
Drivers/Trackball/Source/Trackball.cpp (2)
126-145: LGTM!The deinitialization and runtime configuration methods are correctly implemented.
8-16: Thread safety concern: Global state without synchronization.The global state variables (
g_config,g_indev,g_initialized,g_enabled,g_lastState,g_encDiff) are accessed from multiple contexts:
read_cb()is called by the LVGL tasksetEnabled()andsetMovementStep()can be called from other threadsinit()anddeinit()modify shared stateThis creates potential race conditions, particularly on
g_enabledandg_config.movementStep. Consider adding mutex protection if these functions can be called concurrently with LVGL processing.⛔ Skipped due to learnings
Learnt from: KenVanHoeylandt Repo: ByteWelder/Tactility PR: 395 File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153 Timestamp: 2025-10-27T22:52:15.539Z Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp (1)
4-26: Initialization logic is correct.The code properly handles the already-initialized case and follows the LilyGo reference design. GPIO_NUM_0 is used extensively throughout the codebase in other device configurations (buttons, display control signals, data lines) without boot conflicts. Since trackball initialization occurs in
start()(after device boot), there is no interference with boot mode selection.
Keyboard settings more in line with display settings.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
Devices/lilygo-tdeck/Source/Init.cpp (1)
91-104: Remove unused variable.Line 94 retrieves the
kbBacklightdevice but never uses it. The code directly callsdriver::keyboardbacklight::setBrightness()at line 95 without going through the device interface.🔎 Proposed fix to remove unused variable
static bool keyboardInit() { auto kbSettings = tt::settings::keyboard::loadOrGetDefault(); - auto kbBacklight = tt::hal::findDevice("Keyboard Backlight"); bool result = driver::keyboardbacklight::setBrightness(kbSettings.backlightEnabled ? kbSettings.backlightBrightness : 0); if (!result) { TT_LOG_W(TAG, "Failed to set keyboard backlight brightness"); } driver::trackball::setEnabled(kbSettings.trackballEnabled); return true; }Tactility/Source/app/keyboard/KeyboardSettings.cpp (2)
10-16: Consider including proper driver headers instead of forward declarations.While the forward declarations work, they create a maintenance burden if the driver API changes. Including the actual driver headers would provide compile-time verification and better IDE support.
💡 Alternative approach
-// Forward declare driver functions -namespace driver::keyboardbacklight { - bool setBrightness(uint8_t brightness); -} - -namespace driver::trackball { - void setEnabled(bool enabled); -} +#include <KeyboardBacklight.h> +#include <Trackball.h>
182-185: Consider simplifying the timeout value mapping.The long if-else chain at line 184 makes the timeout→index mapping harder to maintain. A lookup table would be clearer.
🔎 Proposed refactor using a lookup approach
lv_obj_add_event_cb(timeoutDropdown, onTimeoutChanged, LV_EVENT_VALUE_CHANGED, this); // Initialize dropdown selection from settings uint32_t ms = kbSettings.backlightTimeoutMs; - uint32_t idx = 2; // default 1 minute - if (ms == 15000) idx = 0; else if (ms == 30000) idx = 1; else if (ms == 60000) idx = 2; else if (ms == 120000) idx = 3; else if (ms == 300000) idx = 4; else if (ms == 0) idx = 5; + static const uint32_t timeout_values[] = {15000, 30000, 60000, 120000, 300000, 0}; + uint32_t idx = 2; // default 1 minute + for (uint32_t i = 0; i < sizeof(timeout_values)/sizeof(timeout_values[0]); i++) { + if (timeout_values[i] == ms) { + idx = i; + break; + } + } lv_dropdown_set_selected(timeoutDropdown, idx); if (!kbSettings.backlightTimeoutEnabled) { lv_obj_add_state(timeoutDropdown, LV_STATE_DISABLED);Note: This reuses the same array defined in
onTimeoutChangedat line 85. Consider extracting it as a class constant to avoid duplication.Tactility/Source/service/displayidle/DisplayIdle.cpp (3)
60-60: Extract magic number to a named constant.The value
100appears as a threshold for detecting recent user activity (lines 60, 79). Extracting this to a named constant likeACTIVITY_DETECTION_THRESHOLD_MSwould improve code clarity and maintainability.🔎 Suggested refactor
namespace tt::service::displayidle { constexpr auto* TAG = "DisplayIdle"; +constexpr uint32_t ACTIVITY_DETECTION_THRESHOLD_MS = 100; class DisplayIdleService final : public Service {Then use
ACTIVITY_DETECTION_THRESHOLD_MSinstead of the hardcoded100at lines 60 and 79.Also applies to: 79-79
67-84: Extract repeated keyboard brightness calculation.The expression
cachedKeyboardSettings.backlightEnabled ? cachedKeyboardSettings.backlightBrightness : 0is duplicated at lines 72, 80, and 117. Consider extracting it to a helper method to improve maintainability.🔎 Suggested refactor
Add a private helper method:
uint8_t getTargetKeyboardBrightness() const { return cachedKeyboardSettings.backlightEnabled ? cachedKeyboardSettings.backlightBrightness : 0; }Then replace the three occurrences with
getTargetKeyboardBrightness().Also applies to: 103-120
94-95: Consider implementing event-driven settings updates.The TODO comment notes that settings changes currently require a service restart. Implementing DisplaySettingsChanged/KeyboardSettingsChanged events would provide a better user experience by applying changes immediately.
Would you like me to open a new issue to track this enhancement, or would you prefer guidance on implementing an event-driven settings update mechanism?
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Data/data/settings/system.propertiesDevices/lilygo-tdeck/Source/Init.cppDevices/lilygo-tdeck/Source/devices/TrackballDevice.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/espnow/EspNowWifi.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Tactility/Source/service/espnow/EspNowWifi.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp
🧬 Code graph analysis (3)
Tactility/Source/app/keyboard/KeyboardSettings.cpp (4)
Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (2)
setBrightness(29-60)setBrightness(29-29)Drivers/Trackball/Source/Trackball.cpp (2)
setEnabled(142-145)setEnabled(142-142)Tactility/Source/settings/KeyboardSettings.cpp (4)
loadOrGetDefault(47-53)loadOrGetDefault(47-47)save(55-63)save(55-55)Tactility/Source/Tactility.cpp (2)
getMainDispatcher(335-337)getMainDispatcher(335-335)
Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp (1)
Drivers/Trackball/Source/Trackball.cpp (4)
init(75-124)init(75-75)deinit(126-133)deinit(126-126)
Devices/lilygo-tdeck/Source/Init.cpp (2)
Tactility/Source/app/keyboard/KeyboardSettings.cpp (1)
setBrightness(11-11)Tactility/Source/service/displayidle/DisplayIdle.cpp (1)
setBrightness(13-13)
🔇 Additional comments (10)
Data/data/settings/system.properties (1)
2-5: Timezone defaults are now consistent.Verified that both
system.properties(line 5) andTime.cpp(lines 48, 52) now default toAmerica/Los_Angeles. The previous inconsistency has been resolved.Note:
America/Los_Angelesrepresents a specific geographic region (Pacific Time). If this is intended as a generic US default, consider using a more central timezone (e.g.,America/Chicago) or allowing first-time user configuration to set the appropriate timezone.Devices/lilygo-tdeck/Source/devices/TrackballDevice.cpp (1)
4-26: LGTM!The initialization logic is correct: checks for existing initialization, configures GPIO pins for the T-Deck trackball, calls the driver init function, and properly manages the initialization state.
Devices/lilygo-tdeck/Source/Init.cpp (2)
39-89: LGTM! Past review concerns resolved.The deferred I2C initialization is well implemented:
- Timer is properly cleaned up at line 80 (resolves past memory leak concern)
- Null check with error logging at lines 84-88 (resolves past silent failure concern)
- Device start failures are logged appropriately
136-137: LGTM!The deferred I2C device initialization and keyboard settings application are appropriately integrated into the boot sequence.
Tactility/Source/app/keyboard/KeyboardSettings.cpp (3)
36-90: LGTM!The event handlers correctly update settings state, manage UI control states (enabled/disabled), and apply hardware changes via driver calls. The array bounds check at line 86 prevents out-of-bounds access.
93-189: LGTM!The UI construction properly initializes all controls from settings, sets up appropriate event handlers, and manages control states based on toggle switches. The brightness slider range (0-255) at line 132 ensures type safety for the uint8_t cast at line 51.
191-196: LGTM!The settings persistence pattern is correct: creating a copy before dispatching avoids lifetime issues, and only saving when
updatedis true prevents unnecessary I/O.Tactility/Source/service/displayidle/DisplayIdle.cpp (3)
103-120: LGTM: Clean shutdown logic.The
onStop()method properly stops the timer first, then restores both display and keyboard backlights if they were dimmed. The null checks and state restoration ensure a clean service shutdown.
123-127: LGTM: Standard service registration.The service manifest follows the expected pattern for service registration with proper ID and factory function.
1-10: LGTM: Well-structured service implementation.The includes, namespace organization, and helper methods are well-designed. Caching settings at startup to avoid file I/O in the timer callback is an excellent approach to prevent watchdog timeouts, as noted in the comments.
Also applies to: 16-34
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
208-219: Fix inconsistent indentation in theelseblock.Lines 217-218 are not properly indented within the
elseblock. They should be aligned with theifblock contents.🔎 Proposed fix
// If totalRuntime provided, show CPU percentage; otherwise just show state if (totalRuntime > 0) { float cpu_percent = (task.ulRunTimeCounter * 100.0f) / totalRuntime; lv_label_set_text_fmt(label, "%s: %.1f%%", name, cpu_percent); } else { - lv_label_set_text_fmt(label, "%s (%s)", name, getTaskState(task)); -} + lv_label_set_text_fmt(label, "%s (%s)", name, getTaskState(task)); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cppDevices/lilygo-tdeck/Source/devices/TrackballDevice.hDrivers/KeyboardBacklight/Source/KeyboardBacklight.cppTactility/Include/Tactility/hal/Device.hTactility/Source/app/systeminfo/SystemInfo.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp
- Devices/lilygo-tdeck/Source/devices/TrackballDevice.h
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (2)
Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (1)
Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (6)
init(17-28)init(17-17)setBrightness(30-61)setBrightness(30-30)getBrightness(93-95)getBrightness(93-93)
Tactility/Source/app/systeminfo/SystemInfo.cpp (4)
Tactility/Source/app/i2cscanner/I2cScanner.cpp (2)
optApp(70-77)optApp(70-70)Tactility/Source/app/power/Power.cpp (9)
optApp(23-30)optApp(23-23)app(45-50)app(139-141)app(139-139)app(143-184)app(143-143)AppContext(186-188)AppContext(186-186)Tactility/Source/service/loader/Loader.cpp (2)
getCurrentAppContext(306-314)getCurrentAppContext(306-306)Tactility/Source/app/App.cpp (2)
getCurrentAppContext(38-42)getCurrentAppContext(38-38)
🔇 Additional comments (15)
Tactility/Include/Tactility/hal/Device.h (1)
24-25: LGTM — newOtherdevice type is a reasonable catch-all.Adding the trailing comma to
Gpsand introducingOtherfor miscellaneous device types (e.g., trackball) is a clean approach. The conversion function inTactilityC/Source/tt_hal_device.cppalready has a default case, so it will safely handle any unsupported types. IfDevice::Type::Otheris exposed through the C API in the future, the conversion function will need to be updated with a correspondingDEVICE_TYPE_OTHERcase.Devices/lilygo-tdeck/Source/devices/KeyboardBacklight.cpp (3)
16-23: LGTM: Idempotent shutdown logic.The
stop()method correctly turns off the backlight before clearing the initialized flag, and the unconditionaltruereturn makes the method safely idempotent.
25-37: LGTM: Brightness control methods are correctly guarded.Both
setBrightness()andgetBrightness()appropriately check the initialization state before delegating to the driver layer. The const qualifier ongetBrightness()and the default return value of 0 for uninitialized state are both correct design choices.
5-14: The TODO about mutex protection is deferred appropriately given current usage patterns.The
start()method is called exactly once during single-threaded device initialization inInit.cpp:55, so theinitializedflag race condition is not a practical concern for this PR. Additionally, runtime brightness control inKeyboardSettings.cppcalls the driver directly (driver::keyboardbacklight::setBrightness()), bypassing the device wrapper entirely. The TODO can remain as a future refactoring item if the architecture evolves to support concurrent access to these methods.Tactility/Source/app/systeminfo/SystemInfo.cpp (11)
55-69: LGTM: PSRAM helper functions are well-implemented.The new helper functions follow the existing pattern and are properly gated for ESP_PLATFORM builds.
123-154: LGTM: Clean refactoring of memory bar creation.The
MemoryBarWidgetsstruct improves code organization by grouping related UI elements together.
156-182: LGTM: Memory bar update logic is correct.The refactored function properly uses the
MemoryBarWidgetsstructure and displays memory information accurately.
221-251: LGTM: Robust task list rendering with good error handling.The PSRAM-first allocation strategy with fallback to internal RAM is appropriate, and the error handling prevents crashes when memory is exhausted.
275-285: LGTM: Follows established codebase pattern.The
optApp()helper correctly follows the same pattern used in other apps like Power and I2cScanner.
287-318: LGTM: Timer and UI state management is well-organized.The timer callbacks correctly use
optApp()to safely access the app instance, and the member variables are clearly organized.
329-356: LGTM: Storage update logic is clear and well-structured.The method properly queries storage information and updates UI elements only when storage is available.
358-412: LGTM: Task and CPU metrics are correctly calculated.The uptime calculation now correctly uses
xTaskGetTickCount()as suggested in previous reviews. The CPU usage calculation properly accounts for IDLE task time across cores.
414-570: LGTM: Comprehensive PSRAM information display.The detailed PSRAM metrics and explanations provide valuable insight into memory allocation behavior, which is appropriate for a system information app.
572-692: LGTM: Well-structured UI initialization and timer setup.The tab creation, initial updates, and timer configuration are logically organized. The timer intervals (10s for memory, 15s for tasks) are appropriate for system monitoring.
694-697: LGTM: Proper timer cleanup on hide.The timers are correctly stopped when the app is hidden, preventing unnecessary updates and resource usage.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
Devices/lilygo-tdeck/Source/Init.cpp (2)
70-79: Fix inconsistent indentation.These lines have extra leading spaces that break the consistent indentation pattern of the surrounding code.
🔎 Proposed fix
- auto kbBacklight = tt::hal::findDevice("Keyboard Backlight"); - if (kbBacklight != nullptr) { - TT_LOG_I(TAG, "%s starting", kbBacklight->getName().c_str()); - auto kbDevice = std::static_pointer_cast<KeyboardBacklightDevice>(kbBacklight); - if (kbDevice->start()) { - TT_LOG_I(TAG, "%s started", kbBacklight->getName().c_str()); - } else { - TT_LOG_E(TAG, "%s start failed", kbBacklight->getName().c_str()); - } - } - - auto trackball = tt::hal::findDevice("Trackball"); - if (trackball != nullptr) { - TT_LOG_I(TAG, "%s starting", trackball->getName().c_str()); - auto tbDevice = std::static_pointer_cast<TrackballDevice>(trackball); - if (tbDevice->start()) { - TT_LOG_I(TAG, "%s started", trackball->getName().c_str()); - } else { - TT_LOG_E(TAG, "%s start failed", trackball->getName().c_str()); - } - } + auto kbBacklight = tt::hal::findDevice("Keyboard Backlight"); + if (kbBacklight != nullptr) { + TT_LOG_I(TAG, "%s starting", kbBacklight->getName().c_str()); + auto kbDevice = std::static_pointer_cast<KeyboardBacklightDevice>(kbBacklight); + if (kbDevice->start()) { + TT_LOG_I(TAG, "%s started", kbBacklight->getName().c_str()); + } else { + TT_LOG_E(TAG, "%s start failed", kbBacklight->getName().c_str()); + } + } + + auto trackball = tt::hal::findDevice("Trackball"); + if (trackball != nullptr) { + TT_LOG_I(TAG, "%s starting", trackball->getName().c_str()); + auto tbDevice = std::static_pointer_cast<TrackballDevice>(trackball); + if (tbDevice->start()) { + TT_LOG_I(TAG, "%s started", trackball->getName().c_str()); + } else { + TT_LOG_E(TAG, "%s start failed", trackball->getName().c_str()); + } + }Also applies to: 82-89
69-90: Consider storing device start results to avoid applying settings to uninitialized devices.The code checks device start results and logs failures, then unconditionally calls
setBrightness()andsetEnabled()on lines 96 and 101. WhilesetBrightness()result is checked and logged as a warning, a cleaner pattern would store the start results and only apply settings if the devices initialized successfully. This avoids relying on internalinitializedstate checks within the settings methods.Drivers/Trackball/Source/Trackball.cpp (1)
135-140: Log a warning when movementStep is invalid.The function silently ignores
step == 0without logging. This could make debugging difficult if called with an invalid value.🔎 Proposed fix
void setMovementStep(uint8_t step) { if (step > 0) { g_config.movementStep = step; ESP_LOGD(TAG, "Movement step set to %d", step); + } else { + ESP_LOGW(TAG, "Invalid movement step %d ignored (must be > 0)", step); } }Tactility/Source/app/files/View.cpp (1)
194-223: Consider extracting file size formatting to a helper function.The file size display logic is functional and correct, but spans 30 lines within
createDirEntryWidget. Extracting this to a helper function (e.g.,formatFileSize) would improve readability and make the widget creation logic clearer.Example refactor
// Helper function (could be static or in anonymous namespace) std::string formatFileSize(const std::string& file_path) { struct stat st; if (stat(file_path.c_str(), &st) != 0) { return ""; } const char* size_suffix; double size; if (st.st_size < 1024) { size = st.st_size; size_suffix = " B"; } else if (st.st_size < 1024 * 1024) { size = st.st_size / 1024.0; size_suffix = " KB"; } else { size = st.st_size / (1024.0 * 1024.0); size_suffix = " MB"; } char size_str[32]; if (st.st_size < 1024) { snprintf(size_str, sizeof(size_str), " (%d%s)", (int)size, size_suffix); } else { snprintf(size_str, sizeof(size_str), " (%.1f%s)", size, size_suffix); } return size_str; } // Then in createDirEntryWidget: std::string label_text = dir_entry.d_name; if (dir_entry.d_type == file::TT_DT_REG) { std::string file_path = state->getCurrentPath() + "/" + dir_entry.d_name; label_text += formatFileSize(file_path); }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Devices/lilygo-tdeck/Source/Init.cppDrivers/KeyboardBacklight/Source/KeyboardBacklight.cppDrivers/Trackball/Source/Trackball.cppTactility/Source/app/files/View.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/settings/KeyboardSettings.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp
- Tactility/Source/settings/KeyboardSettings.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-11-02T15:49:07.895Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 406
File: Buildscripts/CDN/devices.properties:0-0
Timestamp: 2025-11-02T15:49:07.895Z
Learning: In the ByteWelder/Tactility repository, alphabetical ordering issues (such as entries in Buildscripts/CDN/devices.properties) and similar organizational/formatting concerns should be marked as "minor" severity rather than "major" severity.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/files/View.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (3)
Devices/lilygo-tdeck/Source/Init.cpp (5)
Tactility/Source/app/keyboard/KeyboardSettings.cpp (4)
event(80-90)event(80-80)setBrightness(11-11)setEnabled(15-15)Tactility/Source/settings/KeyboardSettings.cpp (2)
loadOrGetDefault(47-53)loadOrGetDefault(47-47)Tactility/Include/Tactility/app/App.h (1)
result(25-75)Drivers/KeyboardBacklight/Source/KeyboardBacklight.cpp (2)
setBrightness(37-68)setBrightness(37-37)Drivers/Trackball/Source/Trackball.cpp (2)
setEnabled(142-145)setEnabled(142-142)
Tactility/Source/app/systeminfo/SystemInfo.cpp (5)
Tactility/Source/hal/Hal.cpp (2)
getConfiguration(95-97)getConfiguration(95-95)Tactility/Source/app/power/Power.cpp (9)
optApp(23-30)optApp(23-23)app(45-50)app(139-141)app(139-139)app(143-184)app(143-143)AppContext(186-188)AppContext(186-186)Tactility/Source/service/loader/Loader.cpp (2)
getCurrentAppContext(299-307)getCurrentAppContext(299-299)Tactility/Source/app/App.cpp (2)
getCurrentAppContext(38-42)getCurrentAppContext(38-38)Tactility/Source/lvgl/LvglSync.cpp (2)
getSyncLock(55-57)getSyncLock(55-55)
Tactility/Source/app/files/View.cpp (2)
Tactility/Source/app/inputdialog/InputDialog.cpp (4)
start(23-29)start(23-23)getResult(31-35)getResult(31-31)TactilityCore/Source/file/File.cpp (4)
getChildPath(42-49)getChildPath(42-42)getLock(24-36)getLock(24-24)
🔇 Additional comments (17)
Devices/lilygo-tdeck/Source/Init.cpp (2)
7-12: Includes look good.The new includes properly support keyboard backlight and trackball functionality.
101-101: Trackball enablement looks correct.The trackball enabled state is properly loaded from settings and applied.
Drivers/Trackball/Source/Trackball.cpp (6)
1-17: Global state management looks reasonable.The static globals for configuration, state tracking, and LVGL device handle are appropriately scoped for a driver implementation.
43-58: movementStep is now properly applied.The previous review flagged that
movementStepwas configured but not used. This has been fixed—the movement step is now correctly multiplied into the encoder diff calculations (lines 45, 49, 53, 57).
75-124: Initialization logic is solid.The init function properly:
- Guards against double initialization
- Sets a reasonable default for movementStep (10 if unspecified)
- Configures GPIOs as active-low with pull-ups
- Captures initial state to enable edge detection
- Registers with LVGL as an encoder device
- Handles and logs errors appropriately
126-133: Proper cleanup in deinit.The deinitialization correctly cleans up the LVGL input device and resets state.
142-145: setEnabled implementation is correct.The function properly updates the enabled flag and logs the state change.
69-72: No action required—lv_disp_trig_activity(nullptr)is correct. The LVGL API acceptsNULL/nullptrto trigger activity on the default (most-recently-created) display, which is the intended behavior for this wake-on-trackball implementation.Tactility/Source/app/files/View.cpp (3)
67-75: LGTM! Callbacks follow established patterns.The new callback functions for file and folder creation follow the same structure as existing callbacks in this file. Implementation is clean and consistent.
258-268: LGTM! New methods follow established patterns.The
onNewFilePressedandonNewFolderPressedmethods correctly set the pending action state and launch the input dialog, consistent with similar methods likeonRenamePressed.
320-321: LGTM! Toolbar buttons integrated correctly.The new file and folder creation buttons are properly added to the toolbar with appropriate symbols and callbacks.
Tactility/Source/app/systeminfo/SystemInfo.cpp (6)
3-3: LGTM: Appropriate includes for new functionality.The added includes support LVGL synchronization, periodic timers, and ESP heap capabilities—all necessary for the PSRAM monitoring and periodic update features.
Also applies to: 8-8, 16-16
56-70: LGTM: PSRAM metrics helpers follow established patterns.The new
getPsramMinFreeandgetPsramLargestBlockfunctions correctly use ESP-IDF heap capabilities APIs and provide reasonable fallback values for non-ESP platforms, consistent with existing memory helper functions.
124-183: LGTM: Memory bar widget refactoring improves encapsulation.The
MemoryBarWidgetsstruct and refactoredcreateMemoryBar/updateMemoryBarfunctions provide cleaner separation between widget creation and updates. The scale-aware padding and detailed label formatting enhance the UI presentation.
205-252: LGTM: Task rendering logic with PSRAM-aware allocation.The refactored task handling correctly implements:
- PSRAM allocation with internal RAM fallback for task lists
- CPU usage sorting when displaying percentages
- Proper error handling and memory cleanup
- Conditional display (CPU % vs. state) based on context
The PSRAM allocation strategy efficiently uses external RAM while maintaining reliability through fallback.
276-286: LGTM: Safe app instance retrieval pattern.The
optApphelper correctly implements the pattern for retrieving the running app instance with proper type checking, matching the established pattern used elsewhere in the codebase (e.g., Power.cpp). This enables timer callbacks to safely access app methods.
575-700: LGTM: Proper app lifecycle management.The
onShowmethod correctly:
- Constructs a multi-tab UI with conditional content based on hardware capabilities
- Performs initial data population via update methods (which execute in LVGL context here)
- Starts periodic timers with reasonable intervals (10s for memory/PSRAM, 15s for tasks/CPU)
The
onHidemethod properly stops both timers to prevent updates when the app is not visible.Note: The update methods called from
onShow(lines 687-690) execute in LVGL context so they're safe here, but they still require explicit locking when invoked from timer callbacks (as flagged in previous comments).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Tactility/Source/app/files/View.cpp (2)
442-467: Critical: Lock leak when folder already exists.The lock acquired on line 448 is never released when the existence check on line 451 succeeds. The
breakstatement on line 453 exits the switch before reaching theunlock()call on line 461, causing a resource leak that will prevent further operations on this path.🔎 Proposed fix to release lock before breaking
auto lock = file::getLock(new_folder_path); lock->lock(); struct stat st; if (stat(new_folder_path.c_str(), &st) == 0) { TT_LOG_W(TAG, "Folder already exists: \"%s\"", new_folder_path.c_str()); + lock->unlock(); break; } if (mkdir(new_folder_path.c_str(), 0755) == 0) {
414-441: Critical: Lock leak when file already exists.The lock acquired on line 420 is never released when the existence check on line 423 succeeds. The
breakstatement on line 425 exits the switch before reaching theunlock()call on line 435, causing a resource leak that will prevent further operations on this path.🔎 Proposed fix to release lock before breaking
auto lock = file::getLock(new_file_path); lock->lock(); struct stat st; if (stat(new_file_path.c_str(), &st) == 0) { TT_LOG_W(TAG, "File already exists: \"%s\"", new_file_path.c_str()); + lock->unlock(); break; } FILE* new_file = fopen(new_file_path.c_str(), "w");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tactility/Source/app/files/View.cpp
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/files/View.cpp
🧬 Code graph analysis (1)
Tactility/Source/app/files/View.cpp (1)
TactilityCore/Source/file/File.cpp (4)
getChildPath(42-49)getChildPath(42-42)getLock(24-36)getLock(24-24)
🔇 Additional comments (4)
Tactility/Source/app/files/View.cpp (4)
18-20: LGTM!The header additions are appropriate for the new file/folder creation and file size display features.
67-75: LGTM!The callback implementations follow the established pattern used throughout the file and correctly forward to the corresponding handler methods.
258-268: LGTM!The handler methods follow the established pattern used by other action handlers like
onRenamePressed()and correctly set the pending action state before launching the input dialog.
320-321: LGTM!The toolbar button additions follow the established pattern and correctly wire the UI actions to the corresponding callbacks.
correct locks? i don't know at this point. I'm trying!
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Tactility/Source/app/files/View.cpp (1)
442-467: Lock leak when folder already exists.Good work adding the existence check to provide clearer error messages as suggested in previous reviews! However, line 453 breaks out of the switch without calling
lock->unlock(), causing a lock leak if the folder already exists.🔎 Proposed fix to unlock before break
struct stat st; if (stat(new_folder_path.c_str(), &st) == 0) { TT_LOG_W(TAG, "Folder already exists: \"%s\"", new_folder_path.c_str()); + lock->unlock(); break; }Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
338-365: Missing LVGL synchronization in updateStorage.
updateStorage()performs LVGL operations (updateMemoryBarcallslv_bar_set_range,lv_bar_set_value,lv_label_set_text_fmt) without acquiring the LVGL lock. While currently only called fromonShow(line 694), it should either acquire the lock itself or clearly document that the caller must hold it.🔎 Proposed fix: add scoped lock
void updateStorage() { #ifdef ESP_PLATFORM + auto lock = lvgl::getSyncLock()->asScopedLock(); + lock.lock(); + uint64_t storage_total = 0; uint64_t storage_free = 0;Based on learnings and past review comments, consistent LVGL synchronization prevents race conditions.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Tactility/Source/app/files/View.cppTactility/Source/app/systeminfo/SystemInfo.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/files/View.cppTactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (1)
Tactility/Source/app/files/View.cpp (2)
Tactility/Source/app/inputdialog/InputDialog.cpp (4)
start(23-29)start(23-23)getResult(31-35)getResult(31-31)Tactility/Source/lvgl/LvglSync.cpp (2)
lock(31-33)lock(31-31)
🔇 Additional comments (12)
Tactility/Source/app/files/View.cpp (5)
18-20: LGTM!The new includes are necessary for the file operations (fopen, stat, mkdir) used in the create-file/create-folder features and file size display.
67-75: LGTM!The callback functions follow the established LVGL pattern used throughout the file and correctly forward events to the View instance methods.
194-223: Good enhancement with correct path handling.The file size display is a nice usability improvement. The use of
file::getChildPath(line 198) correctly addresses the previous review concern about path construction at root directory.The human-readable size formatting logic is correct and safe.
258-268: LGTM!The handler methods follow the established pattern for action initiation (set pending action, start input dialog) and are consistent with similar methods like
onRenamePressed().
320-321: LGTM!The toolbar button initialization follows the established pattern and correctly wires the new callbacks.
Tactility/Source/app/systeminfo/SystemInfo.cpp (7)
3-3: LGTM!The new includes are appropriate for the LVGL synchronization, timer functionality, and PSRAM heap capabilities added in this file.
Also applies to: 8-8, 16-16
56-70: LGTM!The PSRAM helper functions correctly use ESP-IDF heap capabilities APIs and include appropriate platform guards with sensible fallback values.
124-183: LGTM!The refactoring to use
MemoryBarWidgetsstruct improves code organization by grouping related bar and label widgets, and all functions are updated consistently.
205-252: LGTM!The updated task handling includes good improvements:
- PSRAM allocation with proper fallback to internal RAM
- Error handling for allocation failures
- Conditional sorting by CPU usage
- Flexible display of CPU percentage vs. task state
367-421: LGTM!The
updateTasks()andupdatePsram()methods correctly rely on their timer callbacks to acquire the LVGL lock before invocation. The PSRAM allocation patterns with fallback to internal RAM are well-implemented, and the detailed PSRAM metrics and configuration display is informative.Also applies to: 423-579
581-707: LGTM with minor concern.The lifecycle methods correctly initialize the multi-tab UI, start/stop timers appropriately, and call update methods during initialization. The timer intervals (10s for memory, 15s for tasks) are reasonable for system monitoring.
Note: Initial update calls on lines 693-696 will work correctly if
onShowruns in LVGL context, butupdateStorageneeds synchronization (flagged separately).
326-336: Remove nested lock acquisition causing deadlock or redundancy.The timer callback (line 292) already acquires the LVGL lock before calling
updateMemory(). The nestedlock()/unlock()calls on lines 332-334 will either:
- Deadlock if the lock is non-recursive
- Be redundant and confusing if the lock is recursive
Remove the nested lock acquisition since the caller already holds the lock.
🔎 Proposed fix: remove nested lock
void updateMemory() { - // Timer callbacks run in a task context (not ISR), so we can use LVGL lock - // Don't use kernel::lock() from timer callbacks - causes scheduler assertion updateMemoryBar(internalMemBar, getHeapFree(), getHeapTotal()); if (hasExternalMem) { - lvgl::getSyncLock()->lock(); updateMemoryBar(externalMemBar, getSpiFree(), getSpiTotal()); - lvgl::getSyncLock()->unlock(); } }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Tactility/Source/app/files/View.cpp (1)
414-469: Well done addressing the past review feedback!Both action handlers now correctly:
- Check for existence before creating (avoiding silent overwrites)
- Release the lock in all code paths (fixing the previously flagged lock leak)
The implementation is correct and follows existing patterns in the file.
For future maintainability, consider using an RAII-style scoped lock to ensure the lock is always released, even if new code paths are added later:
🔎 Optional: RAII lock pattern
// Example using a scope guard or unique_lock pattern: class ScopedFileLock { std::shared_ptr<file::FileLock> lock_; public: explicit ScopedFileLock(const std::string& path) : lock_(file::getLock(path)) { lock_->lock(); } ~ScopedFileLock() { lock_->unlock(); } }; // Usage: { ScopedFileLock guard(new_file_path); // ... file operations ... } // automatically unlocksThis is optional since the current manual lock/unlock pattern is consistent with existing code in this file.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Tactility/Source/app/files/View.cppTactility/Source/app/systeminfo/SystemInfo.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/files/View.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp (5)
Tactility/Source/hal/Hal.cpp (2)
getConfiguration(95-97)getConfiguration(95-95)Tactility/Source/Tactility.cpp (2)
getConfiguration(331-333)getConfiguration(331-331)Tactility/Source/service/loader/Loader.cpp (2)
getCurrentAppContext(299-307)getCurrentAppContext(299-299)Tactility/Source/app/App.cpp (2)
getCurrentAppContext(38-42)getCurrentAppContext(38-38)Tactility/Source/lvgl/LvglSync.cpp (2)
lock(31-33)lock(31-31)
🔇 Additional comments (6)
Tactility/Source/app/files/View.cpp (5)
18-20: LGTM!The added headers are necessary for the new file/folder creation functionality (
<cstdio>forfopen/fclose,<sys/stat.h>forstat/mkdir).
67-76: LGTM!The new callbacks follow the established pattern used by other callbacks in this file.
194-225: Nice improvement for UX!The file size display enhancement is well-implemented. Uses
file::getChildPath()for path construction as suggested in past review, and gracefully handlesstatfailures by simply omitting the size suffix.
258-268: LGTM!The new methods follow the established pattern of
onRenamePressedandonDeletePressed.
320-321: LGTM!Good choice of symbols (
LV_SYMBOL_FILEandLV_SYMBOL_DIRECTORY) for intuitive UI. The button references are stored following the same pattern asnavigate_up_button.Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
289-306: This pattern is correct and intentional. TheScopedLockclass is designed to require manuallock()after construction while guaranteeing automaticunlock()on destruction. The documentation in Lock.h explicitly states this design: "You have to lock() this object manually, but unlock() happens automatically on destruction."There is no exception-safety gap because
lock()returns aboolrather than throwing exceptions, and the constructor doesn't perform locking. This is a valid RAII pattern that ensures cleanup regardless of the lock() result. The pattern is used consistently throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Tactility/Source/app/systeminfo/SystemInfo.cpp (2)
420-576: Consider extracting helper functions for readability.This function creates many UI elements and is quite long (~150 lines). While functionally correct and now properly synchronized via the timer callback's scoped lock, consider extracting logical sections into smaller helper functions (e.g.,
createPsramSummarySection,createPsramConfigSection,createPsramStrategySection) to improve maintainability.
648-677: Placeholder text mismatch.Lines 656 and 659 set initial placeholder text with
--.-%format, but these labels show task counts and uptime (not percentages). Consider using more appropriate placeholders.🔎 Proposed fix
taskCountLabel = lv_label_create(cpu_tab); - lv_label_set_text(taskCountLabel, "Active Tasks: --.-%"); + lv_label_set_text(taskCountLabel, "Active Tasks: --"); uptimeLabel = lv_label_create(cpu_tab); - lv_label_set_text(uptimeLabel, "System Uptime: --.-%"); + lv_label_set_text(uptimeLabel, "System Uptime: --");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp (5)
Tactility/Source/Tactility.cpp (2)
getConfiguration(331-333)getConfiguration(331-331)Tactility/Source/app/i2cscanner/I2cScanner.cpp (2)
optApp(70-77)optApp(70-70)Tactility/Source/app/power/Power.cpp (9)
optApp(23-30)optApp(23-23)app(45-50)app(139-141)app(139-139)app(143-184)app(143-143)AppContext(186-188)AppContext(186-186)Tactility/Source/app/App.cpp (2)
getCurrentAppContext(38-42)getCurrentAppContext(38-38)Tactility/Source/lvgl/LvglSync.cpp (2)
getSyncLock(55-57)getSyncLock(55-55)
🔇 Additional comments (11)
Tactility/Source/app/systeminfo/SystemInfo.cpp (11)
1-19: LGTM!The necessary includes are now in place, including
<algorithm>forstd::sort(addressing the previous review comment),<Tactility/lvgl/LvglSync.h>for LVGL synchronization, and<esp_heap_caps.h>for PSRAM-related functions.
57-72: LGTM!The new PSRAM helper functions follow the same pattern as existing memory functions with proper ESP_PLATFORM guards and reasonable fallback values.
125-156: LGTM!Clean widget abstraction with the
MemoryBarWidgetsstruct. The UI scale handling for padding is a nice touch for different display sizes.
158-184: LGTM!The function correctly handles scaling large values to fit
int32_tfor LVGL bars and properly formats the label with both human-readable and raw byte values.
223-253: LGTM!The PSRAM-first allocation with internal RAM fallback is a good memory optimization. The conditional sorting by CPU usage when
showCpuPercentis true provides a better UX for the CPU tab.
290-307: LGTM!The timer callbacks now correctly use RAII-style scoped locking with
asScopedLock(), addressing the previous review comment about exception safety.
327-333: LGTM!The redundant LVGL lock has been removed (addressing the previous review comment). The function now correctly relies on the caller (timer callback) to hold the lock.
335-362: Clarification: LVGL lock context forupdateStorage.This function is currently only called from
onShow(line 691), which runs in the LVGL UI context where the lock is implicitly held by the framework. If this function were to be called from a timer callback in the future, it would need explicit locking. Consider adding a brief comment noting that callers must hold the LVGL lock.
364-418: LGTM!The uptime calculation now correctly uses
xTaskGetTickCount()instead of the runtime stats counter (addressing the previous review comment). The CPU usage calculation by subtracting idle task time is a reasonable approach for overall utilization.
700-703: LGTM!Proper cleanup by stopping timers when the app is hidden, preventing unnecessary updates and potential issues with stale widget pointers.
277-287: LGTM!The
optApp()helper follows the same pattern used in other apps likePower.cpp, providing a safe way to access the app instance from timer callbacks.
|
Finally it has the green tick in the PR list! 😂 |
very usable now.
|
Guys finish this fast i'm excited! When new version will be released? |
It'll be finished when it's finished. 😜 |
|
Tagging @cscott0108 and https://github.com/cscott0108/tactility-enhanced-t-deck for visibility and credit. |
Yes i should have done that at the start. |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
378-379: Same misleading malloc fallback pattern as in updateRtosTasks.Lines 378-379 repeat the same problematic pattern as lines 228-231: calling
malloc()twice doesn't provide a meaningful fallback between PSRAM and internal RAM, sincemalloc()already handles this automatically.Apply the same fix as suggested for
updateRtosTasks(lines 228-237): either remove the redundant secondmalloc()call, or use explicitheap_caps_malloc()withMALLOC_CAP_SPIRAMfollowed byMALLOC_CAP_INTERNAL.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Tactility/CMakeLists.txtTactility/Source/app/systeminfo/SystemInfo.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Tactility/CMakeLists.txt
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🔇 Additional comments (9)
Tactility/Source/app/systeminfo/SystemInfo.cpp (9)
1-20: LGTM! Well-organized includes.All new includes are appropriate for the added functionality: LvglSync for synchronization, Timer for periodic updates, algorithm for task sorting, cstring for name comparisons, and esp_heap_caps for PSRAM metrics.
58-72: LGTM! PSRAM metrics helpers are well-structured.The new
getPsramMinFree()andgetPsramLargestBlock()functions follow the established pattern for memory capability queries and provide appropriate fallback values for non-ESP platforms.
126-157: LGTM! Clean refactoring to encapsulate memory bar components.The
MemoryBarWidgetsstruct groups related UI elements, andcreateMemoryBarnow properly handles both the bar and its associated label. The UI scale-aware padding adjustment demonstrates good attention to responsive design.
159-185: LGTM! updateMemoryBar refactored cleanly.The updated signature using
MemoryBarWidgetsis cleaner, and displaying both converted units and raw byte counts provides valuable diagnostic information.
277-287: LGTM! Safe app instance resolution.The
optApp()helper provides type-safe access to the current app instance with proper null checks, enabling timer callbacks to safely invoke update methods.
290-307: LGTM! Timer callbacks properly synchronized.Both timer callbacks correctly use RAII-style scoped locks (
asScopedLock()) to protect LVGL operations, ensuring exception-safe synchronization. This addresses the critical issues flagged in previous reviews.
420-576: LGTM! Comprehensive and educational PSRAM diagnostics.The PSRAM tab provides excellent visibility into memory usage with detailed metrics (current, peak, min free, largest block) and clearly explains the allocation strategy based on sdkconfig thresholds. The narrative sections help users understand LVGL dynamic allocation and app memory behavior.
578-698: LGTM! Well-structured UI initialization and update lifecycle.The
onShowmethod cleanly organizes tab creation, conditional UI elements based on available hardware (PSRAM, storage types), and schedules periodic updates at appropriate intervals (10s for memory/PSRAM, 15s for tasks/CPU). Initial updates ensure immediate data population.
700-703: LGTM! Proper resource cleanup.Stopping timers in
onHideensures no unnecessary background work when the app is not visible, conserving resources.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
374-376: Remove redundant malloc fallback (duplicate issue).Lines 375-376 contain a redundant malloc pattern: calling
malloc()twice with identical arguments. If the first allocation fails, the second identical call will not succeed. This pattern was flagged in a previous review, and the maintainer confirmed that Option 1 (single malloc with cast) is the correct approach.🔎 Recommended fix (as confirmed by maintainer)
UBaseType_t count = uxTaskGetNumberOfTasks(); - auto* tasks = malloc(sizeof(TaskStatus_t) * count); - if (!tasks) tasks = (TaskStatus_t*)malloc(sizeof(TaskStatus_t) * count); + auto* tasks = (TaskStatus_t*)malloc(sizeof(TaskStatus_t) * count); if (tasks) {
🧹 Nitpick comments (5)
Tactility/Source/app/systeminfo/SystemInfo.cpp (1)
228-228: Consider explicit cast for consistency.The malloc result is implicitly converted to
TaskStatus_t*. While valid in C++, an explicit cast(TaskStatus_t*)malloc(...)would be more consistent with line 376 and improve clarity.🔎 Proposed change
- auto* tasks = malloc(sizeof(TaskStatus_t) * count); + auto* tasks = (TaskStatus_t*)malloc(sizeof(TaskStatus_t) * count);Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (4)
21-21: Consider removing the unused mutex member.The
RecursiveMutex mutexmember is declared but never used. The code correctly useslvgl::lock()for LVGL thread safety inonResultinstead. This appears to be leftover from the refactoring.🔎 Proposed fix
- RecursiveMutex mutex; lv_obj_t* timeZoneLabel = nullptr; lv_obj_t* dateFormatDropdown = nullptr;
35-47: Add defensive bounds checking for array access.While
lv_dropdown_get_selectedshould return a valid index (0-3) for the 4-option dropdown, adding a bounds check before accessing thedateFormatsarray would make the code more robust against unexpected behavior.🔎 Proposed fix
static void onDateFormatChanged(lv_event_t* event) { auto* dropdown = static_cast<lv_obj_t*>(lv_event_get_target(event)); auto index = lv_dropdown_get_selected(dropdown); const char* dateFormats[] = {"MM/DD/YYYY", "DD/MM/YYYY", "YYYY-MM-DD", "YYYY/MM/DD"}; + if (index < 0 || index >= 4) { + TT_LOG_E(TAG, "Invalid dropdown index: %d", index); + return; + } std::string selected_format = dateFormats[index]; settings::SystemSettings sysSettings; if (settings::loadSystemSettings(sysSettings)) { sysSettings.dateFormat = selected_format; settings::saveSystemSettings(sysSettings); } }
100-107: Consider extracting the format-to-index mapping.The if-else chain that maps date format strings to dropdown indices could be more maintainable if extracted into a helper function or if the logic were inverted (using the same array as
onDateFormatChanged).🔎 Example refactor
+ // Helper to find index of date format + auto findFormatIndex = [](const std::string& format) -> int { + const char* dateFormats[] = {"MM/DD/YYYY", "DD/MM/YYYY", "YYYY-MM-DD", "YYYY/MM/DD"}; + for (int i = 0; i < 4; i++) { + if (format == dateFormats[i]) return i; + } + return 0; // default to MM/DD/YYYY + }; + settings::SystemSettings sysSettings; if (settings::loadSystemSettings(sysSettings)) { - int index = 0; - if (sysSettings.dateFormat == "DD/MM/YYYY") index = 1; - else if (sysSettings.dateFormat == "YYYY-MM-DD") index = 2; - else if (sysSettings.dateFormat == "YYYY/MM/DD") index = 3; - lv_dropdown_set_selected(dateFormatDropdown, index); + lv_dropdown_set_selected(dateFormatDropdown, findFormatIndex(sysSettings.dateFormat)); }
128-133: The empty timezone check is unnecessary.Based on the
getTimeZoneName()implementation inTactility/Source/settings/Time.cpp(lines 41-49), the function returns a default value of"America/Los_Angeles"when no preference is set, sotimeZoneName.empty()will never be true.🔎 Proposed simplification
timeZoneLabel = lv_label_create(timezone_button); std::string timeZoneName = settings::getTimeZoneName(); - if (timeZoneName.empty()) { - timeZoneName = "not set"; - } lv_obj_center(timeZoneLabel); lv_label_set_text(timeZoneLabel, timeZoneName.c_str());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Tactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timedatesettings/TimeDateSettings.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Tactility/Source/app/localesettings/LocaleSettings.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧬 Code graph analysis (1)
Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (3)
Tactility/Source/settings/Time.cpp (6)
setTimeFormat24Hour(71-79)setTimeFormat24Hour(71-71)getTimeZoneName(42-50)getTimeZoneName(42-42)setTimeZone(29-40)setTimeZone(29-29)Tactility/Source/app/timezone/TimeZone.cpp (9)
start(237-239)start(237-237)index(98-109)index(98-98)app(74-77)app(186-220)app(186-186)app(222-226)app(222-222)Tactility/Source/settings/SystemSettings.cpp (4)
loadSystemSettings(66-76)loadSystemSettings(66-66)saveSystemSettings(78-95)saveSystemSettings(78-78)
🔇 Additional comments (1)
Tactility/Source/app/timedatesettings/TimeDateSettings.cpp (1)
136-150: LGTM! Proper LVGL thread safety pattern.The
onResultimplementation correctly handles the timezone picker result with proper LVGL locking. The timeout-based lock acquisition and the empty name check are good defensive programming practices.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Tactility/Source/app/systeminfo/SystemInfo.cpp
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-10-27T22:33:23.840Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 394
File: Boards/WaveshareEsp32C6Lcd147/Source/WaveshareEsp32C6Lcd147.cpp:49-50
Timestamp: 2025-10-27T22:33:23.840Z
Learning: In the Tactility project, when configuring SPI for displays that use esp_lvgl_port, the `.lock` field must be set to `tt::lvgl::getSyncLock()` rather than `tt::hal::spi::getLock()`, because esp_lvgl_port creates and manages its own synchronization lock for display operations.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-28T05:42:03.394Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 398
File: Boards/LilygoTdisplayS3/Source/devices/Display.cpp:183-199
Timestamp: 2025-10-28T05:42:03.394Z
Learning: The LilyGo T-Display S3 board (Boards/LilygoTdisplayS3) requires a non-standard LVGL flush pattern in its ST7789 i8080 display driver. In st7789_send_color_cb, lv_display_flush_ready() must be called immediately after esp_lcd_panel_io_tx_color() with a vTaskDelay(1), rather than using standard DMA completion callbacks via esp_lcd_panel_io_register_event_callbacks. Standard DMA callback approaches cause display corruption on this specific board. This pattern was tested against multiple alternatives (callback registration, buffer tuning, clock reduction) and is the only stable solution for T-Display S3 hardware.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-11-01T19:57:58.233Z
Learnt from: NellowTCS
Repo: ByteWelder/Tactility PR: 405
File: Tactility/Source/hal/usb/UsbTusb.cpp:12-12
Timestamp: 2025-11-01T19:57:58.233Z
Learning: In ESP-IDF projects like Tactility, the wear levelling header can be included directly as `#include <wear_levelling.h>` without requiring a directory prefix like `wear_levelling/wear_levelling.h`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-26T12:44:21.858Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 391
File: Boards/CYD-2432S028RV3/Source/devices/Display.h:11-19
Timestamp: 2025-10-26T12:44:21.858Z
Learning: In the Tactility codebase, prefer `constexpr auto` for constant declarations (such as LCD pin configurations, resolutions, and buffer sizes) rather than explicit type annotations like `gpio_num_t`, `spi_host_device_t`, `uint16_t`, or `size_t`.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
📚 Learning: 2025-10-27T22:52:15.539Z
Learnt from: KenVanHoeylandt
Repo: ByteWelder/Tactility PR: 395
File: Boards/M5stackCardputerAdv/Source/devices/CardputerKeyboard.cpp:145-153
Timestamp: 2025-10-27T22:52:15.539Z
Learning: In the Tactility codebase, for LVGL device lifecycle methods (e.g., stopLvgl(), startLvgl()), the maintainer prefers using assert() to catch double-calls and programming errors rather than adding defensive null-checks. The intent is to crash on misuse during development rather than silently handle it, avoiding the overhead of defensive checks in every driver.
Applied to files:
Tactility/Source/app/systeminfo/SystemInfo.cpp
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: BuildSdk (generic-esp32c6, esp32c6)
- GitHub Check: BuildSdk (generic-esp32p4, esp32p4)
- GitHub Check: macOS
- GitHub Check: BuildSdk (generic-esp32s3, esp32s3)
- GitHub Check: BuildSdk (generic-esp32, esp32)
- GitHub Check: Linux
- GitHub Check: run
3 participants
Lots of things "ported" over from the "enhanced" fork. With some adjustments here and there.
KeyboardBacklight driver
Trackball driver
Display sleep/wake
Files - create file/folder
Keyboard settings
Time & Date settings tweaks
Locale settings tweaks
Systeminfo additions
Espnow wifi coexist
initI2cDevices - moved to T-deck init.cpp / initBoot
KeyboardInitService - removed, moved to T-deck init.cpp / initBoot
Adjusted TIMER_UPDATE_INTERVAL to 2 seconds.
Added lock to ActionCreateFolder
Maybe missed some things in the list. Maybe more to come.
A few notes:
Something weird is happening with the language dropdown menu. It appears to have ghost entries. I cannot figure it out. EDIT: that seems to have fixed itself at some point.
Display wake could do with some kind of block on wake first touch to prevent UI elements being hit when waking device with touch. Same with encoder/trackball/keyboard press i guess.