LCD_CAM: Code refactoring to prepare for future RGB panel implementation #1232

seijikun · 2024-03-02T17:04:13Z

Must

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

Nice to have

You add a description of your work to this PR.
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 · 2024-03-04T08:20:57Z

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 · 2024-03-04T09:48:54Z

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 · 2024-03-04T10:34:01Z

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 😞 ):

VID_20240304_113106195.mp4

Dominaezzz · 2024-03-04T13:24:09Z

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 · 2024-03-04T14:33:40Z

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 😅

yanshay · 2024-03-04T14:41:22Z

I tested, and strange results:

When building in release - it works as worked previously, blinking white/black the display (instead of blue/red).
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 · 2024-03-04T14:51:06Z

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

- Since both RGB and I8080 mode share most of their functionality, it makes sense to adopt esp-idf's architecture of the shared esp_ll HAL component for controlling the contained LCD peripheral.

yanshay · 2024-03-04T15:18:35Z

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 · 2024-03-04T15:23:59Z

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 · 2024-03-04T15:28:49Z

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 · 2024-03-04T15:33:07Z

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

jessebraham · 2024-03-04T16:05:40Z

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 · 2024-03-04T17:55:54Z

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 · 2024-03-04T18:05:13Z

@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 · 2024-03-04T18:10:32Z

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 · 2024-03-04T18:12:59Z

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.

Dominaezzz · 2024-03-11T21:54:31Z

esp-hal/src/lcd_cam/lcd/mod.rs

+                    .bit(cmd_cycles > 0)
+                    .lcd_cmd_2_cycle_en()
+                    .bit(cmd_cycles > 1);
+            }


Don't you need an else here?

The idea behind this interface with the optionals was basically that you can decide on calling-point which of the parameters you want to change. So basically:

set_phase_cycles(Some(0), None, Some(1));

means: Set cmd_cycles to 0, set data_cycles to 1, and leave `dummy_cycles´ unchanged.
I should probably clarify this in the docs of the function.

set is the wrong verb, modify, adjust or update would be better.

Dominaezzz

After taking a my time to review, I don't see much value in matching esp-idf exactly. Frankly, trying to understand their *_ll.h pattern has been a pain when trying grok how to talk to the ESP32 hardware and I would be sad to see that pattern followed here. Though I do see value in writing the common bits in the lcd module.

So to begin with, I don't think there's much point to the mode-specific functions, they should stay in the individual drivers for simplicity.

Dominaezzz · 2024-03-11T21:56:47Z

esp-hal/src/lcd_cam/lcd/mod.rs

+
+    /// Reset the LCD peripheral's asynchronous TX FIFO buffer.
+    #[inline(always)]
+    pub fn fifo_reset(&self) {


Suggested change

pub fn fifo_reset(&self) {

pub fn reset_async_fifo(&self) {

Dominaezzz · 2024-03-11T22:00:30Z

esp-hal/src/lcd_cam/lcd/mod.rs

+            .modify(|_, w| w.lcd_afifo_reset().set_bit());
+    }
+
+    /// (I8080-Mode) Set the level of the DC line for different transaction phases.


If this is I8080 specific, why bother having this method here? This can stay in the I8080 driver.

The "(I8080-Mode)" comment is just a "typically used by" which was added by me.
The way I understood it from a short conversation with an Espressif engineer and from the datasheet, there is not actually a mode-specific option. Even if you enable the chip's RGB-mode, some options (which are typically used for I8080 e.g.) would still work to cover certain use-cases.
That's why I followed their lcd_ll scheme: Because I'm not a big fan of making something as basic and as low in the system as a HAL too restrictive by encoding use-cases into the API. IMHO, those interfaces never really age well.
If you move I8080-specific functions to the I8080 class, you effectively prevent anyone who really knows what they are doing from working properly with the hardware by doing expert things.

But in the end, I guess it's for @jessebraham and @bjoernQ to decide.

I don't know if you've seen #1282 or #31, but the general goal with the HAL as I see it is to provide restrictive set of APIs that enforce correct usage. It sounds like what you really want is a richer PAC (which I would kinda like too).

I haven't seen it discussed on this repo yet but I think there's plenty room for improvement when it comes to exotic use cases for a peripheral. The LCD_CAM was designed for LCDs and Cameras but really one could also use it for SPI, PARLIO, etc. The I8080 driver as is can even be used for MOTO6080 (though I couldn't find a good name to capture both so I went with the more popular one imo). And I think this is the point you're making about the lcd_ll scheme to provide flexibility without starting from scratch.

Whilst I 100% agree that a HAL should enable this flexibility, without a real use case to design for, it's impossible to design something sensible or review any design/refactor. Flexibility isn't free, so one needs to make sure they know what they're paying for and if it's worth it.

The current real use case I see here and I'm reviewing for is, "sharing code between the I8080 driver and the RGB driver".
With this in mind, I'm able to determine whether the method names make sense, whether functions like this one we're looking at (set_dc_level) that sets multiple registers at once should be split up or not, or whether the function is needed at all.
At the moment this set_dc_level has fantastic name and granularity for I8080 but for RGB it doesn't. For RGB it should have a different name and should only really care about data_phase (and maybe idle_phase, but in practice once the driver is started in RGB mode it typically needs to continuously run forever).
The only reason I'm able to make this judgement is because we have a real use case to discuss. If we want to handle every possible use case ever, then a richer PAC is simply the answer.

Just my opinion at least. I'll wait for the esp-hal maintainers to comment before I review further as I don't want to waste time and brain power making comments that the hal doesn't care for.

jessebraham · 2024-04-11T13:32:58Z

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

JurajSadel · 2024-04-11T14:06:01Z

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

Unfortunately, it hasn't arrived yet.

JurajSadel · 2024-04-19T11:49:30Z

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

Dominaezzz · 2024-04-19T15:33:27Z

esp-hal/src/lcd_cam/mod.rs

+#[cfg_attr(feature = "defmt", derive(defmt::Format))]
+#[repr(u32)]
+pub enum LcdClockSource {
+    #[default]


Why is this the default? I'd expect the default to be no clock, so Option<LcdClockSource> with a default of None.

Dominaezzz · 2024-04-20T13:38:21Z

@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 · 2024-04-22T09:32:52Z

@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 · 2024-05-02T16:59:59Z

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 #1532

jessebraham · 2024-05-13T17:10:43Z

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 · 2024-05-14T08:59:21Z

We're kinda blocked on this thread #1232 (comment) before this can be wrapped up. So if you could comment on it that would help

MabezDev · 2024-05-23T10:13:14Z

@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 · 2024-05-24T13:02:23Z

@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. 🥲
No Idea if I killed them with the experimenting, or because they were rather cheap 🤔.
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 · 2024-05-24T15:07:35Z

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 :)

seijikun force-pushed the lcd_ll branch from 2af8579 to e1838b7 Compare March 4, 2024 14:49

seijikun force-pushed the lcd_ll branch from e1838b7 to 8331f5f Compare March 4, 2024 14:53

seijikun added 2 commits March 4, 2024 15:55

Implement lcd_ll HAL methods in shared Lcd struct

c524564

- Since both RGB and I8080 mode share most of their functionality, it makes sense to adopt esp-idf's architecture of the shared esp_ll HAL component for controlling the contained LCD peripheral.

Port parts of I8080 implementation to Lcd HAL methods

ab921d0

seijikun force-pushed the lcd_ll branch from 8331f5f to ab921d0 Compare March 4, 2024 14:55

Dominaezzz reviewed Mar 11, 2024

View reviewed changes

bjoernQ mentioned this pull request Mar 14, 2024

RFC: Investigate the need for a lower-level layer of abstraction #1282

Closed

Dominaezzz reviewed Apr 19, 2024

View reviewed changes

jessebraham added the status:needs-attention This should be prioritized label Apr 24, 2024

jessebraham assigned JurajSadel May 2, 2024

jessebraham unassigned JurajSadel May 21, 2024

jessebraham closed this May 24, 2024

LCD_CAM: Code refactoring to prepare for future RGB panel implementation #1232

LCD_CAM: Code refactoring to prepare for future RGB panel implementation #1232

Conversation

seijikun commented Mar 2, 2024 • edited

Must

Nice to have

bjoernQ commented Mar 4, 2024

yanshay commented Mar 4, 2024 • edited

seijikun commented Mar 4, 2024 • edited

Dominaezzz commented Mar 4, 2024

seijikun commented Mar 4, 2024 • edited

yanshay commented Mar 4, 2024

seijikun commented Mar 4, 2024

yanshay commented Mar 4, 2024

seijikun commented Mar 4, 2024 • edited

yanshay commented Mar 4, 2024

yanshay commented Mar 4, 2024

jessebraham commented Mar 4, 2024 • edited

seijikun commented Mar 4, 2024

yanshay commented Mar 4, 2024

seijikun commented Mar 4, 2024

Dominaezzz commented Mar 4, 2024 • edited

Dominaezzz Mar 11, 2024

Choose a reason for hiding this comment

seijikun Apr 19, 2024 • edited

Choose a reason for hiding this comment

Dominaezzz Apr 19, 2024

Choose a reason for hiding this comment

Dominaezzz left a comment

Choose a reason for hiding this comment

Dominaezzz Mar 11, 2024

Choose a reason for hiding this comment

Dominaezzz Mar 11, 2024

Choose a reason for hiding this comment

seijikun Apr 19, 2024 • edited

Choose a reason for hiding this comment

Dominaezzz Apr 19, 2024

Choose a reason for hiding this comment

jessebraham commented Apr 11, 2024

JurajSadel commented Apr 11, 2024

JurajSadel commented Apr 19, 2024

Dominaezzz Apr 19, 2024

Choose a reason for hiding this comment

Dominaezzz commented Apr 20, 2024 • edited

JurajSadel commented Apr 22, 2024 • edited

JurajSadel commented May 2, 2024

jessebraham commented May 13, 2024

Dominaezzz commented May 14, 2024

MabezDev commented May 23, 2024

seijikun commented May 24, 2024 • edited

jessebraham commented May 24, 2024

seijikun commented Mar 2, 2024 •

edited

yanshay commented Mar 4, 2024 •

edited

seijikun commented Mar 4, 2024 •

edited

seijikun commented Mar 4, 2024 •

edited

seijikun commented Mar 4, 2024 •

edited

jessebraham commented Mar 4, 2024 •

edited

Dominaezzz commented Mar 4, 2024 •

edited

seijikun Apr 19, 2024 •

edited

seijikun Apr 19, 2024 •

edited

Dominaezzz commented Apr 20, 2024 •

edited

JurajSadel commented Apr 22, 2024 •

edited

seijikun commented May 24, 2024 •

edited