PS/2 Overhaul

[hecatia-elegua]

No more magic values, less errors, more docs, simpler functions, structs for all the response-bits, so no bit ops.
In general, saner code.

Some notes/questions:

• error handling is still not very good, this seems to be because the Error trait is not no_std and some other problems, so for now we have strings, which I would still like to improve a bit I have improved a bit
• why is there a pattern of .map_err(|_| { warn!("..."); "disable mouse streaming failed"})? it might make sense because it shows a kind of backtrace-like route up the error stack, but still seems/feels bad? I have read the recommended reading, but maybe I should look into some error-handling/logging crates (though I dislike the topic) -> some map_err calls were obsolete, but I'm still not comfortable with some of the error handling, again because we can't impl Error and I wonder in what cases we should just shutdown the system or reset the device/s
• how should I improve my commit messages?
• I chose modular-bitfield because it comes closest to what I would want, this, or this -> feedback: I think this is more approachable than bitflags and some others
• not sure if I did KBD_MODIFIERS right -> there is a note about "remove unsafe static mut" on it, not sure what to do about it (access is unsafe but there also is a note on access like "it should never race"
• there's a lot of code in the ps2 crate now, any thoughts on where/how to split it up? some functions could also be enum/struct methods instead -> I wasted a lot of time scrolling up and down, still not sure how to divide it up (mod mouse, keyboard, controller?)

Feel free to critique!

[kevinaboos]

Thanks for this awesome PR @hecatia-elegua! Much appreciated.

I'm a bit pressed for time this week, but I did a quick review and things generally look quite good! I will do a more detailed review next week, but in the meantime feel free to finish up the various to-do items and clean up tasks that you had noted, since I definitely plan to merge this PR in. I will also answer your questions above when time permits.

[hecatia-elegua]

This is done.
(or rather, I seriously need to stop 😄)

[kevinaboos]

This is done. (or rather, I seriously need to stop smile)

haha, thanks so much, it's really an awesome contribution. I've been a bit backlogged but am slowly catching up; I will definitely do a full review this week. Much appreciated, and sorry for the delays!

[kevinaboos]

Ok phew, I got through all the changes! In general everything looks great! I did leave a lot of comments, some of which are a bit nitpicky, and others which can be applied to many other parts of the code.

Thanks again, and kindly do let me know if there's anything you disagree with; I'm more than open to discussion.

[hecatia-elegua]

Some questions about make view-docs:
can I somehow only build docs for one crate?
why does it create Cargo.lock files for all libs/?
would we want the option to also include all the docs on private items?
thanks

[hecatia-elegua]

wait, why did this break only now?
I mean, I built often, without error. How strange

[kevinaboos]

Some questions about make view-docs: can I somehow only build docs for one crate?

Sure, you can call rustdoc on a specific crate, like I do here. But there's no real reason to do that (unless I'm missing something?)

why does it create Cargo.lock files for all libs/?

The crates in libs/ are not part of the main Cargo workspace, so they don't get documented because we don't want to document all third-party dependencies.
Instead, we manually call rustdoc on the libs/ crates that we do want to include in the docs, which currently includes only the first-party crates in libs/.
The only reason you see those .lock files is because I haven't added them to the .gitignore in each libs crate. You can safely delete/ignore them though.

would we want the option to also include all the docs on private items? thanks

On some things, yes. That's mostly so intra-doc links on private items are checked and processed correctly, but technically it's not required.

[kevinaboos]

@hecatia-elegua thanks for addressing my comments! Everything looks pretty good, but it's a bit hard to tell if I missed any significant changes due to the fact that the branch has been force-pushed, which causes github to lose track of what I had previously reviewed.

Anyway, are there any other remaining to-do items that you intend to work on, or do you consider this PR ready to be tested and merged in?

[hecatia-elegua]

would we want the option to also include all the docs on private items?

What I meant by this: would we want another build-target with --document-private-items specified, so e.g. newcomers could read even the private function docs in the browser? Sorry for going off-topic.

which causes github to lose track of what I had previously reviewed

Does it? It says "outdated" on most of your review comments, but on the lines I didn't change it doesn't.

Changes started from this commit, so you could go through them from there, but
if you want to save time, the only "new new" part is enum MousePacket and its sub-structs, like:

Theseus/kernel/ps2/src/lib.rs

Line 566 in 6693e35

pub struct MousePacketGeneric {

The PR already had CI runs btw. and can be merged.

[kevinaboos]

would we want the option to also include all the docs on private items?

What I meant by this: would we want another build-target with --document-private-items specified, so e.g. newcomers could read even the private function docs? Sorry for going off-topic.

Oh, I see. In that case, yes, potentially. Feel free to open up a new PR with that change; if it's good and only improves the docs then we can enable it unconditionally.

which causes github to lose track of what I had previously reviewed

Does it? It says "outdated" on most of your review comments, but on the lines I didn't change it doesn't.

Yeah it breaks the nice github feature that shows "changes since your last review". But no big deal, since you told me the relevant starting commit too. Thanks!

The PR already had CI runs btw. and can be merged.

Great, but those CI passes don't actually perform any runtime tests (yet... one day we'll get around to it). I test it myself, so I'll do that and merge things in if it goes well. I assume you've tested it thoroughly on QEMU at least.

[kevinaboos]

I tested everything and it's working well, yay! I made a few small changes and merged in the latest changes from theseus_main to ensure the branch still builds on our more recent nightly Rust version.

There's only one very minor issue - when pressing the Caps lock key, I see this error on the log:

[E] kernel/keyboard/src/lib.rs:192: handle_keyboard_input(): Unknown scancode: 250, adjusted scancode: 122
[E] kernel/keyboard/src/lib.rs:90: ps2_keyboard_handler: error handling PS2_PORT input: "unknown keyboard scancode"

If you have a moment to address that error, that'd be great. If not, let me know and I'm happy to look into it. Aside from that, I'd say this PR is ready to merge!

[hecatia-elegua]

Thank you.
I had problems with the caps lock key too.
in set 1, it would be 58 (which is what it should be)
in set 2, it would be 88
250 is FA, so that's an ACK "command acknowledged" response.

If you have an idea what it could be, you can fix or tell me, otherwise I'll look into it later

[hecatia-elegua]

In our code, it looks like a caps lock press first triggers an interrupt with scancode 58 (make caps), then immediately another one with 250 (ack), a release only triggers an interrupt with 186 (break caps).

I'm guessing the set_keyboard_led command has something to do with it, although the ack is already getting handled there...
Also not sure why the keyboard command ack would trigger an interrupt, because afaik these should only happen when output_buffer_full, which should not be full after polling_receive in set_keyboard_led.
Maybe we should wait a bit inside the handler after setting the LED? Otherwise we could also just ignore values of 250.

update: waiting works, but I just printed some stuff in a loop to wait, so instead, what's the right way to do it?

[kevinaboos]

I'm guessing the set_keyboard_led command has something to do with it, although the ack is already getting handled there... Also not sure why the keyboard command ack would trigger an interrupt, because afaik these should only happen when output_buffer_full, which should not be full after polling_receive in set_keyboard_led. Maybe we should wait a bit inside the handler after setting the LED? Otherwise we could also just ignore values of 250.

I'm not sure if the keyboard works this way, but a lot of legacy devices on x86 (e.g., like this in the serial port) require you to spin in a loop while polling a status register in order to determine when the device is ready to receive another command. Perhaps you need to do that here too.

update: waiting works, but I just printed some stuff in a loop to wait, so instead, what's the right way to do it?

If there's not a status register (as I mentioned above) with a bit flag that indicates whether the keyboard is ready to receive a command, then we can insert a wait loop (but not by using print statements). This shouldn't be necessary, but a common way to do that in minimal legacy environments is this.

[kevinaboos]

Ok I took a deeper look, and fortunately this is probably a lot simpler than what we were discussing. You can ignore what i wrote above; i double-checked that your code paths are all properly checking/waiting for status registers.

Your guess about set_keyboard_led is correct. The interrupt to acknowledge your "set kbd LEDs" command going to fire immediately before you can poll for it. There's no real way for you to avoid that interrupt, so I think it's fine for you to ignore any ACK value (0xFA) when handling scancodes, for now.
The real issue is that you're mixing interrupt-based data receive and polling-based data receive. So the byte that you read in from port 0x60 is being treated as a keypress action from the user instead of an acknowledgment or error response from the keyboard. Generally you should only do one or the other, i.e., only polling or only interrupts, but it works here for the keyboard because the keyboard is a simple/dumb device.

[hecatia-elegua]

The real issue is that you're mixing interrupt-based data receive and polling-based data receive.

A lot of wording online sounded like you always need to use polling for commands, e.g. when initializing the devices.
Now reading here again, I guess this is what I got wrong: The last paragraph is only about commands to the PS/2 Controller, so kind of irrelevant for mouse and keyboard commands.

We also at least need polling-send, as the PS/2 Controller does not support interrupt-driven sending to devices.

I think we can merge now :)

[kevinaboos]

Looks good, thanks so much for this amazing contribution!

[kevinaboos]

Did you also intend to tackle #685 and #686 yourself, or were those just reminders for myself or a future contributor?

Also I wonder if this fixes #373, i should test that sometime.

Thanks again!

[hecatia-elegua]

I intend to come back to these, yes.