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

fix data-driven split handedness pin assignement #18254

Closed
wants to merge 1 commit into from

Conversation

Kriechi
Copy link
Contributor

@Kriechi Kriechi commented Sep 3, 2022

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

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

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 cli qmk cli command keyboard python labels Sep 3, 2022
@sigprof
Copy link
Contributor

sigprof commented Sep 3, 2022

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.

@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch from 1ac11f5 to cd83d86 Compare September 3, 2022 15:12
@Kriechi
Copy link
Contributor Author

Kriechi commented Sep 3, 2022

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 Kriechi force-pushed the data-driven-split-hand-pin branch 3 times, most recently from 0bb7fcb to 3d3977f Compare September 3, 2022 15:29
@drashna drashna requested review from sigprof and a team September 3, 2022 17:06
@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch 2 times, most recently from 0ab72e0 to 8940e89 Compare September 4, 2022 09:44
@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch 2 times, most recently from 30f4400 to 6bfeaa2 Compare September 22, 2022 21:00
lib/python/qmk/cli/generate/config_h.py Outdated Show resolved Hide resolved
@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch from 6bfeaa2 to c8884c2 Compare September 23, 2022 18:04
@drashna drashna requested review from zvecr and a team September 24, 2022 16:44
@daskygit
Copy link
Member

daskygit commented Sep 25, 2022

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

@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch 4 times, most recently from f2655e2 to 81217fd Compare September 25, 2022 16:41
@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch from 00aa140 to 29f94ab Compare December 7, 2022 02:31
@Kriechi
Copy link
Contributor Author

Kriechi commented Dec 7, 2022

rebased and fixed conflicts since recent develop merges.

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

@Kriechi Kriechi closed this Dec 7, 2022
@Kriechi Kriechi reopened this Dec 7, 2022
@Kriechi Kriechi force-pushed the data-driven-split-hand-pin branch from 29f94ab to 2f203a1 Compare January 7, 2023 16:00
@zvecr
Copy link
Member

zvecr commented Feb 14, 2023

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.

@github-actions
Copy link

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 github-actions bot added the stale Issues or pull requests that have become inactive without resolution. label Apr 15, 2023
@Kriechi
Copy link
Contributor Author

Kriechi commented Apr 15, 2023

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

@Kriechi
Copy link
Contributor Author

Kriechi commented Nov 1, 2023

Partially implemented and superseeded by #22363 and #22369.

@zvecr
Copy link
Member

zvecr commented Jul 18, 2024

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

@zvecr zvecr closed this Jul 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting review cli qmk cli command keyboard python stale Issues or pull requests that have become inactive without resolution.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants