fix(cpu): reimplement instruction handling

src/cpu.rs

@@ -15,7 +15,7 @@ pub struct Cpu {
     flags: Flags,
     ime: ImeState,
     // TODO: Merge halted and state properties
-    halted: Option<HaltState>,
+    halted: Option<HaltKind>,
     state: State,
 }
 
@@ -53,7 +53,7 @@ impl Cpu {
         self.ime = state;
     }
 
-    pub(crate) fn halt(&mut self, state: HaltState) {
+    pub(crate) fn halt(&mut self, state: HaltKind) {
         self.halted = Some(state);
     }
 
@@ -61,8 +61,8 @@ impl Cpu {
         self.halted = None;
     }
 
-    pub(crate) fn halted(&self) -> Option<&HaltState> {
+    pub(crate) fn halted(&self) -> Option<HaltKind> {
-        self.halted.as_ref()
+        self.halted
     }
 
     pub fn load_cartridge(&mut self, path: &str) -> std::io::Result<()> {
@@ -75,6 +75,8 @@ impl Cpu {
 }
 
 impl Cpu {
+    /// Fetch an [Instruction] from the memory bus
+    /// (4 cycles)
     fn fetch(&mut self) -> u8 {
         let byte = self.read_byte(self.reg.pc);
         self.bus.clock();
@@ -82,6 +84,10 @@ impl Cpu {
         byte
     }
 
+    /// Decode a byte into an [SM83](Cpu) [Instruction]
+    ///
+    /// If opcode == 0xCB, then decoding costs 4 cycles.
+    /// Otherwise, decoding is free
     pub(crate) fn decode(&mut self, opcode: u8) -> Instruction {
         if opcode == 0xCB {
             Instruction::decode(self.fetch(), true)
@@ -90,38 +96,46 @@ impl Cpu {
         }
     }
 
+    /// Execute an [Instruction].
+    ///
+    /// The amount of cycles necessary to execute an instruction range from
+    /// 0 to 20 T-cycles
     fn execute(&mut self, instruction: Instruction) -> Cycle {
         Instruction::execute(self, instruction)
     }
 
+    /// Perform the [`Cpu::fetch()`] [`Cpu::decode(opcode)`] [`Cpu::execute(instr)`]
+    /// routine.
+    ///
+    /// Handle HALT state and interrupts.
     pub fn step(&mut self) -> Cycle {
-        // // Log instructions
+        // Log instructions
         // if self.reg.pc > 0xFF {
         //     let out = std::io::stdout();
-        //     let _ = self._print_debug(out.lock());
+        //     let _ = self._print_logs(out.lock());
         // }
 
-        // FIXME: The Halt instruction takes more cycles than it should in Blargg's 2nd cpu_instrs test
+        if let Some(elapsed) = self.handle_interrupt() {
-        let elapsed = match self.halted() {
+            return elapsed;
-            Some(state) => {
+        }
-                use HaltState::*;
+
+        if let Some(kind) = self.halted() {
+            use HaltKind::*;
 
-                match state {
-                    ImeEnabled | NonePending => {
             self.bus.clock();
-                        Cycle::new(4)
+
-                    }
+            let elapsed = match kind {
+                ImeEnabled | NonePending => Cycle::new(4),
                 SomePending => todo!("Implement HALT bug"),
+            };
+
+            return elapsed;
         }
-            }
+
-            None => {
         let opcode = self.fetch();
         let instr = self.decode(opcode);
         let elapsed = self.execute(instr);
-                self.check_ime();
+        self.handle_ei();
-                elapsed
-            }
-        };
 
         // For use in Blargg's Test ROMs
         if self.read_byte(0xFF02) == 0x81 {
@@ -130,8 +144,6 @@ impl Cpu {
             eprint!("{}", c);
         }
 
-        // TODO: Is this in the wrong place?
-        self.handle_interrupts();
         elapsed
     }
 }
@@ -159,18 +171,11 @@ impl Cpu {
         &mut self.bus.joypad
     }
 
-    fn check_ime(&mut self) {
+    fn handle_ei(&mut self) {
         match self.ime {
-            ImeState::Pending => {
+            ImeState::EiExecuted => self.ime = ImeState::Pending,
-                // This is within the context of the EI instruction, we need to not update EI until the end of the
+            ImeState::Pending => self.ime = ImeState::Enabled,
-                // next executed Instruction
+            ImeState::Disabled | ImeState::Enabled => {}
-                self.ime = ImeState::PendingEnd;
-            }
-            ImeState::PendingEnd => {
-                // The Instruction after EI has now been executed, so we want to enable the IME flag here
-                self.ime = ImeState::Enabled;
-            }
-            ImeState::Disabled | ImeState::Enabled => {} // Do Nothing
         }
     }
 
@@ -182,54 +187,56 @@ impl Cpu {
         self.read_byte(0xFFFF)
     }
 
-    fn handle_interrupts(&mut self) {
+    fn handle_interrupt(&mut self) -> Option<Cycle> {
-        let req = self.int_request();
+        let irq = self.int_request();
-        let enabled = self.int_enable();
+        let enable = self.int_enable();
 
+        // TODO: Ensure that this behaviour is correct
         if self.halted.is_some() {
             // When we're here either a HALT with IME set or
             // a HALT with IME not set and No pending Interrupts was called
 
-            if req & enabled != 0 {
+            if irq & enable != 0 {
                 // The if self.ime() below correctly follows the "resuming from HALT" behaviour so
                 // nothing actually needs to be added here. This is just documentation
                 // since it's a bit weird why nothing is being done
 
-                self.resume()
+                self.resume();
             }
         }
 
-        if let ImeState::Enabled = self.ime() {
+        match self.ime() {
-            let mut req: InterruptFlag = req.into();
+            ImeState::Enabled => {
-            let enabled: InterruptEnable = enabled.into();
+                let mut irq: InterruptFlag = irq.into();
+                let enable: InterruptEnable = enable.into();
 
-            let vector = if req.vblank() && enabled.vblank() {
+                let rst_vector = if irq.vblank() && enable.vblank() {
                     // Handle VBlank Interrupt
-                req.set_vblank(false);
+                    irq.set_vblank(false);
 
                     // INT 40h
                     Some(0x40)
-            } else if req.lcd_stat() && enabled.lcd_stat() {
+                } else if irq.lcd_stat() && enable.lcd_stat() {
                     // Handle LCD STAT Interrupt
-                req.set_lcd_stat(false);
+                    irq.set_lcd_stat(false);
 
                     // INT 48h
                     Some(0x48)
-            } else if req.timer() && enabled.timer() {
+                } else if irq.timer() && enable.timer() {
                     // Handle Timer Interrupt
-                req.set_timer(false);
+                    irq.set_timer(false);
 
                     // INT 50h
                     Some(0x50)
-            } else if req.serial() && enabled.serial() {
+                } else if irq.serial() && enable.serial() {
                     // Handle Serial Interrupt
-                req.set_serial(false);
+                    irq.set_serial(false);
 
                     // INT 58h
                     Some(0x58)
-            } else if req.joypad() && enabled.joypad() {
+                } else if irq.joypad() && enable.joypad() {
                     // Handle Joypad Interrupt
-                req.set_joypad(false);
+                    irq.set_joypad(false);
 
                     // INT 60h
                     Some(0x60)
@@ -237,17 +244,19 @@ impl Cpu {
                     None
                 };
 
-            let _ = match vector {
+                match rst_vector {
-                Some(address) => {
+                    Some(vector) => {
                         //  Write the Changes to 0xFF0F and 0xFFFF registers
-                    self.write_byte(0xFF0F, req.into());
+                        self.write_byte(0xFF0F, irq.into());
 
                         // Disable all future interrupts
                         self.set_ime(ImeState::Disabled);
-                    Instruction::reset(self, address)
+                        Some(Instruction::reset(self, vector))
                     }
-                None => Cycle::new(0), // NO Interrupts were enabled and / or requested
+                    None => None,
-            };
+                }
+            }
+            _ => None,
         }
     }
 }
@@ -505,7 +514,7 @@ impl From<u8> for Flags {
 }
 
 #[derive(Debug, Clone, Copy)]
-pub(crate) enum HaltState {
+pub(crate) enum HaltKind {
     ImeEnabled,
     NonePending,
     SomePending,
@@ -514,8 +523,8 @@ pub(crate) enum HaltState {
 #[derive(Debug, Clone, Copy)]
 pub(crate) enum ImeState {
     Disabled,
+    EiExecuted,
     Pending,
-    PendingEnd,
     Enabled,
 }
 

src/instruction.rs

@@ -9,7 +9,7 @@ use self::table::{
 };
 use self::table::{Group1RegisterPair, Group2RegisterPair, Group3RegisterPair, Register};
 use crate::bus::{Bus, BusIo};
-use crate::cpu::{Cpu, Flags, HaltState, ImeState, Register as CpuRegister, RegisterPair};
+use crate::cpu::{Cpu, Flags, HaltKind, ImeState, Register as CpuRegister, RegisterPair};
 
 #[allow(clippy::upper_case_acronyms)]
 #[derive(Clone, Copy)]
@@ -588,14 +588,14 @@ impl Instruction {
             }
             Instruction::HALT => {
                 // HALT | Enter CPU low power consumption mode until interrupt occurs
-                use HaltState::*;
+                use HaltKind::*;
 
-                let halt_state = match *cpu.ime() {
+                let kind = match *cpu.ime() {
                     ImeState::Enabled => ImeEnabled,
                     _ if cpu.int_request() & cpu.int_enable() != 0 => SomePending,
                     _ => NonePending,
                 };
-                cpu.halt(halt_state);
+                cpu.halt(kind);
                 Cycle::new(4)
             }
             Instruction::ADC(source) => match source {
@@ -971,7 +971,7 @@ impl Instruction {
             }
             Instruction::EI => {
                 // EI | Enable IME after the next instruction
-                cpu.set_ime(ImeState::Pending);
+                cpu.set_ime(ImeState::EiExecuted);
                 Cycle::new(4)
             }
             Instruction::CALL(cond) => {