Remove `window_offset_handler`

[rfuest]

Having to implement a window_offset_handler to use different display sizes shouldn't be necessary and it makes using different display sizes harder.

Only three parameters are required to calculate the correct transformation from logical coordinates to the memory locations in the display framebuffer:

• The maximum supported display size for the controller (aka the framebuffer size)
• The size of the display
• The offset in the default orientation

Let's assume an 32x32 pixel display with a ILI9341 controller, that supports max. 240x320 pixel. (Nobody would use this combination, but it's just an example). For the example I'll only look at the X axis, but the Y axis will work the same way.

If the offset in the default orientation is 10 pixel the x coordinates in the framebuffer will be in the range 10..10+32 = 10..42. If the X axis gets flipped by using MADCTL the display will now be accessible using the range 240 - (10 + 32)..240 - 10 = 198..230.

I think a good way to implement this is to make a better distinction between displays models and display driver ICs inside the crate. The maximum display size is a physical property of each controller IC type and should not be modifiable in ModelOptions.

Roughly something like this:

trait DisplayController {
    const MAX_SIZE: (usize, usize);
    ... // methods like init
}

struct ST7789;

impl DisplayController for ST7789 {
    const MAX_SIZE: (usize, usize) = (240, 320);
}

trait DisplayModel {
    type Controller: DisplayController;
    const SIZE: (usize, usize);
    const OFFSET: (usize, usize) = (
        (Self::Controller::MAX_SIZE.0 - Self::SIZE.0) / 2,
        (Self::Controller::MAX_SIZE.1 - Self::SIZE.1) / 2,
    );
    const COLOR_ORDER: ColorOrder = ColorOrder::Rgb;
    const INVERTED: ColorInversion = ColorInversion::Normal;
}

struct Pico1;

impl DisplayModel for Pico1 {
    type Controller = ST7789;
    const SIZE: (usize, usize) = (135, 240);
    // The default OFFSET could be overridden, but is correct in this case.
    const INVERTED: ColorInversion = ColorInversion::Inverted;
}

The builder would then either take a display, like Pico1, which already contains all necessary settings. Or you could define a new display based on the used controller IC:

let mut display = Builder::with_display(Pico1, di)
    .init(&mud delay, Some(rst))?;
// or
let mut display = Builder::with_controller(ST7789, di)
    .with_display_size(135, 240)
    ...
    .init(&mud delay, Some(rst))?;

[almindor]

Hmm, I like the idea but IIRC pico1 needs specific offset 1-off bumps on different orientations for example no? I remember going "huh" over that oddity when we tested it.

E.g. right now it's

        Orientation::Portrait(false) => (52, 40),
        Orientation::Portrait(true) => (53, 40),

[rfuest]

The coordinate transformations isn't correctly implemented at the moment, which causes the off by one error. If you use the formula from my previous post it will result in the same values: 240 - (52 + 135) = 53.

[almindor]

Ah I see.

Why do you want to use DisplayController as a separate trait though? If it only gives us the "framebuffer size for family of displays" I'd say giving one more const value to the Model would be easier/less work and technically less limiting (if for some reason a model actually has different framebuffer size)

[rfuest]

Why do you want to use DisplayController as a separate trait though? If it only gives us the "framebuffer size for family of displays" I'd say giving one more const value to the Model would be easier/less work and technically less limiting (if for some reason a model actually has different framebuffer size)

IMO it is cleaner to separate the two. A display controller IC will always have a fixed maximum size, I don't see any reason why this wouldn't be the case. The current Model and the new DisplayController traits would actually be very similar.

DisplayModel could be implemented by a BSP crate to make it easier to use the specific display on the board the BSP targets. This wouldn't be possible with the current approach, because Model also contains the driver IC specific code. And it's also not possible for an external crate to implement a Builder::some_display constructor.