From a31dc972050a06a5a8012c3c20fd2bf8cb19f7e5 Mon Sep 17 00:00:00 2001 From: Marek Pieta Date: Mon, 22 Jul 2024 16:46:19 +0200 Subject: [PATCH] applications: nrf_desktop: Submit HID report sent event on USB SOF Delay submitting hid_report_sent_event until subsequent USB Start of Frame. Synchronizing providing HID data to USB SOF helps to avoid jitter related to USB polls. Jira: NCSDK-28224 Signed-off-by: Marek Pieta --- .../nrf_desktop/src/modules/Kconfig.usb_state | 21 ++++++- .../nrf_desktop/src/modules/usb_state.c | 60 +++++++++++++++++-- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/applications/nrf_desktop/src/modules/Kconfig.usb_state b/applications/nrf_desktop/src/modules/Kconfig.usb_state index 7ec325020fb..415de9cff8e 100644 --- a/applications/nrf_desktop/src/modules/Kconfig.usb_state +++ b/applications/nrf_desktop/src/modules/Kconfig.usb_state @@ -47,6 +47,15 @@ config DESKTOP_USB_PM_ENABLE help This enables small module that blocks power down if the USB is active. +config DESKTOP_USB_HID_REPORT_SENT_ON_SOF + bool "Submit HID report sent event on USB Start of Frame (SOF) [EXPERIMENTAL]" + select EXPERIMENTAL + help + Delay submitting hid_report_sent_event until subsequent USB Start of + Frame (SOF). Synchronizing submitting HID data to USB stack to USB SOF + helps to avoid jitter related to USB polls, but it also increases HID + data latency as HID report pipeline with length of 2 needs to be used. + config DESKTOP_USB_INIT_THREAD bool default y if SOC_SERIES_NRF54HX @@ -94,6 +103,7 @@ config DESKTOP_USB_STACK_LEGACY select USB_DEVICE_STACK select USB_DEVICE_HID select USB_DEVICE_REMOTE_WAKEUP if DESKTOP_USB_REMOTE_WAKEUP + select USB_DEVICE_SOF if DESKTOP_USB_HID_REPORT_SENT_ON_SOF endchoice @@ -173,10 +183,17 @@ endif # DESKTOP_USB_STACK_LEGACY if DESKTOP_USB_STACK_NEXT config USBD_HID_IN_BUF_COUNT + default 2 if DESKTOP_USB_HID_REPORT_SENT_ON_SOF default 1 help - nRF Desktop queues HID reports at the source. There is no need to use - multiple buffers in the IN pool per HID instance. + nRF Desktop queues HID reports at the source. Generally there is no + need to use multiple buffers in the IN pool per HID instance. + + An exception is use-case where HID reports are submitted from USB sent + callback (when synchronizing hid_report_sent to USB Start of Frame + (SOF) enforces using pipeline size of 2). An extra buffer must be + allocated to allow providing data to the USB next stack before the + previously allocated buffer is freed. config DESKTOP_USB_STACK_NEXT_DISABLE_ON_VBUS_REMOVAL bool diff --git a/applications/nrf_desktop/src/modules/usb_state.c b/applications/nrf_desktop/src/modules/usb_state.c index a4457403db2..c86d144ea07 100644 --- a/applications/nrf_desktop/src/modules/usb_state.c +++ b/applications/nrf_desktop/src/modules/usb_state.c @@ -47,8 +47,8 @@ LOG_MODULE_REGISTER(MODULE, CONFIG_DESKTOP_USB_STATE_LOG_LEVEL); #define REPORT_TYPE_FEATURE 0x03 #define USB_SUBSCRIBER_PRIORITY CONFIG_DESKTOP_USB_SUBSCRIBER_REPORT_PRIORITY -#define USB_SUBSCRIBER_PIPELINE_SIZE 0x01 -#define USB_SUBSCRIBER_REPORT_MAX 0x01 +#define USB_SUBSCRIBER_PIPELINE_SIZE (IS_ENABLED(CONFIG_DESKTOP_USB_HID_REPORT_SENT_ON_SOF) ? 2 : 1) +#define USB_SUBSCRIBER_REPORT_MAX USB_SUBSCRIBER_PIPELINE_SIZE /* The definitions are available and used only for USB legacy stack. * Define them explicitly if needed to allow compiling code without USB legacy stack. @@ -77,9 +77,10 @@ struct usb_hid_buf { struct usb_hid_device { const struct device *dev; uint32_t report_bm; - uint8_t hid_protocol; struct usb_hid_buf report_bufs[USB_SUBSCRIBER_REPORT_MAX]; + atomic_ptr_t report_sent_on_sof; uint32_t idle_duration[REPORT_ID_COUNT]; + uint8_t hid_protocol; bool report_enabled[REPORT_ID_COUNT]; bool enabled; }; @@ -398,6 +399,15 @@ static int set_report(const struct device *dev, uint8_t report_type, uint8_t rep return err; } +static void report_sent_sof(struct usb_hid_device *usb_hid) +{ + struct hid_report_sent_event *event = atomic_ptr_set(&usb_hid->report_sent_on_sof, NULL); + + if (event) { + APP_EVENT_SUBMIT(event); + } +} + static void report_sent(struct usb_hid_device *usb_hid, struct usb_hid_buf *buf, bool error) { __ASSERT_NO_MSG(buf); @@ -410,7 +420,28 @@ static void report_sent(struct usb_hid_device *usb_hid, struct usb_hid_buf *buf, event->report_id = report_id; event->subscriber = usb_hid; event->error = error; - APP_EVENT_SUBMIT(event); + + if (!IS_ENABLED(CONFIG_DESKTOP_USB_HID_REPORT_SENT_ON_SOF)) { + APP_EVENT_SUBMIT(event); + } else { + if (error) { + /* Synchronization to USB SOF is not used on send error. Instantly send + * enqueued report waiting for USB SOF to ensure proper HID report sent + * event order. + */ + report_sent_sof(usb_hid); + APP_EVENT_SUBMIT(event); + } else { + if (!atomic_ptr_cas(&usb_hid->report_sent_on_sof, NULL, event)) { + /* Instantly submit previous event to ensure proper HID report sent + * event order. + */ + LOG_WRN("Missing USB SOF between HID report sent callbacks"); + report_sent_sof(usb_hid); + (void)atomic_ptr_set(&usb_hid->report_sent_on_sof, event); + } + } + } usb_hid_buf_free(buf); @@ -601,6 +632,11 @@ static void update_usb_hid(struct usb_hid_device *usb_hid, bool enabled) usb_hid->enabled = enabled; + /* Inform about the sent HID report if queued. */ + if (IS_ENABLED(CONFIG_DESKTOP_USB_HID_REPORT_SENT_ON_SOF) && !enabled) { + report_sent_sof(usb_hid); + } + /* USB legacy stack does not notify app about sent report when no longer configured. * Pending report reset needs to be done after usb_hid->enabled field update to prevent * module from submitting HID report to stack and before broadcast of new USB HID subscriber @@ -909,6 +945,16 @@ static void usb_init_legacy_status_cb(enum usb_dc_status_code cb_status, const u } break; + case USB_DC_SOF: + if (IS_ENABLED(CONFIG_DESKTOP_USB_HID_REPORT_SENT_ON_SOF)) { + for (size_t i = 0; i < ARRAY_SIZE(usb_hid_device); i++) { + struct usb_hid_device *usb_hid = &usb_hid_device[i]; + + report_sent_sof(usb_hid); + } + } + /* Fall-through. */ + case USB_DC_SET_HALT: case USB_DC_CLEAR_HALT: /* Ignore */ @@ -1076,6 +1122,11 @@ static void report_sent_cb_next(const struct device *dev) report_sent(usb_hid, buf, error); } +static void sof_next(const struct device *dev) +{ + report_sent_sof(dev_to_usb_hid(dev)); +} + static int usb_init_next_hid_device_init(struct usb_hid_device *usb_hid_dev, uint32_t report_bm) { static const struct hid_device_ops hid_ops = { @@ -1086,6 +1137,7 @@ static int usb_init_next_hid_device_init(struct usb_hid_device *usb_hid_dev, uin .get_idle = get_idle_next, .set_protocol = protocol_change, .input_report_done = report_sent_cb_next, + .sof = sof_next, }; usb_hid_dev->report_bm = report_bm;