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

feat(kernel): add generic KeyboardService abstraction #158

Merged
merged 16 commits into from
Jul 16, 2023
Merged

Conversation

hawkw
Copy link
Contributor

@hawkw hawkw commented Jul 14, 2023

Currently, we have an implementation of a driver for the Blackberry Q10
keyboard on the Beepy in the mnemos-beepy crate. However, there's no
way for cross-platform code in the kernel, such as the graphical forth
shell, to consume keyboard input, because the keyboard driver is
Beepy-specific. This branch fixes this by adding a generic
KeyboardService in kernel::services, which is not hardware-specific.

In addition, in order to support multiple keyboards, such as the SerMux
pseudo-keyboard, we also add a new KeyboardMuxServer, which allows
tasks which want to consume keyboard input to subscribe to key events
from all available keyboards. This type implements the
KeyboardService, for consumers of keyboard events, as well as a
KeyboardMuxService, which keyboard drivers can use to publish keyboard
events to any task subscribed to the mux. The graphical forth shell now
consumes the KeyboardService, allowing it to handle input from both
the SerMux pseudo-keyboard and the Beepy Q10 keyboard without being
aware of either implementation specifically.

This allows us to type on the Beepy keyboard and get text on the screen!
It's currently a bit laggy, which I think is at least partially due to the 50ms
poll interval in the i2c_puppet driver. I hope changing the driver to use a
GPIO interrupt instead will reduce this lag...

It does work, though!!!!
video evidence

@hawkw hawkw added this to the beepberry computer v0.1 milestone Jul 14, 2023
@hawkw hawkw requested a review from jamesmunns July 14, 2023 19:01
@hawkw hawkw self-assigned this Jul 14, 2023
Copy link
Contributor

@jamesmunns jamesmunns 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, there's some failing CI checks (looks mostly to be broken links/imports due to some reorg), and some nits in this review, but OK to merge from me :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Does this empty file need to go away? Or did something weird happen?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

whoops, i started on this in a different branch and i must have accidentally committed it, good catch!

/// returning a [`KeySubscription`].
pub async fn subscribe_to_keys(&mut self) -> Result<KeySubscription, Error> {
if let Response::SubscribeToKeys(sub) = self.request(Request::SubscribeToKeys).await? {
/// Subscribe to raw keyboard input from `i2c_puppet`'s Blackberry Q10
Copy link
Contributor

Choose a reason for hiding this comment

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

technically we use the Q20 keyboard. The OG solder.party hardware used the Q10, the beepy (and newer solder.party hardware) uses the q20. It doesn't really matter, as that hardware is all downstream of the i2c puppet chip we actually talk to though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, will update docs

@@ -426,7 +486,7 @@ impl I2cPuppetServer {
}
}

if !self.subscriptions.is_empty() {
if !self.subscriptions.is_empty() || self.keymux.is_some() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you know if the i2c puppet automatically overwrites oldest chars? doesn't really matter, but if not we might still want to drain it periodically, or on first connect to discard potentially stale data?

This is really nitpicky tho.

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 think we can configure that behavior with the OVERFLOW_ON config flag. we're not currently setting that, so we probably should enable it.

Pause,
/// Menu key.
Menu,
/// The "Begin" key (often mapped to the 5 key when Num Lock is turned on).
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this beepy specific?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, i believe this is a normal keyboard thing (i copied it from crossterm). idk if the beepy keyboard even has this behavior at all.

settings: KeyboardMuxSettings,
) -> Result<(), RegistrationError> {
let (key_tx, key_rx) = KChannel::new_async(settings.buffer_capacity).await.split();
let (sub_tx, sub_rx) = KChannel::new_async(8).await.split();
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be DEFAULT_MAX_KEYBOARDS?

source/kernel/src/services/serial_mux.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Contributor Author

hawkw commented Jul 15, 2023

Overall LGTM, there's some failing CI checks (looks mostly to be broken links/imports due to some reorg), and some nits in this review, but OK to merge from me :)

yup, looks like i still need to fix Pomelo before merging this...

@hawkw hawkw merged commit 7a3bc60 into main Jul 16, 2023
3 checks passed
@hawkw hawkw deleted the eliza/keydoarb-svc branch July 16, 2023 16:47
@jamesmunns jamesmunns added area: kernel Related to the cross-platform kernel platform: D1 Specific to the Allwinner D1 hardware platform labels Jul 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: kernel Related to the cross-platform kernel platform: D1 Specific to the Allwinner D1 hardware platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants