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.

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

input: gpio_keys: implement polling mode support #67208

Merged

Conversation

fabiobaltieri
Copy link
Member

@fabiobaltieri fabiobaltieri commented Jan 4, 2024

Some MCU have limitations with GPIO interrupts. Add a polling mode to the gpio-keys driver to support those cases.

This required a bit of a refactoring of the driver data structure to add a instance wide data, and move the pin specific pointer in the config structure.

For polling, reuse the button 0 delayed work so we minimize the resource waste, the two work handler functions are only referenced when used so at least those are discarded automatically if no instance needs them.

Fix a bug in the PM structure instantiation as well.

Fixes #67049

@fabiobaltieri fabiobaltieri force-pushed the input-keys-no-interrupts branch 2 times, most recently from b894cc0 to 558c30c Compare January 4, 2024 15:40
@fabiobaltieri fabiobaltieri marked this pull request as ready for review January 4, 2024 16:55
@zephyrbot zephyrbot added area: Devicetree Binding PR modifies or adds a Device Tree binding area: Input Input Subsystem and Drivers labels Jan 4, 2024
@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Jan 4, 2024

@kartben can you give this a try? I would split the gpio in two by functionality, even if you just need to move one, something like

--- a/boards/arm/wio_terminal/wio_terminal.dts
+++ b/boards/arm/wio_terminal/wio_terminal.dts
@@ -48,7 +48,7 @@
        };
 
        /* Buttons */
-       gpio_keys {
+       buttons {
                compatible = "gpio-keys";
                user_button_0: button_0 {
                        label = "User Button 0";
@@ -65,6 +65,12 @@
                        gpios = <&portc 28 GPIO_ACTIVE_LOW>;
                        zephyr,code = <INPUT_KEY_2>;
                };
+       };
+
+       buttons-joystick {
+               compatible = "gpio-keys";
+               debounce-interval-ms = <100>;
+               no-interrupts;
                joy_sel: joystick_selection {
                        label = "joystick selection";
                        gpios = <&portd 10 GPIO_ACTIVE_LOW>;

I tested a mixed confing on a nrf52 and seems to work fine, feel free to append a patch to this PR for that board if you fancy it.

@fabiobaltieri
Copy link
Member Author

cc @maxhbr @joelguittet

@kartben
Copy link
Collaborator

kartben commented Jan 4, 2024

@kartben can you give this a try? I would split the gpio in two by functionality, even if you just need to move one, something like

would be great if you could try too, @joelguittet :)

@joelguittet
Copy link
Contributor

I have not tested yet but I wonder about the activity/behavior or the joystick using polling.
If creating a game for example we probably prefer the joystick to be very reactive.
Maybe it 's better to use polling only for buttons or maybe only for button 0 ?

@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Jan 4, 2024

If creating a game for example we probably prefer the joystick to be very reactive.

Then it's simple, you want to fix your hardware to be able to use interrupts. :-)

But let's say you are doing a game, at what rate you could run your game engine and display update on a Cortex-M4? Let's say you do a really good job and hit 30fps so 33ms cycles, you can poll the buttons at say ~20ms, that's probably plenty enough to not bounce (Linux polled driver debounces at 5ms by default, I used 10ms in some application and seems to work fine) have fresh inputs at every cycle and not completely trash the CPU -- or you could do the right thing for an application that has a natural update cycle, skip the driver and just read the GPIO pins directly as part of the game state update cycle. :-)

faxe1008
faxe1008 previously approved these changes Jan 5, 2024
Copy link
Collaborator

@faxe1008 faxe1008 left a comment

Choose a reason for hiding this comment

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

Looks neat 👍. I'd also like the dt flag to be non negated. Imo polling-mode sounds perfect.

drivers/input/input_gpio_keys.c Outdated Show resolved Hide resolved
faxe1008
faxe1008 previously approved these changes Jan 9, 2024
@kartben
Copy link
Collaborator

kartben commented Jan 9, 2024

I have not tested yet but I wonder about the activity/behavior or the joystick using polling. If creating a game for example we probably prefer the joystick to be very reactive. Maybe it 's better to use polling only for buttons or maybe only for button 0 ?

+1 for having the 3 "normal" buttons use the new polling mode, and leave the 5 joystick buttons as regular interrupt-driven keys

@fabiobaltieri
Copy link
Member Author

+1 for having the 3 "normal" buttons use the new polling mode, and leave the 5 joystick buttons as regular interrupt-driven keys

If we get this merged then I'm happy with you folks following up with the board config however you think is appropriate.

Some MCU have limitations with GPIO interrupts. Add a polling mode to
the gpio-keys driver to support those cases.

This required a bit of a refactoring of the driver data structure to add
a instance wide data, and move the pin specific pointer in the config
structure.

For polling, reuse the button 0 delayed work so we minimize the resource
waste, the two work handler functions are only referenced when used so
at least those are discarded automatically if no instance needs them.

Fix a bug in the PM structure instantiation as well.

Signed-off-by: Fabio Baltieri <fabiobaltieri@google.com>
@fabiobaltieri
Copy link
Member Author

Renamed the dt property (and internal structure field) to polling-mode.

@fabiobaltieri fabiobaltieri merged commit e5974b2 into zephyrproject-rtos:main Jan 10, 2024
18 checks passed
@fabiobaltieri fabiobaltieri deleted the input-keys-no-interrupts branch January 10, 2024 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: Input Input Subsystem and Drivers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Only the first five gpio-keys of wio-terminal seem to work
6 participants