LCD_CAM: Code refactoring to prepare for future RGB panel implementation

[seijikun]

Must

• [x] The code compiles without errors or warnings.
• [x] All examples work.
• [x] cargo fmt was run.
• [ ] Your changes were added to the CHANGELOG.md in the proper section.

Nice to have

• [x] You add a description of your work to this PR.
• [x] You added proper docs for your newly added features and code.

---

This PR basically implements esp-idf's internal lcd_ll HAL on the common Lcd<'d> struct with all the common functionality shared between I8080 and a potential future RGB implementation.

In @Dominaezzz 's implementation, all of this register pushing was located inside the I8080 implementation. I attempted to port most of this from code inside I8080 to calling this shared functionality, but there were some places I didn't dare to touch since I don't have the hardware. (I have a RGB-Bus display that I plan to play around with though)

I have multiple reasons for this change:

• More code sharing between I8080 and a potential future RGB implementation
• Closer to esp-idf upstream, which is IMHO never a bad idea
• More flexible - it allows users to operate the hardware in neither pure I8080 nor pure RGB kinda way (there seem to be some very corner-casy uses)

Would be nice if someone could verify my changes by running the I8080 example on suitable hardware.

This can be seen as an RFC of sorts. If this is deemed an acceptable change, I will add an entry to the changelog.

[bjoernQ]

Having common code shared sounds like a good idea. Unfortunately, I don't have the hardware to really test this. Would be nice if @Dominaezzz or @yanshay would have time to verify this

[yanshay]

Having common code shared sounds like a good idea. Unfortunately, I don't have the hardware to really test this. Would be nice if @Dominaezzz or @yanshay would have time to verify this

Happy to assist, @seijikun - do you want me to test this PR on an I8080 device? Or want to make additional changes before I test?

Also, when you test your RGB code I'd recommend testing in release mode. the I8080 example doesn't work well in release mode, seems like timing issues.

BTW - what device are you using with the RGB code? I planned getting something along the lines of https://www.aliexpress.com/item/1005005529208137.html which says it's RGB, will it work with your code?

[seijikun]

Happy to assist, @seijikun - do you want me to test this PR on an I8080 device? Or want to make additional changes before I test?

Exactly. If you could run the I8080 example and verify that my refactoring didn't break anything, that would be nice!

Also, when you test your RGB code I'd recommend testing in release mode. the I8080 example doesn't work well in release mode, seems like timing issues.

I think that might be different for RGB devices, because they don't need timing-critical CPU intervention. You basically allocate a framebuffer for the entire display in PSRAM and then configure the LCD peripheral to endlessly pipe that out to the display using a circular DMA setup. There is no actively driving from the CPU like on I8080. When you write stuff into the framebuffer via CPU, it gets "automatically" streamed to the display.

BTW - what device are you using with the RGB code? I planned getting something along the lines of https://www.aliexpress.com/item/1005005529208137.html which says it's RGB, will it work with your code?

I'm using the Makerfabs ESP32-S3 Parallel TFT 4.3". I tried finding more information on the device from your link but couldn't find any more information than: "Interface: RGB". If that little piece of information is true, then it should.

Though I don't want to make any promises, because even though I 1:1 cloned the esp-idf RGB-Panel implementation for testing, a two-color picture (one color upper half, second color lower half) currently looks like this (tried both Debug & Release already :disappointed: ):

https://github.com/esp-rs/esp-hal/assets/820845/e3305e9d-7944-40d0-97d5-af28ec4a0256

[Dominaezzz]

I'll have some time later this week to take a look and run the example.

I also have a couple of Makerfabs RGB devkits so I can test an RGB driver if/when the time comes. I actually have a driver working myself but it's too bespoke/unsafe for esp-hal, so I'd be interested to see someone else's implementation.

a two-color picture (one color upper half, second color lower half) currently looks like this

If you're streaming from PSRAM you'll want to drop the frequency real low to about 16Mhz. Also make sure you've set the timings correctly, the registers can be confusing to use. esp-hal is also lacking support for this which will be crucial for this workload.

[seijikun]

I actually have a driver working myself but it's too bespoke/unsafe for esp-hal, so I'd be interested to see someone else's implementation.

@Dominaezzz Oh you already have a working one? Could you please upload that somewhere? I think that could save me a lot of frustrating debug time. I'd be very happy if the architecture/unsafeness was my only problem, because I've checked the timings and register values probably about 3 or 4 times now :sweat_smile:

[yanshay]

I tested, and strange results:

1. When building in release - it works as worked previously, blinking white/black the display (instead of blue/red).
2. When building not in release it panics on attempt to subtract on overflow, where I don't see subtraction on the line it panicked. I ran using cargo +esp run --bin lcd_i8080 --target xtensa-esp32s3-none-elf --features esp32s3. So I tried this on esp-hal/main and there it works. So something in your changes seem to be driving this, but it's really strange how that could be the error that is triggered.

!! A panic occured in '/Users/user/QAProj/esp-hal-rgb/esp-hal/src/lcd_cam/lcd/mod.rs', at line 153, column 30:
attempt to subtract with overflow

Backtrace:

0x42029f24
0x42029f24 - core::panicking::panic
    at /Users/user/.rustup/toolchains/esp/lib/rustlib/src/rust/library/core/src/panicking.rs:144
0x4200bf58
0x4200bf58 - esp_hal::lcd_cam::lcd::Lcd::set_phase_cycles::{{closure}}
    at /Users/user/QAProj/esp-hal-rgb/esp-hal/src/lcd_cam/lcd/mod.rs:153
0x4200d406
0x4200d406 - esp32s3::generic::Reg<REG>::modify
    at /Users/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/esp32s3-0.24.0/src/generic.rs:221
0x4200db93
0x4200db93 - esp_hal::lcd_cam::lcd::i8080::I8080<TX,P>::send
    at /Users/user/QAProj/esp-hal-rgb/esp-hal/src/lcd_cam/lcd/i8080.rs:270
0x42010b27
0x42010b27 - lcd_i8080::__xtensa_lx_rt_main
    at /Users/user/QAProj/esp-hal-rgb/examples/src/bin/lcd_i8080.rs:120
0x4201f2e8
0x4201f2e8 - Reset
    at /Users/user/.cargo/registry/src/index.crates.io-6f17d22bba15001f/xtensa-lx-rt-0.16.0/src/lib.rs:70
0x403785ea
0x403785ea - ESP32Reset
    at /Users/user/QAProj/esp-hal-rgb/esp-hal/src/soc/esp32s3/mod.rs:111
0x3ffffffd
0x3ffffffd - _stack_start_cpu0
    at ??:??
0x403cdd79
0x403cdd79 - __user_exception
    at ??:??
0x403c9971
0x403c9971 - __user_exception

[seijikun]

@yanshay If the release build worked, that sounds good! Could you please test Debug again with the lastest changes?

[yanshay]

Good news - it is now working!

What was it in previous version that caused only in debug mode for a panic on subtract overflow where the code didn't have any subtraction?

[seijikun]

Good news - it is now working!

What was it in previous version that caused only in debug mode for a panic on subtract overflow where the code didn't have any subtraction?

In Debug builds, Rust asserts if an integer operation would wrap. e.g.:

let val0: u32 = std::env::args().len() as u32;
let val1: u32 = 9999;
let val2: u32 = val0 - val1;

(I just used std::env::args().len() for a number so the compiler doesn't know it while compiling) This code will crash on debug and run on release. That's what happened here. (The wrapping in this case for the lcd peripheral is known, not a bug and does not have any effects)

There were subtractions in set_phase_cycle(), basically corresponding to these lines in esp-idf.

[yanshay]

I just noticed I was looking at the wrong mod.rs file, that's why it seemed strange (since there was no subtraction on the line I was looking at).

[yanshay]

@seijikun what frame rate do you reach on that device? at what PSRAM clock?

[jessebraham]

Just for the record, we strongly discourage building using the dev profile; you should always use release. dev can be multiple orders of magnitude slower than release and causes some drivers to not work at all.

I've considered making it a hard build error when not targeting release, but I fear that would cause issues.

(We should probably be more explicit about this!)

[seijikun]

what frame rate do you reach on that device? at what PSRAM clock?

@yanshay Oof, that's hard to know, since the updating happens transparently in the background. The manufacturer lists it as > 30 FPS, but quite frankly I don't know. PSRAM is running at 80MHz Octal (in esp-idf c++).

Just for the record, we strongly discourage building using the dev profile; you should always use release. dev can be multiple orders of magnitude slower than release and causes some drivers to not work at all.

Oh, that's good to know. Thanks!

[yanshay]

@yanshay Oof, that's hard to know, since the updating happens transparently in the background. The manufacturer lists it as > 30 FPS, but quite frankly I don't know. PSRAM is running at 80MHz Octal (in esp-idf c++).

Right, forgot about that. Is there a way to get some interrupt or some way to sync screen data updates with display updates (to avoid seeing split screen, half from previous frame and half from next frame)? Or any way to do double buffering?

To see fps It's possible to use slow motion video recording (I did with iphone to verify my code metrics are more or less accurate) to see the screen updates and time. Not the most accurate but gives a feeling. Not critical though, just curious.

[seijikun]

Is there a way to get some interrupt or some way to sync screen data updates with display updates (to avoid seeing split screen, half from previous frame and half from next frame)? Or any way to do double buffering?

Yeah, it's possible. You can read up about the different operation modes in the ESP32S3 LCD docs page. Double buffering is supported by the esp-idf lcd driver, but it has its own kinda problems (what Dominaezzz meantioned, e.g.). I don't know how far away supporting that in esp-hal is, tbh, since that's a fair bit more complex with ownership semantics.

[Dominaezzz]

Is there a way to get some interrupt or some way to sync screen data updates with display updates (to avoid seeing split screen, half from previous frame and half from next frame)? Or any way to do double buffering?

Short answer is yes but it's all very DIY.

Oh you already have a working one? Could you please upload that somewhere?

Sure, I'll consider it when I get some spare cycles.

what frame rate do you reach on that device? at what PSRAM clock?

On that particular display the max FPS I was able to get was 83 FPS. Either using PSRAM or SRAM. If PSRAM (or flash) had to be touched (i.e. to update the frame buffer or read some image from flash), I'd have to drop the frequency to not get garbage, leaving me with an FPS of 45. This is all at 80MHz PSRAM speed. Things may be different with 120MHz or a bounce buffer.

[jessebraham]

@JurajSadel any updates on the LCD devkit we ordered to test this driver?

[JurajSadel]

@JurajSadel any updates on the LCD devkit we ordered to test this driver?

Unfortunately, it hasn't arrived yet.

[JurajSadel]

Hello guys, finally, I received a HW so I can test/help with this PR.

[Dominaezzz]

@JurajSadel Mind running the example in main in release mode (and debug mode if you can)? I'm curious if you get black/white or red/blue. relevance

Also which devkit do you have?

[JurajSadel]

@JurajSadel Mind running the example in main in release mode (and debug mode if you can)? I'm curious if you get black/white or red/blue. relevance

Also which devkit do you have?

@Dominaezzz I ran the example (original one and also with this refactoring changes) and in debug, it works as expected (bleu and red) but in release it's black and white. I've been playing a bit with the delays and tried figured out what might be the issue here, but no luck yet. I will continue looking into that a little bit more. However, the issue is not related to this PR so maybe we can wrap it up? I'm using WT32-SC01 Plus.

[JurajSadel]

Hello @seijikun, sorry I don't have time right now to investigate the release profile issue (not related to this PR, though!). It would be nice if we could wrap up and finish this PR for final review, from my perspective LGTM, however,r rebase is needed. Would you mind doing that, please, or should we find someone to handle that? For the known issue with the release profile I opened https://github.com/esp-rs/esp-hal/issues/1532

[jessebraham]

I would really like to get this wrapped up, @seijikun are you able to rebase this branch, or should we get @JurajSadel to take over here?

[Dominaezzz]

We're kinda blocked on this thread https://github.com/esp-rs/esp-hal/pull/1232#discussion_r1520479233 before this can be wrapped up. So if you could comment on it that would help

[MabezDev]

@seijikun Thanks for the PR! I'm not sure adding flexibility for the sake of flexibility is a good idea in the long run. Do you have any plans for a follow up to this?

[seijikun]

@seijikun Thanks for the PR! I'm not sure adding flexibility for the sake of flexibility is a good idea in the long run. Do you have any plans for a follow up to this?

@MabezDev Originally, that was my plan. I also have (not yet working) experiment code for it (results can be seen from the video above). However, while attempting to build the driver - both of the two displays that I had died. :smiling_face_with_tear: No Idea if I killed them with the experimenting, or because they were rather cheap :thinking:. Since I switched to other hardware for my project, I will have to yield on the follow-up, unfortunately.

W.r.t merging this MR, like @Dominaezzz said, it depends on what the focus of esp-hal is. I didn't know about the discussion in the other Issues.

[jessebraham]

Thanks for the update. I think I am going to go ahead and close this PR at this point, thank you regardless for your contributions. Any conversation(s) can be continued in issues/discussions, and perhaps once we have made some decisions this can be revived and/or revisited in the future :)