New interface traits

[GrantM11235]

No description provided.

[GrantM11235]

Wow, I didn't expect this to impact performance much on my project (8 bit unbuffered parallel interface using fill_contiguous), but its over twice as fast as the master branch!

Please try it out, I think it might be faster than #146

edit: Clearing the screen (320x240) went from 90ms to 10ms 🤯

[almindor]

I generally like this approach it also simplifies the Model trait.

I think if you add CHANGELOG entry and migration documentation I'd prefer this over #146

@rfuest what do you think?

[almindor]

LGTM! @rfuest I want to wait on your opinion as well however.

[rfuest]

I don't think it's a good idea to remove support for display-interface entirely at this point in time. We should at least have a fallback option for users that use custom display-interface impls, for example the FSMC support for STM32F4 (https://github.com/stm32-rs/stm32f4xx-hal/tree/master/src/fsmc_lcd).

[almindor]

I don't think it's a good idea to remove support for display-interface entirely at this point in time. We should at least have a fallback option for users that use custom display-interface impls, for example the FSMC support for STM32F4 (https://github.com/stm32-rs/stm32f4xx-hal/tree/master/src/fsmc_lcd).

How would you want to address this though? Deprecate DI with 0.9 and give some time? Use features to switch between DI and "PI" versions? I don't want to overcomplitcate things or delay them unless necessary.

I think if we release this as 0.9, FSMC can keep supporting 0.8 until they reimplement for the new version. I find it interesting they decided to implement DI inside the HAL directly.

[GrantM11235]

I didn't have time to look into the new traits in detail, but set_pixels being generic over N and still using an iterator doesn't seem to be the perfect solution.

[Word; N] is supposed to represents one pixel¹, so N will be between 1 and 3. When sending rgb565 pixels over an 8 bit interface, we will always send data as pairs of bytes, so we will use an iterator of [u8; 2]. I suspect (but haven't tested) that this is more efficient compared to just an iterator of bytes because the compiler doesn't need to worry about running out of buffer space mid-pixel.

Why do we need to use iterators if we can just pass slices to the pixel data around?

99% of the time we can't just use slices of pixels. For starters, embedded-graphics only uses iterators. Even if we did have a slice of pixels, we would still need to convert rgb565 to big-endian or convert rgb666 to left-aligned. Even if the pixels were already in the right format, they might still need to be copied to a dma capable buffer

but with IntoIterator we need to unnecessarily rebuffer the pixel data in the Interface impls

Using an iterator at the interface boundary instead of a slice means that we don't need to unnecessarily buffer the data when using an unbuffered interface like ParallelInterface

Footnotes

1. If we add support for rgb444, it will probably involve sending pairs of pixels packed into [u8; 3], but I'm not sure exactly how or if that would work at this point. ↩

[GrantM11235]

One of my design decisions when making this trait was to only support mipi dcs style interfaces, not all "displays" that have some concept of "commands" and "data". The reason is that other display interfaces work differently and have different usage patterns. For example, SSD1306 oled command parameters are sent as "commands" but in mipi dcs they are "data".

That means the only other crates that can use this trait are the other crates that support the same displays as us, such as ili9341. They are welcome to use the trait, but they are also welcome to deprecate themselves and just use our crate instead 🤷

---

We can't make the trait private/sealed because we want to allow high performance, hardware specific impls like stm32's fsmc, rpi's pio, or esp32's i80. These impls don't actually need to be in the HALs, they can be in third party crates as long as the HALs support the basic operations.

We could still add convenience constructors like Builder::new_spi(...) but I'll leave that for another day.

[rfuest]

While I might not like all the details of the implementation I won't oppose merging it. Let's see how it works out in practice. And it's definitely a nice improvement over display_interface 👍 .

But one thing I would like to bring up is the mutable slice that is used as a buffer in SpiInterface. There might be use cases where this would be harder to work with, because a Display that uses SpiInterface couldn't be stored in the same struct as the SPI buffer. What is the reason you decided to use this approach instead of making the buffer a const generic inside the SpiInterface?

[GrantM11235]

But one thing I would like to bring up is the mutable slice that is used as a buffer in SpiInterface. There might be use cases where this would be harder to work with, because a Display that uses SpiInterface couldn't be stored in the same struct as the SPI buffer. What is the reason you decided to use this approach instead of making the buffer a const generic inside the SpiInterface?

Some chips can only dma from certain memory regions. If the user is able to create the buffer in one of those regions, then their hal's spi impl won't need to copy the data into another buffer.

I changed it to be generic over AsMut<[u8]>, so now you can use &mut [u8], [u8; N], or even Box<[u8]>, but I'm not sure if it is worth the extra confusion and I'm worried it could encourage people to use a large array and blow up their stack.

Initializing a display with an inline array buffer involves creating the array, moving it into the interface, then moving that interface into the display driver. Each of those moves involves allocating space for the whole array on the stack and copying the whole array. LLVM is notoriously bad at optimizing these copies away. Moving the display driver after that will have the same problem.

For practical purposes it's probably easier to just use static-cell to get a &'static mut [u8]. Then the interface/display is also 'static and can be moved around freely

[almindor]

But one thing I would like to bring up is the mutable slice that is used as a buffer in SpiInterface. There might be use cases where this would be harder to work with, because a Display that uses SpiInterface couldn't be stored in the same struct as the SPI buffer. What is the reason you decided to use this approach instead of making the buffer a const generic inside the SpiInterface?

Some chips can only dma from certain memory regions. If the user is able to create the buffer in one of those regions, then their hal's spi impl won't need to copy the data into another buffer.

I changed it to be generic over AsMut<[u8]>, so now you can use &mut [u8], [u8; N], or even Box<[u8]>, but I'm not sure if it is worth the extra confusion and I'm worried it could encourage people to use a large array and blow up their stack.

Initializing a display with an inline array buffer involves creating the array, moving it into the interface, then moving that interface into the display driver. Each of those moves involves allocating space for the whole array on the stack and copying the whole array. LLVM is notoriously bad at optimizing these copies away. Moving the display driver after that will have the same problem.

For practical purposes it's probably easier to just use static-cell to get a &'static mut [u8]. Then the interface/display is also 'static and can be moved around freely

Correct, when it comes to CS there are essentially two scenarios. Either the CS is handled by the SPI/HAL directly or by us. In general we should favour the SPI/HAL to handle it because at least some MCUs have HW control of CS built-in to the "fast SPI" implementation. Note that many HALs have this implemented "wrong" and do not automatically switch CS and other SPI settings (such as mode, speed etc.) properly when switching between SpiDevice instances, but that's a problem out of scope for this driver.

If we control CS we need to ensure it's reset properly at the end of a transaction of course. If we do not, the best we can do is just document that it's the upstream's responsibility to not let CS hang.

DMA buffers are a must, the speed increase when using them is quite large.

[GrantM11235]

I'm confused, the message you are replying to isn't about CS

[almindor]

I'm confused, the message you are replying to isn't about CS

Ups, misclicked. I read parts in email and then came here and clicked the wrong thing. It was meant mostly as a reaction to the CS discussion, but also l inked to the DMA buffer requirement.

[rfuest]

Some chips can only dma from certain memory regions. If the user is able to create the buffer in one of those regions, then their hal's spi impl won't need to copy the data into another buffer.

I changed it to be generic over AsMut<[u8]>, so now you can use &mut [u8], [u8; N], or even Box<[u8]>, but I'm not sure if it is worth the extra confusion and I'm worried it could encourage people to use a large array and blow up their stack.

Initializing a display with an inline array buffer involves creating the array, moving it into the interface, then moving that interface into the display driver. Each of those moves involves allocating space for the whole array on the stack and copying the whole array. LLVM is notoriously bad at optimizing these copies away. Moving the display driver after that will have the same problem.

For practical purposes it's probably easier to just use static-cell to get a &'static mut [u8]. Then the interface/display is also 'static and can be moved around freely

OK, you convinced me that using &mut [u8] is a better idea. Adding a note about static-cell to the docs would probably be useful.

[almindor]

I think I'm happy with this change atm. @rfuest I'll let you review one more time since your changes were done, but I'm inclined to go ahead with this and merge soon to get the ball rolling.

[rfuest]

@rfuest I'll let you review one more time since your changes were done, but I'm inclined to go ahead with this and merge soon to get the ball rolling.

The PR is in already in a very good state, but there are still 2 unresolved conversations that should be addressed before this can be merged. The one about the duplicate name isn't too important, but I think that the one about the flush method needs to be discussed.