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

Set some default configurations for GMMK Pro #13219

Merged
merged 2 commits into from
Jul 15, 2021
Merged

Conversation

andrebrait
Copy link
Contributor

Description

Adding a few configurations as defaults for the GMMK Pro which I think make sense either because they're either

  1. The defaults on the stock firmware, or
  2. Useful for a keyboard focused on enthusiasts and gamers, or
  3. The same as the default value on QMK, but I tried to make them explicit here

Maybe it would be up to @glorious-qmk to check if these changes make sense for the GMMK Pro, but I think at least setting USB_POLLING_INTERVAL_MS to 1 does.

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).

@andrebrait
Copy link
Contributor Author

@drashna I ended up opening this PR against master but I guess I should've gone for develop? Just asking because I see the other activity for the GMMK Pro is on develop

@drashna
Copy link
Member

drashna commented Jun 17, 2021

This should be okay. It will get merged into develop, if/when it's merged into master. And given the type of change, it shouldn't cause any issues.

@Gigahawk
Copy link

Gigahawk commented Jun 21, 2021

Can confirm this seems to work fine on my board

EDIT: Interestingly this seems to cause problems with my KVM:

  • Switching from desktop to my (really slow) laptop usually results in USB not enumerating correctly
    • When this happens switching back doesn't fix it, stuck with the the USB failed to enumerate notification on Windows
  • Switching from laptop to desktop doesn't seem to have an issue

In any case, this is a fairly niche usecase, and I suspect it has something to do with my laptop just being too slow to do some handshake or something, in general this is probably good for most users.

@andrebrait
Copy link
Contributor Author

@Gigahawk I don't think that's such a niche scenario. I myself switch between machines sometimes, though I have a simple USB switch instead of a full KVM.

Could you maybe comment out some of the options and see if that helps?

@Gigahawk
Copy link

Looks like removing FORCE_NKRO makes the issue go away.
No idea what the problem is, I'm switching between two Windows 10 hosts, one just happens to have really underpowered hardware.

@andrebrait
Copy link
Contributor Author

It's probably more connected to quirks of the USB controller and whatnot, I imagine.

Let's remove it then.

@drashna drashna requested a review from a team July 15, 2021 17:00
@tzarc tzarc merged commit c330fa7 into qmk:master Jul 15, 2021
nhongooi pushed a commit to nhongooi/qmk_firmware that referenced this pull request Dec 5, 2021
* Set some default configurations for GMMK Pro

* Remove FORCE_NKRO to avoid issues with KVMs
@andrebrait
Copy link
Contributor Author

@Gigahawk sorry for the necro, but I found out that MOUSEKEY causes a lot of issues on this board with KVM switches.
I had to disable it in order to get my keyboard to stop failing when switching between my Mac and my Windows PC (see #15943).

It seems to cause it on many others too, see #15949.

Maybe it's worth trying to see if you can now keep NKRO enabled and also be able to use your KVM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants