-
Notifications
You must be signed in to change notification settings - Fork 6.2k
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
usb: device_next: add initial HID device support #65801
usb: device_next: add initial HID device support #65801
Conversation
subsys/usb/device_next/usbd_ch9.c
Outdated
/* | ||
* If the recipient is not the device then it is probably a | ||
* class specific request where wIndex is the interface | ||
* number or endpoing and not the language ID. e.g. HID |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
endpoing
-> endpoint
description: | | ||
Device can be used as a Boot Device and supports a predefined protocol | ||
for mouse or keyboards. | ||
- none: Device does not support the boot interface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just make boot-protocol-code
not required and treat the absence of property as none
. You can determine if the property is there with DT_NODE_HAS_PROP()
and then just do COND_CODE_1()
or COND_CODE_0()
on it.
HID device name. When this property is present, a USB device will use it | ||
as the string descriptor of the interface. | ||
|
||
boot-protocol-code: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe just renamed to boot-protocol
. The user sets the overall protocol and I think it shouldn't be down to individual fields.
Input type reports polling rate in number of 125µs periods. | ||
Depending on the actual speed, the calculated polling rate could be clamped | ||
to 1ms or 255ms. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The 125µs period is the case only on high-speed. On full-speed the meaning is different.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's exactly that either... On High Speed it's the formula of (2^(bInterval-1) * 125us) = x. Where it would follow this table.
bInterval | uS |
---|---|
1 | 125 |
2 | 250 |
3 | 500 |
4 | 1000 |
5 | 2000 |
6 | 4000 |
7 | 8000 |
8 | 16000 |
9 | 32000 |
10 | 64000 |
11 | 128000 |
12 | 256000 |
13 | 512000 |
14 | 1024000 |
15 | 2048000 |
16 | 4096000 |
For Full/Low Speed it's, just 1ms units.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the point. I want to use 125µs granularity for this property. But the actual code does not handle it properly. Changed to microseconds.
Depending on the actual speed, the calculated polling rate could be clamped | ||
to 1ms or 255ms. | ||
|
||
in-reports-mps: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have separate max packet size entry for full-speed and high-speed, because the two have different allowed ranges. While the USB stack does not really support the concept of different configurations for different operating speed we really should have it because otherwise we'll run into unsolvable issues later on.
If there is just one for the time being, put 64 as the maximum value here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b5dfe7a
to
95d7bfe
Compare
95d7bfe
to
cfeb50b
Compare
errno = ops->get_report(dev, type, id, net_buf_tailroom(buf), buf->data); | ||
break; | ||
case HID_REPORT_TYPE_OUTPUT: | ||
LOG_DBG("Get Report, Output Report ID %u", id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Output report type cannot be get, this should be an error case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case should be removed, so it falls to the default error handler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just like with input report being set, HID Specification leaves it up to developer whether it is possible to get output report or not.
const struct hid_device_ops *const ops = ddata->ops; | ||
struct usbd_hid_descriptor *const desc = ddata->desc; | ||
|
||
atomic_set_bit(&ddata->state, HID_DEV_CLASS_ENABLED); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where to put this, and maybe I just didn't find where this happens, but ddata->protocol
should be reset to REPORT when the HID interface is configured.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, fixed here as suggested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could also call application's callback informing about the protocol change here? So that application would be explicitly notified about the used protocol and could rely on single source of truth.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
below, ready callback is called. Also host should set the protocol whenever initializing a device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds ok too
cfeb50b
to
d1aec4f
Compare
LOG_ERR("Failed to enable device support"); | ||
return ret; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An info log informing that sample successfully initialized would be really helpful here. Currently there is only a boot banner if I boot without USB connected (it may look as if something went wrong)
continue; | ||
} | ||
|
||
ret = hid_device_submit_report(hid_dev, sizeof(report), report, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, main thread is preemptive. USB HID device readiness may change after the !kb_ready
check (it could also bypass internal checks done by hid_device_submit_report
in similar way). Is it safe or could it lead to problems or edge cases in USB stack? Maybe we need cooperative priority here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you think the cooperative thread can not be interrupted and is somehow different then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A preemptive thread can be supplanted at any time by the scheduler while a cooperative thread needs to perform an action that would make it unready. As long as input_cb
is called from a thread context (e.g. not from an interrupt), it would not preempt execution of a cooperative thread (but it still may supplant a preemptive thread).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cooperative thread can be interrupted by an interrupt, AFAIK there is no guaranty it can immediately continue after, am I wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cooperative thread will continue execution after it is interrupted by an interrupt. There is guarantee that there won't be other thread executing, see https://docs.zephyrproject.org/3.6.0/kernel/services/scheduling/index.html#cooperative-time-slicing for more information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The CONFIG_INPUT_MODE
is CONFIG_INPUT_MODE_THREAD
by default, so that events seem broadcasted through a common input thread by default (then cooperative main priority could actually prevent supplant).
Ref: https://docs.zephyrproject.org/latest/services/input/index.html#application-api
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought synchronous is on by default, added CONFIG_INPUT_MODE_SYNCHRONOUS=y
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cooperative thread will continue execution after it is interrupted by an interrupt. There is guarantee that there won't be other thread executing,
It seems to me that this article does not pay attention to meta-IRQ threads.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if using a thread for synchronizing inputs wouldn't actually simplify synchronization here (we could rely on cooperative priorities to simplify sample's implementation and prevent races; in sample's use-case extra latency shouldn't be a problem). You are right that meta-IRQ threads are tricky use-case here (still, I don't recall seeing them used instead of regular threads in most of the subsystems or samples - I guess using them would require some "special handling" to ensure safety)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still, since it's an improvement to the sample code, I would suggest to add it (but I would not NACK PR because of it)
|
||
return buf; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty line, please
struct usbd_class_data *c_data = ddata->c_data; | ||
struct net_buf *buf; | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove extra empty line, please
return usbd_ep_buf_free(uds_ctx, buf); | ||
} | ||
|
||
if (sync && ops->input_report_done == NULL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be if (sync)
? Currently it seems misaligned with API doc.
Edit: Looking at the usbd_hid_request
implementation a bit more, I think we may need to use ops->input_report_done == NULL
as condition here (implementation seems not to support concurrent sync and async flows). Maybe we could simply use sync only if ops->input_report_done == NULL
and use async otherwise (then an additional function argument may not be needed here).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated the documentation, but kept the sync parameter for now. I am still thinking about it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be if (sync || ops->input_report_done == NULL)
then? (to align with newly introduced API doc)
Currently, if the callback is NULL and sync is set to false, the application has no way to get information that report was actually sent.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see it's already aligned. Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could simply use sync only if ops->input_report_done == NULL and use async otherwise (then an additional function argument may not be needed here).
I followed this suggestion.
- keyboard: Device supports boot interface and keyboard protocol | ||
- mouse: Device supports boot interface and mouse protocol | ||
enum: | ||
- none |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none
should be removed from here. I know this requires extra macro mapping, but implicitly relying on numbers being the same between different types is something I prefer to avoid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not get it. What types are different and how does removing "none" fix "implicitly relying on numbers being the same between different types" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none
doesn't really belong under "boot-protocol" from devicetree point of view IMHO (the absence of property signifies that). The "different types" I refer to the enum defined here in devicetree and the protocol code as defined by HID Specification - these two are essentially separate.
} | ||
|
||
if (legacy_ops->int_in_ready) { | ||
LOG_ERR("Input report done for %p", dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOG_DBG
?
Can we align check with other propagated callbacks: legacy_ops != NULL && legacy_ops->int_in_ready != NULL
|
||
int usb_hid_init(const struct device *dev) | ||
{ | ||
LOG_DBG("It does nothing for dev %p", dev); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please cast the %p
argument to (void *)
(as required by C standard). Also in other places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Required by what C standard?
I will not add explicit conversion of any pointer to a void pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apparently all of the major ones. C89, C99 and C11: "The argument shall be a pointer to void", and for the scanf variants the standard say: "The corresponding argument shall be a pointer to a pointer to void."
Not respecting this leads to undefined behavior. Whether this is going to lead to actual real world problems is another story, but from the standard point of view it can (as any undefined behavior).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way around is OK. The C standard guarantees conversion from void*
to any type of pointer, but not in this direction as pointed out here.
EDIT: I was wrong, no need to cast in either direction. But it is required to cast when using printf
and %p
, so the fix is required here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The other way around is OK. The C standard guarantees conversion from void* to any type of pointer, but not in this direction as pointed out here.
That is different. C in a Nutshell, "Any object pointer type can be implicitly converted to a pointer to void, and vice versa.".
Not sure fprintf conversion specification is required for cbprintf.
int l = len; | ||
|
||
if (legacy_ops != NULL && legacy_ops->set_report != NULL) { | ||
return legacy_ops->get_report(dev, &setup, &l, &d); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be set_report
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
* | ||
* @param[in] dev Pointer to HID device | ||
* @param[in] size Size of the input report | ||
* @param[in] report Input report buffer. Report buffer must be aligned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw. Do we have e.g. a #define
that could be used to get the required alignment?
Could we add an assertion that would ensure proper alignment, please? (I am worried that wrong alignment bug may be quite hard to catch)
#include <zephyr/devicetree.h> | ||
#include <zephyr/usb/usbd.h> | ||
#include <zephyr/usb/class/usb_hid.h> | ||
#include <zephyr/usb/class/usbd_hid.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicated include
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, sorry for confusion (I misread the second one...)
uint8_t protocol; | ||
}; | ||
|
||
static uint8_t hid_get_in_ep(struct usbd_class_data *const c_data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static inline
? (for consistency with hid_get_out_ep
)
return; | ||
} | ||
|
||
usbd_ep_enqueue(c_data, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider at least emitting an error log on failure here
duration = ops->get_idle(dev, id) / 4UL; | ||
} | ||
|
||
LOG_DBG("Get Idle, Report ID %u Duration %u", id, duration); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we emit log before checks to ensure user would see it? (that would also improve consistency with handle_set_idle
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It may be useful also for other callbacks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not find it practical, then we might also have to log every protocol error and that bloats everything.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would not insist on adding extra logs, we can keep it as is
usbd_ep_enqueue(c_data, buf); | ||
} | ||
|
||
static int hid_dev_submit_report(const struct device *dev, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jfischer-no, would it be possible to allow writing directly to endpoint (in similar way as it was done in legacy stack) to minimize HID data latency here (small latency may be crucial for some HID applications)? Would such functionality be difficult to implement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, one always have to enqueue a transfer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, thanks for clarification.
} | ||
} | ||
|
||
if (bi->ep == hid_get_in_ep(c_data)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
else if
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would anyone do that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking about explicitly avoiding the second comparison in code (bi->ep
cannot match both conditions here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still do not get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding else
before the second if
leads to omitting the second comparison if the first condition is met
if (ops->set_idle != NULL) { | ||
ops->set_idle(dev, id, duration * 4UL); | ||
} else { | ||
errno = -ENOTSUP; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't end up with this error if id == 0U
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not? Id 0 applies to all reports and Set Report is not always required, so we need to issue protocol error if there is no callback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then shouldn't we also return error for id == 0
in handle_get_idle
if there is no callback? (for consistency).
Also, currently we update our common idle rate even though we return protocol error here.
btw. This is set_idle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I have to drop legacy API support.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's worth to keep the legacy API support (even if it has some additional limitations)
switch (type) { | ||
case HID_REPORT_TYPE_INPUT: | ||
LOG_DBG("Set Report, Input Report ID %u", id); | ||
errno = ops->set_report(dev, type, id, buf->len, buf->data); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could make the HID_REPORT_TYPE_
enum part of the application API and use it to pass report type?
const uint8_t *const buf) | ||
{ | ||
LOG_HEXDUMP_DBG(buf, len, "o.r."); | ||
kb_set_report(dev, 2U, 0U, len, buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we actually need to pass type and ID here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, Set Report has different interface.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that most of the parameters passed to kb_set_report
are actually unused and we may desist from passing them as function arguments because of that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
kb_set_report is the callback implementation for set_report, I changed it a bit, maybe it makes it clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you prefer to pass the parameters anyway. We can keep it as is.
28ec369
to
e46a11f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer boot-protocol
instead of protocol-code
in the devicetree property, but that can be argued to be a style choice.
} | ||
|
||
if (usbd_ep_enqueue(c_data, buf)) { | ||
LOG_ERR("Failed to enqueue buffer"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we free buffer on error here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🔥 I will look into what happened here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is already fixed. Thank you
Add initial HID device support. Unlike the existing HID implementation, the new implementation uses a devicetree to instantiate a HID device. To the user, the HID device appears as a normal Zephyr RTOS device. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
Add support for the new HID implementation. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
e46a11f
to
8682953
Compare
Add HID keyboard sample for the new experimental USB device support. This is a limited and not fully compliant HID keyboard implementation. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>
8682953
to
b3867cc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good. Added a few more improvement propositions (and one small doc alignment)
/** | ||
* @brief Submit new input report | ||
* | ||
* Submit a new input report to be sent via the interrupt IN pipe. If sync is |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sync
no longer exists
|
||
if (ops->set_report == NULL) { | ||
errno = -ENOTSUP; | ||
LOG_DBG("Set Report not supported"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider LOG_WRN
to ensure user would see it.
return 0; | ||
} | ||
|
||
LOG_DBG("Set Protocol: %s", protocol ? "Report" : "Boot"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider an explicit comparison here to improve readability:
(protocol == HID_PROTOCOL_REPORT)
*/ | ||
|
||
struct hid_device_driver_api { | ||
int (*enable_output)(const struct device *dev, const bool enable); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable output seems unused. Can we remove it?
The size of the longest input report that the HID device can generate. | ||
This property is used to determine the buffer length used for transfers. | ||
|
||
in-polling-rate: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the naming of this is a bit odd, rate would imply a frequency, how about in-polling-period-us
? same for the out one
Initial HID device support.
Depends on a few patches from different PRs.