Skip to content
This repository has been archived by the owner on Apr 6, 2020. It is now read-only.

Stateful keyboard driver #90

Closed

Conversation

dirk
Copy link
Contributor

@dirk dirk commented Oct 2, 2016

Builds off of #89 to start constructing a stateful keyboard driver capable of modeling the user's interaction with the keyboard and translating that into key codes for the OS and programs.

  • Decode basic set of 1-byte PS/2 scan codes
  • Managed internal state of pressed keys, clear state when keys released
  • Return current printable character to OS (when possible) when inputting latest scan code
  • Report backspace and other non-printables to the OS (send ASCII code?)
  • Tests!
  • Decode 2-byte and 3-byte PS/2 scan codes (stretch goal); this will need more complex state management in the driver

The PS/2 port sends pressed and released codes corresponding to
the user pressing and releasing keys. To accurately turn those into
printable and non-printable key codes we need a state machine to
model the state of the keyboard. If we don't keep track of state
then we cannot know when/if modifier keys (eg. shift, alt, etc.)
should be applied to a given key-stroke.
@dirk dirk force-pushed the dirk-keyboard-driver-state-machine branch from 43dfed6 to 725ef40 Compare October 2, 2016 07:57
@dirk
Copy link
Contributor Author

dirk commented Oct 2, 2016

Added shift key support in 725ef40!

image

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

Just a small review for now, thanks for this! Will investigate more deeply soon.


// Guard against overflow
if self.size == MAX_KEYS {
// TODO: Panic!
Copy link
Member

Choose a reason for hiding this comment

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

panic! should Just Work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Implemented in 46afc2a. I just realized that if that Just Works then it will be the first kernel panic, literally, I've ever written. 😂

@dirk dirk changed the title Keyboard driver state machine (WIP) Stateful keyboard driver Oct 2, 2016
@dirk
Copy link
Contributor Author

dirk commented Oct 2, 2016

@steveklabnik: Been pondering about how to report backspace and similar commands. The correct solution, I think, would be to return the corresponding ASCII code. Does that seem alright?

@dirk
Copy link
Contributor Author

dirk commented Oct 2, 2016

@steveklabnik: Finished off the last two tasks (leaving the stretch goal for a follow-up PR). Ready for a real review now!

Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

This looks good overall! So awesome. I'd like to do better review again, later, before merging though.


#[derive(Clone, Copy, Debug, PartialEq)]
pub enum Key {
KA,
Copy link
Member

Choose a reason for hiding this comment

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

Why the K prefix on all of these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The Rust compiler wasn't happy with 0, 1, etc. as plain numbers. I updated it to use plain letters/words for most of the codes, and then these keyboard (not keypad) number codes are prefixed with Keyboard. Also included a comment before them to note the difference between the keyboard number keys and the keypad number keys.

use Key::*;

let character = match (self, shift) {
(KA, false) => 'a', (KA, true) => 'A',
Copy link
Member

Choose a reason for hiding this comment

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

Usual Rust style is to not align stuff like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was trying to make it a little easier to scan, but it's definitely non-standard. 😛 Reformatted in cc4a8fc to be more like the Rust standard.

}

/// Size of the buffer used for storing the state of the keyboard
const KEY_BUFFER: usize = 256;
Copy link
Member

Choose a reason for hiding this comment

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

In general, since we don't have a heap yet, this seems like the way to go. But I also wonder if there isn't something better. Hm.

}

#[cfg(test)]
mod tests {
Copy link
Member

Choose a reason for hiding this comment

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

🤘

@@ -23,6 +25,7 @@ impl Context {
Context {
vga: Mutex::new(Vga::new(slice)),
idt: IdtRef::new(),
keyboard: Mutex::new(Keyboard::new()),
Copy link
Member

Choose a reason for hiding this comment

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

Leaving a note for myself that I have some feels here; I'd like to handle this more like IdtRef. Will elaborate later.

@steveklabnik
Copy link
Member

This also has merge conflicts now.


#[derive(Clone, Copy, Debug, PartialEq)]
pub enum Key {
KA,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is mainly for the plain digits below (0, 1, etc.). I can remove the K prefix and just do Digit0, Digit1, etc. for those. How does that approach sound to you?

}

/// Size of the buffer used for storing the state of the keyboard
const KEY_BUFFER: usize = 256;
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 was originally planning on using EnumSet, but that required having an allocator. 😕

@dirk
Copy link
Contributor Author

dirk commented Oct 12, 2016

Haven't forgotten about this! Will get to it in a bit when I have time. 😄

@steveklabnik
Copy link
Member

It's all good, I've been slow on the review. intermezzOS is a long term thing, there's no rush :)

On Oct 11, 2016, 22:18 -0400, Dirk Gadsden notifications@github.com, wrote:

Haven't forgotten about this! Will get to it in a bit when I have time. 😄


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub (#90 (comment)), or mute the thread (https://github.com/notifications/unsubscribe-auth/AABsigwquUg_NzDvuJrel61NnJJXRP45ks5qzENbgaJpZM4KL8Jc).

@dirk
Copy link
Contributor Author

dirk commented Oct 13, 2016

@steveklabnik: Addressed your formatting and naming comments! 😄

@steveklabnik
Copy link
Member

guh. sorry that i am the worst and let this sit for over a year. I feel terrible. I'm trying to get back to a clean slate here, so sadly, i need to close this. I hope you got something out of this, sorry for failing you :(

@dirk
Copy link
Contributor Author

dirk commented Mar 6, 2018

@steveklabnik: I got a lot out of it: was a fun learning exercise! Please don't beat yourself up, life and work happens. 💖

@dirk dirk deleted the dirk-keyboard-driver-state-machine branch March 6, 2018 20:06
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants