add ST7789VW waveshare model

[almindor]

Adds support for ST7789VW waveshare display model. Superseded #19

[almindor]

@cloneable could you please check if this works with the display?

[cloneable]

Yes, will do later! Pretty busy day. Thanks for this!

[cloneable]

Yes, works perfectly. However, I'm not sure this the best way to solve this since Waveshare alone has many other LCDs with different properties. For instance, the reference driver shows 7 models ranging from 0.96in to 2in with different window sizes and offsets.

What if you simply define a trait that exposes display_size(Orientation) and display_offset(Orientation) and also provide a generic impl with 240,320 and 0,0. I could then provide a custom impl for the display I have here?

[almindor]

I guess I could have these be defined for all the other models and then have the st7789vw model not have them implemented, blocking it from being used as-is. The user would then be forced to do it, but I'm not sure if they'd "get" that this is required. Might be better to do a bunch of constructor variables that they have to provide.

[almindor]

@cloneable Do you think this would work? I made new as a special model constructor for just the ST7789VW model requiring users to specify the offsets (same for the Display helper constructors for this model).

This is a bit of a refactor so all the old models transition from new to default but that should be ok.

[cloneable]

Hey, sorry, was traveling. I'll try it out later and will get back to you soon.

Btw, I had to turn on mirroring when switching to landscape orientation (Orientation::Landscape(true)) because text was mirrored. Does the display you're working with not need this? Could this be another quirk of my display?

[almindor]

Btw, I had to turn on mirroring when switching to landscape orientation (Orientation::Landscape(true)) because text was mirrored. Does the display you're working with not need this? Could this be another quirk of my display?

Hmm, it should be fine, but I do remember that ST7789 had this odd case of needing INVON in some cases.

[Chippit]

I also have a waveshare ST7889 display (1.14" in my case), and independantly implemented pretty much exactly this change, before checking that this pull request had done nearly the same thing I had.

Some notes from my adventures, in case it is useful for this PR:

• I needed to remove the madctl color flip OR, because my display did in fact take colours in RGB format and with it blues and reds were flipped. I'm not sure I even entirely understood why that's in the base ST7789 driver here - should it not just have an associated ColorFormat type of Bgr565 for displays where that is necessary?
• I too needed to use Orientation::Landscape(true) for my display, which matches the madctl setup the waveshare example driver uses as well.
• I personally think it still makes sense to leave the framebuffer size at the full 320x240, because then a user might be able to draw offscreen and scroll the display for smoother animation.

[almindor]

I also have a waveshare ST7889 display (1.14" in my case), and independantly implemented pretty much exactly this change, before checking that this pull request had done nearly the same thing I had.

Some notes from my adventures, in case it is useful for this PR:

* I needed to remove the `madctl` color flip OR, because my display did in fact take colours in RGB format and with it blues and reds were flipped. I'm not sure I even entirely understood why that's in the base `ST7789` driver here - should it not just have an associated ColorFormat type of Bgr565 for displays where that is necessary?

* I too needed to use `Orientation::Landscape(true)` for my display, which matches the `madctl` setup the waveshare example driver uses as well.

* I personally think it still makes sense to leave the framebuffer size at the full 320x240, because then a user might be able to draw offscreen and scroll the display for smoother animation.

Did you also need to use the window (caset/raset) offset for your display? That part is to me the most odd one.

[Chippit]

Did you also need to use the window (caset/raset) offset for your display? That part is to me the most odd one.

I did, using effectively the same change as made here in this PR (I referenced the exact offset values from the Waveshare reference C implementation).

Curiously I also noticed today that if using the upside-down landscape orientation, the display appears to be off by one pixel vertically. There might be an off-by-one error in the rust driver somewhere, or perhaps the windows need an additional offset adjustment for that case.

[almindor]

Did you also need to use the window (caset/raset) offset for your display? That part is to me the most odd one.

I did, using effectively the same change as made here in this PR (I referenced the exact offset values from the Waveshare reference C implementation).

Curiously I also noticed today that if using the upside-down landscape orientation, the display appears to be off by one pixel vertically. There might be an off-by-one error in the rust driver somewhere, or perhaps the windows need an additional offset adjustment for that case.

o.O

I'm in Europe on a family visit atm. but I ordered some waveshare displays before going (square 240x240 and the 320x240 full one). I'll try these out when I get back sometime in september.

[almindor]

@cloneable @Chippit if possible please see if #28 fixes this issue in a more elegant way by allowing offset, display_size and framebuffer_size overrides via DisplayOptions and subsequently via helper constructors when using the same base Model

[Chippit]

Thanks for the new update. I checked it out today, and it partially works for me. So certainly having the offsets configurable this way allows me to more conveniently setup the right offsets and sizes for my device without needing to have to implement an entirely new custom display with the right values, as I was doing before.

However the new st7789_135x240 constructor does not work for my hardware, and has both the offsets and the sizes set incorrectly when running in landscape or inverted landscape mode (I have not tested portrait modes as my current application is not designed for them).

I also, frankly, find that the system is now treating portrait values as the 'defaults' and flipping landscape ones very confusing as a user of this library. I suspect this confusion may also the cause of the offset issue I am experiencing, as the offsets I am setting are transposed from the ones the constructor sets.

This does not work (has offset issues)

    let display_options = DisplayOptions {
        orientation: Orientation::LandscapeInverted(true),
        invert_vertical_refresh: true,
        invert_horizontal_refresh: false,
        ..Default::default()
    };

    let mut display = Display::st7789_135x240(di, spi_reset, display_options);

The following code works for me:

    let display_options = DisplayOptions {
        orientation: Orientation::LandscapeInverted(true),
        invert_vertical_refresh: true,
        invert_horizontal_refresh: false,
        window_offset: (40, 52),
        display_size: (135, 240),
        framebuffer_size: (135, 240),
        ..Default::default()
    };

    let mut display = Display::st7789(di, spi_reset, display_options);

Incidentally, it doesn't seem to matter if display_size is transposed or not. Both ways work O_o. But note the confusing (to me at least) transposition of framebuffer_size - it also works when set to the full native panel resolution of 320x240 (here needing to be set as (240, 320), though of course incidentally (320, 240) also works because it happens to fully contain 240x135). (240,135) however does not work, and part of the display is never drawn to when set that way.

Also note that I need to use window_offset: (40, 53) when in Orientation::Landscape, the off-by-one error I mentioned earlier.

[almindor]

@Chippit I think the main issue was that DisplayOptions::window_offset was not getting transposed (while the others were). I fixed that, see if that fixes the x/y issue.

I'm unsure about the +1 problem though. CASET and RASET (which are the two calls that use all of this) are both "inclusive".

[Chippit]

I can confirm that from 92e1ddf0 the offset works as I expect.

I'm not sure about the +1 problem either, my (brief) look into the datasheets did not turn up anything that implied that it should be necessary, so I can do no more than speculate. Admittedly, I cannot pretend to be especially familiar with embedded programming, so much of this is largely new to me. Perhaps it is a subtle bug elsewhere in this driver, or something to do with how the chip addresses offset data in its memory? But my brief comparisons to mipidsi's commands and their parameters to Waveshare's C and Python examples didn't turn up anything suspicious I could see.

Nevertheless, I can confirm that, for my screen at least, the current options.window_offset = (52, 40); works fine for LandscapeInverted, but is off by one pixel (vertically) in Landscape. The first horizontal line that should draw at the top of the physical screen (in that orientation) instead draws at the bottom. If I instead use x=53 for the inverted mode, the top line just ends up with random uninitialized data.

Curiously, the Waveshare C example driver features the following snippet, using a different offset for landscape only:

    if(LCD_1IN14_V2.SCAN_DIR == HORIZONTAL){x=40;y=53;}
    else{ x=52; y=40; }

The example unfortunately does not implement the inverting behaviour mipidsi does so there isn't a direct comparison. And the python example doesn't implement portrait mode at all. It simply hardcodes the following (comments mine), which is exactly equivalent to what the C code does in landscape mode:

        self.write_cmd(0x2A) # CASET
        self.write_data(0x00)
        self.write_data(0x28) # 40
        self.write_data(0x01)
        self.write_data(0x17) # 279 (40 + 240 - 1)

        self.write_cmd(0x2B) # RASET
        self.write_data(0x00)
        self.write_data(0x35) # 53
        self.write_data(0x00)
        self.write_data(0xBB) # 187 (53 + 135 - 1)

All in all, it's very mysterious

[almindor]

Closing in favour of #28 please continue any discussion there, things are getting a bit too fragmented.