Smart tab5keyboard#533
Conversation
wrong logo was selected if rotation was changed from the default... yep it was weird.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughThis PR makes several hardware and software improvements for the M5Stack Tab5 device. The SD card interface is migrated from SPI to SDMMC with a new 4-bit bus configuration, configurable slot and frequency parameters, and an LDO power-on delay. Keyboard hot-plug detection is added to 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Devices/m5stack-tab5/Source/module.cpp (1)
124-134:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRoll back
hp_detect_timerwhen keyboard timer setup fails.
start()can returnERROR_RESOURCEafterhp_detect_timeris already running. That leaves a partially started module state and can block correct re-init behavior on the nextstart()call.Suggested fix
kb_detect_timer = xTimerCreate("kb_detect", pdMS_TO_TICKS(KB_DETECT_POLL_MS), pdTRUE, nullptr, keyboardDetectCallback); if (!kb_detect_timer) { LOG_E(TAG, "Failed to create kb_detect timer"); + xTimerStop(hp_detect_timer, pdMS_TO_TICKS(100)); + xTimerDelete(hp_detect_timer, pdMS_TO_TICKS(100)); + hp_detect_timer = nullptr; return ERROR_RESOURCE; } if (xTimerStart(kb_detect_timer, pdMS_TO_TICKS(100)) != pdPASS) { LOG_E(TAG, "Failed to start kb_detect timer"); xTimerDelete(kb_detect_timer, pdMS_TO_TICKS(100)); kb_detect_timer = nullptr; + xTimerStop(hp_detect_timer, pdMS_TO_TICKS(100)); + xTimerDelete(hp_detect_timer, pdMS_TO_TICKS(100)); + hp_detect_timer = nullptr; return ERROR_RESOURCE; }Also applies to: 138-148
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7ce668b-27ed-459f-84c9-14cd0d272d22
📒 Files selected for processing (22)
Devices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cppDevices/m5stack-tab5/Source/devices/SdCard.cppDevices/m5stack-tab5/Source/devices/SdCard.hDevices/m5stack-tab5/Source/devices/St7123Display.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.hDevices/m5stack-tab5/Source/module.cppDevices/m5stack-tab5/m5stack,tab5.dtsDrivers/EspLcdCompat/Source/EspLcdDisplayDriver.hPlatforms/platform-esp32/bindings/espressif,esp32-sdmmc.yamlPlatforms/platform-esp32/include/tactility/drivers/esp32_sdmmc.hPlatforms/platform-esp32/source/drivers/esp32_sdmmc_fs.cppTactility/Include/Tactility/hal/display/DisplayDriver.hTactility/Source/app/boot/Boot.cppTactility/Source/app/launcher/Launcher.cppTactility/Source/hal/usb/UsbTusb.cppTactilityC/CMakeLists.txtTactilityC/Include/tt_hal_display.hTactilityC/Source/symbols/gcc_soft_float_p4.cppTactilityC/Source/tt_hal_display.cppTactilityC/Source/tt_init.cpp
💤 Files with no reviewable changes (2)
- Devices/m5stack-tab5/Source/devices/SdCard.h
- Devices/m5stack-tab5/Source/devices/SdCard.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3ab40414-e598-4267-8318-6f55de2aab62
📒 Files selected for processing (9)
Devices/m5stack-tab5/Source/devices/Tab5Keyboard.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.hDevices/m5stack-tab5/Source/module.cppPlatforms/platform-esp32/bindings/espressif,esp32-sdmmc.yamlTactility/Include/Tactility/hal/usb/Usb.hTactility/Private/Tactility/hal/usb/UsbTusb.hTactility/Source/app/boot/Boot.cppTactility/Source/hal/usb/Usb.cppTactility/Source/hal/usb/UsbTusb.cpp
🚧 Files skipped from review as they are similar to previous changes (5)
- Platforms/platform-esp32/bindings/espressif,esp32-sdmmc.yaml
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.h
- Tactility/Source/app/boot/Boot.cpp
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.cpp
- Devices/m5stack-tab5/Source/module.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Devices/m5stack-tab5/m5stack,tab5.dts (1)
90-90:⚠️ Potential issue | 🟠 MajorReplace
SDMMC_FREQ_HIGHSPEEDwith an explicit frequency to match the Tab5 board-validated limit.The binding documentation explicitly states that
SDMMC_FREQ_HIGHSPEEDexpands to40000kHz, but the intended limit for this board is20000kHz (the binding's default value). Using the high-speed macro would run the SD bus at twice the validated frequency. Change line 90 tomax-freq-khz = <20000>;to maintain board stability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 127e2780-f329-40ef-9fcf-e6be307f427b
📒 Files selected for processing (14)
Devices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/St7123Touch.cppDevices/m5stack-tab5/Source/devices/St7123Touch.hDevices/m5stack-tab5/Source/devices/Tab5Keyboard.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.hDevices/m5stack-tab5/m5stack,tab5.dtsDrivers/GT911Ng/CMakeLists.txtDrivers/GT911Ng/README.mdDrivers/GT911Ng/Source/Gt911TouchNg.cppDrivers/GT911Ng/Source/Gt911TouchNg.hPlatforms/platform-esp32/include/tactility/drivers/esp32_i2c_master.hPlatforms/platform-esp32/source/drivers/esp32_i2c_master.cpp
💤 Files with no reviewable changes (1)
- Devices/m5stack-tab5/Source/Configuration.cpp
✅ Files skipped from review due to trivial changes (3)
- Devices/m5stack-tab5/CMakeLists.txt
- Drivers/GT911Ng/CMakeLists.txt
- Drivers/GT911Ng/README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.h
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.cpp
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbbf7050-2454-4a52-9992-9873f8f99d97
📒 Files selected for processing (15)
Buildscripts/TactilitySDK/TactilitySDK.cmakeDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/St7123Touch.cppDevices/m5stack-tab5/Source/devices/St7123Touch.hDevices/m5stack-tab5/Source/devices/Tab5Keyboard.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.hDevices/m5stack-tab5/m5stack,tab5.dtsPlatforms/platform-esp32/bindings/espressif,esp32-grove.yamlPlatforms/platform-esp32/include/tactility/drivers/esp32_grove.hPlatforms/platform-esp32/include/tactility/drivers/esp32_i2c_master.hPlatforms/platform-esp32/source/drivers/esp32_grove.cppPlatforms/platform-esp32/source/drivers/esp32_i2c_master.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactilityKernel/source/kernel_symbols.c
💤 Files with no reviewable changes (1)
- Buildscripts/TactilitySDK/TactilitySDK.cmake
🚧 Files skipped from review as they are similar to previous changes (6)
- Devices/m5stack-tab5/Source/devices/St7123Touch.h
- Devices/m5stack-tab5/Source/devices/St7123Touch.cpp
- Platforms/platform-esp32/include/tactility/drivers/esp32_i2c_master.h
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.h
- Platforms/platform-esp32/source/drivers/esp32_i2c_master.cpp
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.cpp
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: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: dbbf7050-2454-4a52-9992-9873f8f99d97
📒 Files selected for processing (15)
Buildscripts/TactilitySDK/TactilitySDK.cmakeDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/St7123Touch.cppDevices/m5stack-tab5/Source/devices/St7123Touch.hDevices/m5stack-tab5/Source/devices/Tab5Keyboard.cppDevices/m5stack-tab5/Source/devices/Tab5Keyboard.hDevices/m5stack-tab5/m5stack,tab5.dtsPlatforms/platform-esp32/bindings/espressif,esp32-grove.yamlPlatforms/platform-esp32/include/tactility/drivers/esp32_grove.hPlatforms/platform-esp32/include/tactility/drivers/esp32_i2c_master.hPlatforms/platform-esp32/source/drivers/esp32_grove.cppPlatforms/platform-esp32/source/drivers/esp32_i2c_master.cppTactility/Source/app/i2cscanner/I2cScanner.cppTactilityKernel/source/kernel_symbols.c
💤 Files with no reviewable changes (1)
- Buildscripts/TactilitySDK/TactilitySDK.cmake
🚧 Files skipped from review as they are similar to previous changes (6)
- Devices/m5stack-tab5/Source/devices/St7123Touch.h
- Devices/m5stack-tab5/Source/devices/St7123Touch.cpp
- Platforms/platform-esp32/include/tactility/drivers/esp32_i2c_master.h
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.h
- Platforms/platform-esp32/source/drivers/esp32_i2c_master.cpp
- Devices/m5stack-tab5/Source/devices/Tab5Keyboard.cpp
🛑 Comments failed to post (2)
Platforms/platform-esp32/source/drivers/esp32_grove.cpp (1)
156-160:
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
child_configleaks when child startup fails.On Line 156, if
device_construct_add_start(...)fails,stop_child()runs whiledata->current_modeis stillGROVE_MODE_DISABLED, so Line 53-58 won’t delete the allocatedEsp32UartConfig/Esp32I2cMasterConfig. This is a real error-path leak.Proposed fix
- error_t err = device_construct_add_start(data->child_device, compatible); + // Set mode before startup so stop_child() can free mode-specific config on failure. + data->current_mode = mode; + error_t err = device_construct_add_start(data->child_device, compatible); if (err != ERROR_NONE) { LOG_E(TAG, "%s: failed to start child device: %d", device->name, err); stop_child(device); return err; } - - data->current_mode = mode; return ERROR_NONE;📝 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.// Set mode before startup so stop_child() can free mode-specific config on failure. data->current_mode = mode; error_t err = device_construct_add_start(data->child_device, compatible); if (err != ERROR_NONE) { LOG_E(TAG, "%s: failed to start child device: %d", device->name, err); stop_child(device); return err; } return ERROR_NONE;Tactility/Source/app/i2cscanner/I2cScanner.cpp (1)
187-191:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail
getPortwhen no bus is selected.
getPortreports success even whenportDevice == nullptr, thenonScanTimertreats that pointer as valid. This leaves a reachable null-pointer path during scan startup when no active bus was selected.Suggested fix
bool I2cScannerApp::getPort(struct Device** outPort) { if (mutex.lock(100 / portTICK_PERIOD_MS)) { - *outPort = this->portDevice; + *outPort = this->portDevice; + if (*outPort == nullptr) { + mutex.unlock(); + logger.warn("No I2C port selected"); + return false; + } mutex.unlock(); return true; } else { logger.warn(LOG_MESSAGE_MUTEX_LOCK_FAILED_FMT, "getPort"); return false;Also applies to: 222-233
|
Thanks a lot! |
2 participants
Tab5 + Tab5 Keyboard are now "smart" 🧠
Misc changes:
Summary by CodeRabbit
0x00