Incorrect pixel data being displayed

[fivdi]

I'm currently trying out the esp_lcd_ili9488 driver and it's going quite well. What I have noticed is that sometimes incorrect pixel data is being displayed. If I try out the lvgl example, there are 10 incorrect pixels displayed down the right hand side of the display. This can be seen in the following photo:

Similar issues can be seen when the display is being updated during the animation when the pointer on the meter moves from 0 to 100 and back to 0. In the following photo, the the pointer on the meter has already moved from 0 to about 10 and there are a few invalid pixels being displayed to the left of the 100 marking on the meter:

By the time the pointer on the meter has moved to about 50, there are more invalid pixels being displayed to the left of the 100 marking on the meter:

By the time the pointer on the meter has moved to about 100, the invalid pixels that were displayed to the left of the 100 marking on the meter are no longer there, but there are new invalid pixels being displayed to the right of the red part of the scale:

Each time the driver sends a block of data to the display, it looks like the data for the last pixel in the block isn't always being sent correctly. What I think is that there's +/- 1 problem somewhere in the code. I have spent some time looking at the code but I can't figure out what the problem is. The results are the same with esp-idf v4.4 and v5.0.

Do you know how to fix this issue?

[atanisoft]

I was able to reproduce this but I'm not certain on the cause. I think you may be correct with the +/- 1 pixel issue in the data window being pushed but I also have not spotted it yet. It could be in a lower level (esp_lcd).

@espzav @tore-espressif any ideas?

[fivdi]

I spent some more time looking into this and think I have figured out what the problem is.

In function panel_ili9488_draw_bitmap, memory is allocated, filled with the appropriate color data, passed to esp_lcd_panel_io_tx_color for transmission to the display, and then the allocated memory is freed. This all occurs in this block of code.

However, esp_lcd_panel_io_tx_color transmits data to the display asynchronously via DMA in the background. esp_lcd_panel_io_tx_color does not wait for the transmission to complete before returning. On the other hand, the memory allocated for the color data is immediately freed after the call to esp_lcd_panel_io_tx_color. This means that the memory is being freed while it is still being used by DMA in the background. This in turn can lead to all sorts of problems with the pixel data being displayed.

A quick hack to confirm that the memory is being freed to early can be seen by replacing these two lines of code:

        esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, buf, color_data_len * 3);
        heap_caps_free(buf);

with these three lines of code which give the DMA some time to complete:

        esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, buf, color_data_len * 3);
        vTaskDelay(pdMS_TO_TICKS(20));
        heap_caps_free(buf);

esp_lcd_panel_io_tx_color is documented here.

[atanisoft]

I'm not sure if this is related to DMA or not, but it's something to consider. This is one of the odd things with the ILI9488 when using the four wire SPI interface, it only works correctly with 18-bit colors whereas everything else is using 16-bit.

[fivdi]

I'm not sure if this is related to DMA or not, but it's something to consider.

I'm more or less 100% convinced that the issue is being caused by the memory block being freed while it is still being used asynchronously in the background.

To help further convince you, consider the following: if it's ok to call heap_caps_free to free the block of memory as it is no longer being used, then it is also ok to clear the block of memory before it is freed. In other words, if we replace this code:

        esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, buf, color_data_len * 3);
        heap_caps_free(buf);

with this code:

        esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, buf, color_data_len * 3);
        for (int i = color_data_len * 3 - 1; i >= 0; --i) {
             buf[i] = 0;
        }
        heap_caps_free(buf);

then it should not influence what is displayed on the screen. However, if this modification is made to the code, the following is displayed:

Please note that I'm not suggesting calling vTaskDelay(pdMS_TO_TICKS(20)) to resolve the issue as this is certainly not an appropriate solution.

The hack that I'm currently using to resolve the issue is to replace this code with this code:

        static uint8_t buf[320 * 50 * 3];

        uint16_t *raw_color_data = (uint16_t *) color_data;
        for (uint32_t i = 0, pixel_index = 0; i < color_data_len; i++) {
            buf[pixel_index++] = (uint8_t) (((raw_color_data[i] & 0xF800) >> 8) |
                                            ((raw_color_data[i] & 0x8000) >> 13));
            buf[pixel_index++] = (uint8_t) ((raw_color_data[i] & 0x07E0) >> 3);
            buf[pixel_index++] = (uint8_t) (((raw_color_data[i] & 0x001F) << 3) |
                                            ((raw_color_data[i] & 0x0010) >> 2));
        }

        esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, buf, color_data_len * 3);

This is one of the odd things with the ILI9488 when using the four wire SPI interface, it only works correctly with 18-bit colors whereas everything else is using 16-bit.

Yes, this is odd and unfortunate.

[atanisoft]

However, if this modification is made to the code, the following is displayed:

Yeah, that does confirm that there is async memory usage going on for sure. The question is how to avoid this as the allocation / free is necessary due to the 16->18bit conversion.

Using a pre-allocated buffer matching the LVGL buffer size should work nicely but should be allocated in the init method rather than embed in the panel_ili9488_draw_bitmap method.

Another option to explore is using esp_lcd_panel_io_handle_t -> register_event_callbacks to hook into the TX complete event. That could be used then to free a buffer allocated inside panel_ili9488_draw_bitmap possibly.

[igndrag]

My experience:

1. Add buffer_size parameter to li9488_panel_t struct

typedef struct { esp_lcd_panel_t base; esp_lcd_panel_io_handle_t io; int reset_gpio_num; bool reset_level; int x_gap; int y_gap; uint8_t memory_access_control; uint8_t color_mode; size_t buffer_size; } ili9488_panel_t;

2. Create static uint8_t *test_buf

3. Fix function esp_err_t esp_lcd_new_panel_ili9488( const esp_lcd_panel_io_handle_t io, const esp_lcd_panel_dev_config_t panel_dev_config, esp_lcd_panel_handle_t ret_panel, size_t buffer_size) { ... ili9488->buffer_size = buffer_size; ... }

4. Fix function

static esp_err_t panel_ili9488_init(esp_lcd_panel_t panel) { ... test_buf = (uint8_t ) heap_caps_malloc(ili9488->buffer_size * 3 + 1, MALLOC_CAP_DMA); ... }

5. Fix function static esp_err_t panel_ili9488_draw_bitmap(esp_lcd_panel_t panel, int x_start, int y_start, int x_end, int y_end, const void color_data) { ... uint16_t raw_color_data = (uint16_t ) color_data; for (uint32_t i = 0, pixel_index = 0; i < color_data_len; i++) { test_buf[pixel_index++] = (uint8_t) (((raw_color_data[i] & 0xF800) >> 8) | ((raw_color_data[i] & 0x8000) >> 13)); test_buf[pixel_index++] = (uint8_t) ((raw_color_data[i] & 0x07E0) >> 3); test_buf[pixel_index++] = (uint8_t) (((raw_color_data[i] & 0x001F) << 3) | ((raw_color_data[i] & 0x0010) >> 2)); } esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, test_buf, color_data_len * 3); }

6. In LVGL ui init use ESP_ERROR_CHECK(esp_lcd_new_panel_ili9488(lcd_io_handle, &lcd_config, &lcd_handle, LV_BUFFER_SIZE));

All works great

[atanisoft]

@igndrag This is similar to what I was thinking, pre-allocate the buffer during init but I wasn't certain on the size. Having it provided as an input will certainly help. It will only be needed for the 18-bit color when using four wire SPI.

Let me implement this strategy and do some testing.

[atanisoft]

@igndrag @fivdi If you can test the changes in the PR that would be appreciated!

[fivdi]

@atanisoft thank you for the fix.

The issue has been resolved for me with the dma_buf branch. I tested the example with esp-idf v5.0 and can no longer see incorrect pixels.

For esp-idf v5.0, I had to add esp_timer to the REQUIRES here. #include <esp_timer.h> also had to be added to main.c.

[igndrag]

Checked. Looks great. Thanks for fast update!