search

almindor / mipidsi

MIPI Display Serial Interface unified driver
MIT License
108 stars 46 forks source link

use DisplayOptions to allow MADCTL control, fixes #10 #17

Closed almindor closed 2 years ago

almindor commented 2 years ago

Introduces DisplayOptions with all possible MADCTL values abstracted into a more digestible struct.

I've also refactored Orientation to include inversion logic so we don't have init vs set_orientation asymmetry.

This should fix #10

almindor commented 2 years ago

@brianmay @KerryRJ happy to hear your thoughts on this

KerryRJ commented 2 years ago

I am finding that switching between the portrait()/landscape() methods, and the enums(Inversion) a bit strange. Might just be me. I would prefer if @brianmay weighs in on this.

almindor commented 2 years ago

I am finding that switching between the portrait()/landscape() methods, and the enums(Inversion) a bit strange. Might just be me. I would prefer if @brianmay weighs in on this.

I agree it's not ideal. The problem is if we disconnect the inverts then set_orientation becomes asymmetric with init which is silly, basically preventing anyone from initializing the display inverted "just because".

I'm open to suggestions that'd make this more ergonomic given this constraint.

brianmay commented 2 years ago

Taking a step back, and doing a complete rethink, I believe that there are two use cases for setting the inversion bits (actually MY and MX I think):

  1. My screen is mirror image! I need to hold it to a mirror just to read the writing! Toggling one of the invert lines is required. Contrary to what I though previously, It doesn't actually matter which one, if you get the wrong one, the screen will appear upside-down. In which case the next use case will apply.

  2. My screen is upside down! Toggle both of the invert lines from the values previously set.

So how about:

1, For the init function we have an argument mirror_image: bool. This will set one of the invert bits. Either one. But not both.

  1. The orientations enum values have an invert boolean. i.e.:
pub enum Orientation {
    Portrait(bool),
    Landscape(bool),
}

Or maybe we just have 4 variations like before. The bool flag means "use this orientation but display upside-down".

If this bool flag is false, we do nothing. If the bool flag is true, then we toggle both of MY and MX. If the values were both 0, we set them to both 1. If one of the values was set previously in step 1, then we set that bit to 0, and the other bit to 1.

The theory is that once the mirror_image bit is set correctly, you should be able to use all 4 possible combinations for Orientation, and all of them will display correctly without being reversed.

Assumption: I believe that toggling the MV (rotate) bit should not cause the image to be reversed, on any display. At least this is the case for my display. Correct me if I am wrong. If I am wrong, might need to have two mirror_portrait and mirror_landscape parameters that only take affect for the respective orientation. But hoping this isn't required.

KerryRJ commented 2 years ago

If memory serves me correct the st7735s needs both invert bits set to swap the image other wise the text ends up reversed on a single plane. I'd need to double check to confirm.

Confirming that single axis inversion is reversing text on the plane and I have to use XY to get it swapped.

brianmay commented 2 years ago

Yes, this is as I said. You toggle one bit to mirror the display, or toggle both bits to turn the display upside down.

almindor commented 2 years ago

Taking a step back, and doing a complete rethink, I believe that there are two use cases for setting the inversion bits (actually MY and MX I think):

  1. My screen is mirror image! I need to hold it to a mirror just to read the writing! Toggling one of the invert lines is required. Contrary to what I though previously, It doesn't actually matter which one, if you get the wrong one, the screen will appear upside-down. In which case the next use case will apply.
  2. My screen is upside down! Toggle both of the invert lines from the values previously set.

So how about:

1, For the init function we have an argument mirror_image: bool. This will set one of the invert bits. Either one. But not both. 2. The orientations enum values have an invert boolean. i.e.:

pub enum Orientation {
    Portrait(bool),
    Landscape(bool),
}

Or maybe we just have 4 variations like before. The bool flag means "use this orientation but display upside-down".

If this bool flag is false, we do nothing. If the bool flag is true, then we toggle both of MY and MX. If the values were both 0, we set them to both 1. If one of the values was set previously in step 1, then we set that bit to 0, and the other bit to 1.

The theory is that once the mirror_image bit is set correctly, you should be able to use all 4 possible combinations for Orientation, and all of them will display correctly without being reversed.

Assumption: I believe that toggling the MV (rotate) bit should not cause the image to be reversed, on any display. At least this is the case for my display. Correct me if I am wrong. If I am wrong, might need to have two mirror_portrait and mirror_landscape parameters that only take affect for the respective orientation. But hoping this isn't required.

Wait, if we wanted to do it as 4 enum values what would the MADCTL look like for each?

#[repr(u8)]
#[derive(Copy, Clone)]
pub enum Orientation {
    Portrait = 0b0000_0000,
    Landscape = 0b0110_0000,
    PortraitInverted = ???,
    LandscapeInverted = ???,
}
brianmay commented 2 years ago

Wait, if we wanted to do it as 4 enum values what would the MADCTL look like for each?

Depends on if mirror was set to true or not in the init call.

So for the init call:

fn init(mirror: bool)  {
    let madctl = if mirror {0x0b1000_0000 } else {  0b0000_0000 }
}

And for the set_orientation call:

fn set_orientation(&mut self, orientation: Orientation) {
    let madctl = match orientation {
        Portrait => 0b0000_0000 | self.madctl & 0b1101_1111,
        Landscape => 0b0010_0000 | self.madctl & 0b1101_1111,
        PortraitInverted => 0b0000_0000  | (self.madctl ^ 0x0b1100_0000) & 0b1101_1111
        LandscapeInverted => 0b0010_0000 | (self.madctl ^ 0x0b1100_0000) & 0b1101_1111
    }
    self.madctl = mactl;
    // output new madctl value
}

Perhaps a truth table might help:

Mirror Orientation MX MY MV
FALSE Portrait FALSE FALSE FALSE
FALSE Landscape FALSE FALSE TRUE
FALSE Portrait Inverse TRUE TRUE FALSE
FALSE Landscape Inverse TRUE TRUE TRUE
TRUE Portrait TRUE FALSE FALSE
TRUE Landscape TRUE FALSE TRUE
TRUE Portrait Inverse FALSE TRUE FALSE
TRUE Landscape Inverse FALSE TRUE TRUE
almindor commented 2 years ago

Wait, if we wanted to do it as 4 enum values what would the MADCTL look like for each?

Depends on if mirror was set to true or not in the init call.

So for the init call:

fn init(mirror: bool)  {
    let madctl = if mirror {0x0b1000_0000 } else {  0b0000_0000 }
}

And for the set_orientation call:

fn set_orientation(&mut self, orientation: Orientation) {
    let madctl = match orientation {
        Portrait => 0b0000_0000 | self.madctl & 0b1101_1111,
        Landscape => 0b0010_0000 | self.madctl & 0b1101_1111,
        PortraitInverted => 0b0000_0000  | (self.madctl ^ 0x0b1100_0000) & 0b1101_1111
        LandscapeInverted => 0b0010_0000 | (self.madctl ^ 0x0b1100_0000) & 0b1101_1111
    }
    self.madctl = mactl;
    // output new madctl value
}

Perhaps a truth table might help:

Mirror Orientation MX MY MV FALSE Portrait FALSE FALSE FALSE FALSE Landscape FALSE FALSE TRUE FALSE Portrait Inverse TRUE TRUE FALSE FALSE Landscape Inverse TRUE TRUE TRUE TRUE Portrait TRUE FALSE FALSE TRUE Landscape TRUE FALSE TRUE TRUE Portrait Inverse FALSE TRUE FALSE TRUE Landscape Inverse FALSE TRUE TRUE

This means that we cannot just have 4 values in the Orientation enum then tho. We'd still need to have either a (bool) or store the mirror boolean somewhere else somehow.

brianmay commented 2 years ago

This means that we cannot just have 4 values in the Orientation enum then tho. We'd still need to have either a (bool) or store the mirror boolean somewhere else somehow.

That is one way.

You can also store the values in the madctl and use XOR operation to toggle the bits, as I have shown in the sample (untested) code above.

Possibly a boolean might make the code easier to read, not sure.

almindor commented 2 years ago

This means that we cannot just have 4 values in the Orientation enum then tho. We'd still need to have either a (bool) or store the mirror boolean somewhere else somehow.

That is one way.

You can also store the values in the madctl and use XOR operation to toggle the bits, as I have shown in the sample (untested) code above.

Possibly a boolean might make the code easier to read, not sure.

Added the bool to Orientation + some more documentation to it.

brianmay commented 2 years ago

Actually not sure what is going on with this example:

let options = DisplayOptions {
        orientation: Orientation::Landscape(false),
        invert_vertical_refresh: false,
        color_order: ColorOrder::Bgr,
        invert_horizontal_refresh: false,
    };
    display.init(&mut delay::Ets, options).unwrap();
    display
        .set_orientation(Orientation::Portrait(false))
        .unwrap();

I get Portrait screen dimensions, but the screen appears in Landscape. Huh?

OH, wonder if the problem is this line on set_orientation:

let value = self.madctl | (orientation.value_u8() & 0b1110_0000);

Probably need to use self.madctl & 0b0001_1111 here.

brianmay commented 2 years ago

Contrary to what I said above (and I thought had tested too) Portrait(false) is mirrored but Landscape(false) isn't. Weird. Using Portrait(true) fixes it, so not a problem. But thought I should correct what I said before.

almindor commented 2 years ago

Contrary to what I said above (and I thought had tested too) Portrait(false) is mirrored but Landscape(false) isn't. Weird. Using Portrait(true) fixes it, so not a problem. But thought I should correct what I said before.

Wait, I'm confused now. When you say Portrait(false) is mirrored, do you mean it's bugged and should be swapped with what Portrait(true) gives instead?

brianmay commented 2 years ago

Wait, I'm confused now. When you say Portrait(false) is mirrored, do you mean it's bugged and should be swapped with what Portrait(true) gives instead?

On my hardware maybe. This might be hardware specific, not sure, I only have one device to test. The current situation with this PR doesn't really bother me though,

brianmay commented 2 years ago

Don't forget to get the set_orientationfunction call as per https://github.com/almindor/mipidsi/pull/17#issuecomment-1094387568

almindor commented 2 years ago

Actually not sure what is going on with this example:

let options = DisplayOptions {
        orientation: Orientation::Landscape(false),
        invert_vertical_refresh: false,
        color_order: ColorOrder::Bgr,
        invert_horizontal_refresh: false,
    };
    display.init(&mut delay::Ets, options).unwrap();
    display
        .set_orientation(Orientation::Portrait(false))
        .unwrap();

I get Portrait screen dimensions, but the screen appears in Landscape. Huh?

OH, wonder if the problem is this line on set_orientation:

let value = self.madctl | (orientation.value_u8() & 0b1110_0000);

Probably need to use self.madctl & 0b0001_1111 here.

Fixed