feat: Crowpanel V2 brightness control#203
Conversation
📝 WalkthroughWalkthroughAdds a new Elecrow_V2_Light class implementing ILight that detects V2/V3 ELECROW light controllers over I2C and maps brightness to controller commands. Integrates this into LGFX_ELECROW70's init_impl, light() accessor, and sleep()/wakeup() to conditionally control backlight via I2C versus TCA9534 IO expander. ChangesELECROW V2 Light Integration
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant InitImpl as LGFX_ELECROW70::init_impl
participant I2C
participant LightInstance as Elecrow_V2_Light
participant TCA9534
InitImpl->>I2C: probe ELECROW_V1_ADDR
I2C-->>InitImpl: ACK/NACK result
InitImpl->>TCA9534: attach, configure pins/levels
InitImpl->>LightInstance: init(brightness)
LightInstance->>I2C: detect device type via ACK
LightInstance-->>InitImpl: isV2/isV3 status
InitImpl->>InitImpl: delegate to LGFX_Device::init_impl
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: 2
🤖 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_ELECROW70.h`:
- Around line 222-236: The sleep/wakeup handling in LGFX_ELECROW70 currently
only toggles panel sleep and the IO expander for non-V2/V3 boards, so V2/V3
backlight remains on. Update the sleep() and wakeup() methods to explicitly turn
the backlight off by sending brightness 0 during sleep and restore the
previously saved brightness during wakeup, while keeping the existing
_panel->setSleep(true/false) and TCA9534 output logic intact. Use the existing
config checks on _light_instance and the current backlight control path in
LGFX_ELECROW70 to place the fix.
- Around line 28-36: Update ELECROW70::init(uint8_t brightness) so it detects
the V3 device and sets _cfg.isV3 when the V3 address/handshake is found, keeping
the existing V2 detection intact. Also stop hardcoding the startup command: use
the brightness argument when sending the initial backlight command so init()
matches getBrightness() and the later setBrightness()/light()/sleep()/wakeup()
paths stay consistent.
🪄 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: 4b01aafd-0848-4872-9ccd-c4a62daa0547
📒 Files selected for processing (1)
include/graphics/LGFX/LGFX_ELECROW70.h
| bool init(uint8_t brightness) override | ||
| { | ||
| Wire.beginTransmission(ELECROW_V2_ADDR); | ||
| if (Wire.endTransmission() == 0) { | ||
| sendI2CCommand(0x10); | ||
| _cfg.isV2 = true; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP 'isV3' --type=cpp --type-add 'header:*.h' --type=header
rg -nP '\.init\s*\(' -C2 include/graphics/LGFX/LGFX_ELECROW70.hRepository: meshtastic/device-ui
Length of output: 158
🌐 Web query:
LovyanGFX ILight init setBrightness interface contract
💡 Result:
In the LovyanGFX library, ILight is an abstract base class (interface) defined in the lgfx::v1 namespace, responsible for abstracting backlight control [1][2]. The ILight interface contract consists of two primary pure virtual functions [1][2]: 1. bool init(uint8_t brightness) = 0; This method is responsible for initializing the backlight hardware, such as configuring PWM pins or I2C communication for a PMIC [2][3][4]. The brightness parameter typically defines the initial light intensity level (range 0–255) [1][5][2]. 2. void setBrightness(uint8_t brightness) = 0; This method updates the backlight intensity to the specified value in the range of 0–255 [1][5][2]. Concrete implementations, such as Light_PWM or those for I2C-controlled backlights, override these methods to provide platform-specific hardware control [1][3][4]. Within the broader architecture, a Panel_Device instance composes an ILight object [6][7]. When a user calls the high-level lcd.setBrightness(b) method on their LGFX device, the library internally delegates the call to the registered ILight implementation's setBrightness method [4][7]. The init process for the light component is triggered automatically when the main LGFX_Device::init function is called during the application startup [4][7].
Citations:
- 1: https://cardputer.free.nf/structlgfx_1_1v1_1_1_i_light.html
- 2: https://cardputer.free.nf/_light_8hpp_source.html
- 3: https://cardputer.free.nf/_light___p_w_m_8hpp_source.html
- 4: https://deepwiki.com/lovyan03/LovyanGFX/2.2-custom-configuration
- 5: https://github.com/lovyan03/lovyangfx
- 6: https://deepwiki.com/lovyan03/LovyanGFX/3-core-architecture
- 7: https://deepwiki.com/lovyan03/LovyanGFX/3.4-panel-device-abstraction
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline include/graphics/LGFX/LGFX_ELECROW70.h --view expanded || true
printf '\n== relevant file slice ==\n'
sed -n '1,220p' include/graphics/LGFX/LGFX_ELECROW70.h | cat -n
printf '\n== search for ELECROW variants and V3 ==\n'
rg -n "ELECROW|isV3|setBrightness\\(|init\\(uint8_t brightness\\)" include/graphics/LGFX -SRepository: meshtastic/device-ui
Length of output: 10896
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,250p' include/graphics/LGFX/LGFX_ELECROW70.h | cat -nRepository: meshtastic/device-ui
Length of output: 752
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '220,240p' include/graphics/LGFX/LGFX_ELECROW70.h | cat -nRepository: meshtastic/device-ui
Length of output: 752
Detect V3 and apply the requested brightness in init(). _cfg.isV3 is never set anywhere, so the V3 path in setBrightness()/light()/sleep()/wakeup() is dead and V3 backlight control never activates. init(uint8_t brightness) also ignores the caller’s value and always sends 0x10, so startup brightness and getBrightness() are out of sync.
🤖 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_ELECROW70.h` around lines 28 - 36, Update
ELECROW70::init(uint8_t brightness) so it detects the V3 device and sets
_cfg.isV3 when the V3 address/handshake is found, keeping the existing V2
detection intact. Also stop hardcoding the startup command: use the brightness
argument when sending the initial backlight command so init() matches
getBrightness() and the later setBrightness()/light()/sleep()/wakeup() paths
stay consistent.
| void sleep(void) | ||
| { | ||
| ioex.output(1, TCA9534::Level::L); | ||
| if (!_light_instance.config().isV2 && !_light_instance.config().isV3) { | ||
| ioex.output(1, TCA9534::Level::L); | ||
| } | ||
| _panel->setSleep(true); | ||
| } | ||
|
|
||
| void wakeup(void) | ||
| { | ||
| ioex.output(1, TCA9534::Level::H); | ||
| _panel->setSleep(false); | ||
| if (!_light_instance.config().isV2 && !_light_instance.config().isV3) { | ||
| ioex.output(1, TCA9534::Level::H); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers.
sed -n '180,280p' include/graphics/LGFX/LGFX_ELECROW70.h
# Find references to the light instance and sleep/wakeup behavior.
rg -n "_light_instance|setSleep\\(|isV2|isV3|brightness|0x05|backlight" include/graphics/LGFX -S
# List nearby files that may define panel/light behavior.
fd -a "LGFX_ELECROW70.h|LGFX_.*\\.h|*Light*" include/graphicsRepository: meshtastic/device-ui
Length of output: 17831
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the Elecrow light implementation in context.
sed -n '1,120p' include/graphics/LGFX/LGFX_ELECROW70.h
# Inspect the LGFX configuration wrapper for light/panel interactions.
sed -n '1,260p' include/graphics/LGFX/LGFXConfig.h
# Search for setSleep implementations and backlight behavior in the LGFX subtree.
rg -n "setSleep\\(|setLight\\(|ILight|setBrightness\\(|brightness\\s*=|sleep\\(" include/graphics/LGFX -SRepository: meshtastic/device-ui
Length of output: 17233
🌐 Web query:
LovyanGFX Panel_Device setSleep attached light backlight sleep behavior ILight setBrightness 0
💡 Result:
In the LovyanGFX library, the setSleep(bool) function typically commands the display controller IC itself to enter a low-power mode (e.g., sending MIPI DCS commands like CMD_SLPIN) [1][2]. It does not inherently control the physical backlight GPIO pin [3][2]. Key points regarding your query: 1. Backlight Management: LovyanGFX does not automatically manage the physical backlight GPIO during sleep [3]. Because the library does not explicitly handle the backlight state during sleep transitions, you must manually control the backlight (e.g., set the GPIO pin to LOW) when preparing for system sleep [3]. 2. ILight and Brightness: The ILight interface provides the setBrightness(uint8_t brightness) method [4]. While setting brightness to 0 is intended to turn off or dim the backlight, its effectiveness depends entirely on the underlying hardware implementation of the light controller associated with your panel [5][6]. If the backlight hardware does not support dimming via the ILight interface, calling setBrightness(0) will have no effect [5]. 3. Recommendations: To ensure the backlight is off during sleep, you should explicitly set the backlight pin to an "off" state in your code before calling the system's sleep function [3]. Do not rely on Panel_Device::setSleep or ILight::setBrightness(0) to perform this hardware-specific power-down task [3][5]. For reference, the Panel_Device class acts as a base for hardware abstraction [7], but because backlight circuits vary significantly across different boards, the library leaves the physical GPIO management to the user to avoid unintended side effects or hardware damage [3].
Citations:
- 1: https://github.com/lovyan03/LovyanGFX/blob/42998359/src/lgfx/v1/platforms/esp32p4/Panel_DSI.hpp
- 2: Problem XPT2046 and Deep Sleep. lovyan03/LovyanGFX#179
- 3: Keep backlight off during deep sleep lovyan03/LovyanGFX#138
- 4: https://github.com/lovyan03/LovyanGFX/blob/master/src/lgfx/v1/Light.hpp
- 5: https://spotpear.com/index.php/index/forum/detail/id/411.html
- 6: https://forum.elecrow.com/discussion/575/dim-backlight-on-esp32-terminal
- 7: https://deepwiki.com/lovyan03/LovyanGFX/3.4-panel-device-abstraction
V2/V3 backlight still needs an explicit off/on command
_panel->setSleep(true) only puts the panel IC to sleep; it does not power down the backlight. For V2/V3, send brightness 0 in sleep() and restore the saved brightness in wakeup(), otherwise the backlight can stay lit while the panel is asleep.
🤖 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_ELECROW70.h` around lines 222 - 236, The
sleep/wakeup handling in LGFX_ELECROW70 currently only toggles panel sleep and
the IO expander for non-V2/V3 boards, so V2/V3 backlight remains on. Update the
sleep() and wakeup() methods to explicitly turn the backlight off by sending
brightness 0 during sleep and restore the previously saved brightness during
wakeup, while keeping the existing _panel->setSleep(true/false) and TCA9534
output logic intact. Use the existing config checks on _light_instance and the
current backlight control path in LGFX_ELECROW70 to place the fix.
1 participant
see meshtastic/firmware#7126
Summary by CodeRabbit
New Features
Bug Fixes