-
Notifications
You must be signed in to change notification settings - Fork 45
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
Scancodes #164
Conversation
So this will cause issues with boards on the older firmware, where |
Doesn't seem that bad to me, the buttons still work, just users with (eventually) outdated firmware will not have labeled GUI buttons, until they update. |
Also if
Not just that. Mapping a key to "Fn" will set the wrong binding, and "Reset Layout" will set the key to the wrong binding. Breaking use of the "Fn" key with no obvious way to fix it, which is a fairly serious issue. |
I haven't tried this change or the firmware update, but |
Just checked with levi, this works, but other keys are broken too. I think we should look into the |
There is some way that Via knows without needing scancode config |
Running this branch with older firmware, and using "Reset Layout", |
Yeah there are definitely some problems with this fix. I think Brock is right about trying to identify keys the way VIA does it. This could also make VIA compatibility more realistic, which could be a nice addition. |
Unfortunately, the via implementation will be too large for now. So this doesn't do that. eventhough I would like to in the future. This works, but I still need to do cleanup tomorrow morning. Just pushing to here for now, just in case my dog eats my laptop. |
8e286ff
to
d4db632
Compare
This has changed On older firmware, this still doesn't seem to show the FN key mapped properly. |
On old or new firmware?
You will have to re-flash firmware on your keyboard. If it was plugged in on the old commit version than the broken values stick |
Should be an issue on either. You're changing the strings used to represent keycodes in
This is after reverting the layout to defaults on an older version of the Configurator. |
With firmware from the rebase this seems like it's mostly working. There are just a few keys that are showing up a bit weird, and they aren't showing up weird the same time every time I launch the configurator or unplug and re-plug Heavy From what I can tell, these are the ones that are sometimes showing up weird:
This list might not get everything, but I'll update it if I catch any more. Since some of them are different each time the keyboard is detected, I think there's some race condition stuff happening. |
@leviport there may be a few more missing, but those seem to be fixed now |
Luckily it's not a race, and just non-deterministic hashing. If Looks like there are fewer but that's still an issue. Here's a script to test that: #!/usr/bin/env python3
import json
import pprint
import sys
def duplicates(l):
d = {}
for k, v in l:
if v in d:
d[v].append(k)
else:
d[v] = [k]
return {k:v for k,v in d.items() if len(v) > 1}
path = sys.argv[1]
file = open(path)
output = json.load(file, object_pairs_hook=duplicates)
pprint.pprint(output) Not an issue I thought of before, but something we should have a unit test for. There are already some other tests to validate correctness of Otherwise, this is generally looking reasonable. Once we make sure the |
Now on firmware from the rebase, I'm no longer seeing unlabeled or strangely labeled keys while testing on Heavy. I'll keep an eye out, in case any others need to be added to the list, but it's definitely much better than it was. On firmware currently in |
There are still some duplicates in |
We need to unit test that, and update the unit tests to test with before "legacy scancodes" and the new ones. |
* Handle firmware version `0.12.20`, as used in system76/qmk_firmware#14 (which I think was released?) * Don't mutate header files. * Map names to match what previous version used - Need to make sure everything, including `RESET`/`BOOT`, is working as expected, with both versions. * Add `\n` to end of generated json files. * Fix behavior without `--override`. This still avoids updating the default keymaps for builtin keyboards pending #140.
@leviport can add more details |
It would be best to keep those changes and fix whatever the issue is. What did it break? |
Hm, my commit doesn't change What is the right half of the spacebar bound to there? Presumably not space, or it's particularly strange. |
Ah, so that should be "Access Layer 3". Regardless, 6536c5b doesn't change the name or numeric value for any of the layer scancodes, so it shouldn't be the issue... |
It's reproducible on the default maps as well as my own map on launch_1, launch_lite_1, and launch_heavy_1. It's probably the same on launch_2, I just don't have one handy. Current QMK |
Is this building QMK from a clean source tree? I see something similar, but the version is being reported wrong by the keyboard. Removing an old |
It's a pretty fresh clone, but I don't remember for sure whether or not I ever had the rebase branch checked out, so I'll re-clone it and try again soon |
Just removing the old |
Sorry I didn't get to this sooner, but I can confirm that freshly cloning QMK and flashing |
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.
This is also working on the rebase. Tested with launch_heavy_1, launch_lite_1, and launch_1, both with the 0.19.12 rebase firmware, and with firmware in current QMK master
.
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.
Everything here should be correct now.
It looks like these were broken in #164. The `BACKLIGHT_` scancodes in QMK are different, and don't do anything since the `rules.mk` files for our keyboards have `BACKLIGHT_ENABLE = no`.
It looks like these were broken in #164. The `BACKLIGHT_` scancodes in QMK are different, and don't do anything since the `rules.mk` files for our keyboards have `BACKLIGHT_ENABLE = no`.
for system76/qmk_firmware#34