pimoroni / pimoroni-pico

Libraries and examples to support Pimoroni Pico add-ons in C++ and MicroPython.
https://shop.pimoroni.com/collections/pico
MIT License
1.28k stars 490 forks source link

Remove endian byte swap in RGB565 colour handling #468

Open lowfatcode opened 2 years ago

lowfatcode commented 2 years ago

It looks like at some point in the past an issue (perhaps during writing image data to one of our LCD products?) was fixed by byte swapping our basic RGB565 type.

For example here: https://github.com/pimoroni/pimoroni-pico/blob/8d736324d4c6670f196a31ee7363e63cf107905b/libraries/pico_graphics/pico_graphics.hpp#L81

The issue should be fixed at the specific driver level as byte swapping our internal representation just leads to confusion when trying to directly manipulate the framebuffer contents.

I'm not sure which driver was causing the issue hence posting this here instead of just fixing it. If you can point me in the right direction I'll take a look!

Gadgetoid commented 2 years ago

Both ST7789 and ST7735 use byte-swapped RGB565 IIRC. Pretty sure this can now be handled by our scanline conversion setup without too much trouble.

This wart predates PicoGraphics, from simpler times when we just stored the entire screen buffer in native format only and sent it in one SPI transaction.

Gadgetoid commented 2 years ago

I think we'd need to differentiate endianess so that displays can request in their specific, supported byte-order.

At the moment RGB565 is treated as display-native, so the ST7789 driver does this:

    if(graphics->pen_type == PicoGraphics::PEN_RGB565) { // Display buffer is screen native
      command(cmd, width * height * sizeof(uint16_t), (const char*)graphics->frame_buffer);

It should, perhaps, be something like:

    if(graphics->pen_type == PicoGraphics::PEN_RGB565 && graphics->byte_order == this->byte_order) { // Display buffer is screen native
      command(cmd, width * height * sizeof(uint16_t), (const char*)graphics->frame_buffer);

Basically need a way to know if Pico Graphics and the display driver disagree about endianness without simply having a PEN_RGB565_BE and PEN_RGB565_LE type, which would feel somewhat redundant.

Scanline conversion could also accept an endianess argument:

graphics->scanline_convert(PicoGraphics::PEN_RGB565, PicoGraphics::BYTE_ORDER_LE, [this](void *data, size_t length) {
        spi_write_blocking(spi, (const uint8_t*)data, length);
});
Gadgetoid commented 1 year ago

Scan-line conversion was changed to per-pixel "frame_convert" into an arbitrary sized buffer as of - https://github.com/pimoroni/pimoroni-pico/pull/422

This shouldn't meaningfully affect any byte swapping behavior.