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

More dynamic predefined hid descriptors #67

Open
perigoso opened this issue Jun 20, 2021 · 9 comments
Open

More dynamic predefined hid descriptors #67

perigoso opened this issue Jun 20, 2021 · 9 comments
Labels
enhancement New feature or request

Comments

@perigoso
Copy link
Member

We right now have static simple hid descriptors defined for mouse and keyboard, but some targets might want slightly different reports. For instance, 5 buttons instead of 3, pan in addition to wheel, 16 bit xy deltas.

This could always be done target side, but we could use the preprocessor to do this:

struct mouse_report {
        	u8 id;
#if defined(MOUSE_REPORT_16BIT_DELTA)
        	s16 x;
        	s16 y;
#else
        	s8 x;
        	s8 y;
#endif
        	s8 wheel;
#if defined(MOUSE_REPORT_PAN)
        	s8 wheel;
#endif
        	u8 button1 : 1;
        	u8 button2 : 1;
        	u8 button3 : 1;
#if defined(MOUSE_REPORT_5BUTTONS)
        	u8 button4 : 1;
        	u8 button5 : 1;
#endif
        } __attribute__((__packed__))

(same for the actual report descriptor)

@perigoso perigoso added the enhancement New feature or request label Jun 20, 2021
@FFY00
Copy link
Member

FFY00 commented Jun 20, 2021

Instead of using the preprocessor to configure this, I was thinking about having different variants. There's actually not that much that we'd want to configure, the button count for eg. could be bumped to 8 directly as the last 5 bits are just padding currently, so we would be left with 2 variants, one for 8bit axis and one for 16bit.

I'd also like to note that these are just helpers, if a device has custom needs, a custom report could be used there. The point of this code is just to remove most of the duplication in targets.

@t-8ch
Copy link
Contributor

t-8ch commented Jun 20, 2021

What advantage would it make to have a absolutely fitting record?
Would it be bad to have unused bits in the descriptor?

For example the QMK project (open source keyboard firmware) just has one descriptor for each kind of use (keyboard/mouse/joystick/consumer control).
Many bits won't every be used but it seems not to be an issue.

@FFY00
Copy link
Member

FFY00 commented Jun 20, 2021

We have to align the data to the byte level -- we can't send a half byte, we need to send full bytes -- so padding bits might as well be used to send data, there isn't any drawback there. One example is the buttons in the current report descriptor, we only define 3 buttons at the beginning of a byte and the last 5 bits are just padding, so we don't lose anything in defining 8 buttons instead.

Since we are also targeting high-performance applications, if there is a need for more data in some devices, such as the described 16bit axis, we should have a different report descriptor for that, to avoid adding unnecessary overhead to the devices that don't need it.

@t-8ch
Copy link
Contributor

t-8ch commented Jun 20, 2021

Ok. I wouldn't have expected it to be a performance issue.

@FFY00
Copy link
Member

FFY00 commented Jun 20, 2021

I mean, it shouldn't really make that much of a difference, but it is a trivial thing to do, so why not do it? The worst case scenario for the current scope would be using a 8kHz report rate, and 2 extra bytes would be 16kB extra per second, it shouldn't really be an issue but 🤷

@t-8ch
Copy link
Contributor

t-8ch commented Jun 20, 2021

so why not do it?

Because the struct and report descriptors will be more #ifdef than actual code IMO.

Your choice though :-)

@FFY00
Copy link
Member

FFY00 commented Jun 20, 2021

No, I don't want any #ifdef, I want different structs for each 😛. And this would only be for common reports, like I mentioned in the first reply, if a target needs a custom one, they could define it themselves since it's pretty straightforward.

@t-8ch
Copy link
Contributor

t-8ch commented Jun 20, 2021

Ah ok, I missed that.

@perigoso
Copy link
Member Author

i was thinking a couple different reports too, but then i though about the pan axis, i guess we can just have the two for now, 8 and 16 bit, if any particular target needs the pan they can define it

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

No branches or pull requests

3 participants