Key press sometimes gets stuck

[covanam]

The issue

• Sometimes, after pressing a key and releasing it; RVVM sees the key press event, but not the key release event. In other words, RVVM behaves as if the key is held indefinitely.

Steps to reproduce

• Use these images: images
• Run rvvm_x86_64 -smp 4 -res 1920x1080 fw_jump.bin -k Image -i rootfs.ext2 (Linux, X11)
• After booting, hold any key for ~ 1 second and release (for example, 'a'). Occasionally RVVM never sees the release (and 'a' continuously appears in the terminal)

Investigation

• Git bisect: 5d20e7c
• It seems like RVVM misses the X11's key release event

Workarounds

• Press another key and the problem disappears.

[LekKit]

Can't reproduce on a KDE Plasma X11 system, what is your host setup?
It is definitely possible that it has been broken by threading X11 backend rewrite.
Please also note there is code that is filtering key autorepeat events, which drops autorepeat key release events.

I've attached a small patch that removes autorepeat event filtering altogether and logs x11 events / key press / key release. Please report whether it possibly fixes the issue, or that the repeat event is not logged at all - which then would mean some Xlib handling issues.

diff --git a/src/devices/x11window_xlib.c b/src/devices/x11window_xlib.c
index 05bd7ea..7c93c01 100644
--- a/src/devices/x11window_xlib.c
+++ b/src/devices/x11window_xlib.c
@@ -476,6 +476,7 @@ static void x11_dispatch_events(void)
     while ((pending = XPending(x11_dsp))) {
         XEvent ev = {0};
         XNextEvent(x11_dsp, &ev);
+        rvvm_warn("x11 event %x", ev.type);
 
         switch (ev.type) {
             case ButtonPress:
@@ -488,17 +489,11 @@ static void x11_dispatch_events(void)
                 x11_handle_mouse_motion(&ev.xmotion);
                 break;
             case KeyPress:
+                rvvm_warn("x11 press   %x", ev.xkey.keycode);
                 x11_keyboard_press(&ev.xkey);
                 break;
             case KeyRelease:
-                if (pending > 1) {
-                    XEvent tmp = {0};
-                    XPeekEvent(x11_dsp, &tmp);
-                    if (tmp.type == KeyPress && tmp.xkey.time == ev.xkey.time && tmp.xkey.keycode == ev.xkey.keycode) {
-                        // Skip the repeated key release event, repeated presses are filtered by hid_keyboard
-                        break;
-                    }
-                }
+                rvvm_warn("x11 release %x", ev.xkey.keycode);
                 x11_keyboard_release(&ev.xkey);
                 break;
             case MappingNotify:

[covanam]

Can't reproduce on a KDE Plasma X11 system, what is your host setup?

Debian 12, Cinnamon DE.

It is definitely possible that it has been broken by threading X11 backend rewrite. Please also note there is code that is filtering key autorepeat events, which drops autorepeat key release events.

I've attached a small patch that removes autorepeat event filtering altogether and logs x11 events / key press / key release. Please report whether it possibly fixes the issue, or that the repeat event is not logged at all - which then would mean some Xlib handling issues.

The problem still appears with this patch. Here's the debug log:

WARN: x11 event 2
WARN: x11 press   26
WARN: x11 event 3
WARN: x11 release 26
WARN: x11 event 2
WARN: x11 press   26
# at this point, 'A' gets stuck, it behaves as if I haven't released 'A'
# then, I pressed 'B' and the problem disappears
WARN: x11 event 3
WARN: x11 release 26
WARN: x11 event 2
WARN: x11 press   38
WARN: x11 event 3
WARN: x11 release 38
# I close the window
WARN: x11 event 21

Looking at this log, one may conclude that the X11 release event is somehow lost: WARN: x11 release 26 doesn't appear until after I press 'B'.

However, interestingly, sometimes I got the following debug log, and WARN: x11 release 26 does appear before 'A' gets stuck:

WARN: x11 event 2
WARN: x11 press   26
WARN: x11 event 3
WARN: x11 release 26
WARN: x11 event 2
WARN: x11 press   26
WARN: x11 event 3
WARN: x11 release 26
WARN: x11 event 2
WARN: x11 press   26
WARN: x11 event 3
WARN: x11 release 26
# 'A' gets stuck here, but this time we do see X11 release
# I then pressed 'B'. Then 'B' gets stuck. I pressed 'B' 2 more times, and the problem disappears.
WARN: x11 event 2
WARN: x11 press   38
WARN: x11 event 3
WARN: x11 release 38
WARN: x11 event 2
WARN: x11 press   38
WARN: x11 event 3
WARN: x11 release 38
WARN: x11 event 2
WARN: x11 press   38
WARN: x11 event 3
WARN: x11 release 38
# close the window
WARN: x11 event 21

Note that the second debug log is harder to reproduce, compared to the first debug log.

It is interesting to note that in both cases, I only pressed 'A' key exactly once, but multiple press/release X11 events appear. Maybe due to the "bouncing button problem"?

I followed the advice from this site, I tried the following patch:

diff --git a/src/devices/x11window_xlib.c b/src/devices/x11window_xlib.c
index 05bd7ea..80b6e8e 100644
--- a/src/devices/x11window_xlib.c
+++ b/src/devices/x11window_xlib.c
@@ -537,7 +537,7 @@ static void* x11_event_thread(void* arg)
     while (running) {
         FD_ZERO(&fds);
         FD_SET(fd, &fds);
-        if (select(fd + 1, &fds, NULL, NULL, NULL) > 0) {
+        if (XPending(x11_dsp) ||  select(fd + 1, &fds, NULL, NULL, NULL) > 0) {
             spin_lock(&x11_lock);
             if (vector_size(x11_windows)) {
                 x11_dispatch_events();

unfortunately the problem still appears.

[covanam]

I added a few more debug printing:

diff --git a/src/devices/hid-keyboard.c b/src/devices/hid-keyboard.c
index f6be259..df65974 100644
--- a/src/devices/hid-keyboard.c
+++ b/src/devices/hid-keyboard.c
@@ -99,6 +99,7 @@ static void hid_keyboard_read_report(void* dev, uint8_t report_type, uint8_t rep
     spin_lock(&kb->lock);
     if (report_type == REPORT_TYPE_INPUT) {
         if (offset == 0) {
+            //
             kb->input_report[0] = bit_cut(sizeof(kb->input_report), 0, 8);
             kb->input_report[1] = bit_cut(sizeof(kb->input_report), 8, 8);
             kb->input_report[2] = bit_cut(atomic_load_uint32(&kb->keys_pressed[7])
@@ -170,7 +171,8 @@ PUBLIC void hid_keyboard_press(hid_keyboard_t* kb, hid_key_t key)
         uint32_t off = key >> 5;
         uint32_t bit = 1U << (key & 0x1F);
         uint32_t old_pressed = atomic_or_uint32(&kb->keys_pressed_now[off], bit);
-
+        uint32_t new_pressed = atomic_load_uint32_ex(&kb->keys_pressed_now[off], ATOMIC_ACQUIRE);
+        rvvm_warn("hid key press old=%x, new=%x", old_pressed, new_pressed);
         // Send input_available if it's an actual key press
         if (bit & ~old_pressed) {
             // Mark that this key was pressed, prevents loosing keypresses
@@ -186,7 +188,9 @@ PUBLIC void hid_keyboard_release(hid_keyboard_t* kb, hid_key_t key)
         uint32_t off = key >> 5;
         uint32_t bit = 1U << (key & 0x1F);
         uint32_t old_pressed = atomic_and_uint32(&kb->keys_pressed_now[off], ~bit);
+        uint32_t new_pressed = atomic_load_uint32_ex(&kb->keys_pressed_now[off], ATOMIC_ACQUIRE);
 
+        rvvm_warn("hid key release old=%x, new=%x", old_pressed, new_pressed);
         // Send input_available if it's an actual key release
         if (old_pressed & bit) {
             kb->hid_dev.input_available(kb->hid_dev.host, 0);
diff --git a/src/devices/x11window_xlib.c b/src/devices/x11window_xlib.c
index 05bd7ea..728dbd5 100644
--- a/src/devices/x11window_xlib.c
+++ b/src/devices/x11window_xlib.c
@@ -476,6 +476,7 @@ static void x11_dispatch_events(void)
     while ((pending = XPending(x11_dsp))) {
         XEvent ev = {0};
         XNextEvent(x11_dsp, &ev);
+        rvvm_warn("x11 event %x", ev.type);
 
         switch (ev.type) {
             case ButtonPress:
@@ -488,17 +489,11 @@ static void x11_dispatch_events(void)
                 x11_handle_mouse_motion(&ev.xmotion);
                 break;
             case KeyPress:
+                rvvm_warn("x11 press   %x", ev.xkey.keycode);
                 x11_keyboard_press(&ev.xkey);
                 break;
             case KeyRelease:
-                if (pending > 1) {
-                    XEvent tmp = {0};
-                    XPeekEvent(x11_dsp, &tmp);
-                    if (tmp.type == KeyPress && tmp.xkey.time == ev.xkey.time && tmp.xkey.keycode == ev.xkey.keycode) {
-                        // Skip the repeated key release event, repeated presses are filtered by hid_keyboard
-                        break;
-                    }
-                }
+                rvvm_warn("x11 release %x", ev.xkey.keycode);
                 x11_keyboard_release(&ev.xkey);
                 break;
             case MappingNotify:
@@ -537,7 +532,7 @@ static void* x11_event_thread(void* arg)
     while (running) {
         FD_ZERO(&fds);
         FD_SET(fd, &fds);
-        if (select(fd + 1, &fds, NULL, NULL, NULL) > 0) {
+        if (XPending(x11_dsp) ||  select(fd + 1, &fds, NULL, NULL, NULL) > 0) {
             spin_lock(&x11_lock);
             if (vector_size(x11_windows)) {
                 x11_dispatch_events();

And also, I print out what Linux kernel HID driver sees:

diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
index 43664a24176f..af11abba45f5 100644
--- a/drivers/hid/i2c-hid/i2c-hid-core.c
+++ b/drivers/hid/i2c-hid/i2c-hid-core.c
@@ -549,7 +549,7 @@ static void i2c_hid_get_input(struct i2c_hid *ihid)
                }
        }
 
-       i2c_hid_dbg(ihid, "input: %*ph\n", ret_size, ihid->inbuf);
+       pr_err("input: %*ph\n", ret_size, ihid->inbuf);
 
        if (test_bit(I2C_HID_STARTED, &ihid->flags)) {
                if (ihid->hid->group != HID_GROUP_RMI)

After an 'A' key press & release:

WARN: x11 event 2
WARN: x11 press   26
WARN: hid key press old=0, new=10
[   22.040931] input: 0a 00 00 00 04 00 00 00 00 00
WARN: x11 event 3
WARN: x11 release 26
WARN: hid key release old=10, new=0
[   22.062908] input: 0a 00 00 00 00 00 00 00 00 00

So, the Linux kernel sees 0a 00 00 00 04 00 00 00 00 00 for key press, and 0a 00 00 00 00 00 00 00 00 00 for key release. Looks correct.

However, look at what happens when 'A' gets stuck, but X11 release event still appears (so, the second debug log case in my previous comment):

WARN: x11 event 2
WARN: x11 press   26
WARN: hid key press old=0, new=10
[   23.323074] input: 0a 00 00 00 04 00 00 00 00 00
WARN: x11 event 3
WARN: x11 release 26
WARN: hid key release old=10, new=0
WARN: x11 event 2
WARN: x11 press   26
WARN: hid key press old=0, new=10
WARN: x11 event 3
WARN: x11 release 26
WARN: hid key release old=10, new=0
[   23.355792] input: 0a 00 00 00 04 00 00 00 00 00

Look at what Linux kernel sees for the key release event: 0a 00 00 00 04 00 00 00 00 00. It sees a key press?

I'm totally confused. Any ideas?

[LekKit]

Reproduced this on a Win32 GUI backend, so it is very likely a HID emulation issue rather than a GUI backend one.