lvgl / lvgl_esp32_drivers

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

Communication interface abstraction #143

Open C47D opened 3 years ago

C47D commented 3 years ago

Hi,

As we've discussed before there's the desire to abstract the communication interface (SPI, I2C, Parallel, etc.) in the display drivers.

Current implementation

We only have support for SPI and I2C. Here's how currently (develop branch) a command and data are being sent over SPI (ili9341):

static inline void set_cmd_mode(lv_disp_drv_t * drv)
{
    display_port_gpio_dc(drv, 0);
}

static inline void set_data_mode(lv_disp_drv_t * drv)
{
    display_port_gpio_dc(drv, 1);
}

static void ili9341_send_cmd(lv_disp_drv_t * drv, uint8_t cmd)
{
    disp_wait_for_pending_transactions();
    set_cmd_mode(drv);
    disp_spi_send_data(&cmd, 1);
}

static void ili9341_send_data(lv_disp_drv_t *drv, void * data, uint16_t length)
{
    disp_wait_for_pending_transactions();
    set_data_mode(drv);
    disp_spi_send_data(data, length);
}

static void ili9341_send_color(lv_disp_drv_t *drv, void * data, uint16_t length)
{
    disp_wait_for_pending_transactions();
    set_data_mode(drv);
    disp_spi_send_colors(data, length);
}

Here's how they're sent over I2C:

static uint8_t send_data(lv_disp_drv_t *disp_drv, void *bytes, size_t bytes_len)
{
    (void) disp_drv;

    uint8_t *data = (uint8_t *) bytes;

    return lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, data[0], data + 1, bytes_len - 1 );
}

static uint8_t send_pixels(lv_disp_drv_t *disp_drv, void *color_buffer, size_t buffer_len)
{
    (void) disp_drv;

    return lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, OLED_CONTROL_BYTE_DATA_STREAM, color_buffer, buffer_len);
}

In the SPI case we have two functions to send data: disp_spi_send_data and disp_spi_send_colors, defined in disp_spi.h:

static inline void disp_spi_send_data(uint8_t *data, size_t length) {
    disp_spi_transaction(data, length, DISP_SPI_SEND_POLLING, NULL, 0, 0);
}

static inline void disp_spi_send_colors(uint8_t *data, size_t length) {
    disp_spi_transaction(data, length,
        DISP_SPI_SEND_QUEUED | DISP_SPI_SIGNAL_FLUSH,
        NULL, 0, 0);
}

The difference between them is that the latest will be sent using DMA, and when the transmission is done an interrupt is generated and we call lv_disp_flush_ready.

What we need?

Communication interface shouldn't matter to the display driver, it only cares to send data in polling or async mode (and knowing when it's done).

Proposal

We could initiate by adding some functions to the lv_port API (names are long, but descriptive, suggestions are very welcomed)

Here's the proposed implementation, I think it's a good first step (arguments and names to be improved):

void display_send_data(lv_disp_drv_t *drv, void * data, uint16_t length)
{
#if defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI
    set_data_mode(drv);    
    disp_spi_send_data(drv, data, length);
#elif defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_I2C
    lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, OLED_CONTROL_BYTE_DATA_STREAM, data, length);
#endif
}

void display_send_data_async(lv_disp_drv_t *drv, void * data, uint16_t length)
{
#if defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI
    set_data_mode(drv);    
    disp_spi_send_colors(drv, data, length);
#elif defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_I2C
    /* TBD */
#endif
}

void display_send_cmd(lv_disp_drv_t *drv, uint8_t cmd, void * args, uint16_t args_len)
{
#if defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI
    set_cmd_mode(drv);    
    disp_spi_send_data(drv, cmd, args, args_len);
#elif defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_I2C
    lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, cmd, args, args_len);
#endif
}

What do you think @kisvegabor @tore-espressif

kisvegabor commented 3 years ago

Hi,

I like the idea in general, but I'd split the API a little bit differently.

We can de two things with a display:

Therefore I suggest something like this:

void display_send_cmd(lv_disp_drv_t *drv, uint8_t cmd, void * args, uint16_t args_len);

//It also calls `lv_disp_flush_ready()` as it wants. 
void display_update_image(lv_disp_drv_t *drv, const lv_area_t * area, lv_color_t * image);

display_update_image is quite abstract therefore it can work with any kind of display refreshing. (display controller, double buffer, single buffer, etc)

What do you think?

C47D commented 3 years ago

display_update_image is quite abstract therefore it can work with any kind of display refreshing. (display controller, double buffer, single buffer, etc)

It seems like the flush functions will become a wrapper of it.


Here's a first approach I had laying around https://github.com/lvgl/lvgl_esp32_drivers/pull/144 :

kisvegabor commented 3 years ago

IMO we should not tell from the display controller that images (colors) are sent with async. What if a platform doesn't have support for async I2C sending. Or want to buffer commands too into a bundle.

As there are only 2 type of communication

  1. command sending
  2. image sending

we can let the user use how to handle them.

What do you think?

C47D commented 3 years ago

Then we would need to add a configuration switch to choose between async and polling image sending.

I will add that to the proposal.

tore-espressif commented 3 years ago

Few points here:

void display_send_cmd(lv_disp_drv_t drv, uint8_t cmd, void args, uint16_t args_len)

Some displays have 16bit wide commands.

if defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_SPI

set_data_mode(drv);
disp_spi_send_data(drv, data, length);

elif defined CONFIG_LV_TFT_DISPLAY_PROTOCOL_I2C

lvgl_i2c_write(OLED_I2C_PORT, OLED_I2C_ADDRESS, OLED_CONTROL_BYTE_DATA_STREAM, data, length);

endif

I thought that the main idea was to get rid of these #ifdefs , what if the user has multidisplay setup and wants to use both i2c and spi? There should be a way of dynamically assigning communication interface to a display driver.

kisvegabor commented 3 years ago

Then we would need to add a configuration switch to choose between async and polling image sending.

Yes, but it's related only to the given platform.

Some displays have 16bit wide commands.

I think, uint32_t cmd should be enough.

There should be a way of dynamically assigning communication interface to a display driver.

If we consider the examples folder as platform examples (e.g. ESP SPI, ESP LCD) we can show a couple of things, and the user can copy/paste an example and extend it according to his special needs. What do you think?

C47D commented 3 years ago

I thought that the main idea was to get rid of these #ifdefs , what if the user has multidisplay setup and wants to use both i2c and spi? There should be a way of dynamically assigning communication interface to a display driver.

As a first step I'm removing the spi or i2c function calls from the drivers, those calls would now be in the port functions implementation. For the ability to switch communication interfaces at runtime I think we could use some function pointers?

Yes, but it's related only to the given platform.

Maybe we could add a parameter flag in the send_image function and let the implementer/user use it as convenient.

I think, uint32_t cmd should be enough.

I think we would need to add a configuration parameter to know how big (in bytes) the command is, for now we only use the SPI in 8bit mode.

What do you think?

C47D commented 3 years ago

How does https://github.com/lvgl/lvgl_esp32_drivers/pull/144/commits/8a065b32b66b0e6f17b2c079609937a1a317af4b look?