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

A few questions about setting up a project via menuconfig #277

Closed
SinglWolf opened this issue May 19, 2021 · 11 comments
Closed

A few questions about setting up a project via menuconfig #277

SinglWolf opened this issue May 19, 2021 · 11 comments

Comments

@SinglWolf
Copy link

SinglWolf commented May 19, 2021

I express my deep gratitude for the wonderful library. My project sparkled with new colors and acquired new opportunities. Thank you! BUT there are a few annoying points.
Why is there no way to select a display driver at runtime? Flash size 4M (or larger) allows you to store multiple display driver code.
Only 1 display driver (touchscreen driver) is always available in a project. It is even spelled out at the CMakeLists.txt level.
Why is there no way to dynamically change the LV_HOR_RES_MAX and LV_VER_RES_MAX values ​​depending on the display driver (for example, as an external variable)?
For example, for ILI9341 LV_HOR_RES_MAX = 320, LV_VER_RES_MAX = 240. And for ILI9488 LV_HOR_RES_MAX = 480, LV_VER_RES_MAX = 320.
There are many places in the lvgl library as well as in the lvgl_esp32_drivers files where the LV_HOR_RES_MAX and LV_VER_RES_MAX macros are used. E.g.

void lv_disp_drv_init(lv_disp_drv_t * driver)
{
    _lv_memset_00(driver, sizeof(lv_disp_drv_t));

    driver->flush_cb         = NULL;
    driver->hor_res          = LV_HOR_RES_MAX;
    driver->ver_res          = LV_VER_RES_MAX;
    driver->buffer           = NULL;
    driver->rotated          = LV_DISP_ROT_NONE;
    driver->sw_rotate        = 0;
    driver->color_chroma_key = LV_COLOR_TRANSP;
    driver->dpi = LV_DPI;
....
}

When choosing a DARK theme, the LIGHT theme always remains.
Kconfig

choice LV_THEME_DEFAULT_FLAG
    depends on LV_THEME_MATERIAL

    prompt "Select theme default flag"
    default LV_THEME_DEFAULT_FLAG_LIGHT
    help
        Select theme default flag

    config LV_THEME_DEFAULT_FLAG_LIGHT
        bool "Light theme"
    config LV_THEME_DEFAULT_FLAG_DARK
        bool "Dark theme"
endchoice

lv_conf_internal.h

#ifndef LV_THEME_DEFAULT_FLAG
#  ifdef CONFIG_LV_THEME_DEFAULT_FLAG
#    define LV_THEME_DEFAULT_FLAG CONFIG_LV_THEME_DEFAULT_FLAG
#  else
#    define  LV_THEME_DEFAULT_FLAG               LV_THEME_MATERIAL_FLAG_LIGHT
#  endif
#endif

Initially I used the ILI9341 driver and the LV_COLOR_16_SWAP option was selected in the menuconfig.
When the ILI9488 driver was selected, the color display on the screen was incorrect. I spent a lot of time and effort to understand the problem of disgusting color on the display. The color was broken due to the selected LV_COLOR_16_SWAP option.

Of course, I can modify the source myself to fix these problems. But on the next update, everything will break again.

@SinglWolf SinglWolf changed the title Несколько вопросов по настройке проекта через menuconfig A few questions about setting up a project via menuconfig May 19, 2021
@tore-espressif
Copy link
Collaborator

Hello @SinglWolf ,

TL;DR Nobody has done it yet.

Few thoughts on this:

  1. lvgl_esp32_drivers don't support all functionality from LVGL (e.g. multiple displays). There is some refactoring planned, to make most of the options configurable on runtime, thus reducing the complexity of menuconfig. But it will take some time (weeks/months).
    Meanwhile: LV_HOR_RES_MAX and LV_VER_RES_MAX are not tightly coupled with the driver. Just above

void lv_disp_drv_init(lv_disp_drv_t * driver)
{
_lv_memset_00(driver, sizeof(lv_disp_drv_t));

driver->flush_cb = NULL;
driver->hor_res = LV_HOR_RES_MAX;
driver->ver_res = LV_VER_RES_MAX;
driver->buffer = NULL;
driver->rotated = LV_DISP_ROT_NONE;
driver->sw_rotate = 0;
driver->color_chroma_key = LV_COLOR_TRANSP;
driver->dpi = LV_DPI;
....
}

There is a comment:

/**
 * Initialize a display driver with default values.
 * It is used to surly have known values in the fields ant not memory junk.
 * After it you can set the fields.
 * @param driver pointer to driver variable to initialize
 */

so feel free to change(lower) driver.hor_res and driver.ver_res (although I haven't tried it)

Just out of my curiosity, why do you need to change display driver on runtime? Do you plan to support different displays with single binary? (haven't checked you project yet, sry ¯_(ツ)_/¯)

FYI: the code was heavily modified in latest release of LVGL, see here:
https://github.com/lvgl/lvgl/blob/master/src/hal/lv_hal_disp.c#L56
But this example repo was not updated.

  1. Another set of issues rises from the fact that Kconfig does not support (at least in ESP-IDF) merging multiple menuconfig menus from different projects (Kconfig files). So you will end up with one menu for LVGL and another one for LVGL-TFT. Which is really annoying with LV_COLOR_16_SWAP because the swapping is managed by LVGL and not the display driver...

@SinglWolf
Copy link
Author

so feel free to change(lower) driver.hor_res and driver.ver_res (although I haven't tried it)

Not entirely true statement. There are several places in the library where the LV_HOR_RES_MAX and LV_VER_RES_MAX macros are used. For example, in the lv_draw_label.c file

static void draw_letter_subpx(lv_coord_t pos_x, lv_coord_t pos_y, lv_font_glyph_dsc_t * g, const lv_area_t * clip_area,
                              const uint8_t * map_p, lv_color_t color, lv_opa_t opa, lv_blend_mode_t blend_mode)
{
.......
int32_t mask_buf_size = box_w * box_h > LV_HOR_RES_MAX ? LV_HOR_RES_MAX : g->box_w * g->box_h;
.......
}

And I am using this widget.
Also macros LV_HOR_RES_MAX are used when calculating the buffer size. For example,

#elif defined CONFIG_LV_TFT_DISPLAY_CONTROLLER_ILI9488
#define DISP_BUF_SIZE (LV_HOR_RES_MAX * 40)
#elif defined CONFIG_LV_TFT_DISPLAY_CONTROLLER_ILI9341
#define DISP_BUF_SIZE (LV_HOR_RES_MAX * 40)

Do you plan to support different displays with single binary?

Yes, I want to support multiple displays on the same firmware. Collecting separate firmware for a separate display is tedious and simply wrong.

@tore-espressif
Copy link
Collaborator

Again, just above the DISP_BUF_SIZE there is a comment:

/* DISP_BUF_SIZE value doesn't have an special meaning, but it's the size
 * of the buffer(s) passed to LVGL as display buffers. The default values used
 * were the values working for the contributor of the display controller.
 *
 * As LVGL supports partial display updates the DISP_BUF_SIZE doesn't
 * necessarily need to be equal to the display size.
 *
 * When using RGB displays the display buffer size will also depends on the
 * color format being used, for RGB565 each pixel needs 2 bytes.
 * When using the mono theme, the display pixels can be represented in one bit,
 * so the buffer size can be divided by 8, e.g. see SSD1306 display size. */

@tore-espressif
Copy link
Collaborator

I'll check draw_label issue tomorrow

@SinglWolf
Copy link
Author

Again, just above the DISP_BUF_SIZE there is a comment:

I read this. But another macro depends on the value of the DISP_BUF_SIZE macro. This is SPI_BUS_MAX_TRANSFER_SZ, which is used in the void lvgl_driver_init(void) function.

@SinglWolf
Copy link
Author

SinglWolf commented May 20, 2021

Here is an example of my display initialization code:

	lv_init();

	lv_disp_drv_t disp_drv;
	lv_disp_drv_init(&disp_drv);
	if (MainConfig->lcd_type == 0) // ILI9341
	{
		disp_drv.hor_res = 320;
		disp_drv.ver_res = 240;
	}
	else if (MainConfig->lcd_type == 1) // ILI9488
	{
		disp_drv.hor_res = 480;
		disp_drv.ver_res = 320;
	}

	static lv_color_t *buf1 = NULL;
	static uint16_t disp_buff_size = 0, max_transfer = 0;
	if ((MainConfig->lcd_type == 0) || (MainConfig->lcd_type == 1))
	{
		disp_buff_size = disp_drv.hor_res * 40;
	}
	if (!bigSram())
	{
		disp_buff_size = disp_drv.hor_res * 20;
	}
	if (MainConfig->lcd_type == 0) // ILI9341
	{
		max_transfer = disp_buff_size * 2;
	}
	else if (MainConfig->lcd_type == 1) // ILI9488
	{
		max_transfer = disp_buff_size * 3;
	}
	/* Initialize SPI or I2C bus used by the drivers */
	lvgl_driver_init(max_transfer);
/* ****** */
	ESP_LOGI(TAG, "Display buffer size: %d", disp_buff_size);
	lv_disp_buf_init(&disp_buf, buf1, buf2, size_in_px);
/* ****** */
	disp_drv.flush_cb = disp_driver_flush;
	disp_drv.buffer = &disp_buf;
	lv_disp_drv_register(&disp_drv);

My fixes in lvgl_esp32_drivers library files:
lvgl_helpers.h

/* Initialize detected SPI and I2C bus and devices */
// void lvgl_driver_init(void);
void lvgl_driver_init(uint16_t max_transfer_size);

lvgl_helpers.c

/* Interface and driver initialization */
void lvgl_driver_init(uint16_t max_transfer_size)
{
    // ESP_LOGI(TAG, "Display hor size: %d, ver size: %d", LV_HOR_RES_MAX, LV_VER_RES_MAX);
    // ESP_LOGI(TAG, "Display buffer size: %d", DISP_BUF_SIZE);

    ESP_LOGI(TAG, "Initializing shared SPI master");
    gpio_num_t miso;
    gpio_num_t mosi;
    gpio_num_t sclk;

    gpio_get_spi_bus_lcd(&miso, &mosi, &sclk);
    lvgl_spi_driver_init(LCD_HOST,
                         miso, mosi, sclk,
                             max_transfer_size, 1,
                         -1, -1);

    disp_spi_add_device(LCD_HOST);
    tp_spi_add_device(LCD_HOST);

    disp_driver_init();
    touch_driver_init();

    return;
/* ****** */
}

lvgl\src\lv_hal\lv_hal_disp.c

void lv_disp_drv_init(lv_disp_drv_t * driver)
{
    _lv_memset_00(driver, sizeof(lv_disp_drv_t));

    driver->flush_cb         = NULL;
    // driver->hor_res          = LV_HOR_RES_MAX; // It makes no sense to initialize
    // driver->ver_res          = LV_VER_RES_MAX; // It makes no sense to initialize
    driver->buffer           = NULL;
    driver->rotated          = LV_DISP_ROT_NONE;
    driver->sw_rotate        = 0;
    driver->color_chroma_key = LV_COLOR_TRANSP;
    driver->dpi = LV_DPI;
/* ****** */
}

I showed the code that I will use in my project, as food for thought, an idea, but not as a requirement to remake the library in this way.

@stale
Copy link

stale bot commented Jun 11, 2021

This issue or pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale This will not be worked on label Jun 11, 2021
@SinglWolf
Copy link
Author

This issue or pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

I'm waiting for the developer's answer.

@stale stale bot removed the stale This will not be worked on label Jun 11, 2021
@tore-espressif
Copy link
Collaborator

Hi,
I'm sorry, based on your last comment I figured out that you got it working the way you need it to.

So back to you original question:

Why is there no way to select a display driver at runtime?

Change like this requires huge refactoring as multidisplay setups were not taken into account when designing these drivers. There is refactoring planned, but it will take ~months.
Feedback, such as yours, is very valuable for us and enables us to design the new drivers in more flexible and (foremost) maintainable way.

When choosing a DARK theme, the LIGHT theme always remains.

This is a topic for lvgl/lvgl repo.

Initially I used the ILI9341 driver and the LV_COLOR_16_SWAP option was selected in the menuconfig.

Some displays swap these bytes in their graphical interface. We are lucky enough to have this handled by LVGL. Since LVGL does not know which display controller are you going to use, you have to specify this option at compile time.
It is technically possible to make it runtime configurable, and we will add it our refactoring plans.

I spent a lot of time and effort to understand the problem of disgusting color on the display.

You could have saved 'a lot of time and effort' by reading the README, which I'd like to encourage you to read in every open-source project.

Thanks for those points, we will definitely take them into account.
Let me know if there is anything else I can help you with.

@SinglWolf
Copy link
Author

I'll check draw_label issue tomorrow

I never got an answer.

I am also sincerely convinced that connecting the touchscreen controller to a separate SPI bus is only suitable for demonstration purposes. For example, the ESP32-S2 only has 2 SPI buses and the VS1053 module can no longer be connected.

@tore-espressif
Copy link
Collaborator

Checked on Wrover-kit with following setup:

LV_HOR_RES_MAX = 100
LV_VER_RES_MAX = 100
driver.hor_res = 320
driver.ver_res = 240
label with size > 200

Everything works as expected.

VS1053 module can no longer be connected.

Off topic.

Closing this issue.

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

No branches or pull requests

2 participants