applications: nrf_desktop: Support passthrough reports in hid state #19690

pdunaj · 2024-12-20T13:41:12Z

Allow the hid state module to handle reports created by external modules. These reports will still be sent according to the his state rules.

NordicBuilder · 2024-12-20T13:43:40Z

CI Information

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

Inputs:

Sources:

sdk-nrf: PR head: 911b0e43413099845edbb17a32fdfeca3643fe2b

more details

sdk-nrf:

PR head: 911b0e43413099845edbb17a32fdfeca3643fe2b
merge base: 1133ff3eb088ec3b3082b2e52a392aa85c577707
target head (main): 6e0cb3a5efefa5417863050198c99afacfcd9686
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 (4)

applications
│  ├── nrf_desktop
│  │  ├── configuration
│  │  │  ├── common
│  │  │  │  │ hid_report_desc.h
│  │  ├── src
│  │  │  ├── modules
│  │  │  │  ├── Kconfig.hid_state
│  │  │  │  ├── hid_state.c
│  │  │  │  │ usb_state.c

Outputs:

Toolchain

Version: b77d8c1312
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:b77d8c1312_912848a074

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

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister
- sdk-nrf test count: 96
✅ Integration tests
- ✅ desktop52_verification

Disabled integration tests

- doc-internal
- test_ble_nrf_config
- test-fw-nrfconnect-apps
- test-fw-nrfconnect-ble_mesh
- test-fw-nrfconnect-ble_samples
- test-fw-nrfconnect-boot
- 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_mosh
- test-fw-nrfconnect-nrf-iot_nrf_provisioning
- test-fw-nrfconnect-nrf-iot_positioning
- test-fw-nrfconnect-nrf-iot_samples
- test-fw-nrfconnect-nrf-iot_serial_lte_modem
- test-fw-nrfconnect-nrf-iot_thingy91
- test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
- test-fw-nrfconnect-nrf_crypto
- test-fw-nrfconnect-ps
- test-fw-nrfconnect-rpc
- test-fw-nrfconnect-rs
- test-fw-nrfconnect-tfm
- test-fw-nrfconnect-thread
- test-fw-nrfconnect-zigbee
- test-low-level
- test-sdk-audio
- test-sdk-dfu
- test-sdk-find-my
- test-sdk-mcuboot
- test-sdk-pmic-samples
- test-sdk-sidewalk
- test-sdk-wifi
- test-secdom-samples-public

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

pdunaj · 2024-12-20T13:49:10Z

why @nrfconnect/ncs-co-build-system is needed for review of app specific kconfig?
@carlescufi , can it be fixed for our apps or is this some global setting on all kconfigs?

nordicjm · 2024-12-20T13:51:56Z

why @nrfconnect/ncs-co-build-system is needed for review of app specific kconfig? @carlescufi , can it be fixed for our apps or is this some global setting on all kconfigs?

Because of the amount of abuse and wrong things in Kconfigs all over the tree that have improved substantially now things are actually reviewed and issues pointed out

NordicBuilder · 2024-12-20T13:58:00Z

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 Publish GitHub Action.

pdunaj · 2024-12-20T15:44:24Z

Appease pedantic checkpatch

nordicjm · 2024-12-23T07:52:26Z

applications/nrf_desktop/src/modules/Kconfig.hid_state

@@ -60,6 +60,13 @@ config DESKTOP_HID_EVENT_QUEUE_SIZE
 	help
 	  Size of the HID event queue.

+config DESKTOP_HID_STATE_RAW_REPORTS
+	bool


missing a prompt?

I think we could add the prompt

I have not added prompt as I want this to be selected only when specific type of report is actually enabled.

Then none of this makes sense, the help text clearly states it can be enabled and there is no way to enable it, and none of this code is tested

I have not added prompt as I want this to be selected only when specific type of report is actually enabled.

but no Kconfigs are selecting this symbol.
And which specific type of reports are you referring to ?
The setting has no depends on.

I think help text should be improved so that NCS users at least are able to understand when / how this setting is supposed to be used / why it is a hidden (prompt-less) setting.

Then none of this makes sense

It can be that I have simply failed at writing simple enough help. Have not focus on this really. I have checked help text in various promptless options in Zephyr repo. I see that the word "Enable/d" is often used so I expect it's not impossible to get that enable means select in such cases.

I think help text should be improved so that NCS users...

@tejlmand , this change is part of more work needed, which is not public. I don't expect this will impact a generic NCS user since this app is very specific. Anyway this time waste is a good example of what I meant in my original comment. You clearly don't understand what is being done (which is fine as you have your own problems) and we have entered a bikeshedding discussion which is simply a time waste.

I will need to run one more edit as test failed and will add additional explanation to make you happy. Let's focus on something more productive on both ends.

MarekPieta · 2025-01-03T07:03:11Z

applications/nrf_desktop/src/modules/Kconfig.hid_state

@@ -60,6 +60,13 @@ config DESKTOP_HID_EVENT_QUEUE_SIZE
 	help
 	  Size of the HID event queue.

+config DESKTOP_HID_STATE_RAW_REPORTS
+	bool


I think we could add the prompt

MarekPieta · 2025-01-03T07:04:47Z

applications/nrf_desktop/src/modules/hid_state.c

+#define INPUT_REPORT_DATA_COUNT   (ARRAY_SIZE(input_reports) -					\
+				   IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE) -	\
+				   IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_KEYBOARD))


Suggested change

#define INPUT_REPORT_DATA_COUNT (ARRAY_SIZE(input_reports) - \

IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE) - \

IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_KEYBOARD))

#define INPUT_REPORT_DATA_COUNT (ARRAY_SIZE(input_reports) - \

IS_ENABLED(CONFIG_DESKTOP_HID_BOOT_INTERFACE_MOUSE) - \

IS_ENABLED(CONFIG_DESKTOP_HID_BOOT_INTERFACE_KEYBOARD))

Also in other places

MarekPieta · 2025-01-03T07:06:30Z

applications/nrf_desktop/src/modules/hid_state.c

+{
+	if (raw_data->data) {
+		k_free((void *)raw_data->data);
+		__ASSERT_NO_MSG(false);


What is purpose of this assert? Shouldn't we submit hid_report_sent_event informing about an error ?

This should never occur - I have left the free in case this happens for any reason but we can stick to assert only

MarekPieta · 2025-01-03T07:10:11Z

applications/nrf_desktop/src/modules/hid_state.c

+
+	/* Encode report. */
+	size_t report_size = rd->raw_data.size;
+	struct hid_report_event *event = new_hid_report_event(report_size);


Maybe we could enqueue the raw report instantly as hid_report_event and just submit it here? That would significantly speed up processing of the external HID reports (1 less allocation+free, no copy)

Or maybe we could even add Event Manager API that would allow to "steal" processed event (app_event_steal(const struct app_event_header *aeh)). The API could be called only while processing a given event (we could assert that) and it would be used to mark event as "stolen". Event Manager would consume the "stolen" event (later subscribers would not see the event) and pass the memory to module that "stole" the event (then the module is responsible for submitting/freeing the event).

"Stealing" events could be quite generic and useful feature. We might even think about splitting report formatting functionality out of HID state completely if we opt for adding it.

I was thinking about this. Also about moving the report composition outside of this file and so splitting the responsibilities. At this point I don't want to make such a big rework as there is no value in it.

MarekPieta · 2025-01-03T07:16:13Z

applications/nrf_desktop/src/modules/hid_state.c

@@ -923,7 +992,7 @@ static bool report_send(struct report_state *rs,
 	}

 	/* Ensure that report marked as send_always will be sent. */
-	if (send_always && !report_sent) {
+	if (try_send_always && !report_sent) {


Should we set it also in case we try to send raw HID report but have nothing to send? Maybe we need ability to generate an empty external report to "clear state"?

There was no use case for this. I would stick only to what is required to achieve the required functionality.

MarekPieta · 2025-01-03T07:24:38Z

applications/nrf_desktop/src/modules/usb_state.c

-	    (usb_hid->report_bm & BIT(REPORT_ID_MOUSE))) {
-		struct hid_report_subscription_event *event =
-			new_hid_report_subscription_event();
+	for (size_t i = 0; i < ARRAY_SIZE(input_reports); i++) {


USB related changes looks good already. Maybe we could add it under a separate PR to speed up introducing this part ?

Kept it here as it is a change following the input_reports.
I don't see a reason why we would spend more then few more minutes in this PR, why opening new PR then?

MarekPieta · 2025-01-03T07:49:40Z

applications/nrf_desktop/src/modules/hid_state.c

+		LOG_INF("Report id %u was not subscribed", report_id);
+		error = true;
+	} else {
+		rd->raw_data.data = k_malloc(size + sizeof(report_id));


Can we handle case with queuing multiple reports (or at least properly emit an error here)?

Don't see what you refer to.
This will work similarly to consumer/system ctrl report - no pipeline. It serves the purpose it was created for.
Error is handled, unless I missed something...

MarekPieta · 2025-01-03T08:23:57Z

applications/nrf_desktop/src/modules/hid_state.c

+		/* Nothing to send. */
+		return false;
+	}
+	LOG_WRN("Sending raw data report %d of size %zu", rs->report_id, rd->raw_data.size);


LOG_DBG/LOG_INF ?

Removed the log - there is no reason as this occurrence will be logged in the event manager.

MarekPieta · 2025-01-03T08:27:35Z

applications/nrf_desktop/src/modules/hid_state.c

-
-		state.report_data[data_id].items.item_count_max = KEYBOARD_REPORT_KEY_COUNT_MAX;
+	if (IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_KEYBOARD_SUPPORT) ||
+	    IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE)) {


Suggested change

IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE)) {

IS_ENABLED(CONFIG_DESKTOP_HID_BOOT_INTERFACE_KEYBOARD)) {

MarekPieta · 2025-01-03T08:28:34Z

applications/nrf_desktop/src/modules/hid_state.c

-		report_data_index[REPORT_ID_MOUSE] = data_id;
-		report_state_index[REPORT_ID_MOUSE] = state_id;
+	if (IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_MOUSE_SUPPORT) ||
+	    IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE)) {


Suggested change

IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE)) {

IS_ENABLED(CONFIG_DESKTOP_HID_BOOT_INTERFACE_MOUSE)) {

pdunaj · 2025-01-08T11:00:32Z

why @nrfconnect/ncs-co-build-system is needed for review of app specific kconfig? @carlescufi , can it be fixed for our apps or is this some global setting on all kconfigs?

Because of the amount of abuse and wrong things in Kconfigs all over the tree that have improved substantially now things are actually reviewed and issues pointed out

Each should focus on making they part work and be done on time, instead of worrying about "abuse and wrong things" in source they are not responsible for.

pdunaj · 2025-01-08T11:29:20Z

rebased

Allow the hid state module to handle reports created by external modules. These reports will still be sent according to the his state rules. Signed-off-by: Pawel Dunaj <[email protected]>

pdunaj · 2025-01-08T15:44:44Z

Fixed issue that appeared after fixing boot report config option names.
Improved the raw report help.

pdunaj requested a review from MarekPieta December 20, 2024 13:41

pdunaj requested review from a team as code owners December 20, 2024 13:41

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

pdunaj force-pushed the hid_raw_reports branch 2 times, most recently from 7ce0144 to 4a866a4 Compare December 20, 2024 15:43

nordicjm reviewed Dec 23, 2024

View reviewed changes

MarekPieta requested changes Jan 3, 2025

View reviewed changes

pdunaj force-pushed the hid_raw_reports branch 2 times, most recently from 4e1ae07 to 2ad29ca Compare January 8, 2025 11:28

applications: nrf_desktop: Support passthrough reports in hid state

911b0e4

Allow the hid state module to handle reports created by external modules. These reports will still be sent according to the his state rules. Signed-off-by: Pawel Dunaj <[email protected]>

pdunaj force-pushed the hid_raw_reports branch from 2ad29ca to 911b0e4 Compare January 8, 2025 15:43

pdunaj requested a review from MarekPieta January 9, 2025 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

applications: nrf_desktop: Support passthrough reports in hid state #19690

applications: nrf_desktop: Support passthrough reports in hid state #19690

pdunaj commented Dec 20, 2024

NordicBuilder commented Dec 20, 2024 •

edited

Loading

pdunaj commented Dec 20, 2024

nordicjm commented Dec 20, 2024

NordicBuilder commented Dec 20, 2024

pdunaj commented Dec 20, 2024

nordicjm Dec 23, 2024

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

nordicjm Jan 8, 2025

tejlmand Jan 8, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

MarekPieta Jan 3, 2025

MarekPieta Jan 3, 2025

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

pdunaj Jan 8, 2025

MarekPieta Jan 3, 2025

MarekPieta Jan 3, 2025

pdunaj commented Jan 8, 2025

pdunaj commented Jan 8, 2025

pdunaj commented Jan 8, 2025

	IS_ENABLED(CONFIG_DESKTOP_HID_REPORT_BOOT_INTERFACE_MOUSE)) {
	IS_ENABLED(CONFIG_DESKTOP_HID_BOOT_INTERFACE_KEYBOARD)) {

applications: nrf_desktop: Support passthrough reports in hid state #19690

Are you sure you want to change the base?

applications: nrf_desktop: Support passthrough reports in hid state #19690

Conversation

pdunaj commented Dec 20, 2024

NordicBuilder commented Dec 20, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

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

pdunaj commented Dec 20, 2024

nordicjm commented Dec 20, 2024

NordicBuilder commented Dec 20, 2024

pdunaj commented Dec 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pdunaj commented Jan 8, 2025

pdunaj commented Jan 8, 2025

pdunaj commented Jan 8, 2025

NordicBuilder commented Dec 20, 2024 •

edited

Loading