[Feature Request] USB feature report handling

[george-norton]

Feature Request Type

• Core functionality
• Add-on hardware support (eg. audio, RGB, OLED screen, etc.)
• Alteration (enhancement/optimization) of existing feature(s)
• New behavior

Description

I have been doing some work to support trackpads within the digitizer feature. To do this I have had to implement support for a few Get/Set Feature reports. In particular:

Microsoft require 2 Get Report features:

• A device capabilities report (https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-certification-status-feature-report)
• A device certification report (https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-windows-precision-touchpad-collection#device-certification-status-feature-report)

The certification report is a pain as its 257 bytes, but Windows 8.1 wont work without a valid response. Later versions of Windows require us to return something, but they don't validate it.

We also need to support the following Set Report features:

• Input mode - we are supposed to report as if we are a mouse until the host sends us this message (https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-configuration-collection?source=recommendations#input-mode-feature-report)
• Selective mode reporting - we should disable the trackpad/buttons on demand (https://learn.microsoft.com/en-us/windows-hardware/design/component-guidelines/touchpad-configuration-collection?source=recommendations#selective-reporting-feature-report)

I currently have some hacked together report handling code which works, but I would like to make it nicer. I have seen that @KarlK90 has just redesigned all the report handling code, and I was wondering how I can best take advantage of it. The Get Report values are known at compile time, so I was wondering about populating them in the report storage structure as complete reports at compile time, but the buffer (64 bytes) isn't big enough for the certification report. The set report handling doesn't appear to be in place, but perhaps the code could be extended a little?

It may also be nice if the actual content of these descriptors is platform independent, although the certification report will never fit on an AVR, we could at least support multitouch on Linux and fall back to mouse reporting on Mac (Apple do not support 3rd party trackpads) and Windows.

This functionality would also be useful for supporting high resolution scrolling in the pointing device feature.

[KarlK90]

Hi @george-norton 👋,

I have been doing some work to support trackpads within the digitizer feature.
I currently have some hacked together report handling code which works, but I would like to make it nicer.

Cool! I see that you have pushed your implementation to https://github.com/george-norton/qmk_firmware/tree/multitouch_experiment. So I'll base my suggestions on that branch.

The certification report is a pain as its 257 bytes, but Windows 8.1 wont work without a valid response. Later versions of Windows require us to return something, but they don't validate it.

We can just go with the example certification report - like you already implemented.

I was wondering how I can best take advantage of it.

Sure, happy to give some feedback. The usb_report_storage_t structure is the right place to implement the needed functionality:

For the capabilities and certification feature the default get_report handlers implementations e.g. usb(_shared)_get_report won't cut it. You'll have to implement a specialization usb(_shared)_get_report_multitouch_foobar (naming is made up, you'll find a better one 🙂) that additionally handles the two extra cases. For code re-use just call the already existing handlers if the request you're getting isn't one of the two extra cases.

When constructing your IN endpoint in usb_endpoints.c you can just the QMK_USB_REPORT_STORAGE constructor and pass in your new handlers.

Make sure to increase the DIGITIZER_EPSIZE and SHARED_EPSIZE to match biggest report you're going to send over IN endpoints (if there is a size increase). The capabilities and certification reports are only send over EP0 - e.g. no need to increase the IN endpoint sizes to e.g. 257 bytes.

The Get Report values are known at compile time, so I was wondering about populating them in the report storage structure as complete reports at compile time, but the buffer (64 bytes) isn't big enough for the certification report.

The usb_report_storage_t.reports buffer is solely meant for handling the regular HID get report request which shall return the last sent input report over the IN endpoint. Just store the well know reports in a const array in a source file that is platform independent - which can be then be referenced in the handlers. We maybe won't support AVR for this feature but there is a port of QMK to riot-os coming up, that probably wants to implement this as well.

The set report handling doesn't appear to be in place, but perhaps the code could be extended a little?

Sure, you can just add a function pointer (maybe rename the existing set_report to store_report and re-use set_report?) to the usb_report_storage_t and add a new dispatch function usb_set_report_cb (like usb_get_report_cb) which is then called in usb_requests_hook_cb. For the keyboard and shared interface endpoints this would mean the same implementation, but most of the endpoints get a NULL pointer as the default implementation that isn't called at all.

At this point I would say that usb_report_storage_t has outgrown it's name. Why not rename it to usb_report_handler_t or something like that.

It may also be nice if the actual content of these descriptors is platform independent, although the certification report will never fit on an AVR, we could at least support multitouch on Linux and fall back to mouse reporting on Mac (Apple do not support 3rd party trackpads) and Windows.

That would be great - I personally neither use Windows nor MacOS on a daily basis.

This functionality would also be useful for supporting high resolution scrolling in the pointing device feature.

Looking forward to it :)

[george-norton]

Hello @KarlK90,
Thanks for your guidance. I have started playing around with the suggestions above and I am not really happy with how it is looking. My changes are here:

george-norton@38591d9

• usb_fs_report_t has a fixed 64 byte buffer, I have replaced this with a pointer and have allocated various storage objects for the different sized reports, the get_report handlers populate an output report object, which I have had to initialize with a pointer to the largest possible report size. Currently this is a magic number, not sure if there is some way to determine the maximum size at compile time. But the whole thing seems a bit messier than the old code.
• Typically the digitizer is on the shared endpoint, so replacing the get_report handler with a digitizer specific one seems like the wrong approach, it will breakdown as soon as a different feature on the shared endpoint wants its own feature report handlers. I think the handlers should probably be associated with a report id rather than the endpoint (falling back to the endpoint where the default report id is used)?
• You mentioned that the feature requests arrive on EP0, and this is true, I see it in wireshark. But in usb_get_report_cb the ep is derived from the interface (wIndex), so the callback for the shared endpoint is used. Is this what you expect?

[george-norton]

I assume there is a good reason the get_report handler needs to return a copy of the report rather than a pointer to the stored report? I guess there is a chance that the data will be altered before the report has been sent over the USB link?

[KarlK90]

@george-norton just a short heads up. I'm currently on vacation for the next two weeks and will reply in detail after that. I've started to implement a more flexible API for the get report functions that allows for dynamically sized reports in https://github.com/KarlK90/qmk_firmware/tree/feature%2Fusb-report-handling branch. It currently crashes with a null pointer dereference and I haven't figured out why yet...

[george-norton]

Thanks for taking a look at this! Have a great holiday, don't read the rest of this until you are back!

I have reproduced your crash with a onekey. Seems to run OK until I enable the console - then it crashes after printing the first message. Not quite sure what is going on at this point. Here is what I see:

0x100002ca in _unhandled_exception ()
(gdb) p/a *(uint32_t[8] *)$msp
$1 = {0x71657266, 0x20001c50 <__compound_literal.16+40>, 0x20, 0x32203a79, 0x20000275 <divmod_u32u32_unsafe>, 
  0x10003d8d <usb_endpoint_in_tx_complete_cb+92>, 0x32203a78, 0x21000015}

__compound_literal.16+40 seems to be something to do with the usb_endpoints_in structure, here is the structure:

(gdb) p usb_endpoints_in
$4 = {{
    obqueue = {
    waiting = {
        queue = {
        next = 0x200007ec <usb_endpoints_in>,
        prev = 0x200007ec <usb_endpoints_in>
        }
    },
    suspended = false,
    bcounter = 4,
    bwrptr = 0x20001cb8 <__compound_literal.6> "",
    brdptr = 0x20001cb8 <__compound_literal.6> "",
    btop = 0x20001d48 <__compound_literal.7> "",
    bsize = 36,
    bn = 4,
    buffers = 0x20001cb8 <__compound_literal.6> "",
    ptr = 0x0,
    top = 0x0,
    notify = 0x10003c61 <obnotify>,
    link = 0x200007ec <usb_endpoints_in>
    },
    ep_config = {
    ep_mode = 3,
    setup_cb = 0x0,
    in_cb = 0x10003d31 <usb_endpoint_in_tx_complete_cb>,
    out_cb = 0x0,
    in_maxsize = 32,
    out_maxsize = 0,
    in_state = 0x20000840 <usb_endpoints_in+84>,
    out_state = 0x0
    },
    ep_in_state = {
    txsize = 0,
    txcnt = 0,
    txbuf = 0x0,
    thread = 0x0,
    txlast = 0,
    next_pid = 0 '\000',
    hw_buf = 0x50100180 "",
    buf_size = 64
    },
    config = {
    usbp = 0x20002048 <USBD1>,
    ep = 2 '\002',
    buffer_capacity = 4,
    buffer_size = 32,
    buffer = 0x20001cb8 <__compound_literal.6> ""
    },
    usb_requests_cb = 0x0,
    timed_out = false,
    report_handler = 0x200007b8 <__compound_literal.5>
}, {
    obqueue = {
    waiting = {
        queue = {
        next = 0x20000880 <usb_endpoints_in+148>,
        prev = 0x20000880 <usb_endpoints_in+148>
        }
    },
    suspended = false,
    bcounter = 4,
    bwrptr = 0x20001bf4 <__compound_literal.11> "",
    brdptr = 0x20001bf4 <__compound_literal.11> "",
    btop = 0x20001c24 <__compound_literal.12> "",
    bsize = 12,
    bn = 4,
    buffers = 0x20001bf4 <__compound_literal.11> "",
    ptr = 0x0,
    top = 0x0,
    notify = 0x10003c61 <obnotify>,
    link = 0x20000880 <usb_endpoints_in+148>
    },
    ep_config = {
    ep_mode = 3,
    setup_cb = 0x0,
    in_cb = 0x10003d31 <usb_endpoint_in_tx_complete_cb>,
    out_cb = 0x0,
    in_maxsize = 8,
    out_maxsize = 0,
    in_state = 0x200008d4 <usb_endpoints_in+232>,
    out_state = 0x0
    },
    ep_in_state = {
    txsize = 0,
    txcnt = 0,
    txbuf = 0x0,
    thread = 0x0,
    txlast = 0,
    next_pid = 0 '\000',
    hw_buf = 0x50100200 "",
    buf_size = 64
    },
    config = {
    usbp = 0x20002048 <USBD1>,
    ep = 1 '\001',
    buffer_capacity = 4,
    buffer_size = 8,
    buffer = 0x20001bf4 <__compound_literal.11> ""
    },
    usb_requests_cb = 0x0,
    timed_out = false,
    report_handler = 0x20000734 <__compound_literal.10>
}, {
    obqueue = {
    waiting = {
        queue = {
        next = 0x20000914 <usb_endpoints_in+296>,
        prev = 0x20000914 <usb_endpoints_in+296>
        }
    },
    suspended = false,
    bcounter = 3,
    bwrptr = 0x20001c70 <__compound_literal.16+72> " ",
    brdptr = 0x20001c4c <__compound_literal.16+36> " ",
    btop = 0x20001cb8 <__compound_literal.6> "",
    bsize = 36,
    bn = 4,
    buffers = 0x20001c28 <__compound_literal.16> " ",
    ptr = 0x0,
    top = 0x20001c70 <__compound_literal.16+72> " ",
    notify = 0x10003c61 <obnotify>,
    link = 0x20000914 <usb_endpoints_in+296>
    },
    ep_config = {
    ep_mode = 3,
    setup_cb = 0x0,
    in_cb = 0x10003d31 <usb_endpoint_in_tx_complete_cb>,
    out_cb = 0x0,
    in_maxsize = 32,
    out_maxsize = 0,
    in_state = 0x20000968 <usb_endpoints_in+380>,
    out_state = 0x0
    },
    ep_in_state = {
    txsize = 32,
    txcnt = 32,
    txbuf = 0x20001c70 <__compound_literal.16+72> " ",
    thread = 0x0,
    txlast = 32,
    next_pid = 0 '\000',
    hw_buf = 0x50100280 "matrix scan frequency: 22404\n",
    buf_size = 64
    },
    config = {
    usbp = 0x20002048 <USBD1>,
    ep = 3 '\003',
    buffer_capacity = 4,
    buffer_size = 32,
    buffer = 0x20001c28 <__compound_literal.16> " "
    },
    usb_requests_cb = 0x0,
    timed_out = false,
    report_handler = 0x20000764 <__compound_literal.15>
}}

The typeinfo suggest we are somewhere in obqueue:

(gdb) ptype/o usb_endpoints_in
type = struct {
/*      0      |      56 */    output_buffers_queue_t obqueue;
/*     56      |      28 */    USBEndpointConfig ep_config;
/*     84      |      32 */    USBInEndpointState ep_in_state;
/*    116      |      20 */    usb_endpoint_config_t config;
/*    136      |       4 */    usbreqhandler_t usb_requests_cb;
/*    140      |       1 */    _Bool timed_out;
/* XXX  3-byte hole      */
/*    144      |       4 */    usb_report_handler_t *report_handler;

                               /* total size (bytes):  148 */
                             } [3]

Here is the output_buffers_queue_t typeinfo:

type = struct io_buffers_queue {
/*      0      |       8 */    threads_queue_t waiting;
/*      8      |       1 */    _Bool suspended;
/* XXX  3-byte hole      */
/*     12      |       4 */    volatile size_t bcounter;
/*     16      |       4 */    uint8_t *bwrptr;
/*     20      |       4 */    uint8_t *brdptr;
/*     24      |       4 */    uint8_t *btop;
/*     28      |       4 */    size_t bsize;
/*     32      |       4 */    size_t bn;
/*     36      |       4 */    uint8_t *buffers;
/*     40      |       4 */    uint8_t *ptr;
/*     44      |       4 */    uint8_t *top;
/*     48      |       4 */    bqnotify_t notify;
/*     52      |       4 */    void *link;

                               /* total size (bytes):   56 */
                             }

So __compound_literal.16+40 seems to be usb_endpoints_in[0].obqeue.ptr, but there doesn't seem to be anything much at 0x71657266.

[sigprof]

One problematic place is QMK_USB_REPORT_HANDLER_DEFAULT(CONSOLE_EPSIZE) — the macro now expects to receive the type name, so you get sizeof(CONSOLE_EPSIZE) instead of the proper buffer size.

[george-norton]

I have fixed the issue sigprof found, but there is another crash. This appears to have been introduced by commit #6d26f189a211e80778de77ebfb7b9a2454d67b74. My suspicion is that the static report_data arrays are ending up in SPI flash rather than RAM and that is causing some sort of issue. If I remove the static keyword, the code works and the .bss.report_data.1 entries disappear from the .map file.

[KarlK90]

@sigprof Thanks for the catch

@george-norton I've had a look at my code an found some more bugs, with these fixed the onekey enumerates and works correctly again. The code now lives in this draft PR: #23388, removing the static from the report_data wasn't necessary in my short tests. Could you try again?

[george-norton]

Hope you had a nice vacation. I just ran a quick test and, it seems to be working well. I see the scan rate reported over the console and messages about the led_task when I toggle the caps lock.

[george-norton]

Hello, I have started looking at what would need to change to support SET REPORT requests. I believe that SET and GET report calls are not symmetric? For example, the keyboard endpoint defines an input report that reports modifiers and keycodes and an output report that reports the LED state. So GET/SET requests on a particular endpoint should not be working with the same report?

I haven't yet looked into the idle stuff, but I suspect the current usb_report_t structure will either need extending, or we will need two per endpoint.

[george-norton]

I guess SET_REPORT messages don't need to end up in the store as they will.be updating some state in some QMK feature, so they probably all need to be handled individually?