Comments and refactor

tetros/src/drivers/pic.rs

@@ -1,18 +1,25 @@
+//! Control routines for the x86
+//! 8259 Programmable Interrupt Controller
+//!
+//! This helps us configure interrupts that receive
+//! keyboard input and timer pulses.
+
 use crate::os::util::outb;
 
 /// IO base address for master PIC
 const PIC_A: u32 = 0x20;
+/// Command address for master PIC
 const PIC_A_COMMAND: u32 = PIC_A;
+/// Data address for master PIC
 const PIC_A_DATA: u32 = PIC_A + 1;
 
 /// IO base address for slave PIC
 const PIC_B: u32 = 0xA0;
+/// Command address for slave PIC
 const PIC_B_COMMAND: u32 = PIC_B;
+/// Data address for slave PIC
 const PIC_B_DATA: u32 = PIC_B + 1;
 
-/// PIC `EOI` command
-const CMD_EOI: u8 = 0x20;
-
 /// A driver for the PIC
 ///
 /// Reference:
@@ -24,6 +31,7 @@ pub struct PICDriver {
 }
 
 impl PICDriver {
+	/// Create a PIC driver with the given offsets
 	pub const fn new(offset_pic_a: u8, offset_pic_b: u8) -> Self {
 		Self {
 			offset_pic_a,
@@ -47,14 +55,20 @@ impl PICDriver {
 		unsafe { outb(PIC_B_DATA, cmd) }
 	}
 
-	pub fn send_eoi(&self, irq: u8) {
+	/// Send an EOI for the given IRQ.
-		if irq > 8 {
+	///
-			self.send_b_cmd(CMD_EOI);
+	/// This needs to be called at the end of each interrupt handler.
+	/// If `both` is true, reset both PICs. This is only necessary
+	/// when we handle interrupts from PIC_B.
+	pub fn send_eoi(&self, both: bool) {
+		if both {
+			self.send_b_cmd(0x20);
 		}
-
+		self.send_a_cmd(0x20);
-		self.send_a_cmd(CMD_EOI);
 	}
 
+	/// Initialize this PIC driver.
+	/// This should be called as early as possible.
 	pub fn init(&mut self) {
 		const ICW1_ICW4: u8 = 0x01; /* Indicates that ICW4 will be present */
 		const ICW1_INIT: u8 = 0x10; /* Initialization - required! */

tetros/src/drivers/serial.rs

@@ -1,3 +1,10 @@
+//! Serial port driver, for debug.
+//!
+//! This file provides the usual `print`
+//! and `println` macros (which are usually
+//! provided by `std`) that send messages out
+//! of the serial port.
+
 use lazy_static::lazy_static;
 use spin::Mutex;
 use uart_16550::SerialPort;

tetros/src/drivers/vga.rs

@@ -3,7 +3,7 @@ use rand::seq::IndexedRandom;
 
 use crate::RNG;
 
-#[repr(u8)]
+#[allow(missing_docs)]
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum VgaColor {
 	Black,
@@ -17,6 +17,7 @@ pub enum VgaColor {
 	Yellow,
 }
 
+#[allow(missing_docs)]
 impl VgaColor {
 	pub fn as_u8(self) -> u8 {
 		match self {
@@ -73,6 +74,9 @@ impl Vga13h {
 	pub const HEIGHT: usize = 200;
 	pub const ADDR: usize = 0xA0000;
 
+	/// Initialize a new VGA driver.
+	///
+	/// Only one of these should exist.
 	pub const unsafe fn new() -> Self {
 		Self {
 			fb_a: [0; Vga13h::WIDTH * Vga13h::HEIGHT],

tetros/src/game/board.rs

@@ -2,21 +2,26 @@ use crate::drivers::vga::{Vga13h, VgaColor};
 
 use super::FallingTetromino;
 
-#[repr(u8)]
+/// The state of a cell in the game board
 #[derive(Debug, Clone, Copy, PartialEq, Eq)]
 pub enum TetrisCell {
 	Empty,
 	Filled { color: VgaColor },
 }
 
+/// The tetris board
 pub struct TetrisBoard {
 	board: [TetrisCell; TetrisBoard::BOARD_WIDTH * TetrisBoard::BOARD_HEIGHT],
 }
 
 impl TetrisBoard {
+	/// The width of this board, in cells
 	const BOARD_WIDTH: usize = 10;
+
+	/// The height of this board, in cells
 	const BOARD_HEIGHT: usize = 20;
 
+	/// The side length of a (square) cell, in pixels
 	const CELL_SIZE: usize = 9;
 
 	pub const fn new() -> Self {
@@ -25,6 +30,8 @@ impl TetrisBoard {
 		}
 	}
 
+	/// Find and remove all filled rows,
+	/// shifting upper rows down.
 	pub fn collapse(&mut self) {
 		let mut y = Self::BOARD_HEIGHT - 1;
 		'outer: loop {
@@ -58,6 +65,11 @@ impl TetrisBoard {
 		}
 	}
 
+	/// Place the given tetromino on the board,
+	/// filling the cells it occupies.
+	///
+	/// If the tetromino cells that overlap
+	/// non-empty board cells are ignored.
 	pub fn place_tetromino(&mut self, tetromino: FallingTetromino) {
 		for (x, y) in tetromino.tiles() {
 			let cell = self.get_cell_mut(x, y);
@@ -82,32 +94,26 @@ impl TetrisBoard {
 		return true;
 	}
 
+	/// Get the value of the cell at the given position.
+	/// Returns [`None`] if (x, y) exceeds the board's bounds.
 	pub fn get_cell(&self, x: usize, y: usize) -> Option<&TetrisCell> {
-		if y >= TetrisBoard::BOARD_HEIGHT {
+		return self.board.get(y * TetrisBoard::BOARD_WIDTH + x);
-			return None;
-		}
-
-		if x >= TetrisBoard::BOARD_WIDTH {
-			return None;
-		}
-
-		return Some(&self.board[y * TetrisBoard::BOARD_WIDTH + x]);
 	}
 
+	/// Get a mutable reference to the cell at the given position.
+	/// Returns [`None`] if (x, y) exceeds the board's bounds.
 	pub fn get_cell_mut(&mut self, x: usize, y: usize) -> Option<&mut TetrisCell> {
-		if y >= TetrisBoard::BOARD_HEIGHT {
+		return self.board.get_mut(y * TetrisBoard::BOARD_WIDTH + x);
-			return None;
-		}
-
-		if x >= TetrisBoard::BOARD_WIDTH {
-			return None;
-		}
-
-		return Some(&mut self.board[y * TetrisBoard::BOARD_WIDTH + x]);
 	}
 }
 
+//
+// MARK: draw routines
+//
+
 impl TetrisBoard {
+	/// Draw a cell of the given color on `fb`.
+	/// (x, y) is the pixel position of the cell (NOT board coordinates).
 	fn draw_cell(&self, fb: &mut [u8], color: VgaColor, x: usize, y: usize) {
 		let color = color.as_u8();
 		for yo in 0..TetrisBoard::CELL_SIZE {
@@ -117,6 +123,7 @@ impl TetrisBoard {
 		}
 	}
 
+	/// Draw the tetris board's frame
 	fn draw_frame(&self, fb: &mut [u8], x: usize, y: usize) {
 		let color = VgaColor::Gray.as_u8();
 		for yo in 0..TetrisBoard::CELL_SIZE {
@@ -126,6 +133,7 @@ impl TetrisBoard {
 		}
 	}
 
+	/// Draw this tetris board using the given VGA driver.
 	pub fn draw(&self, vga: &mut Vga13h, falling: Option<&FallingTetromino>) {
 		let fb = vga.get_fb();
 

tetros/src/game/falling.rs

@@ -53,6 +53,7 @@ pub enum Direction {
 }
 
 impl Direction {
+	/// Rotate this direction clockwise
 	pub fn rot_cw(self) -> Self {
 		match self {
 			Self::North => Self::East,
@@ -61,17 +62,6 @@ impl Direction {
 			Self::West => Self::North,
 		}
 	}
-
-	/*
-	pub fn rot_ccw(self) -> Self {
-		match self {
-			Self::North => Self::West,
-			Self::West => Self::South,
-			Self::South => Self::East,
-			Self::East => Self::North,
-		}
-	}
-	*/
 }
 
 #[derive(Debug, Clone)]
@@ -85,6 +75,7 @@ pub struct FallingTetromino {
 }
 
 impl FallingTetromino {
+	/// Make a new falling tetromino
 	pub fn new(tetromino: Tetromino, color: VgaColor, center_x: usize, center_y: usize) -> Self {
 		Self {
 			tetromino,
@@ -95,6 +86,7 @@ impl FallingTetromino {
 		}
 	}
 
+	/// Generate a random tetromino at the given position
 	pub fn random(center_x: usize, center_y: usize) -> Self {
 		Self::new(
 			Tetromino::choose_rand(),
@@ -104,6 +96,7 @@ impl FallingTetromino {
 		)
 	}
 
+	// Move this tetromino
 	pub fn translate(&mut self, x: i16, y: i16) {
 		if x > 0 {
 			let x = usize::try_from(x).unwrap();
@@ -122,16 +115,11 @@ impl FallingTetromino {
 		}
 	}
 
+	/// Rotate this tetromino clockwise
 	pub fn rotate_cw(&mut self) {
 		self.direction = self.direction.rot_cw()
 	}
 
-	/*
-	pub fn rotate_ccw(&mut self) {
-		self.direction = self.direction.rot_ccw()
-	}
-	*/
-
 	/// Returns the positions of this falling tetromino's tiles.
 	pub fn tiles(&self) -> [(usize, usize); 4] {
 		match (&self.tetromino, self.direction) {

tetros/src/game/mod.rs

@@ -1,3 +1,6 @@
+//! This crate contains all tetris game logic.
+//! No low-level magic here.
+
 mod board;
 pub use board::*;
 

tetros/src/idt/stackframe.rs

@@ -1,16 +1,14 @@
 use core::{fmt, ops::Deref};
 
-use crate::os::EFlags;
-
 use super::VirtAddr;
+use crate::os::EFlags;
 
 /// Wrapper type for the interrupt stack frame pushed by the CPU.
 ///
 /// This type derefs to an [`InterruptStackFrameValue`], which allows reading the actual values.
 ///
-/// This wrapper type ensures that no accidental modification of the interrupt stack frame
+/// This wrapper ensures that the stack frame cannot be modified.
-/// occurs, which can cause undefined behavior (see the [`as_mut`](InterruptStackFrame::as_mut)
+/// This prevents undefined behavior.
-/// method for more information).
 #[repr(transparent)]
 pub struct InterruptStackFrame(InterruptStackFrameValue);
 

tetros/src/idt/table.rs

@@ -5,17 +5,6 @@ use super::{
 	HandlerFuncWithErrCode, PageFaultHandlerFunc,
 };
 
-// TODO: comments
-#[repr(C, packed(2))]
-struct Idtr {
-	size: u16,
-	offset: u32,
-}
-
-//
-// MARK: idt
-//
-
 // spell:off
 #[derive(Clone, Debug)]
 #[repr(C)]
@@ -467,12 +456,17 @@ impl InterruptDescriptorTable {
 	/// # Safety
 	///
 	/// As long as it is the active IDT, you must ensure that:
-	///
 	/// - `self` is never destroyed.
 	/// - `self` always stays at the same memory location.
-	///   It is recommended to wrap it in a `Box`.
 	#[inline]
 	pub unsafe fn load_unsafe(&self) {
+		/// The data we push to the IDTR register
+		#[repr(C, packed(2))]
+		struct Idtr {
+			size: u16,
+			offset: u32,
+		}
+
 		let idtr = {
 			Idtr {
 				size: (size_of::<InterruptDescriptorTable>() - 1) as u16,

tetros/src/idt/virtaddr.rs

@@ -1,7 +1,4 @@
-use core::{
+use core::fmt::{self};
-	fmt::{self},
-	ops::{Add, AddAssign, Sub, SubAssign},
-};
 
 /// A canonical 32-bit virtual memory address.
 #[derive(Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
@@ -57,40 +54,3 @@ impl fmt::UpperHex for VirtAddr {
 		fmt::UpperHex::fmt(&self.0, f)
 	}
 }
-
-impl fmt::Pointer for VirtAddr {
-	#[inline]
-	fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-		fmt::Pointer::fmt(&(self.0 as *const ()), f)
-	}
-}
-
-impl Add<u32> for VirtAddr {
-	type Output = Self;
-	#[inline]
-	fn add(self, rhs: u32) -> Self::Output {
-		VirtAddr(self.0.checked_add(rhs).unwrap())
-	}
-}
-
-impl AddAssign<u32> for VirtAddr {
-	#[inline]
-	fn add_assign(&mut self, rhs: u32) {
-		*self = *self + rhs;
-	}
-}
-
-impl Sub<u32> for VirtAddr {
-	type Output = Self;
-	#[inline]
-	fn sub(self, rhs: u32) -> Self::Output {
-		VirtAddr(self.0.checked_sub(rhs).unwrap())
-	}
-}
-
-impl SubAssign<u32> for VirtAddr {
-	#[inline]
-	fn sub_assign(&mut self, rhs: u32) {
-		*self = *self - rhs;
-	}
-}

tetros/src/lib.rs

@@ -23,12 +23,18 @@ mod os;
 #[macro_use]
 mod drivers;
 
-const PIC_OFFSET: u8 = 32;
-
 //
 // MARK: globals
 //
+// This code has no parallelism, so we don't _really_
+// need locks. The Mutexes here satisfy Rust's
+// "no mutable global state" rule.
+//
+// They also help prevent bugs, since we get deadlocks
+// instead of hard-to-debug surprising behavior.
+//
 
+const PIC_OFFSET: u8 = 32;
 static VGA: Mutex<Vga13h> = Mutex::new(unsafe { Vga13h::new() });
 static PIC: Mutex<PICDriver> = Mutex::new(PICDriver::new(PIC_OFFSET, PIC_OFFSET + 8));
 static TICK_COUNTER: Mutex<u32> = Mutex::new(0);
@@ -36,6 +42,8 @@ static BOARD: Mutex<TetrisBoard> = Mutex::new(TetrisBoard::new());
 static FALLING: Mutex<Option<FallingTetromino>> = Mutex::new(None);
 static LAST_INPUT: Mutex<Option<InputKey>> = Mutex::new(None);
 
+// These values can't be initialized statically,
+// so we cheat with `lazy_static`
 lazy_static! {
 	static ref RNG: Mutex<SmallRng> = Mutex::new(SmallRng::seed_from_u64(1337));
 	static ref IDT: InterruptDescriptorTable = {
@@ -50,9 +58,25 @@ lazy_static! {
 	};
 }
 
+#[allow(missing_docs)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+enum InputKey {
+	Left,
+	Right,
+	Up,
+	Down,
+}
+
 //
 // MARK: interrupts
 //
+// These functions are called when we receive interrupts.
+// This can occur between ANY two instructions---which is
+// why we use `without_interrupts` when acquiring locks.
+//
+// Notice how we do as little work as possible in our
+// interrupt handlers. All our business logic goes into
+// the main loop.
 
 #[derive(Debug, Clone, Copy)]
 #[repr(u8)]
@@ -72,22 +96,20 @@ impl InterruptIndex {
 }
 
 extern "x86-interrupt" fn divide_handler(stack_frame: InterruptStackFrame) {
+	// Simple interrupt handler, as an example.
+	// This can be triggered manually using `asm!("int 0")`,
+	// even if interrupts are disabled.
 	println!("DIVIDE ERROR {:?}", stack_frame);
 }
 
-#[derive(Debug, Clone, Copy, PartialEq, Eq)]
-enum InputKey {
-	Left,
-	Right,
-	Up,
-	Down,
-}
-
 extern "x86-interrupt" fn keyboard_handler(_stack_frame: InterruptStackFrame) {
 	{
 		// Re-seed our rng using user input.
 		// This is a simple hack that makes our
 		// "random" tile selector less deterministic.
+		//
+		// Getting random seeds from hardware is
+		// more trouble than its worth.
 		let mut rng = RNG.lock();
 		let past: u64 = rng.random();
 		let tcr = u64::from(*TICK_COUNTER.lock());
@@ -125,13 +147,13 @@ extern "x86-interrupt" fn keyboard_handler(_stack_frame: InterruptStackFrame) {
 		*LAST_INPUT.lock() = key;
 	}
 
-	PIC.lock().send_eoi(InterruptIndex::Keyboard.as_u8());
+	PIC.lock().send_eoi(false);
 }
 
 extern "x86-interrupt" fn timer_handler(_stack_frame: InterruptStackFrame) {
 	let mut t = TICK_COUNTER.lock();
 	*t = (*t).wrapping_add(1);
-	PIC.lock().send_eoi(InterruptIndex::Timer.as_u8());
+	PIC.lock().send_eoi(false);
 }
 
 extern "x86-interrupt" fn double_fault_handler(
@@ -190,6 +212,7 @@ pub unsafe extern "C" fn start(thunk10: extern "C" fn()) -> ! {
 		}
 		last_t = t;
 
+		// MARK: input
 		// Handle user input
 		without_interrupts(|| {
 			if let Some(fall) = &mut *FALLING.lock() {
@@ -236,6 +259,7 @@ pub unsafe extern "C" fn start(thunk10: extern "C" fn()) -> ! {
 			}
 		});
 
+		// MARK: update board
 		// Update board
 		without_interrupts(|| {
 			let mut v = VGA.lock();

tetros/src/os/eflags.rs

@@ -80,6 +80,7 @@ bitflags! {
 }
 
 impl EFlags {
+	/// Read the EFLAGS register
 	#[inline]
 	pub fn read() -> EFlags {
 		EFlags::from_bits_truncate(EFlags::read_raw())

tetros/src/os/panic.rs

@@ -1,8 +1,12 @@
-//! Intrinsics for panic handling
+//! Rust intrinsics for panic handling.
+//!
+//! These are usually provided by `std`,
+//! but we don't have that luxury!
 
 use core::arch::asm;
 use core::panic::PanicInfo;
 
+// Use serial println
 use crate::println;
 
 #[lang = "eh_personality"]

tetros/src/os/util.rs

@@ -18,11 +18,13 @@ pub fn sti() {
 	}
 }
 
-/// Run a closure with disabled interrupts.
-///
 /// Run the given closure, disabling interrupts before running it (if they aren't already disabled).
 /// Afterwards, interrupts are enabling again if they were enabled before.
 ///
+/// This helps us prevent deadlocks, which can occur if
+/// an interrupt handler tries to acquire a lock that was
+/// locked at the time of the interrupt.
+///
 /// If you have other `enable` and `disable` calls _within_ the closure, things may not work as expected.
 #[inline]
 pub fn without_interrupts<F, R>(f: F) -> R
@@ -46,6 +48,7 @@ where
 	ret
 }
 
+/// Wraps the `in` instruction
 pub unsafe fn inb(port: u32) -> u8 {
 	let mut out;
 
@@ -58,6 +61,7 @@ pub unsafe fn inb(port: u32) -> u8 {
 	return out;
 }
 
+/// Wraps the `out` instruction
 pub unsafe fn outb(port: u32, value: u8) {
 	asm!(
 		"out dx, al",