Support for 240x135 ST7789 displays

[andreban]

It seems the library only supports 240x240 displays at the moment, but there are 240x135 displays available (eg: the Pimoroni Display Pack).

[almindor]

it seems that this is just a cropping effect, see https://github.com/pimoroni/pimoroni-pico/blob/main/drivers/st7789/st7789.cpp#L178-L194

There's a PR for a similar display. I think I'll just unify these all into ST7789 model and use a size indicator either in the constructor, or via the DisplayOptions (with sane defaults).

[almindor]

@andreban does mipidsi (as is) work with this at all? e.g. does clear() work? Is drawing working ok as long as you use the "cropped" region?

[almindor]

@andreban please see if #28 fixes this issue by using the provided ST7789::st7789_135x240 constructor (or by manually overriding the offset and sizes in DisplayOptions if using with_model)

[andreban]

Thanks for putting together the PR so quickly!

does mipidsi (as is) work with this at all? e.g. does clear() work? Is drawing working ok as long as you use the "cropped" region?

Yes, and yes.

please see if https://github.com/almindor/mipidsi/pull/28 fixes this issue by using the provided ST7789::st7789_135x240 constructor (or by manually overriding the offset and sizes in DisplayOptions if using with_model)

Offsets seem a bit off. Here's what's working for me:

let display_options = DisplayOptions {
    window_offset: (52, 40),
    display_size: (135, 240),
    framebuffer_size: (135, 240),
    ..Default::default()
};
let mut display = Display::st7789_without_rst(lcd_spi_interface, display_options);

[almindor]

Thanks for putting together the PR so quickly!

does mipidsi (as is) work with this at all? e.g. does clear() work? Is drawing working ok as long as you use the "cropped" region?

Yes, and yes.

please see if #28 fixes this issue by using the provided ST7789::st7789_135x240 constructor (or by manually overriding the offset and sizes in DisplayOptions if using with_model)

Offsets seem a bit off. Here's what's working for me:

let display_options = DisplayOptions {
    window_offset: (52, 40),
    display_size: (135, 240),
    framebuffer_size: (135, 240),
    ..Default::default()
};
let mut display = Display::st7789_without_rst(lcd_spi_interface, display_options);

Thanks, fixed. I'm going to work a bit more on finalizing the helper constructors in a better way and if other use cases work fine will release this as v0.4 later.

[9names]

Offsets seem a bit off. Here's what's working for me

I tested #28 on Pico with Pimoroni Display Pack today. For PortraitInverted(false) and Landscape(true) x offset is 53, for Landscape(false) and PortraitInverted(true) it is 52 - is there an off-by-one still in the calcs?

        mipidsi::DisplayOptions {
            orientation: mipidsi::Orientation::PortraitInverted(true),
            invert_vertical_refresh: false,
            color_order: Default::default(),
            invert_horizontal_refresh: false,
            display_size: (135, 240),
            framebuffer_size: (135, 240),
            window_offset: (52, 40),
        },

and

mipidsi::DisplayOptions {
            orientation: mipidsi::Orientation::Landscape(false),
            invert_vertical_refresh: false,
            color_order: Default::default(),
            invert_horizontal_refresh: false,
            display_size: (135, 240),
            framebuffer_size: (135, 240),
            window_offset: (52, 40),
        },

are the "natural" orientations for this board anyway, so it won't really affect us.

Things I noticed while checking this out:

• help for mipidsi::DisplayOptions::framebuffer_size says (0,0) is no-override for framebuffer, but it's mostly broken - fill functions fail, lines still draw for some reason.
• help for mipidsi::Display::st7789 still says it takes model as an argument, probably should remove that.

[9names]

x offset is 53, for Landscape(false) and PortraitInverted(true) it is 52

oh, that was in the linked driver code in the first reply too. :facepalm:

[almindor]

Things I noticed while checking this out:

• help for mipidsi::DisplayOptions::framebuffer_size says (0,0) is no-override for framebuffer, but it's mostly broken - fill functions fail, lines still draw for some reason.

It's not broken it just means that "clipped" display variants make things undefined if you actually write into the framebuffer outside of the clipped region. Clear always wipes the full framebuffer size, if you leave it (0, 0) it'd use the full frame buffer for this model (240x320). I'm considering refactoring the code to enforce variant construction instead of having a default to prevent implicitess issues.

• help for mipidsi::Display::st7789 still says it takes model as an argument, probably should remove that.

I wIll change that. Thanks.

The fact that the off by one logic is present in C drivers too leads me to believe it's "correct". If anyone has any documentation that would explain this logic though I'd love to see it.

I think I'll make two changes to my PR based on this:

1. Change DisplayOptions::window_offset into a Option<Fn(Orientation) -> (u16, u16)> so that logic can be applied from a closure instead of depending on direct values
2. Change Model constructor logic to enforce "variant constructors" directly so there's no "default" config to fall back to.
3. In relation to 2. probably split DisplayOptions so that Model/variant sizing definitions are in Model specific options instead.

[9names]

if you leave it (0, 0) it'd use the full frame buffer for this model (240x320)

Manually setting it to sizes larger than the physical resolution (like 240x320) did not exhibit the "broken" behaviour if I remember correctly, I'll verify this later.

The fact that the off by one logic is present in C drivers too leads me to believe it's "correct"

Agreed. I also share your desire to know "why" it's correct.

[9names]

I've shared my test program and some screenshots to show what I mean. With this test program https://gist.github.com/9names/1a1227c7365b100123bcedea74e2aa4c Framebuffer set to (135, 240) or (240, 320) it displays correctly.

with it set to 67x100 (half of each dimension) the clear and circle within those bounds is drawn and the lines both within and outside those bounds are drawn.

with it set to 0x0, the clear and circles have no effect, but the lines are still drawn.

so, at least by observation, it appears that setting the framebuffer to 0,0 is setting the framebuffer size to 0x0 (and not using the default of 240x320 like the docs say). and my assumption that lines would not draw outside of the framebuffer is being violated - i don't know if this is a bug or a feature.

[almindor]

I've shared my test program and some screenshots to show what I mean. With this test program https://gist.github.com/9names/1a1227c7365b100123bcedea74e2aa4c Framebuffer set to (135, 240) or (240, 320) it displays correctly.

This seems "expected" in that if the FB is screen size or bigger and offset + fb size < actual fb size it should be ok (e.g we're not out of bounds of the HW framebuffer size)

with it set to 67x100 (half of each dimension) the clear and circle within those bounds is drawn and the lines both within and outside those bounds are drawn.

I can explain this as well. Lines are drawn as individual pixels each requiring a call to CASET + RASET. Fills are done in one call per entire "bounds rect" of the geometrical shape. So if the FB is clipped and we try to draw outside, it seems a single pixel CASET/RASET + data works, but a rect sized CASET/RASET + multiple pixel data fail. I wonder if the "first" top left pixel of each fill area is correct :) with it set to 0x0, the clear and circles have no effect, but the lines are still drawn.

Same as above so, at least by observation, it appears that setting the framebuffer to 0,0 is setting the framebuffer size to 0x0 (and not using the default of 240x320 like the docs say). and my assumption that lines would not draw outside of the framebuffer is being violated - i don't know if this is a bug or a feature.

This is a good question, 0,0 framebuffer should actually mean 240, 320 in this driver's case, I'll check this out. I'll probably refactor the PR further to remove default/implicitness however so this might be moot in the end.

[almindor]

Offsets seem a bit off. Here's what's working for me

I tested #28 on Pico with Pimoroni Display Pack today. For PortraitInverted(false) and Landscape(true) x offset is 53, for Landscape(false) and PortraitInverted(true) it is 52 - is there an off-by-one still in the calcs?

        mipidsi::DisplayOptions {
            orientation: mipidsi::Orientation::PortraitInverted(true),
            invert_vertical_refresh: false,
            color_order: Default::default(),
            invert_horizontal_refresh: false,
            display_size: (135, 240),
            framebuffer_size: (135, 240),
            window_offset: (52, 40),
        },

and

mipidsi::DisplayOptions {
            orientation: mipidsi::Orientation::Landscape(false),
            invert_vertical_refresh: false,
            color_order: Default::default(),
            invert_horizontal_refresh: false,
            display_size: (135, 240),
            framebuffer_size: (135, 240),
            window_offset: (52, 40),
        },

are the "natural" orientations for this board anyway, so it won't really affect us.

Things I noticed while checking this out:

• help for mipidsi::DisplayOptions::framebuffer_size says (0,0) is no-override for framebuffer, but it's mostly broken - fill functions fail, lines still draw for some reason.
• help for mipidsi::Display::st7789 still says it takes model as an argument, probably should remove that.

I'm updating the PR to change this using a function callback instead of direct numbers so we can do the 52/53 swap. Could you please provide the full list of orientations and the offsets that work with them?

From your previous info I got this:

    match orientation {
        Orientation::Landscape(true) | Orientation::PortraitInverted(false) => (53, 40),
        _ => (52, 40),
    }

But I'm unsure if that's correct.

[almindor]

This should now work. Orientation changes are fixed via #32

[almindor]

Should be fixed now, please re-open if it still occurs