search

lvgl / lvgl_esp32_drivers

Drivers for ESP32 to be used with LVGL
MIT License
339 stars 285 forks source link

Cleanup lvgl_helpers #171

Closed C47D closed 2 years ago

C47D commented 2 years ago

Cleanup of lvgl_helpers and fixes compilation errors.

C47D commented 2 years ago

Project builds successfully in my pc with ESP-IDF v4.4-dev-2740-gf65c8249af and ESP32 as target. @tore-espressif Any idea on why it might be failing?

Also, all of the functions in lvgl_helpers are espressif specific, do you think it would be proper to move them into the lv_port directory? Same for the esp_lcd_backlight source file.

And rename the helper functions, the names are not very consistent.

tore-espressif commented 2 years ago

yep, there were some changes in esp_rom_*.h headers in IDF4.4. I'll fix it today or tomorrow

(it is weird though, I remember fixing this problem few months ago)

C47D commented 2 years ago

I can try to check your commits and look for the fixes.

arktrin commented 2 years ago

well, I have these errors and warnings:

../components/lvgl_esp32_drivers/lvgl_helpers.c:330:13: warning: 'init_ft81x' defined but not used [-Wunused-function]
 static void init_ft81x(int dma_channel)
             ^~~~~~~~~~

../components/lvgl_esp32_drivers/lvgl_tft/ra8875.c: In function 'ra8875_init':
../components/lvgl_esp32_drivers/lvgl_tft/ra8875.c:186:5: error: implicit declaration of function 'gpio_pad_select_gpio'; did you mean 'esp_rom_gpio_pad_select_gpio'? [-Werror=implicit-function-declaration]
     gpio_pad_select_gpio(RA8875_RST);
     ^~~~~~~~~~~~~~~~~~~~
     esp_rom_gpio_pad_select_gpio

../components/lvgl_esp32_drivers/lvgl_tft/GC9A01.c: In function 'GC9A01_init':
../components/lvgl_esp32_drivers/lvgl_tft/GC9A01.c:114:5: error: implicit declaration of function 'gpio_pad_select_gpio'; did you mean 'esp_rom_gpio_pad_select_gpio'? [-Werror=implicit-function-declaration]
     gpio_pad_select_gpio(GC9A01_DC);
     ^~~~~~~~~~~~~~~~~~~~
     esp_rom_gpio_pad_select_gpio

I guess gpio_pad_select_gpio(GC9A01_RST) from lvgl_tft/GC9A01.c, gpio_pad_select_gpio(ignore) and gpio_pad_select_gpio(measure) from lvgl_touch/adcraw.c will also invoke these problems.

arktrin commented 2 years ago

Single warning left during compiling due to 'init_ft81x' defined but not used. But SPI configuration fails in my esp32c3:

I (327) lvgl_helpers: Initializing SPI master
I (347) lvgl_helpers: Configuring SPI host SPI1_HOST
I (347) lvgl_helpers: MISO pin: -1, MOSI pin: 1, SCLK pin: 2, IO2/WP pin: -1, IO3/HD pin: -1
I (347) lvgl_helpers: Max transfer size: 19200 (bytes)
I (367) lvgl_helpers: Initializing SPI bus...
E (367) spi: spi_bus_initialize(756): SPI bus already initialized.

assert failed: lvgl_spi_driver_init lvgl_helpers.c:301 (ret == ESP_OK)

It tries to use SPI1_HOST but I've set SPI2_HOST in sdkconfig: CONFIG_LV_TFT_DISPLAY_SPI2_HOST=y. SPI bus already initialized message looks kind of weird to me or I've just forgotten how it should look like?

C47D commented 2 years ago

Thanks for the update, can you please attach the sdkconfig.h file?

arktrin commented 2 years ago

Content of my ./build/config/sdkconfig.h

tore-espressif commented 2 years ago

Hi all,

I couldn't follow all changes you have been doing recently, but I hope you find this useful:

@C47D This commit https://github.com/lvgl/lvgl_esp32_drivers/commit/5043946699f3e34d0f90d3fa9d31aee6ae081485 is fixing esp_lcd_backlight compilation issue (it is from august 2021), but apparently @arktrin is having a different problem.

This commit https://github.com/lvgl/lvgl_esp32_drivers/commit/8c7bc4214012b4b08854d07751a47836c5738d99 breaks compatibility with IDF v4.1 and 4.2. The esp_rom_gpio.h header file is included from v4.3 onwards

@arktrin All your gpio missing declarations (gpio_pad_select_gpio, gpio_matrix_out) are included from driver/gpio.h. This works on all IDF version and all targets. So make sure all compilation units that use these function have driver/gpio.h included

EDIT: looking to commits history in this PR, this is the trouble maker https://github.com/lvgl/lvgl_esp32_drivers/pull/171/commits/24e4bf0b888e68f09bcf1b900afa41ef76db2371 , effectively reverting that fix from august

arktrin commented 2 years ago

@tore-espressif Not sure I understand you correctly. As you can see all of my errors associated with gpio_pad_select_gpio()function are the part of lvgl_esp32_drivers itself. Where should I include driver/gpio.h ?

EDIT Currently I'm using special branch of lv_port_esp32 ( feat/new_driver_test - made by @C47D ) for testing develop branch of lvgl_esp32_drivers.

tore-espressif commented 2 years ago

I see, I thought that you are authoring the branch :) So the branch is probably broken and misses the driver/gpio.h includes.

arktrin commented 2 years ago

@tore-espressif Would you be so kind to fix feat/new_driver_test branch of lv_port_esp32 project ? :)

C47D commented 2 years ago

This commit 8c7bc42 breaks compatibility with IDF v4.1 and 4.2. The esp_rom_gpio.h header file is included from v4.3 onwards

So replacing that header with driver/gpio.h would fix the compilation error? Because AFAIK that header was present on the files, but I will double check.

I see, I thought that you are authoring the branch :) So the branch is probably broken and misses the driver/gpio.h includes.

@tore-espressif Would you be so kind to fix feat/new_driver_test branch of lv_port_esp32 project ? :)

The errors are in lvgl_esp32_drivers, this is what we are currently trying to solve, in this branch.

C47D commented 2 years ago

Single warning left during compiling due to 'init_ft81x' defined but not used. But SPI configuration fails in my esp32c3:

I (327) lvgl_helpers: Initializing SPI master
I (347) lvgl_helpers: Configuring SPI host SPI1_HOST
I (347) lvgl_helpers: MISO pin: -1, MOSI pin: 1, SCLK pin: 2, IO2/WP pin: -1, IO3/HD pin: -1
I (347) lvgl_helpers: Max transfer size: 19200 (bytes)
I (367) lvgl_helpers: Initializing SPI bus...
E (367) spi: spi_bus_initialize(756): SPI bus already initialized.

assert failed: lvgl_spi_driver_init lvgl_helpers.c:301 (ret == ESP_OK)

It tries to use SPI1_HOST but I've set SPI2_HOST in sdkconfig: CONFIG_LV_TFT_DISPLAY_SPI2_HOST=y. SPI bus already initialized message looks kind of weird to me or I've just forgotten how it should look like?

@arktrin Can you print the value of TFT_SPI_HOST? Based on your file the selected bus is SPI2, as you configured: image

So the TFT_SPI_HOST should be set to SPI2_HOST image

As a patch, can you add a return after this lines:

    lvgl_spi_driver_init(TFT_SPI_HOST, miso, DISP_SPI_MOSI, DISP_SPI_CLK,
        spi_max_transfer_size, dma_channel, DISP_SPI_IO2, DISP_SPI_IO3);

    disp_spi_add_device(TFT_SPI_HOST);
arktrin commented 2 years ago

As a patch, can you add a return after this lines:

    lvgl_spi_driver_init(TFT_SPI_HOST, miso, DISP_SPI_MOSI, DISP_SPI_CLK,
        spi_max_transfer_size, dma_channel, DISP_SPI_IO2, DISP_SPI_IO3);

    disp_spi_add_device(TFT_SPI_HOST);

I have found these lines in lvgl_helpers.c, but I'm not sure what do mean by "add a return after these lines". Should I put return; after these lines ? Putting return; do not help. I am still getting the same SPI configuration error as before.

C47D commented 2 years ago

Were you able to print TFT_SPI_HOST? I don't remember if it's possible to increase the ESP logging level, but I didn't saw some logging information on your last message

arktrin commented 2 years ago

Do this part have all corresponding endifs for proper working ?

/* Display controller initialization */
#if defined (CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI) || defined (SHARED_SPI_BUS)
    ESP_LOGI(TAG, "Initializing SPI master");

    int miso = DISP_SPI_MISO;
    int spi_max_transfer_size = calculate_spi_max_transfer_size(display_buffer_size);

    /* Set the miso signal to be the selected for the touch driver */
#if defined (SHARED_SPI_BUS)
    miso = TP_SPI_MISO;
#endif

    lvgl_spi_driver_init(TFT_SPI_HOST, miso, DISP_SPI_MOSI, DISP_SPI_CLK,
        spi_max_transfer_size, dma_channel, DISP_SPI_IO2, DISP_SPI_IO3);

    disp_spi_add_device(TFT_SPI_HOST);

    ESP_LOGI(TAG, "TFT_SPI_HOST is %d", TFT_SPI_HOST);

    /* Add device for touch driver */
#if defined (SHARED_SPI_BUS)
    tp_spi_add_device(TOUCH_SPI_HOST);
    touch_driver_init();

    return;
#endif

#elif defined (CONFIG_LV_I2C_DISPLAY)
#else
#error "No protocol defined for display controller"
#endif
C47D commented 2 years ago

Thanks for the pointers @tore-espressif, it seems to be passing now 😸

One question, is it possible to have multiple versions of ESP-IDF in the same environment? I'm working with WSL (on Windows 10) and only have ESP-IDF v4 installed. I'm wondering if cloning other versions, and exporting one at the time, would allow me to have "multiple" versions of ESP-IDF.

C47D commented 2 years ago

Do this part have all corresponding endifs for proper working ?

/* Display controller initialization */
#if defined (CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI) || defined (SHARED_SPI_BUS)
    ESP_LOGI(TAG, "Initializing SPI master");

    int miso = DISP_SPI_MISO;
    int spi_max_transfer_size = calculate_spi_max_transfer_size(display_buffer_size);

    /* Set the miso signal to be the selected for the touch driver */
#if defined (SHARED_SPI_BUS)
    miso = TP_SPI_MISO;
#endif

    lvgl_spi_driver_init(TFT_SPI_HOST, miso, DISP_SPI_MOSI, DISP_SPI_CLK,
        spi_max_transfer_size, dma_channel, DISP_SPI_IO2, DISP_SPI_IO3);

    disp_spi_add_device(TFT_SPI_HOST);

    ESP_LOGI(TAG, "TFT_SPI_HOST is %d", TFT_SPI_HOST);

    /* Add device for touch driver */
#if defined (SHARED_SPI_BUS)
    tp_spi_add_device(TOUCH_SPI_HOST);
    touch_driver_init();

    return;
#endif

#elif defined (CONFIG_LV_I2C_DISPLAY)
#else
#error "No protocol defined for display controller"
#endif

Yes, it does.

arktrin commented 2 years ago

Set this:

/* Display controller initialization */
#if defined (CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI) || defined (SHARED_SPI_BUS)
    ESP_LOGI(TAG, "Initializing SPI master");
    ESP_LOGI(TAG, "1.TFT_SPI_HOST is %d", TFT_SPI_HOST);
    int miso = DISP_SPI_MISO;
    int spi_max_transfer_size = calculate_spi_max_transfer_size(display_buffer_size);

    /* Set the miso signal to be the selected for the touch driver */
#if defined (SHARED_SPI_BUS)
    miso = TP_SPI_MISO;
#endif
    ESP_LOGI(TAG, "2.TFT_SPI_HOST is %d", TFT_SPI_HOST);
    lvgl_spi_driver_init(TFT_SPI_HOST, miso, DISP_SPI_MOSI, DISP_SPI_CLK,
        spi_max_transfer_size, dma_channel, DISP_SPI_IO2, DISP_SPI_IO3);
    ESP_LOGI(TAG, "3.TFT_SPI_HOST is %d", TFT_SPI_HOST);
    disp_spi_add_device(TFT_SPI_HOST);
    ESP_LOGI(TAG, "4.TFT_SPI_HOST is %d", TFT_SPI_HOST);

Got this:

I (332) lvgl_helpers: Initializing SPI master
I (352) lvgl_helpers: 1.TFT_SPI_HOST is 0
I (352) lvgl_helpers: 2.TFT_SPI_HOST is 0
I (352) lvgl_helpers: Configuring SPI host SPI1_HOST
I (352) lvgl_helpers: MISO pin: -1, MOSI pin: 1, SCLK pin: 2, IO2/WP pin: -1, IO3/HD pin: -1
I (372) lvgl_helpers: Max transfer size: 19200 (bytes)
I (372) lvgl_helpers: Initializing SPI bus...
E (372) spi: spi_bus_initialize(756): SPI bus already initialized.

assert failed: lvgl_spi_driver_init lvgl_helpers.c:304 (ret == ESP_OK)
arktrin commented 2 years ago

One question, is it possible to have multiple versions of ESP-IDF in the same environment? I'm working with WSL (on Windows 10) and only have ESP-IDF v4 installed. I'm wondering if cloning other versions, and exporting one at the time, would allow me to have "multiple" versions of ESP-IDF.

Not sure about WSL particularly but in linux you can have different ESP-IDFs (different path for each ESP-IDF) and export different ESP-IDF version in different session. Current session will use the IDF_PATH that was exported in it. 

C47D commented 2 years ago

Yep, TFT_SPI_HOST is not being set correctly, is set to 0, you can hardcode it to 2 while we try to figure out why it's being set to 0, it's defined in lvgl_spi_config.h

arktrin commented 2 years ago

With latest ff7b380 and 9b66665 commits I'm getting gpio_pad_select_gpio and gpio_matrix_out related stuff again :(

../components/lvgl_esp32_drivers/lvgl_tft/esp_lcd_backlight.c: In function 'disp_backlight_new':
../components/lvgl_esp32_drivers/lvgl_tft/esp_lcd_backlight.c:59:9: error: implicit declaration of function 'gpio_matrix_out'; did you mean 'gpio_iomux_out'? [-Werror=implicit-function-declaration]
         gpio_matrix_out(config->gpio_num, ledc_periph_signal[LEDC_LOW_SPEED_MODE].sig_out0_idx + config->channel_idx, config->output_invert, 0);
         ^~~~~~~~~~~~~~~
         gpio_iomux_out
../components/lvgl_esp32_drivers/lvgl_tft/esp_lcd_backlight.c:65:9: error: implicit declaration of function 'gpio_pad_select_gpio'; did you mean 'esp_rom_gpio_pad_select_gpio'? [-Werror=implicit-function-declaration]
         gpio_pad_select_gpio(config->gpio_num);
         ^~~~~~~~~~~~~~~~~~~~
         esp_rom_gpio_pad_select_gpio
../components/lvgl_esp32_drivers/lvgl_tft/esp_lcd_backlight.c:67:43: error: 'SIG_GPIO_OUT_IDX' undeclared (first use in this function); did you mean 'GPIO_NUM_MAX'?
         gpio_matrix_out(config->gpio_num, SIG_GPIO_OUT_IDX, config->output_invert, false);
                                           ^~~~~~~~~~~~~~~~
                                           GPIO_NUM_MAX
../components/lvgl_esp32_drivers/lvgl_tft/esp_lcd_backlight.c:67:43: note: each undeclared identifier is reported only once for each function it appears in

../components/lvgl_esp32_drivers/lvgl_tft/FT81x.c: In function 'FT81x_init':
../components/lvgl_esp32_drivers/lvgl_tft/FT81x.c:266:2: error: implicit declaration of function 'gpio_pad_select_gpio'; did you mean 'esp_rom_gpio_pad_select_gpio'? [-Werror=implicit-function-declaration]
  gpio_pad_select_gpio(EVE_PDN);
  ^~~~~~~~~~~~~~~~~~~~
  esp_rom_gpio_pad_select_gpio
C47D commented 2 years ago

I have been able to replicate the TFT_SPI_BUS issue, it only appears when flashing the program. Working on it.

With latest ff7b380 and 9b66665 commits I'm getting gpio_pad_select_gpio and gpio_matrix_out related stuff again :(

I'wlll push a very hacky patch, see 2f70856 and 7722f17

arktrin commented 2 years ago

with latest 2f70856 and 7722f17 :

../components/lvgl_esp32_drivers/lvgl_tft/FT81x.c: In function 'FT81x_init':
../components/lvgl_esp32_drivers/lvgl_tft/FT81x.c:266:2: error: implicit declaration of function 'gpio_pad_select_gpio'; did you mean 'esp_rom_gpio_pad_select_gpio'? [-Werror=implicit-function-declaration]
  gpio_pad_select_gpio(EVE_PDN);
  ^~~~~~~~~~~~~~~~~~~~
  esp_rom_gpio_pad_select_gpio

EDIT This is true for the latest master branch of the esp-idf.

arktrin commented 2 years ago

There are no errors with the latest release/v4.4 branch of the esp-idf. I've hardcoded TFT_SPI_HOST to work with SPI2_HOST, everything looks fine but i have nothing on my st7789 display :(

C47D commented 2 years ago

Hi @arktrin , sorry for the late reply, I was offline all weekend.

There are no errors with the latest release/v4.4 branch of the esp-idf. I've hardcoded TFT_SPI_HOST to work with SPI2_HOST, everything looks fine but i have nothing on the screen :(

I see the same issue, also tested develop (without this PR) and still have the same issue. I will try to bisect it later today.

with latest 2f70856 and 7722f17 :

../components/lvgl_esp32_drivers/lvgl_tft/FT81x.c: In function 'FT81x_init':
../components/lvgl_esp32_drivers/lvgl_tft/FT81x.c:266:2: error: implicit declaration of function 'gpio_pad_select_gpio'; did you mean 'esp_rom_gpio_pad_select_gpio'? [-Werror=implicit-function-declaration]
  gpio_pad_select_gpio(EVE_PDN);
  ^~~~~~~~~~~~~~~~~~~~
  esp_rom_gpio_pad_select_gpio

EDIT This is true for the latest master branch of the esp-idf.

Thanks for pointing out the compilation error is present on the master branch, I don't think we added it into the CI.

tore-espressif commented 2 years ago

One question, is it possible to have multiple versions of ESP-IDF in the same environment? I'm working with WSL (on Windows 10) and only have ESP-IDF v4 installed. I'm wondering if cloning other versions, and exporting one at the time, would allow me to have "multiple" versions of ESP-IDF.

@C47D

You should have only one IDF version in single environment. But you can have multiple environments.

I'm on Windows 10 and work with Windows Terminal. I have multiple versions of idf cloned so my folder tree look like this:

C:/esp_repos
├───esp-idf-v4.1
├───esp-idf-v4.2
├───esp-idf-v4.3
├───esp-idf-v4.4
├───esp-idf-master

The windows terminal JSON file contains (among other things):

            {
                "altGrAliasing": true,
                "antialiasingMode": "grayscale",
                "closeOnExit": "graceful",
                "colorScheme": "Campbell",
                "commandline": "powershell.exe -NoExit C:\\esp_repos\\esp-idf-v4.2\\export.ps1",
                "cursorShape": "bar",
                "font": 
                {
                    "face": "Cascadia Mono",
                    "size": 12
                },
                "guid": "{d6bd6b46-f392-494d-bf17-e9c670cc8e12}",
                "historySize": 9001,
                "name": "IDF v4.2",
                "padding": "8, 8, 8, 8",
                "snapOnInput": true,
                "startingDirectory": "C:\\esp_repos\\",
                "suppressApplicationTitle": true,
                "tabTitle": "IDF v4.2",
                "useAcrylic": false
            },

            ... and so on...

The shell uses Powershell, but it should be easy to use WSL too :) Result looks like this: image

C47D commented 2 years ago

There are no errors with the latest release/v4.4 branch of the esp-idf. I've hardcoded TFT_SPI_HOST to work with SPI2_HOST, everything looks fine but i have nothing on my st7789 display :(

@arktrin I've just pushed a fix to the lv_port_esp32 repo. The disp_drv registration was commented out 😿

C47D commented 2 years ago

@tore-espressif Thanks for the explanation, I'm working from a borrowed pc, will try to setup multiple environments new week, I will also try to get github actions running locally.

arktrin commented 2 years ago

TFT_SPI_HOST issue seems to be fixed ( but I'm still have nothing on my st7789 display ).

Single warning left (this is not related to my issue with display):

../components/lvgl_esp32_drivers/lvgl_helpers.c:330:13: warning: 'init_ft81x' defined but not used [-Wunused-function]
 static void init_ft81x(int dma_channel)
             ^~~~~~~~~~

I've just pushed a fix to the lv_port_esp32 repo. The disp_drv registration was commented out

Can't see any new commits in lv_port_esp32 repo.

C47D commented 2 years ago

@arktrin thanks for letting me know, I wasn't pushing to a public remote. Now you should be able to see the fix in the feat/new_driver_test branch.

arktrin commented 2 years ago

Now you should be able to see the fix in the feat/new_driver_test branch.

No warning at all! And finally now I see my app on the screen! Yay! This is definitely may be merged from my perspective.

C47D commented 2 years ago

I will add a resume so @tore-espressif can review it as well. Thanks a lot for testing it @arktrin , it took a lot longer than I was expecting.

Basically we did:

Some open topics (apart of merging develop into master):

tore-espressif commented 2 years ago

I will add a resume so @tore-espressif can review it as well.

That's an impressive amount of work, I'll do my best to review next week.

C47D commented 2 years ago

The open topics are not implemented in this PR, they are meant to be done later.

tore-espressif commented 2 years ago

As you might have already noticed, I'm not a big fan of huge #ifdefs 🗡️ :D

But apart from that, if we want to make the drivers platform agnostic we will have to deal with them, because not all platforms use Kconfig.

C47D commented 2 years ago

As you might have already noticed, I'm not a big fan of huge #ifdefs 🗡️ :D

But apart from that, if we want to make the drivers platform agnostic we will have to deal with them, because not all platforms use Kconfig.

Yes, me neither, but I think cleaning up the existing code so later it can be refactored later is a good first step. I will address a couple of comments with leftover code, and wait for your reply on all the others. Thanks for taking the time to review this :)

I will also integrate the latest develop changes 😄

C47D commented 2 years ago

Hi @tore-espressif, I did the requested changes. Is it OK if we merge this?

C47D commented 2 years ago

Version checks removed and the CI is now passing @tore-espressif