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

Remove unneeded color argument in Clear() method following changes in PR #161 #162

Closed
wants to merge 2 commits into from

Conversation

txoof
Copy link
Contributor

@txoof txoof commented May 8, 2021

Aligns the example/demo code in PR #161

@SSYYL
Copy link
Collaborator

SSYYL commented May 10, 2021

Why not modify Clear(self) to support a specified color?

@txoof
Copy link
Contributor Author

txoof commented May 10, 2021

The 2in7 display does not support color and the Clear() method doesn't appear to even use the "color" argument. It just sends 0xFF and that appears to work.

I'm not sure why the color arg is even here. Removing it would align the 2in7 with the Clear() method in both the 2in7b and V2. See line 246.

If it makes more sense to have a color arg, perhaps it would be a good idea to update the 2in7b/V2 to match. Either way, it would make it more consistent to have the same methods wherever possible.

I can produce a PR for either at your preference.

@SSYYL
Copy link
Collaborator

SSYYL commented May 11, 2021

I think providing color options is a good way to do it. Black or white is also a choice(Black is probably used less often).

@txoof
Copy link
Contributor Author

txoof commented May 11, 2021

Ok, I'll close this PR and submit an alternative with the color option.

@txoof txoof closed this May 11, 2021
@txoof txoof mentioned this pull request May 11, 2021
@txoof txoof deleted the 2in7_demo branch December 18, 2023 16:55
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.

None yet

2 participants