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 oleds #397

Merged
merged 10 commits into from
May 12, 2022
Merged

added oleds #397

merged 10 commits into from
May 12, 2022

Conversation

daysgobye
Copy link
Collaborator

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.

@kdb424
Copy link
Contributor

kdb424 commented Apr 15, 2022

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.

Copy link
Member

@klardotsh klardotsh left a 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

docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
kmk/extensions/peg_oledDisplay.py Outdated Show resolved Hide resolved
kmk/extensions/peg_oledDisplay.py Outdated Show resolved Hide resolved
@daysgobye
Copy link
Collaborator Author

@klardotsh Thank you for looking it over I agree on 9/10 points I'm going to fix it up .

@daysgobye
Copy link
Collaborator Author

I think it got it all changed over very good changes I am happy with them.

@daysgobye daysgobye requested a review from klardotsh April 18, 2022 05:04
kdb424
kdb424 previously requested changes Apr 18, 2022
Copy link
Contributor

@kdb424 kdb424 left a 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.

docs/peg_oled_display.md Outdated Show resolved Hide resolved
@kdb424 kdb424 requested a review from xs5871 April 18, 2022 22:41
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
docs/peg_oled_display.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@xs5871 xs5871 left a 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.

@daysgobye
Copy link
Collaborator Author

Thank you xs. Are we missing anything?

@xs5871
Copy link
Collaborator

xs5871 commented Apr 23, 2022

Merge is still blocked. I think @klardotsh and @kdb424 need to re-review or approve the changes?

@xs5871 xs5871 self-requested a review May 12, 2022 21:52
@xs5871 xs5871 dismissed stale reviews from kdb424 and klardotsh May 12, 2022 21:55

changes requests are implemented

@xs5871 xs5871 merged commit 2de0444 into KMKfw:master May 12, 2022
@xs5871
Copy link
Collaborator

xs5871 commented May 12, 2022

I'm sorry, I don't know how I can mark the change requests as resolved, other than dismissing them.

@kdb424
Copy link
Contributor

kdb424 commented May 12, 2022

Seems reasonable to me. Thanks for the review and merge!

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.

4 participants