lvgl / lv_binding_micropython

LVGL binding for MicroPython
MIT License
250 stars 161 forks source link

Avoid esp32-s3 compile error #243

Open imliubo opened 1 year ago

imliubo commented 1 year ago

Related: #227

No guarantee that everyone‘s ESP32-S3 boards will work, but it worked for my ESP32-S3 board.It will not affect the ESP32 boards, because the new added content has a judgment on the chip type by like blow code:

#if CONFIG_IDF_TARGET_ESP32
// old...
#elif CONFIG_IDF_TARGET_ESP32S3
// new added...
#endif

But the this added may have some impact:Link. I do some simple test it no impact with my device(both esp32 and esp32-s3 device) and simple demos.

amirgon commented 1 year ago

Hi @imliubo!

Thank you for this contribution!

Which display driver do you use with esp32-s3?

It will not affect the ESP32 boards, because the new added content has a judgment on the chip type by like blow code

ESP32 compiles with your changes, but does not run correctly. For example, esp_clk_cpu_freq function is missing from the espidf module, but is used by ili9XXX.py.
That's because you filter clk.h (and other important headers such as gpio.h)

What was your motivation for removing these headers from the espidf module?

I do some simple test it no impact with my device(both esp32 and esp32-s3 device) and simple demos.

If you complete the tasks in https://github.com/lvgl/lv_binding_micropython/issues/227 you will be eligable to get paid for your contribution!


Question for @kisvegabor: https://github.com/lvgl/lv_binding_micropython/issues/227 offers 100 USD for adding support for ESP32-S2, ESP32-C3, ESP32-S3.
What does LVGL offer for fully implementing only one of these flavours (esp32-s3) including testing, videos etc.?
I would recommend paying the total sum of 100$ for fully implementing even one of these ESP flavours (but it's your decision of course).

imliubo commented 1 year ago

Which display driver do you use with esp32-s3?

I use a C code driver(not ili94341.py).

ESP32 compiles with your changes, but does not run correctly. For example, esp_clk_cpu_freq function is missing from the espidf module, but is used by ili9XXX.py. That's because you filter clk.h (and other important headers such as gpio.h) What was your motivation for removing these headers from the espidf module?

emmm... I use my own display driver,so I am not test the ili9341.py driver.I remove those headers beacuse it has some problem with ESP32-S3 compile, looks need figrue out why the error happen instead of just remove these files :), late I will try again.

If you complete the tasks in https://github.com/lvgl/lv_binding_micropython/issues/227 you will be eligable to get paid for your contribution!

Actually this also was my work, so no need any paid. But thanks for this awesome project. I would be happy to contribute some very little help for this.

Anyway, I will try fix these mistakes late. Have a nice day :)

kisvegabor commented 1 year ago

Question for @kisvegabor: https://github.com/lvgl/lv_binding_micropython/issues/227 offers 100 USD for adding support for ESP32-S2, ESP32-C3, ESP32-S3. What does LVGL offer for fully implementing only one of these flavours (esp32-s3) including testing, videos etc.? I would recommend paying the total sum of 100$ for fully implementing even one of these ESP flavours (but it's your decision of course).

Actually this also was my work, so no need any paid. But thanks for this awesome project. I would be happy to contribute some very little help for this.

I know it's not a life changer amount of money, but that's what we can offer now and I'd really like to give it to people who work on making LVGL better. We are a little bit tight on budget but 70 USD for the S3 part still work.

imliubo commented 1 year ago

A liitle process for esp32s3.

Simple test with example3 and use ili9XXX.py driver, but esp32s3 has some problem.

video

Left is esp32, right is esp32s3.

kisvegabor commented 1 year ago

It seems for some reason you DMA can't send out all the bytes in the flush_cb. Maybe the DMA has limit for the number of bytes in one transfer?

imliubo commented 1 year ago

It seems for some reason you DMA can't send out all the bytes in the flush_cb. Maybe the DMA has limit for the number of bytes in one transfer?

@kisvegabor Yeah, looks had some limit, but I am not deep in check this,it can be work by reducing display buf_size (set factor=8).

Test result with this example:

  1. esp32 with ili9xxx.py driver OK.
  2. esp32s3 with ili9xxx.py driver OK(need reduce display buf_size for now).

video

It need more test with different lcds, and still has some issuse with esp-idf module, because some API is not support esp32s3 for now,see related issues: IDFGH-8771 and IDFGH-8773, To fix this, I added 3 functions (temporary solution). IDFGH-8772, to fix this, I add lldesc.h to FILTER, because i think this funciton is not use?If I am wrong please told me :)

All test is build with esp-idf v4.4.2-1b16ef6cfc2479a08136782f9dc57effefa86f66

@amirgon has any suggest? Have a nice day.

amirgon commented 1 year ago

emmm... I use my own display driver,so I am not test the ili9341.py driver

Could you tell us more about your display driver?

Also, note that there are two different ili9341 drivers: one that uses DMA driver/esp32/ili9XXX.py and a slower pure Python one driver/generic/ili9xxx.py that is not ESP32-specific.
Which one are you trying? Or both?

Actually this also was my work, so no need any paid. But thanks for this awesome project. I would be happy to contribute some very little help for this.

Thanks!!

and still has some issuse with esp-idf module, because some API is not support esp32s3 for now,see related issues:

It's ok to filter out some espidf headers that are not in use.
Many headers are included unintentionally (because headers we want include other headers we don't want), so as long as the APIs we want are not excluded, it's ok to filter out (and in fact helps because it reduces program size)

These are the headers that we want and that are included explicitly:

https://github.com/lvgl/lv_binding_micropython/blob/c544c51c97b38907709a0cd7ea7ca8ab96410cbb/driver/esp32/espidf.h#L117-L130

So I think you can filter out other headers such as rtc_io.h, lldesc.h, esp_eth.h, etc. for all ESP flavors.

If a header is required but specific functions are missing (For example, we need gpio.h, but gpio_input_get_high and gpio_output_set_high are missing) then I think it's ok to provide an empty implementation that invokes an error as long as these functions are not in use today in our code (at least until the issues your opened on esp-idf are resolved).

imliubo commented 1 year ago

Could you tell us more about your display driver?

Yeah, it kind like TFT_eSPI, when not use LVGL, it still has many draw functions can be use.

Also, note that there are two different ili9341 drivers: one that uses DMA driver/esp32/ili9XXX.py and a slower pure Python one driver/generic/ili9xxx.py that is not ESP32-specific. Which one are you trying? Or both?

This one driver/esp32/ili9XXX.py, not driver/generic/ili9xxx.py.

If a header is required but specific functions are missing (For example, we need gpio.h, but gpio_input_get_high and gpio_output_set_high are missing) then I think it's ok to provide an empty implementation that invokes an error as long as these functions are not in use today in our code (at least until the issues your opened on esp-idf are resolved).

Yes,need espressif resolved this.

amirgon commented 1 year ago

@imliubo Is this ready from your side or do you plan to make more changes?
I think mkrules.cmake can be simplified by filtering some headers from all ESP flavours instead of specifically from esp32s3.

imliubo commented 1 year ago

@amirgon Yeah I can do this, but I only have ESP32 and ESP32-S3 for now, I will buy new C3 and S2 board test this. And the issue opened at esp-idf is not resolved for now. I will do more test at this weekend.

amirgon commented 1 year ago

I only have ESP32 and ESP32-S3 for now

I think that even if your PR covers only ESP32 and ESP32-S3, it's good enough for now.
We can always add C3 and S2 later.
So basically what's missing is simplifying mkrules.cmake a little, and testing.

eggfly commented 8 months ago

LGTM! @imliubo This patch is all I need for my S3 board.