Conversation
📝 WalkthroughWalkthroughThis PR adds a complete LP5814 I2C LED driver (init/deinit, chip/current/LED control, DC/PWM programming, current calculation), a new LGFX_WIO_TRACKER_L2 display header integrating this driver as a backlight with GT911 touch, generic LGFX SPI/quad-SPI configuration macros, an ESP_LOG logging backend, and factory/config wiring for the new device. ChangesLED driver and display device integration
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant App
participant LGFX_WIO_TRACKER_L2
participant Wio_Tracker_Light
participant LP5814Chip
participant Wire
App->>LGFX_WIO_TRACKER_L2: init_impl()
LGFX_WIO_TRACKER_L2->>Wio_Tracker_Light: init(brightness)
Wio_Tracker_Light->>LP5814Chip: probe, configure, set DC/PWM
LGFX_WIO_TRACKER_L2->>LGFX_WIO_TRACKER_L2: base init_impl (GT911 probe)
LGFX_WIO_TRACKER_L2->>Wire: end()
LGFX_WIO_TRACKER_L2->>Wire: begin(sda, scl)
App->>LGFX_WIO_TRACKER_L2: sleep()
LGFX_WIO_TRACKER_L2->>Wio_Tracker_Light: setBrightness(0)
App->>LGFX_WIO_TRACKER_L2: wakeup()
LGFX_WIO_TRACKER_L2->>Wio_Tracker_Light: setBrightness(cached)
Wio_Tracker_Light->>LP5814Chip: write PWM registers
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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: 3
🧹 Nitpick comments (6)
drivers/lp5814.h (1)
184-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueIncomplete/incorrect Doxygen for the dim/engine/update setters.
The
@briefbodies are empty and the copied@returnlines referenceled_mask is invalid, but none of these functions take aled_maskparameter. Please fill in the briefs and drop the irrelevantled_maskclause so the public API docs are accurate.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/lp5814.h` around lines 184 - 221, Update the Doxygen comments for lp5814_set_led_dim_mode, lp5814_set_led_engine_mode, and lp5814_set_led_update_param so the `@brief` fields actually describe each setter’s purpose, and remove the incorrect “led_mask is invalid” text from the `@return` sections since these APIs do not take a led_mask parameter. Keep the return descriptions aligned with the actual handle and I2C failure behavior for each function.drivers/lp5814.c (2)
197-202: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
lp5814_initignores errors from the final configuration writes.The first three writes are error-checked and unwind on failure, but these six calls discard their return values, and the function then returns
ESP_OKunconditionally. If any of these I2C writes fail (e.g., NAK), init reports success while the device is left partially configured. Capture and propagate the results (and unwind consistently with the earlier failure path).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/lp5814.c` around lines 197 - 202, lp5814_init is ignoring failures from the final device-configuration writes, so update the lp5814_set_led_dim_mode, lp5814_set_led_engine_mode, lp5814_set_led_dc, lp5814_set_led_enable, lp5814_set_led_update_param, and lp5814_set_led_pwm calls to capture and check their return values. If any call fails, propagate the error from lp5814_init and unwind consistently with the earlier error path instead of returning ESP_OK unconditionally.
93-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused DC/PWM register helpers
drivers/lp5814.c:93-146—lp5814_get_dc_register()andlp5814_get_pwm_register()aren’t called anywhere, whilelp5814_set_led_dc()/lp5814_set_led_pwm()already compute the same offsets inline. Either delete the dead helpers or switch the loops to use them; keeping both risks drift. The current fallback to LED0 for unknown masks also makes them a poor drop-in replacement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@drivers/lp5814.c` around lines 93 - 146, Remove the dead DC/PWM register helpers in lp5814.c by either deleting lp5814_get_dc_register() and lp5814_get_pwm_register() or updating lp5814_set_led_dc() and lp5814_set_led_pwm() to call them consistently. If you keep the helpers, ensure their LED mask handling matches the existing loops and does not silently fall back to LED0 for invalid masks; otherwise remove the duplication to avoid drift.include/util/ILog.h (1)
34-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ILOG_TRACEmaps to the same verbosity asILOG_DEBUG.ESP-IDF has
ESP_LOGV(verbose), which is a level belowESP_LOGDand better matches "trace" semantics. MappingILOG_TRACEtoESP_LOGDmakes it indistinguishable fromILOG_DEBUGoutput/filtering.♻️ Suggested fix
-#define ILOG_TRACE(...) ESP_LOGD("DeviceUI", __VA_ARGS__) +#define ILOG_TRACE(...) ESP_LOGV("DeviceUI", __VA_ARGS__)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/util/ILog.h` around lines 34 - 44, `ILOG_TRACE` currently aliases `ESP_LOGD`, so trace logs are treated the same as debug logs in the `USE_ESP_LOG` branch of `ILog.h`. Update the `ILOG_TRACE` macro in that ESP log mapping block to use the verbose ESP-IDF log level instead, while keeping the other `ILOG_*` macros unchanged, so trace output is distinct from `ILOG_DEBUG`.include/graphics/LGFX/LGFX_GENERIC.h (1)
67-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winQuad-SPI pins applied unconditionally even if only partially defined.
The comment says quad pins are "only used if all 4 are defined," but each
LGFX_PIN_IOxmacro independently defaults to-1and all four are applied unconditionally tocfg.pin_io0-cfg.pin_io3. If a board only defines a subset (e.g.LGFX_PIN_IO0/IO1but notIO2/IO3), the bus gets a partially-valid quad-SPI pin configuration instead of falling back to standard SPI, which could misconfigure the bus for that board.💡 Suggested guard to enforce all-or-nothing quad pin definition
+#if defined(LGFX_PIN_IO0) || defined(LGFX_PIN_IO1) || defined(LGFX_PIN_IO2) || defined(LGFX_PIN_IO3) +#if !defined(LGFX_PIN_IO0) || !defined(LGFX_PIN_IO1) || !defined(LGFX_PIN_IO2) || !defined(LGFX_PIN_IO3) +#error "All four LGFX_PIN_IO0..IO3 must be defined together for quad SPI" +#endif +#endifAlso applies to: 254-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/graphics/LGFX/LGFX_GENERIC.h` around lines 67 - 82, The quad-SPI pin handling in LGFX_GENERIC.h is applied even when only some of the LGFX_PIN_IO0 through LGFX_PIN_IO3 macros are defined, so update the configuration logic around the cfg.pin_io0–cfg.pin_io3 assignments to require all four pins before enabling quad mode. Use the existing LGFX_PIN_IOx symbols in the generic display config path to add an all-or-nothing guard, and otherwise leave the bus in standard SPI mode when any of the quad pins are missing.include/graphics/LGFX/LGFX_WIO_TRACKER_L2.h (1)
103-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic-number I2C pins duplicated across the class.
Pins
47/48appear both inWire.begin(47, 48)(line 115) and in the touchcfg.pin_sda/cfg.pin_scl(lines 187-188). If one is updated without the other, the LP5814 recovery reset would silently reconfigure the bus with mismatched pins.♻️ Suggested consolidation
+ static constexpr int8_t kI2cSda = 47; + static constexpr int8_t kI2cScl = 48; + bool init_impl(bool use_reset, bool use_clear) override { ... - Wire.end(); - Wire.begin(47, 48); + Wire.end(); + Wire.begin(kI2cSda, kI2cScl);and use the same constants for
cfg.pin_sda/cfg.pin_sclin the touch config block.Also applies to: 185-190
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@include/graphics/LGFX/LGFX_WIO_TRACKER_L2.h` around lines 103 - 118, The I2C pin values are duplicated in both the recovery logic inside init_impl and the touch configuration block, which risks them drifting out of sync. Introduce shared named constants or a single source of truth in LGFX_WIO_TRACKER_L2 and use those symbols in both Wire.begin and the cfg.pin_sda/cfg.pin_scl assignments so the LP5814 reset and GT911 setup always use the same pins.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@include/graphics/LGFX/LGFX_WIO_TRACKER_L2.h`:
- Around line 45-60: The init() setup sequence in LGFX_WIO_TRACKER_L2 is
ignoring failed register writes and still reporting success. Update init() to
track the result of every writeReg call in the LP5814 configuration path,
including the loop over REG_LED0_DC and the final REG_UPDATE write, and return
the aggregated success state instead of unconditionally returning true. Keep the
existing setBrightness(brightness) call after the writes, but only report
initialization success when all register operations succeed.
- Around line 1-2: The include guard in LGFX_WIO_TRACKER_L2 is colliding with
another header because it still uses the LGFX_PICO_H symbol. Update the guard in
this header to a unique name tied to LGFX_WIO_TRACKER_L2, and make sure both the
define and matching endif protection use that new symbol so this header can be
included alongside LGFX_PICO_TFT_240x135 without being skipped.
---
Nitpick comments:
In `@drivers/lp5814.c`:
- Around line 197-202: lp5814_init is ignoring failures from the final
device-configuration writes, so update the lp5814_set_led_dim_mode,
lp5814_set_led_engine_mode, lp5814_set_led_dc, lp5814_set_led_enable,
lp5814_set_led_update_param, and lp5814_set_led_pwm calls to capture and check
their return values. If any call fails, propagate the error from lp5814_init and
unwind consistently with the earlier error path instead of returning ESP_OK
unconditionally.
- Around line 93-146: Remove the dead DC/PWM register helpers in lp5814.c by
either deleting lp5814_get_dc_register() and lp5814_get_pwm_register() or
updating lp5814_set_led_dc() and lp5814_set_led_pwm() to call them consistently.
If you keep the helpers, ensure their LED mask handling matches the existing
loops and does not silently fall back to LED0 for invalid masks; otherwise
remove the duplication to avoid drift.
In `@drivers/lp5814.h`:
- Around line 184-221: Update the Doxygen comments for lp5814_set_led_dim_mode,
lp5814_set_led_engine_mode, and lp5814_set_led_update_param so the `@brief` fields
actually describe each setter’s purpose, and remove the incorrect “led_mask is
invalid” text from the `@return` sections since these APIs do not take a led_mask
parameter. Keep the return descriptions aligned with the actual handle and I2C
failure behavior for each function.
In `@include/graphics/LGFX/LGFX_GENERIC.h`:
- Around line 67-82: The quad-SPI pin handling in LGFX_GENERIC.h is applied even
when only some of the LGFX_PIN_IO0 through LGFX_PIN_IO3 macros are defined, so
update the configuration logic around the cfg.pin_io0–cfg.pin_io3 assignments to
require all four pins before enabling quad mode. Use the existing LGFX_PIN_IOx
symbols in the generic display config path to add an all-or-nothing guard, and
otherwise leave the bus in standard SPI mode when any of the quad pins are
missing.
In `@include/graphics/LGFX/LGFX_WIO_TRACKER_L2.h`:
- Around line 103-118: The I2C pin values are duplicated in both the recovery
logic inside init_impl and the touch configuration block, which risks them
drifting out of sync. Introduce shared named constants or a single source of
truth in LGFX_WIO_TRACKER_L2 and use those symbols in both Wire.begin and the
cfg.pin_sda/cfg.pin_scl assignments so the LP5814 reset and GT911 setup always
use the same pins.
In `@include/util/ILog.h`:
- Around line 34-44: `ILOG_TRACE` currently aliases `ESP_LOGD`, so trace logs
are treated the same as debug logs in the `USE_ESP_LOG` branch of `ILog.h`.
Update the `ILOG_TRACE` macro in that ESP log mapping block to use the verbose
ESP-IDF log level instead, while keeping the other `ILOG_*` macros unchanged, so
trace output is distinct from `ILOG_DEBUG`.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a0b83900-6d57-4b76-8371-a8f70f7c3a87
📒 Files selected for processing (7)
drivers/lp5814.cdrivers/lp5814.hinclude/graphics/LGFX/LGFX_GENERIC.hinclude/graphics/LGFX/LGFX_WIO_TRACKER_L2.hinclude/graphics/driver/DisplayDriverConfig.hinclude/util/ILog.hsource/graphics/driver/DisplayDriverFactory.cpp
| // Set default max current to 25.5mA | ||
| ret = lp5814_set_max_current(handle, LP5814_MAX_CURRENT_51MA); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Comment/value mismatch and macro used where enum is expected.
The comment says "default max current to 25.5mA", but the code programs LP5814_MAX_CURRENT_51MA. Also, LP5814_MAX_CURRENT_51MA is the raw register-bit macro (0x01); the setter expects the lp5814_max_current_t enum. It compiles because the values coincide, but pass the enum to keep intent clear and align the comment with the actual default.
🔧 Proposed fix
- // Set default max current to 25.5mA
- ret = lp5814_set_max_current(handle, LP5814_MAX_CURRENT_51MA);
+ // Set default max current to 51mA
+ ret = lp5814_set_max_current(handle, LP5814_MAX_CURRENT_51_MA);📝 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 default max current to 25.5mA | |
| ret = lp5814_set_max_current(handle, LP5814_MAX_CURRENT_51MA); | |
| // Set default max current to 51mA | |
| ret = lp5814_set_max_current(handle, LP5814_MAX_CURRENT_51_MA); |
| #ifndef LGFX_PICO_H | ||
| #define LGFX_PICO_H |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for guard-name collisions with LGFX_PICO_H across the repo
rg -n 'LGFX_PICO_H' --type=cpp -g '!**/build/**'Repository: meshtastic/device-ui
Length of output: 158
🏁 Script executed:
#!/bin/bash
sed -n '1,20p' include/graphics/LGFX/LGFX_WIO_TRACKER_L2.hRepository: meshtastic/device-ui
Length of output: 617
🏁 Script executed:
#!/bin/bash
rg -n --hidden --glob '!**/.git/**' 'LGFX_PICO_H' .Repository: meshtastic/device-ui
Length of output: 434
Rename this include guard
LGFX_PICO_H is also used by include/graphics/LGFX/LGFX_PICO_TFT_240x135.h, so including both headers in one translation unit will suppress one of them. Use LGFX_WIO_TRACKER_L2_H here instead.
🐛 Suggested fix
-#ifndef LGFX_PICO_H
-#define LGFX_PICO_H
+#ifndef LGFX_WIO_TRACKER_L2_H
+#define LGFX_WIO_TRACKER_L2_H📝 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.
| #ifndef LGFX_PICO_H | |
| #define LGFX_PICO_H | |
| `#ifndef` LGFX_WIO_TRACKER_L2_H | |
| `#define` LGFX_WIO_TRACKER_L2_H |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/graphics/LGFX/LGFX_WIO_TRACKER_L2.h` around lines 1 - 2, The include
guard in LGFX_WIO_TRACKER_L2 is colliding with another header because it still
uses the LGFX_PICO_H symbol. Update the guard in this header to a unique name
tied to LGFX_WIO_TRACKER_L2, and make sure both the define and matching endif
protection use that new symbol so this header can be included alongside
LGFX_PICO_TFT_240x135 without being skipped.
| writeReg(REG_DEVICE_CONFIG0, 0x01); // chip enable | ||
| writeReg(REG_MAX_CURRENT, 0x01); // 51 mA max current | ||
| writeReg(REG_ENABLE_CONTROL, 0x00); // disable outputs while configuring | ||
| writeReg(REG_DIM_MODE, 0x4E); // dim mode config | ||
| writeReg(REG_ENGINE_MODE, 0xF0); // engine mode config | ||
| // Set DC current for all 4 channels (registers 0x14..0x17) | ||
| for (uint8_t i = 0; i < 4; i++) { | ||
| writeReg(REG_LED0_DC + i, 200); | ||
| } | ||
| writeReg(REG_ENABLE_CONTROL, 0x0F); // enable all 4 channels | ||
| writeReg(REG_UPDATE, 0x55); // latch parameters (LP5814 requires 0x55) | ||
| delay(5); // LP5814 engine startup settling time | ||
|
|
||
| setBrightness(brightness); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
init() returns true even when individual register writes fail.
Each writeReg call in the setup sequence logs on failure but doesn't affect the final return true;. A partial/failed LP5814 configuration would still be reported as successful init.
♻️ Suggested fix to propagate write failures
- void writeReg(uint8_t reg, uint8_t value)
+ bool writeReg(uint8_t reg, uint8_t value)
{
Wire.beginTransmission(LP5814_I2C_ADDR_DEFAULT);
Wire.write(reg);
Wire.write(value);
uint8_t error = Wire.endTransmission();
- if (error != 0)
+ if (error != 0) {
ILOG_ERROR("LP5814 write reg 0x%02x failed: %d", reg, error);
+ return false;
+ }
+ return true;
}Then aggregate the boolean results in init() (e.g. bool ok = true; ok &= writeReg(...); ...; return ok;) instead of unconditionally returning true.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@include/graphics/LGFX/LGFX_WIO_TRACKER_L2.h` around lines 45 - 60, The init()
setup sequence in LGFX_WIO_TRACKER_L2 is ignoring failed register writes and
still reporting success. Update init() to track the result of every writeReg
call in the LP5814 configuration path, including the loop over REG_LED0_DC and
the final REG_UPDATE write, and return the aggregated success state instead of
unconditionally returning true. Keep the existing setBrightness(brightness) call
after the writes, but only report initialization success when all register
operations succeed.
1 participant
QSPI display NV3031B
Summary by CodeRabbit
New Features
Bug Fixes