bsp: Fix display headers for use in C++ and update BSP constants

[espzav]

Fixed display and touch headers for using with C++ and updated BSP header with LCD constants.

Closes #143, #144, #145

[espzav]

@TTauriainenQt please, is it enough for your Qt portation? As you mention, you need to know the target format i.e. RGB565 / RGB888 / ARGB8888, we haven't got this in our BSPs. All LCDs are set to RGB565.

[TT-Qt]

Hi @espzav. BSP_LCD_COLOR_SPACE is not usable as per my understanding that control bit makes display driver IC convert RGB into BGR for the LCD panel. This is invisible operation for us. ( https://cdn-shop.adafruit.com/datasheets/ILI9341.pdf page 127 ).

We are interested about the MCU->LCD driver IC interface format so for example:

Pixelformat of the display interface:

typedef enum {
    LCD_IF_PIXELFORMAT_RGB565,
    ...
} lcd_interface_pixelformat_t;

Display IC interface endianess ( see https://cdn-shop.adafruit.com/datasheets/ILI9341.pdf page 192 )

typedef enum {
    LCD_IF_ENDIANESS_BE, ( BE seems to be used on Korvo-2 and ESP-BOX )
    LCD_IF_ENDIANESS_LE
} lcd_interface_endianess_t;

Or combined

typedef enum {
    LCD_IF_PIXELFORMAT_RGB565_LE,
    LCD_IF_PIXELFORMAT_RGB565_BE,
    ...
} lcd_interface_pixelformat_t;

And then defines or functions that tell proper values for the BSP user.

RGB itself does not tell us enough about the pixel format as it can be for example RGB332, RGB565, RGB888...

[espzav]

Hi @TTauriainenQt, Thank you for your clarification. But if I go to ILI9341 driver implementation (https://github.com/espressif/esp-bsp/blob/master/components/lcd/esp_lcd_ili9341/esp_lcd_ili9341.c) I see, that the information bits_per_pixel is strictly for color format (line 73).

I think that you need the BSP_LCD_COLOR_SPACE, because we set it in the driver too (line 60). This is information about input color data.

It seems that we haven't got any API for setting endianness in the driver. A didn't find any info in the driver about set endianness. We are using default. This is the reason why we cannot provide this information to you from BSP.

[TT-Qt]

Hi @espzav,

There is no need for additional API to set endianess from our side. We just need to know the endianess per board and Board Support Package would be logical place to store such information as it contains board specific information.

Like earlier mentioned ESP_LCD_COLOR_SPACE_RGB or ESP_LCD_COLOR_SPACE_BGR info is useless for us as it does not specify the actual pixel format. Also pixel format would be something that you could define in BSP by yourself for the BSP users as ESP-IDF does not have such info.

[tore-espressif]

@TTauriainenQt That is a good point, for most of our examples we solve it in LVGL that can swap the upper and lower bytes (the config is here https://github.com/espressif/esp-bsp/blob/master/examples/display/sdkconfig.defaults#L6).

Some display controllers provide an option to accept both low- and high-endian data, but there are some restrictions. Typically that it does not work for all interfaces (SPI, parallel) or for all color formats. That is why we use the default endianness and swap the bytes manually, if needed.

We will add definitions of display endianess to display.h

[TT-Qt]

We will add definitions of display endianess to display.h

Sounds good. Actually the definitions are also better for us as we can then do compile time check instead of runtime check.

[espzav]

Hello @TTauriainenQt and @tore-espressif I updated the header files. Moved to display.h and added new constants. The BSP_LCD_COLOR_FORMAT taking one of these values:

/* LCD color formats */
#define ESP_LCD_COLOR_FORMAT_RGB565    (1)
#define ESP_LCD_COLOR_FORMAT_RGB888    (2)

I think, we should move these color formats definition to esp_lcd in future as a enum.

The endian will be used in LVGL port in future for set the color swap in runtime instead of CONFIG_LV_COLOR_16_SWAP=y (it is not released in LVGL yet)

[TT-Qt]

Hi @espzav,

I tried your changes locally in my project. I included display.h and touch.h + modified my code to use these new defines.
No issues found so far.

Thanks! For me this PR looks good.

[tore-espressif]

Thank you @espzav ! Only one minor comment, otherwise LGTM!

[TT-Qt]

Sorry I did not notice during the review that you also reduced the drawing buffer sizes. This adds more overhead for drawing and lowers the overall performance. Was there a specific reason for this change?

[espzav]

@TTauriainenQt I didn't change the size of drawing buffer. I only move all constants to header. Now, there is #define BSP_LCD_DRAW_BUFF_SIZE (BSP_LCD_H_RES * 50) and before the BSP_LCD_H_RES * 50 was used as a size of buffer in C file. The buffer can be higher, but the ESP32S3 have too small RAM and if you increase the buffer, it is working, but there isn't too much RAM for other things.

@tore-espressif We should think about mechanism to set the size of buffer from custommer project? What about move it to KConfig?

[TT-Qt]

Sorry, to be precise it was the maximum transfer size for the SPI bus that decreased. This limits how much we can draw at once before flushing.

Example from ESP-BOX:

#define BSP_LCD_H_RES              (320)

Original code:

...
const spi_bus_config_t buscfg = {
    ...
    .max_transfer_sz = BSP_LCD_H_RES * 80 * sizeof(uint16_t),  // 320 * 80 * 2 = 51200 bytes
};

In new version:

#define BSP_LCD_DRAW_BUFF_SIZE     (BSP_LCD_H_RES * 10) = 320 * 10 = 3200 pixels

const spi_bus_config_t buscfg = {
    ...
    .max_transfer_sz = BSP_LCD_DRAW_BUFF_SIZE * sizeof(uint16_t), // 3200 * 2 = 6400 bytes
};

Can you return the original values or make BSP_LCD_DRAW_BUFF_SIZE settable with #ifndef flags or by Kconfig?