Tab5 features, StackChan, fixes, drivers....#526
Conversation
|
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:
📝 Walkthrough<review_stack_artifact> </review_stack_artifact> WalkthroughAdds BLE across many ESP32 device trees and properties. Introduces new INA226 and PY32 IO-expander modules with bindings and APIs. Adds the full M5Stack StackChan board (DTS, properties, module, display, power, SD) and includes it in CI. Refactors the FT6x36 touch driver and updates consumers. Extends I2S with TDM RX support and I2C 16-bit helpers. Enhances UI with quick-charge controls and a StackChan screensaver. Updates ST7789 to support SPIRAM buffering and adjusts several build dependencies and symbols. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
for now at least? memory usage on these devices is rough...
![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 (6)
Platforms/platform-esp32/source/drivers/esp32_i2s.cpp (1)
367-368: 💤 Low valueValidate
config->mclk_multiplebefore casting toi2s_mclk_multiple_t(Platforms/platform-esp32/source/drivers/esp32_i2s.cpp:367-368)
I2sTdmRxConfig::mclk_multipleis a rawuint32_tand is directly cast to.clk_cfg.mclk_multiplewith no validation in either the kernel forwarding layer or the ESP32 TDM setup. ESP-IDF’s validi2s_mclk_multiple_tvalues are{128, 192, 256, 384, 512, 576, 768, 1024, 1152}(and for 24-bit data, ESP-IDF recommends using a multiple-of-3 like384/576/768). Consider rejecting invalid values to avoid silently programming an unsupported MCLK ratio.Drivers/ina226-module/bindings/ti,ina226.yaml (1)
8-10: ⚡ Quick winAdd schema typing/range validation for
shunt-milliohms.
shunt-milliohmsis currently only markedrequired(notype/ numeric bounds), so invalid values can slip through schema validation. Constrain it to a positive integer.Suggested schema update
properties: shunt-milliohms: + type: int required: true + minimum: 1 description: Shunt resistor value in milliohmsDevices/m5stack-tab5/m5stack,tab5.dts (1)
89-95: ⚡ Quick winDocument or remap the UART1 pin overlap with
i2c1.
uart1is mapped to GPIO53/54, which are already used byi2c_port_a(Line 66 and Line 67). This is fine whilestatus = "disabled", but enabling UART later will silently conflict. A short DTS comment or alternate pin assignment would prevent accidental misconfiguration.Devices/m5stack-cores3/Source/devices/Display.cpp (1)
22-23: ⚡ Quick winReplace hard-coded touch bounds with derived constants.
Using
319/239with commented-out constants is brittle and obscures intent. Prefer deriving from panel constants (or explicit- 1if inclusive max is intentional) to prevent drift.♻️ Proposed cleanup
- 319,//LCD_HORIZONTAL_RESOLUTION, - 239,//LCD_VERTICAL_RESOLUTION, + LCD_HORIZONTAL_RESOLUTION, + LCD_VERTICAL_RESOLUTION,If the controller needs inclusive maxima:
- 319,//LCD_HORIZONTAL_RESOLUTION, - 239,//LCD_VERTICAL_RESOLUTION, + LCD_HORIZONTAL_RESOLUTION - 1, + LCD_VERTICAL_RESOLUTION - 1,Devices/m5stack-stackchan/Source/devices/Display.cpp (1)
21-22: ⚡ Quick winDerive touch bounds from display constants instead of hardcoded literals.
Using raw
319/239risks silent drift if panel resolution constants change. Keep the same behavior but tie it to shared constants.Proposed refactor
- 319,//LCD_HORIZONTAL_RESOLUTION, - 239,//LCD_VERTICAL_RESOLUTION, + LCD_HORIZONTAL_RESOLUTION - 1, + LCD_VERTICAL_RESOLUTION - 1,Devices/m5stack-stackchan/Source/devices/SdCard.h (1)
3-7: ⚡ Quick winHarden this public header against transitive-include breakage.
Please include
<memory>explicitly and avoid the globalusingalias in a header to prevent namespace leakage into all includers.Proposed change
+#include <memory> `#include` "Tactility/hal/sdcard/SdCardDevice.h" - -using tt::hal::sdcard::SdCardDevice; - -std::shared_ptr<SdCardDevice> createSdCard(); +std::shared_ptr<tt::hal::sdcard::SdCardDevice> createSdCard();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 23702d88-de95-45f1-afbb-7303695ec2ff
⛔ Files ignored due to path filters (2)
partitions-16mb.csvis excluded by!**/*.csvpartitions-8mb.csvis excluded by!**/*.csv
📒 Files selected for processing (137)
.github/workflows/build.ymlBuildscripts/DevicetreeCompiler/source/generator.pyBuildscripts/DevicetreeCompiler/tests/data/expected_devicetree.cBuildscripts/TactilitySDK/CMakeLists.txtDevices/btt-panda-touch/bigtreetech,panda-touch.dtsDevices/btt-panda-touch/device.propertiesDevices/cyd-4848s040c/cyd,4848s040c.dtsDevices/cyd-4848s040c/device.propertiesDevices/cyd-8048s043c/cyd,8048s043c.dtsDevices/cyd-8048s043c/device.propertiesDevices/elecrow-crowpanel-advance-28/device.propertiesDevices/elecrow-crowpanel-advance-28/elecrow,crowpanel-advance-28.dtsDevices/elecrow-crowpanel-advance-35/device.propertiesDevices/elecrow-crowpanel-advance-35/elecrow,crowpanel-advance-35.dtsDevices/elecrow-crowpanel-advance-50/device.propertiesDevices/elecrow-crowpanel-advance-50/elecrow,crowpanel-advance-50.dtsDevices/elecrow-crowpanel-basic-50/device.propertiesDevices/elecrow-crowpanel-basic-50/elecrow,crowpanel-basic-50.dtsDevices/guition-jc1060p470ciwy/device.propertiesDevices/guition-jc1060p470ciwy/guition,jc1060p470ciwy.dtsDevices/guition-jc3248w535c/device.propertiesDevices/guition-jc3248w535c/guition,jc3248w535c.dtsDevices/guition-jc8048w550c/device.propertiesDevices/guition-jc8048w550c/guition,jc8048w550c.dtsDevices/heltec-wifi-lora-32-v3/device.propertiesDevices/heltec-wifi-lora-32-v3/heltec,wifi-lora-32-v3.dtsDevices/lilygo-tdeck/Source/devices/Display.cppDevices/lilygo-tdeck/lilygo,tdeck.dtsDevices/lilygo-tdisplay-s3/device.propertiesDevices/lilygo-tdisplay-s3/lilygo,tdisplay-s3.dtsDevices/lilygo-tdongle-s3/device.propertiesDevices/lilygo-tdongle-s3/lilygo,tdongle-s3.dtsDevices/lilygo-thmi/device.propertiesDevices/lilygo-thmi/lilygo,thmi.dtsDevices/lilygo-tlora-pager/Source/drivers/TloraPager.cppDevices/lilygo-tlora-pager/device.propertiesDevices/lilygo-tlora-pager/lilygo,tlora-pager.dtsDevices/m5stack-cardputer-adv/device.propertiesDevices/m5stack-cardputer-adv/m5stack,cardputer-adv.dtsDevices/m5stack-cardputer/device.propertiesDevices/m5stack-cardputer/m5stack,cardputer.dtsDevices/m5stack-core2/Source/devices/Display.cppDevices/m5stack-cores3/CMakeLists.txtDevices/m5stack-cores3/Source/devices/Display.cppDevices/m5stack-cores3/device.propertiesDevices/m5stack-cores3/m5stack,cores3.dtsDevices/m5stack-papers3/device.propertiesDevices/m5stack-papers3/m5stack,papers3.dtsDevices/m5stack-stackchan/CMakeLists.txtDevices/m5stack-stackchan/Source/Configuration.cppDevices/m5stack-stackchan/Source/devices/Display.cppDevices/m5stack-stackchan/Source/devices/Display.hDevices/m5stack-stackchan/Source/devices/Power.cppDevices/m5stack-stackchan/Source/devices/Power.hDevices/m5stack-stackchan/Source/devices/SdCard.cppDevices/m5stack-stackchan/Source/devices/SdCard.hDevices/m5stack-stackchan/Source/module.cppDevices/m5stack-stackchan/device.propertiesDevices/m5stack-stackchan/devicetree.yamlDevices/m5stack-stackchan/m5stack,stackchan.dtsDevices/m5stack-sticks3/CMakeLists.txtDevices/m5stack-sticks3/Source/Configuration.cppDevices/m5stack-sticks3/Source/devices/SdCard.cppDevices/m5stack-sticks3/Source/devices/SdCard.hDevices/m5stack-sticks3/device.propertiesDevices/m5stack-sticks3/m5stack,sticks3.dtsDevices/m5stack-tab5/CMakeLists.txtDevices/m5stack-tab5/Source/Configuration.cppDevices/m5stack-tab5/Source/devices/Power.cppDevices/m5stack-tab5/Source/devices/Power.hDevices/m5stack-tab5/Source/module.cppDevices/m5stack-tab5/device.propertiesDevices/m5stack-tab5/devicetree.yamlDevices/m5stack-tab5/m5stack,tab5.dtsDevices/unphone/device.propertiesDevices/unphone/unphone.dtsDevices/waveshare-esp32-s3-geek/device.propertiesDevices/waveshare-esp32-s3-geek/waveshare,esp32-s3-geek.dtsDevices/waveshare-s3-lcd-13/device.propertiesDevices/waveshare-s3-lcd-13/waveshare,s3-lcd-13.dtsDevices/waveshare-s3-touch-lcd-128/device.propertiesDevices/waveshare-s3-touch-lcd-128/waveshare,s3-touch-lcd-128.dtsDevices/waveshare-s3-touch-lcd-147/device.propertiesDevices/waveshare-s3-touch-lcd-147/waveshare,s3-touch-lcd-147.dtsDevices/waveshare-s3-touch-lcd-43/device.propertiesDevices/waveshare-s3-touch-lcd-43/waveshare,s3-touch-lcd-43.dtsDevices/wireless-tag-wt32-sc01-plus/Source/devices/Display.cppDevices/wireless-tag-wt32-sc01-plus/device.propertiesDevices/wireless-tag-wt32-sc01-plus/wireless-tag,wt32-sc01-plus.dtsDrivers/FT6x36/CMakeLists.txtDrivers/FT6x36/Source/Ft6x36Touch.cppDrivers/FT6x36/Source/Ft6x36Touch.hDrivers/FT6x36/Source/ft6x36/FT6X36.cppDrivers/FT6x36/Source/ft6x36/FT6X36.hDrivers/FT6x36/Source/ft6x36/LICENSEDrivers/FT6x36/Source/ft6x36/README.mdDrivers/ST7789/Source/St7789Display.cppDrivers/ST7789/Source/St7789Display.hDrivers/ina226-module/CMakeLists.txtDrivers/ina226-module/README.mdDrivers/ina226-module/bindings/ti,ina226.yamlDrivers/ina226-module/devicetree.yamlDrivers/ina226-module/include/bindings/ina226.hDrivers/ina226-module/include/drivers/ina226.hDrivers/ina226-module/include/ina226_module.hDrivers/ina226-module/source/ina226.cppDrivers/ina226-module/source/module.cppDrivers/ina226-module/source/symbols.cDrivers/m5pm1-module/include/drivers/m5pm1.hDrivers/m5pm1-module/source/m5pm1.cppDrivers/py32ioexpander-module/CMakeLists.txtDrivers/py32ioexpander-module/bindings/m5stack,py32ioexpander.yamlDrivers/py32ioexpander-module/devicetree.yamlDrivers/py32ioexpander-module/include/bindings/py32ioexpander.hDrivers/py32ioexpander-module/include/drivers/py32ioexpander.hDrivers/py32ioexpander-module/include/py32ioexpander_module.hDrivers/py32ioexpander-module/source/module.cppDrivers/py32ioexpander-module/source/py32ioexpander.cppFirmware/idf_component.ymlModules/lvgl-module/source/symbols.cPlatforms/platform-esp32/source/drivers/esp32_i2s.cppTactility/Include/Tactility/hal/power/PowerDevice.hTactility/Include/Tactility/settings/DisplaySettings.hTactility/Source/app/display/Display.cppTactility/Source/app/power/Power.cppTactility/Source/service/displayidle/DisplayIdle.cppTactility/Source/service/displayidle/StackChanScreensaver.cppTactility/Source/service/displayidle/StackChanScreensaver.hTactility/Source/settings/DisplaySettings.cppTactilityC/Source/symbols/freertos.cppTactilityC/Source/symbols/stl.cppTactilityKernel/include/tactility/drivers/i2c_controller.hTactilityKernel/include/tactility/drivers/i2s_controller.hTactilityKernel/source/drivers/i2c_controller.cppTactilityKernel/source/drivers/i2s_controller.cppTactilityKernel/source/kernel_symbols.cdevice.py
💤 Files with no reviewable changes (4)
- Drivers/FT6x36/Source/ft6x36/README.md
- Drivers/FT6x36/Source/ft6x36/FT6X36.cpp
- Drivers/FT6x36/Source/ft6x36/LICENSE
- Drivers/FT6x36/Source/ft6x36/FT6X36.h
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Drivers/py32ioexpander-module/source/py32ioexpander.cpp (1)
112-119:⚠️ Potential issue | 🔴 Critical | ⚡ Quick win
countis undefined; should validateindexparameter.Line 113 references
countwhich does not exist in this function scope. This will fail to compile. The bounds check should validateindexagainst the maximum LED count (32).🐛 Proposed fix
error_t py32_led_set_color(Device* device, uint8_t index, uint8_t r, uint8_t g, uint8_t b) { - if (count > 32U) return ERROR_RESOURCE; + if (index >= 32U) return ERROR_RESOURCE; Device* i2c = device_get_parent(device);
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33f18d7f-4b92-4d71-a445-e1a886b5eed9
📒 Files selected for processing (6)
Devices/m5stack-stackchan/Source/Configuration.cppDevices/m5stack-tab5/Source/module.cppDrivers/ina226-module/bindings/ti,ina226.yamlDrivers/ina226-module/source/ina226.cppDrivers/m5pm1-module/source/m5pm1.cppDrivers/py32ioexpander-module/source/py32ioexpander.cpp
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Drivers/py32ioexpander-module/source/py32ioexpander.cpp (1)
112-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject LED index
32; only0..31are valid.Line 113 still allows
index == 32, but the data window only has 32 slots. That makes the write targetREG_LED_DATA + 64, which is outside the valid LED payload range.Proposed fix
error_t py32_led_set_color(Device* device, uint8_t index, uint8_t r, uint8_t g, uint8_t b) { - if (index > 32U) return ERROR_RESOURCE; + if (index >= 32U) return ERROR_RESOURCE; Device* i2c = device_get_parent(device); // RGB565: [15:11]=R5 [10:5]=G6 [4:0]=B5, stored little-endian uint16_t rgb565 = static_cast<uint16_t>(((r >> 3U) << 11U) | ((g >> 2U) << 5U) | (b >> 3U)); uint8_t buf[2] = {static_cast<uint8_t>(rgb565 & 0xFFU), static_cast<uint8_t>(rgb565 >> 8U)}; return i2c_controller_write_register(i2c, GET_CONFIG(device)->address, static_cast<uint8_t>(REG_LED_DATA + index * 2U), buf, 2, TIMEOUT); }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cae01ce6-09ad-4d3a-ab22-a07da670da5d
📒 Files selected for processing (1)
Drivers/py32ioexpander-module/source/py32ioexpander.cpp
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Devices/m5stack-stackchan/Source/Configuration.cpp (1)
234-234: ⚡ Quick winHardcoded
I2C_NUM_0matches"i2c_internal"on StackChan.
i2c_internalin the StackChan devicetree setsport = <I2C_NUM_0>, andinitBoot()looks updevice_find_by_name("i2c_internal"), soAxp2101(I2C_NUM_0)should use the correct bus for the current configuration.Still, to reduce future drift, derive the port passed to
Axp2101from thei2c_internaldevice handle (or the same DT constant) instead of hardcoding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a812d611-6dd2-4527-9a3e-792c962e7606
📒 Files selected for processing (1)
Devices/m5stack-stackchan/Source/Configuration.cpp
| #include <Tactility/hal/Configuration.h> | ||
| #include <Axp2101Power.h> | ||
| #include <AXP2101.h> | ||
| #include <Axp2101.h> |
There was a problem hiding this comment.
All driver h/c/cpp files are CamelCase, so let's not change that.
|
|
||
| static std::shared_ptr<tt::hal::touch::TouchDevice> createTouch() { | ||
| auto configuration = std::make_unique<Ft6x36Touch::Configuration>( | ||
| auto configuration = std::make_unique<FT6x36Touch::Configuration>( |
There was a problem hiding this comment.
All driver h/c/cpp files are CamelCase, so let's not change that.
| shunt-milliohms: | ||
| type: int | ||
| required: true | ||
| minimum: 1 |
There was a problem hiding this comment.
Is "minimum" supported? If not: Perhaps just add a comment that it should be a minimum of 1.
2 participants
Enable bluetooth for all devices
Tab5 performance improvements
Tab5 Power added along with ina226 driver module
Updated esp_lvgl_port to 2.7.2 for said improvements to work
Added StackChan device + py32ioexpander driver module
Reduced T-Deck memory pressure by shifting display buffers to psram
Added microphone support to i2s_controller
StackChan screensaver because why not
Updated CoreS3 / StackChan touch driver / offsets... spoiler, it still kinda sucks. :(
Added StickS3 sdcard for those who want to make a diy module.
Bonus symbols as usual.
Fixed indentation in DisplayIdle
Added some new functions to i2c_controller
Increased 8MB and 16MB app partition size by 1MB
Probably more... I'm sure the rabbit will summarize it all.
Summary by CodeRabbit
New Features
Improvements