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

Adding Keyboard: GMMK Pro #12030

Merged
merged 18 commits into from
Mar 17, 2021
Merged

Adding Keyboard: GMMK Pro #12030

merged 18 commits into from
Mar 17, 2021

Conversation

GloriousThrall
Copy link

@GloriousThrall GloriousThrall commented Feb 26, 2021

Description

Added QMK Compatibility for GMMK Pro.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@GloriousThrall GloriousThrall reopened this Mar 1, 2021
@GloriousThrall GloriousThrall reopened this Mar 1, 2021
Copy link
Member

@drashna drashna left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you have any questions, don't hesitate to ask.

Also, if you go to the "files changed" tab, you can add the suggestions to be merged all at once.

keyboards/gmmk/config.h Outdated Show resolved Hide resolved
keyboards/gmmk/config.h Outdated Show resolved Hide resolved
keyboards/gmmk/config.h Outdated Show resolved Hide resolved
keyboards/gmmk/config.h Outdated Show resolved Hide resolved
keyboards/gmmk/config.h Outdated Show resolved Hide resolved
keyboards/gmmk/pro/pro.c Outdated Show resolved Hide resolved
keyboards/gmmk/pro/pro.h Outdated Show resolved Hide resolved
keyboards/gmmk/pro/pro.h Outdated Show resolved Hide resolved
keyboards/gmmk/keymaps/default/config.h Outdated Show resolved Hide resolved
keyboards/gmmk/pro/rules.mk Outdated Show resolved Hide resolved
keyboards/gmmk/rules.mk Outdated Show resolved Hide resolved
@drashna
Copy link
Member

drashna commented Mar 4, 2021

Oh, and could you add a .noci file added to the gmmk folder?

@drashna
Copy link
Member

drashna commented Mar 4, 2021

Ideally, all of the files in the gmmk files into the pro folder, and merge the files, if needed.

@GloriousThrall
Copy link
Author

GloriousThrall commented Mar 5, 2021

Ideally, all of the files in the gmmk files into the pro folder, and merge the files, if needed.

Hey @drashna, First of all, thank you for taking the time to review all of the code :) 💯
Just wanted to ask if you can clarify what you meant here, I am not sure what you are specifically asking me regarding merging files. Thank you!

@drashna
Copy link
Member

drashna commented Mar 5, 2021

You are very welcome!

And I mean, basically move all of the files, from gmmk to pro. Maybe leave a vendor readme file in gmmk. This would help prevent build errors (due to how the build process works, which is based on if files are present or not), and a bit easier to maintain, I think.

If you would like, I can see about cleaning it up and opening a PR to your repo with the changes.

@GloriousThrall
Copy link
Author

You are very welcome!

And I mean, basically move all of the files, from gmmk to pro. Maybe leave a vendor readme file in gmmk. This would help prevent build errors (due to how the build process works, which is based on if files are present or not), and a bit easier to maintain, I think.

If you would like, I can see about cleaning it up and opening a PR to your repo with the changes.

Ah, I see okay that makes sense. Yeah, you are more than welcome to open up a PR to the repo and add any changes. I would love to have the final touch from the pros :) You guys are great! @drashna

@drashna
Copy link
Member

drashna commented Mar 7, 2021

I opened a PR on your fork with the changes.

@GloriousThrall
Copy link
Author

@drashna The particular chip that is being used is the Awinic AW20216S.

@Gigahawk
Copy link

As I said, that's not even a publicly available part.

Is this a custom part made for G-SPY?

Either way can we at least have a datasheet?

@drashna
Copy link
Member

drashna commented May 10, 2021

Yeah, without a datasheet, adding support will be impossible

@GloriousThrall
Copy link
Author

@drashna I reached out to your email. Please take a look when you get a chance.

@Gigahawk
Copy link

Gigahawk commented May 11, 2021

Let me guess, part is restricted to G-SPY and their partners, datasheet is only released with an NDA?

You understand how this works right?

If the code eventually gets implemented we won't need the datasheet to know how it (well at least the communication protocol) works.

@pinpox
Copy link
Contributor

pinpox commented May 11, 2021

Is this the issue to watch out for full GMMK pro support?
As far as I could figure out from the glorious blog, qmk support is on the way, but there are still issues with RGB and the encoder. I suppose this is the PR that tracks development?

@Gigahawk
Copy link

There will be a new PR if RGB ever comes.

As for rotary, that is already supported in QMK, just not in VIA, and that's not really a QMK or Glorious problem, it will happen when it happens.

As for VIA support in general, that was merged a while ago and will presumably be in the next VIA release.

If you really want a tool to edit keymaps with rotary support right now I would probably look at Vial instead https://get.vial.today/

@pinpox
Copy link
Contributor

pinpox commented May 11, 2021

I'm fine with editing keymaps files manually and flashing them via cli tools. I need my backlighting/rgb to work though, are there any plans for it or work in progress?

@GloriousThrall
Copy link
Author

@pinpox Yes, we are working on it now! :)

Hey @Gigahawk, no we have not signed any NDA's and we are trying to fully implement the QMK RGB Configuration even if that means we have to do some extra work to get it supported. Appreciate the help :)

@Gigahawk
Copy link

Gigahawk commented May 11, 2021

@GloriousThrall So what was in that email to @drashna that can't just be discussed here?

Edit: for that matter, why remove the LED matrix spreadsheet, you know it's still available from the edit history right?

@pinpox
Copy link
Contributor

pinpox commented May 11, 2021

@pinpox Yes, we are working on it now! :)

Thanks a lot. Let me know if there is a PR for it to follow.

@van1337
Copy link

van1337 commented May 12, 2021

@GloriousThrall How certain are you that QMK RGB can be implemented with the chip you're using? I have about 3 days left before I enter a preorder to get it by June.

@drashna
Copy link
Member

drashna commented May 12, 2021

@drashna I reached out to your email. Please take a look when you get a chance.

Awesome, thanks! I have some other stuff that has priority, but I'll try to get to this soon.

Unless you beat me to it.

And yeah, a quick look at the data sheet, it looks very similar to the ISSI stuff, which is a plus.

If you need any help with it, let me know.

@Gigahawk
Copy link

Is there a reason this datasheet isn't being made at least partially public?

I'd love to work on this too

@drashna
Copy link
Member

drashna commented May 12, 2021

Is there a reason this datasheet isn't being made at least partially public?

I'd love to work on this too

I think that the biggest reason is that the datasheet isn't publicly released, so distributing it is... iffy.

@pascalpfeil
Copy link

That is why he said partially, the communication protocol will be pulic in the code anyway

@Gigahawk
Copy link

Do they know this will be used to implement open source code?

If there's concerns about leaking electrical performance characteristics or something we don't need those, but by the time support gets implemented it will effectively just be a copy of the datasheet.

Also given all of this, again the choice of this chip is quite suprising; this sure didn't age well:

MC: Was it difficult to implement QMK and VIA firmware for the GMMK Pro?

SM: It wasn’t difficult necessarily. We’ve had a lot of experience with the original GMMK, so we understood the caveat with developing software for keyboards. It was as simple as sourcing compatible parts for it. Once we were able to do that, it wasn’t much of an issue. Of course, there have been global shortages because of COVID, so that was a struggle, but overall it wasn’t too challenging to get that.

@Gigahawk
Copy link

Yes, we are working on it now! :)

literally been two months
image

@van1337
Copy link

van1337 commented May 19, 2021

QMK thread sticky removed from the glorious subreddit too. Lots of recent comments in there were complaints.

@pinpox
Copy link
Contributor

pinpox commented May 19, 2021

@van1337 Do you still have the link? I'd be interested to read it.

@van1337
Copy link

van1337 commented May 19, 2021

@pinpox

https://www.reddit.com/r/glorious/comments/mzwhqm/an_official_glorious_dev_update_including/?sort=new

The post is long, but it's nothing but just saying, "it's coming soon."

That post was actually in response to @Gigahawk 's post because it became a top post on /r/mechanicalkeyboards for a day. They never replied to his followup to their response.

@Gigahawk
Copy link

Gigahawk commented May 21, 2021

DISCLAIMER: I don't really know what I'm doing, if someone knows I'm wrong about this please call me out.

So I've taken a look a the Glorious firmware binary in Ghidra (mostly just using stacksmashing's tutorial), and it doesn't look like the firmware even directly accesses any I2C busses (nor any other busses that might be used to control a chip like this):
image
(the peripherals labelled in dark blue indicate they are referenced somewhere in firmware, black indicates not referenced).

Now of course RGB does actually work on the Glorious firmware, so the MCU is somehow communicating with these ICs, presumably the I2C registers are being accessed indirectly.
This may be some due to some weird compiler optimization, but it may also be because G-SPY has purposely obfuscated parts of the firmware.

@GloriousThrall I ask this again, does awinic or G-SPY know that a firmware driver for this chip is going to be implemented in an open source project?

@fauxpark
Copy link
Member

@Gigahawk According to drashna, it is an SPI-based chip, not I2C, so no wonder you are not seeing anything there.

@Gigahawk
Copy link

image
SPI is also not directly accessed interestingly, so my point still stands.

@van1337
Copy link

van1337 commented May 21, 2021

I wonder what will be announced, still "coming soon" tm

@pinpox
Copy link
Contributor

pinpox commented May 22, 2021

I wonder what will be announced, still "coming soon" tm

Me too, this was one of the features I was looking for when deciding for a keyboard. I need qmk as promised.

@Gigahawk
Copy link

Gigahawk commented May 25, 2021

Tried posting this to /r/mechanicalkeyboards, mods don't seem to want to approve it, so I'm posting here instead.

Glorious claims to be "working" on adding RGB support to QMK, yet /u/GloriousThrall hasn't had any real Github activity for over two months now.

Now, I would be happy to give them the benefit of the doubt and say that implementing RGB is complicated...
that is, if I didn't manage to do it myself in like 2 days.

Proof: https://youtu.be/KGloqwgCuhg

If you want, you can try it out yourself here (at your own risk, I take no responsibility for bricked boards): https://drive.google.com/file/d/1NpuYt0k1-uu1wfUAfCMfcrMQBPrqtqGZ/view?usp=sharing

Note: In the video some keys don't light up, this may be due to:

  • I mapped some LEDs incorrectly in the firmware
  • I damaged the second LED driver when desoldering it to probe some pads.

Edit: seems to be a bit of both, the mapping for printscreen (next to function row) is probably mapped incorrectly, and I think I have bad solder connections for the enter and down arrow keys.

If you see some keys not light up, please let me know, as that means I have to spend some time figuring out what address those LEDs are mapped to.

The keymap used for this build is here: https://pastebin.com/QjcKTtK5

You will see that I have disabled a lot of keys for this demo, and I have decided to withhold the source code for this until:

  • I get a personal apology for being ignored and banned from the Glorious discord
  • The community gets a proper apology/explanation for how Glorious has handled this situation, pinned to /r/Glorious until all remaining issues have been resolved
    • What has actually been going on with RGB support? This took me like 2 days to write, it should not have taken more than a week or two even if you were busy doing other things
    • Why was a community member required to make the pull request for VIA support after the board was released? This is something that could have easily been done alongside the initial pull request for QMK.
    • What chip is going to be used in the Q3 batch? You say you've identified a chip, yet chose not to name it even though the community probably would have helped to implement it into QMK. Have you already secured enough chips for production? If you haven't, are they even still available?
  • Consent from Awinic or G-SPY to release the source code
    • The LED driver chip being used is a proprietary chip made by Awinic, presumably for your contractor G-SPY, the datasheet for this is not public, and I'm not releasing source code until they are aware that publicly available code will contain info from this datasheet.

@pinpox
Copy link
Contributor

pinpox commented May 25, 2021

@Gigahawk awesome, thanks!

Common glorious, help us out here: you get your work done for free and we get the software we were promised. Win - win. Can't be that much work to look over it, can it?

@tzarc
Copy link
Member

tzarc commented May 25, 2021

Whilst we share your frustrations, this isn't the forum for airing out grievances. This PR has been merged -- you're welcome to create an issue and/or a PR, but please keep things on-topic in future. Locking thread to contributors.

@qmk qmk locked as resolved and limited conversation to collaborators May 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants