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

VIA Protocol 12 #19697

Merged
merged 21 commits into from Feb 22, 2023
Merged

VIA Protocol 12 #19697

merged 21 commits into from Feb 22, 2023

Conversation

wilba
Copy link
Contributor

@wilba wilba commented Jan 28, 2023

This PR contains the changes to VIA code in QMK to match with the VIA V12 protocol (functionality and keycodes).

It pairs with the-via/app#104 which will contain a link to the webapp that uses these changes (i.e. the next version of https://usevia.app).

The intent of this PR is to remove VIA-specific keycodes/handling, such that VIA's keycode dictionary can work entirely from QMK's JSON definition of keycodes.

  • Added QK_USER_0 .. QK_USER_31 keycodes based on existing QK_USER range.
  • Added QK_KB_0 .. QK_KB_31 keycodes based on existing QK_KB range.
  • Added range SAFE_RANGE as QK_KB_0 value overlaps with previous SAFE_RANGE value.
  • Removed the VIA specific MACRO00..MACRO15, converted uses to QK_MACRO_0..QK_MACRO_15 etc.
  • Removed the VIA specific USER00..USER15, converted uses to QK_KB_0..QK_KB_15
  • Refactored the VIA specific FN_MO13, FN_MO23 keycodes/handling into a generalized simple tri-layer feature
  • Refactored WT RGB keycodes to be user keycodes, which map to "generalized" custom keycodes in VIA.

Also:

  • Support more than 16 macros
  • Make switch matrix tester work with large switch matrices using multiple queries to get state.
  • Fix device indication (i.e. flash backlight) when active keyboard changes in VIA

Regarding the tri-layer feature:

The intent of adding this to core is to refactor it out of via.c and remove the VIA-specific keycodes. A long time ago, when generalizing my keyboard-level code into VIA code, I decided it was useful enough to be added there, so that everyone using VIA could have access to the ability to access three layers with only two keys, without requiring every keyboard achieve this with custom keycodes and duplicated implementation code. I still believe it's a great feature that everyone should be able to use easily in QMK even when not using VIA. I am open to suggestions regarding changing the keycode names, adding aliases, etc.

Update: Tri-layer feature refactored in #19795

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

@github-actions github-actions bot added core keyboard keymap via Adds via keymap and/or updates keyboard for via support labels Jan 28, 2023
Copy link
Member

@zvecr zvecr left a comment

Choose a reason for hiding this comment

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

Personally I would like to see this broken up so that the tri-layer feature isnt coupled to keycode related changes which are also not coupled to via.

data/constants/keycodes/keycodes_0.0.1_user.hjson Outdated Show resolved Hide resolved
data/constants/keycodes/keycodes_0.0.1_quantum.hjson Outdated Show resolved Hide resolved
quantum/keycodes.h Outdated Show resolved Hide resolved
@wilba
Copy link
Contributor Author

wilba commented Jan 28, 2023

Unit tests fail because qmk generate-keycodes --version 0.0.2 -o quantum/keycodes.h doesn't merge in SAFE_RANGE from keycodes_0.0.1.hjson, not sure why, the ranges get merged.

@zvecr
Copy link
Member

zvecr commented Jan 28, 2023

SAFE_RANGE wasnt declared as a range, it was a single keycode that is now being overwritten. To replicate the old behaviour you would have to add it as an alias to the new definition of 0x7E00. However it might be better to add the following to keycodes_0.0.2.hjson

{
    "ranges": {
        "0x7E00/0x01FF": {
            "define": "SAFE_RANGE"
        }
    }
}

As ranges are currently not checked for overlapping values.

Also, you cant add 0.0.1 files, they would be

keycodes_0.0.1_kb.hjson -> keycodes_0.0.2_kb.hjson
keycodes_0.0.1_user.hjson -> keycodes_0.0.2_user.hjson

@wilba
Copy link
Contributor Author

wilba commented Jan 28, 2023

Thanks.

Adding SAFE_RANGE to keycodes_0.0.2.hjson didn't work, since it's a duplicate value of QK_KB_0 and so it fails on the merge. It works as an alias of QK_KB_0.

@zvecr
Copy link
Member

zvecr commented Jan 28, 2023

Sounds like you added it as a key rather than range?

Ive update the suggestion, to further clarify what I meant. If its working then thats probably good enough while we work through the reset of the PRs content.

@wilba
Copy link
Contributor Author

wilba commented Jan 28, 2023

Yep, my bad, thanks.

@drashna
Copy link
Member

drashna commented Feb 9, 2023

As I'm sure you're aware, the preference for PRs is to contain a single isolated change to simplify reviews. This PR could quite easily be broken up into smaller parts.
The tri-layer code is a prime example -- though in its current form it's fairly restrictive and could be expanded upon, into its own feature. To that end, I've created PR #19795, which already includes unit tests, which would certainly be required for any lower-level code that modifies layer functionality, in an effort to expedite things.
If you could, take a look at that PR, as we would appreciate your feedback on it.

@wilba
Copy link
Contributor Author

wilba commented Feb 10, 2023

#19795 looks good to me. When it's merged to develop, I'll update this PR and remove the tri-layer feature. I'd prefer to replace FN_MO13 FN_MO23 keycodes with TL_LOWR TL_UPPR in keymaps rather than maintain more legacy aliases.

@drashna
Copy link
Member

drashna commented Feb 12, 2023

#19795 looks good to me. When it's merged to develop, I'll update this PR and remove the tri-layer feature. I'd prefer to replace FN_MO13 FN_MO23 keycodes with TL_LOWR TL_UPPR in keymaps rather than maintain more legacy aliases.

It's merged.

@drashna
Copy link
Member

drashna commented Feb 14, 2023

util/regen.sh should fix the keycode regen warnings

@drashna drashna requested a review from a team February 15, 2023 20:42
keyboards/mwstudio/mw75r2/keymaps/via/keymap.c Outdated Show resolved Hide resolved
keyboards/idobao/id67/keymaps/vinorodrigues/keymap.c Outdated Show resolved Hide resolved
@wilba
Copy link
Contributor Author

wilba commented Feb 22, 2023

@tzarc merged develop

@zvecr zvecr changed the base branch from develop to via_staging February 22, 2023 19:54
@zvecr zvecr merged commit 227d423 into qmk:via_staging Feb 22, 2023
@tzarc
Copy link
Member

tzarc commented Feb 22, 2023

Just a heads-up, the merge target was changed to a staging branch so some cleanup from our side can occur.
We'll get it into develop when practical.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core keyboard keymap via Adds via keymap and/or updates keyboard for via support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants