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

[Bug] Fast typing can drop characters, or not output the expect char #20318

Open
2 tasks
Roming22 opened this issue Apr 2, 2023 · 7 comments
Open
2 tasks

[Bug] Fast typing can drop characters, or not output the expect char #20318

Roming22 opened this issue Apr 2, 2023 · 7 comments

Comments

@Roming22
Copy link

Roming22 commented Apr 2, 2023

Describe the Bug

Keyboard configuration available here

  • Fast typing issue: fast roll 'h','i' returns 'i' instead of 'hi'. The 'H' key is not triggered at all.
  • Fast typing issue: fast 'i' + ('t'+'a') returns 'f' instead of '['. A fast 'i'+'t' will return a symbol from layer1 as expected. However when a combo is involved, it returns the combo from layer0.
  • Fast typing issue: fast 'i'+'t' will sometimes return '#' as expected, but may sometimes return '3'.

Keyboard Used

No response

Link to product page (if applicable)

https://shop.beekeeb.com/product/pre-soldered-hillside-keyboard/

Operating System

Linux (Fedora 37)

qmk doctor Output

Ψ QMK Doctor is checking your environment.
Ψ CLI version: 1.1.1
Ψ QMK home: /workspaces/firmware
Ψ Detected Linux (Fedora Linux 37 (Container Image)).
Ψ Git branch: master
Ψ Repo version: 0.20.3
⚠ Git has unstashed/uncommitted changes.
Ψ - Latest master: 2023-03-31 08:41:11 +1100 (c3c401f91d) -- [QP] Fix up delta frame boundaries (#20296)
Ψ - Latest upstream/master: 2023-04-02 03:57:54 -0500 (be9f6e679b) -- [Keymap] update arkag keymap, add hitbox layout (#20271)
Ψ - Latest upstream/develop: None
Ψ - Common ancestor with upstream/master: 2023-03-31 08:41:11 +1100 (c3c401f91d) -- [QP] Fix up delta frame boundaries (#20296)
Ψ - Common ancestor with upstream/develop: None
Ψ All dependencies are installed.
Ψ Found arm-none-eabi-gcc version 12.2.0
Ψ Found avr-gcc version 12.1.0
⚠ We do not recommend avr-gcc newer than 8. Downgrading to 8.x is recommended.
Ψ Found avrdude version 6.4
Ψ Found dfu-programmer version 0.7.2
Ψ Found dfu-util version 0.11
Ψ Submodules are up to date.
Ψ Submodule status:
Ψ - lib/chibios: 2023-01-03 19:29:26 +0000 --  (0062927e3)
Ψ - lib/chibios-contrib: 2023-01-11 16:42:27 +0100 --  (a224be15)
Ψ - lib/googletest: 2021-06-11 06:37:43 -0700 --  (e2239ee6)
Ψ - lib/lufa: 2022-08-26 12:09:55 +1000 --  (549b97320)
Ψ - lib/vusb: 2022-06-13 09:18:17 +1000 --  (819dbc1)
Ψ - lib/printf: 2022-06-29 23:59:58 +0300 --  (c2e3b4e)
Ψ - lib/pico-sdk: 2023-02-12 20:19:37 +0100 --  (a3398d8)
Ψ - lib/lvgl: 2022-04-11 04:44:53 -0600 --  (e19410f8)
Ψ QMK is ready to go, but minor problems were found

Is AutoHotKey / Karabiner installed

  • AutoHotKey (Windows)
  • Karabiner (macOS)

Other keyboard-related software installed

None

Additional Context

Board: Hillside46
MCU: Sea-picro controller (RP2040).

@sigprof
Copy link
Contributor

sigprof commented Apr 2, 2023

That's one really complicated keymap…

Fast typing issue: fast 'i' + ('t'+'a') returns 'f' instead of '['. A fast 'i'+'t' will return a symbol from layer1 as expected. However when a combo is involved, it returns the combo from layer0.

This might not be fixable with the current combo implementation. The order of key event processing is as follows:

  1. process_combo() — combos are resolved here; key events involved in combos are buffered until the combo term expires, then either the original events or the combo results are sent to the next step.
  2. action_tapping_process() — dual role keys like LT() or MT() are resolved here; key events are buffered until the action can be resolved as a tap or a hold.
  3. process_record() — here the key events are actually translated to the corresponding action (send some key events to the host, switch layers, …).

Your i key is apparently #define RHI0 LT(1, KC_I) (and it's also involved in some combos), and you have PERMISSIVE_HOLD defined, so you may expect that quickly pressing i, then pressing and releasing another key, then releasing i would result in the i key actually switching the layer to 1 even if you perform that action faster than the tapping term. However, if you press the t and a keys quickly, the combo code looks up the keycodes for your t and a keys before the LT(1, KC_I) key has been processed, therefore it gets the keycodes from layer 0 and uses the corresponding combo; subsequent action_tapping_process() gets just the combo lookup result of KC_F, and even though LT(1, KC_I) gets processed before that KC_F and switches the layer, KC_F remains unchanged, because it's no longer a normal key event with a matrix row and column, but a combo result with a fixed keycode.

Using MO(1) on a dedicated key would avoid the problem (at least if it's not part of another combo, or if you always press the next key after the combo term for MO(1) had expired).

Alternatively, you may try using #define COMBO_ONLY_FROM_LAYER 0, defining all your combos based on layer 0 to return custom keycodes, and then handling the layer switching for those custom keycodes yourself:

    case COMBO_1:
        static uint8_t combo_1_layer;
        if (record->event.pressed) {
            combo_1_layer = get_highest_layer(layer_state);
        }
        if (combo_1_layer == 0) {
            // ... handle press/release on layer 0
        } else if (combo_1_layer == 1) {
            // ... handle press/release on layer 1
        }
        break;

You need to save the layer state on press separately for each such keycode, so that the release action would match the press action even if the layer state had changed in the meantime. This essentially duplicates the builtin keymap lookup functionality and the source layer cache, but is apparently the only way to apply the layer state set by LT() to combos.

(Instead of using #define COMBO_ONLY_FROM_LAYER 0, you may try to define custom keycodes only for combos from layer 0 keys, so that you can detect that a switch to layer 1 had happened and handle them as layer 1 equivalents in that case; however, there would be some chance that the tapping term expires between the keypresses in the combo — in that case the combo code would see a mix of keycodes from layer 0 and layer 1 and won't be able to build a combo from that.)

@sigprof
Copy link
Contributor

sigprof commented Apr 2, 2023

Fast typing issue: fast 'i'+'t' will sometimes return '#' as expected, but may sometimes return '3'.

This might be the issue with dropped modifiers on keycodes like KC_HASH (which is actually S(KC_3)) which happens on some configurations (mostly Windows RDP/Hyper-V and Linux with Wayland). There were various attempts to fix that problem; one of them is #19449, but I have no idea why this approach is reported to work, while just adding a delay does not.

@sigprof
Copy link
Contributor

sigprof commented Apr 2, 2023

Fast typing issue: fast roll 'h','i' returns 'i' instead of 'hi'. The 'H' key is not triggered at all.

Just to be sure, this issue involves these keys:

#define RTR0 RSFT_T(KC_H)
#define RHI0 LT(1, KC_I)

and you are sure to press h, press i, release h, release i in exactly that order? (A delayed release of h should give a capital I in this case due to PERMISSIVE_HOLD.)

Both of those keycodes are also involved in some combos (but there is no combo that includes both of them):

const uint16_t PROGMEM RTM0_RTR0_combo[] = {RTR0, RTM0, COMBO_END};            // KC_Q
const uint16_t PROGMEM RTI0_RTR0_combo[] = {RTR0, RTI0, COMBO_END};            // KC_Z
const uint16_t PROGMEM RTI0_RTM0_RTR0_combo[] = {RTR0, RTM0, RTI0, COMBO_END}; // KC_BACKSPACE

const uint16_t PROGMEM RHI0_RHM0_combo[] = {RHM0, RHI0, COMBO_END};            // KC_Y
const uint16_t PROGMEM RHI0_RHR0_combo[] = {RHR0, RHI0, COMBO_END};            // KC_M
const uint16_t PROGMEM RHI0_RHM0_RHR0_combo[] = {RHR0, RHM0, RHI0, COMBO_END}; // KC_B

@Roming22
Copy link
Author

Roming22 commented Apr 2, 2023

Your i key is apparently #define RHI0 LT(1, KC_I) (and it's also involved in some combos), and you have PERMISSIVE_HOLD defined, so you may expect that quickly pressing i, then pressing and releasing another key, then releasing i would result in the i key actually switching the layer to 1 even if you perform that action faster than the tapping term.

Correct, that's what I would expect. To be precise, Because of PERMISSIVE_HOLD, I would expect that any key press while RHI0 is pressed would first switch to layer 1, no matter if it's a single key press or part of a combo. The single key press is working fine, the combo is not. Any chance that resolution of layer changes could be split from action_tapping_process and resolved first?

This might be the issue with dropped modifiers on keycodes like KC_HASH (which is actually S(KC_3)) which happens on some configurations (mostly Windows RDP/Hyper-V and Linux with Wayland). There were various attempts to fix that problem; one of them is #19449, but I have no idea why this approach is reported to work, while just adding a delay does not.

Thanks. I'll wait for the PR to be merged and will report again if it isn't solved.
Update: I've replaced action_utils.c with the patched version, and still see the behavior.

You are sure to press h, press i, release h, release i in exactly that order? (A delayed release of h should give a capital I in this case due to PERMISSIVE_HOLD.)

It's a fast roll. I can't be 100% sure, but what's is certain is that I always get i and never I nor hi no matter how many time I try with a full speed roll. I can reliably get I or hi if I slow down my input.

@sigprof
Copy link
Contributor

sigprof commented Apr 2, 2023

Any chance that resolution of layer changes could be split from action_tapping_process and resolved first?

This seems to be fundamentally impossible, because with PERMISSIVE_HOLD you would need to go back in time — if you have a fast sequence of events like A↓—B↓—B↑—A↑, the hold of A can be detected only at the B↑ moment, but if B is actually a combo, you would need to have that information available at the B↓ moment (or, more precisely, at the moment when the first key of the combo is pressed). Note that if the sequence ends up being A↓—B↓—A↑—B↑, it would need to be handled as a tap of A (so there would be no layer switch, and the B combo would need to be looked up on layer 0), but the sequence of events at the B↓ moment would be exactly identical, so it's impossible to make the combo lookup depend on the tap/hold decision for A.

If you have a limited number of combos which are problematic in that way, you may just replace the outputs of those combos on layer 0 with custom keycodes and then check the layer state to choose the proper action. This would still fail if some combo on layer 1 does not and must not have an equivalent combo on layer 0 - in that case those keys would be handled separately, but their mappings would be taken from layer 1.

@Roming22
Copy link
Author

Roming22 commented Apr 2, 2023

I don't know about QMK internal, but I was expecting a buffer for the inputs, and a decision made once enough information is known.

Apparently that is not the case. Some actions are taken while the input is still undetermined.

I have probably about a dozen combos. I suppose I can try declaring a combo for each combination.

@TristanCacqueray
Copy link

TristanCacqueray commented Nov 10, 2023

Fast typing issue: fast 'i'+'t' will sometimes return '#' as expected, but may sometimes return '3'.

This might be the issue with dropped modifiers on keycodes like KC_HASH (which is actually S(KC_3)) which happens on some configurations (mostly Windows RDP/Hyper-V and Linux with Wayland). There were various attempts to fix that problem; one of them is #19449, but I have no idea why this approach is reported to work, while just adding a delay does not.

That's good to know, I am having this exact same issue, as well as 0 being sent instead of ).
Pressing shift before the last key seems to prevent the issue.

#19449 didn't solved that issue, but #19405 appears to work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants