Skip to content

feat(esp_lcd_sh8601): add brightness control to the panel (AEGHB-1429) #668

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ldab wants to merge 1 commit into
base: master
Choose a base branch
from
+13 −0
Open

feat(esp_lcd_sh8601): add brightness control to the panel (AEGHB-1429) #668

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions components/display/lcd/esp_lcd_sh8601/esp_lcd_sh8601.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ static esp_err_t panel_sh8601_mirror(esp_lcd_panel_t *panel, bool mirror_x, bool
static esp_err_t panel_sh8601_swap_xy(esp_lcd_panel_t *panel, bool swap_axes);
static esp_err_t panel_sh8601_set_gap(esp_lcd_panel_t *panel, int x_gap, int y_gap);
static esp_err_t panel_sh8601_disp_on_off(esp_lcd_panel_t *panel, bool off);
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The forward declaration panel_sh8601_disp_on_off(…, bool off) uses a parameter name (off) that doesn't match the implementation (bool on_off). This is minor but can be confusing when reading stack traces or debugging; consider renaming the parameter in the prototype to match the definition.

Suggested change
static esp_err_t panel_sh8601_disp_on_off(esp_lcd_panel_t *panel, bool off);
static esp_err_t panel_sh8601_disp_on_off(esp_lcd_panel_t *panel, bool on_off);

Copilot uses AI. Check for mistakes.
static esp_err_t panel_sh8601_set_brightness(esp_lcd_panel_t *panel, int brightness);

typedef struct {
esp_lcd_panel_t base;
Expand Down Expand Up @@ -121,6 +122,7 @@ esp_err_t esp_lcd_new_panel_sh8601(const esp_lcd_panel_io_handle_t io, const esp
sh8601->base.mirror = panel_sh8601_mirror;
sh8601->base.swap_xy = panel_sh8601_swap_xy;
sh8601->base.disp_on_off = panel_sh8601_disp_on_off;
sh8601->base.set_brightness = panel_sh8601_set_brightness;
*ret_panel = &(sh8601->base);
Comment on lines 123 to 126
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Assigning sh8601->base.set_brightness assumes the set_brightness function pointer exists in esp_lcd_panel_t. If this component is meant to support IDF versions where that field is absent, this assignment will break the build. Please guard this assignment with an appropriate IDF version/feature check or avoid touching the base struct when building against older IDF versions.

Copilot uses AI. Check for mistakes.
ESP_LOGD(TAG, "new sh8601 panel @%p", sh8601);

Expand Down Expand Up @@ -345,3 +347,14 @@ static esp_err_t panel_sh8601_disp_on_off(esp_lcd_panel_t *panel, bool on_off)
ESP_RETURN_ON_ERROR(tx_param(sh8601, io, command, NULL, 0), TAG, "send command failed");
return ESP_OK;
}

static esp_err_t panel_sh8601_set_brightness(esp_lcd_panel_t *panel, int brightness)
{
sh8601_panel_t *sh8601 = __containerof(panel, sh8601_panel_t, base);
esp_lcd_panel_io_handle_t io = sh8601->io;

// Clamp brightness to 0-1023 range (10-bit)
uint16_t brightness_val = (brightness < 0) ? 0 : (brightness > 1023) ? 1023 : (uint16_t)brightness;
// Send as 2 bytes: MSB and LSB
return tx_param(sh8601, io, LCD_CMD_WRDISBV, (uint8_t[]) { (brightness_val >> 8) & 0xFF, brightness_val & 0xFF }, 2);
}
Comment on lines +351 to +360
Copy link

Copilot AI Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Brightness support is introduced here, but there are existing Unity hardware tests for SH8601 under test_apps/ and none exercise the new brightness path. Please add at least a basic call (and error check) in the test app(s) so the new API is built and executed in CI/hardware test runs.

Copilot uses AI. Check for mistakes.