Support ili9488 16bit parallel display attached via SPI about circuitpython

[jepler]

There seems to be some kind of support for 16 bit coordinates, it may be called "ram width" / "ram height". This is in bus_core.c:

    if (self->ram_width < 0x100) {
        data[data_length++] = x1;
        data[data_length++] = x2;
    } else {
        if (self->address_little_endian) {
            x1 = __builtin_bswap16(x1);
            x2 = __builtin_bswap16(x2);
        }
        data[data_length++] = x1 >> 8;
        data[data_length++] = x1 & 0xff;
        data[data_length++] = x2 >> 8;
        data[data_length++] = x2 & 0xff;
    }

or "single byte bounds"

    uint16_t ram_width = 0x100;
    uint16_t ram_height = 0x100;
    if (single_byte_bounds) {
        ram_width = 0xff;
        ram_height = 0xff;
    }

[bablokb]

I initially also thought that would be the way to go, but it is not. The C-driver code of Waveshare does:

        //set the X coordinates
        LCD_WriteReg(0x2A);
        LCD_WriteData(Xstart >> 8);             //Set the horizontal starting point to the high octet
        LCD_WriteData(Xstart & 0xff);           //Set the horizontal starting point to the low octet
        LCD_WriteData((Xend - 1) >> 8);         //Set the horizontal end to the high octet
        LCD_WriteData((Xend - 1) & 0xff);       //Set the horizontal end to the low octet

And LCD_WriteData() does:

        DEV_Digital_Write(LCD_DC_PIN,1);
        DEV_Digital_Write(LCD_CS_PIN,0);
        SPI4W_Write_Byte(Data >> 8);
        SPI4W_Write_Byte(Data & 0XFF);
        DEV_Digital_Write(LCD_CS_PIN,1);

This code sends every byte of the coordinates as 16-bit down the line.

[jepler]

I see -- so to send the coordinate 479 (= 0x1df) it is necessary to send 0x00 0x01 0x00 0xdf, with CS changes at various times? 🤯

[bablokb]

Yes. The two SPI4W_Write_Byte calls push the 2 bytes to the shift register which sends the 16-bit word to the chip. The actual display data is not mangled in this way.

[bablokb]

This is really an obscure piece of hardware. Instead of directly attaching the SPI-interface to the ILI9488, they put 4 logic ICs in between that translate from SPI to 16-bit parallel.

The data is clocked out to the chip

• on a rising edge of CS
• every 16 bit

Commands and command-parameters are 8-bit, so it is necessary to either toggle CS after each command/command-parameter or send commands and command-parameters with an additional 0x00 fill-byte. Normal display data is 16-bit, so no CS-toggling in between is possible.

I made this work by hacking some lines in displayio/bus_core.c (mainly sending the coordinates as discussed above). But this is more of a POC than a solution. The real solution would need an additional parameter to BusDisplay (or FourWire) that allows to distinguish this hardware from standard SPI-displays.

The main question is: does it make sense to support bad hardware design?!

[jepler]

I worry about the flash size cost for this new mode/flag on all the boards that would never use it. If a 2nd manufacturer used this scheme for some reason, or if there was a reason it was "sensible", I'd be happy to reconsider that position.

[bablokb]

That is exactly what I think. So I just document what I have done and close this issue:

diff --git a/shared-module/displayio/bus_core.c b/shared-module/displayio/bus_core.c
index 301606c1f3..0b434ccd05 100644
--- a/shared-module/displayio/bus_core.c
+++ b/shared-module/displayio/bus_core.c
@@ -145,12 +145,12 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
 
     // Set column.
     displayio_display_bus_begin_transaction(self);
-    uint8_t data[5];
+    uint8_t data[9];
     data[0] = self->column_command;
     uint8_t data_length = 1;
     display_byte_type_t data_type = DISPLAY_DATA;
     if (!self->data_as_commands) {
-        self->send(self->bus, DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, data, 1);
+        self->send(self->bus, DISPLAY_COMMAND, CHIP_SELECT_TOGGLE_EVERY_BYTE, data, 1);
         data_length = 0;
     } else {
         data_type = DISPLAY_COMMAND;
@@ -164,9 +164,13 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
             x1 = __builtin_bswap16(x1);
             x2 = __builtin_bswap16(x2);
         }
+        data[data_length++] = 0;
         data[data_length++] = x1 >> 8;
+        data[data_length++] = 0;
         data[data_length++] = x1 & 0xff;
+        data[data_length++] = 0;
         data[data_length++] = x2 >> 8;
+        data[data_length++] = 0;
         data[data_length++] = x2 & 0xff;
     }
 
@@ -196,7 +200,7 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
     data[0] = self->row_command;
     data_length = 1;
     if (!self->data_as_commands) {
-        self->send(self->bus, DISPLAY_COMMAND, CHIP_SELECT_UNTOUCHED, data, 1);
+        self->send(self->bus, DISPLAY_COMMAND, CHIP_SELECT_TOGGLE_EVERY_BYTE, data, 1);
         data_length = 0;
     }
 
@@ -208,9 +212,13 @@ void displayio_display_bus_set_region_to_update(displayio_display_bus_t *self, d
             y1 = __builtin_bswap16(y1);
             y2 = __builtin_bswap16(y2);
         }
+        data[data_length++] = 0;
         data[data_length++] = y1 >> 8;
+        data[data_length++] = 0;
         data[data_length++] = y1 & 0xff;
+        data[data_length++] = 0;
         data[data_length++] = y2 >> 8;
+        data[data_length++] = 0;
         data[data_length++] = y2 & 0xff;
     }