USB host HID and DFU classes implementation - #3
vgololobov wants to merge 10 commits into
Conversation
… 1 (REQ-VND-USB-002) Signed-off-by: Valentyn Gololobov <val@embedd.it>
d5f9154 to
02b08d9
Compare
626c88f to
327b369
Compare
… 3 (REQ-VND-USB-005) Signed-off-by: Valentyn Gololobov <val@embedd.it>
327b369 to
5c566f9
Compare
thenguyenyf
left a comment
There was a problem hiding this comment.
There is an issue with the current software model: the service built on top of the USBH HID is mutually exclusive with the choices made at build time. This model will prevent support of multiple devices with difference protocol (e.g 1 keyboard + 1 HID CFU device in an application).
I will recommend the new software model featuring per-instance protocol selection. The prototype proposal is below (refers from USBD core model and USBD init sample):
- DTS hierarchy:
&zephyr_uhc0 {
/* A specific Logitech keyboard (046d:c31c), forced boot protocol,
* routed to the input subsystem by a service in the app.
*/
hid_kbd: hid-device-0 {
compatible = "zephyr,usbh-hid-device";
protocol = "boot";
protocol-code = "keyboard";
in-report-size = <8>;
in-polling-period-us = <8000>;
};
/* Any non-boot HID interface, full report protocol, consumed raw. */
hid_raw: hid-device-1 {
compatible = "zephyr,usbh-hid-device";
match-class;
protocol = "report";
protocol-code = "none";
in-report-size = <64>;
report-max-fields = <48>;
report-max-variants = <8>;
};
};
- Device instance binding: USBH <- USBH HID devices
/*
* Using this macro as a helper to define USBH HID context from application
* _hid_ctx_name: USBH HID device context name
* _usb_controller_ctx: USBH controller context name, which defined by USBH_CONTROLLER_DEFINE
* _node_id: The device node ID of a "zephyr,usbh-hid-device"
* /
#define USBH_HID_DEFINE(_hid_ctx_name, _usb_controller_ctx, _node_id);There was a problem hiding this comment.
As I understand it, the enumeration flow in the current implementation is
GET_DESC -> SET_PROTOCOL -> SET_IDLE
But when investigate the common enumeration that is applied on HID USB stack of other platforms/vendor SDKs, it's usually
GET_DESC -> SET_IDLE -> GET_IDLE (optional) -> SET_PROTOCOL -> GET_PROTOCOL (optional) -> start interrupt EPs scheduling
The HID 1.11 specifications do not define a fixed enumeration flow; they simply define each request and its constraints. Therefore, I'm not sure if there is any issue with the current enumeration flow in this PR. Just add this comment as a note — it is not a request to be fixed.
drivers: usbh: hid: Added missing description for `in-idle-rate-ms` drivers: usbh: hid: Reverted SET_IDLE failure to non-fatal SET_IDLE is an optional request and an HID device may not support it; its failure should not compromise the probing function. drivers: usbh: hid: Added set_idle/get_idle as API calls Added `usbh_hid_set_idle_rate` and `usbh_hid_get_idle_rate` to the USB host HID driver. Added a test case to verify their implementation. drivers: usbh: hid: Minor fixes - Removed unused includes - Removed Doxygen description of private functions - Removed debug print function - Fixed physical limits default value drivers: usbh: Reverted unintended file change `uhc_mcux_common.c` drivers: usbh: hid: added clearing of IN endpoint in case of stall `hid.h` should only contain common macros; API and typedefes like the ones used by `hid_report.c` belong into `usbh_hid.h`. Moved those and changed namespacing accordingly. drivers: usbh: hid: Reverted formatting of untouched parts of `hid.h` drivers: usbh: hid: Defined DTS layout for HID devices Moved some configuration options for the USB HID host driver from macros to DTS properties. Now HID driver instances are tied to DTS nodes. drivers: usbh: hid: Fixed licensing attribution Signed-off-by: Valentyn Gololobov <val@embedd.it>
…-rtos#3 Review: - Fixed return value in 'getstate' fn - Eliminated test warnings - Used MIN instead of ternary operator - Updated license text Improvements: - Added recovery in case of DFU device internal status error - Updated dfu_dnload shell command, the FW text is passed from CLI - Compile dfu shell sub-commands only when CONFIG_USBH_DFU_CLASS is enabled - Remove unecessary 'dfu_get_status_msg' API - Increase default USBH_DFU_LIMIT_DATA_ALLOC_BYTES to 512 Bytes Signed-off-by: Valentyn Gololobov <val@embedd.it>
Added `usbh_hid_set_protocol` and `usbh_hid_get_protocol` to change the configured protocol of an HID device. Consequently moved the status of `boot_protocol` to RAM in the driver. Signed-off-by: Mattia Maldini <mattia.m@embedd.it>
nhutnguyenkc
left a comment
There was a problem hiding this comment.
I try the shell and see some mismatches in help. Could you please check and correct them?
- Added README for test cases - Fixed HID tests so that they can run on ek_ra8m2 - Added a test for the `set_protocol`/`get_protocol` API. Signed-off-by: Mattia Maldini <mattia.m@embedd.it> Signed-off-by: Valentyn Gololobov <val@embedd.it>
- Remove unnecessary includes - Update Readme and shell help for DFU commands - Use sizeof(struct) instead of numeric constant - Fix typos in comments Signed-off-by: Marian Cingel <marian.c@embedd.it> Signed-off-by: Valentyn Gololobov <val@embedd.it>
…concurrent deallocation and MISRA VLA rule
- Now the `tests/subsys/usb/hid` test also checks that the data provided
to the user callback is correct.
- Removed Doxygen comments from internal methods.
- If a transfer is cancelled it may still be needed by the underlying UHC
driver, so it must only be allocated by the callback.
- A couple of APIs had the `data[data_length]` notation, fixed.
- Verified functionality with MAX3421E, aligned the renesas uhc driver to
use `-EPIPE` error code for stall events.
Signed-off-by: Mattia Maldini <mattia.m@embedd.it>
Signed-off-by: Valentyn Gololobov <val@embedd.it>
- `hid_report` test now checks multiple examples of erroneous report descriptors. - `hid` test now checks invalid parameters and a non responsive device. - Fixes some deallocations of uninitialized pointers. Signed-off-by: Mattia Maldini <mattia.m@embedd.it>
z_arm64_sync_exc() demuxes SVC exceptions by the low 8 bits of the SVC immediate (svc #1: IRQ offload, svc zephyrproject-rtos#2: runtime except, svc #3: system call) without checking which exception level the SVC was taken from. On ARM64, SVC from EL0 and EL1 both report ESR EC 0x15, so hardware does not distinguish them. With CONFIG_USERSPACE=y and CONFIG_IRQ_OFFLOAD=y, any user thread can execute a raw svc #1 with x0/x1 pointing at an arbitrary kernel function and argument; the handler then runs 'blr x1' at EL1, i.e. privilege escalation / arbitrary code execution in kernel mode. The RISC-V port avoids this class of bug by distinguishing ECALLU/ECALLM via mcause. Note that CONFIG_IRQ_OFFLOAD depends on TEST (arch/Kconfig), so this is not reachable in a production build; only test builds that enable irq_offload() are exposed. This distinction matters for backport decisions. Read the saved SPSR from the ESF and route any SVC taken from EL0 to a user-only path that accepts just the system call and runtime-except immediates. The runtime-except entry must stay reachable from EL0 because user threads raise stack-check failures and oopses through it; z_arm64_do_kernel_oops() sanitizes the user-supplied reason, so this is not a privilege boundary. All other EL0 SVCs raise a fatal CPU exception. Kernel-mode SVCs keep their previous behavior, except that svc #3 from EL1 is no longer dispatched to the syscall handler, which kernel code never uses. Fixes zephyrproject-rtos#116254 Signed-off-by: Hongquan Li <hongquan.li@processmission.com>
|
Is this intended for submission to upstream Zephyr? FYI zephyrproject-rtos#118758 |
PoC stage according to SoW Block 1 (REQ-VND-USB-002)
for Renesas internal review