qmk_firmware
qmk_firmware copied to clipboard
fix data-driven split handedness pin assignement
Description
(updated PR description based on changes from discussion)
This PR fixes a number of issues with the current data-driven configuration for split handedness detection:
- There was a mix and inconsistent usage of
split.primary
andsplit.main
. The generator forconfig.h
was looking for aprimary
key, the but jsonschema andinfo.py
only usedmain
. -
matrix_grid
config was incorrectly generated with additional{ }
aroundGP1,GP2
pin pairs - handedness by pin high or low never was hooked up - missing pin value
- naming weirdness of
main
/primary
- which has nothing to do with detecting which side we are on - missing options for left or right configs when using pin or matrix_grid depending on low/high state when reading the pin or grid intersection
This PR marks the old json object split.main
as deprecated while remaining a backwards compatible jsonschema, and introduces a new object for side detection:
"split": {
"handedness": {
"method": "pin|matrix_grid|eeprom|usb",
"pin": "GP1", // only needed when using pin method
"matrix_grid": ["GP1", "GP2"], // only needed when using matrix_grid method
"determined_side": "left|right" // optional, only used with pin, matrix_grid, or fixed method
}
}
This PR brings the config generation, the info command, and the only existing usage of the now deprecated data-driven config in line with the new side_detection
schema and handles the conversion of old to new options.
This data-driven config maps to the following config.h keys:
#define SPLIT_HAND_PIN GP1
#define SPLIT_HAND_PIN_LOW_IS_LEFT
#define SPLIT_HAND_MATRIX_GRID GP1, GP2
#define SPLIT_HAND_MATRIX_GRID_LOW_IS_RIGHT
#define EE_HANDS
#define MASTER_LEFT
#define MASTER_RIGHT
Types of Changes
- [X] Core
- [X] Bugfix
- [ ] New feature
- [ ] Enhancement/optimization
- [X] Keyboard (addition or update)
- [ ] Keymap/layout/userspace (addition or update)
- [ ] Documentation
Checklist
- [X] My code follows the code style of this project: C, Python
- [X] 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.
- [X] I have read the CONTRIBUTING document.
- [ ] I have added tests to cover my changes.
- [X] I have tested the changes and verified that they work and don't break anything (as well as I can manage).
The more important question is why main
is even used here? The handedness options do not select which side of the keyboard is “main”/“master”, they select which side is left (but the default method is driven by the master detection status). Only the "main": "left"
and "main": "right"
values make some sense.
The handedness configuration should probably look like this:
"split": {
"side_detection": {
"method": "pin",
"pin": "GP4",
"low_side": "left"
}
}
"split": {
"side_detection": {
"method": "matrix_grid",
"matrix_grid": ["GP4", "GP10"],
"low_side": "left"
}
}
"split": {
"side_detection": {
"method": "eeprom"
}
}
"split": {
"side_detection": {
"method": "main_status",
"main_side": "left"
}
}
To avoid confusing defaults, we could make the low_side
parameter mandatory when "method": "pin"
or "method": "matrix_grid"
is specified in JSON (currently at the C level SPLIT_HAND_PIN
defaults to "low_side": "right"
, but SPLIT_HAND_MATRIX_GRID
defaults to "low_side": "left"
); alternatively we could do a breaking change for SPLIT_HAND_PIN
, but that might be a lot of work.
Thanks @sigprof - this makes way more sense!
I've pushed the changes - with small changes to your proposed naming. I did not add strict validation / error throwing for weird combinations, with the idea that somebody might want to configure "pin" method, while keeping the line for matrix_grid for debugging or other reasons. So whatever the method is set to - will only read the fields that are actually needed by the currently selected method.
And I think I got away with all the LOW/HIGH default handling without too much issues - please verify.
There's been some further discussion on discord with no resolution.
Quick recap is I'd like to see the options renamed with method: usb
requiring it's own further configuration option. For example,
"split": {
"handedness": {
"method": "pin|matrix_grid|eeprom|usb",
"pin": "GP1",
"matrix_grid": ["GP1", "GP2"],
"active_low": "left|right",
"usb_side": "left|right"
}
}
rebased and fixed conflicts since recent develop merges.
@fauxpark I believe I addressed you requested changes - let me know If I missed something!
Seems a bit of discussion on the implementation vs the proposal here happened on discord, without resolution. Ive added the q2 label as a reminder to start discussions again when the current cycle is complete.
Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug
, awaiting review
, breaking_change
, in progress
, or on hold
to prevent the issue from being re-flagged.
bump - please assign awaiting review label, or close directly.
Partially implemented and superseeded by #22363 and #22369.
Closing as I plan to re-implement based on the current state of the repo.