fix step on interrupts (#80)

* fix step on interrupts

* tweak clippy in pre-commit hook

* mutated_pc must be after cycles

* dummy

.github/workflows/main.yml

@@ -2,26 +2,25 @@ name: main
 
 on:
   push:
-    branches: [ main ]
+    branches: [main]
   pull_request:
-    branches: [ main ]
+    branches: [main]
 
 env:
   CARGO_TERM_COLOR: always
   RUSTFLAGS: -D warnings
 
-
 jobs:
   CI:
     runs-on: ubuntu-latest
     steps:
-    - uses: actions/checkout@v4
-    - uses: Swatinem/rust-cache@v2
-    - name: Version
-      run: rustc --version
-    - name: Run rustfmt
-      run: cargo fmt --check
-    - name: Unit tests
-      run: cargo test -- --include-ignored --nocapture
-    - name : Clippy
-      run: cargo clippy --all-features
+      - uses: actions/checkout@v4
+      - uses: Swatinem/rust-cache@v2
+      - name: Version
+        run: rustc --version
+      - name: Run rustfmt
+        run: cargo fmt --check
+      - name: Unit tests
+        run: cargo test -- --include-ignored --nocapture
+      - name: Clippy
+        run: cargo clippy --all-features

bin/pre-commit

@@ -2,5 +2,6 @@
 set -euo pipefail
 FILES=$(git diff --cached --name-only --diff-filter=ACMR | sed 's| |\\ |g')
 cargo fmt
-cargo clippy --all-features
-echo "$FILES" | xargs git add
+if cargo clippy --all-features --fix --allow-dirty | grep "warning"; then
+  exit 1
+fi

fpt/src/bw.rs

@@ -22,6 +22,10 @@ pub fn set_bit8<const INDEX: u8>(word: u8, value: bool) -> u8 {
     word & !(1 << INDEX) | (u8::from(value) << INDEX)
 }
 
+pub fn set_bit8_dyn(word: u8, index: u8, value: bool) -> u8 {
+    word & !(1 << index) | (u8::from(value) << index)
+}
+
 pub fn set_bit16<const INDEX: u8>(word: u16, value: bool) -> u16 {
     word & !(1 << INDEX) | (u16::from(value) << INDEX)
 }

fpt/src/lr35902.rs

@@ -656,55 +656,61 @@ impl LR35902 {
 
         self.execute(inst);
 
-        self.set_inst_cycle_count(0);
-        let cycles = if inst.kind == InstructionKind::Jump && !self.mutated_pc() {
+        let inst_cycles = if inst.kind == InstructionKind::Jump && !self.mutated_pc() {
             inst.cycles_not_taken
         } else {
             inst.cycles
         };
-        self.set_clock_cycles(self.clock_cycles() + cycles as u64);
-
         if !self.mutated_pc() {
             self.set_pc(self.pc() + inst.size as u16);
         }
         self.set_mutated_pc(false);
 
-        if self.ime {
-            let iflag = self.bus.iflag();
-            let intr = iflag & self.bus.ie();
-            let isv;
-            if bw::test_bit8::<0>(intr) {
-                isv = 0x40;
-                self.bus.set_iflag(bw::set_bit8::<0>(iflag, false));
-            } else if bw::test_bit8::<1>(intr) {
-                isv = 0x48;
-                self.bus.set_iflag(bw::set_bit8::<1>(iflag, false));
-            } else if bw::test_bit8::<2>(intr) {
-                isv = 0x50;
-                self.bus.set_iflag(bw::set_bit8::<2>(iflag, false));
-            } else if bw::test_bit8::<3>(intr) {
-                isv = 0x58;
-                self.bus.set_iflag(bw::set_bit8::<3>(iflag, false));
-            } else if bw::test_bit8::<4>(intr) {
-                isv = 0x60;
-                self.bus.set_iflag(bw::set_bit8::<4>(iflag, false));
-            } else {
-                return cycles;
-            }
-            // TODO: this is a big lie. we cant just run 5 cycles inside the function supposed to run 1 cycle lmao
-            self.set_ime(false);
-            self.push(self.pc());
-            self.set_pc(isv);
-            self.set_clock_cycles(self.clock_cycles() + 5);
-            return cycles + 5;
-        }
+        let intr_preamble_cycles = if self.ime { self.run_interrupts() } else { 0 };
+
+        let total_cycles = inst_cycles + intr_preamble_cycles;
+        self.set_clock_cycles(self.clock_cycles() + total_cycles as u64);
+        self.set_inst_cycle_count(0);
 
         if self.debugger.step {
             self.set_paused(true);
             self.debugger.step = false;
         }
 
-        cycles
+        total_cycles
+    }
+
+    /// To run an interrupt, we first run the interrupt service routine (ISR).
+    /// That on itself takes 5 M-cycles, or 20 T-cycles.
+    fn run_interrupts(&mut self) -> u8 {
+        let iflag = self.bus.iflag();
+        let intr = iflag & self.bus.ie();
+        let isv;
+        let intr_bit = if bw::test_bit8::<0>(intr) {
+            isv = 0x40;
+            0
+        } else if bw::test_bit8::<1>(intr) {
+            isv = 0x48;
+            1
+        } else if bw::test_bit8::<2>(intr) {
+            isv = 0x50;
+            2
+        } else if bw::test_bit8::<3>(intr) {
+            isv = 0x58;
+            3
+        } else if bw::test_bit8::<4>(intr) {
+            isv = 0x60;
+            4
+        } else {
+            // No interrupt is pending
+            return 0;
+        };
+        self.bus.set_iflag(bw::set_bit8_dyn(iflag, intr_bit, false));
+        self.set_ime(false);
+        self.push(self.pc());
+        self.set_pc(isv);
+        // https://gbdev.io/pandocs/Interrupts.html#interrupt-handling
+        20
     }
 
     /// Run one complete instruction - NOT a machine cycle (4 t-cycles)
@@ -714,7 +720,7 @@ impl LR35902 {
         while cycles_ran < instruction.cycles {
             cycles_ran += self.step();
         }
-        instruction.cycles
+        cycles_ran
     }
 
     fn execute(&mut self, instruction: Instruction) {

fpt/src/memory.rs

@@ -245,7 +245,7 @@ impl Memory {
         }
     }
 
-    pub fn array_ref<const N: usize>(&self, from: Address) -> &[u8; N] {
+    fn array_ref<const N: usize>(&self, from: Address) -> &[u8; N] {
         self.mem[from..from + N].try_into().unwrap() // guaranteed to have size N
     }
 
@@ -302,29 +302,14 @@ impl Bus {
     }
 
     pub fn read(&self, address: Address) -> u8 {
-        if address == map::JOYP {
-            self.joyp()
-        } else {
-            self.memory().mem[address as Address]
-        }
-    }
-
-    pub fn write(&mut self, address: Address, value: u8) {
-        self.memory_mut().mem[address as Address] = value;
-    }
-
-    fn _read(&self, address: Address) -> u8 {
         if address == map::JOYP {
             self.joyp()
         } else {
             self.memory().mem[address]
         }
     }
 
-    fn _write(&mut self, address: Address, value: u8) {
-        if address == map::TAC {
-            println!("write to TAC: {}", value);
-        }
+    pub fn write(&mut self, address: Address, value: u8) {
         self.memory_mut().mem[address] = value;
     }
 
@@ -351,51 +336,51 @@ impl Bus {
 
     // registers
     pub fn lcdc(&self) -> u8 {
-        self._read(map::LCDC)
+        self.read(map::LCDC)
     }
 
     pub fn set_lcdc(&mut self, value: u8) {
-        self._write(map::LCDC, value);
+        self.write(map::LCDC, value);
     }
 
     pub fn stat(&self) -> u8 {
-        self._read(map::STAT)
+        self.read(map::STAT)
     }
 
     pub fn set_stat(&mut self, value: u8) {
-        self._write(map::STAT, value);
+        self.write(map::STAT, value);
     }
 
     pub fn scy(&self) -> u8 {
-        self._read(map::SCY)
+        self.read(map::SCY)
     }
 
     pub fn set_scy(&mut self, value: u8) {
-        self._write(map::SCY, value);
+        self.write(map::SCY, value);
     }
 
     pub fn scx(&self) -> u8 {
-        self._read(map::SCX)
+        self.read(map::SCX)
     }
 
     pub fn set_scx(&mut self, value: u8) {
-        self._write(map::SCX, value);
+        self.write(map::SCX, value);
     }
 
     pub fn ly(&self) -> u8 {
-        self._read(map::LY)
+        self.read(map::LY)
     }
 
     pub fn set_ly(&mut self, value: u8) {
-        self._write(map::LY, value);
+        self.write(map::LY, value);
     }
 
     pub fn lyc(&self) -> u8 {
-        self._read(map::LYC)
+        self.read(map::LYC)
     }
 
     pub fn set_lyc(&mut self, value: u8) {
-        self._write(map::LYC, value)
+        self.write(map::LYC, value)
     }
 
     pub fn with_vram<R>(&self, reader: impl FnOnce(&[u8]) -> R) -> R {
@@ -435,14 +420,14 @@ impl Bus {
     }
 
     pub fn ie(&self) -> u8 {
-        self._read(map::IE)
+        self.read(map::IE)
     }
 
     pub fn iflag(&self) -> u8 {
-        self._read(map::IF)
+        self.read(map::IF)
     }
 
     pub fn set_iflag(&mut self, value: u8) {
-        self._write(map::IF, value)
+        self.write(map::IF, value)
     }
 }

fpt/tests/templates/header

@@ -21,6 +21,7 @@ fn rom_test(rom_path: &str, termination_address: u16, passing: bool) {{
     gb.boot_fake();
 
     let mut success = true;
+    #[allow(clippy::never_loop)]
     'outer: loop {{
         gb.step();
         let debug_events = gb.get_debug_events();