Conversation
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.
This also adds shift key support!
43dfed6
to
725ef40
Compare
Added shift key support in 725ef40! |
There was a problem hiding this 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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic! should Just Work
There was a problem hiding this comment.
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. 😂
@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? |
@steveklabnik: Finished off the last two tasks (leaving the stretch goal for a follow-up PR). Ready for a real review now! |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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()), |
There was a problem hiding this comment.
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.
This also has merge conflicts now. |
|
||
#[derive(Clone, Copy, Debug, PartialEq)] | ||
pub enum Key { | ||
KA, |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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. 😕
Haven't forgotten about this! Will get to it in a bit when I have time. 😄 |
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:
|
@steveklabnik: Addressed your formatting and naming comments! 😄 |
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 :( |
@steveklabnik: I got a lot out of it: was a fun learning exercise! Please don't beat yourself up, life and work happens. 💖 |
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.