qmk / qmk_firmware

Open-source keyboard firmware for Atmel AVR and Arm USB families
https://qmk.fm
GNU General Public License v2.0
18.02k stars 38.73k forks source link

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

Open Roming22 opened 1 year ago

Roming22 commented 1 year ago

Describe the Bug

Keyboard configuration available here

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

Other keyboard-related software installed

None

Additional Context

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

sigprof commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 https://github.com/qmk/qmk_firmware/pull/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 commented 1 year ago

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 commented 1 year ago

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 commented 10 months ago

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!