HaoboGu / rmk

Rust keyboard firmware library with layers, macros, real-time keymap editing, wireless(BLE) and split support
https://haobogu.github.io/rmk/
Apache License 2.0
568 stars 36 forks source link

Tap hold implementation issue #124

Open jackbrad1ey opened 1 day ago

jackbrad1ey commented 1 day ago

I've noticed there's a slight issue in the logic for the tap hold implementation. Specifically, relating to this section of code in the process_key_action_tap_hold function:

                embassy_futures::select::Either::Second(e) => {
                    if e.row == key_event.row && e.col == key_event.col {
                        // If it's same key event and releasing within 200ms, trigger tap
                        if !e.pressed {
                            let elapsed = self.timer[col][row].unwrap().elapsed().as_millis();
                            debug!("TAP action: {}, time elapsed: {}ms", tap_action, elapsed);
                            self.process_key_action_tap(tap_action, key_event).await;

                            // Clear timer
                            self.timer[col][row] = None;
                        }
                    } else {
                        // A different key comes within the threshold, trigger hold + that key

                        // Process hold action first
                        self.process_key_action_normal(hold_action, key_event).await;

                        // The actual processing is postponed because we cannot do recursion on async function without alloc
                        // After current key processing is done, we can process events in queue until the queue is empty
                        if self.unprocessed_events.push(e).is_err() {
                            warn!("unprocessed event queue is full, dropping event");
                        }
                    }
                }

Correct me if I'm wrong, but the else here can also be triggered by a key release event? What I've noticed is that if I'm typing fast, the tap of a tap hold key fails to trigger, this is because I press the tap hold key before releasing another key, so when I then release the other key it causes the code to go into this else condition, and then when I release the tap hold key (expecting the tap action to occur) it doesn't do anything since the else triggered the hold action.

I'm not quite familiar enough with Rust to make a PR with a fix, but in my head it sounds like what we'd want to do is loop until we timeout or get a keypress event. If we get a key release event we should just process that action normally and then go back to waiting for a timeout or keypress.

HaoboGu commented 1 day ago

Yes, you're right. There are some issues on current implementation of tap_hold. But processing keypress only might not be enough(see #118).

RMK implements key scanning and processing quite different from qmk/zmk. I want to find a better default config that is good for home row mods. If you have any idea, please share to me! 😄