Module improvements#585
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe change introduces module-specific public headers for cryptography, GPS, and LVGL APIs. Existing implementations, applications, services, device modules, wrappers, and tests are updated to use the new header namespaces. LVGL lifecycle configuration moves into 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
178-208: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winOnly unlock after successfully acquiring the LVGL mutex.
When
lvgl_try_lock()times out, the unconditionallvgl_unlock()at line 208 is still executed. Both LVGL unlock implementations call down to the underlying unlock primitive when initialized, so this can violate mutex ownership and hit a mutex assertion or corrupt synchronization state.Proposed fix
if (lvgl_try_lock(100 / portTICK_PERIOD_MS)) { for (auto& row : deviceRows) { // ... } + lvgl_unlock(); } - lvgl_unlock();
🧹 Nitpick comments (3)
Modules/crypt-module/source/module.cpp (1)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the public module header in its implementation.
module.cppdefinescrypt_modulebut does not include<crypt/module.h>, so the new export declaration is not checked against its definition. Include the module header while retaining<tactility/module.h>for the complete type definition.Proposed include update
`#include` <crypt/crypt.h> `#include` <crypt/hash.h> +#include <crypt/module.h> `#include` <tactility/module.h>Modules/lvgl-module/include/lvgl/lvgl_fonts.h (1)
17-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
(void)for no-argument C declarations.Because this header exposes a C API, declarations such as
lvgl_get_shared_icon_font()do not enforce an empty parameter list in C. Change all no-argument getters, and their definitions, to(void).Proposed fix
-const lv_font_t* lvgl_get_shared_icon_font(); -uint32_t lvgl_get_shared_icon_font_height(); +const lv_font_t* lvgl_get_shared_icon_font(void); +uint32_t lvgl_get_shared_icon_font_height(void); -const lv_font_t* lvgl_get_launcher_icon_font(); -uint32_t lvgl_get_launcher_icon_font_height(); +const lv_font_t* lvgl_get_launcher_icon_font(void); +uint32_t lvgl_get_launcher_icon_font_height(void); -const lv_font_t* lvgl_get_statusbar_icon_font(); -uint32_t lvgl_get_statusbar_icon_font_height(); +const lv_font_t* lvgl_get_statusbar_icon_font(void); +uint32_t lvgl_get_statusbar_icon_font_height(void);Modules/lvgl-module/include/lvgl/lvgl_pointer.h (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the standard types used directly by this public header.
LvglPointerCalibrationusesint32_tand the API returnsbool, but this header relies on transitive declarations from<lvgl.h>. Add<stdint.h>and<stdbool.h>explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e75c22ec-964e-4a4a-9edd-cd124a49f0b1
📒 Files selected for processing (87)
Devices/lilygo-tdeck-plus/source/module.cppDevices/lilygo-tdeck/source/module.cppDevices/lilygo-tlora-pager/source/module.cppDevices/m5stack-tab5/Source/devices/tab5_keyboard.cppModules/crypt-module/include/crypt/crypt.hModules/crypt-module/include/crypt/hash.hModules/crypt-module/include/crypt/module.hModules/crypt-module/source/crypt.cppModules/crypt-module/source/hash.cppModules/crypt-module/source/module.cppModules/gps-module/include/gps/module.hModules/gps-module/source/module.cppModules/lvgl-module/include/lvgl/lvgl.hModules/lvgl-module/include/lvgl/lvgl_display.hModules/lvgl-module/include/lvgl/lvgl_fonts.hModules/lvgl-module/include/lvgl/lvgl_icon_launcher.hModules/lvgl-module/include/lvgl/lvgl_icon_shared.hModules/lvgl-module/include/lvgl/lvgl_icon_statusbar.hModules/lvgl-module/include/lvgl/lvgl_keyboard.hModules/lvgl-module/include/lvgl/lvgl_pointer.hModules/lvgl-module/include/lvgl/lvgl_ppa.hModules/lvgl-module/include/lvgl/module.hModules/lvgl-module/source/arch/lvgl_esp32.cModules/lvgl-module/source/arch/lvgl_posix.cModules/lvgl-module/source/lvgl.cModules/lvgl-module/source/lvgl_devices.cModules/lvgl-module/source/lvgl_display.cModules/lvgl-module/source/lvgl_fonts.cModules/lvgl-module/source/lvgl_keyboard.cModules/lvgl-module/source/lvgl_pointer.cModules/lvgl-module/source/lvgl_ppa.cModules/lvgl-module/source/module.cModules/lvgl-module/source/symbols.cTactility/Source/Tactility.cppTactility/Source/app/addgps/AddGps.cppTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/appsettings/AppSettings.cppTactility/Source/app/audiosettings/AudioSettings.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/btmanage/View.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/chat/ChatSettings.cppTactility/Source/app/development/Development.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/app/grovesettings/GroveSettings.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/kerneldisplay/KernelDisplay.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/notes/Notes.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/settings/Settings.cppTactility/Source/app/setup/Setup.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timedatesettings/TimeDateSettings.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/app/trackball/TrackballSettings.cppTactility/Source/app/usbsettings/UsbSettings.cppTactility/Source/app/webserversettings/WebServerSettings.cppTactility/Source/app/wifimanage/View.cppTactility/Source/app/wifimanage/WifiManage.cppTactility/Source/file/FileMutexLvgl.cppTactility/Source/lvgl/LvglSync.cppTactility/Source/lvgl/SliderBox.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/lvgl/Toolbar.cppTactility/Source/lvgl/wrappers/button.cppTactility/Source/lvgl/wrappers/dropdown.cppTactility/Source/lvgl/wrappers/list.cppTactility/Source/lvgl/wrappers/obj.cppTactility/Source/lvgl/wrappers/switch.cppTactility/Source/lvgl/wrappers/textarea.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/gui/GuiService.cppTactility/Source/service/keyboardidle/KeyboardIdle.cppTactility/Source/service/memorychecker/MemoryCheckerService.cppTactility/Source/service/statusbar/Statusbar.cppTactility/Source/service/webserver/WebServerService.cppTactility/Source/service/wifi/WifiApSettings.cppTests/SdkIntegration/main/Source/main.cTests/crypt-module/Source/CryptTest.cppTests/crypt-module/Source/HashTest.cpp
💤 Files with no reviewable changes (1)
- Modules/lvgl-module/include/lvgl/lvgl.h
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Tactility/Source/app/gpssettings/GpsSettings.cpp (1)
178-208: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winOnly unlock after successfully acquiring the LVGL mutex.
When
lvgl_try_lock()times out, the unconditionallvgl_unlock()at line 208 is still executed. Both LVGL unlock implementations call down to the underlying unlock primitive when initialized, so this can violate mutex ownership and hit a mutex assertion or corrupt synchronization state.Proposed fix
if (lvgl_try_lock(100 / portTICK_PERIOD_MS)) { for (auto& row : deviceRows) { // ... } + lvgl_unlock(); } - lvgl_unlock();
🧹 Nitpick comments (3)
Modules/crypt-module/source/module.cpp (1)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the public module header in its implementation.
module.cppdefinescrypt_modulebut does not include<crypt/module.h>, so the new export declaration is not checked against its definition. Include the module header while retaining<tactility/module.h>for the complete type definition.Proposed include update
`#include` <crypt/crypt.h> `#include` <crypt/hash.h> +#include <crypt/module.h> `#include` <tactility/module.h>Modules/lvgl-module/include/lvgl/lvgl_fonts.h (1)
17-27: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUse
(void)for no-argument C declarations.Because this header exposes a C API, declarations such as
lvgl_get_shared_icon_font()do not enforce an empty parameter list in C. Change all no-argument getters, and their definitions, to(void).Proposed fix
-const lv_font_t* lvgl_get_shared_icon_font(); -uint32_t lvgl_get_shared_icon_font_height(); +const lv_font_t* lvgl_get_shared_icon_font(void); +uint32_t lvgl_get_shared_icon_font_height(void); -const lv_font_t* lvgl_get_launcher_icon_font(); -uint32_t lvgl_get_launcher_icon_font_height(); +const lv_font_t* lvgl_get_launcher_icon_font(void); +uint32_t lvgl_get_launcher_icon_font_height(void); -const lv_font_t* lvgl_get_statusbar_icon_font(); -uint32_t lvgl_get_statusbar_icon_font_height(); +const lv_font_t* lvgl_get_statusbar_icon_font(void); +uint32_t lvgl_get_statusbar_icon_font_height(void);Modules/lvgl-module/include/lvgl/lvgl_pointer.h (1)
8-11: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the standard types used directly by this public header.
LvglPointerCalibrationusesint32_tand the API returnsbool, but this header relies on transitive declarations from<lvgl.h>. Add<stdint.h>and<stdbool.h>explicitly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e75c22ec-964e-4a4a-9edd-cd124a49f0b1
📒 Files selected for processing (87)
Devices/lilygo-tdeck-plus/source/module.cppDevices/lilygo-tdeck/source/module.cppDevices/lilygo-tlora-pager/source/module.cppDevices/m5stack-tab5/Source/devices/tab5_keyboard.cppModules/crypt-module/include/crypt/crypt.hModules/crypt-module/include/crypt/hash.hModules/crypt-module/include/crypt/module.hModules/crypt-module/source/crypt.cppModules/crypt-module/source/hash.cppModules/crypt-module/source/module.cppModules/gps-module/include/gps/module.hModules/gps-module/source/module.cppModules/lvgl-module/include/lvgl/lvgl.hModules/lvgl-module/include/lvgl/lvgl_display.hModules/lvgl-module/include/lvgl/lvgl_fonts.hModules/lvgl-module/include/lvgl/lvgl_icon_launcher.hModules/lvgl-module/include/lvgl/lvgl_icon_shared.hModules/lvgl-module/include/lvgl/lvgl_icon_statusbar.hModules/lvgl-module/include/lvgl/lvgl_keyboard.hModules/lvgl-module/include/lvgl/lvgl_pointer.hModules/lvgl-module/include/lvgl/lvgl_ppa.hModules/lvgl-module/include/lvgl/module.hModules/lvgl-module/source/arch/lvgl_esp32.cModules/lvgl-module/source/arch/lvgl_posix.cModules/lvgl-module/source/lvgl.cModules/lvgl-module/source/lvgl_devices.cModules/lvgl-module/source/lvgl_display.cModules/lvgl-module/source/lvgl_fonts.cModules/lvgl-module/source/lvgl_keyboard.cModules/lvgl-module/source/lvgl_pointer.cModules/lvgl-module/source/lvgl_ppa.cModules/lvgl-module/source/module.cModules/lvgl-module/source/symbols.cTactility/Source/Tactility.cppTactility/Source/app/addgps/AddGps.cppTactility/Source/app/apphub/AppHubApp.cppTactility/Source/app/applist/AppList.cppTactility/Source/app/appsettings/AppSettings.cppTactility/Source/app/audiosettings/AudioSettings.cppTactility/Source/app/btmanage/BtManage.cppTactility/Source/app/btmanage/View.cppTactility/Source/app/chat/ChatApp.cppTactility/Source/app/chat/ChatSettings.cppTactility/Source/app/development/Development.cppTactility/Source/app/gpssettings/GpsSettings.cppTactility/Source/app/grovesettings/GroveSettings.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactility/Source/app/kerneldisplay/KernelDisplay.cppTactility/Source/app/keyboard/KeyboardSettings.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/app/localesettings/LocaleSettings.cppTactility/Source/app/notes/Notes.cppTactility/Source/app/power/Power.cppTactility/Source/app/poweroff/PowerOff.cppTactility/Source/app/screenshot/Screenshot.cppTactility/Source/app/settings/Settings.cppTactility/Source/app/setup/Setup.cppTactility/Source/app/systeminfo/SystemInfo.cppTactility/Source/app/timedatesettings/TimeDateSettings.cppTactility/Source/app/timezone/TimeZone.cppTactility/Source/app/touchcalibration/TouchCalibration.cppTactility/Source/app/trackball/TrackballSettings.cppTactility/Source/app/usbsettings/UsbSettings.cppTactility/Source/app/webserversettings/WebServerSettings.cppTactility/Source/app/wifimanage/View.cppTactility/Source/app/wifimanage/WifiManage.cppTactility/Source/file/FileMutexLvgl.cppTactility/Source/lvgl/LvglSync.cppTactility/Source/lvgl/SliderBox.cppTactility/Source/lvgl/Statusbar.cppTactility/Source/lvgl/Toolbar.cppTactility/Source/lvgl/wrappers/button.cppTactility/Source/lvgl/wrappers/dropdown.cppTactility/Source/lvgl/wrappers/list.cppTactility/Source/lvgl/wrappers/obj.cppTactility/Source/lvgl/wrappers/switch.cppTactility/Source/lvgl/wrappers/textarea.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/gui/GuiService.cppTactility/Source/service/keyboardidle/KeyboardIdle.cppTactility/Source/service/memorychecker/MemoryCheckerService.cppTactility/Source/service/statusbar/Statusbar.cppTactility/Source/service/webserver/WebServerService.cppTactility/Source/service/wifi/WifiApSettings.cppTests/SdkIntegration/main/Source/main.cTests/crypt-module/Source/CryptTest.cppTests/crypt-module/Source/HashTest.cpp
💤 Files with no reviewable changes (1)
- Modules/lvgl-module/include/lvgl/lvgl.h
🛑 Comments failed to post (3)
Modules/lvgl-module/include/lvgl/lvgl_pointer.h (1)
51-61: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Keep the default pointer indev valid after removing the first one.
The implementation in
Modules/lvgl-module/source/lvgl_pointer.csetsdefault_pointer_indev = NULLwhen the current default is removed, even if anotherlvgl_pointer_add()indev remains. That contradicts this documented “first indev ... that hasn’t been removed” contract. Track the registered indevs or select the next remaining one during removal.Modules/lvgl-module/source/lvgl.c (1)
2-5: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix the missing
UiDensityandTT_LVGL_UI_DENSITYdefinitions.With the shown includes, this translation unit has no visible complete declaration for
enum UiDensity, andTT_LVGL_UI_DENSITYis undeclared. Clang reports both errors, preventing the LVGL module from compiling. Include the header that ownsUiDensityand ensure the density configuration header/definition is attached to this CMake target.🧰 Tools
🪛 Clang (14.0.6)
[error] 4-4: incomplete result type 'enum UiDensity' in function definition
(clang-diagnostic-error)
[error] 5-5: use of undeclared identifier 'TT_LVGL_UI_DENSITY'
(clang-diagnostic-error)
Source: Linters/SAST tools
Tactility/Source/app/development/Development.cpp (1)
34-37: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Repository files matching Development.cpp:" fd -a 'Development\.cpp$' . || true echo echo "Search lvgl_is_running/lvgl_lock/lvgl_unlock/updateViewState definitions and usages:" rg -n "lvgl_is_running|lvgl_lock|lvgl_unlock|updateViewState|lvgl_deinit|lvgl_init" . echo echo "Git status/stat:" git status --short git diff --stat || trueRepository: TactilityProject/Tactility
Length of output: 6874
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Development.cpp relevant lines:" sed -n '1,220p' Tactility/Source/app/development/Development.cpp | cat -n echo echo "LvglModule source outlines/lines:" for f in Tactility/Modules/lvgl-module/source/module.c Tactility/Modules/lvgl-module/source/arch/lvgl_posix.c Tactility/Modules/lvgl-module/source/arch/lvgl_esp32.c Tactility/Modules/lvgl-module/include/lvgl/lvgl.h; do echo "---- $f ----" wc -l "$f" sed -n '1,180p' "$f" | cat -n doneRepository: TactilityProject/Tactility
Length of output: 8087
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Find lvgl module files:" git ls-files | rg 'lvgl-module|Lvgl|lvgl' echo echo "Modules directory files:" find . -path './Tactility/Modules/lvgl-module*' -type f -print | sort echo echo "Relevant source files:" for f in $(git ls-files | rg 'lvgl-module/(source|include)|lvgl_(pos|esp|lock|module)'); do echo "---- $f ----" wc -l "$f" sed -n '1,180p' "$f" | cat -n done echo echo "Search state/run symbols:" rg -n "running|deinit|init|running_state|deinitialized|mutex|lock|unlock|lv_draw_buf|lv_display" Tactility/Modules/lvgl-module Tactility/Source app Tactility/Include 2>/dev/null | head -200Repository: TactilityProject/Tactility
Length of output: 50383
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "LVGL module source files relevant to synchronization/state:" for f in Modules/lvgl-module/source/module.c Modules/lvgl-module/source/arch/lvgl_posix.c Modules/lvgl-module/source/arch/lvgl_esp32.c; do echo "---- $f ----" wc -l "$f" sed -n '1,180p' "$f" | cat -n done echo echo "Development.cpp update/hide block:" sed -n'33,40p;159,164p' Tactility/Source/app/development/Development.cpp | cat -n echo echo "Similar lvgl_is_running usage around updates:" sed -n '260,295p' Tactility/Source/service/statusbar/Statusbar.cpp | cat -n echo echo "Lock definitions with line numbers:" rg -n -C 5 "void lvgl_(lock|unlock|try_lock)|bool lvgl_is_running" Modules/lvgl-module/source Modules/lvgl-module/includeRepository: TactilityProject/Tactility
Length of output: 11568
Move the running-state check under the LVGL lock.
lvgl_is_running()returns module state separately from the LVGL lock, so LVGL can stop/deinitialize after the check and beforelvgl_lock()returns. On POSIX,lvgl_lock()can also return immediately if the mutex is not initialized, leavingupdateViewState()to call LVGL APIs unprotected. Acquire the lock first and skipupdateViewState()iflvgl_is_running()is false; keeponHide()’stimer.stop()under lock as well.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2a3bea0d-8b53-45e1-801b-e5f2dc403c8c
📒 Files selected for processing (8)
Modules/crypt-module/include/crypt/module.hModules/crypt-module/source/module.cppModules/lvgl-module/include/lvgl/lvgl.hModules/lvgl-module/include/lvgl/lvgl_display.hModules/lvgl-module/include/lvgl/lvgl_fonts.hModules/lvgl-module/include/lvgl/lvgl_keyboard.hModules/lvgl-module/include/lvgl/lvgl_pointer.hTactility/Source/app/gpssettings/GpsSettings.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- Modules/crypt-module/source/module.cpp
- Modules/lvgl-module/include/lvgl/lvgl_fonts.h
- Modules/lvgl-module/include/lvgl/lvgl_keyboard.h
- Modules/lvgl-module/include/lvgl/lvgl.h
- Modules/lvgl-module/include/lvgl/lvgl_pointer.h
1 participant
Summary by CodeRabbit