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

USBHIDKeyboard: Fix 200ms delay for every key #7218

Merged
merged 2 commits into from
Sep 14, 2022

Conversation

RefactorFactory
Copy link
Contributor

@RefactorFactory RefactorFactory commented Sep 3, 2022

Description of Change

Arduino-esp32 2.0.4 was released with a version of TinyUSB hid_device.h that uses uint16_t for the last argument:

TU_ATTR_WEAK void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, uint16_t len);

But USBHID implements this callback with uint8_t:

void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, uint8_t len){
    if (tinyusb_hid_device_input_sem) {
        xSemaphoreGive(tinyusb_hid_device_input_sem);
    }
}

The result is that when USBHIDKeyboard sends a report to the host, it times out, waiting 100 ms for the callback to be called (which never occurs because the function signature does not match). It does this once for pressing the key and once for releasing the key, so 100 ms * 2 = 200 ms.

The latest version of hid_device.h reverts the last argument to uint8_t:

TU_ATTR_WEAK void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, /*uint16_t*/ uint8_t len );

But these commits suggest that the last argument will eventually be changed to uint16_t:

hathach/tinyusb@556b5d5

change report len in hid API from uint8_t to uint16_t

since HS interrupt endpoint can be up to 1024, 8-bit is not enough.
affected APIs are:
- tud_hid_n_report() / tud_hid_report()
- tud_hid_report_complete_cb()

hathach/tinyusb@b495d6f

temporarily revert len back to uint8_t in tud_hid_report_complete_cb() for up coming release

To prevent this from becoming broken again, in preparation for the change to uint16_t, make USBHID resilient to any type for the last argument for tud_hid_report_complete_cb() by using some C++ template metaprogramming,
adapted from https://stackoverflow.com/questions/22632236/how-is-possible-to-deduce-function-argument-type-in-c.

Tests scenarios

I have tested my Pull Request on Arduino-esp32 core v2.0.4 with ESP32-S2 Board with this scenario.

Arduino-esp32 2.0.4 was released with a version of TinyUSB hid_device.h
that uses uint16_t for the last argument:

https://github.com/espressif/arduino-esp32/blob/2.0.4/tools/sdk/esp32s2/include/arduino_tinyusb/tinyusb/src/class/hid/hid_device.h

    TU_ATTR_WEAK void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, uint16_t len);

But USBHID implements this callback with uint8_t:

https://github.com/espressif/arduino-esp32/blob/2.0.4/libraries/USB/src/USBHID.cpp

    void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, uint8_t len){
        if (tinyusb_hid_device_input_sem) {
            xSemaphoreGive(tinyusb_hid_device_input_sem);
        }
    }

The result is that when USBHIDKeyboard sends a report to the host, it
times out, waiting 100 ms for the callback to be called. It does this
once for pressing the key and once for releasing the key, so
100 ms * 2 = 200 ms.

The latest version of hid_device.h reverts the last argument to uint8_t:

https://github.com/espressif/arduino-esp32/blob/860b104691a28f77896ac544c7745de1ba53642d/tools/sdk/esp32s2/include/arduino_tinyusb/tinyusb/src/class/hid/hid_device.h

    TU_ATTR_WEAK void tud_hid_report_complete_cb(uint8_t instance, uint8_t const* report, /*uint16_t*/ uint8_t len );

But these commits suggest that the last argument will eventually be
changed to uint16_t:

hathach/tinyusb@556b5d5

    change report len in hid API from uint8_t to uint16_t

    since HS interrupt endpoint can be up to 1024, 8-bit is not enough.
    affected APIs are:
    - tud_hid_n_report() / tud_hid_report()
    - tud_hid_report_complete_cb()

hathach/tinyusb@b495d6f

    temporarily revert len back to uint8_t in tud_hid_report_complete_cb() for up coming release

To prevent this from becoming broken again, in preparation for the change
to uint16_t, make USBHID resilient to any type for the last argument for
tud_hid_report_complete_cb() by using some C++ template metaprogramming,
adapted from https://stackoverflow.com/a/22632571.
@Jason2866
Copy link
Collaborator

@me-no-dev Considering to use in Arduino Lib Builder not always latest commit, instead a fixed known good commit/release?
This is not the only case where a surprise happened. I remember similar for LittleFs and WebCam (changed my Lib Builder fork lately this way to avoid).

@me-no-dev
Copy link
Member

@me-no-dev Considering to use in Arduino Lib Builder not always latest commit, instead a fixed known good commit/release?

How do we define what is "known good"? Also that would mean that we could break the component if someone uses versions different than that outside of the lib-builder.

@me-no-dev me-no-dev merged commit 2e72894 into espressif:master Sep 14, 2022
@Jason2866
Copy link
Collaborator

How do we define what is "known good"? Also that would mean that we could break the component if someone uses versions different than that outside of the lib-builder.

@me-no-dev
How do you know that the used ones from the master branch are working?

People are responsible not using the variants which are used in official version. This is normal.

Known good, if there are no bug issues for the fixed used version. There are release versions. In this case the maintainer did know the issue. The actual release version reverted the change

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

Successfully merging this pull request may close these issues.

4 participants