samples: hid-mouse: rework sample to use input subsys #61104

jfischer-no · 2023-08-03T09:37:38Z

Overall, this makes the sample much simpler and smaller.

The problem is different naming for compatible gpio-keys parent nodes and that we have to describe the same number of child nodes.

jfischer-no · 2023-08-03T09:55:33Z

Btw, we need fewer HID samples.
There is another similar HID mouse sample fxos8700-hid which can easily be changed to be generic and use accel0

diff --git a/samples/sensor/fxos8700-hid/src/main.c b/samples/sensor/fxos8700-hid/src/main.c
index 14845ac3b33..fbf580560b4 100644
--- a/samples/sensor/fxos8700-hid/src/main.c
+++ b/samples/sensor/fxos8700-hid/src/main.c
@@ -170,7 +170,8 @@ int main(void)
 {
        int ret;
        uint8_t report[4] = { 0x00 };
-       const struct device *accel_dev, *hid_dev;
+       const struct device *hid_dev;
+       const struct device *const accel_dev = DEVICE_DT_GET(DT_ALIAS(accel0));
 
        if (!device_is_ready(led_gpio.port)) {
                LOG_ERR("%s: device not ready.", led_gpio.port->name);
@@ -197,7 +198,6 @@ int main(void)
        }
 #endif
 
-       accel_dev = DEVICE_DT_GET_ONE(nxp_fxos8700);
        if (!device_is_ready(accel_dev)) {
                LOG_ERR("%s: device not ready.", accel_dev->name);
                return 0;

But it might be better to add a new relative X/Y input driver (FYI @fabiobaltieri) based on the accelerometer code in fxos8700-hid and use that new input driver in this example, and finally remove fxos8700-hid.

fabiobaltieri

Hey cool, I think it makes sense to use this for samples other than the buttons one rather than copying the gpio callback stuff all over, plus you get debouncing.

Actually one step further, how about using the LED APIs for the LEDs?

About the overlay, I was thinking about proposing to get rid of the two templates and have the input driver using the plain gpio-keys one directly. The driver would only use it if CONFIG_INPUT is enabled anyway, which is the same as gpio-leds, then in cases like these you could just grab the keycodes from the child nodes and use them for the sample. What do you think?

samples/subsys/usb/hid-mouse/src/main.c

jfischer-no · 2023-08-03T10:47:54Z

About the overlay, I was thinking about proposing to get rid of the two templates and have the input driver using the plain gpio-keys one directly. The driver would only use it if CONFIG_INPUT is enabled anyway, which is the same as gpio-leds, then in cases like these you could just grab the keycodes from the child nodes and use them for the sample. What do you think?

The only thing that spoiled my fun were the two extra inflexible overlays. With your suggestion, we would have to add a zephyr,code = <INPUT_BTN_FOOBAR>; to each board button, right? I think it is worth it.

tmon-nordic

Confirmed to work fine on nRF52840DK.

I used west build -d build-usb-hid-mouse -b nrf52840dk_nrf52840 samples/subsys/usb/hid-mouse/ -DDTC_OVERLAY_FILE=four_buttons.overlay to build. The only gotcha was that I initially had SW7 switch in Alt position (instead of Def) which disconnects BUTTON 1 and 2 (as marked on DK). Switching SW7 switch to Def makes all four button work as intended.

fabiobaltieri · 2023-08-03T11:11:33Z

The only thing that spoiled my fun were the two extra inflexible overlays. With your suggestion, we would have to add a zephyr,code = <INPUT_BTN_FOOBAR>; to each board button, right? I think it is worth it.

Yes #61115

fabiobaltieri · 2023-08-07T10:52:33Z

The only thing that spoiled my fun were the two extra inflexible overlays. With your suggestion, we would have to add a zephyr,code = <INPUT_BTN_FOOBAR>; to each board button, right? I think it is worth it.

Yes #61115

Sorted. Now the problem is that you can't rely on a specific code for boards. For the purpose of the sample, if you just want to grab "the first button" I was thinking, how about enumerating the specific board codes with something like:

#define GPIO_KEYS_NODE DT_COMPAT_GET_ANY_STATUS_OKAY(gpio_keys)

#define DT_GPIO_KEYS_CODE(node_id) DT_PROP(node_id, zephyr_code)
static const int codes[] = {DT_FOREACH_CHILD_SEP(GPIO_KEYS_NODE, DT_GPIO_KEYS_CODE, (,))};

#define BUTTON_COUNT ARRAY_SIZE(codes)

Separately from this, I was thinking it'd be cool if the HID stuff was a general library with macros and stuff to help defining HID descriptors and flipping bits in HID reports, and then it'd be cool to have a single HID sample that implements, for example, a mouse-keyboard-gamepad combination on both USB and Bluetooth.

jfischer-no · 2023-08-07T12:30:49Z

The only thing that spoiled my fun were the two extra inflexible overlays. With your suggestion, we would have to add a zephyr,code = <INPUT_BTN_FOOBAR>; to each board button, right? I think it is worth it.

Yes #61115

Sorted. Now the problem is that you can't rely on a specific code for boards. For the purpose of the sample, if you just want to grab "the first button" I was thinking, how about enumerating the specific board codes with something like:
#define GPIO_KEYS_NODE DT_COMPAT_GET_ANY_STATUS_OKAY(gpio_keys)

#define DT_GPIO_KEYS_CODE(node_id) DT_PROP(node_id, zephyr_code)
static const int codes[] = {DT_FOREACH_CHILD_SEP(GPIO_KEYS_NODE, DT_GPIO_KEYS_CODE, (,))};

#define BUTTON_COUNT ARRAY_SIZE(codes)

I have updated sample key mapping in the README. We could also just map the first 1...4 keys to L,R,X,Y, not sure it is worth it.

Separately from this, I was thinking it'd be cool if the HID stuff was a general library with macros and stuff to help defining HID descriptors and flipping bits in HID reports,

We already have include/zephyr/usb/class/hid.h. Not sure "flipping bits" could be generic, perhaps for mouse/keyboards.

and then it'd be cool to have a single HID sample that implements, for example, a mouse-keyboard-gamepad combination on both USB and Bluetooth.

I am currently working on reducing the number of hidden samples and lines of code there :-) (Working on HID implementation for the new device support). What you are suggesting is more of an application example.

samples/subsys/usb/hid-mouse/src/main.c

fabiobaltieri · 2023-08-07T14:16:35Z

samples/subsys/usb/hid-mouse/src/main.c

+	if (report[idx] != value) {
+		report[idx] = value;


How about memcpy'ing a local copy of the report at the function start and memcmp compare it at the end? Would save the idx and value variables, you could just WRITE_BIT or set the right report byte, would make the various case more compact.

It increased flash consumption by 64 bytes (nRF52840).

Just 996472 to go before running out flash! 😱

samples/subsys/usb/hid-mouse/src/main.c

fabiobaltieri · 2023-08-07T14:19:33Z

We already have include/zephyr/usb/class/hid.h. Not sure "flipping bits" could be generic, perhaps for mouse/keyboards.

Yeah I was thinking about a function to map events to report bits and one to handle the 6 bytes keyboard report. I have some code already, will try to fit it to a sample and send it out in the future.

fabiobaltieri

Cool! I would also rip off the custom LED part and use LED_GPIO instead, would drop few more lines and it would be right way of doing it for a proper Zephyr application.

fabiobaltieri · 2023-08-08T12:39:56Z

samples/subsys/usb/hid-mouse/src/main.c

-		LOG_ERR("Failed to get the state of port %s pin %u, error: %d",
-			gpio->name, sw3.pin, ret);
+		break;
+	default:
 		return;


Should there be a LOG_INF for "unrecognized code" here? May help out if someone is trying this on a custom board with different codes.

jfischer-no · 2023-08-08T12:54:24Z

Cool! I would also rip off the custom LED part and use LED_GPIO instead, would drop few more lines and it would be right way of doing it for a proper Zephyr application.

I looked at it, nobody uses it just to toggle the LED, more overhead compared to gpio.

fabiobaltieri · 2023-08-10T11:34:04Z

samples/subsys/usb/hid-mouse/src/main.c

-
-	return 0;
-}
+INPUT_LISTENER_CB_DEFINE(NULL, input_cb);


This needs to be updated (sorry...) ;-)

tmon-nordic · 2023-08-14T08:18:15Z

Yeah I was thinking about a function to map events to report bits and one to handle the 6 bytes keyboard report. I have some code already, will try to fit it to a sample and send it out in the future.

What you describe is pretty much impossible because the keycodes are reported as values (unless I am missing something or misunderstanding what you wrote). Only the modifiers are reported as bits, e.g. CTRL or ALT. In the report you can have multiple keycodes, which determines how many keys can be simultaneously pressed (6 slots in the Boot Keyboard interface). Note that the actual values reported in scan code do not necessarily have to be equal to the scancodes because the logical min/max and usage min/max can be different (these are fixed in the Boot Keyboard interface, but can be arbitrarily chosen in "proper" keyboard).

fabiobaltieri

(blocking on the input macro definition change)

Overall, this makes the sample code much simpler and smaller. Whit the changes introduced in commit commit 57e0da4 ("boards: add zephyr,code properties to the various gpio-keys nodes")' this example will work out of the box with any board that has at least zephyr,code = <INPUT_KEY_0>; in its devicetree. Signed-off-by: Johann Fischer <johann.fischer@nordicsemi.no>

jfischer-no requested review from fabiobaltieri, carlescufi and tmon-nordic August 3, 2023 09:37

jfischer-no requested a review from nashif as a code owner August 3, 2023 09:37

zephyrbot added the area: USB Universal Serial Bus label Aug 3, 2023

zephyrbot assigned jfischer-no Aug 3, 2023

fabiobaltieri reviewed Aug 3, 2023

View reviewed changes

samples/subsys/usb/hid-mouse/src/main.c Outdated Show resolved Hide resolved

jfischer-no marked this pull request as draft August 3, 2023 10:48

tmon-nordic previously approved these changes Aug 3, 2023

View reviewed changes

tmon-nordic mentioned this pull request Aug 3, 2023

input: unify gpio-keys and zephyr,gpio-keys #61115

Merged

jfischer-no dismissed tmon-nordic’s stale review via 001913b August 7, 2023 12:05

jfischer-no force-pushed the pr-usb-hidmouse-input branch from 5933c59 to 001913b Compare August 7, 2023 12:05

jfischer-no marked this pull request as ready for review August 7, 2023 12:08

fabiobaltieri reviewed Aug 7, 2023

View reviewed changes

jfischer-no force-pushed the pr-usb-hidmouse-input branch from 001913b to aa69e03 Compare August 8, 2023 10:55

fabiobaltieri previously approved these changes Aug 8, 2023

View reviewed changes

jfischer-no dismissed fabiobaltieri’s stale review via 4faef6d August 8, 2023 13:05

jfischer-no force-pushed the pr-usb-hidmouse-input branch from aa69e03 to 4faef6d Compare August 8, 2023 13:05

fabiobaltieri approved these changes Aug 8, 2023

View reviewed changes

fabiobaltieri reviewed Aug 10, 2023

View reviewed changes

tmon-nordic previously approved these changes Aug 14, 2023

View reviewed changes

fabiobaltieri requested changes Aug 14, 2023

View reviewed changes

jfischer-no dismissed tmon-nordic’s stale review via 22648fc August 16, 2023 22:14

jfischer-no force-pushed the pr-usb-hidmouse-input branch from 4faef6d to 22648fc Compare August 16, 2023 22:14

fabiobaltieri approved these changes Aug 16, 2023

View reviewed changes

tmon-nordic approved these changes Aug 17, 2023

View reviewed changes

fabiobaltieri merged commit 42ddc61 into zephyrproject-rtos:main Aug 17, 2023
15 checks passed

jfischer-no deleted the pr-usb-hidmouse-input branch August 30, 2023 20:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

samples: hid-mouse: rework sample to use input subsys #61104

samples: hid-mouse: rework sample to use input subsys #61104

jfischer-no commented Aug 3, 2023 •

edited

jfischer-no commented Aug 3, 2023 •

edited

fabiobaltieri left a comment

jfischer-no commented Aug 3, 2023

tmon-nordic left a comment

fabiobaltieri commented Aug 3, 2023

fabiobaltieri commented Aug 7, 2023

jfischer-no commented Aug 7, 2023

fabiobaltieri Aug 7, 2023

jfischer-no Aug 8, 2023 •

edited

fabiobaltieri Aug 8, 2023

fabiobaltieri commented Aug 7, 2023

fabiobaltieri left a comment

fabiobaltieri Aug 8, 2023

jfischer-no commented Aug 8, 2023

fabiobaltieri Aug 10, 2023

tmon-nordic commented Aug 14, 2023 •

edited

fabiobaltieri left a comment

samples: hid-mouse: rework sample to use input subsys #61104

samples: hid-mouse: rework sample to use input subsys #61104

Conversation

jfischer-no commented Aug 3, 2023 • edited

jfischer-no commented Aug 3, 2023 • edited

fabiobaltieri left a comment

Choose a reason for hiding this comment

jfischer-no commented Aug 3, 2023

tmon-nordic left a comment

Choose a reason for hiding this comment

fabiobaltieri commented Aug 3, 2023

fabiobaltieri commented Aug 7, 2023

jfischer-no commented Aug 7, 2023

fabiobaltieri Aug 7, 2023

Choose a reason for hiding this comment

jfischer-no Aug 8, 2023 • edited

Choose a reason for hiding this comment

fabiobaltieri Aug 8, 2023

Choose a reason for hiding this comment

fabiobaltieri commented Aug 7, 2023

fabiobaltieri left a comment

Choose a reason for hiding this comment

fabiobaltieri Aug 8, 2023

Choose a reason for hiding this comment

jfischer-no commented Aug 8, 2023

fabiobaltieri Aug 10, 2023

Choose a reason for hiding this comment

tmon-nordic commented Aug 14, 2023 • edited

fabiobaltieri left a comment

Choose a reason for hiding this comment

jfischer-no commented Aug 3, 2023 •

edited

jfischer-no commented Aug 3, 2023 •

edited

jfischer-no Aug 8, 2023 •

edited

tmon-nordic commented Aug 14, 2023 •

edited