applications: nrf_desktop: Integrate USB next stack #15557

MarekPieta · 2024-05-29T06:08:28Z

Change integrates USB next stack to the application. USB HID next class is supported too.

Jira: NCSDK-27014

NordicBuilder · 2024-05-29T06:11:44Z

Test specification

CI/Jenkins/NRF

Integration Platforms

CI/Jenkins/integration

Test Module	File based changes	Manually selected	West overwrite
desktop52_verification	X

Detailed information of selected test modules

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-05-29T06:45:46Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

kapi-no · 2024-05-29T06:47:11Z

applications/nrf_desktop/configuration/nrf52820dongle_nrf52820/app.overlay

Why do we add support for all of our nRF Desktop USB targets? Didn't we agree to add support for one legacy platform in one configuration variant(e.g. gaming mouse / nrf52840dk) to reduce the scope of this change right before the release?

We need to add supported platforms to sample.yaml file so they run in CI

I think we can add DTS configuration to all of the platforms (change is quite simple and allows testing locally on any supported board by just setting a single Kconfig option during build). Keep in mind that legacy USB stack ignores the USB HID instances configured in DTS anyway, so this part should not have negative impact on default configurations.

I agree that we should add configurations to CI. What do you think about the following set?

nrf52840dk (prj.conf + prj_keyboard.conf + prj_dongle.conf)

nrf52840gmouse (prj.conf)

nrf52840dongle (prj.conf)

The configuration set looks good.

We can do many things at the cost of the execution speed. Bigger PRs tend to be merged more slowly (harder to review, more conflict-prone, more review discussion points). However, let's keep these configurations as you already added them.

The sole purpose for adding support for this feature in the nRF Desktop right before the NCS release is to unlock the USB support for the nRF54H20 and we should keep this in mind.

kapi-no · 2024-05-29T06:56:49Z

applications/nrf_desktop/src/modules/usb_state.c

@@ -77,6 +93,8 @@ BUILD_ASSERT(!IS_ENABLED(CONFIG_DESKTOP_HID_STATE_ENABLE) ||
 	     IS_ENABLED(CONFIG_DESKTOP_USB_SELECTIVE_REPORT_SUBSCRIPTION) ||
 	     (ARRAY_SIZE(usb_hid_device) <= 1));

+static struct usbd_contex *usbd_ctx;


Is there a typo in USBD API? it would be great to report this to @jfischer-no

Suggested change

static struct usbd_contex *usbd_ctx;

static struct usbd_context *usbd_ctx;

or

Suggested change

static struct usbd_contex *usbd_ctx;

static struct usbd_ctx *usbd_ctx;

Hi @jfischer-no, @tmon-nordic
Could you explain the naming scheme here please? I guess there is a reason why it's called usbd_contex

static struct usbd_ctx *usbd_ctx; is wrong, there is no struct usbd_ctx defined anywhere.

The struct is called struct usbd_contex. If you want to rename it (I prefer struct usbd_context), then the rename should first go upstream.

The question is: Do you recall why it's currently called usbd_contex in upstream? It seems a bit like a typo, but I guess there is a reason why it's named like that.

This is just a typo and the only reason it stays is that it needs a rather big PR to change it everywhere. But if you send PR upstream, I believe it can go in rather quickly (and we could cherry-pick it to NCS as nrf fromtree).

We are just letting you know about the potential improvement for the upstream USB API. This typo isn't in any way a blocker for this PR. We can align it once it is patched upstream

zephyrproject-rtos/zephyr#73463

kapi-no · 2024-05-29T06:59:44Z

applications/nrf_desktop/src/modules/usb_state.c

+{
+	int err;
+
+	USBD_DEVICE_DEFINE(usbd, DEVICE_DT_GET(DT_NODELABEL(usbd)),


Can we simplify this part and simply define the USBD instance at the file-level scope instead of passing it in return value?

I wanted to keep all of the functions and macros that are specific to USB next stack separated from implementation of generic functionalities. Whole USB next configuration is currently kept together too. That's why I would prefer to keep it as is.
The usbd_ctx pointer is an exception here, because we need to access it if we want to trigger USB remote wakeup with USB next stack.

I think the remote wakeup should rather be up to the stack, which is precisely what zephyrproject-rtos/zephyr#73362 does.

I wanted to keep all of the functions and macros that are specific to USB next stack separated from implementation of generic functionalities. Whole USB next configuration is currently kept together too. That's why I would prefer to keep it as is. The usbd_ctx pointer is an exception here, because we need to access it if we want to trigger USB remote wakeup with USB next stack.

I don't understand why defining the instance in the same line as you define currently pointer to it would invalidate your assumptions

I think the remote wakeup should rather be up to the stack, which is precisely what zephyrproject-rtos/zephyr#73362 does.

That may be a nice improvement. I will look into it more in my subsequent PRs.

I wanted to keep all of the functions and macros that are specific to USB next stack separated from implementation of generic functionalities. Whole USB next configuration is currently kept together too. That's why I would prefer to keep it as is. The usbd_ctx pointer is an exception here, because we need to access it if we want to trigger USB remote wakeup with USB next stack.

I don't understand why defining the instance in the same line as you define currently pointer to it would invalidate your assumptions

It is "bundled" with assigning VID and PID. I prefer to assign all of the properties in one place if possible. Once improvement linked by @tmon-nordic goes in, we will hopefully be able to get rid of the global reference pointer.

kapi-no · 2024-05-29T07:00:09Z

applications/nrf_desktop/src/modules/usb_state.c

+	USBD_DESC_SERIAL_NUMBER_DEFINE(serial_number);
+
+	/* Maximum power consumption of a device: 250 * 2 mA = 500 mA. */
+	static const uint8_t max_power = 250;


shouldn't we make this setting configurable?

We need to actually align it for both USB stacks. Added follow-up ticket: https://nordicsemi.atlassian.net/browse/NCSDK-27658

kapi-no · 2024-05-29T07:10:10Z

applications/nrf_desktop/src/modules/usb_state.c

@@ -480,8 +487,8 @@ static void usb_wakeup(void)
 {
 	int err = 0;

-	if (IS_ENABLED(CONFIG_DESKTOP_USB_STACK_NEXT)) {
-		LOG_WRN("usb_wakeup not integrated for USB next");
+	if (IS_ENABLED(CONFIG_DESKTOP_USB_STACK_NEXT) && usbd_ctx) {


If we get rid of this usbd_ctx pointer and define the instance at the file scope, we can use the instance status field to check if it is enabled.

See #15557 (comment)

kapi-no · 2024-05-29T07:17:43Z

applications/nrf_desktop/src/modules/usb_state.c

@@ -1016,15 +1154,21 @@ static struct usbd_contex *usb_init_next_usbd_init(void)

 static int usb_init_next(void)
 {
+	int err = usb_init_next_hids_init();


why _hids_ and not _hid_ suffix in the name of this function?

We initialize multiple USB HID instances here. It's also consistent with naming used for legacy USB stack: usb_init_legacy_hids_init

HIDS is the Bluetooth construct as far as I can tell HID service
USB uses HID classes HIDC would be more sutiable

I used HIDs and plural form of HID (actually I didn't even think about HIDS GATT Service here). I can rename if needed

kapi-no · 2024-05-29T07:19:18Z

applications/nrf_desktop/src/modules/usb_state.c

+		if ((state == USB_STATE_SUSPENDED) && is_usb_active_next()) {
+			broadcast_usb_state(USB_STATE_ACTIVE);
+		} else {
+			broadcast_usb_state(new_state);
+		}


you could move this change to the previous commit and declare a placeholder function is_usb_active_next function that always returns false.

Since USB_STATE_ACTIVE support was introduced together with adding USB HID support, I think we could keep it as is. The changes go in together anyway.

kapi-no · 2024-05-29T07:58:39Z

applications/nrf_desktop/src/modules/usb_state.c

+			uint8_t temp[report_size] __aligned(sizeof(void *));
+
+			memcpy(temp, report_buffer, report_size);
+			err = hid_device_submit_report(usb_hid->dev, report_size, temp);


Can we require the report-sending modules to always align the report buffer in memory so it is aligned? Then, we could simply assert on unaligned buffers.

The modules rely on allocator function from app Event Manager (this eventually leads to k_malloc). The k_malloc allocations are aligned by default so it should be fine. The only problem here is HID boot protocol handling. For the boot protocol, we omit the first byte which "breaks" the alignment. That's why we apply this simple workaround here.

NordicBuilder · 2024-05-29T12:21:37Z

Memory footprint analysis revealed the following potential issues

applications.nrf_desktop.zdebug_4llpmconn[nrf52840dongle/nrf52840]: RAM size increased by 312[B] in comparison to the main[643c3f4] branch. - link (cc: @MarekPieta)

Note: This message is automatically posted and updated by the CI (latest/sdk-nrf/PR-15557/6)

Do not enable legacy USB stack by default. It is not required for the board and leads to problems with using USB next stack. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

Change integrates USB next stack to the application. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

Change adds USB HID next APIs to the application. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

…B next Change adds USB HID next configuration to DTS configurations. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

Change expands `sample.yaml` file to build some of the configurations with USB next stack. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

MarekPieta requested review from kapi-no, zycz, alstrzebonski and mkapala-nordic May 29, 2024 06:08

MarekPieta requested a review from tejlmand as a code owner May 29, 2024 06:08

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 29, 2024

MarekPieta force-pushed the usb_next_pr branch 2 times, most recently from f23c724 to 8062475 Compare May 29, 2024 07:22

kapi-no reviewed May 29, 2024

View reviewed changes

kapi-no added this to the 2.7.0 milestone May 29, 2024

carlescufi requested a review from jfischer-no May 29, 2024 09:59

MarekPieta force-pushed the usb_next_pr branch from 637b264 to c2477a8 Compare May 29, 2024 11:32

MarekPieta requested a review from anangl as a code owner May 29, 2024 11:32

MarekPieta requested review from kapi-no and tmon-nordic May 29, 2024 11:34

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 29, 2024

mkapala-nordic approved these changes May 29, 2024

View reviewed changes

kapi-no approved these changes May 29, 2024

View reviewed changes

MarekPieta added 5 commits May 29, 2024 15:05

boards: nordic: nrf52840gmouse: Do not enable legacy USB by default

af6c99f

Do not enable legacy USB stack by default. It is not required for the board and leads to problems with using USB next stack. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

applications: nrf_desktop: Integrate USB next stack

51e62a6

Change integrates USB next stack to the application. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

applications: nrf_desktop: Add USB HID next APIs

4ef5389

Change adds USB HID next APIs to the application. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

applications: nrf_desktop: Align DTS configurations to allow using US…

7258aee

…B next Change adds USB HID next configuration to DTS configurations. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

applications: nrf_desktop: Test USB next stack integration in CI

a5fbcbf

Change expands `sample.yaml` file to build some of the configurations with USB next stack. Jira: NCSDK-27014 Signed-off-by: Marek Pieta <Marek.Pieta@nordicsemi.no>

MarekPieta force-pushed the usb_next_pr branch from c2477a8 to a5fbcbf Compare May 29, 2024 13:07

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 29, 2024

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label May 29, 2024

rlubos merged commit aa4d9c2 into nrfconnect:main May 29, 2024

	static struct usbd_contex *usbd_ctx;
	static struct usbd_context *usbd_ctx;

	static struct usbd_contex *usbd_ctx;
	static struct usbd_ctx *usbd_ctx;

applications: nrf_desktop: Integrate USB next stack #15557

applications: nrf_desktop: Integrate USB next stack #15557

Uh oh!

Conversation

MarekPieta commented May 29, 2024

Uh oh!

NordicBuilder commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test specification

Uh oh!

NordicBuilder commented May 29, 2024

Uh oh!

kapi-no May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarekPieta May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarekPieta May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MarekPieta May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NordicBuilder commented May 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

NordicBuilder commented May 29, 2024 •

edited

Loading

kapi-no May 29, 2024 •

edited

Loading

MarekPieta May 29, 2024 •

edited

Loading

MarekPieta May 29, 2024 •

edited

Loading

MarekPieta May 29, 2024 •

edited

Loading

NordicBuilder commented May 29, 2024 •

edited

Loading