Add a way to specify packing direction for framebuffer

[jamwaffles]

IIUC, the current framebuffer implementation packs colours of less than 8BPP horizontally. It would be useful for displays like the SSD1306 if the colours (in this case BinaryColor) could also be packed vertically, meaning the display data is laid out like this:

      X
   
      1 2 3 ... n n n
   
Y 1   0 0 0     0 0 0
  2   1 1 1     1 1 1
  3   2 2 2     2 2 2
  4   3 3 3 ... 3 3 3
  5   4 4 4     4 4 4
  6   5 5 5     5 5 5
  7   6 6 6     6 6 6
  8   7 7 7     7 7 7
  9   0 0 0     0 0 0
  10  1 1 1     1 1 1
  11  2 2 2     2 2 2
  12  3 3 3 ... 3 3 3
  13  4 4 4     4 4 4
  14  5 5 5     5 5 5
  15  6 6 6     6 6 6
  16  7 7 7     7 7 7
             .
             .
             .

I think this is the only part of FrameBuffer that would block a new release as it would change the API to include a new argument/type parameter to dictate the mapping direction. The performance optimisations could come in a patch release when I have more time to focus on e-g, or someone else wants to pick them up.

[rfuest]

Didn't you intend to implement this packing in the packed-display-buffer crate?

[jamwaffles]

I did, but seeing as we now have FrameBuffer in e-g I thought it would be a better idea to migrate that feature here.

[rfuest]

OK, that might help in reducing some code duplication and having to maintain another crate.

Adding the SSD1306 compatible layout will not only require a parameter for the direction, but also the bit order. To keep to_image working we will also need to add support for the different bit order to RawDataSlice.

[jamwaffles]

Sorry for not being clear on this point earlier: since FrameBuffer is in e-g now, I'll treat packed-display-buffer as an experiment and will move/implement its features/optimisations into FrameBuffer instead. Sounds like it'll be more work than anticipated though :(

[rfuest]

Is there a better way to implement this than to add another type parameter for the bit order?

Framebuffer<C, R, BYTE_ORDER, BIT_ORDER, DIRECTION, const WIDTH: usize, const HEIGHT: usize, const N: usize> 

This would be a terrible type.

Maybe we could wrap R in another type, which specifies bit order for < 8 BPP and byte order for > 8 BPP:

let fb = Framebuffer::<Rgb565, LittleEndian<RawU16>, Horizontal, 10, 10, { buffer_size::<Horizontal>(10, 10) }>::new();
let fb = Framebuffer::<BinaryColor, Lsb0<RawU1>, Vertical, 10, 10, { buffer_size::<Vertical>(10, 10) }>::new();

[jamwaffles]

I suggested a type parameter for perf reasons, but yes that's a horrid type. I like your solution, but I'm not sure I follow with the naming; the bit order wouldn't change for vertical byte alignment, only the direction of the byte. I haven't given it much thought but don't we essentially have to swap X and Y coordinates without swapping WIDTH and HEIGHT to be able to support SSD1306 et al?

[rfuest]

The DIRECTION parameter swaps X and Y. But without also chaining the bit order we would end up with this pixel layout:

      X
   
      1 2 ... n
   
Y 1   7 7     7
  2   6 6     6
  3   5 5     5
  4   4 4 ... 4
  5   3 3     3
  6   2 2     2
  7   1 1     1
  8   0 0     0
  9   7 7     7
  10  6 6     6
  11  5 5     5
  12  4 4 ... 4
  13  3 3     3
  14  2 2     2
  15  1 1     1
  16  0 0     0
             .
             .
             .

In the current implementation the MSB of the first byte in the buffer is located at (0, 0), but the SSD1306 starts with the LSB at (0, 0).

[jamwaffles]

Ah I see. That's a bit unfortunate. Perhaps this is too complex, but we could consolidate the bit ordering and direction:

let fb = Framebuffer::<Rgb565, LittleEndian<RawU16, Horizontal>, 10, 10, { buffer_size::<Horizontal>(10, 10) }>::new();
let fb = Framebuffer::<BinaryColor, Lsb0<RawU1, Vertical>, 10, 10, { buffer_size::<Vertical>(10, 10) }>::new();

although having now written it, the above doesn't really help reduce the size of the type.

That said, I'm not sure this is too much of an issue for the end user. The SSD1306 (etc) driver writers will have to deal with all the LSB/MSB horizontal/vertical configuration but that doesn't need to be passed on to the user and can be type aliased within the crate.

FWIW this is the full SSD1306 type using an SPI interface, so maybe we're in the right club when it comes to long types ;)

type Display = Ssd1306<
    SPIInterfaceNoCS<
        spi::Spi<
            SPI1,
            spi::Spi1NoRemap,
            (
                gpio::gpioa::PA5<gpio::Alternate<gpio::PushPull>>,
                gpio::gpioa::PA6<gpio::Input<gpio::Floating>>,
                gpio::gpioa::PA7<gpio::Alternate<gpio::PushPull>>,
            ),
            u8,
        >,
        gpio::gpiob::PB1<gpio::Output<gpio::PushPull>>,
    >,
    DisplaySize128x64,
    BufferedGraphicsMode<DisplaySize128x64>,
>;

[rfuest]

Ah I see. That's a bit unfortunate. Perhaps this is too complex, but we could consolidate the bit ordering and direction:

I think that keeping them separate is better. RawDataSlice would for example only need to use LittleEndian<RawU16> or Lsb0<RawU1>, because the direction is already handled in Framebuffer or Image.

That said, I'm not sure this is too much of an issue for the end user. The SSD1306 (etc) driver writers will have to deal with all the LSB/MSB horizontal/vertical configuration but that doesn't need to be passed on to the user and can be type aliased within the crate.

FWIW this is the full SSD1306 type using an SPI interface, so maybe we're in the right club when it comes to long types ;)

Yes, Rust embedded users should all be familiar with looong types. My concern was primarily that users who use Framebuffer directly, e.g. for caching, need to deal with a more complex API. But I guess that's the cost of more flexibility. And if we add this we should also expose the feature for users of RawImage IMO, which might complicate the image API a bit.

[jamwaffles]

I think that keeping them separate is better.

Sounds good to me. Grouping them brings no benefits.

But I guess that's the cost of more flexibility.

A cost I'm personally happy to shift onto our users 😁

And if we add this we should also expose the feature for users of RawImage IMO, which might complicate the image API a bit.

Hm, interesting idea, although I'm not sure how useful that would be when programs like imagemagick can already convert images to show on the SSD1306 well enough. Is there a specific use case you had in mind?

[rfuest]

Hm, interesting idea, although I'm not sure how useful that would be when programs like imagemagick can already convert images to show on the SSD1306 well enough. Is there a specific use case you had in mind?

I think we will need this if we want to keep Framebuffer::as_image working for all Framebuffers. If we only support as_image for a subset of framebuffer formats the pixel getter would also only work for these formats.

[jamwaffles]

Ok, no problem then :)

By the way, I made a milestone for the 0.8 release and put some issues into it. Please add/remove them or any others as you see fit.

[rfuest]

By the way, I made a milestone for the 0.8 release and put some issues into it. Please add/remove them or any others as you see fit.

I just noticed that and I'll take a look. If you want to start working on any of the issues please assign yourself to the issue to prevent that we accidentally start working on the same one.

[jamwaffles]

Will do! I'm focussed on other projects at the moment so I'll probably not pick anything up. I think the focus for 0.8 should be mostly breaking changes, e.g. yes to #697 but #694 can be released at a later date because it doesn't change the API.

[rfuest]

The most complicated issue, but also the most important to solve, for the next breaking release will be #613 IMO. But that issue has been blocked for months.

If we want to release 0.8 soon I would suggest that we base it on e-g-core 0.3 and leave the breaking core changes to e-g 0.9 and core 0.4. That's not ideal, because we cannot remove the broken RawU18, but the display drivers wouldn't need to be updated to support e-g 0.8.

[jamwaffles]

Soon would be good, but it feels messy to do a double release like you suggest. I don't think there are any objective blockers to #613, more that nobody has been able to spend some time on it, no?

[rfuest]

... ,but it feels messy to do a double release like you suggest.

What do you mean by "double release"? E-g 0.9 would probably not released shortly after the 0.8 release, if we release 0.8 soon (ish).

I don't think there are any objective blockers to #613, more that nobody has been able to spend some time on it, no?

I wanted to do tests on real hardware before deciding on a new DrawTarget API and without hardware I wasn't able to work on this.

[jamwaffles]

What do you mean by "double release"

Yeah, releasing 0.8 then 0.9.

I wanted to do tests on real hardware before deciding on a new DrawTarget API and without hardware I wasn't able to work on this.

Makes sense :). Did you end up ordering all the right stuff by the way? And please send me an email or Element message so I can reimburse you for those purchases - I totally forgot!

[rfuest]

The displays from China arrived OK (see embedded-graphics/hardware-bench#1 (comment)). I didn't order any other development hardware yet, because we hadn't finally decided what to order and the STM32H7 board did go out of stock in the meantime (at least at Digikey and Mouser).

[jamwaffles]

Thanks for the update. I've left a comment over at hardware-bench

[rfuest]

I just noticed that you assigned yourself to this issue. I had perviously worked on this and have uploaded my WIP version to #711, if you want to continue from where I stopped working on this.

[jamwaffles]

Argh, I knew I'd forget to unassign. I had a quick play around but nothing concrete. Please feel free to carry on with this in #711; I won't step on your toes.

[rfuest]

You wouldn't step on my toes and I wouldn't mind to share some of the work. If you are still interested to work on this, go ahead.

[jamwaffles]

Ok, noted :) thanks also for making a start on it