applications: nrf_desktop: Fix USB selective HID report subscription #25598

MarekPieta · 2025-11-14T15:43:43Z

Change updates HID state module to allow sharing the same HID subscriber priority among multiple subscribers. The HID subscribers with the same priority cannot simultaneously subscribe for the same HID input report.

Change is required to fix USB selective HID report subscription (USB HID instances share the same HID subscriber priority).

PR also introduces small HID state improvements.

Jira: NCSDK-35718

Copilot

Pull Request Overview

This PR fixes USB selective HID report subscription by enabling multiple HID subscribers to share the same priority level. The key change replaces the single active subscriber pointer with a priority-based system that tracks the active subscriber priority and allows multiple subscribers at the same priority to subscribe to different HID input reports.

Key Changes:

Replaced single active_subscriber pointer with active_subscriber_priority tracking to support multiple subscribers at the same priority level
Introduced get_active_report_state() function to find the active subscriber for a specific report ID
Refactored subscriber activation/deactivation logic to handle priority-based groups of subscribers

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

applications/nrf_desktop/src/modules/hid_state.c

NordicBuilder · 2025-11-14T15:48:10Z

CI Information

^{To view the history of this post, click the 'edited' button above}
Build number: 5

Inputs:

Sources:

sdk-nrf: PR head: f0a24424f48447d28739eebfb3eb027b546eebf2

more details

sdk-nrf:

PR head: f0a24424f48447d28739eebfb3eb027b546eebf2
merge base: 59453305e5144a0d65cc168a48d0d4014212546b
target head (main): 7a762b56f289684e7d8140f43d75ed46611a8212
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (1)

applications
│  ├── nrf_desktop
│  │  ├── src
│  │  │  ├── modules
│  │  │  │  │ hid_state.c

Outputs:

Toolchain

Version: df3cc9d822
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:df3cc9d822_e595b21c39

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 4
✅ Integration tests
- ✅ desktop52_verification - Skipped: Job was skipped as it succeeded in a previous run

Disabled integration tests

- test-fw-nrfconnect-nrf_lrcs_positioning
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-chip
- test-fw-nrfconnect-fem
- test-fw-nrfconnect-nfc
- test-fw-nrfconnect-nrf-iot_libmodem-nrf
- test-fw-nrfconnect-nrf-iot_lwm2m
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps-main
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread-main
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-wifi
- test-secdom-samples-public

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

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-14T16:12:58Z

applications/nrf_desktop/src/modules/hid_state.c

+			__ASSERT_NO_MSG(!rs->provider || !rs->provider->api ||
+					(rs->provider->linked_rs == rs));


[nitpick] The assertion logic is overly complex with multiple OR conditions. Consider splitting into separate assertions with descriptive messages explaining what each condition validates, improving debuggability when assertions fail.

We already use similar approach (combining multiple OR conditions under an assertion) in other places. As an alternative, we could make it:

if (rs->provider) { __ASSERT_NO_MSG(!rs->provider->api || (rs->provider->linked_rs == rs)); }

Still, I prefer the existing approach (it's also more consistent with existing codebase).
@alstrzebonski or @kapi-no, what do you think?

I think you can leave it as is.

Copilot · 2025-11-14T16:12:58Z

applications/nrf_desktop/src/modules/hid_state.c

-			link_providers(state.active_subscriber);
+		if (new_priority != state.active_subscriber_priority) {
+			update_active_subscriber_priority(new_priority);
+		} else {


The logic in the else branch is unclear. When new_priority equals active_subscriber_priority and we're disconnecting a subscriber with that priority, only unlinking this specific subscriber's providers may leave other subscribers at the same priority still linked. Consider adding a comment explaining why this partial unlink is correct.

Suggested change

} else {

} else {

/*

* Only unlink providers for the disconnecting subscriber.

* Other subscribers with the same priority may remain linked.

* This is intentional: each subscriber manages its own provider links,

* and unlinking only the disconnecting subscriber's providers avoids

* disrupting other active subscribers at the same priority.

*/

I think that introducing the extra comment here might not be necessary (code behavior is quite intuitive).

Copilot · 2025-11-14T16:12:59Z

applications/nrf_desktop/src/modules/hid_state.c

-		}
+	if (rs) {
+		link_provider_to_rs(provider, rs);
+		__ASSERT_NO_MSG(provider->linked_rs == rs);


[nitpick] The assertion on line 677 checks the result of the function call on line 676, but this validation is redundant since link_provider_to_rs should guarantee this relationship. If this is a critical invariant, consider moving it inside link_provider_to_rs or adding a comment explaining why post-condition verification is needed here.

Suggested change

__ASSERT_NO_MSG(provider->linked_rs == rs);

Assertion ensures that link_provider_to_rs does not early return on neither if (!provider->api) { nor if (provider->linked_rs == rs) {. I will add a small comment here.

github-actions · 2025-11-14T16:34:23Z

You can find the documentation preview for this PR here.

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Separate `get_input_report_idx` function to avoid code duplication. Jira: NCSDK-35718 Signed-off-by: Marek Pieta <[email protected]>

Change removes the `report_id` field from `struct report_state` (the field is no longer used). Jira: NCSDK-35718 Signed-off-by: Marek Pieta <[email protected]>

Change updates HID state module to allow sharing the same HID subscriber priority among multiple subscribers. The HID subscribers with the same priority cannot simultaneously subscribe for the same HID input report. Change is required to fix USB selective HID report subscription (USB HID instances share the same HID subscriber priority). Jira: NCSDK-35718 Signed-off-by: Marek Pieta <[email protected]>

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-17T14:22:54Z

applications/nrf_desktop/src/modules/hid_state.c

 };

-#define SUBSCRIBER_COUNT CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT
+#define SUBSCRIBER_COUNT		CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT


Add a comment explaining that SUBSCRIBER_PRIORITY_UNUSED (0) is a reserved value and valid subscriber priorities must be non-zero. This would clarify the assertion at line 495.

Suggested change

#define SUBSCRIBER_COUNT CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT

#define SUBSCRIBER_COUNT CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT

/*

* SUBSCRIBER_PRIORITY_UNUSED (0) is a reserved value.

* Valid subscriber priorities must be non-zero.

* This clarifies the assertion that checks for non-zero priorities.

*/

Copilot · 2025-11-17T14:22:54Z

applications/nrf_desktop/src/modules/hid_state.c

+			__ASSERT_NO_MSG(!rs->provider || !rs->provider->api ||
+					(rs->provider->linked_rs == rs));


This assertion condition is complex and difficult to parse. Consider adding a comment explaining what invariant is being checked, or split into separate assertions with descriptive messages.

Suggested change

__ASSERT_NO_MSG(!rs->provider || !rs->provider->api ||

(rs->provider->linked_rs == rs));

/* If provider is set, its API must be set. */

__ASSERT((!rs->provider) || (rs->provider->api), "Provider is set but API is NULL");

/* If provider is set and API is set, linked_rs must point to this report_state. */

__ASSERT((!rs->provider) || (!rs->provider->api) || (rs->provider->linked_rs == rs),

"Provider's linked_rs does not point to this report_state");

applications/nrf_desktop/src/modules/hid_state.c

Copilot

Pull Request Overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-17T15:17:57Z

applications/nrf_desktop/src/modules/hid_state.c

-	struct subscriber *prev_sub = state.active_subscriber;
+
+	/* The CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT must be large enough to handle all of the
+	 * simultaneously connected subscribers.


This comment should mention the consequence of an insufficient count (assertion failure at line 504) to help developers understand the severity and debugging implications.

Suggested change

* simultaneously connected subscribers.

* simultaneously connected subscribers.

* If this is insufficient, an assertion failure will occur here (line 504),

* halting execution to aid debugging.

Change introduces an additional assertion in HID state module to verify if `CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT` is big enough to handle all of the simultaneously connected HID subscribers. This simplifies debugging improper configuration. Jira: NCSDK-35718 Signed-off-by: Marek Pieta <[email protected]>

Copilot AI review requested due to automatic review settings November 14, 2025 15:43

MarekPieta requested a review from a team as a code owner November 14, 2025 15:43

NordicBuilder added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 14, 2025

Copilot AI reviewed Nov 14, 2025

View reviewed changes

applications/nrf_desktop/src/modules/hid_state.c Outdated Show resolved Hide resolved

applications/nrf_desktop/src/modules/hid_state.c Outdated Show resolved Hide resolved

MarekPieta force-pushed the multi_usb_hid_fix branch from f930b88 to 3371d07 Compare November 14, 2025 15:53

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 14, 2025

MarekPieta requested a review from Copilot November 14, 2025 16:12

Copilot AI reviewed Nov 14, 2025

View reviewed changes

MarekPieta force-pushed the multi_usb_hid_fix branch from 3371d07 to 872cb00 Compare November 17, 2025 10:49

NordicBuilder added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 17, 2025

MarekPieta requested a review from Copilot November 17, 2025 10:49

Copilot AI reviewed Nov 17, 2025

View reviewed changes

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 17, 2025

MarekPieta added 3 commits November 17, 2025 13:57

applications: nrf_desktop: Avoid code duplication in HID state

e519e4b

Separate `get_input_report_idx` function to avoid code duplication. Jira: NCSDK-35718 Signed-off-by: Marek Pieta <[email protected]>

applications: nrf_desktop: Remove unused datafield in HID state module

d174095

Change removes the `report_id` field from `struct report_state` (the field is no longer used). Jira: NCSDK-35718 Signed-off-by: Marek Pieta <[email protected]>

MarekPieta force-pushed the multi_usb_hid_fix branch from 872cb00 to ef529f3 Compare November 17, 2025 14:21

NordicBuilder added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 17, 2025

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 17, 2025

MarekPieta requested review from alstrzebonski and Copilot November 17, 2025 14:22

Copilot AI reviewed Nov 17, 2025

View reviewed changes

Copilot AI review requested due to automatic review settings November 17, 2025 15:17

NordicBuilder added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 17, 2025

Copilot AI reviewed Nov 17, 2025

View reviewed changes

MarekPieta force-pushed the multi_usb_hid_fix branch from 90ba6f9 to f0a2442 Compare November 17, 2025 15:20

MarekPieta removed the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Nov 17, 2025

kapi-no approved these changes Nov 19, 2025

View reviewed changes

		__ASSERT_NO_MSG(!rs->provider \|\| !rs->provider->api \|\|
		(rs->provider->linked_rs == rs));

-		} else {
+		} else {
+			/*
+			 * Only unlink providers for the disconnecting subscriber.
+			 * Other subscribers with the same priority may remain linked.
+			 * This is intentional: each subscriber manages its own provider links,
+			 * and unlinking only the disconnecting subscriber's providers avoids
+			 * disrupting other active subscribers at the same priority.
+			 */

-#define SUBSCRIBER_COUNT		CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT
+#define SUBSCRIBER_COUNT		CONFIG_DESKTOP_HID_STATE_SUBSCRIBER_COUNT
+/*
+ * SUBSCRIBER_PRIORITY_UNUSED (0) is a reserved value.
+ * Valid subscriber priorities must be non-zero.
+ * This clarifies the assertion that checks for non-zero priorities.
+ */

-			__ASSERT_NO_MSG(!rs->provider || !rs->provider->api ||
-					(rs->provider->linked_rs == rs));
+			/* If provider is set, its API must be set. */
+			__ASSERT((!rs->provider) || (rs->provider->api), "Provider is set but API is NULL");
+			/* If provider is set and API is set, linked_rs must point to this report_state. */
+			__ASSERT((!rs->provider) || (!rs->provider->api) || (rs->provider->linked_rs == rs),
+				 "Provider's linked_rs does not point to this report_state");

applications: nrf_desktop: Fix USB selective HID report subscription #25598

Are you sure you want to change the base?

applications: nrf_desktop: Fix USB selective HID report subscription #25598

Conversation

MarekPieta commented Nov 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

NordicBuilder commented Nov 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

MarekPieta Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

alstrzebonski Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

MarekPieta Nov 17, 2025 • 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!

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

Uh oh!

MarekPieta Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 14, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 17, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

NordicBuilder commented Nov 14, 2025 •

edited

Loading

MarekPieta Nov 17, 2025 •

edited

Loading