Guition JC3248W535C #467
Guition JC3248W535C #467
Conversation
Files app fix alert dialog and selection dialog fixes symbol export
📝 WalkthroughWalkthroughAdds support for a new ESP32‑S3 device "Guition JC3248W535C": firmware build matrix entry, Kconfig option, device tree, and device.properties. Adds AXS15231B display and touch drivers with LVGL and esp_lcd integration, device configuration, display/touch/SD card factory functions, and a no-op C registration function. Firmware idf_component.yml adds espressif/esp_lcd_axs15231b. Tactility changes: View::deinit added and FilesApp::onHide calls it; several C dialog APIs now return AppLaunchId; exports lv_group_set_editing symbol. 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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: 7
🧹 Nitpick comments (1)
Firmware/idf_component.yml (1)
32-32: Consider adding target rules for consistency.Other display-specific drivers in this file use conditional rules (e.g.,
esp_lcd_ili9341restricts toesp32, esp32s3). Sinceesp_lcd_axs15231bappears to be used only by the JC3248W535C (esp32s3), you could add a similar rule to avoid pulling this dependency for unrelated targets.♻️ Optional: Add target restriction
- espressif/esp_lcd_axs15231b: "2.0.2" + espressif/esp_lcd_axs15231b: + version: "2.0.2" + rules: + - if: "target == esp32s3"
| void View::deinit(const AppContext& appContext) { | ||
| lv_obj_remove_event_cb(dir_entry_list, dirEntryListScrollBeginCallback); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "View.cpp" | head -5Repository: TactilityProject/Tactility
Length of output: 240
🏁 Script executed:
git ls-files | grep -E "(View\.(cpp|h)|lvgl)" | head -20Repository: TactilityProject/Tactility
Length of output: 1174
🏁 Script executed:
find . -type f -name "*.cpp" -o -name "*.h" | grep -i view | head -10Repository: TactilityProject/Tactility
Length of output: 562
🏁 Script executed:
find ./Tactility/Source/app/files -type f | head -20Repository: TactilityProject/Tactility
Length of output: 236
🏁 Script executed:
wc -l ./Tactility/Source/app/files/View.cppRepository: TactilityProject/Tactility
Length of output: 111
🏁 Script executed:
sed -n '470,490p' ./Tactility/Source/app/files/View.cppRepository: TactilityProject/Tactility
Length of output: 246
🏁 Script executed:
sed -n '1,100p' ./Tactility/Source/app/files/View.cppRepository: TactilityProject/Tactility
Length of output: 3316
🏁 Script executed:
cat -n ./Tactility/Source/app/files/View.cpp | sed -n '150,250p'Repository: TactilityProject/Tactility
Length of output: 4228
🏁 Script executed:
cat -n ./Tactility/Source/app/files/View.cpp | sed -n '250,350p'Repository: TactilityProject/Tactility
Length of output: 4818
🏁 Script executed:
cat -n ./Tactility/Source/app/files/View.cpp | sed -n '350,450p'Repository: TactilityProject/Tactility
Length of output: 4513
🏁 Script executed:
cat -n ./Tactility/Source/app/files/View.cpp | sed -n '450,479p'Repository: TactilityProject/Tactility
Length of output: 1154
🏁 Script executed:
cat -n ./Tactility/Private/Tactility/app/files/View.h | head -80Repository: TactilityProject/Tactility
Length of output: 1710
🏁 Script executed:
rg "lv_obj_remove_event_cb|deinit" --type-list | head -20Repository: TactilityProject/Tactility
Length of output: 459
🏁 Script executed:
rg "lv_obj_remove_event_cb" -A 2 -B 2Repository: TactilityProject/Tactility
Length of output: 749
🏁 Script executed:
rg "getSyncLock" -A 2 -B 2 | head -60Repository: TactilityProject/Tactility
Length of output: 3989
🏁 Script executed:
rg "void.*deinit" -A 5 | head -60Repository: TactilityProject/Tactility
Length of output: 3847
🏁 Script executed:
find . -type f -name "*.cpp" -o -name "*.h" | xargs rg "lv_obj_add_event_cb|lv_obj_remove_event_cb" | head -30Repository: TactilityProject/Tactility
Length of output: 4386
🏁 Script executed:
rg "lv_obj_remove_event_cb" -B 5 -A 5Repository: TactilityProject/Tactility
Length of output: 1357
🏁 Script executed:
find . -type f \( -name "*.cpp" -o -name "*.h" \) -exec grep -l "deinit" {} \; | grep -i view | head -10Repository: TactilityProject/Tactility
Length of output: 154
🏁 Script executed:
cat ./Tactility/Private/Tactility/app/files/View.hRepository: TactilityProject/Tactility
Length of output: 1360
🏁 Script executed:
cat -n ./Tactility/Source/app/files/FilesApp.cppRepository: TactilityProject/Tactility
Length of output: 1641
🏁 Script executed:
rg "View::" ./Tactility/Source/app/files/ -A 3 -B 3Repository: TactilityProject/Tactility
Length of output: 8084
🏁 Script executed:
find . -type f -name "*.h" | xargs grep -l "LvglSync\|getSyncLock" | head -5Repository: TactilityProject/Tactility
Length of output: 115
🏁 Script executed:
cat ./Tactility/Include/Tactility/lvgl/LvglSync.h 2>/dev/null || cat ./Tactility/Private/Tactility/lvgl/LvglSync.h 2>/dev/null || find . -name "LvglSync.h"Repository: TactilityProject/Tactility
Length of output: 1010
🏁 Script executed:
rg "lv_obj_add_flag|lv_obj_remove_flag" -B 2 -A 2 | head -50Repository: TactilityProject/Tactility
Length of output: 3130
🏁 Script executed:
rg "LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT" -B 2 -A 2Repository: TactilityProject/Tactility
Length of output: 7531
🏁 Script executed:
find . -type f -name "*.h" | xargs grep -l "class App" | grep -i app | head -5Repository: TactilityProject/Tactility
Length of output: 297
🏁 Script executed:
rg "onShow|onHide" --type-list | head -1 || rg "onShow.*{" -A 5 -B 5 | head -40Repository: TactilityProject/Tactility
Length of output: 459
🏁 Script executed:
cat ./Tactility/Include/Tactility/app/App.hRepository: TactilityProject/Tactility
Length of output: 3528
Add LVGL lock + null guard in deinit to match the pattern used in other methods.
lv_obj_remove_event_cb is called without the LVGL sync lock, inconsistent with update(), onNavigate(), and onDirEntryListScrollBegin() which all acquire the lock before any LVGL operations. Additionally, there is no null guard for dir_entry_list. While the app lifecycle ensures init is called before deinit, adding defensive checks prevents crashes if the code is refactored.
🛠️ Proposed fix
void View::deinit(const AppContext& appContext) {
- lv_obj_remove_event_cb(dir_entry_list, dirEntryListScrollBeginCallback);
+ if (dir_entry_list == nullptr) {
+ return;
+ }
+ auto scoped_lockable = lvgl::getSyncLock()->asScopedLock();
+ if (scoped_lockable.lock(lvgl::defaultLockTime)) {
+ lv_obj_remove_event_cb(dir_entry_list, dirEntryListScrollBeginCallback);
+ } else {
+ LOGGER.error(LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, "lvgl");
+ }
}| AppLaunchId tt_app_alertdialog_start(const char* title, const char* message, const char* buttonLabels[], uint32_t buttonLabelCount) { | ||
| std::vector<std::string> list; | ||
| for (int i = 0; i < buttonLabelCount; i++) { | ||
| list.emplace_back(buttonLabels[i]); | ||
| } |
There was a problem hiding this comment.
Validate buttonLabels when buttonLabelCount > 0 and avoid signed/unsigned loop mismatch.
Line 9 dereferences buttonLabels[i] without guarding; if a caller passes a non‑zero count with a null array (or null elements), this will crash. Also, Line 8 mixes int with uint32_t. A small guard and unsigned loop index removes both risks.
✅ Suggested fix
AppLaunchId tt_app_alertdialog_start(const char* title, const char* message, const char* buttonLabels[], uint32_t buttonLabelCount) {
std::vector<std::string> list;
- for (int i = 0; i < buttonLabelCount; i++) {
- list.emplace_back(buttonLabels[i]);
- }
+ if (buttonLabelCount > 0 && buttonLabels == nullptr) {
+ return AppLaunchId{};
+ }
+ for (uint32_t i = 0; i < buttonLabelCount; ++i) {
+ const char* label = buttonLabels[i];
+ list.emplace_back(label ? label : "");
+ }
return tt::app::alertdialog::start(title, message, list);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AppLaunchId tt_app_alertdialog_start(const char* title, const char* message, const char* buttonLabels[], uint32_t buttonLabelCount) { | |
| std::vector<std::string> list; | |
| for (int i = 0; i < buttonLabelCount; i++) { | |
| list.emplace_back(buttonLabels[i]); | |
| } | |
| AppLaunchId tt_app_alertdialog_start(const char* title, const char* message, const char* buttonLabels[], uint32_t buttonLabelCount) { | |
| std::vector<std::string> list; | |
| if (buttonLabelCount > 0 && buttonLabels == nullptr) { | |
| return AppLaunchId{}; | |
| } | |
| for (uint32_t i = 0; i < buttonLabelCount; ++i) { | |
| const char* label = buttonLabels[i]; | |
| list.emplace_back(label ? label : ""); | |
| } | |
| return tt::app::alertdialog::start(title, message, list); | |
| } |
| AppLaunchId tt_app_selectiondialog_start(const char* title, int argc, const char* argv[]) { | ||
| std::vector<std::string> list; | ||
| for (int i = 0; i < argc; i++) { | ||
| list.emplace_back(argv[i]); | ||
| } |
There was a problem hiding this comment.
Guard argv when argc > 0 to prevent null dereference.
Line 9 unconditionally dereferences argv[i]. If a caller passes a non‑zero count with a null array (or null elements), this crashes. Please add a defensive guard consistent with your C API error handling.
✅ Suggested fix
AppLaunchId tt_app_selectiondialog_start(const char* title, int argc, const char* argv[]) {
std::vector<std::string> list;
+ if (argc > 0 && argv == nullptr) {
+ return AppLaunchId{};
+ }
for (int i = 0; i < argc; i++) {
- list.emplace_back(argv[i]);
+ const char* item = argv[i];
+ list.emplace_back(item ? item : "");
}
return tt::app::selectiondialog::start(title, list);
}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Devices/guition-jc3248w535c/Source/Axs15231b/Axs15231bDisplay.cpp (1)
69-86: Consider makingallocationErrorLoggedan instance member.The
static bool allocationErrorLoggedis shared across all instances ofAxs15231bDisplay. While unlikely in practice (typically one display), if multiple instances existed, the flag would suppress error logs for subsequent instances even if they experience allocation failures independently.💡 Suggested refactor
Move to class member in the header:
bool tempBufAllocationErrorLogged = false;Then in the callback:
- static bool allocationErrorLogged = false; if (self->tempBuf == nullptr) { self->tempBuf = (uint16_t *)heap_caps_malloc(...); if (self->tempBuf == nullptr) { - if (!allocationErrorLogged) { + if (!self->tempBufAllocationErrorLogged) { LOGGER.error("Failed to allocate rotation buffer, drawing unrotated"); - allocationErrorLogged = true; + self->tempBufAllocationErrorLogged = true; }
1 participant
Files app fix
alert dialog and selection dialog fixes
symbol export
Docs are also done for the website.
Summary by CodeRabbit
New Features
Improvements
Chores
✏️ Tip: You can customize this high-level summary in your review settings.