qmk_firmware icon indicating copy to clipboard operation
qmk_firmware copied to clipboard

fix data-driven split handedness pin assignement

Open Kriechi opened this issue 2 years ago • 4 comments

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 and split.main. The generator for config.h was looking for a primary key, the but jsonschema and info.py only used main.
  • matrix_grid config was incorrectly generated with additional { } around GP1,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).

Kriechi avatar Sep 03 '22 12:09 Kriechi

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.

sigprof avatar Sep 03 '22 13:09 sigprof

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.

Kriechi avatar Sep 03 '22 15:09 Kriechi

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"
    }
}

daskygit avatar Sep 25 '22 11:09 daskygit

rebased and fixed conflicts since recent develop merges.

@fauxpark I believe I addressed you requested changes - let me know If I missed something!

Kriechi avatar Dec 07 '22 02:12 Kriechi

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.

zvecr avatar Feb 14 '23 22:02 zvecr

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.

github-actions[bot] avatar Apr 15 '23 01:04 github-actions[bot]

bump - please assign awaiting review label, or close directly.

Kriechi avatar Apr 15 '23 13:04 Kriechi

Partially implemented and superseeded by #22363 and #22369.

Kriechi avatar Nov 01 '23 12:11 Kriechi

Closing as I plan to re-implement based on the current state of the repo.

zvecr avatar Jul 18 '24 13:07 zvecr