usb: kbd: Fix key repeat not always using

The usb-kbd key repeat code assumes that reports get repeated every 40 ms, this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and does not always works for CONFIG_SYS_USB_EVENT_POLL and CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor the usb_set_idle() command.

For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a blocking wait for the hid report, so if we do not tell the keyboard to send a hid report every 40ms even if nothing changes then we will block u-boot for 1s (the default u-boot usb interrupt packet timeout). Note that in this case on keyboards which do not support usb_set_idle() we loose and we actually get 1s latencies on other u-boot activities.

For the other poll-methods this commit stops using usb_set_idle() and instead repeats the last received hid-report every 40 ms as long as no new hid-report is received. This fixes key-repeat not working at all with CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with keyboards which do not implement usb_set_idle() when using CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.

Signed-off-by Hans de Goede <[email protected]>

---
 common/usb_kbd.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

common/usb_kbd.c +6 -4

index 24a1a56..d4733b1 100644
@@ -31,7 +31,7 @@ int overwrite_console(void)
 #endif
 
 /* Keyboard sampling rate */
-#define REPEAT_RATE    (40 / 4)        /* 40msec -> 25cps */
+#define REPEAT_RATE    40              /* 40msec -> 25cps */
 #define REPEAT_DELAY   10              /* 10 x REPEAT_RATE = 400msec */
 
 #define NUM_LOCK       0x53

@@ -103,6 +103,7 @@ struct usb_kbd_pdata {
        unsigned long   intpipe;
        int             intpktsize;
        int             intinterval;
+       int             last_report;
        struct int_queue *intq;
 
        uint32_t        repeat_delay;

@@ -318,15 +319,16 @@ static inline void usb_kbd_poll_for_event(struct usb_device *dev)
                           data->intinterval);
 
        usb_kbd_irq_worker(dev);
-#elif  defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
+#else
+# if   defined(CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP)
        struct usb_interface *iface;
        struct usb_kbd_pdata *data = dev->privptr;
        iface = &dev->config.if_desc[0];
        usb_get_report(dev, iface->desc.bInterfaceNumber,
                       1, 0, data->new, USB_KBD_BOOT_REPORT_SIZE);
-       if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE))
+       if (memcmp(data->old, data->new, USB_KBD_BOOT_REPORT_SIZE)) {
                usb_kbd_irq_worker(dev);
-#elif  defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
+# elif defined(CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE)
        struct usb_kbd_pdata *data = dev->privptr;
        if (poll_int_queue(dev, data->intq)) {
                usb_kbd_irq_worker(dev);

common/usb_kbd.c +4 -2

        }
 #endif
 }

@@ -445,12 +454,15 @@ static int usb_kbd_probe(struct usb_device *dev, unsigned int ifnum)
        data->intpktsize = min(usb_maxpacket(dev, data->intpipe),
                               USB_KBD_BOOT_REPORT_SIZE);
        data->intinterval = ep->bInterval;
+       data->last_report = -1;
 
        /* We found a USB Keyboard, install it. */
        usb_set_protocol(dev, iface->desc.bInterfaceNumber, 0);
 
+#ifdef CONFIG_SYS_USB_EVENT_POLL
        debug("USB KBD: found set idle...\n");
-       usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE, 0);
+       usb_set_idle(dev, iface->desc.bInterfaceNumber, REPEAT_RATE / 4, 0);
+#endif
 
        debug("USB KBD: enable interrupt pipe...\n");
 #ifdef CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE
--

2.3.6

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

The usb-kbd key repeat code assumes that reports get repeated every 40 ms, this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and does not always works for CONFIG_SYS_USB_EVENT_POLL and CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor the usb_set_idle() command.

For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a blocking wait for the hid report, so if we do not tell the keyboard to send a hid report every 40ms even if nothing changes then we will block u-boot for 1s (the default u-boot usb interrupt packet timeout). Note that in this case on keyboards which do not support usb_set_idle() we loose and we actually get 1s latencies on other u-boot activities.

For the other poll-methods this commit stops using usb_set_idle() and instead repeats the last received hid-report every 40 ms as long as no new hid-report is received. This fixes key-repeat not working at all with CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with keyboards which do not implement usb_set_idle() when using CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.

Signed-off-by Hans de Goede <[email protected]>

Quoted 20 lines · Hans de Goede

The usb-kbd key repeat code assumes that reports get repeated every 40 ms, this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and does not always works for CONFIG_SYS_USB_EVENT_POLL and CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor the usb_set_idle() command.

For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a blocking wait for the hid report, so if we do not tell the keyboard to send a hid report every 40ms even if nothing changes then we will block u-boot for 1s (the default u-boot usb interrupt packet timeout). Note that in this case on keyboards which do not support usb_set_idle() we loose and we actually get 1s latencies on other u-boot activities.

For the other poll-methods this commit stops using usb_set_idle() and instead repeats the last received hid-report every 40 ms as long as no new hid-report is received. This fixes key-repeat not working at all with CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with keyboards which do not implement usb_set_idle() when using CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.

Quoted 1 line

Signed-off-by Hans de Goede <[email protected]>

Looks reasonable, I don't really care about the ifdef indent, but the data type for get_timer() should be fixed. Otherwise,

Reviewed-by Marek Vasut <[email protected]>

Would this finally be a patch to pick through the USB tree ? :b

Hi,

Quoted 22 lines · Marek Vasut

Quoted 21 lines · Hans de Goede

The usb-kbd key repeat code assumes that reports get repeated every 40 ms, this is never true when using CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP, and does not always works for CONFIG_SYS_USB_EVENT_POLL and CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE since not all usb keyboards honor the usb_set_idle() command.

For CONFIG_SYS_USB_EVENT_POLL we must use usb_set_idle() since we do a blocking wait for the hid report, so if we do not tell the keyboard to send a hid report every 40ms even if nothing changes then we will block u-boot for 1s (the default u-boot usb interrupt packet timeout). Note that in this case on keyboards which do not support usb_set_idle() we loose and we actually get 1s latencies on other u-boot activities.

For the other poll-methods this commit stops using usb_set_idle() and instead repeats the last received hid-report every 40 ms as long as no new hid-report is received. This fixes key-repeat not working at all with CONFIG_SYS_USB_EVENT_POLL_VIA_CONTROL_EP and fixes it not working with keyboards which do not implement usb_set_idle() when using CONFIG_SYS_USB_EVENT_POLL_VIA_INT_QUEUE.

Quoted 4 lines

Quoted 1 line

Signed-off-by Hans de Goede <[email protected]>

Looks reasonable, I don't really care about the ifdef indent, but the data type for get_timer() should be fixed. Otherwise,

OK v2 with last_report changed to unsigned_long is coming up.

Quoted 3 lines

Reviewed-by Marek Vasut <[email protected]>

Would this finally be a patch to pick through the USB tree ? :b

Yes! :)