Add a ReadWriteInterface for displays.

[chrismoos]

This change adds a ReadWriteInterface for displays that have both read/write capabilities. The interface is actually much different than the current one, so let me know what you think. This has been implemented in an ILI9486 driver I'm working on here: https://github.com/chrismoos/ili9486-driver/blob/master/src/gpio/gpio8.rs

Highlights for the change are:

• The DataFormat moves from an enum (useable at runtime), to the implementations. In practice, when you wire up an implementation, from compile time, you'll have a fixed mode (GPIO8/16, SPI, etc,.) so this means implementors can focus on the actual formats they support, rather than runtime handling of the enum.
• Read support via read_stream - this allows for efficient transfer (allowing chip to stay selected), invoking a closure until the caller is done reading, which it signals to the interface with a bool return in the closure.
• Write support via write_iter - similar to read_stream, this supports efficient transfer (pipelining) of a stream of bytes to the device.
• An Iterator is provided automatically for ReadWriteInterface, allowing you to easily iterate over the interface to read bytes.
• Implementors of ReadWriteInterface must only implement the above 2 methods, the trait provides some default "helper" methods to read into a fixed buffer, or write from a fixed buffer.

PS @therealprof I saw SSD1306 does have read support, also, so not sure which ones would be "write only". Regardless, this could be combined (the RW) into a super trait if you think that makes the most sense.

[therealprof]

Thanks for the PR. My few thoughts from looking over this:

This has been implemented in an ILI9486 driver I'm working on here: https://github.com/chrismoos/ili9486-driver/blob/master/src/gpio/gpio8.rs

It's not ideal that the display-interface is intertwined with the actual display driver since it obscures the important bits about the abstraction layer.

The DataFormat moves from an enum (useable at runtime), to the implementations. In practice, when you wire up an implementation, from compile time, you'll have a fixed mode (GPIO8/16, SPI, etc,.) so this means implementors can focus on the actual formats they support, rather than runtime handling of the enum.

The enum handling is pretty much free. The problem here is that you don't know how applications are actually connecting the display. Some offer many different interfaces, even with different data widths so ideally a display-interface should implement all of the variants which make sense for a particular hardware connection so the display driver can choose the correct data format and the display-interface driver can format it properly for the channel, e.g. when sending 16bit data over an 16bit channel you might need to do endian conversion but only if the display endianess differs from the CPU/MCU endianess, over an 8bit channel you don't. Also some hardware drivers might work great with iterators, for some others you can only work with slices and might need to buffer the stream. Also some displays do used mixed operations, e.g. 16bit pixel data with 8bit commands...

PS @therealprof I saw SSD1306 does have read support, also, so not sure which ones would be "write only". Regardless, this could be combined (the RW) into a super trait if you think that makes the most sense.

That is only possible in parallel mode which has never reported to exist in the real world (and is also not supported by the driver). The I2C and SPI implementations are write-only:

To write data to the GDDRAM, select LOW for the R/W# (WR#) pin and HIGH for the D/C# pin for both
6800-series parallel mode and 8080-series parallel mode. The serial interface mode is always in write mode.

[chrismoos]

@therealprof Thanks for reviewing.

It's not ideal that the display-interface is intertwined with the actual display driver since it obscures the important bits about the abstraction layer.

Can you elaborate on this? Are there some changes in this PR that you feel intertwine the actual driver with the abstraction layer? The read and write methods seem generic enough, and provide flexibility via the DataFormat generic. Also, any default trait methods can be overridden if there are performance concerns.

The problem here is that you don't know how applications are actually connecting the display. Some offer many different interfaces, even with different data widths so ideally a display-interface should implement all of the variants which make sense for a particular hardware connection so the display driver can choose the correct data format and the display-interface driver can format it properly for the channel, e.g. when sending 16bit data over an 16bit channel you might need to do endian conversion but only if the display endianess differs from the CPU/MCU endianess, over an 8bit channel you don't.

Right, so by adding the generic type DataFormat to ReadWriteInterface it still provides that flexibility. The difference is, when someone goes to actually write a display-interface driver, they will then choose which DataFormat types to support. In addition, regarding the pixel format, this flexibility is also achieved. For example, an implementation of ReadWriteInterface<u8> can implement this as applicable for that platform. I do something similar here, which lets an 8-bit parallel interface output the proper pixel format. In the 16-bit parallel interface version, it's different.

That is only possible in parallel mode which has never reported to exist in the real world (and is also not supported by the driver). The I2C and SPI implementations are write-only

Thanks for that info.

[therealprof]

Are there some changes in this PR that you feel intertwine the actual driver with the abstraction layer?

No, this PR is clean but by having the actual display-interface adapter implemented in your display driver you're bypassing the layering of this concept, making it hard to verify that the proposed interface works as intended in a typical setting. This is certainly not helped by implementations like https://github.com/chrismoos/ili9486-driver/blob/bebebdbc83ffc240517ad3a4584fc0e627b7b315/src/gpio/gpio8.rs#L99 which should not really be there (according to the concept).

Right, so by adding the generic type DataFormat to ReadWriteInterface it still provides that flexibility. The difference is, when someone goes to actually write a display-interface driver, they will then choose which DataFormat types to support.

The do get to choose the ones they want to support right now, but if they want to provide the most flexibility they realistically have to implement most of them. You don't get the benefit of working in a lot of different situations if you don't. The necessity of including multiple data types and also endianess is something we learned the "hard" way. Only the display driver knows which format and endianess the display chip expects but only the display-interface driver knows how to actually deliver the data in that format.

In addition, regarding the pixel format, this flexibility is also achieved. For example, an implementation of ReadWriteInterface can implement this as applicable for that platform. I do something similar here, which lets an 8-bit parallel interface output the proper pixel format. In the 16-bit parallel interface version, it's different.

I have not checked what kind of magic your display driver does to make that happen but believe me it's not that simple.

There're at least two more display drivers besides ssd1306 which already use this interface, cf https://github.com/yuri91/ili9341-rs (not released yet) and https://crates.io/crates/st7789 . I've verified them in a lot of different settings and hardware configurations to do the right thing (I2C, SPI, 8bit and 16bit paralllel using GPIO, 16bit parallel using a MCU provided parallel bus, ...) and it took a lot of tweaking to make the interface flexible, universal, correct and very efficient (in fact all of the implementations have considerably improved their performance since the adoption of display-interface and I haven't even started adding any hardware specific optimisation yet).

[chrismoos]

No, this PR is clean but by having the actual display-interface adapter implemented in your display driver you're bypassing the layering of this concept, making it hard to verify that the proposed interface works as intended in a typical setting. This is certainly not helped by implementations like https://github.com/chrismoos/ili9486-driver/blob/bebebdbc83ffc240517ad3a4584fc0e627b7b315/src/gpio/gpio8.rs#L99 which should not really be there (according to the concept).

I 100% agree, it's best to have them implemented here so they can be consumed by multiple display drivers. The only reason I didn't include my implementation (gpio8 and gpio16), and why I didn't adapt the existing ones here, is because of my use of IoPin to provide switchable I/O. I didn't think it made sense to bring that trait in here, it's better tracked upstream in embedded-hal.

Only the display driver knows which format and endianess the display chip expects but only the display-interface driver knows how to actually deliver the data in that format.

In the current interface, the display-interface driver takes a DataFormat (U8, U16LE, U16BE, etc,.) and writes out the bytes. In a parallel interface, there is no discrepancy about which bit should go first (on the data frame/unit level), instead, the concern is in most cases, how to provide pixel data (i.e 18-bit pixel spread across two bytes, or 2 18-bit pixels spread across three bytes in a 16-bit parallel interface). As you said, the display driver is the one who knows what the active pixel format and they should be doing the conversion (whether its Rgb565, Rgb666, etc,.).

The driver should format the pixel(s) needed for the active pixel format and push them down to the display-interface driver via a write call. The caller (the actual driver) doesn't care if the ReadWriteInterface clocks out the data MSB (bit) or LSB, that is specific to the implementor of the ReadWriteInterface, i.e via parallel data lines it doesn't matter, or with SPI it does and you configure SPI to clock out MSB, before it even gets to the display-interface layer.

I have not checked what kind of magic your display driver does to make that happen but believe me it's not that simple.

No magic really :). I updated the code to be a bit more clearer as to how it works, see here.

impl<T> PixelWriter<u8> for T
where
    T: ReadWriteInterface<u8>,

The above lets us determine how to write pixel data for an 8-bit interface. Same can be done with a u16. Now, you aren't constrained either by using just primitives here. For example, by defining type U18 = u32; you can target color when in 18-bit parallel mode:

impl<T> PixelWriter<U18> for T
where
    T: ReadWriteInterface<U18>,

There're at least two more display drivers besides ssd1306 which already use this interface, cf https://github.com/yuri91/ili9341-rs (not released yet) and https://crates.io/crates/st7789 . I've verified them in a lot of different settings and hardware configurations to do the right thing (I2C, SPI, 8bit and 16bit paralllel using GPIO, 16bit parallel using a MCU provided parallel bus, ...) and it took a lot of tweaking to make the interface flexible, universal, correct and very efficient (in fact all of the implementations have considerably improved their performance since the adoption of display-interface and I haven't even started adding any hardware specific optimisation yet).

I reviewed the ili9341 one and, for example on this line,

.send_data(U16BEIter(&mut data.into_iter()))

The endianness specifier is pretty much useless, the driver really just want's to send some u16 words down to the interface, if it's an SPI implementation of WriteOnlyDataCommand, it will take care of clocking MSB out first (based on SPI configuration). If it's parallel, then it doesn't matter. Same thing with the other driver.

[therealprof]

I didn't think it made sense to bring that trait in here, it's better tracked upstream in embedded-hal.

Yeah, but I very much prefer a clean separation from the getgo. Maybe something like an established SPI trait would be a better way to do the proofing.

The driver should format the pixel(s) needed for the active pixel format and push them down to the display-interface driver via a write call. The caller (the actual driver) doesn't care if the ReadWriteInterface clocks out the data MSB (bit) or LSB, that is specific to the implementor of the ReadWriteInterface, i.e via parallel data lines it doesn't matter, or with SPI it does and you configure SPI to clock out MSB, before it even gets to the display-interface layer.

It does care. The display driver only knows in which format the display expects the format, it does not know which format the display-interface driver is pushing out the data to the display; that's why we have the different data types in the display-interface, the display driver pushes out the data in the expected format and it's up to the display-interface to make sure it ends up at the display in the expected format.

The endianness specifier is pretty much useless, the driver really just want's to send some u16 words down to the interface, if it's an SPI implementation of WriteOnlyDataCommand, it will take care of clocking MSB out first (based on SPI configuration). If it's parallel, then it doesn't matter. Same thing with the other driver.

No it is not useless. If your system is little endian but you need the data in big endian format (which is very common nowadays) then you will need to do the conversion somewhere, you can either do it in each display driver which is error prone (and potentially have a superfluous byteswap there because you could actually do the swap in hardware or worse you'll have to swap back) or you run into other weird mismatches.

We've been there which is why we have added it in the first place.

[chrismoos]

Yeah, but I very much prefer a clean separation from the getgo. Maybe something like an established SPI trait would be a better way to do the proofing.

Done, implemented on existing SPIInterface implementation in this repository.

It does care. The display driver only knows in which format the display expects the format, it does not know which format the display-interface driver is pushing out the data to the display; that's why we have the different data types in the display-interface, the display driver pushes out the data in the expected format and it's up to the display-interface to make sure it ends up at the display in the expected format.

It is possible we are just thinking about different ways to accomplish the same thing. :)

In the end though, the driver wants to send command 0x01. This is where its concern ends; the WriteInterface implementation's job is to translate 0x01 -- if it's a GPIO8 implementation, it writes out each bit to the correct data line (i.e Bit 0 to D0, etc,.). If it's an SPI interface, it will call write on a SPI interface. In display-interface speak, the user is constructing an SPIInterface, passing in their device specific SPI instance. When the user constructs the SPI peripheral instance, they will indicate if MSB or LSB should be used. The actual WriteInterface SPI implementation has no idea how the data (in this case u8) will actually be clocked out.

As for pixel format, from what I've seen, depending on the word size of the interface being used (8-bit parallel, 16-bit, SPI - which usually follows 8-bit) is how the pixel format is determined. In this case, it's important the display interface driver knows the interface's word size. This is how the pixel format detection/writing works here.

In fact, in one of the drivers you linked, it is using RGB 565 encoding and here is where it's encoding the pixel. While it works, it doesn't take into consideration differences in encoding if you are on a different interface. The transmission is different depending on 8/16/18-bit modes, for example, and in some cases, the actual pixel format.

In ILI9486, if you want to transmit RGB666 on 8-bit, it takes 3 write cycles, and the 2 LSB bits for each write are not used. But, if you implement on the 18-bit interface, you can send one whole pixel in one write cycle (filling up all 18 bits). In the current system, how can you facilitate this?

No it is not useless. If your system is little endian but you need the data in big endian format (which is very common nowadays) then you will need to do the conversion somewhere, you can either do it in each display driver which is error prone (and potentially have a superfluous byteswap there because you could actually do the swap in hardware or worse you'll have to swap back) or you run into other weird mismatches.

So, I'm not sure I 100% understand the concern here. When we are sending around (u8, u16, u32) throughout our code, the underlying system endianness is abstracted from us. The system endianness is only relevant when you go and access data directly from memory, probably via some unsafe mechanism. Implementations of display-interface may or may not get that far (FSMC, maybe), but at that point, the endianness of the data is not the question. 0x01 is 0x01. Whether you need to write MSB/LSB to the register is another story, but again, that doesn't have any bearing on what we're getting upstream from the driver.

[therealprof]

When the user constructs the SPI peripheral instance, they will indicate if MSB or LSB should be used. The actual WriteInterface SPI implementation has no idea how the data (in this case u8) will actually be clocked out.

That is completely irrelevant, we're not talking about bit order here, we're talking about endianess, i.e. how the bytes are ordered in datatypes > u8.

In ILI9486, if you want to transmit RGB666 on 8-bit, it takes 3 write cycles, and the 2 LSB bits for each write are not used. But, if you implement on the 18-bit interface, you can send one whole pixel in one write cycle (filling up all 18 bits). In the current system, how can you facilitate this?

At the moment we can't, there's only 8bit and 16bit support, but others can be added if necessary. Some SPI displays e.g. have a 9bit mode.

So, I'm not sure I 100% understand the concern here. When we are sending around (u8, u16, u32) throughout our code, the underlying system endianness is abstracted from us.

No, it is not abstracted. It is implicit. When you assemble pixel data in the MCU it's in native endian (often little endian), the display expects in a certain endian (often big endian). When you send 16 bit values over an 8 bit interface someone has to convert the data (or not).

That's the whole reason for the U16BE and U16LE types. Those allow the display driver to specify in which order the bytes are supposed to arrive at the display and they also allow the display-interface driver to do the conversion (if needed). This removes the need to do gnarly endiness conversions in each and every display driver and allows the display-interface to work as efficiently as possible.

Please have a look at the ST7789 driver which was our implementation playground to make the interface both correct and efficient (in fact we're hitting an embedded-graphics bottleneck now due to the way pixel iterators work and I'm hoping for the new window based iterators to arrive somewhen this lifetime still). I've extensively benchmarked this in 8bit SPI, 8bit parallel and 16bit parallel (both via GPIO and FSMC) modes.

[chrismoos]

That is completely irrelevant, we're not talking about bit order here, we're talking about endianess, i.e. how the bytes are ordered in datatypes > u8.

Yes, I understand, U16LE and U16BE are about byte order and the calls to to_le and to_be end up doing a byte_swap if necessary. But when you talk about If your system is little endian, I'm assuming you are referring to the MCU, and if that's the case, byte order isn't relevant, but instead if the bits are stored 0..15 or 15..0 in address space.

I think we're at an impasse here as we fundamentally think about this differently, as to how the abstraction should work.

[therealprof]

That is completely irrelevant, we're not talking about bit order here, we're talking about endianess, i.e. how the bytes are ordered in datatypes > u8.

Yes, I understand, U16LE and U16BE are about byte order and the calls to to_le and to_be end up doing a byte_swap if necessary. But when you talk about If your system is little endian, I'm assuming you are referring to the MCU, and if that's the case, byte order isn't relevant, but instead if the bits are stored 0..15 or 15..0 in address space.

Yes, internally the MCU uses whatever it uses and that is irrelevant because pixel data will always be in native order. When you push it to the display you may have to change the byte order to accomodate for the required data format (or not which is the idea behind specifying what the data should be). This is not something the display driver should worry about, it simply should format the data as described in the datasheet and let the display interface know what it's supposed to be.

I think we're at an impasse here as we fundamentally think about this differently, as to how the abstraction should work.

I agree.

[chrismoos]

In ILI9486, if you want to transmit RGB666 on 8-bit, it takes 3 write cycles, and the 2 LSB bits for each write are not used. But, if you implement on the 18-bit interface, you can send one whole pixel in one write cycle (filling up all 18 bits). In the current system, how can you facilitate this?

At the moment we can't, there's only 8bit and 16bit support, but others can be added if necessary. Some SPI displays e.g. have a 9bit mode.

Before we close this out, can you let me know how to implement RGB 6-6-6 on the driver side and support the encoding required for 8, 16, and 18-bit parallel interfaces with the current display-interface? High level is fine, but each encoding is slightly different:

• 8-bit interface - 3 write cycles, 24 bits
• 16-bit interface - 1.5 write cycles, 24 bits, and 3 cycles required for 2 pixels (mixes two pixel's color data on the same cycle)
• 18-bit interface - 1 write cycle, 18 bits

[therealprof]

18bit data needs a new set of enum values U18* and the data provided needs to be in a u32.

[chrismoos]

Yeah, I assumed that would be the suggestion. But in the case of the 16-bit Data Bus for 18-bit/pixel, must the interface read-ahead (in the U18Iter case) to transmit the two pixels in 3 cycles? In addition, if it's wrapped in U18BE, I'm curious how the byte swap would work, especially with the padding in 8/16-bit modes.

Also, for the endianness in general, based on how you read bits in here, if you pass a U16BE and are running on a big endian system, no byte swap would happen. But on a little-endian, like an STM32F103, it would swap the bytes. If that's the case, and you check the bits to set based on the value of what you just swapped, wouldn't this give you the wrong behavior? Let me know if I'm not understanding this correctly.

[therealprof]

Yeah, I assumed that would be the suggestion. But in the case of the 16-bit Data Bus for 18-bit/pixel, must the interface read-ahead (in the U18Iter case) to transmit the two pixels in 3 cycles? In addition, if it's wrapped in U18BE, I'm curious how the byte swap would work, especially with the padding in 8/16-bit modes.

The whole point of this API is that it's up to the the display-interface driver to take care of all splitting and bitjuggling and the display driver doesn't have to care. The display driver just says: "I'm giving you an iterator of 18bit pixels values, please take them and provide them as 18bit value in big endian format to the display".

Also, for the endianness in general, based on how you read bits in here, if you pass a U16BE and are running on a big endian system, no byte swap would happen. But on a little-endian, like an STM32F103, it would swap the bytes. If that's the case, and you check the bits to set based on the value of what you just swapped, wouldn't this give you the wrong behavior? Let me know if I'm not understanding this correctly.

This code is just an optimisation to not change pin state if it's the same as the previous state. That's not the whole 16bit value but whatever part of it you've put on the bus last.

[chrismoos]

The whole point of this API is that it's up to the the display-interface driver to take care of all splitting and bitjuggling and the display driver doesn't have to care. The display driver just says: "I'm giving you an iterator of 18bit pixels values, please take them and provide them as 18bit value in big endian format to the display".

Yeah, I understand this, but then you are hard coding pixel format in the display-interface implementation. Maybe this is fine, I have only read through a few datasheets for different controllers and most seem to encode RGB data the same way for 8-bit, 16-bit etc,., so in practice this may be fine, but still, a consideration.

This code is just an optimisation to not change pin state if it's the same as the previous state. That's not the whole 16bit value but whatever part of it you've put on the bus last.

I'm not talking about the changed state, I'm talking about how the bits are written out to the data lines (specifically, how each bit is checked to see if it's 1).

I see that you are actually using to_be_bytes / to_le_bytes so in this case system endianness has no bearing, so I take back what I said above. It converts to a consistent byte order (big/network order or little, regardless of underlying system endianness).

I'll keep this open as I may update this PR to still add "R/W" support, but with the current DataFormat system instead.

Chris

[therealprof]

Yeah, I understand this, but then you are hard coding pixel format in the display-interface implementation. Maybe this is fine, I have only read through a few datasheets for different controllers and most seem to encode RGB data the same way for 8-bit, 16-bit etc,., so in practice this may be fine, but still, a consideration.

No, the display driver provides the pixel data encoding, the display-interface provides the transport. If the display requires some weird interleaving then it's up the display driver to supply that if it wants to support it, e.g.

A usable datasheet will always specifies the acceptable data format and how to stuff pixels into it:

Does your display actually offer a native 18bit mode? If not, there's no need for a 18bit data mode.

I'm not talking about the changed state, I'm talking about how the bits are written out to the data lines (specifically, how each bit is checked to see if it's 1).

? The line you've highlighted checked whether the bit has changed from the previous byte which was clocked out, maybe you've missed the XOR ? https://github.com/therealprof/display-interface/blob/48079b792bebb3bf60330bc5129828c564861a1e/parallel-gpio/src/lib.rs#L88

I'll keep this open as I may update this PR to still add "R/W" support, but with the current DataFormat system instead.

That would be very welcome.

[chrismoos]

Does your display actually offer a native 18bit mode? If not, there's no need for a 18bit data mode.

Yeah, it does, although I don't have one of those displays in hand.

? The line you've highlighted checked whether the bit has changed from the previous byte which was clocked out, maybe you've missed the XOR ?

I was referring to if value & 4 != 0 {, if value & 8 != 0 {, etc,. for checking if a bit is set.

[therealprof]

I was referring to if value & 4 != 0 {, if value & 8 != 0 {, etc,. for checking if a bit is set.

That's just to cater for the GPIO API: if the bit is set the GPIO is pulled high, if not it's pulled low. All the changes are clocked in at once when pulling the WRX high.

https://github.com/therealprof/display-interface/blob/48079b792bebb3bf60330bc5129828c564861a1e/parallel-gpio/src/lib.rs#L211