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

Avoid esp32-s3 compile error #243

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

imliubo
Copy link
Contributor

@imliubo imliubo commented Nov 16, 2022

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
Copy link
Collaborator

amirgon commented Nov 17, 2022

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 #227 you will be eligable to get paid for your contribution!


Question for @kisvegabor: #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
Copy link
Contributor Author

imliubo commented Nov 18, 2022

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 #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
Copy link
Member

Question for @kisvegabor: #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
Copy link
Contributor Author

imliubo commented Nov 19, 2022

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
Copy link
Member

kisvegabor commented Nov 19, 2022

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
Copy link
Contributor Author

imliubo commented Nov 19, 2022

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
Copy link
Collaborator

amirgon commented Nov 19, 2022

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:

#else
# include "esp_clk.h"
#endif
#include "driver/gpio.h"
#include "driver/spi_master.h"
#include "esp_heap_caps.h"
#include "esp_log.h"
#include "driver/adc.h"
#include "driver/i2s.h"
#include "driver/pcnt.h"
#include "mdns.h"
#include "esp_http_client.h"
#include "sh2lib.h"

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
Copy link
Contributor Author

imliubo commented Nov 20, 2022

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
Copy link
Collaborator

amirgon commented Nov 22, 2022

@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
Copy link
Contributor Author

imliubo commented Nov 24, 2022

@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
Copy link
Collaborator

amirgon commented Nov 25, 2022

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
Copy link

eggfly commented Jan 23, 2024

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

@Phil-LJZ Phil-LJZ mentioned this pull request Aug 22, 2024
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.

4 participants