Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

theloukou
Copy link

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

Added support for WaveShare-like modules (greentab offsets with blacktab colors)
Added support for WaveShare-like modules (greentab offsets with blacktab colors)
@theloukou
Copy link
Author

theloukou commented Mar 17, 2022

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 theloukou closed this Mar 17, 2022
Fixed missing code
@theloukou theloukou reopened this Mar 17, 2022
@theloukou
Copy link
Author

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

@theloukou
Copy link
Author

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

@brightproject
Copy link

brightproject commented Mar 16, 2023

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:
#154 (comment)

@theloukou
Copy link
Author

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
Copy link

brightproject commented Mar 16, 2023

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
Copy link
Author

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
Copy link

samo-sk commented Aug 10, 2023

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
Copy link
Author

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
Copy link

samo-sk commented Aug 11, 2023

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_GREENTAB_WS 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 <stdio.h>

#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
Copy link
Author

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
Copy link

samo-sk commented Aug 11, 2023

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
Copy link

mchacher commented Oct 29, 2023

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

@theloukou
Copy link
Author

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
Copy link
Member

ladyada commented Oct 29, 2023

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
Copy link
Author

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 #168 (comment) if I was not understood.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants