feat(lcd): ST7796 Add Sleep command support (BSP-780)#719
feat(lcd): ST7796 Add Sleep command support (BSP-780)#719AndreaGreco wants to merge 1 commit intoespressif:masterfrom
Conversation
|
|
There was a problem hiding this comment.
Pull request overview
This pull request adds sleep command support (SLPIN/SLPOUT) to the ST7796 LCD driver's general (non-MIPI) variant. The feature allows entering and exiting sleep mode, which is useful for power management in LCD applications.
Changes:
- Added
panel_st7796_disp_sleep()function declaration and implementation - Assigned the sleep function pointer in the panel initialization code
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
github-actions
bot
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
AndreaGreco
force-pushed
the
master
branch
2 times, most recently
from
46d045b to
ccf24d4
Compare
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.
|
|
||
| static esp_err_t panel_st7796_disp_sleep(esp_lcd_panel_t *panel, bool sleep) | ||
| { | ||
| st7796_panel_t *st7796 = __containerof(panel, st7796_panel_t, base); |
There was a problem hiding this comment.
Wrong struct access pattern causes undefined behavior
High Severity
The panel_st7796_disp_sleep function in the MIPI file uses __containerof(panel, st7796_panel_t, base) to access the panel structure, but the MIPI version's st7796_panel_t struct doesn't have a base member. All other functions in this file correctly use (st7796_panel_t *)panel->user_data instead. Using __containerof with a non-existent member will result in undefined behavior and likely memory corruption or crash when the function is called.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| static esp_err_t panel_st7796_disp_sleep(esp_lcd_panel_t *panel, bool sleep) | ||
| { | ||
| st7796_panel_t *st7796 = __containerof(panel, st7796_panel_t, base); |
There was a problem hiding this comment.
Incorrect use of __containerof. The st7796_panel_t struct in this file does not have a 'base' member. The correct way to retrieve the st7796 pointer is: (st7796_panel_t *)panel->user_data, as used in all other functions in this file (see lines 315, 270, 287, 250). The __containerof pattern is only correct in the general variant where st7796_panel_t has 'esp_lcd_panel_t base' as its first member.
| st7796_panel_t *st7796 = __containerof(panel, st7796_panel_t, base); | |
| st7796_panel_t *st7796 = (st7796_panel_t *)panel->user_data; |
There was a problem hiding this comment.
@AndreaGreco Please, could you apply this suggestion?
espzav
left a comment
espzav
left a comment
There was a problem hiding this comment.
@AndreaGreco Thank you for this PR. LGTM - I left one comment
|
|
||
| static esp_err_t panel_st7796_disp_sleep(esp_lcd_panel_t *panel, bool sleep) | ||
| { | ||
| st7796_panel_t *st7796 = __containerof(panel, st7796_panel_t, base); |
There was a problem hiding this comment.
@AndreaGreco Please, could you apply this suggestion?
ST7796 Add Sleep command support
Add Sleep command support to ST7796
Note
Low Risk
Low risk: adds a new
disp_sleeppanel op that sends standard ST7796 sleep in/out commands with a datasheet-required delay; existing init/draw logic is unchanged.Overview
Adds ST7796 sleep mode support by wiring up the
disp_sleeppanel operation for both the general and MIPI variants.Implements
panel_st7796_disp_sleep()to sendLCD_CMD_SLPIN/LCD_CMD_SLPOUTand enforce a 120ms post-command delay per the datasheet.Written by Cursor Bugbot for commit 34fc586. This will update automatically on new commits. Configure here.