Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions applications/nrf_desktop/configuration/common/hid_report_desc.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,32 @@ enum report_id {

/** @brief Input reports map. */
static const uint8_t input_reports[] = {
#if CONFIG_DESKTOP_HID_REPORT_MOUSE_SUPPORT || CONFIG_DESKTOP_HID_BOOT_INTERFACE_MOUSE
REPORT_ID_MOUSE,
#endif
#if CONFIG_DESKTOP_HID_REPORT_KEYBOARD_SUPPORT || CONFIG_DESKTOP_HID_BOOT_INTERFACE_KEYBOARD
REPORT_ID_KEYBOARD_KEYS,
#endif
#if CONFIG_DESKTOP_HID_REPORT_SYSTEM_CTRL_SUPPORT
REPORT_ID_SYSTEM_CTRL,
#endif
#if CONFIG_DESKTOP_HID_REPORT_CONSUMER_CTRL_SUPPORT
REPORT_ID_CONSUMER_CTRL,
#endif
/* Keep boot reports at the end as these don't have own data. */
#if CONFIG_DESKTOP_HID_BOOT_INTERFACE_MOUSE
REPORT_ID_BOOT_MOUSE,
#endif
#if CONFIG_DESKTOP_HID_BOOT_INTERFACE_KEYBOARD
REPORT_ID_BOOT_KEYBOARD,
#endif
};

/** @brief Output reports map. */
static const uint8_t output_reports[] = {
#if CONFIG_DESKTOP_HID_REPORT_KEYBOARD_SUPPORT
REPORT_ID_KEYBOARD_LEDS,
#endif
};

/* Internal definitions used to calculate size of the biggest supported HID input report. */
Expand Down
10 changes: 10 additions & 0 deletions applications/nrf_desktop/src/modules/Kconfig.hid_state
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,16 @@ config DESKTOP_HID_EVENT_QUEUE_SIZE
help
Size of the HID event queue.

config DESKTOP_HID_STATE_RAW_REPORTS
bool
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing a prompt?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could add the prompt

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

help
This option should be selected if the application code issue reports
prepared outside of the HID state module.
The HID state module will work as proxy collecting and holding such
reports.
The HID state will not alter the data but will make sure the reports
are sent to the transport in an organized manner.

module = DESKTOP_HID_STATE
module-str = HID state
source "subsys/logging/Kconfig.template.log_config"
Expand Down
Loading