feat(lcd): add brightness control for LCD panels (IDFGH-17284)#18273
feat(lcd): add brightness control for LCD panels (IDFGH-17284)#18273ldab wants to merge 1 commit intoespressif:release/v5.5from
Conversation
|
|
👋 Hello ldab, we appreciate your contribution to this project! 📘 Please review the project's Contributions Guide for key guidelines on code, documentation, testing, and more. 🖊️ Please also make sure you have read and signed the Contributor License Agreement for this project. Click to see more instructions ...
Review and merge process you can expect ...
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
This is the final PR Bugbot will review for you during this billing cycle
Your free Bugbot reviews will reset on March 20
Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| * - ESP_ERR_NOT_SUPPORTED if this function is not supported by the panel | ||
| */ | ||
| esp_err_t (*set_brightness)(esp_lcd_panel_t *panel, int brightness); | ||
|
|
There was a problem hiding this comment.
ABI break from interface struct change
High Severity
Adding set_brightness to struct esp_lcd_panel_t changes the struct layout, which can break binary compatibility for any out-of-tree/prebuilt panel drivers that allocate or embed esp_lcd_panel_t with the old size/layout. This can manifest as memory corruption when user_data is accessed or when the struct is initialized.
| ESP_RETURN_ON_FALSE(panel, ESP_ERR_INVALID_ARG, TAG, "invalid panel handle"); | ||
| ESP_RETURN_ON_FALSE(panel->set_brightness, ESP_ERR_NOT_SUPPORTED, TAG, "set_brightness is not supported by this panel"); | ||
| return panel->set_brightness(panel, brightness); | ||
| } |
There was a problem hiding this comment.
Brightness API lacks input validation
Medium Severity
esp_lcd_panel_set_brightness() forwards brightness directly to panel->set_brightness() without any validation (e.g., negative values). If callers pass an out-of-range int, panel drivers may cast to unsigned or program invalid hardware values, leading to unexpected behavior.
There was a problem hiding this comment.
Pull request overview
Adds a new brightness-control entry point to the ESP-IDF LCD panel abstraction so panel drivers can optionally expose brightness adjustment through a common API.
Changes:
- Introduces
esp_lcd_panel_set_brightness()as a new public LCD panel ops API. - Extends
esp_lcd_panel_twith aset_brightnessfunction pointer. - Implements the new wrapper in
esp_lcd_panel_ops.cwith basic handle/support checks.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| components/esp_lcd/src/esp_lcd_panel_ops.c | Adds the public wrapper that dispatches brightness control to the panel implementation. |
| components/esp_lcd/interface/esp_lcd_panel_interface.h | Extends the panel vtable with a set_brightness hook and documents it. |
| components/esp_lcd/include/esp_lcd_panel_ops.h | Exposes the new brightness API in the public header and documents it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * @brief Set the brightness of the display | ||
| * | ||
| * @param[in] panel LCD panel handle, which is created by other factory API like `esp_lcd_new_panel_st7789()` | ||
| * @param[in] brightness Brightness value (range depends on panel implementation, typically 0-255 or 0-100) | ||
| * @return | ||
| * - ESP_OK on success | ||
| * - ESP_ERR_NOT_SUPPORTED if this function is not supported by the panel | ||
| */ | ||
| esp_err_t (*set_brightness)(esp_lcd_panel_t *panel, int brightness); | ||
|
|
||
| void *user_data; /*!< User data, used to store externally customized data */ |
There was a problem hiding this comment.
esp_lcd_panel_t is a public struct; inserting set_brightness before user_data changes the offset of user_data. This can break binary compatibility (and even source compatibility for any code using positional aggregate initialization). Consider appending the new function pointer after user_data, or adding reserved fields for future expansion to keep existing offsets stable.
| /** | ||
| * @brief Set the brightness of the display | ||
| * | ||
| * @param[in] panel LCD panel handle, which is created by other factory API like `esp_lcd_new_panel_st7789()` | ||
| * @param[in] brightness Brightness value (range depends on panel implementation, typically 0-255, 0-1023, or 0-100) | ||
| * @return | ||
| * - ESP_OK on success | ||
| * - ESP_ERR_NOT_SUPPORTED if this function is not supported by the panel | ||
| */ | ||
| esp_err_t esp_lcd_panel_set_brightness(esp_lcd_panel_handle_t panel, int brightness); |
There was a problem hiding this comment.
The documentation says the brightness range depends on the panel implementation (and the range examples differ between this header and esp_lcd_panel_interface.h). For a “standardized” API, this ambiguity makes it hard for applications to use consistently. Consider defining a single unit/range for brightness (e.g., 0–100 percent) or adding an accompanying capability/range query so callers can scale appropriately.
| * @brief Set the brightness of the display | ||
| * | ||
| * @param[in] panel LCD panel handle, which is created by other factory API like `esp_lcd_new_panel_st7789()` | ||
| * @param[in] brightness Brightness value (range depends on panel implementation, typically 0-255 or 0-100) | ||
| * @return | ||
| * - ESP_OK on success | ||
| * - ESP_ERR_NOT_SUPPORTED if this function is not supported by the panel | ||
| */ | ||
| esp_err_t (*set_brightness)(esp_lcd_panel_t *panel, int brightness); |
There was a problem hiding this comment.
The brightness range examples are inconsistent between the interface (typically 0-255 or 0-100) and the public ops header (0-255, 0-1023, or 0-100). Please align these docs so panel authors and API users get the same guidance.
github-actions
bot
changed the title
|
sha=7d58f1b0099d57199090d33e1a9e0b08e0f55e38 |
espressif-bot
added
Status: In Progress
suda-morris
added
PR-Sync-Merge
Description
This pull request introduces a new API for setting the brightness of LCD panels in the ESP-IDF LCD driver. The changes add both a high-level function and an interface method to support brightness control, allowing panels that implement this feature to expose it in a standardized way.
New brightness control API:
esp_lcd_panel_set_brightnessfunction to the public API inesp_lcd_panel_ops.h, allowing users to set the display brightness for panels that support this feature.esp_lcd_panel_set_brightnessfunction inesp_lcd_panel_ops.c, including input validation and error handling if the panel does not support brightness control.Panel interface extension:
set_brightnessfunction pointer to theesp_lcd_panel_tstruct inesp_lcd_panel_interface.h, enabling panel drivers to implement their own brightness control logic.Related
Testing
Checklist
Before submitting a Pull Request, please ensure the following:
Note
Medium Risk
Adds a new public API and extends the core
esp_lcd_panel_tinterface struct, which can affect ABI/compatibility and any out-of-tree panel implementations that initialize the struct.Overview
Adds a new optional brightness-control API for LCD panels via
esp_lcd_panel_set_brightness().Extends
esp_lcd_panel_twith aset_brightnessfunction pointer and wires the public wrapper to validate the handle, returnESP_ERR_NOT_SUPPORTEDwhen unimplemented, and delegate to the panel driver implementation.Written by Cursor Bugbot for commit 7d58f1b. This will update automatically on new commits. Configure here.