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

usb: device_next: add initial HID device support #65801

Merged
merged 5 commits into from
May 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 58 additions & 0 deletions dts/bindings/usb/zephyr,hid-device.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
# Copyright (c) 2023 Nordic Semiconductor ASA
# SPDX-License-Identifier: Apache-2.0

description: Bindings for HID device

compatible: "zephyr,hid-device"

include: base.yaml

properties:
interface-name:
type: string
description: |
HID device name. When this property is present, a USB device will use it
as the string descriptor of the interface.

protocol-code:
type: string
description: |
This property corresponds to the protocol codes defined in Chapter 4.3
of the HID specification. Only boot devices are required to set one of
the protocols, keyboard or mouse. For non-boot devices, this property is
not required or can be set to none.
- none: Device does not support the boot interface
Copy link
Contributor

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.

- keyboard: Device supports boot interface and keyboard protocol
- mouse: Device supports boot interface and mouse protocol
enum:
- none
Copy link
Contributor

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.

Copy link
Collaborator Author

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" ?

Copy link
Contributor

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.

- keyboard
- mouse

in-report-size:
type: int
required: true
description: |
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:
Copy link
Member

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

type: int
required: true
description: |
Input or output type reports polling rate in microseconds. For USB full
speed this could be clamped to 1ms or 255ms depending on the value.

out-report-size:
type: int
description: |
The size of the longest output report that the HID device can generate.
When this property is present, a USB device will use out pipe for output
reports, otherwise control pipe will be used for output reports.

out-polling-rate:
type: int
description: |
Output type reports polling rate in microseconds. For USB full
speed this could be clamped to 1ms or 255ms depending on the value.
This option is only effective if the out-report-size property is defined.
2 changes: 0 additions & 2 deletions include/zephyr/usb/class/usb_hid.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,9 +54,7 @@ struct hid_ops {
* the next transfer.
*/
hid_int_ready_callback int_in_ready;
#ifdef CONFIG_ENABLE_HID_INT_OUT_EP
hid_int_ready_callback int_out_ready;
#endif
};

/**
Expand Down
218 changes: 218 additions & 0 deletions include/zephyr/usb/class/usbd_hid.h
Original file line number Diff line number Diff line change
@@ -0,0 +1,218 @@
/*
* Copyright (c) 2023 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

/**
* @file
* @brief USBD HID device API header
*/

#ifndef ZEPHYR_INCLUDE_USBD_HID_CLASS_DEVICE_H_
#define ZEPHYR_INCLUDE_USBD_HID_CLASS_DEVICE_H_

#include <stdint.h>
#include <zephyr/device.h>
#include <zephyr/usb/class/hid.h>

#ifdef __cplusplus
extern "C" {
#endif

/**
* @brief USBD HID Device API
* @defgroup usbd_hid_device USBD HID device API
* @ingroup usb
* @{
*/

/*
* HID Device overview:
*
* +---------------------+
* | |
* | |
* | HID Device |
* | User "top half" |
* | of the device that +-------+
* | deals with input | |
* | sampling | |
* | | |
* | | |
* | ------------------- | |
* | | |
* | HID Device user | |
* | callbacks | |
* | handlers | |
* +---------------------+ |
* ^ | HID Device Driver API:
* | |
* set_protocol() | | hid_device_register()
* get_report() | | hid_device_submit_report(
* .... | | ...
* v |
* +---------------------+ |
* | | |
* | HID Device | |
* | "bottom half" |<------+
* | USB HID class |
* | implementation |
* | |
* | |
* +---------------------+
* ^
* v
* +--------------------+
* | |
* | USB Device |
* | Support |
* | |
* +--------------------+
*/

/** HID report types
* Report types used in Get/Set Report requests.
*/
enum {
HID_REPORT_TYPE_INPUT = 1,
HID_REPORT_TYPE_OUTPUT,
HID_REPORT_TYPE_FEATURE,
};

/**
* @brief HID device user callbacks
*
* Each device depends on a user part that handles feature, input, and output
* report processing according to the device functionality described by the
* report descriptor. Which callbacks must be implemented depends on the device
* functionality. The USB device part of the HID device, cannot interpret
* device specific report descriptor and only handles USB specific parts,
* transfers and validation of requests, all reports are opaque to it.
* Callbacks are called from the USB device stack thread and must not block.
*/
struct hid_device_ops {
/**
* The interface ready callback is called with the ready argument set
* to true when the corresponding interface is part of the active
* configuration and the device can e.g. begin submitting input
* reports, and with the argument set to false when the interface is no
* longer active. This callback is optional.
*/
void (*iface_ready)(const struct device *dev, const bool ready);

/**
* This callback is called for the HID Get Report request to get a
* feature, input, or output report, which is specified by the argument
* type. If there is no report ID in the report descriptor, the id
* argument is zero. The callback implementation must check the
* arguments, such as whether the report type is supported, and return
* a nonzero value to indicate an unsupported type or an error.
*/
int (*get_report)(const struct device *dev,
const uint8_t type, const uint8_t id,
const uint16_t len, uint8_t *const buf);

/**
* This callback is called for the HID Set Report request to set a
* feature, input, or output report, which is specified by the argument
* type. If there is no report ID in the report descriptor, the id
* argument is zero. The callback implementation must check the
* arguments, such as whether the report type is supported, and return
* a nonzero value to indicate an unsupported type or an error.
*/
int (*set_report)(const struct device *dev,
const uint8_t type, const uint8_t id,
const uint16_t len, const uint8_t *const buf);

/**
* Notification to limit intput report frequency.
* The device should mute an input report submission until a new
* event occurs or until the time specified by the duration value has

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

  1. It would be nice to mention that the duration is expressed in milliseconds (as opposed to the raw USB value which has 4ms resolution).
  2. It would be nice to group the USB-only part of the HID API (set_idle, get_idle, sof) separately, even a prefix would be nice, just as an indication of transport dependent support for these.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two things here:

1. It would be nice to mention that the duration is expressed in milliseconds (as opposed to the raw USB value which has 4ms resolution).

I removed */4U, it is 4 millisecond resolution now.

2. It would be nice to group the USB-only part of the HID API (set_idle, get_idle, sof) separately, even a prefix would be nice, just as an indication of transport dependent support for these.

It is USB-only part, it is prefixed with usbd_. However it evolves, there is no reason to add another prefix to anything. A callback is either optional or mandatory, it depends on the device. Adding prefixes does not change that.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed */4U, it is 4 millisecond resolution now.

I actually preferred having ms resolution, but I'm also vehemently against the whole idle logic, so as you see fit.

It is USB-only part, it is prefixed with usbd_.

The header is prefixed, but the API itself (types and methods) isn't, and also the explaining comment hinted at that this is supposed to be transport-independent. Moving on:

How about a suspend - resume callback, I'd see it as more relevant than having a sof callback? On the other hand these events are best handled on a whole device and not on an interface level.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed */4U, it is 4 millisecond resolution now.

I actually preferred having ms resolution, but I'm also vehemently against the whole idle logic, so as you see fit.

Changed it back and added comments about duration time resolution.
IMHO Get/Set Idle is an accident. Bluetooth was right to drop it.

How about a suspend - resume callback, I'd see it as more relevant than having a sof callback? On the other hand these events are best handled on a whole device and not on an interface level.

There is common notification mechanism for the bus events. I also tested HID with suspend/rwup-requests (L2->L0), there will be a follow up PR to enable it for all functions. Also note that this API is experimental; if any limitations become apparent, we can fix them quickly.

* elapsed. If a report ID is used in the report descriptor, the
* device must store the duration and handle the specified report
* accordingly. Duration time resolution is in miliseconds.
*/
void (*set_idle)(const struct device *dev,
const uint8_t id, const uint32_t duration);

/**
* If a report ID is used in the report descriptor, the device
* must implement this callback and return the duration for the
* specified report ID. Duration time resolution is in miliseconds.
*/
uint32_t (*get_idle)(const struct device *dev, const uint8_t id);

/**
* Notification that the host has changed the protocol from
* Boot Protocol(0) to Report Protocol(1) or vice versa.
*/
void (*set_protocol)(const struct device *dev, const uint8_t proto);

/**
* Notification that input report submitted with
* hid_device_submit_report() has been sent.
* If the device does not use the callback, hid_device_submit_report()
* will be processed synchronously.
*/
void (*input_report_done)(const struct device *dev);

/**
* New output report callback. Callback will only be called for reports
* received through the optional interrupt OUT pipe. If there is no
* interrupt OUT pipe, output reports will be received using set_report().
* If a report ID is used in the report descriptor, the host places the ID
* in the buffer first, followed by the report data.
*/
void (*output_report)(const struct device *dev, const uint16_t len,
const uint8_t *const buf);
Comment on lines +160 to +168

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not feed the data received through the OUT interrupt pipe also through the set_report callback instead? The upper level HID application should be agnostic to the detail of which pipe the output report arrived on, especially if the intention is to make this API transport independent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had changed it to use set_report, not sure if the commit was published in that PR, but changed it back because I found it confusing. In the Get/Set Report, there is an explicit report id parameter. Report id is optional, but then the id parameter is reserved 0 and everything is clear. For the input report, the IN endpoint should be used, although the input report can also be retrieved using Get Report, the presentation is slightly different. So different representations -> different interfaces. For output reports, I find it more intuitive to make them similar to input report interface and not abuse Set Report with ID 0. So it can be useful to know which interface is being used, passing it through the same callback as Set Report will remove this information. The application, like keyboard sample, can just call the same input report code with ID 0 if there are no IDs in the report descriptor.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well if there is a report ID parameter, that report ID will also occupy the first byte of the transferred report, so passing the report ID as an extra argument in the set_report is redundant. The same high level application might be ported to different targets with or without a dedicated OUT endpoint, which might create some inconvenience.

How about keeping the separate callback, but if it's not implemented by the application, the driver falls back to calling the set_report?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about keeping the separate callback, but if it's not implemented by the application, the driver falls back to calling the set_report?

I am very much against it. Having or not having an optional OUT endpoint is not a bagatelle. The HID device implementor needs to be aware of the differences. If there is no need for low latency output reports then OUT endpoint should not be used. Also, something like "HID RAW" for regular data transfer should not be used and is discouraged. Btw, ID parameter in set_report is not redundant, interface is different, type and id in set_report are from setup packet, report "payload" from the data stage.

Copy link

@ddavidebor ddavidebor Apr 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very much against it. Having or not having an optional OUT endpoint is not a bagatelle. The HID device implementor needs to be aware of the differences. If there is no need for low latency output reports then OUT endpoint should not be used.

I agree with @jfischer-no , it should be explicit, not a fallback.

I can infer that @benedekkupper would like a more polished, higher-lever HID experience, probably thinking about HID over GATT + HID over USB. That's great as it's own thing, but for the USB, it needs to stick to the way USB HID works, or it will cause a lot of headaches.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, ID parameter in set_report is not redundant, interface is different, type and id in set_report are from setup packet, report "payload" from the data stage.

They are redundant in the sense that the spec requires them to be the same, even if the same values appears twice on the bus.

Also, something like "HID RAW" for regular data transfer should not be used and is discouraged.

How do you define "regular data transfer"? HID has seen a lot of use as a general purpose middleware transport, just on top of my head there's FIDO, CMSIS DAP and NXP's KBOOT flashing protocols.

In the end I'm ok with the current situation, I understand the justifications now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you define "regular data transfer"? HID has seen a lot of use as a general purpose middleware transport, just on top of my head there's FIDO, CMSIS DAP and NXP's KBOOT flashing protocols.

By regular, I mean something that uses reports as a data payload and does not require periodic transfers. CMSIS-DAP moved to bulk transfers, e.g. discussion in #53798

/**
* Optional Start of Frame (SoF) event callback.
* There will always be software and hardware dependent jitter and
* latency. This should be used very carefully, it should not block
* and the execution time should be quite short.
*/
void (*sof)(const struct device *dev);
};

/**
* @brief Register HID device report descriptor and user callbacks
*
* The device must register report descriptor and user callbacks before
* USB device support is initialized and enabled.
*
* @param[in] dev Pointer to HID device
* @param[in] rdesc Pointer to HID report descriptor
* @param[in] rsize Size of HID report descriptor
* @param[in] ops Pointer to HID device callbacks
*/
int hid_device_register(const struct device *dev,
const uint8_t *const rdesc, const uint16_t rsize,
const struct hid_device_ops *const ops);

/**
* @brief Submit new input report
*
* Submit a new input report to be sent via the interrupt IN pipe. If sync is
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sync no longer exists

* true, the functions will block until the report is sent.
* If the device does not provide input_report_done() callback,
* hid_device_submit_report() will be processed synchronously.
*
* @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.
Copy link
Collaborator

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)

*
* @return 0 on success, negative errno code on fail.
*/
int hid_device_submit_report(const struct device *dev,
const uint16_t size, const uint8_t *const report);

/**
* @}
*/

#ifdef __cplusplus
}
#endif

#endif /* ZEPHYR_INCLUDE_USBD_HID_CLASS_DEVICE_H_ */
9 changes: 9 additions & 0 deletions samples/subsys/usb/hid-keyboard/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# SPDX-License-Identifier: Apache-2.0

cmake_minimum_required(VERSION 3.20.0)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(hid-keyboard)

include(${ZEPHYR_BASE}/samples/subsys/usb/common/common.cmake)
FILE(GLOB app_sources src/*.c)
target_sources(app PRIVATE ${app_sources})
9 changes: 9 additions & 0 deletions samples/subsys/usb/hid-keyboard/Kconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
# Copyright (c) 2023 Nordic Semiconductor ASA
# SPDX-License-Identifier: Apache-2.0

# Source common USB sample options used to initialize new experimental USB
# device stack. The scope of these options is limited to USB samples in project
# tree, you cannot use them in your own application.
source "samples/subsys/usb/common/Kconfig.sample_usbd"

source "Kconfig.zephyr"
36 changes: 36 additions & 0 deletions samples/subsys/usb/hid-keyboard/README.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
.. zephyr:code-sample:: usb-hid-keyboard
:name: USB HID keyboard
:relevant-api: usbd_api usbd_hid_class input_interface

Implement a basic HID keyboard device.

Overview
********

This sample application demonstrates the HID keyboard implementation using the
new experimental USB device stack.

Requirements
************

This project requires an experimental USB device driver (UDC API) and uses the
:ref:`input` API. There must be a :dtcompatible:`gpio-keys` group of buttons
or keys defined at the board level that can generate input events.
At least one key is required and up to four can be used. The first three keys
are used for Num Lock, Caps Lock and Scroll Lock. The fourth key is used to
report HID keys 1, 2, 3 and the right Alt modifier at once.

The example can use up to three LEDs, configured via the devicetree alias such
as ``led0``, to indicate the state of the keyboard LEDs.

Building and Running
********************

This sample can be built for multiple boards, in this example we will build it
for the nRF52840DK board:

.. zephyr-app-commands::
:zephyr-app: samples/subsys/usb/hid-keyboard
:board: nrf52840dk/nrf52840
:goals: build flash
:compact:
15 changes: 15 additions & 0 deletions samples/subsys/usb/hid-keyboard/app.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright (c) 2023 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
hid_dev_0: hid_dev_0 {
compatible = "zephyr,hid-device";
interface-name = "HID0";
protocol-code = "keyboard";
in-report-size = <64>;
in-polling-rate = <1000>;
};
};
14 changes: 14 additions & 0 deletions samples/subsys/usb/hid-keyboard/large_in_report.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
/*
* Copyright (c) 2023 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

/ {
hid_dev_0: hid_dev_0 {
compatible = "zephyr,hid-device";
interface-name = "HID0";
in-report-size = <256>;
in-polling-rate = <1000>;
};
};
15 changes: 15 additions & 0 deletions samples/subsys/usb/hid-keyboard/large_out_report.overlay
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
/*
* Copyright (c) 2023 Nordic Semiconductor ASA
*
* SPDX-License-Identifier: Apache-2.0
*/

#include "large_in_report.overlay"

/ {
hid_dev_0: hid_dev_0 {
compatible = "zephyr,hid-device";
out-report-size = <128>;
out-polling-rate = <16000>;
};
};
Loading
Loading