-
-
Notifications
You must be signed in to change notification settings - Fork 39.6k
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
Auto shift: support repeats and early registration #9826
Conversation
That was fast. Lots of free time in quarantine? 😄
I'd argue that the tap-then-hold behavior should also apply to shifted keys (or a flag to change whether it happens on its own). Also, With this being nearly a rewrite, would you be alright with adding something similar to #8127 to merge in one go? (or is that a bad idea anyway? Kinda new to GitHub) I could code it, just want to try to get custom shifts in as nothing's happened to that for four months. Figuring out autorepeat with shifted keycodes may be tricky. |
Here's a way of thinking of the functionality I implemented: your auto-shift key is physically stuck to to a
I will punt on it for now... It took me a few tries (and force pushes) to get an intuitive shift behaviour that I was happy with, which mostly means that the keyboard is faithfully representing its physical state with the exception of the shift key. Custom shift actions seem much more complicated to think about in the edge cases I've been testing (if I hold "1" then press and release "2", should the keyboard continue to hold the custom shifted "1" or switch it to normal "1"? What if I only tap "2", effectively requiring shift to be released - will the keyboard be pretending that "1" was released even when it wasn't?). I think your feature is cool but probably belongs in tap dance rather than auto-shift. |
I understand the comparison with the key pressing a MT and itself, but don't see that in the code or a reason to stick to it. I'm tired, might understand in the morning. Either way, I feel a way to disable keyrepeat is necessary.
See here. AutoShift keys are normally evaluated on release, are they not? You are simply making the timeout an alternate trigger to handle it. Evaluating on 'release' means we have more info about what the user is trying to do. In the case you mention, shift (but not 1) should be released and 2 tapped, then shift restored. In normal typing, this produces the exact effect as with normal keys - hold a, then press b. a will stop being repeated and there will be one b at the end. In a game, it would also likely produce the correct effect, as 1 is not a modifier (and everyone using AS has a gaming layer right now anyway, as there is no keyrepeat). Should 2 be held, it is the thing that would repeat in typing, and so the shift state should belong to it. Both of these fire when the second key is released or hits time, and so updating the shift states and current key is simple as we either swap shift, press, and restore shift without changing anything or (if hit time) swap all AS variables as if the first key did not exist. Sorry for the wall of text. As I said above, I can code it, and if you don't want it in this pull I can always do it later, but I feel this pull and custom AutoShifts would make the feature truly complete. |
Cool, that should be consistent with what I've implemented (except maybe the "restore shift" part - this PR won't remember how long you held "1", and you can potentially be still holding it down without it ever being auto-shifted, e.g. by rolling from "1" to "1"+"2" quickly, which I find is common for sequences like "st" on Dvorak). The main philosophical issue I have with the custom auto shifts (should we call these "custom hold keys" or something instead, since they're not actually sending shifts?) is that if you press a bunch of them together, you have to know when you pressed each one to figure out what the keyboard might actually be sending (if your fingers are on "1" and "2", but autoshift_custom_shifts(KC_1) == KC_7, what's QMK reporting as being held down?). In the simpler pure auto-shift case, you only need to look at where your fingers are and when you pressed the last key. Pragmatically (for typing and such), this probably doesn't matter, but it seemed like a nice theoretical goal when I was staring at a keyboard viewer when testing. I'll need to have more of a think about whether that's compatible with preventing repeats. The intuitive behaviour for me is that if you hold a key down QMK should faithfully report that key as being held down to the OS.
Please do go ahead and code it - it doesn't really matter what I think, plenty of QMK features don't follow my "stay true to the physical keyboard" philosophy! Whether I merge it into this PR will depend on what the QMK folks say about both - in the worst case we can have dependent PRs and rebase. I also suspect that this PR needs to be cleaned up a little bit, particularly to support custom hold actions (there are a bunch of assumptions about what's going on with the shift key which will break when the hold actions aren't actually sending shifts). Regardless of the custom hold actions, cleaning this PR up is a good thing, so if you organize your commits in a way that makes it easy to merge cleanups/refactors that don't actually change the behaviour, I'd be interested in merging those. You can send PRs to my branch (though note I'm using a rebase workflow). |
You seem to view AutoShift as a way to automate the shift layer, and elsewhere also describe issues with it being "half-active". I want it to remove the shift layer, and simply be a fast way to make every key have a different hold action (otherwise you have to use an advanced tapdance function [and, even if there was a better way, either lots of defines or XX(KC_X, LSFT(KC_X)) for every key]). It's just that the obvious second value for typing keys are their capital versions, and so the old shift layer's value for that key is the default. Describing new ones is necessary, on a small board exclamation point as period's second value makes way more sense than carat. Also, I couldn't get the tap hold to repeat the unshifted character even with TAPPING_TERM over 1s. Don't see why it isn't working for me, the code flow looks correct. EDIT: Fixed in cleanup as many key event paths were changed but still not sure what was causing it. Only an issue if cleanup isn't merged at all I suppose. |
} | ||
autoshift_flush(record->event.time); | ||
} else if (record->event.pressed) { | ||
if (keycode == KC_LSFT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs right shift added if my cleanup isn't merged.
if (keycode == KC_LSFT) { | |
if (keycode == KC_LSFT || keycode == KC_RSFT) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The auto-shifting here only ever holds down LSFT. If the user presses RSFT, it shouldn't interfere with auto-shift and can just be processed normally.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that holding LSFT, repeating a key, and releasing shift would unshift while RSFT would not. I don't know why, but I just tested and that's not the case. Tapping either while repeating shifted also unshifts. You're right, not an issue.
p00ya#1 For the latter there are a few options.
Thoughts? |
Well, yes, I think automating the shift layer is strongly implied by the name "AutoShift"! ;) Have you considered rebranding "custom auto shift" as "custom hold actions" or similar?
Per my philosophy above, my personal preference would be per-key release, which probably means more state to track.
You can hack around that the same way custom macros work: allocate a new keycode, and have a table that expands that to whatever you need (just a struct with 2 keycodes?). One nice feature of this is that you can have two keys with the same keycode for the "tap" action but different keycodes for the "hold" action.
I think a user-defined function (perhaps with a predefined one respecting AUTO_SHIFT_ALPHAS etc. for compatibility) would be clean and pragmatic. Though I do still think the custom hold actions are something that more properly belongs in tap dance, e.g. if it the hold was part of Probably out of scope but worth mentioning: the really advanced option is to hook right into |
I know, it felt wrong as I was writing it (how can you misinterpret 'AutoShift'?) but really, what is AS except a single-pattern from tap dance with defaults? There should be support for the most basic of configuration, especially since AS+TD is a good chunk of your available space and doing more than a few custom through TD pollutes the keymap so recreating AS isn't practical.
I'm planning on leaving the current I get that this is going deep into TapDance territory, but that's all AutoShift is. Heck, it probably would have been a good idea to make AutoShift just enable TD and create a bunch of advanced functions for whatever AUTO_SHIFT_XXXX are defined (isn't TD smaller firmware size even before this pull?) (EDIT: Not viable now as keyrepeat wouldn't work, but initially). I'll code this up tomorrow, if I don't hit any roadblocks I think it could be pretty clean. |
This should no longer be a breaking change due to to flag changes, aside from the firmware size increase. An option to get AS to return true for later key processing is having a constant custom keycode and editing the record to be it on any AS key down/up. Not sure where it would change anything that can be used parallel to AS, but could be useful later. That would allow other things to see that they shouldn't handle the key (how would they?) but know that the physical event happened. |
This needs to be rebased upon |
Sorry; need another rebase/conflict fix here. There was another PR for Auto Shift that got merged. |
f9d52d6
to
339fe5b
Compare
@noroadsleft ping for merge before you force-push develop again |
Manual testing has shown event.time is not reliable on some keyboards. Perform a timer_read() instead.
94a59d7
to
4c7f736
Compare
Please test again. I've kept the |
Everything's still good, same results I had before. |
Thanks! |
* Branch point for 2020 November 28 Breaking Change * Remove matrix_col_t to allow MATRIX_ROWS > 32 (#10183) * Add support for soft serial to ATmega32U2 (#10204) * Change MIDI velocity implementation to allow direct control of velocity value (#9940) * Add ability to build a subset of all keyboards based on platform. * Actually use eeprom_driver_init(). * Make bootloader_jump weak for ChibiOS. (#10417) * Joystick 16-bit support (#10439) * Per-encoder resolutions (#10259) * Share button state from mousekey to pointing_device (#10179) * Add hotfix for chibios keyboards not wake (#10088) * Add advanced/efficient RGB Matrix Indicators (#8564) * Naming change. * Support for STM32 GPIOF,G,H,I,J,K (#10206) * Add milc as a dependency and remove the installed milc (#10563) * ChibiOS upgrade: early init conversions (#10214) * ChibiOS upgrade: configuration file migrator (#9952) * Haptic and solenoid cleanup (#9700) * XD75 cleanup (#10524) * OLED display update interval support (#10388) * Add definition based on currently-selected serial driver. (#10716) * New feature: Retro Tapping per key (#10622) * Allow for modification of output RGB values when using rgblight/rgb_matrix. (#10638) * Add housekeeping task callbacks so that keyboards/keymaps are capable of executing code for each main loop iteration. (#10530) * Rescale both ChibiOS and AVR backlighting. * Reduce Helix keyboard build variation (#8669) * Minor change to behavior allowing display updates to continue between task ticks (#10750) * Some GPIO manipulations in matrix.c change to atomic. (#10491) * qmk cformat (#10767) * [Keyboard] Update the Speedo firmware for v3.0 (#10657) * Maartenwut/Maarten namechange to evyd13/Evy (#10274) * [quantum] combine repeated lines of code (#10837) * Add step sequencer feature (#9703) * aeboards/ext65 refactor (#10820) * Refactor xelus/dawn60 for Rev2 later (#10584) * add DEBUG_MATRIX_SCAN_RATE_ENABLE to common_features.mk (#10824) * [Core] Added `add_oneshot_mods` & `del_oneshot_mods` (#10549) * update chibios os usb for the otg driver (#8893) * Remove HD44780 References, Part 4 (#10735) * [Keyboard] Add Valor FRL TKL (+refactor) (#10512) * Fix cursor position bug in oled_write_raw functions (#10800) * Fixup version.h writing when using SKIP_VERSION=yes (#10972) * Allow for certain code in the codebase assuming length of string. (#10974) * Add AT90USB support for serial.c (#10706) * Auto shift: support repeats and early registration (#9826) * Rename ledmatrix.h to match .c file (#7949) * Split RGB_MATRIX_ENABLE into _ENABLE and _DRIVER (#10231) * Split LED_MATRIX_ENABLE into _ENABLE and _DRIVER (#10840) * Merge point for 2020 Nov 28 Breaking Change
* Branch point for 2020 November 28 Breaking Change * Remove matrix_col_t to allow MATRIX_ROWS > 32 (qmk#10183) * Add support for soft serial to ATmega32U2 (qmk#10204) * Change MIDI velocity implementation to allow direct control of velocity value (qmk#9940) * Add ability to build a subset of all keyboards based on platform. * Actually use eeprom_driver_init(). * Make bootloader_jump weak for ChibiOS. (qmk#10417) * Joystick 16-bit support (qmk#10439) * Per-encoder resolutions (qmk#10259) * Share button state from mousekey to pointing_device (qmk#10179) * Add hotfix for chibios keyboards not wake (qmk#10088) * Add advanced/efficient RGB Matrix Indicators (qmk#8564) * Naming change. * Support for STM32 GPIOF,G,H,I,J,K (qmk#10206) * Add milc as a dependency and remove the installed milc (qmk#10563) * ChibiOS upgrade: early init conversions (qmk#10214) * ChibiOS upgrade: configuration file migrator (qmk#9952) * Haptic and solenoid cleanup (qmk#9700) * XD75 cleanup (qmk#10524) * OLED display update interval support (qmk#10388) * Add definition based on currently-selected serial driver. (qmk#10716) * New feature: Retro Tapping per key (qmk#10622) * Allow for modification of output RGB values when using rgblight/rgb_matrix. (qmk#10638) * Add housekeeping task callbacks so that keyboards/keymaps are capable of executing code for each main loop iteration. (qmk#10530) * Rescale both ChibiOS and AVR backlighting. * Reduce Helix keyboard build variation (qmk#8669) * Minor change to behavior allowing display updates to continue between task ticks (qmk#10750) * Some GPIO manipulations in matrix.c change to atomic. (qmk#10491) * qmk cformat (qmk#10767) * [Keyboard] Update the Speedo firmware for v3.0 (qmk#10657) * Maartenwut/Maarten namechange to evyd13/Evy (qmk#10274) * [quantum] combine repeated lines of code (qmk#10837) * Add step sequencer feature (qmk#9703) * aeboards/ext65 refactor (qmk#10820) * Refactor xelus/dawn60 for Rev2 later (qmk#10584) * add DEBUG_MATRIX_SCAN_RATE_ENABLE to common_features.mk (qmk#10824) * [Core] Added `add_oneshot_mods` & `del_oneshot_mods` (qmk#10549) * update chibios os usb for the otg driver (qmk#8893) * Remove HD44780 References, Part 4 (qmk#10735) * [Keyboard] Add Valor FRL TKL (+refactor) (qmk#10512) * Fix cursor position bug in oled_write_raw functions (qmk#10800) * Fixup version.h writing when using SKIP_VERSION=yes (qmk#10972) * Allow for certain code in the codebase assuming length of string. (qmk#10974) * Add AT90USB support for serial.c (qmk#10706) * Auto shift: support repeats and early registration (qmk#9826) * Rename ledmatrix.h to match .c file (qmk#7949) * Split RGB_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10231) * Split LED_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10840) * Merge point for 2020 Nov 28 Breaking Change
* Branch point for 2020 November 28 Breaking Change * Remove matrix_col_t to allow MATRIX_ROWS > 32 (qmk#10183) * Add support for soft serial to ATmega32U2 (qmk#10204) * Change MIDI velocity implementation to allow direct control of velocity value (qmk#9940) * Add ability to build a subset of all keyboards based on platform. * Actually use eeprom_driver_init(). * Make bootloader_jump weak for ChibiOS. (qmk#10417) * Joystick 16-bit support (qmk#10439) * Per-encoder resolutions (qmk#10259) * Share button state from mousekey to pointing_device (qmk#10179) * Add hotfix for chibios keyboards not wake (qmk#10088) * Add advanced/efficient RGB Matrix Indicators (qmk#8564) * Naming change. * Support for STM32 GPIOF,G,H,I,J,K (qmk#10206) * Add milc as a dependency and remove the installed milc (qmk#10563) * ChibiOS upgrade: early init conversions (qmk#10214) * ChibiOS upgrade: configuration file migrator (qmk#9952) * Haptic and solenoid cleanup (qmk#9700) * XD75 cleanup (qmk#10524) * OLED display update interval support (qmk#10388) * Add definition based on currently-selected serial driver. (qmk#10716) * New feature: Retro Tapping per key (qmk#10622) * Allow for modification of output RGB values when using rgblight/rgb_matrix. (qmk#10638) * Add housekeeping task callbacks so that keyboards/keymaps are capable of executing code for each main loop iteration. (qmk#10530) * Rescale both ChibiOS and AVR backlighting. * Reduce Helix keyboard build variation (qmk#8669) * Minor change to behavior allowing display updates to continue between task ticks (qmk#10750) * Some GPIO manipulations in matrix.c change to atomic. (qmk#10491) * qmk cformat (qmk#10767) * [Keyboard] Update the Speedo firmware for v3.0 (qmk#10657) * Maartenwut/Maarten namechange to evyd13/Evy (qmk#10274) * [quantum] combine repeated lines of code (qmk#10837) * Add step sequencer feature (qmk#9703) * aeboards/ext65 refactor (qmk#10820) * Refactor xelus/dawn60 for Rev2 later (qmk#10584) * add DEBUG_MATRIX_SCAN_RATE_ENABLE to common_features.mk (qmk#10824) * [Core] Added `add_oneshot_mods` & `del_oneshot_mods` (qmk#10549) * update chibios os usb for the otg driver (qmk#8893) * Remove HD44780 References, Part 4 (qmk#10735) * [Keyboard] Add Valor FRL TKL (+refactor) (qmk#10512) * Fix cursor position bug in oled_write_raw functions (qmk#10800) * Fixup version.h writing when using SKIP_VERSION=yes (qmk#10972) * Allow for certain code in the codebase assuming length of string. (qmk#10974) * Add AT90USB support for serial.c (qmk#10706) * Auto shift: support repeats and early registration (qmk#9826) * Rename ledmatrix.h to match .c file (qmk#7949) * Split RGB_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10231) * Split LED_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10840) * Merge point for 2020 Nov 28 Breaking Change
Referenced in process_auto_shift.c: qmk#9826 (comment)
* Branch point for 2020 November 28 Breaking Change * Remove matrix_col_t to allow MATRIX_ROWS > 32 (qmk#10183) * Add support for soft serial to ATmega32U2 (qmk#10204) * Change MIDI velocity implementation to allow direct control of velocity value (qmk#9940) * Add ability to build a subset of all keyboards based on platform. * Actually use eeprom_driver_init(). * Make bootloader_jump weak for ChibiOS. (qmk#10417) * Joystick 16-bit support (qmk#10439) * Per-encoder resolutions (qmk#10259) * Share button state from mousekey to pointing_device (qmk#10179) * Add hotfix for chibios keyboards not wake (qmk#10088) * Add advanced/efficient RGB Matrix Indicators (qmk#8564) * Naming change. * Support for STM32 GPIOF,G,H,I,J,K (qmk#10206) * Add milc as a dependency and remove the installed milc (qmk#10563) * ChibiOS upgrade: early init conversions (qmk#10214) * ChibiOS upgrade: configuration file migrator (qmk#9952) * Haptic and solenoid cleanup (qmk#9700) * XD75 cleanup (qmk#10524) * OLED display update interval support (qmk#10388) * Add definition based on currently-selected serial driver. (qmk#10716) * New feature: Retro Tapping per key (qmk#10622) * Allow for modification of output RGB values when using rgblight/rgb_matrix. (qmk#10638) * Add housekeeping task callbacks so that keyboards/keymaps are capable of executing code for each main loop iteration. (qmk#10530) * Rescale both ChibiOS and AVR backlighting. * Reduce Helix keyboard build variation (qmk#8669) * Minor change to behavior allowing display updates to continue between task ticks (qmk#10750) * Some GPIO manipulations in matrix.c change to atomic. (qmk#10491) * qmk cformat (qmk#10767) * [Keyboard] Update the Speedo firmware for v3.0 (qmk#10657) * Maartenwut/Maarten namechange to evyd13/Evy (qmk#10274) * [quantum] combine repeated lines of code (qmk#10837) * Add step sequencer feature (qmk#9703) * aeboards/ext65 refactor (qmk#10820) * Refactor xelus/dawn60 for Rev2 later (qmk#10584) * add DEBUG_MATRIX_SCAN_RATE_ENABLE to common_features.mk (qmk#10824) * [Core] Added `add_oneshot_mods` & `del_oneshot_mods` (qmk#10549) * update chibios os usb for the otg driver (qmk#8893) * Remove HD44780 References, Part 4 (qmk#10735) * [Keyboard] Add Valor FRL TKL (+refactor) (qmk#10512) * Fix cursor position bug in oled_write_raw functions (qmk#10800) * Fixup version.h writing when using SKIP_VERSION=yes (qmk#10972) * Allow for certain code in the codebase assuming length of string. (qmk#10974) * Add AT90USB support for serial.c (qmk#10706) * Auto shift: support repeats and early registration (qmk#9826) * Rename ledmatrix.h to match .c file (qmk#7949) * Split RGB_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10231) * Split LED_MATRIX_ENABLE into _ENABLE and _DRIVER (qmk#10840) * Merge point for 2020 Nov 28 Breaking Change
Fixes qmk#7048. Co-authored-by: IsaacElenbaas <isaacelenbaas@gmail.com>
Description
Instead of waiting for the next record to tap the autoshiftable key, register a matrix_scan function that will check the timeout and register the key immediately if the timeout period has elapsed. This means the user gets immediate feedback about their press and doesn't have to guess whether it's been long enough.
Additionally, don't unregister the keypress immediately. This means that auto-shifted keys can be held down.
Types of Changes
Issues Fixed or Closed by This PR
Checklist
I've tested with
AUTO_SHIFT_ENABLE
,NO_AUTO_SHIFT_ALPHA
on an Ergodox EZ, with taps, holds, and tap-then-holds, and rollovers to autoshiftable and non-autoshiftable keys. There are a lot of edge cases.