Skip to content

Conversation

@mdwn
Copy link
Contributor

@mdwn mdwn commented Jan 20, 2026

The esp32p4 supports LCDs through DSI. This is the initial LCD support for esp32p4.

Thank you for your contribution!

We appreciate the time and effort you've put into this pull request.
To help us review it efficiently, please ensure you've gone through the following checklist:

Submission Checklist 📝

  • I have updated existing examples or added new ones (if applicable).
  • I have used cargo fmt command to ensure that all changed code is formatted correctly.
  • I have used cargo clippy command to ensure that all changed code passes latest Clippy nightly lints.
  • My changes were added to the CHANGELOG.md in the proper section.

Pull Request Details 📖

Description

This introduces the initial LCD support into esp-idf-hal.

I've struggled a bit with figuring out the best way to do this. The biggest issue is that there's a vendor dependent step in the middle of initialization, as can be seen here: https://docs.espressif.com/projects/esp-idf/en/stable/esp32p4/api-reference/peripherals/lcd/dsi_lcd.html

I thought of having the lcd driver take in a callback that is responsible for doing vendor specific initialization, but before doing anything so severe I want to throw this into the ring for discussion.

Another complicating factor is that I'm using a Waveshare LCD. The library that supports it does part of its own initialization, needing us to do a set_dpi_panel as opposed to leaning on the LCD library's built in create_dpi_panel. I'm unsure how frequent this use case is, so I don't know whether allowing create or set is the right way to go here.

I'd love to hear your thoughts on this.

Note: I can't test the example I've added, as I've got a waveshare LCD that has its own wonky initialization process. However, it largely models the LCD reference above.

Testing

I created a small test program and got a Waveshare LCD working:

image

The esp32p4 supports LCDs through DSI. This is the initial LCD support
for esp32p4.
@SakiiCode
Copy link

SakiiCode commented Jan 20, 2026

Nice work! I can help testing and reviewing this with 2 other DSI displays if needed later this week

Some ideas from a quick glance

  • Refactor LcdDriver to use some kind of typestate pattern instead of passing an Option everywhere
  • Removing the null checks wherever possible and take a NonNull<T> instead
  • Similarly unsigned ints instead of > 0 checks
  • Replace primitive values in LcdConfig with esp-idf structs like in LcdDriver because
    • If they are changed on the C side this code breaks
    • DSI bus values are different for each display, those defaults are not ideal
  • The vendor specific functions could be a trait and then LcdDriver would be generic
  • Add type aliases like type DsiBusHandle = esp_lcd_dsi_bus_handle_t;
  • It would be great if PixelFormat was somehow interoperable with embedded_graphics::pixelcolor::PixelColor

pub ldo3: ldo::LDO3<'static, ldo::Adjustable>,
#[cfg(esp32p4)]
pub ldo4: ldo::LDO4<'static, ldo::Adjustable>,
#[cfg(esp32p4)]
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is correct to name this peripheral LCD. I would rather suggest, we name it DSI, because that's what it is. And then the name of the peripheral variable should be dsi as well.

We might be confusing the LCD C driver (esp_lcd) with just one of the peripherals/interfaces it supports - DSI.

Other peripherals/interfaces supported by this driver are SPI, and I80, so the LCD driver is in fact a larger thing than the concrete DSI peripheral.

The situation is a bit similar to the EspEth driver, which supports SPI as well as RMII.

- esp32p4 pins and core command added.
- Removed the modem peripheral for esp32p4
- LDO support for esp32p4
- LCD support for esp32p4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps:

  • LCD driver support
  • DSI peripheral support with the LCD driver for esp32p4

// Create the DPI panel for pixel data transfers
lcd.create_dpi_panel()?;

// Simple test framebuffer: clear screen to black.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think it is fair to call the bitmap you use here a "framebuffer".

A "framebuffer" is something else: in drivers that do support/need framebuffers, this is a region of memory, where a simple write of one or more bytes gets immediately (or almost immediately) reflected on the actual screen.

In fact, one of my biggest concerns below is regarding the usage of a hidden (large) framebuffer by the LCD driver itself.

@@ -0,0 +1,1128 @@
//! LCD peripheral control
Copy link
Collaborator

Choose a reason for hiding this comment

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

I've never really liked the esp_lcd C driver because it is somewhat difficult to connect it to the larger, embedded-graphics based Rust echosystem.

The reason for this is that the esp_lcd C driver (I think) wants to do its own framebuffer memory management (not bitmaps!), while in Rust-land (at least with SPI, I2C and I80 parallel mode) there is no notion of a "driver-managed" frame buffer. And that's good because those framebuffers tend to take a lot of memory, for higher-resolution higher-depth displays.

For example if you look at this pure-Rust driver, the way it operates from the point of view of Rust and the MCU is somewhat of an immediate mode: you push some pixels to the display, and yes - once they are displayed, they are also remembered by the display itself (these displays have their own internal memory) - but the "Rust side" can "forget" about those pixels immediately after it had pushed them down the SPI wire.

I'm not sure if this is possible to implement this with a pure "DSI" interface though (without an SPI or I2C layer "bolted" on top). My primitive understanding of DSI-only displays is that these do not have internal memory and rely on the MCU maintaining its own frame-buffer in precious MCU memory and then refreshing the display by pushing data all the time. Or am I wrong?

I think:

  • If the DSI display you use has its own internal memory, and we can provide an API that looks like "blit this bitmap at that position in the LCD internal framebuffer + screen" we should do that. Because this way we have something which is very similar to how SPI and I2C displays operate
  • If DSI displays do not have internal framebuffers then I guess we have to live with what we have...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants