-
Notifications
You must be signed in to change notification settings - Fork 585
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
Ili9341 #1642
Ili9341 #1642
Conversation
Not sure why the build fails at this point - The errors appear to be unrelated. |
Most probably unrelated. The CI has some hiccups. If that is the case, it will be merged anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the submission! Great first work on this one. Few comments related to the fact this is a driver and you can attached a smaller screen and few other elements.
@dotMorten seems you got build errors - you might need to rebase to see them locally |
Follows SSD1351 implementation
Took the liberty of rebasing and fixing the issue with the build, hope that's ok @dotMorten |
@Ellerbach this is ready for one final look so that we can merge. |
@joperezr Not at all. Sorry this one has fallen a bit off my radar |
I don't think the ball was on your court 😉. You've addressed all feedback, we went over this PR today in triage and @Ellerbach will take one final look now that the build is passing. Thank you for your patience, and for your contribution! |
@krwq PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM after resolving comments
don't miss also: #1642 (comment) |
[Triage] Hey @dotMorten any chance you can take a look at @krwq latest comments? As soon as they are addressed we can go ahead and merge this in. |
Thanks so much @dotMorten and @krwq for the contribution! |
@joperezr Sorry I had this on my list but it's been busy lately, and need to "fix" my display due to some bad soldering. But I see it's merged now :) |
no worries at all. sorry that we went ahead and merge without asking you first. Basically during our triage last week we realized that most of the remaining comments we had were very minor, and @krwq had the device with him so volunteered to help ensuring everything was working and merging. Thanks again for helping making this binding possible 👍 |
No sorries needed. Much appreciated finishing this up before I got a chance. |
Addresses #819.
Original work by @krwq. I took his code and fixed some things to make it work for me. Then compared the code with SSD1351 display code and followed its patterns and API as closely to get consistency.
Microsoft Reviewers: Open in CodeFlow