adafruit / Adafruit-ST7735-Library

This is a library for the Adafruit 1.8" SPI display http://www.adafruit.com/products/358 and http://www.adafruit.com/products/618
https://learn.adafruit.com/1-8-tft-display
547 stars 303 forks source link

Added support for WaveShare-like modules (greentab offsets with blacktab colors-yellowtab?) #168

Open theloukou opened 2 years ago

theloukou commented 2 years ago

Added simple definition for WaveShare version module. They use GREENTAB offsets, but BLACKTAB colors (someone referred to them as YELLOWTAB?) Random cheapo modules seem to use this scheme as well.

*Initialize display with INITR_GREENTAB_WS, simple as that

theloukou commented 2 years ago

Just a note, the Waveshare module linked in the PR shows wrong colors and has correct offsets with the existing GREENTAB init (the example reccomended one), and correct colors but has wrong offsets in BLACKTAB init. Please confirm if anyone has this module or random ebay ones.

theloukou commented 2 years ago

This should be a cleaner solution to issue #154 as well, than having to manually edit library files.

theloukou commented 1 year ago

Hey, is anyone even checking/reviewing these PRs anymore?

brightproject commented 1 year ago

You solution doesn't work! It doesn't work, it just duplicates the already created initialization #define INITR_GREENTAB_WS INITR_GREENTAB This way works: https://github.com/adafruit/Adafruit-ST7735-Library/issues/154#issue-1041004912

theloukou commented 1 year ago

Have you even checked all the changed files in the PR? There are more changed besides the "duplicated initialization". The comment you linked does the exact same thing, but you "mess up" the original intend of the initializer. Mine adds a new one, so you keep both.

brightproject commented 1 year ago

Yes, I made changes in the Adafruit_ST7735.cpp and Adafruit_ST7735.h files as you indicated. Why doesn't the library developer change the code in his archive?

theloukou commented 1 year ago

Beats me, I am not the one responsible about merging this. You can just clone my forked repo btw instead of manually changing the files. Cheers!

samo-sk commented 11 months ago

I believe your code is incorrect. By using #define INITR_GREENTAB_WS INITR_GREENTAB, you make INITR_GREENTAB indistinguishable from INITR_GREENTAB_WS, so even if your code works, it will break support for normal green tab displays.

theloukou commented 11 months ago

I believe your code is incorrect. By using #define INITR_GREENTAB_WS INITR_GREENTAB, you make INITR_GREENTAB indistinguishable from INITR_GREENTAB_WS, so even if your code works, it will break support for normal green tab displays.

Nope. You need to brush up your C stuff. Defining something does not work backwards. Code works fine. IF you test it and it doesn't, then come back and post your results with data.
Either way, it doesn't really matter, since noone from Adafruit seems to check this stuff since a loooong time.

samo-sk commented 11 months ago

I only own a display that functions the same as the one described by you in the description of this pull request, so I can't test your changes directly. I can however test them indirectly. Using the original library with blacktab init causes the output to be shifted and with correct colors. Using the original library and greentab init causes the output to be not shifted and with incorrect colors. Using _INITR_GREENTABWS with your library works OK with my display – no shift, correct colors. Output with blacktab doesn't change. However, output using greentab init is different from original – it is the same as when is used INITR_GREENTAB_WS. This is incorrect – you didn't want to modify the behavior of initR with greentab. PS: Compiling and running this code (on PC, not on Arduino) prints Equals., which disproves your argument on define:

include

define INITR_GREENTAB 0x00

define INITR_GREENTAB_WS INITR_GREENTAB

int testVar = INITR_GREENTAB_WS; int main() { if(testVar == INITR_GREENTAB) { puts("Equals."); } return 0; }

theloukou commented 11 months ago

You still don't get the point of my code. Your piece of code runs correctly, no problems there, but it's irellevant. IF you actually look at BOTH the files in this PR, the changes are not only the added definition. INIT_GREENTAB_WS uses some parts of INIT_GREENTAB code, and some parts of INIT_BLACKTAB code. The original functionality of INIT_GREENTAB is NOT affected, because it does not "back-define" INIT_GREENTAB_WS (which is the factor used in the added checks in the .cpp file.)

If this answer still does not satisfy you, consider this: the original .h file contains these lines

define INITR_18GREENTAB INITR_GREENTAB

define INITR_18REDTAB INITR_REDTAB

define INITR_18BLACKTAB INITR_BLACKTAB

By your logic, defining any of these "18" variants should also mess the plain initializers. If you actually believe this, you should start wondering what black magic Adafruit does, and the original codes works. I'm over and out ☮️

samo-sk commented 11 months ago

Please let me clarify. I know that you modified both of the files. I have copied your changes of both of the files into my copy of the library and I have tested your changes on a real Arduino and a real display – the comment above wasn't just a mere theoretical speculation. The displayed image when greentab was used was different from when the original library was used. You asked me not to argue with you on your code, so I won't do it. I just wanted to clarify my previous comment.

mchacher commented 8 months ago

Hi @theloukou. I am reading this thread. Do you think the lib would be compatible with this waveshare display? Thanks.

theloukou commented 8 months ago

Hi @theloukou. I am reading this thread. Do you think the lib would be compatible with this waveshare display? Thanks.

Your display uses a completely different driver from this library, so.... No!

ladyada commented 8 months ago

rather than adding a new #define which probably will change and requires maintenance, better to just add a comment to the example letting people know they can use BLACKTAB

theloukou commented 8 months ago

rather than adding a new #define which probably will change and requires maintenance, better to just add a comment to the example letting people know they can use BLACKTAB

The BLACKTAB define has wrong offsets with this screen, correct ones are GREENTAB ones. Hence why i made a new #define. Please check https://github.com/adafruit/Adafruit-ST7735-Library/pull/168#issuecomment-1071022756 if I was not understood.