M5Stack StickS3 - New Tab5 - driver modules#516
M5Stack StickS3 - New Tab5 - driver modules#516Shadowtrance wants to merge 7 commits intoTactilityProject:mainfrom
Conversation
Font size set to 18 for 800x480 displays Fix web server dashboard not rendering when sdcard isn't present Added new driver modules for BM8563 RTC, RX8130CE RTC, MPU6886 IMU, QMI8658 IMU. Applied the above modules to applicable devicetrees. Added driver module for the M5Stack M5PM1 Power Management Chip. Added new device: M5Stack StickS3 Added new M5Stack Tab5 St7123 variant. ButtonControl changed to use interupts and xQueue, added AppClose action. And some bonus symbols of course, the apps are hungry for symbols.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multiple new sensor and peripheral drivers and bindings (bm8563, mpu6886, qmi8658, rx8130ce, m5pm1) with public headers, driver implementations, module symbols, and devicetree metadata. Introduces M5Stack StickS3 device support (DTS, devicetree.yaml, device.properties, display and power implementations, CMake, module). Extends several M5Stack device trees with additional I2C peripherals (BMI270, MPU6886, BM8563, QMI8658). Adds ST7123 display and touch support with Tab5 variant detection and init data. Refactors ButtonControl to an interrupt-driven queued model and adds EspLcdConfiguration flags (swRotate, buffSpiram) plus LVGL symbol exports. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 8
🧹 Nitpick comments (9)
Drivers/bm8563-module/LICENSE-Apache-2.0.md (1)
1-1: Avoid rewriting license boilerplate; suppress markdown heading rules for this file.Line 7 and subsequent section headings trigger MD001/MD003, but reformatting canonical license text is unnecessary risk. Prefer file-level markdownlint suppression for these rules.
Proposed non-invasive fix
+<!-- markdownlint-disable MD001 MD003 --> Apache License ============== @@ limitations under the License. + +<!-- markdownlint-enable MD001 MD003 -->Drivers/rx8130ce-module/CMakeLists.txt (1)
5-5: Prefer reconfigure-aware source discovery forGLOB_RECURSE.Line [5] uses
file(GLOB_RECURSE ...)withoutCONFIGURE_DEPENDS, which can miss newly added source files until CMake is re-run manually. Consider addingCONFIGURE_DEPENDS(or explicitly listing sources).Suggested change
-file(GLOB_RECURSE SOURCE_FILES "source/*.c*") +file(GLOB_RECURSE SOURCE_FILES CONFIGURE_DEPENDS "source/*.c*")Devices/m5stack-tab5/Source/devices/st7123_init_data.h (1)
28-28: Minor inconsistency: length parameter uses hex notation.This line uses
0x04for the length parameter while all other entries use decimal notation. Consider using4for consistency with the rest of the array.✏️ Suggested fix
- {0xB7, (uint8_t[]){0x00, 0x00, 0x73, 0x73}, 0x04, 0}, + {0xB7, (uint8_t[]){0x00, 0x00, 0x73, 0x73}, 4, 0},Drivers/mpu6886-module/include/drivers/mpu6886.h (1)
7-11: Consider moving the forward declaration inside theextern "C"block.The
struct Deviceforward declaration on line 7 is outside theextern "C"block. For consistency and to avoid potential linkage issues in mixed C/C++ compilation, consider moving it inside:♻️ Suggested refactor
-struct Device; - `#ifdef` __cplusplus extern "C" { `#endif` +struct Device; + struct Mpu6886Config {Devices/m5stack-sticks3/Source/devices/Display.h (1)
16-16: Consider documenting the LV_COLOR_DEPTH dependency.
LCD_SPI_TRANSFER_SIZE_LIMITdepends onLV_COLOR_DEPTH, which must be defined before this header is included. If this header is ever included without the LVGL configuration in scope, compilation will fail. This is likely fine given the project structure, but adding a comment or an include guard could clarify the dependency.Devices/m5stack-tab5/Source/devices/Display.cpp (1)
62-67: Prefer GPIO enum over raw literal for the GT911 workaround pin.Using
GPIO_NUM_23here keeps this aligned with the rest of the file and reduces pin drift risk.♻️ Proposed tweak
- tt::hal::gpio::configure(23, tt::hal::gpio::Mode::Output, true, false); - tt::hal::gpio::setLevel(23, false); + tt::hal::gpio::configure(GPIO_NUM_23, tt::hal::gpio::Mode::Output, true, false); + tt::hal::gpio::setLevel(GPIO_NUM_23, false);Drivers/m5pm1-module/source/m5pm1.cpp (1)
185-212: Consider adding a brief delay before the first poll.The ADC conversion is started, then immediately polled. While the loop has
vTaskDelay(5)at the start of each iteration, the first poll occurs after only 5ms. If the chip typically needs more time, this burns a few unnecessary I2C transactions.Devices/m5stack-sticks3/Source/devices/Power.cpp (1)
53-66: Note: Linear battery percentage mapping is a simplification.The current implementation uses a linear mapping between 3300–4200 mV for charge level percentage. Li-ion batteries have a non-linear discharge curve (relatively flat in the middle, steep drops at extremes). This is acceptable for a basic implementation, but could be refined with a lookup table or polynomial approximation if more accurate percentage reporting is desired in the future.
Drivers/bm8563-module/source/bm8563.cpp (1)
97-97: Don't hard-code weekday to Sunday.Line 97 writes
0into the weekday register on everyset_datetime()call. The BM8563 calendar includes a weekday counter, and weekday alarms use that register, so the RTC state is inconsistent for 6 out of 7 dates after every write. Compute weekday fromyear/month/daybefore programming register0x06. (makerguides.com)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1f041fc2-7a78-4421-860f-399ff60181e7
📒 Files selected for processing (110)
Data/data/webserver/dashboard.htmlDevices/btt-panda-touch/device.propertiesDevices/cyd-8048s043c/device.propertiesDevices/elecrow-crowpanel-advance-50/device.propertiesDevices/elecrow-crowpanel-basic-50/device.propertiesDevices/guition-jc8048w550c/device.propertiesDevices/m5stack-cardputer-adv/devicetree.yamlDevices/m5stack-cardputer-adv/m5stack,cardputer-adv.dtsDevices/m5stack-core2/devicetree.yamlDevices/m5stack-core2/m5stack,core2.dtsDevices/m5stack-cores3/devicetree.yamlDevices/m5stack-cores3/m5stack,cores3.dtsDevices/m5stack-papers3/devicetree.yamlDevices/m5stack-papers3/m5stack,papers3.dtsDevices/m5stack-stickc-plus/devicetree.yamlDevices/m5stack-stickc-plus/m5stack,stickc-plus.dtsDevices/m5stack-stickc-plus2/devicetree.yamlDevices/m5stack-stickc-plus2/m5stack,stickc-plus2.dtsDevices/m5stack-sticks3/CMakeLists.txtDevices/m5stack-sticks3/Source/Configuration.cppDevices/m5stack-sticks3/Source/devices/Display.cppDevices/m5stack-sticks3/Source/devices/Display.hDevices/m5stack-sticks3/Source/devices/Power.cppDevices/m5stack-sticks3/Source/devices/Power.hDevices/m5stack-sticks3/Source/module.cppDevices/m5stack-sticks3/device.propertiesDevices/m5stack-sticks3/devicetree.yamlDevices/m5stack-sticks3/m5stack,sticks3.dtsDevices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/devices/Detect.cppDevices/m5stack-tab5/Source/devices/Detect.hDevices/m5stack-tab5/Source/devices/Display.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cppDevices/m5stack-tab5/Source/devices/St7123Display.cppDevices/m5stack-tab5/Source/devices/St7123Display.hDevices/m5stack-tab5/Source/devices/St7123Touch.cppDevices/m5stack-tab5/Source/devices/St7123Touch.hDevices/m5stack-tab5/Source/devices/ili9881_init_data.hDevices/m5stack-tab5/Source/devices/st7123_init_data.hDevices/m5stack-tab5/devicetree.yamlDevices/m5stack-tab5/m5stack,tab5.dtsDevices/waveshare-s3-lcd-13/devicetree.yamlDevices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dtsDevices/waveshare-s3-touch-lcd-128/devicetree.yamlDevices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dtsDocumentation/ideas.mdDrivers/ButtonControl/Source/ButtonControl.cppDrivers/ButtonControl/Source/ButtonControl.hDrivers/EspLcdCompat/Source/EspLcdDisplayV2.cppDrivers/EspLcdCompat/Source/EspLcdDisplayV2.hDrivers/bm8563-module/CMakeLists.txtDrivers/bm8563-module/LICENSE-Apache-2.0.mdDrivers/bm8563-module/README.mdDrivers/bm8563-module/bindings/belling,bm8563.yamlDrivers/bm8563-module/devicetree.yamlDrivers/bm8563-module/include/bindings/bm8563.hDrivers/bm8563-module/include/bm8563_module.hDrivers/bm8563-module/include/drivers/bm8563.hDrivers/bm8563-module/source/bm8563.cppDrivers/bm8563-module/source/module.cppDrivers/bm8563-module/source/symbols.cDrivers/bmi270-module/source/bmi270.cppDrivers/m5pm1-module/CMakeLists.txtDrivers/m5pm1-module/LICENSE-Apache-2.0.mdDrivers/m5pm1-module/README.mdDrivers/m5pm1-module/bindings/m5stack,m5pm1.yamlDrivers/m5pm1-module/devicetree.yamlDrivers/m5pm1-module/include/bindings/m5pm1.hDrivers/m5pm1-module/include/drivers/m5pm1.hDrivers/m5pm1-module/include/m5pm1_module.hDrivers/m5pm1-module/source/m5pm1.cppDrivers/m5pm1-module/source/module.cppDrivers/m5pm1-module/source/symbols.cDrivers/mpu6886-module/CMakeLists.txtDrivers/mpu6886-module/LICENSE-Apache-2.0.mdDrivers/mpu6886-module/README.mdDrivers/mpu6886-module/bindings/invensense,mpu6886.yamlDrivers/mpu6886-module/devicetree.yamlDrivers/mpu6886-module/include/bindings/mpu6886.hDrivers/mpu6886-module/include/drivers/mpu6886.hDrivers/mpu6886-module/include/mpu6886_module.hDrivers/mpu6886-module/source/module.cppDrivers/mpu6886-module/source/mpu6886.cppDrivers/mpu6886-module/source/symbols.cDrivers/qmi8658-module/CMakeLists.txtDrivers/qmi8658-module/LICENSE-Apache-2.0.mdDrivers/qmi8658-module/README.mdDrivers/qmi8658-module/bindings/qst,qmi8658.yamlDrivers/qmi8658-module/devicetree.yamlDrivers/qmi8658-module/include/bindings/qmi8658.hDrivers/qmi8658-module/include/drivers/qmi8658.hDrivers/qmi8658-module/include/qmi8658_module.hDrivers/qmi8658-module/source/module.cppDrivers/qmi8658-module/source/qmi8658.cppDrivers/qmi8658-module/source/symbols.cDrivers/rx8130ce-module/CMakeLists.txtDrivers/rx8130ce-module/LICENSE-Apache-2.0.mdDrivers/rx8130ce-module/README.mdDrivers/rx8130ce-module/bindings/epson,rx8130ce.yamlDrivers/rx8130ce-module/devicetree.yamlDrivers/rx8130ce-module/include/bindings/rx8130ce.hDrivers/rx8130ce-module/include/drivers/rx8130ce.hDrivers/rx8130ce-module/include/rx8130ce_module.hDrivers/rx8130ce-module/source/module.cppDrivers/rx8130ce-module/source/rx8130ce.cppDrivers/rx8130ce-module/source/symbols.cFirmware/idf_component.ymlModules/lvgl-module/source/symbols.cTactilityC/Source/symbols/gcc_soft_float_p4.cppTactilityC/Source/tt_init.cpp
💤 Files with no reviewable changes (1)
- Documentation/ideas.md
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)
Devices/m5stack-tab5/Source/Configuration.cpp (1)
121-122:⚠️ Potential issue | 🟠 MajorFix inverted pin directions for IP2326 charge control pins.
The directions are swapped:
ip2326_charge_state_led_pinshould be INPUT (reads status from IC), andip2326_charge_enable_pinshould be OUTPUT (controls charging from MCU). The current configuration prevents charging enable from functioning. The charge enable pin is never read or written, confirming the misconfiguration.Required fix
- gpio_descriptor_set_flags(ip2326_charge_state_led_pin, GPIO_FLAG_DIRECTION_OUTPUT); - gpio_descriptor_set_flags(ip2326_charge_enable_pin, GPIO_FLAG_DIRECTION_INPUT | GPIO_FLAG_PULL_UP); + gpio_descriptor_set_flags(ip2326_charge_state_led_pin, GPIO_FLAG_DIRECTION_INPUT); + gpio_descriptor_set_flags(ip2326_charge_enable_pin, GPIO_FLAG_DIRECTION_OUTPUT);
🧹 Nitpick comments (1)
Drivers/mpu6886-module/source/mpu6886.cpp (1)
124-136: Consider adding I2C controller type validation for consistency.The
start()andstop()functions validate that the parent device is an I2C controller, butmpu6886_read()does not. While the lifecycle ensuresstart()succeeds before reads occur, adding the check here would provide consistent defensive behavior and clearer error messages if misused.That said, given the Linux kernel style preference to avoid defensive checks in internal APIs (per project learnings), this is purely optional.
♻️ Optional: Add validation for consistency
error_t mpu6886_read(Device* device, Mpu6886Data* data) { auto* i2c_controller = device_get_parent(device); + if (device_get_type(i2c_controller) != &I2C_CONTROLLER_TYPE) { + return ERROR_RESOURCE; + } auto address = GET_CONFIG(device)->address;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0b7a6a48-7156-449a-a1c9-2f4b5dd9c26d
📒 Files selected for processing (12)
Devices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Detect.cppDevices/m5stack-tab5/Source/devices/Ili9881cDisplay.cppDevices/m5stack-tab5/Source/devices/St7123Display.cppDrivers/bm8563-module/include/drivers/bm8563.hDrivers/bm8563-module/source/bm8563.cppDrivers/bmi270-module/source/bmi270.cppDrivers/m5pm1-module/source/module.cppDrivers/mpu6886-module/source/mpu6886.cppDrivers/qmi8658-module/source/module.cppDrivers/rx8130ce-module/include/drivers/rx8130ce.hDrivers/rx8130ce-module/source/rx8130ce.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Drivers/bmi270-module/source/bmi270.cpp
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Devices/m5stack-tab5/Source/devices/St7123Display.cpp (1)
132-135:⚠️ Potential issue | 🟠 MajorIncomplete cleanup on panel creation failure.
When
esp_lcd_new_panel_st7123fails, the function returnsfalsebutmipiDsiBusandldoChannelremain allocated. The base classstart()will deleteioHandlebut won't release the DSI bus resources—they stay live until object destruction. This keeps the PHY powered and may make clean retries unreliable.Possible fix to clean up DSI resources on failure
if (esp_lcd_new_panel_st7123(ioHandle, &mutable_panel_config, &panelHandle) != ESP_OK) { LOGGER.error("Failed to create panel"); + // Note: ioHandle cleanup is handled by base class, but we need to release DSI bus + esp_lcd_del_dsi_bus(mipiDsiBus); + mipiDsiBus = nullptr; + esp_ldo_release_channel(ldoChannel); + ldoChannel = nullptr; return false; }
🧹 Nitpick comments (1)
Devices/m5stack-tab5/Source/devices/St7123Display.cpp (1)
9-19: Acknowledge the cleanup ordering concern noted in the TODO.The destructor serves as a fallback for releasing DSI bus resources, but if
stop()is not called before destruction,panelHandle(a base class member) may still internally referencemipiDsiBuswhen it's deleted here. While this is an architectural limitation of the base class (as your TODO notes), consider documenting that callers must callstop()before destruction for clean resource release.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b7bceb87-04c6-45a5-93dc-d2628a85cae0
📒 Files selected for processing (3)
Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cppDevices/m5stack-tab5/Source/devices/St7123Display.cppDrivers/rx8130ce-module/source/rx8130ce.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- Devices/m5stack-tab5/Source/devices/Ili9881cDisplay.cpp
KenVanHoeylandt
left a comment
KenVanHoeylandt
left a comment
There was a problem hiding this comment.
Great job! I have surprisingly little feedback for the amount of changes.
Keep in mind that any of my "minor" or "nitpick"-prefixed comments are suggestions and you can ignore them if you don't feel like processing them.
| constexpr auto LCD_VERTICAL_RESOLUTION = 240; | ||
| constexpr auto LCD_BUFFER_HEIGHT = LCD_VERTICAL_RESOLUTION / 3; | ||
| constexpr auto LCD_BUFFER_SIZE = LCD_HORIZONTAL_RESOLUTION * LCD_BUFFER_HEIGHT; | ||
| constexpr auto LCD_SPI_TRANSFER_SIZE_LIMIT = LCD_BUFFER_SIZE * LV_COLOR_DEPTH / 8; |
There was a problem hiding this comment.
Unused / can be removed?
There was a problem hiding this comment.
Probably. i think a lot of devices may need cleaning up stuff like this but I'll check what is and isn't used in this device. I copied one of the other sticks as a starting point.
|
|
||
| case IsCharging: { | ||
| uint8_t gpio_in = 0; | ||
| if (!tt::hal::i2c::masterReadRegister(M5PM1_I2C_PORT, M5PM1_ADDR, REG_GPIO_IN, &gpio_in, 1)) { |
There was a problem hiding this comment.
We can use the m5pm1 driver here. The constructor of Power can accept it as a ::Device* m5mp1Device.
You can find it by name from the tree via device_find_by_name("m5pm1")
A proper driver for the M5PM1 would be even better, but I don't mind to keep it in here as-is for now.
| // GPIO 23 has a pull-up resistor to 3V3, so INT is high at reset → GT911 uses 0x5D (primary) | ||
| // It may also appear at 0x14 (backup) if the pin happened to be driven low | ||
| if (tt::hal::i2c::masterHasDeviceAtAddress(I2C_NUM_0, ESP_LCD_TOUCH_IO_I2C_GT911_ADDRESS) || | ||
| tt::hal::i2c::masterHasDeviceAtAddress(I2C_NUM_0, ESP_LCD_TOUCH_IO_I2C_GT911_ADDRESS_BACKUP)) { |
There was a problem hiding this comment.
I'd rather see i2c_controller_has_device_at_address() being used.
tt::hal::i2c::* is deprecated - I just forgot to mark it as such.
| #pragma once | ||
| #include <esp_lcd_st7123.h> | ||
|
|
||
| static const st7123_lcd_init_cmd_t st7123_init_data[] = { |
There was a problem hiding this comment.
Was this copied from somewhere? If so, we should probably attribute it. If not, then just resolve this comment.
There was a problem hiding this comment.
It was copied from the tab5 user demo. https://github.com/m5stack/M5Tab5-UserDemo/blob/main/platforms/tab5/components/m5stack_tab5/m5stack_tab5.c#L1303
There was a problem hiding this comment.
We should probably put refer to its repo and its MIT license in the file, just in case.
https://github.com/m5stack/M5Tab5-UserDemo/blob/main/LICENSE
| if (err != ERROR_NONE) return err; | ||
| *out = static_cast<uint16_t>(buf[0] | (buf[1] << 8)); | ||
| return ERROR_NONE; | ||
| } |
There was a problem hiding this comment.
Would you mind moving this to TactilityKernel/source/drivers/i2c_controller.cpp and rename it to i2c_controller_register16le_get()?
Seems like it could be very useful for other drivers.
There was a problem hiding this comment.
If decide to move it, don't forget to rename Device* i2c to Device* device and addr to address, so that it's consistent with the other functions 😅
| // Enable DATA_RDY interrupt | ||
| if (i2c_controller_register8_set(i2c_controller, address, REG_INT_ENABLE, 0x01, I2C_TIMEOUT_TICKS) != ERROR_NONE) return ERROR_RESOURCE; | ||
|
|
||
| vTaskDelay(pdMS_TO_TICKS(100)); |
There was a problem hiding this comment.
Did you see errors without this delay?
If not, I'd remove it as there's a reasonable chance that no-one uses the device in that timeframe.
There was a problem hiding this comment.
Not sure. I only tested it with the cardputer and the m5stack IMU unit module (which is the mpu6886), i don't have any devices with it built-in.
| // Enable accel + gyro | ||
| if (i2c_controller_register8_set(i2c_controller, address, REG_CTRL7, CTRL7_ENABLE, I2C_TIMEOUT_TICKS) != ERROR_NONE) return ERROR_RESOURCE; | ||
|
|
||
| vTaskDelay(pdMS_TO_TICKS(10)); |
There was a problem hiding this comment.
Same here: if it's not causing errors, we can remove it.
|
|
||
| // Clear VLF and other status flags | ||
| uint8_t flag = 0x00; | ||
| if (i2c_controller_write_register(i2c_controller, address, REG_FLAG, &flag, 1, I2C_TIMEOUT_TICKS) != ERROR_NONE) { |
There was a problem hiding this comment.
We can probably use i2c_controller_register8_set() here.
2 participants
Font size set to 18 for 800x480 displays
Fix web server dashboard not rendering when sdcard isn't present
Added new driver modules
Applied the above modules to applicable devicetrees.
Added new device: M5Stack StickS3
Added new M5Stack Tab5 St7123 variant.
ButtonControl changed to use interupts and xQueue, added AppClose action.
And some bonus symbols of course, the apps are hungry for symbols.
Summary by CodeRabbit
New Features
Bug Fixes
Configuration Updates
Refactor