Skip to content
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.

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

Introduce RGB/DPI driver #2415

Merged
merged 8 commits into from
Nov 19, 2024
Merged

Introduce RGB/DPI driver #2415

merged 8 commits into from
Nov 19, 2024

Conversation

Dominaezzz
Copy link
Collaborator

@Dominaezzz Dominaezzz commented Oct 27, 2024

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 xtask fmt-packages command to ensure that all changed code is formatted correctly.
  • My changes were added to the CHANGELOG.md in the proper section.
  • I have added necessary changes to user code to the Migration Guide.
  • My changes are in accordance to the esp-rs API guidelines

Extra:

Pull Request Details 📖

Description

Add a driver for the RGB mode of the LCD_CAM peripheral. Closes #2081.
I've called it Dpi since Rgb is a generic name (though I don't mind renaming it if anyone insists). The I8080 driver has similar opinionated name.

I've opened this early as a formal request for one of the maintainers to acquire a devkit that can run the example.
Makerfabs MaTouch_ESP32-S3 Parallel TFT with Touch 4.3"

(I tried using the espressif devkit linked in the issue but the initialization code is buried deep in esp-bsp and it would complicate the example with bitbanging SPI over an I2C gpio expander)

The example runs on the official espressif devkit ESP32-S3-LCD-EV-Board.

Testing

HIL test and example

Copy link
Contributor

@Georges760 Georges760 left a comment

Choose a reason for hiding this comment

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

Great to see this PR, I was considering working on this need.

hil-test/Cargo.toml Show resolved Hide resolved
@jgtaylor
Copy link

I've tested this on a 7" with an EK9716 display controller, and it appears to be working. Board used: https://www.aliexpress.com/item/1005005560920555.html

I've not done any kind of exhaustive tests - just run the lcd_dpi.rs from the example directories (with pins modified for the above board).

The example code results in the screen going through various colors. On my device, it appears to flicker fairly intensely. Changing the clock to 40 & 50 Mhz seems to help with the flicker a bit. Using 33 Mhz results in interesting artifacts on 2/3 of the display.

In any case, this MR has my vote! Getting this into main would be a tremendous step forward!

@Dominaezzz
Copy link
Collaborator Author

The example code results in the screen going through various colors. On my device, it appears to flicker fairly intensely. Changing the clock to 40 & 50 Mhz seems to help with the flicker a bit. Using 33 Mhz results in interesting artifacts on 2/3 of the display.

Curious, in addition to the pins, did you also update the frame timings?
Also maybe try making the loop buffer bugger.
I'd also appreciate a video recording of the flickering if you can share one.

@jgtaylor
Copy link

please forgive the horrible video and my silly commentary (also, apologies for using YouTube) - https://www.youtube.com/shorts/tHPwoWvb11o

Also, I had to turn on the backlight to actually see the screen. The flicker could be from the backlight, or not using ledc to dim it... or, or or ... I confess, I'm so new to rust, I'm still fighting with the borrow checker, but I am excited as can be to get this working!

@jgtaylor
Copy link

quick update - i've played with the timings, however they're pretty spot on for the EK9716. I increased the dma_loop_buf to 16 * 16 which helped. It removed the artifacts at 33.MHz(). I think my flicker is more a result of the back-light.

somewhat random question: do the "back porch" timings need to be defined? Pic from datasheet for posterity, or in-case it helps anything (pls ignore if its useless)
image

@Dominaezzz
Copy link
Collaborator Author

please forgive the horrible video

I suppose it can't be helped for this example since every single frame is a different color. Trying to see it on video was a lost cause to begin with haha. Would need a different example, one that alternates between two colors once a second.

somewhat random question: do the "back porch" timings need to be defined? Pic from datasheet for posterity, or in-case it helps anything (pls ignore if its useless)

This is where I need to write some documentation.
The short answer is no, the hardware can calculate it based on the other params.

I increased the dma_loop_buf to 16 * 16 which helped. It removed the artifacts at 33.MHz()

I'm glad this helped!

Just to be sure about the flickering, try changing the example to only draw on color and see if it still flickers.
You can do that by changing.

dpi.send(false, dma_buf).map_err(|e| e.0).unwrap();

to this

dpi.send(true, dma_buf).map_err(|e| e.0).unwrap();

@jgtaylor
Copy link

jgtaylor commented Nov 2, 2024

I can add to the review that I've tried this on the 4.3" cheap yellow board, at 800x480, and it looks wonderful. The 4.3" uses a ST7262.

The "flicker" i'd reported is definitely not flicker, but the backlight being a bit over-powered.

@MabezDev MabezDev mentioned this pull request Nov 13, 2024
@Dominaezzz Dominaezzz force-pushed the dpi_driver branch 2 times, most recently from 590b5fa to e4a9fa8 Compare November 16, 2024 20:15
@Dominaezzz
Copy link
Collaborator Author

@MabezDev as requested, the example now runs on the official devkit.
I'd be very happy if this could make it into the next release.

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thanks for porting the example to the official devkit ❤️, unfortunately the devkit we have in the office is an older rev and the example isn't working.

I think we can still merge this as is for now, given that you have it tested, and we'll get a new rev of the board for testing later down the line.

esp-hal/src/lcd_cam/lcd/dpi.rs Show resolved Hide resolved
esp-hal/src/lcd_cam/lcd/dpi.rs Show resolved Hide resolved
esp-hal/src/lcd_cam/lcd/dpi.rs Show resolved Hide resolved
Copy link
Member

@jessebraham jessebraham left a comment

Choose a reason for hiding this comment

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

Agree with the comment regarding #[non_exhaustive] on the Config struct, but otherwise changes LGTM I think

@Dominaezzz
Copy link
Collaborator Author

fwiw
https://github.com/user-attachments/assets/f142efc1-4d2a-4209-a85f-45edb63c9d61

Copy link
Member

@MabezDev MabezDev left a comment

Choose a reason for hiding this comment

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

Thank you!

@MabezDev MabezDev added this pull request to the merge queue Nov 19, 2024
Merged via the queue into esp-rs:main with commit 7821968 Nov 19, 2024
28 checks passed
@Dominaezzz Dominaezzz deleted the dpi_driver branch November 19, 2024 14:08
@yanshay
Copy link
Contributor

yanshay commented Dec 7, 2024

@Dominaezzz I can't find the example any longer in the latest examples folder, did it move?

I can add to the review that I've tried this on the 4.3" cheap yellow board, at 800x480, and it looks wonderful. The 4.3" uses a ST7262.

The "flicker" i'd reported is definitely not flicker, but the backlight being a bit over-powered.

@jgtaylor I also want to use ST7262, does the example work as is or did it require any changes?

@Dominaezzz
Copy link
Collaborator Author

It was moved. https://github.com/esp-rs/esp-hal/blob/main/qa-test/src/bin/lcd_dpi.rs

Also, note that I changed the example after jgtaylor's comment. So you'll need to update the FrameTimings

@yanshay
Copy link
Contributor

yanshay commented Dec 23, 2024

I received the board I want to use, it is 5", based on ST7262, 800x480.
I found the RGB/vsync/hsync/de/pclk pins and updated in the example (I'm using the one in esp-hal 0.22)

I don't understand what the expander is for and what pins should I fill in?
What values should I fill for total_width/blank_front_porch/hsync_width/vsync_width/hsync_position ?
Anything else I should change in the example?

These are the pins descriptions they provide:

<style> </style>
Serial Number Pin Notes Connect
1 IO8 U1RXD DB1(B)
2 IO3   DB2(B)
3 IO46   DB3(B)
4 IO9   DB4(B)
5 IO1   DB5(B)
6 IO39   HSYNC
7 IO41   VSYNC
8 IO40   DE
9 IO5   DB6(G)
10 IO7   DB8(G)
11 IO15   DB9(G)
12 IO16   DB10(G)
13 IO4   DB11(G)
14 IO45   DB13(R)
15 IO48   DB14(R)
16 IO47   DB15(R)
17 IO21   DB16(R)
18 IO43 U0TXD TX
19 IO44 U0RXD RX
20 IO0 BOOT I2S-BCLK/L3
21 IO18   I2S-LRCLK/L2
22 IO10 SPICS0 TF(CS)
23 IO11 SPID TF(MISO)
24 IO17 U1TXD IS2-DIN/L1
25 IO38   TP_CS
26 IO21   BL_C
27 IO37    
28 IO36    
29 IO35    
30 IO14   DB17(R)
31 IO20 USB_D+ TP_SCL
32 IO12 SPICLK TF(SCK)/TP_CLK
33 IO13 SPIQ TF(MOSI)
34 IO42   PCLK
35 IO6   DB7(G)
36 IO19 USB_D- TP_SDA

This is from an Arduino example included, some values are available here I see:

Arduino_ESP32RGBPanel *bus = new Arduino_ESP32RGBPanel(
    GFX_NOT_DEFINED /* CS */, GFX_NOT_DEFINED /* SCK */, GFX_NOT_DEFINED /* SDA */,
    40 /* DE */, 41 /* VSYNC */, 39 /* HSYNC */, 42 /* PCLK */,
    45 /* R0 */, 48 /* R1 */, 47 /* R2 */, 21 /* R3 */, 14 /* R4 */,
    5 /* G0 */, 6 /* G1 */, 7 /* G2 */, 15 /* G3 */, 16 /* G4 */, 4 /* G5 */,
    8 /* B0 */, 3 /* B1 */, 46 /* B2 */, 9 /* B3 */, 1 /* B4 */
);
// option 1:
// ST7262 IPS LCD 800x480
 Arduino_RPi_DPI_RGBPanel *gfx = new Arduino_RPi_DPI_RGBPanel(
   bus,
   800 /* width */, 0 /* hsync_polarity */, 8 /* hsync_front_porch */, 4 /* hsync_pulse_width */, 8 /* hsync_back_porch */,
   480 /* height */, 0 /* vsync_polarity */, 8 /* vsync_front_porch */, 4 /* vsync_pulse_width */, 8 /* vsync_back_porch */,
   1 /* pclk_active_neg */, 16000000 /* prefer_speed */, true /* auto_flush */);

@Dominaezzz
Copy link
Collaborator Author

I don't understand what the expander is for and what pins should I fill in?

Some displays require initialisation via SPI, before the DPI interfaced can be used.
Seems like yours doesn't have this (judging from the pin descriptions), so you can probably skip that step.

With regards to what pins to use, the pin names mostly match what the Dpi driver asks for. vsync hsync, etc.
Your data pins look weirdly labelled, as it goes from 1-11,13-17 rather than 0-16.
Judging from the R,G,B labels you just need to specify then in order.
I hope that makes sense 🙂

@yanshay
Copy link
Contributor

yanshay commented Dec 23, 2024

So all those INIT_ CMDS are not relevant? (since I'm commenting out most of the code, want to verify)

@Dominaezzz
Copy link
Collaborator Author

Dominaezzz commented Dec 23, 2024

What values should I fill for total_width/blank_front_porch/hsync_width/vsync_width/hsync_position ?

This part is unfortunately a bit confusing because the names don't quite mean what you think.
It's best to read the doc comment on each field to understand what each one means, then use the display's datasheet to help you specify them.

I know it's probably not the quick answer you're looking for but I recommend taking a look a this resource that explains the protocol.

The LCD_CAM is able to infer the some of the timings so you won't find every single LCD timing parameter in the FrameTiming struct.

hsync_position is almost always 0.

So all those INIT_ CMDS are not relevant? (since I'm commenting out most of the code, want to verify)

Probably not. If your display doesn't have SPI then it's not relevant.
If it was relevant, you'd probably need a different set of init cmds anyway.

@yanshay
Copy link
Contributor

yanshay commented Dec 23, 2024

Thanks!

It's working. I used some of the values from the Arduino code for the the timing, I see on the right most vertical line some glitches but I guess by playing a bit with the numbers it will be resolved.

Now need to move on to make it produce some graphics. Expect questions on DMA soon 😄

If I understand correctly your demo then its a 16u16 dma buffer and when the code does dpi.send, it returns after a full screen frame is full with as many 16u16 as required to fill the complete frame?

@Dominaezzz
Copy link
Collaborator Author

I see on the right most vertical line some glitches

Try increasing the DMA buffer size.

If I understand correctly your demo then its a 16u16 dma buffer and when the code does dpi.send, it returns after a full screen frame is full with as many 16u16 as required to fill the complete frame?

Yup!

Now need to move on to make it produce some graphics. Expect questions on DMA soon 😄

I'm excited to see where this goes. Just be aware that esp-hal is currently lacking #2083, which limits your options.

@yanshay
Copy link
Contributor

yanshay commented Dec 23, 2024

I'm excited to see where this goes. Just be aware that esp-hal is currently lacking #2083, which limits your options.

Oh ...
Does that mean I can't use PSRAM for display at all or can't use it for the display while at the same time use the PSRAM for other things?
My application relies heavily on PSRAM - let's say that issue is resolved, is it practical to use PSRAM for both display AND other applicative uses? How much slower will the PSRAM be?
I also access the flash for storing config data (let's say I'll cache them for immediate use), won't that saving work?

When is that planned to be resolved?

@Dominaezzz
Copy link
Collaborator Author

Does that mean I can't use PSRAM for display at all or can't use it for the display while at the same time use the PSRAM for other things?

The latter mostly but of course, it depends on the frequency of all the moving parts.
Just try and see what happens. Was just letting you know a possible issue you might run into.

My application relies heavily on PSRAM - let's say that issue is resolved, is it practical to use PSRAM for both display AND other applicative uses? How much slower will the PSRAM be?

It is practical, yes. Just means the FPS would be on the lower side of the spectrum.

I also access the flash for storing config data (let's say I'll cache them for immediate use), won't that saving work?

This is probably fine.

Any issues arising from PSRAM bandwidth, would manifest in the image being rendered, the rest of your application would run just fine.

When is that planned to be resolved?

The last time I brought it up in a community meeting, it wasn't accepted for prioritisation. (Perhaps lack of demand at the time didn't help)
You probably won't see unless it's contributed.

Are you planning on using Slint on this as well? Or embedded-graphics? Or?

@yanshay
Copy link
Contributor

yanshay commented Dec 23, 2024

Are you planning on using Slint on this as well? Or embedded-graphics? Or?

Yes, for Slint.

How can that not be prioritized? There are so many esp32 boards with RGB displays (3.5" is the largest I found without) that can't be used w/o this (or at risk of failing at some point). IMO this is so much more important than so many other changes made, that have their value, but they don't increase the reach of esp-hal to additional applications and use cases. And it doesn't seem like something the community would be able to take care of, this looks like hard-core esp32 internals.

You can bring my vote in favor of this.

I guess the reason for this not being voted is because people who want to do graphics on esp32, check out what's available, and eventually dump rust and esp-hal in favor of C (lvgl, etc.) because it looks so challenging to get something working (and it really is).
But once Slint is working on a board it becomes so easy to program UI, easier and faster even than HTML with the toolset they provide (subject to some specific Slint graphical limitations)

This could make rust the easiest language to develop ESP32 applications with UI if you ask me.

@Dominaezzz
Copy link
Collaborator Author

With Slint you may be able to implement bounce buffers (see esp-idf docs for explanation) and avoid PSRAM altogether.

Yeah there's also other benefits to that feature but oh well. Another problem is none of the Rust devs know how it works, they'd have to do some additional research.

Put your +1/vote on the issue at least. 🙂

@yanshay
Copy link
Contributor

yanshay commented Dec 23, 2024

With Slint you may be able to implement bounce buffers (see esp-idf docs for explanation) and avoid PSRAM altogether.

That would mean pretty heavy load on the CPU, won't it? full time rendering. With complex UI it's not cheap.

Yeah there's also other benefits to that feature but oh well. Another problem is none of the Rust devs know how it works, they'd have to do some additional research.

I'm sure you can do it 😄

Put your +1/vote on the issue at least. 🙂

Done! + wrote the justification.

@Dominaezzz
Copy link
Collaborator Author

With Slint you may be able to implement bounce buffers (see esp-idf docs for explanation) and avoid PSRAM altogether.

That would mean pretty heavy load on the CPU, won't it? full time rendering. With complex UI it's not cheap.

yhh, trade offs

@Dominaezzz
Copy link
Collaborator Author

Dominaezzz commented Dec 28, 2024

With Slint you may be able to implement bounce buffers (see esp-idf docs for explanation) and avoid PSRAM altogether.

That would mean pretty heavy load on the CPU, won't it? full time rendering. With complex UI it's not cheap.

I just remembered, the CPU load could be minimised by using DMA memcpy. It'll need some tuning to get right.

@yanshay
Copy link
Contributor

yanshay commented Jan 4, 2025

I just remembered, the CPU load could be minimised by using DMA memcpy. It'll need some tuning to get right.

By 'bounce buffers' do you mean to render using slint at real time as the display is being fed with data using a small memory area of only several lines?
Or is there an option (which isn't slint specific) to have the entire screen in PSRAM and then do DMA from PSRAM to regular RAM and then DMA from regular RAM to display? Will DMA from PSRAM to regular RAM won't conflict with code execution from flash?

@Dominaezzz
Copy link
Collaborator Author

I just remembered, the CPU load could be minimised by using DMA memcpy. It'll need some tuning to get right.

By 'bounce buffers' do you mean to render using slint at real time as the display is being fed with data using a small memory area of only several lines? Or is there an option (which isn't slint specific) to have the entire screen in PSRAM and then do DMA from PSRAM to regular RAM and then DMA from regular RAM to display? Will DMA from PSRAM to regular RAM won't conflict with code execution from flash?

Bounce buffers work for both. The former would be ideal but I'm not sure if Slint will actually draw the screen fast enough to keep up with the LCD_CAM. Maybe with a bit of a head start it could work.

Code execution will impact the DMA from PSRAM to internal RAM, but if the bounce buffer is large enough, any slow downs should be absorbed by the buffer. I did get this to work with on the idf side, so it's certainly feasible. esp-hal is currently not exposing the DMA interrupts yet so you'll need the PAC to do this rn.

@yanshay
Copy link
Contributor

yanshay commented Jan 4, 2025

Code execution will impact the DMA from PSRAM to internal RAM, but if the bounce buffer is large enough, any slow downs should be absorbed by the buffer. I did get this to work with on the idf side, so it's certainly feasible. esp-hal is currently not exposing the DMA interrupts yet so you'll need the PAC to do this rn.

Oh boy, that sounds complex...
As an alternative, who needs to be convinced to prioritize #2083?

@Dominaezzz
Copy link
Collaborator Author

It is complex indeed.

who needs to be convinced

@ MabezDev is the team lead, he's the one that needs convincing.
If you ask @ bjoernQ very very very nicely, he might do it in his free time 😄

@jgtaylor
Copy link

jgtaylor commented Jan 5, 2025

I've been working on trying to implement a bounce-buffer, but ... I've only manged to basically replicate the current DmaTxBuf, but with more steps and slower 😢 . I was looking into a DMA memcpy , however, I couldn't get all the parts to fit together (and I think I have some extra screws left-over!)

My latest effort has been to look into how it's done in the esp-idf with LVGL - they use a 256k bounce-buffer, but they also run code from PSRAM (which is #2083 ).

Sadly, unless I become a Rust expert very soon (improbable), I will merely continue to bang my head on my keyboard as I very, very, slowly get good 😀

@yanshay
Copy link
Contributor

yanshay commented Jan 5, 2025

@jgtaylor How about adding your request for prioritizing #2083 on the issue discussion there?

@jessebraham
Copy link
Member

Oh boy, that sounds complex... As an alternative, who needs to be convinced to prioritize #2083?

Given the tremendous amount of work we have ahead of us trying to get a stable release out, I would not get your hopes up. This is completely outside the scope of our current focus. Will probably be faster for you to implement it yourself and submit a PR.

@yanshay
Copy link
Contributor

yanshay commented Jan 5, 2025

@jessebraham thanks for the update and setting expectations.

Will probably be faster for you to implement it yourself and submit a PR.

I wish I knew how to implement something like this. #2083 looks like way beyond my understanding of esp32. I'm more on the application programming side.
I'd be happy to assist in any way I can, like testing it under real world application etc.

Given the tremendous amount of work we have ahead of us trying to get a stable release out, I would not get your hopes up.

Can we ask for it to be considered for next release then? As I stated on the issue, it opens up large set of applications that are currently blocked from esp-hal. I think there's a strong justification for implementing it.

@Dominaezzz
Copy link
Collaborator Author

The esp-idf code snippet that bjeorn posted looks like it would do the trick.
I just don't have the time to type that in rust ATM.

@jessebraham
Copy link
Member

jessebraham commented Jan 5, 2025

Can we ask for it to be considered for next release then? As I stated on the issue, it opens up large set of applications that are currently blocked from esp-hal. I think there's a strong justification for implementing it.

When we have published the 1.0.0 release, updated the book and all other documentation, and ensured that our tooling is in reasonably good shape, then there will of course be discussions with regards to which drivers will be stabilized next.

Right now, we need to focus on creating a product that paying customers are able to use so that we can continue to justify an entire team of engineers being paid to work on this project. We have been discussing and planning this release for months already, so we need to focus on the scope of work which we have already committed to.

Realistically, our current plans will likely take us until late spring or early summer to complete. The community is welcome to contribute unstable drivers in the meantime and we will of course be happy to review them.

@yanshay
Copy link
Contributor

yanshay commented Jan 8, 2025

Great to hear 1.0.0 is on the horizon! I wasn't aware of that.

Speaking of paying customers, I believe there's a significant commercial opportunity in GUI based embedded systems, and with Slint available (looks like they are building their revenue stream mostly around that) Rust seems to have the potential to become the easiest language for developing such applications.
Seems like the only thing standing between esp-hal and such applications is this relatively small feature because all available >3.5" displays require that.
But I'm not familiar with your business directions, so its only my 2 cents.

Anyway, I'll try sometime soon to take a stab at implementing this myself, maybe I'll be able to get something working.

Thanks!

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.

RGB Interface Support for esp32-s3
7 participants