Code review

[espzav]

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]

@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 lvgl/lvgl_esp32_drivers#103

[espzav]

@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]

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]

@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]

@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]

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

[espzav]

@atanisoft Thank you very much!

[espzav]

@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]

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]

@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.