fix(m5stack_tab5): support ST7121 LCD variant - #804
Conversation
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 453faf1. Configure here.
|
HI @RiviaRammer thank you for this PR. Great job. I have one question:
|
Thank you for reviewing. The two panel drivers share a very similar MIPI-DPI wrapper, but they do not have the same default initialization sequence. In particular, the ST7121 sequence uses different vendor commands (for example, the 0x60 command starts with 0x71, 0x21 rather than 0x71, 0x23), and the ST7121 timing also differs. I kept a separate ST7121 driver because M5Stack's Tab5 UserDemo follows the same approach: it detects touch FW version 1, creates esp_lcd_st7121, and uses the ST7121-specific default init sequence. I verified this path on an FW-version-1 Tab5 unit with a stable RGB/color-bar test. Using the current ST7123 driver unchanged for both would select the ST7123 default initialization sequence for ST7121. It could be refactored into a shared ST712x driver in a separate change, but that would be a broader component API change. |
If the difference is only in init sequence, it can be same like this:
The detection can be still same. Detection is only reading register from I2C. As I said before, I haven't got HW and it is not available now. But I would test new init commands with |
|
I'd like to help move this PR forward toward merging. I'll arrange for one of our engineers to test it and share the test results here. Thank you, @RiviaRammer, for your support, and @espzav for the review. |
|
@Forairaaaaa Thank you for tests. Please, could you give me more info, how did you test it? Thank you! |
Thank you for the clarification. For my previous test, I built the However, that test used the implementation in this PR as-is. On the ST7121 unit it called I now understand the test you are requesting: keep the existing runtime detection, use I have not tested that configuration yet. I will prepare a build using this approach, test it on the ST7121 hardware, and report the result here. |
|
@Forairaaaaa Hi, please do you have any update here? |
Hi, I tested this with a local change on top of af2e35d. Both ST7121 and ST7123 use esp_lcd_new_panel_st7123(). For ST7121, I passed its original initialization commands through st7123_vendor_config_t, while keeping the ST7121-specific DSI lane rate and DPI timing. I also removed the esp_lcd_st7121 dependency and verified that the final ELF contains esp_lcd_new_panel_st7123, but not esp_lcd_new_panel_st7121. I built examples/display with ESP-IDF v5.4.4 and flashed it to physical Tab5 boards with ST7121 and ST7123 panels. On both boards, the display initialized correctly, the animation rendered normally, and touch input worked. I did not observe any artifacts, flickering, or unexpected resets. So yes, ST7121 works with the ST7123 driver when using the ST7121-specific vendor initialization commands. |
@Forairaaaaa Thank you very much for help! @RiviaRammer Please, could you rework this PR by the latest information about driver? |
|
Thank you for the additional hardware validation. I have reworked this PR accordingly. The separate esp_lcd_st7121 component and its dependency have been removed. The ST7121-specific initialization commands are now kept in the Tab5 BSP, while both ST7121 and ST7123 use esp_lcd_new_panel_st7123() with their respective st7123_vendor_config_t settings. The existing runtime detection is retained. ST7121 also keeps its specific 965 Mbps DSI lane rate and DPI timing. I rebuilt the Tab5 bring-up demo successfully, and its ELF contains esp_lcd_new_panel_st7123 only. |
espzav
left a comment
There was a problem hiding this comment.
@RiviaRammer Thank you for changes. It looks good, I left some minor comments. After your changes, I will check it on HW yet.
Co-authored-by: Vilem Zavodny <98878239+espzav@users.noreply.github.com>




问题背景
M5Stack Tab5 存在不同 LCD 方案。当前 BSP 在 I2C 地址
0x55探测到 ST712x 触摸控制器后,会统一按 ST7123 LCD 初始化。Tab5 UserDemo 会读取 ST712x 寄存器
0x0000的固件版本区分屏幕:FW version = 1: ST7121FW version = 3: ST7123因此,ST7121 屏幕版本需要使用不同的初始化命令和 DPI 时序。
修改内容
esp_lcd_new_panel_st7123()driveresp_lcd_st7121component测试
esp_lcd_new_panel_st7123,不包含esp_lcd_new_panel_st7121git diff --check通过Note
Medium Risk
Changes auto-detection and MIPI/DPI init for all Tab5 ST712x boards; mis-read FW versions could block display init, though ST7123/ST7121 paths were validated on hardware.
Overview
Adds ST7121 as a third Tab5 hardware revision alongside ILI9881C/GT911 and ST7123, instead of treating every ST712x touch probe as ST7123.
Board detection now reads the ST712x touch firmware byte at register
0x0000after I2C probe: FW1selects ST7121, FW3keeps ST7123; unknown values fail detection instead of assuming one panel. Magic1/2integers are replaced with absp_tab5_board_version_tenum, and display/touch setup bail out withESP_ERR_NOT_SUPPORTEDwhen detection fails.ST7121 display path adds
disp_init_data_st7121.h, wires it through the existingesp_lcd_new_panel_st7123()driver with ST7121-specific init commands and DPI porch/timing, and raises MIPI DSI lane bitrate to 965 Mbps for that variant in the LVGL display start path. Touch for ST7121 shares the ST7123 I2C touch driver.Component version is bumped to 1.3.0.
Reviewed by Cursor Bugbot for commit df6ebeb. Bugbot is set up for automated code reviews on this repo. Configure here.