Handling rotation and mirroring with MADCTL

[santisis]

As commented in #19 , my display is showing a mirror image. Fixing that issue I've ended up rewritten the entire handling of rotations in order to manage it via MADCTL settings.

The parameter values are taken from page 61 in Sytronix datasheet¹. I'm using the convention of 0 degrees corresponding with MV,MX,MY=0 and positive angles rotating clockwise. I think that's the driver manufacturer convention.

If it breaks the current library behavior, since the default initialization were MV,MX,MY=011 it could be fixed by switching the 0 and 180 degree definitions on ST7735_MADCTL_ROTATIONS dictionary. I've no preference and I've no problem to update my patch in order to reflect that change.

Please, test it since my display hardware is clearly different that yours.

Footnotes

1. https://www.displayfuture.com/Display/datasheet/controller/ST7735.pdf ↩

[Gadgetoid]

Did you intend to delete this?

[santisis]

Yes, sorry.

I thought that it was legacy code from a prior version of image_to_data since there was no public method that receives a RGB565 color and the conversion is being handled with bit manipulation in numpy.

Should I restore it?

(Or delete the references in the tests... I didn't see the tests 🤦 .)

[Gadgetoid]

Nice work, thank you. I'll have to test this against our board/examples to make sure everything still works as expected- the tests aren't comprehensive in that respect.

With rotation happening entirely on device, it might be worth also grabbing the clearer (and faster somehow) version of image_to_data from here:

https://github.com/pimoroni/st7789-python/blob/5c16baecb8844b1807da221b40d03dec500b3a9e/library/ST7789/__init__.py#L345-L361

something like:

    def image_to_data(self, image, rotation=0):
        if not isinstance(image, np.ndarray):
            image = np.array(image.convert('RGB'))

        # Rotate the image
        pb = np.astype('uint16')

        # Mask and shift the 888 RGB into 565 RGB
        red   = (pb[..., [0]] & 0xf8) << 8
        green = (pb[..., [1]] & 0xfc) << 3
        blue  = (pb[..., [2]] & 0xf8) >> 3

        # Stick 'em together
        result = red | green | blue

        # Output the raw bytes
        return result.byteswap().tobytes()

[santisis]

I've ported image_to_data from ST7789 with some minor changes. There's no reason to perform RGB -> uint16 conversion in two steps.

Is it OK for image_to_data to be a function and not a method as it is in ST7789 implementation?

[Gadgetoid]

Hmmm tests look like they're breaking because they mock numpy, IIRC. This causes the types (on the mock object) not to be detected as types and break isinstance. That's a failing of the tests and not the code. Could probably adjust them to fake a type. I'll have a try.

[Gadgetoid]

Got tests up and running, plus a little bit more test coverage and dropping Python 2.x, with this commit: 826dd48

[santisis]

Cool!

I'm going to revisit a little more image_to_data. It bothers me that the function is converting image to RGB without prior checking if it's already RGB. Also I'm not sure if the list() conversion is avoidable.

[Gadgetoid]

Historically the image conversion only worked with a PIL image, so it was pretty cut and dry, but some users wanted to pipe some AI image recognition output into there or something similar. I don't know how you'd reasonably check that raw bytes are any given format but if you're using something that outputs RGB565 directly then - yes - this function would both break it horribly and be wasted effort!

Edit: Ah I guess RGB565 = len(image) = width * height * 2 and RGB888 = len(image) = width * height * 3 in a pinch.

Edit: Also the conversion function expects at least 2D array if one is given, with RGB888 pixels, eg:

[[255, 0, 0], [0, 255, 0], [0, 0, 255]]

Since PIL.image.convert("RGB") is coerced into a 3D array by numpy.array(). eg:

>>> numpy.array(Image.new("RGB", (2, 2)))
array([[[0, 0, 0],
        [0, 0, 0]],

       [[0, 0, 0],
        [0, 0, 0]]], dtype=uint8)

It's all a bit of a mess!

[santisis]

Yes, PIL only supports 24bits RGB ( https://pillow.readthedocs.io/en/stable/handbook/concepts.html#concept-modes ).

My sugestion is something like:

        if not isinstance(image, np.ndarray):
            if image.mode != 'RGB':
                 image = image.convert('RGB')
            image = np.array(image)

It will still convert if image is monocrome, gray, et cetera, but it will not be wasting time creating a copy if the image is already RGB.

To pass something more low level I think that passing a numpy array is OK.

Now, if the data in the numpy matrix is already a raw RGB565 it could not be checked, but it could be passed as a boolean parameter: def display(self, image, raw=False):, which if raw=True it passes the data to the SPI as is.

[Gadgetoid]

raw=True sounds like a sensible idea!

[santisis]

Finally I didn't add the raw parameter.

If image is a list or a bytes object it will be considered as raw data.

If not it must be a PIL Image or a numpy array.

I've checked and SpiDev accepts bytes as a parameter to xfer3() method**, so I've deleted the conversion to list.

(**: Only one method in the library documents "values must be a list or buffer" and, yes, bytes seems to be a buffer for the SpiDev developers.)

[Gadgetoid]

Updated the tests in 314f973 and they seem to be okay.

Will have to try the examples on real hardware, but seems good so far! Thank you.

I think spidev was originally written for one specific purpose and ended up getting adopted for wider use. It's a little rough around the edges. I, uh, guess I count as one of the developers 😆

[santisis]

I, uh, guess I count as one of the developers laughing

Oops ;)

Will have to try the examples on real hardware, but seems good so far! Thank you.

Yes!, and pay special attention on the orientation of the image. The keys in ST7735_MADCTL_ROTATIONS must be reorderer to ensure backward compatibility on the driver. I've no way to test it since my display is different.

[Gadgetoid]

Sorry to come back years later after leaving this to the wayside. I am desperately short on bandwidth for touching stuff that works - this library was pretty much just for our breakout and Grow HAT.

I've now conflicted this by merging a smaller, less comprehensive change which I think this functionally replaces or at least works in conjunction with.

I don't mind if you have no appetite to come back, but I'm going to leave this PR open for future adventurers. Ideally this library should be more complete than it is and if that involves breaking changes and a bit of shakeup I can version pin our products.