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

Scancodes #164

Merged
merged 11 commits into from
Mar 28, 2023
Merged

Scancodes #164

merged 11 commits into from
Mar 28, 2023

Conversation

13r0ck
Copy link
Contributor

@13r0ck 13r0ck commented Mar 14, 2023

@13r0ck 13r0ck requested review from a team March 14, 2023 19:09
@13r0ck 13r0ck mentioned this pull request Mar 14, 2023
1 task
@ids1024
Copy link
Member

ids1024 commented Mar 14, 2023

So this will cause issues with boards on the older firmware, where FN has a different value? This is a problem if firmware updates aren't automatic, and we don't have firmware updates integrated for Window and macOS.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 14, 2023

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.
The keyboard-configurator code here is just a simple map, so there shoundnt be an issue if we have two "FN" values. That would support both

@ids1024
Copy link
Member

ids1024 commented Mar 14, 2023

Also if FN has changed the other LAYER_ACCCESS_* bindings probably have too. And possibly others.

users with (eventually) outdated firmware will not have labeled GUI buttons, until they update.

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.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 14, 2023

And possibly others.

@leviport hasn't found those issues.

Not just that.

I havent seen those issues

As far as we have seen this is simply a graphical bug in the configurator itself, but @leviport can you check those please?

@ids1024
Copy link
Member

ids1024 commented Mar 14, 2023

I haven't tried this change or the firmware update, but keymap.json defines the keycode values that are used in layouts/system76/launch_1/default.json and layouts/picker.json for the default layout and picker, so it should impact those. This mapping isn't just about labels.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 14, 2023

Just checked with levi, this works, but other keys are broken too. I think we should look into the info.json files that upstream (and via use), rather than the scancode map

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 14, 2023

There is some way that Via knows without needing scancode config

@ids1024
Copy link
Member

ids1024 commented Mar 14, 2023

Running this branch with older firmware, and using "Reset Layout", Fn still transitions to layer 1, but it's not momentary, and is stuck on that until I unplug it and plug it back in.

@leviport
Copy link
Member

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.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 17, 2023

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 currently just maps firmware version to a different scancode to name map. This fixes the other scancodes that I didn't notice before, as well as not breaking compatibility for non-updated keyboards.

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.

@13r0ck 13r0ck force-pushed the scancodes branch 2 times, most recently from 8e286ff to d4db632 Compare March 17, 2023 15:20
@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 17, 2023

@ids1024 @leviport this is ready for review now.

@ids1024
Copy link
Member

ids1024 commented Mar 17, 2023

This has changed BACKSPACE to BKSP. This makes the label defined in picker.json no longer match. We'll also want to keep the name unchanged to keep exported layouts compatible.

On older firmware, this still doesn't seem to show the FN key mapped properly.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 17, 2023

This has changed BACKSPACE to BKSP.

On old or new firmware?

On older firmware, this still doesn't seem to show the FN key mapped properly.

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

@ids1024
Copy link
Member

ids1024 commented Mar 17, 2023

On old or new firmware?

Should be an issue on either. You're changing the strings used to represent keycodes in layouts.py. This needs to match layouts/picker.json, and is also used in layout export, so changing these will break import of existing layout exports.

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

This is after reverting the layout to defaults on an older version of the Configurator.

@leviport
Copy link
Member

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
2023-03-17_14-31-07
2023-03-17_14-31-27
2023-03-17_15-17-23
2023-03-17_15-19-07

From what I can tell, these are the ones that are sometimes showing up weird:

  • Reset/Unlock: showing up as BOOT
  • Media Previous: showing up as MPRV
  • Media Next: showing up as MNXT
  • Numpad Slash: showing up as PSLS
  • Numpad minus: showing up as PMNS
  • Numpad plus: showing up as PPLS
  • Numpad enter: showing up as PENT
  • Numpad period: showing up as PDOT
  • Numpad asterisk: showing up as PAST
  • Num Lock: showing up as NUM
  • Bksp: showing up as BACKSPACE
  • Right alt: showing up as ALGR
  • Right arrow: showing up as RGHT

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.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 20, 2023

@leviport there may be a few more missing, but those seem to be fixed now

@ids1024
Copy link
Member

ids1024 commented Mar 20, 2023

Since some of them are different each time the keyboard is detected, I think there's some race condition stuff happening.

Luckily it's not a race, and just non-deterministic hashing. If keymap.json maps multiple names to the same number.

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

Otherwise, this is generally looking reasonable. Once we make sure the keymap.json files are all correct.

@leviport
Copy link
Member

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 master of our QMK firmware, the layer toggle and access keys are still showing up blank in the configurator. If I remap the Access keys with the configurator, they are labeled correctly afterwards, but they act like toggle keys instead. So "Access layer 3" gets me stuck on layer 3 until I re-toggle layer 1 again.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 21, 2023

@leviport @ids1024 hows this looking now?

@ids1024
Copy link
Member

ids1024 commented Mar 21, 2023

There are still some duplicates in keymap.json files as tested with the script above.

@ids1024
Copy link
Member

ids1024 commented Mar 21, 2023

We need to unit test that, and update the unit tests to test with before "legacy scancodes" and the new ones.

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 23, 2023

@leviport #166 has been merged, and this has been rebased and is ready for review.

13r0ck and others added 2 commits March 23, 2023 15:26
* 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.
@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 23, 2023

@ids1024
After QA-ing this with @leviport it seems like your commit broke this. Would you like to keep working on this, or just drop that commit?

@13r0ck
Copy link
Contributor Author

13r0ck commented Mar 23, 2023

@leviport can add more details

@ids1024
Copy link
Member

ids1024 commented Mar 23, 2023

It would be best to keep those changes and fix whatever the issue is. What did it break?

@leviport
Copy link
Member

leviport commented Mar 23, 2023

It's kinda the same thing that was happening on our master branch of QMK earlier, but now it's happening on the rebase:
2023-03-23_15-55-12

The layer keys are showing up blank, and they do not work right if I map them manually. Flashing firmware from current master now works as expected.

@ids1024
Copy link
Member

ids1024 commented Mar 23, 2023

Hm, my commit doesn't change FN in any way, so that shouldn't be impacted by it. Is a different commit here the issue?

What is the right half of the spacebar bound to there? Presumably not space, or it's particularly strange.

@leviport
Copy link
Member

leviport commented Mar 23, 2023

Sorry, here's the mapping:
L1:
2023-03-23_16-19-15
L3:
2023-03-23_16-19-22

Those screenshots were taken with the Lite flashed from QMK master, and everything is showing up correctly. Here's L3 with the QMK rebase firmware, for comparison:
2023-03-23_16-18-40

The toggle layer keys are also blank, as well as my reset key. On the default keymap, access layer 2 is doing the same thing, so I think it's all the layer keys.

@ids1024
Copy link
Member

ids1024 commented Mar 23, 2023

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

@leviport
Copy link
Member

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 master was working, then flashing the rebased firmware results in blank layer keys.

@ids1024
Copy link
Member

ids1024 commented Mar 24, 2023

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 ./quantum/version.h file (which is in .gitignore, but also wasn't removed by make distclean). After removing that it seems the generated ./.build/obj_system76_launch_1_default/src/version.h is being used and the Configurator works as expected.

@leviport
Copy link
Member

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

@ids1024
Copy link
Member

ids1024 commented Mar 27, 2023

Just removing the old quantum/version.h should fix it if this is the issue. And I guess a make distclean to make sure everything is rebuilt.

@leviport
Copy link
Member

Sorry I didn't get to this sooner, but I can confirm that freshly cloning QMK and flashing master has working layer keys in the configurator. I'm going to do a little more testing before approving, but this might be good to go now.

Copy link
Member

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

Copy link
Member

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

@13r0ck 13r0ck merged commit 47ba2c5 into master Mar 28, 2023
@13r0ck 13r0ck deleted the scancodes branch March 28, 2023 20:57
ids1024 added a commit that referenced this pull request Apr 5, 2023
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`.
ids1024 added a commit that referenced this pull request Apr 11, 2023
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`.
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.

3 participants