search

atanisoft / esp_lcd_ili9488

esp_lcd compatible driver interface for ILI9488 displays
MIT License
18 stars 8 forks source link

Code review #2

Closed espzav closed 2 years ago

espzav commented 2 years ago

Hello, I checked this component and tested it with Intel 8080 Parallel 16bit interface. It is working but I have some notes for code:

  1. Why do you use malloc and color data recalculation in panel_ili9488_draw_bitmap? It is wasting of memory, it is working properly, when changed init to { LCD_CMD_COLMOD, { ILI9488_COLOR_MODE_16BIT }, 1 } and put whole color data esp_lcd_panel_io_tx_color(io, LCD_CMD_RAMWR, color_data, color_data_len * 2);
  2. Use this ili9488->bits_per_pixel / 8 instead of the constant color size. You can initialize right by this and do not need allocate another buffer (again).

I didn't checked all code line by line but these things are important to change. Please, could you fix it. Or should I make PR? If you have any questions, feel free to contact me.

tore-espressif commented 2 years ago

@espzav the 16 bit color mode (RGB565) does not work with SPI :( You are using parallel interface, so you don't face the issue. So I guess that's the reason.

Here's my past attempt https://github.com/lvgl/lvgl_esp32_drivers/pull/103

espzav commented 2 years ago

@espzav the 16 bit color mode (RGB565) does not work with SPI :( so I guess that's the reason.

Here's my past attempt lvgl/lvgl_esp32_drivers#103

Oh, thank you for clarification. Then, there should be any condition for using with parallel without wasting memory.

atanisoft commented 2 years ago

2. Use this ili9488->bits_per_pixel / 8 instead of the constant color size.

This value does not exist in the current code and is not necessary since the size is known up-front based on color mode (16-bit or 18-bit).

  1. Why do you use malloc and color data recalculation in panel_ili9488_draw_bitmap?

It may be a waste but it is the only way to support the SPI interface mode.

atanisoft commented 2 years ago

@tore-espressif Please pull the latest master and give it a test as I've addressed your feedback and it should work correctly now for both with no need to modify the ili9488 code.

After you've tested I'll tag v1.0.5

espzav commented 2 years ago

@atanisoft I saw the readme, please, could we add "Intel 8080" into the Communication interface? I will test the changes on parallel LCD, tomorrow.

atanisoft commented 2 years ago

@espzav I've added that and a note on the color depth mode requirements for SPI vs Intel 8080 on master.

espzav commented 2 years ago

@atanisoft Thank you very much!

espzav commented 2 years ago

@atanisoft I tested parallel interface on HW now. It is working properly. You can upload it into Component manager as version 1.0.5. Thank you for changes!

atanisoft commented 2 years ago

Thanks @espzav for testing on HW. Do you have any recommendations on HW that supports the parallel IO modes? I'm currently looking at a few bare displays using a 40-pin FPC and using an adapter to convert to breadboard while developing a possible hardware product.

espzav commented 2 years ago

@atanisoft Do you think recomendation with ILI9488 controller or any parallel controller? It depends on size and application. All supported and tested LCD displays are here: https://github.com/espressif/esp-bsp/blob/master/LCD.md Parallel are ESP32-S2-HMI-DevKit-1 and Waveshare 7" LCD. Both are 800x480, the physical size is different.