Builder pattern refactor

[almindor]

This PR tries to address a few things with a major refactor:

1. Fixes #31 by forcing init before use via typestates
2. Simplifies construction of Display via the builder pattern and unifies all options into one place
3. Adds framebuffer > display size default offset handling logic
4. Removes unnecessary RST reset pin from Display itself into the builder's init function*

Together these 3 changes should make things more straight forward while removing a set of possible bugs if users called things like set_orientation before calling init. It also simplifies the Model by removing the ModelOptions ownership .

• NOTE - this change technically allows abuse of the reset pin outside of the display's control since we do not consume it, only use it during init call. This means that someone could change that pin to other state, or send a reset signal in the middle of the display doing something. I consider this an extreme edge case not worth the "consume" logic that'd be needed here and additional generic param propagation + release logic.

[almindor]

@xgroleau could you please check if things still work fine for you with this PR? I think this should now finally enable proper orientation with some needed code simplification.

[xgroleau]

Looks good! Though when I use the the new drivers, nothing is written to the screen. Maybe my init sequence is not valid? Though it seems fine.

Here is the code that I used

Before

        let mut delay = Delay;
        let di = SPIInterface::new(spi, dc, cs);
        let display_cfg = DisplayOptions {
            orientation: mipidsi::Orientation::PortraitInverted(false),
            ..Default::default()
        };
        let mut display = mipidsi::Display::st7789_240x240_b240x320(di, Some(reset), display_cfg);
         display.init(&mut delay).unwrap();

After

        let mut delay = Delay;
        let di = SPIInterface::new(spi, dc, cs);
        let display = mipidsi::Builder::st7789(di)
            .with_display_size(240, 240)
            .with_framebuffer_size(240, 320)
            .with_orientation(mipidsi::Orientation::PortraitInverted(false))
            .init(&mut delay, Some(reset))
            .unwrap();

[almindor]

Looks good! Though when I use the the new drivers, nothing is written to the screen. Maybe my init sequence is not valid? Though it seems fine.

Here is the code that I used

Before

        let mut delay = Delay;
        let di = SPIInterface::new(spi, dc, cs);
        let display_cfg = DisplayOptions {
            orientation: mipidsi::Orientation::PortraitInverted(false),
            ..Default::default()
        };
        let mut display = mipidsi::Display::st7789_240x240_b240x320(di, Some(reset), display_cfg);
         display.init(&mut delay).unwrap();

After

        let mut delay = Delay;
        let di = SPIInterface::new(spi, dc, cs);
        let display = mipidsi::Builder::st7789(di)
            .with_display_size(240, 240)
            .with_framebuffer_size(240, 320)
            .with_orientation(mipidsi::Orientation::PortraitInverted(false))
            .init(&mut delay, Some(reset))
            .unwrap();

Huh, that's odd. I have a 240x320/240x240 display (waveshare) too and things work here. Things work if you don't specify the orientation?

[almindor]

I just tested your exact init sequence and it works ok for me o.O

You get absolutely nothing on the display this way?

[xgroleau]

I think it's an issue on my communication bus, looking up what is going on. It's probably not the lib actually

[almindor]

I think it's an issue on my communication bus, looking up what is going on. It's probably not the lib actually

Ah, one thing that comes to mind, do you use the CS pin? Make sure it's set right if you use it "manually" (e.g. outside the SPI struct), it's possible it has been flipped from last time and is now always "high" making the display not listen. Something like this happened to me once, was a nice head scratcher.

[xgroleau]

It does draw properly but then the device is cleared immediately after rendering, not sure why though, still investigating. Maybe an issue with my reset pin.

Not sure why it works with the previous version and not this one though :/

[almindor]

It does draw properly but then the device is cleared immediately after rendering, not sure why though, still investigating. Maybe an issue with my reset pin.

Not sure why it works with the previous version and not this one though :/

Hmm I did change the reset pin logic a bit. It shouldn't cause any functional changes but now the pin is no longer owned for the duration of the Display's existence, only during init. Is it possible you're accessing the same pin afterwards?

Could you try making sure the pin is not used completely? Reset should work if you pass None as well.

[xgroleau]

That's what i thought, probably that the pin goes out of scope, is dropped and dropping it causes a reset.

[xgroleau]

Yep that was it. Just so you know, it is still required to give a type to the None, which causes some awkward typing like this to make the compiler happy. Though that is just a general issue when using generics

init(&mut delay, None::<Output<P0_03>>)

To avoid, I personally create a dummy implementation that cannot be instantiated. See here for the None, here for the Some and the dummy implentation here. Though maybe that is not ideal in your case and prefer let the user handle the typing

[xgroleau]

After fixing the pin issue, all orientations works fine!

[almindor]

Yep that was it. Just so you know, it is still required to give a type to the None, which causes some awkward typing like this to make the compiler happy. Though that is just a general issue when using generics

init(&mut delay, None::<Output<P0_03>>)

To avoid, I personally create a dummy implementation that cannot be instantiated. See here and here. Though maybe that is not ideal in your case and prefer let the user handle the typing

Good point, I'll think about how to solve it.

Question on the pin though, how come dropping it caused a reset? is this part of the HAL implementation for GPIO in your case? I find it odd that dropping a pin should change its function or set a value on it.

[xgroleau]

When dropped, embassy, the hal that I use, try to optimise for power consumption. Thus dropping a pin deconfigures and when deconfigured, it's in an unknown state.

[almindor]

When dropped, embassy, the hal that I use, try to optimise for power consumption. Thus dropping a pin deconfigures and when deconfigured, it's in an unknown state.

Ok that changes things quite a bit. I will have to re-introduce the HW pin into the Display then (I wanted to simplify by removing the generally unneeded generic param), because otherwise these kind of mistakes will confound people.

I'm also considering just not allowing hw reset pin and depending on sw reset only as the other alternative, that way if someone does use the HW reset pin they will know it needs to be handled properly. Not sure which way to go tbf.

[xgroleau]

Is there anything that the software reset cannot do that the hardware reset can? If not, software only may be a good option if you want to avoid generics. I personally don't mind the added generic if there is a gain, but some may prefer avoiding it.

[almindor]

Is there anything that the software reset cannot do that the hardware reset can? If not, software only may be a good option if you want to avoid generics. I personally don't mind the added generic if there is a gain, but some may prefer avoiding it.

AFAICS the spec says sw reset is a must. The only thing I can think of is if somehow the device stops responding to commands, a hardware reset could bring it back, but then so can a power cycle then.

I'll probably add the param back to make it safer, it's not such a huge deal.

[almindor]

Is there anything that the software reset cannot do that the hardware reset can? If not, software only may be a good option if you want to avoid generics. I personally don't mind the added generic if there is a gain, but some may prefer avoiding it.

AFAICS the spec says sw reset is a must. The only thing I can think of is if somehow the device stops responding to commands, a hardware reset could bring it back, but then so can a power cycle then.

I'll probably add the param back to make it safer, it's not such a huge deal.

I think I have found a solution to this that doesn't necessitate either. Can you please retry the "drop the pin" case again? It should work ok now.

[xgroleau]

After testing the fix doesn't work. After checking the drop implementation, the pin is actually disconnected, so a floating pin. And after some testing, I actually DO need to initialize the pin and keep it in scope for the whole duration of the program (as in just passing None and never initializing the pins does not work), thus probably ownership would be required? Maybe this is caused by a weak pull resistor on my board though, I am using a OEM board so I don't have the schematics.

[almindor]

After testing the fix doesn't work. After checking the drop implementation, the pin is actually disconnected, so a floating pin. And after some testing, I actually DO need to initialize the pin and keep it in scope for the whole duration of the program (as in just passing None and never initializing the pins does not work), thus probably ownership would be required? Maybe this is caused by a weak pull resistor on my board though, I am using a OEM board so I don't have the schematics.

Interesting, so essentially you either need to not connect the HW reset pin to prevent flutter from the MCU, or make sure that it's initialized and locked in properly for the whole duration.

I'll reintroduce the pin ownership then, it won't prevent cases where people send in None while having a physical connection but that's a user error.

[xgroleau]

I'll reintroduce the pin ownership then, it won't prevent cases where people send in None while having a physical connection but that's a user error.

Perfect thanks a lot. And yes, electrical issue due to a floating pin is definitely a user error, as long as there is a small line in the doc for that, it should be fine.

[almindor]

I just checked the spec. The RESX pin needs to be kept high for normal display operation and lowering it for over 5us will cause a reset. The display will NOT work if the RESX is kept low however, so technically it's a requirement.

I will keep it optional on the basic level for those cases where the reset pin is hardwired to VIN.

[almindor]

@xgroleau sorry for the spam today :) could I bother you with one last test run please? The pin is now owned again (if supplied).

[xgroleau]

I can confirm that it works properly! No worries, I'm more than happy to help :)