Skip to content

Input subsystem core implementation #54908

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

Jump to bottom
Merged
merged 6 commits into from
Mar 6, 2023
Merged

Input subsystem core implementation #54908

merged 6 commits into from
Mar 6, 2023

Conversation

@fabiobaltieri
Copy link
Member

@fabiobaltieri fabiobaltieri commented Feb 16, 2023

Hi, this is the first PR for the input subsystem, adding the core functionalities, sample, testing and documentation. This has been proposed in #54622 and is built on the proof of concept code presented there and in the architecture WG.

Tentatively planning to follow-up with the kscan adapter implementation, porting over the zephyr,gpio-keys driver and then the three kscan keyboard drivers (npcx, mchp and ite, there's some code dedup opportunity there).

@fabiobaltieri fabiobaltieri force-pushed the input-core branch 6 times, most recently from 728dd75 to c3462d6 Compare February 16, 2023 18:37
@fabiobaltieri fabiobaltieri marked this pull request as ready for review February 16, 2023 20:18
@albertofloyd albertofloyd mentioned this pull request Feb 16, 2023
Closed
@fabiobaltieri fabiobaltieri mentioned this pull request Feb 22, 2023
Closed
gmarull
gmarull reviewed Feb 23, 2023
View reviewed changes
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

overall lgtm, nice addition to Zephyr! Please check my comments

rodrigopex
rodrigopex reviewed Feb 23, 2023
View reviewed changes
Copy link
Contributor

@rodrigopex rodrigopex left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Zbus can handle all the event work done here without significant overhead. I think you should use zbus for this part. I will propose an alternative using zbus. If we have a Zephyr message bus in the RTOS, using another implementation of the same thing would be strange to an official input driver. Don't you fellows think? @fabiobaltieri @gmarull

@gmarull
Copy link
Member

gmarull commented Feb 23, 2023

Zbus can handle all the event work done here without significant overhead. I think you should use zbus for this part. I will propose an alternative using zbus. If we have a Zephyr message bus in the RTOS, using another implementation of the same thing would be strange to an official input driver. Don't you fellows think? @fabiobaltieri @gmarull

I haven't checked in detail, but from what I heard in the Arch WG, ZBus could be added as another option, so you'd have callbacks, thread+queue and zbus. I guess it's the most flexible solution.

gmarull
gmarull previously approved these changes Mar 6, 2023
View reviewed changes
nordicjm
nordicjm requested changes Mar 6, 2023
View reviewed changes
Copy link
Contributor

@nordicjm nordicjm left a comment

Choose a reason for hiding this comment

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

Also needs an entry in the API table

@fabiobaltieri
input: add input subsystem
18d1dce 
Initial commit introducing the input subsystem into Zephyr.

Includes the input_event data structure, the input_report_* APIs, an
iterables sections based subscription API and two operation modes:
synchronous, where the listeners are called directly, and asynchronous,
where the listeners are called in a dedicated thread.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Mar 6, 2023

Also needs an entry in the API table

Yeah good point.

  • few more inline doc fixes
  • added an entry in docs/develop/api/overview

nordicjm
nordicjm previously approved these changes Mar 6, 2023
View reviewed changes
gmarull
gmarull previously approved these changes Mar 6, 2023
View reviewed changes
Copy link
Member

@gmarull gmarull left a comment

Choose a reason for hiding this comment

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

re-approval bot

rodrigopex
rodrigopex previously approved these changes Mar 6, 2023
View reviewed changes
mbolivar-nordic
mbolivar-nordic suggested changes Mar 6, 2023
View reviewed changes
mbolivar-nordic
mbolivar-nordic reviewed Mar 6, 2023
View reviewed changes
Copy link
Contributor

@mbolivar-nordic mbolivar-nordic left a comment

Choose a reason for hiding this comment

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

Couple other suggestions for the docs. At least I got confused about whether input_report() is a function or a family of functions from just reading the documentation. Since the answer is "both", I think this could be made clearer

@fabiobaltieri
doc: input: add the initial input subsystem documentation
2c89ade 
Add the initial documentation for the input subsystem.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri
samples: add input_dump sample
2b5bbcd 
Add a first input sample in the subsys/input directory. This just prints
any input event on the console.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri
tests: input: add an initial input testsuite
2cb2c3f 
Add some initial tests for the input subsystem, covering all the APIs in
both synchronous and thread mode.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri
doc: api: overview: add an entry for input
67fa88a 
Add an entry for the input subsystem, experimental, 3.4.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri
MAINTAINERS: add an Input section
89125e3 
Add a maintainer section for Input.

Signed-off-by: Fabio Baltieri <[email protected]>
@fabiobaltieri
Copy link
Member Author

fabiobaltieri commented Mar 6, 2023

Couple other suggestions for the docs. At least I got confused about whether input_report() is a function or a family of functions from just reading the documentation. Since the answer is "both", I think this could be made clearer

Cool, all applied.

@mbolivar-nordic mbolivar-nordic merged commit 3659017 into zephyrproject-rtos:main Mar 6, 2023
@fabiobaltieri fabiobaltieri deleted the input-core branch March 6, 2023 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@henrikbrixandersen henrikbrixandersen henrikbrixandersen approved these changes

@rodrigopex rodrigopex rodrigopex left review comments

@gmarull gmarull gmarull left review comments

@nordicjm nordicjm nordicjm left review comments

@nashif nashif Awaiting requested review from nashif

@carlescufi carlescufi Awaiting requested review from carlescufi

@galak galak Awaiting requested review from galak

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@tbursztyka tbursztyka Awaiting requested review from tbursztyka

@albertofloyd albertofloyd Awaiting requested review from albertofloyd

@dcpleung dcpleung Awaiting requested review from dcpleung

@keith-zephyr keith-zephyr Awaiting requested review from keith-zephyr

@stephanosio stephanosio Awaiting requested review from stephanosio

@cfriedt cfriedt Awaiting requested review from cfriedt

+1 more reviewer

@mbolivar-nordic mbolivar-nordic mbolivar-nordic approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area: Documentation area: Process

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@fabiobaltieri @gmarull @rodrigopex @carlescufi @cfriedt @henrikbrixandersen @nordicjm @mbolivar-nordic @zephyrbot