Incorrect colors when using Raspberry Pi 4 (Bullseye) and Adafruit ST7735R display.

[lynniemagoo]

• Platform/operating system (i.e. Raspberry Pi with Raspbian operating system,
  Windows 32-bit, Windows 64-bit, Mac OSX 64-bit, etc.): Raspberry Pi Bullseye

• Python version (run python -version or python3 -version): Python 3.9.2

• Error message you are receiving, including any Python exception traces: No Error Messages

• List the steps to reproduce the problem below (if possible attach code or commands
  to run): Run attached imagePortrait.py against attached file parrot.bmp

Compare the results to parrot.bmp as displayed in Chrome on Windows or any other system.

I'm using the Adafruit ST7735R 160 x 128 display with the Green tab on the screen protector.

Note that if you are using the stock example image.py you will also see that cat.jpg is not displayed with correct colors on the display.

I'm going to do a PR which will fix the issue but I don't have a Pimoroni display with which to test against.

Adafruit7735RIncorrectColors.zip

[Gadgetoid]

These displays have a colour order bit in the MADCTL register (0x36) which can be set, instead of changing the output data format code; which would break the colour order on the displays this was originally written to support.

See: https://cdn-shop.adafruit.com/datasheets/ST7735R_V0.2.pdf

It's sent to the LCD here:

st7735-python/library/ST7735/__init__.py

Lines 287 to 288 in 1a70d3a

	self.command(ST7735_MADCTL) # Memory access control (directions)
	self.data(0xC8) # row addr/col addr, bottom to top refresh

The library init would need a new property like bgr which accepts True or False and sets this bit accordingly.

To test in your case, you can revert your changes and try setting the RGB bit:

        self.command(ST7735_MADCTL)     # Memory access control (directions)
        self.data(0xC8 | 0b00001000)                 # row addr/col addr, bottom to top refresh

[lynniemagoo]

OK. Had some time to review your comment and the PDF again.

Apologies if we are holding a discussion here.

0xC8 === 0b11001000

And asking to OR a value of 8 with this value yields 0xC8 or 0b11001000..

Isn't this a NO-OP?

I understand and agree with the concern for breaking original displays.

The question I have is if you have the display available, when you run the example image.py with invert=True, does it properly display the cat.jpg on the intended display? Or does it perhaps only invert the red and blue pixels (~color)?

I'm hoping the answer is yes and if that is the case, maybe does this because init defaults to a value of invert=True?

Based on the way the code is currently written, sets a MADCTL value of 0xC8 which sets value of 1 for bit D3 which is telling the display to use BGR format and not RGB format which is the pixel format that the JPEG gets converted to by PIL.

A value of 1 in Bit D3 is what the library currently has as the default. Instead I reworked my original PR to support bgr parameter with a value of True based on your suggestion..

If you have time to test with the original display for which this module was written and find that the default value of invert needs to change from 'True' to 'False' to get the colors to display properly, please consider changing the default values of invert and bgr to False from True.

[lynniemagoo]

OK. I've closed my previous PR and opened another that supports the 'bgr' parameter.

Turns out for Adafruit 358, I must use both invert=False and bgr=False to get this to work correctly. I also had to set the offsets to 0.

The original library is issuing 0xC8 which is setting the D3 bit of MADCTL to BGR and as a result, what is really happening in the image_to_data is that the variable r actually contains the color for blue and the value of b actually contains the color for red.

The easy way to test this is to use a solid color (red or blue) bitmap (.bmp) created in GIMP or some other tool. Hoping what you see with this image is that the color is indeed wrong which means my change to image_to_data earlier was indeed correct.

In the meantime, rather than disrupt this library to fix what I believe is a behavior that has been there a long time, adding the bgr parameter will suffice.

Just completed all testing of new PR and can confirm that all examples function as expected on Adafruit 358 using invert=False, bgr=False.

Hope you will accept this PR and provide a version 0.0.5 so I don't have to use my local edits.

If you have any questions or wish to discuss privately via email, you can reach me via my profile on github.

Regards,
LynnieMagoo

[lynniemagoo]

I just reviewed the adafruit cpp implementation and it also sets MADCTL to 0xC8. So it appears that all the implementations thus far specify bgr. Therefore I am now of the opinion that the current implementation with the bgr color ordering is best as it is coupled with both MADCTL value as well as the endianess of the platform.

If this library could support this new bgr init param, I think we have the best of all worlds. At some point, I would to port this code along with the adafruit cpp library to a combined NodeJS implementation similar to the one currently available for the SSD1351 Oled display. I will cite both this lib as well as adafruit when I do so.

[Gadgetoid]

Sorry, it looks like we're a timezone or two apart here!

Anyway your sleuthing seems correct. I had mistakenly thought that we'd not be setting BGR and didn't think to unpack 0xC81 into bits before suggesting that change.

You seem to have come to the right conclusion, anyway, and the PR looks good! Thank you.

[lynniemagoo]

I'm in US in CST/CDT. (Texas).

You're welcome. I personally did not like the defaults of BGR (even in Adafruit) and then the invert=True in this library but as long as we have dials to turn for both 'bgr' and 'invert', I think it will be OK.

I also edited my original comment above as I had a bug on my side where I was setting width to 120 and height to 160. When fixed, I immediately saw that I also had to override offset_left=0, offset_top=0 to get the parrot.bmp file to display properly.

Thanks again!