WICG / webhid

Web API for accessing Human Interface Devices (HID)
Other
142 stars 35 forks source link

[Windows] Incorrect report size calculation #102

Open codembed opened 2 years ago

codembed commented 2 years ago

For each report in a collection, more than one field type is included in the calculation of the report size, however the calculation should be filtered by field type

For example, for a definition similar to:

Usage Page (Generic Desktop Ctrls)
Usage (Mouse)
Collection (Application)
    Usage ()
    Collection (Logical)
        Usage Page ()
        Report ID (1)
        Logical Minimum (0)
        Logical Maximum (127)
        Report Count (1)
        Report Size (8)
        Input (Data,Var,Abs)
        Feature (Data,Var,Abs)
        Output (Data,Var,Abs)
    End Collection
End Collection

The actual report sizes (including id byte) are InputReport(ID_1) -> 2 bytes OutputReport(ID_1) -> 2 bytes FeatureReport(ID_1) -> 2 bytes

However, the size is incorrectly described as 4 bytes for each report (1-id + 1-feature + 1-output + 1-input)

Chromium appears to pad the collection definitions with additional 'const' bytes, and the actual inputReport event data also contains trailing zero padding, which is not present over the wire.

The unexpected padding can be observed in webhid-explorer as extra 'Cnst,Ary,Abs' bits near the end of each report description.

For inputs, there is likely minimal impact, however Feature and Output reports that are created according to the invalid definition could be rejected (typically by the OS, or perhaps the device?). I have not tested this.

codembed commented 2 years ago

So, observed this on Windows (10), but Chrome OS did not have this problem and has correct report sizes.

codembed commented 2 years ago

My assumptions in the opening comment are misleading.

It looks like this is the same as #86

On closer inspection, it seems that all reports of one type have a size equal to the maximum for that type, which is not because of common IDs.

So, if I have one input report size = 12 and one size = 4, both are reported with length 12 Two feature reports with size 2 and 7 bytes are both described with length 7, etc.

Since the characteristics of each report field is known, why can't the correct size be calculated for each report and then reflected in HIDDevice.collections?

nondebug commented 2 years ago

On closer inspection, it seems that all reports of one type have a size equal to the maximum for that type, which is not because of common IDs.

This is a platform limitation on Windows where the size of the report buffer is required to match the maximum size of a report of that type (input, output, or feature). WebHID API reflects this requirement by inserting a padding item at the end of the HIDDevice.collections array that extends each report to the maximum length.

I'd like to remove the limitation and have WebHID return the same collection information on Windows that we return on other platforms. However, this doesn't appear to be possible using the information exposed through the Windows HID API. On Linux, macOS, and ChromeOS we read the HID report descriptor from the device and parse it to learn the sizes of each report. On Windows the report descriptor isn't accessible to userspace applications. Instead, Windows provides the information from the report descriptor as an opaque object HIDP_PREPARSED_DATA which can be passed to other API methods used for reading and writing report fields.

Chrome inspects the opaque HIDP_PREPARSED_DATA object and uses it to reconstruct the layout of each report. The preparsed object loses some information from the original report descriptor, so the reconstructed reports are not exactly the same as what we get on other platforms. Details like const padding items and report lengths are not preserved.

Since the characteristics of each report field is known, why can't the correct size be calculated for each report and then reflected in HIDDevice.collections?

The correct report size is always the sum of reportSize * reportCount for each item in HIDReportInfo.items. If you use this size to allocate report buffers you'll automatically include the extra padding on Windows.

I'm in favor of adding HIDReportInfo.byteLength or similar as a convenience but it's not high on my list of priorities. A pull request to add it to the spec would be welcome.