-
Notifications
You must be signed in to change notification settings - Fork 489
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 oleds #397
added oleds #397
Conversation
Unlike your other module, this is much less invasive, and OLED modules will likely never be so generic to work with absolutely everything. Once tests pass, I'll take a final look at code, but considering your use case, I'm fine having a semi-specialized OLED module like this in the repo. |
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.
Other notes:
- rename
peg_oledDisplay.py
->peg_oled_display.py
- bonus points if we can get a desktop unit test for any of this, but it's not a blocker
- @xs5871 and @kdb424 can dismiss my review at basically any point to unblock merge; I'm going to be out of town all of next week and not on GitHub and don't want to hold this up
@klardotsh Thank you for looking it over I agree on 9/10 points I'm going to fix it up . |
I think it got it all changed over very good changes I am happy with them. |
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.
Once the changes to the docs formatting is done, it's good by me. I'll let XS have a week or two to nitpick as they are busy, but otherwise, I won't block a merge pending those fixes.
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.
I don't see a reason to block. It's documented and self-contained; even if it is special purpose, it may either differentiate in the future, or provide a reference for others.
And @klardotsh did a better job at nitpicking than I would have.
Thank you xs. Are we missing anything? |
Merge is still blocked. I think @klardotsh and @kdb424 need to re-review or approve the changes? |
changes requests are implemented
I'm sorry, I don't know how I can mark the change requests as resolved, other than dismissing them. |
Seems reasonable to me. Thanks for the review and merge! |
Hello, another pr on the docket for you lol. This is my oled extension, I have been spending a lot of time on this and I don't think I need to make any more changes in the near future so it's time to let others play with it.
Lets git into it (haha)
peg prefix
you will see that it's prefixed with Peg. Peg is some software I am working on to let you configure your kmk board. The prefix is to denote that this is the OLED extension that peg interacts with. Anyone can use it and make changes to it but if there were 2 OLED extensions I wanted people to know that they are different. This does not seem to be a problem the last OLED extension never got pulled in. Being that not only users will interact with this but also other code some of my design decisions were about that. I have however tried to make this easy to use for the end-user.
Docs
I kinda did something a little different in the docs by including images. But I wanted to try and make the best docs I could.
Code
So one of the biggest things I focused on with this extension is to make it work on nrf52xx chips most chips people seem to be using have more rom&ram so if it can fit on the n!n it can work on anything.
On that note, you will see my GC calls often I have tested the extension quite a bit to make sure it is stable. The next thing I wanted to make sure I did was to make this expandable, I want to add other features as time goes on. I feel like I have a solid base here that we can expand on. You will see in "returnCurrectRenderText" my plan. I want to be able to do custom logic for each type of reaction and with this layout, I should be able to. WPM might need to do the math here or read a pin for battery percentage. But the user code stays the same in field 0 tell it what to react to > in field 1 gives it the data to display (images or text)
how we do images
Due to the memory constraints, I was not able to get the images working in the more traditional manner and have to load them right from the disk. This works well and saves us some ram.