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

add new board and fix spelling mistake. #517

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

Conversation

labrusca
Copy link

@kdb424
Copy link
Contributor

kdb424 commented Jul 15, 2022

While this work is absolutely stunning, I am going to ask if there is is a chance that some of these features, like the battery monitor, or library, may make their way into KMK proper to allow for more boards to make use of these great features. I'll tag in our more qualified maintainers to put their opinions out there, but overall, I'm happy with the level of work that went in, and it looks great over all!

@kdb424 kdb424 requested review from xs5871 and klardotsh July 15, 2022 02:34
@kdb424 kdb424 added the board addition/change Adds or modifies a keyboard, or MCU to a keyboard label Jul 15, 2022
@labrusca
Copy link
Author

labrusca commented Jul 15, 2022

First of all, thank you for your response to the work I have done.

Initially I was just trying to get KMK to work on my M60 keyboard, and in order to get my keyboard's LEDs to work, I used some of the is31fl3733 driver code on github and added an "interface" to it for the default RGB extensions.
So I think the is31fl3733 driver code will work for other keyboards that use the is31fl3733 as the LED driver chip (but I haven't found any other keyboards that use the is31fl3733 chip other than the M60 keyboards)

As for the battery monitor, the value of "BATTERY_VOLTAGE" can be modified according to the type of battery used in the keyboard, so that "battery_level()" can output the exact percentage of power to the Bluetooth, allowing the Bluetooth service will display the battery level to the user.

There is also a USB connection detection function in my code that can be used for common Bluetooth keyboards.
If I come up with any other features, I'll open a new issue to let you know.

Thanks again.

@xs5871
Copy link
Collaborator

xs5871 commented Jul 18, 2022

I'm not familiar with the is31fl3733. It seems to be only used as a pixel buffer implementation, despite there being a lot more to the interface. I don't think that it fits our definition of an extension on its own and we don't have a place for general drivers yet (or aRGB specifically for that matter). Something to consider: is this "original" code, or is there a driver repo out there that we can link to?

The battery charge level code on the other hand would fit nicely into an extension. Though I'd refrain from using a look-up table, but that's a detail.

The python implemention of the pixel buffer has me confused. adafruit_pixelbuffer is compiled into every CP port, isn't it? Unless you specifically exclude it and compile yourself. Please correct me if I'm wrong.

@labrusca
Copy link
Author

labrusca commented Jul 24, 2022

@xs5871
There is nothing wrong with what you have said. I'm not a good programmer as a cyber security engineer, so I need more help from people in programming.

Something to consider: is this "original" code, or is there a driver repo out there that we can link to?

here is the link: is31fl3733_driver

@xs5871
Copy link
Collaborator

xs5871 commented Jul 25, 2022

Ok. We usually don't include third-party libraries in our repository, but encourage links in the documentation and interfaces that may freely utilize third-party code.
I believe you can drop both the adafruit_pixelbuf and is31fl3733 driver code, and probably the pixelbuffer also from the doc.
We can then go over the rest of the code and docs to make it ready to merge.
We can open a new issue for converting the battery indicator into an extension. In my opinion it's not absolutely necessary to do right now, and, if you'd want to do that yourself, give you more time to familiarize with creating a module, otherwise someone else can take over eventually.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
board addition/change Adds or modifies a keyboard, or MCU to a keyboard
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants