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

Control padding for byte-alignment #1

Open
manuelbl opened this issue Feb 11, 2022 · 6 comments
Open

Control padding for byte-alignment #1

manuelbl opened this issue Feb 11, 2022 · 6 comments

Comments

@manuelbl
Copy link

The sample for a keyboard (see below) uses 7 bits for each regular button. Thus the array items are not byte-aligned. While it is correct and compact, it's very cumbersome to use, e.g. when programming a MCU as a USB HID device. How can the report size be increased to 8 bits to make life easier?

Similarly for the below mouse descriptor. It's similar to the provided sample, however the buttons are moved to the front of the report. Waratah automatically adds 5 bits of padding, but it's added at the end. Thus, the mouse x and y movements are not byte aligned. How can the padding be moved before x and y?

Keyboard descriptor

[[applicationCollection]]
usage = ['Generic Desktop', 'Keyboard']

    [[applicationCollection.inputReport]]

        # Regular Buttons
        [[applicationCollection.inputReport.arrayItem]]
        usageRange = ['Keyboard/Keypad', 'ErrorRollOver', 'Keyboard Application']
        count = 8

        # Special buttons
        [[applicationCollection.inputReport.variableItem]]
        usageRange = ['Keyboard/Keypad', 'Keyboard LeftControl', 'Keyboard Right GUI']
        logicalValueRange = [0, 1]

    # LEDS
    [[applicationCollection.outputReport]]

        [[applicationCollection.outputReport.variableItem]]
        usageRange = ['LED', 'Num Lock', 'Kana']
        logicalValueRange = [0, 1]

Mouse descriptor

[[applicationCollection]]
usage = ['Generic Desktop', 'Mouse']

    [[applicationCollection.inputReport]]

        [[applicationCollection.inputReport.physicalCollection]]
        usage = ['Generic Desktop', 'Pointer']


            [[applicationCollection.inputReport.physicalCollection.variableItem]]
            usageRange = ['Button', 'Button 1', 'Button 3']
            logicalValueRange = [0, 1]

            [[applicationCollection.inputReport.physicalCollection.variableItem]]
            usage = ['Generic Desktop', 'X']
            sizeInBits = 8
            logicalValueRange = 'maxSignedSizeRange'
            reportFlags = ['relative']

            [[applicationCollection.inputReport.physicalCollection.variableItem]]
            usage = ['Generic Desktop', 'Y']
            sizeInBits = 8
            logicalValueRange = 'maxSignedSizeRange'
            reportFlags = ['relative']
@matwilli
Copy link
Contributor

There's gross packing support using the packingInBytes setting (e.g. below). This ensures every control will be a multiple of X bytes (1, 2, 4). This also has the added benefit of auto C++ struct generation (There's currently no support for non-packed descriptors, but it'd be nice to add).

This however will affect every control, so while 'easy', it doesn't give you the fine-grained control you might be after. (i.e. packing for a single control).

[[settings]]
packingInBytes = 1

[[applicationCollection]]
usage = ['Generic Desktop', 'Mouse']
...

Padding (between controls) can be done by [[paddingItem]] section.

Do those facilities cover enough of your scenario?

@manuelbl
Copy link
Author

Thanks for the hint to [[paddingItem]]. It helps for moving the padding in the mouse descriptor forward so the remaining fields are aligned. The mouse descriptor is then straight-forward to use in software.

However, it doesn't help for the keyboard descriptor. And packingInBytes doesn't either. It pads the key array. But it unnecessarily grows the special buttons array by a factor of 8. So the intended descriptor cannot be achieved.

A really useful option would be to have a padding option that automatically inserts padding bits such that multi-bit items start on a byte boundary. In the case of the keyboard, it would add a single padding bit after each 7-bit array item, but no padding for the special key bit array.

BTW: Have you ever tried the sample keyboard descriptor with a real device. Since the special keys are after the regular keys, they do not apply to the regular keys in input report. Instead, they will apply to the regular keys in the next input report (if any). If keys are pressed slowly, that's not a problem. But if they are pressed quickly or for programmatic key generation, it's problematic. So you would be better to swap them.

@matwilli
Copy link
Contributor

matwilli commented Feb 24, 2022

Ok, I think I see what you're getting at. Yes, this is certainly something that could be added to Waratah (we have all the data, and already do something similar with packingInBytes/paddingItem)

I think this a little tricky to do generically (hence the 'hammer' of packingInBytes), particularly w.r.t overspanning and C++ struct generation. We'd also need to make it obvious to users that they can get better packing efficiency by grouping single-bit values together, otherwise they might get non-obvious generation, "Why did the tool do that??". I'd expect though, users of a tool like this would be keenly aware of situations like this, and would look to the generated output many times to optimize their descriptor (and if they don't care, the descriptor will still be valid). I don't think we want to get into 're-arranging' controls (to enable more efficient packing), because the output could be confusing (particularly in large descriptors).

I'll have to think more about how to do this; so let's keep this issue open for tracking. (Feel free to suggestion a PR with any improvements!)

Re. keyboard descriptor, no I haven't tried any of the descriptors on real devices (much to my pain), so I really appreciate you finding such an impactful issue (most descriptors I wrote were based of existing devices I had lying-around, or just wrote from scratch). As part of a next effort, I want to create reference software implementations for a variety of devices (using Waratah to generate descriptors), so I had hoped to find any issues with the descriptors at that time (but you beat me to it!). A co-worker tried out the descriptor in the past week and confirmed the same issue and solution. Fixed in samples and wiki docs

@henrygab
Copy link

henrygab commented Dec 8, 2022

a padding option that automatically inserts padding bits such that multi-bit items start on a byte boundary.

If you are still considering options, it might be useful to have the following, which covers more situations:

First, rather than limiting to declaring byte-boundary, allow an attribute to force alignment to any 2**N (power of 2) bits; thus byte boundary is simply N=3

Second, allow that alignment to be specified to each ... scope ... for lack of a better word:

  1. Per item ... substantially identical to adjusting the bit size of each item, while restricting/limiting the logical min/max values
  2. At Array boundary... allowing packing six (6x) 5-bit items into a four-byte (32-bit) report
  3. At Usage Page boundary ... allowing arbitrary bits within the page, while ensuring 8-bit alignment when the page ends
  4. At Collection boundary... again, this only kicks in when the collection is closed, ensuring the alignment at that time

@Previsou
Copy link

Any news on this front? Would be very helpful, as I am in the same situation. To try and keep backward compatibility with my other existing software, I'll have to patch the generated report by other means.

@matwilli
Copy link
Contributor

No update here :(.

The generated report will now create byte[] for each report if packingInBytes isn't used, which should be helpful.

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

No branches or pull requests

4 participants