Closed bablokb closed 6 months ago
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;
}
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.
I see -- so to send the coordinate 479 (= 0x1df
) it is necessary to send 0x00 0x01 0x00 0xdf
, with CS changes at various times? :exploding_head:
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.
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
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?!
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.
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;
}
I tried to create a CircuitPython driver for this display: https://www.waveshare.com/wiki/Pico-ResTouch-LCD-3.5
This is a 480x320 display with an ILI9488 driver-chip. The display uses a 16-bit parallel interface and the SPI of the Pico is connected via two chained 8-bit SIPO shift registers. This converts 2x 8-bit SPI to 16-bit parallel.
The problem: this does not work with the current displayio implementation, since the interface always needs 16-bit sized data (8-bits for commands is fine). For the init-sequence I have a workaround because I can add the fill-bytes manually. The display data should also be ok, since it is 16-bit (5-6-5) anyway. But for setting rows and columns I also need to send 16-bit coordinates.
My idea is to add a parameter (something like
send_data_as_16bit
) and change the implementation accordingly. Do you think this is feasible? Would you accept such a PR? Any suggestions regarding on how to best implement this?