Conversation
Reset and interrupt pins on an IO expander were passed to esp_lcd_touch as native GPIO numbers. The driver now resets the controller itself for such pins, holding INT low so it comes up at address 0x5D. Native pins behave as before.
New driver modules: - nv3031b-module: NV3031B display over QSPI. The esp_lcd panel is adapted from Espressif's esp_lcd_sh8601 (Apache-2.0), the init sequence is from LovyanGFX (BSD-2-Clause). - lp5814-module: TI LP5814 LED driver as display backlight. The device folder is dts-only. Working, tested on hardware: display (320x240 landscape), touch, backlight brightness, SD card (SDMMC 1-bit), WiFi, GPS detection (L76K on UART1). Not implemented yet: battery voltage (ADS1115), buttons (BOOT, WAKE), audio (ES8311 speaker, ES7243E microphone), LoRa (SX1262), user LED, Grove port, USB-C controller (AW35615).
New driver modules: - ads1115-module: TI ADS1115 ADC, written from the datasheet. The device uses it with battery-sense for the battery voltage. - es7243e-module: ES7243E microphone ADC, wraps the esp_codec_dev implementation. Device additions: ES8311 speaker and ES7243E microphone on I2S, battery voltage, SX1262 LoRa radio, Grove port, WAKE and BOOT buttons as navigation keys. Tested on hardware: speaker and microphone (audio recorder app), battery voltage and charge level, both buttons, SX1262 probe. Not tested: LoRa transmit and receive, Grove port.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (14)
🚧 Files skipped from review as they are similar to previous changes (5)
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughAdds device-tree and module configuration for the Seeed Studio Wio Tracker L2 Pro. Adds ADS1115, ES7243E, LP5814, and NV3031B driver modules. Updates GT911 startup to handle reset and interrupt pins on GPIO expanders. Changes TCA95xx GPIO handling to invert active-low output levels and accept active-low output flags. Adds license files and third-party notices for the new modules and adapted display code. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to Short audio-read deadlines can be exceeded, and display orientation queries can be stale after a runtime change. Resolve or explicitly accept these limitations before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The changes are primarily confined to hardware support. Failed initialization can leave reset or interrupt pins configured without an owner, but no expanded remote access or credential authority is demonstrated. Remaining uncertainty concerns hardware recovery and integrations outside the reviewed code. Retained concerns
Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 15.71% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 70 functions across 25 files. (6 skipped: 6 unsupported.)
✨ 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
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Organization UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
2935f7dd-f59e-42c4-ba83-f1ad00320082
📒 Files selected for processing (51)
Devices/seeed-wio-tracker-l2-pro/LICENSE-Apache-2.0.mdDevices/seeed-wio-tracker-l2-pro/device.propertiesDevices/seeed-wio-tracker-l2-pro/module.yamlDevices/seeed-wio-tracker-l2-pro/seeed,wio-tracker-l2-pro.dtsDrivers/ads1115-module/CMakeLists.txtDrivers/ads1115-module/LICENSE-Apache-2.0.mdDrivers/ads1115-module/README.mdDrivers/ads1115-module/bindings/ti,ads1115.yamlDrivers/ads1115-module/include/ads1115_module.hDrivers/ads1115-module/include/bindings/ads1115.hDrivers/ads1115-module/include/drivers/ads1115.hDrivers/ads1115-module/module.yamlDrivers/ads1115-module/source/ads1115.cppDrivers/ads1115-module/source/module.cppDrivers/es7243e-module/CMakeLists.txtDrivers/es7243e-module/LICENSE-Apache-2.0.mdDrivers/es7243e-module/README.mdDrivers/es7243e-module/bindings/everest,es7243e.yamlDrivers/es7243e-module/include/bindings/es7243e.hDrivers/es7243e-module/include/drivers/es7243e.hDrivers/es7243e-module/include/es7243e_module.hDrivers/es7243e-module/module.yamlDrivers/es7243e-module/source/es7243e.cppDrivers/es7243e-module/source/module.cppDrivers/gt911-module/bindings/goodix,gt911.yamlDrivers/gt911-module/source/gt911.cppDrivers/lp5814-module/CMakeLists.txtDrivers/lp5814-module/LICENSE-Apache-2.0.mdDrivers/lp5814-module/README.mdDrivers/lp5814-module/bindings/ti,lp5814.yamlDrivers/lp5814-module/include/bindings/lp5814.hDrivers/lp5814-module/include/drivers/lp5814.hDrivers/lp5814-module/include/lp5814_module.hDrivers/lp5814-module/module.yamlDrivers/lp5814-module/source/lp5814.cppDrivers/lp5814-module/source/module.cppDrivers/nv3031b-module/CMakeLists.txtDrivers/nv3031b-module/LICENSE-Apache-2.0.mdDrivers/nv3031b-module/LICENSE-BSD-2-Clause.mdDrivers/nv3031b-module/README.mdDrivers/nv3031b-module/bindings/newvision,nv3031b.yamlDrivers/nv3031b-module/include/bindings/nv3031b.hDrivers/nv3031b-module/include/drivers/nv3031b.hDrivers/nv3031b-module/include/esp_lcd_nv3031b.hDrivers/nv3031b-module/include/nv3031b_module.hDrivers/nv3031b-module/module.yamlDrivers/nv3031b-module/source/esp_lcd_nv3031b.cDrivers/nv3031b-module/source/module.cppDrivers/nv3031b-module/source/nv3031b.cppDrivers/nv3031b-module/source/nv3031b_init_cmds.hTHIRD-PARTY-NOTICES.md
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
| } | ||
|
|
||
| error_t read(Device* device, void* buffer, size_t size, size_t* bytes_read, TickType_t timeout) { | ||
| (void) timeout; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🔎 Supported by static analysis
🏁 Script executed:
printf '%s\n' '--- changed driver and nearby read implementation ---'
nl -ba Drivers/es7243e-module/source/es7243e.cpp | sed -n '1,190p'
printf '%s\n' '--- PR diff for driver ---'
git diff --no-ext-diff --unified=60 04ada9df8789fc2a7549073cc84a03c0b3d42424 18f94a829e0872020ceedd6359e4deb4e2b0e64c -- Drivers/es7243e-module/source/es7243e.cpp
printf '%s\n' '--- read API and implementation references ---'
rg -n -F -- 'esp_codec_dev_read' .
printf '%s\n' '--- codec read contract / timeout signatures ---'
rg -n -i -- 'timeout|read\(' Drivers/audio-stream-module Drivers/es7243e-module | head -240
printf '%s\n' '--- adapter-related source files ---'
rg --files | rg -i '(i2s|audio.stream|codec)' | head -160Repository: TactilityProject/Tactility
Length of output: 23861
🤖 get_repo_knowledge executed:
get_repo_knowledge TactilityProject/Tactility /tmp/coderabbit-repo-knowledge/tactilityproject-tactility-59065d0e/architecture
Length of output: 40600
🏁 Script executed:
printf '%s\n' '--- I2S data adapter ---'
nl -ba Drivers/audio-codec-module/source/audio_codec_data_if_i2s.c | sed -n '1,260p'
printf '%s\n' '--- audio stream read and cancellation-related flow ---'
nl -ba Drivers/audio-stream-module/source/audio_stream.cpp | sed -n '280,410p'
nl -ba Drivers/audio-stream-module/source/audio_stream.cpp | sed -n '520,630p'
rg -n -i -- 'cancel|abort|timeout|read_stream|audio_codec_read' Drivers/audio-stream-module/source/audio_stream.cpp TactilityKernel/source/drivers/audio_stream.cpp TactilityKernel/source/drivers/audio_codec.cpp
printf '%s\n' '--- audio codec and I2S APIs ---'
nl -ba TactilityKernel/include/tactility/drivers/audio_codec.h | sed -n '1,220p'
nl -ba TactilityKernel/include/tactility/drivers/i2s_controller.h | sed -n '1,230p'
nl -ba TactilityKernel/source/drivers/i2s_controller.cpp | sed -n '1,240p'
printf '%s\n' '--- ESP32 I2S implementation ---'
rg -n -F -- 'i2s_controller_read' Platforms/platform-esp32/source/drivers/esp32_i2s.cpp
nl -ba Platforms/platform-esp32/source/drivers/esp32_i2s.cpp | sed -n '1,320p'
printf '%s\n' '--- codec dependency metadata and local sources ---'
rg -n -i -- 'esp.audio|esp_audio|esp_codec_dev|esp-codec' Tactility/idf_component.yml Drivers/es7243e-module Drivers/audio-codec-module
rg --files | rg '(^|/)(esp_codec_dev|esp_codec_dev_defaults)(\.h|\.c|\.cpp)?$|esp_codec_dev' | head -100Repository: TactilityProject/Tactility
Length of output: 42635
🏁 Script executed:
printf '%s\n' '--- audio codec read API declarations and wrapper ---'
rg -n -C 5 -- 'audio_codec_read|AudioCodecApi|read\\).*timeout|TickType_t timeout' TactilityKernel/include/tactility/drivers/audio_codec.h TactilityKernel/source/drivers/audio_codec.cpp
printf '%s\n' '--- I2S controller read API and wrapper ---'
rg -n -C 6 -- 'i2s_controller_read|I2sApi|TickType_t timeout' TactilityKernel/include/tactility/drivers/i2s_controller.h TactilityKernel/source/drivers/i2s_controller.cpp
printf '%s\n' '--- stream read completion / contract docs ---'
nl -ba Drivers/audio-stream-module/source/audio_stream.cpp | sed -n '553,625p'
rg -n -i -C 3 -- 'timeout|read' Drivers/audio-stream-module/include Drivers/audio-stream-module/README.md TactilityKernel/include/tactility/drivers/audio_stream.h
printf '%s\n' '--- exact adapter binding and timeout implementation ---'
nl -ba Drivers/es7243e-module/source/es7243e.cpp | sed -n '90,110p;230,282p'
nl -ba Drivers/audio-codec-module/source/audio_codec_data_if_i2s.c | sed -n '8,14p;128,139p'
nl -ba Platforms/platform-esp32/source/drivers/esp32_i2s.cpp | sed -n '161,170p'
printf '%s\n' '--- dependency version ---'
nl -ba Tactility/idf_component.yml | sed -n '1,8p'Repository: TactilityProject/Tactility
Length of output: 25180
🏁 Script executed:
printf '%s\n' '--- audio codec API declaration ---'
rg -n -F -C 7 -- 'audio_codec_read' TactilityKernel/include/tactility/drivers/audio_codec.h TactilityKernel/source/drivers/audio_codec.cpp
printf '%s\n' '--- codec API read callback declaration ---'
rg -n -F -C 6 -- '(*read)' TactilityKernel/include/tactility/drivers/audio_codec.h
printf '%s\n' '--- audio codec source with line numbers ---'
nl -ba TactilityKernel/source/drivers/audio_codec.cpp | sed -n '1,130p'Repository: TactilityProject/Tactility
Length of output: 7907
Bound I2S reads by the caller’s deadline.
AudioCodecApi::read defines timeout as the maximum wait, and the audio-stream layer forwards that timeout to this callback. The ES7243E callback discards it. Its I2S adapter instead allows up to 200 ms per transfer, so a stalled transfer can exceed a shorter caller deadline. The stream layer cannot cancel an in-flight codec read. Use a read path that applies the caller’s remaining deadline to I2S transfers and returns ERROR_TIMEOUT when it expires.
There was a problem hiding this comment.
Drivers es7210, es8311, es8388 and aw88298 ignore the timeout in the same way, because esp_codec_dev_read() has no timeout parameter and the 200 ms limit is in the shared I2S adapter (audio_codec_data_if_i2s.c). Honoring the caller's deadline would mean changing that adapter for all codecs.
There was a problem hiding this comment.
@AdaSzi Thanks for the clarification. esp_codec_dev_read() has no timeout parameter, and the shared I2S adapter sets the 200 ms limit. My recommendation was too narrowly scoped to ES7243E.
The caller-deadline limitation remains, but it is a shared audio-codec issue, not an ES7243E-specific regression. A fix should cover all affected codecs rather than require a separate read path in this driver.
Would you like me to open a follow-up issue for the shared adapter?
✏️ Learnings added
Learnt from: AdaSzi
Repo: TactilityProject/Tactility PR: 676
File: Drivers/es7243e-module/source/es7243e.cpp:97-97
Timestamp: 2026-10-05T21:54:54.962Z
Learning: In Tactility, the ES7243E, ES7210, ES8311, ES8388, and AW88298 audio codec drivers ignore the read timeout because esp_codec_dev_read() has no timeout parameter. Drivers/audio-codec-module/source/audio_codec_data_if_i2s.c applies a shared 200 ms I2S transfer timeout. Caller-deadline support requires a shared audio-codec adapter solution; do not treat this limitation as an ES7243E-specific regression.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
You are interacting with an AI system.
|
|
||
| // esp_lcd_touch only accepts native pins, so the reset is done here. INT is held low while reset | ||
| // is released, which selects address 0x5D. Physical levels are used because IO expanders reject | ||
| // ACTIVE_LOW on outputs. |
There was a problem hiding this comment.
// is released, which selects address 0x5D. Physical levels are used because IO expanders reject
Comment view// ACTIVE_LOW on outputs.
IO expanders generally should support ACTIVE_LOW. If it's not natively supported, it should be done in software.
I'd rather have that fixed, but if you don't feel like fixing that driver, then please add it to ideas.md in the "high prio" section. Thanks!
There was a problem hiding this comment.
Updated in newest commit
| // Bit mask of OUT0 to OUT3 | ||
| uint8_t channels; | ||
| bool high_current; | ||
| uint8_t dot_current; |
There was a problem hiding this comment.
It is useful to add what the units are. (e.g. mA)
There was a problem hiding this comment.
Updated in newest commit
|
|
||
| #include <new> | ||
|
|
||
| #define TAG "LP5814" |
There was a problem hiding this comment.
Minor: the new drivers use #define for TAG, but should use static constexpr auto* TAG
There was a problem hiding this comment.
Updated in newest commit
| return pin.gpio_controller == nullptr ? -1 : static_cast<int>(pin.pin); | ||
| } | ||
|
|
||
| // Drives physical levels because IO expanders reject ACTIVE_LOW on outputs |
There was a problem hiding this comment.
Same here: if not fixed, please mention it in ideas.md at high prio.
There was a problem hiding this comment.
Updated in newest commit
A native reset pin combined with an expander interrupt pin now keeps its configured polarity and the reset is pulsed reset-pulses times like on the native path.
…otes - TCA95xx: active-low outputs are inverted in software. The GT911 and NV3031B drivers use that for their reset pins instead of driving physical levels and the device sets GPIO_FLAG_ACTIVE_LOW on the touch reset pin. ideas.md notes the same limitation in xl9555 and tca9534. - LP5814: document the units of the current settings. - New drivers use constexpr for TAG. - Device: drive expander pin 4 high like the vendor firmware. Without it the audio codecs do not answer after a cold boot.
2 participants
Adds the Seeed Studio Wio Tracker L2 Pro: ESP32-S3 (16 MB flash, 8 MB PSRAM), 3.2" 320x240 IPS display with an NV3031B controller on QSPI, GT911 touch, SD card, L76K GPS, SX1262 LoRa, ES8311 speaker codec, ES7243E microphone ADC and an ADS1115 for the battery voltage. A PCA9555 IO expander switches the power and reset lines of most parts. Product page: https://wiki.seeedstudio.com/meshtastic_wio_tracker_l2_intro/
The device folder is dts-only. New driver modules:
nv3031b-module: NV3031B display over QSPI (esp_lcd panel plus Tactility display driver). The reset pin can be on an IO expander.lp5814-module: TI LP5814 LED driver used as the display backlight.ads1115-module: TI ADS1115 ADC, used withbattery-sensefor the battery voltage.es7243e-module: ES7243E microphone ADC, a wrapper around theesp_codec_devimplementation (same approach ases7210-module).One change to an existing driver:
gt911-modulenow handles reset and interrupt pins on an IO expander. Before, such pins were passed toesp_lcd_touchas native GPIO numbers (on this board that would have toggled GPIO8 and GPIO3). For expander pins the driver now does the reset itself and holds INT low, so the GT911 comes up at 0x5D. That matters here because the ES7243E already uses 0x14. Boards with native pins behave exactly as before.Licensing
nv3031b-module/source/nv3031b_init_cmds.hcontains the init sequence from LovyanGFX (FreeBSD / BSD-2-Clause). It has its own SPDX header and license file in the module.esp_lcd_nv3031b.cis adapted from Espressif'sesp_lcd_sh8601(Apache-2.0), which uses the same QSPI command format.THIRD-PARTY-NOTICES.md. Everything else is Apache-2.0. The LP5814 and ADS1115 drivers are written from the TI datasheets. No code from the Meshtastic firmware was copied, it was only used as a reference for the expander pins and the power-up order.Testing
button-control.Known gaps
Summary by CodeRabbit
New Features
Bug Fixes
Documentation