Refactor and improve instruction execution behavior

Standardize instruction cycle handling and update return types for better clarity and accuracy. Add cycle count assertions in tests to ensure proper execution timings. Minor formatting fixes and typo corrections throughout the codebase.

.idea/dictionaries/project.xml

@@ -16,6 +16,7 @@
       <w>reti</w>
       <w>rrca</w>
       <w>rrla</w>
+      <w>tama</w>
       <w>vram</w>
     </words>
   </dictionary>

Cargo.toml

@@ -7,4 +7,4 @@ edition = "2021"
 
 [dependencies]
 serde_json = "1.0.140"
-glob = "0.3.2"
+glob = "0.3.2"

src/instructions.rs

@@ -4,7 +4,7 @@ pub enum TargetRegister { A, B, C, D, E, H, L, }
 #[derive(Clone, Copy, Debug)]
 pub enum TargetU16Register {AF, BC, DE, HL, SP, PC}
 
-#[derive(Clone, Copy, Debug)]
+#[derive(Clone, Copy, Debug, PartialEq)]
 pub enum Condition {
     NZ, // Not Zero
     Z,  // Zero
@@ -606,4 +606,4 @@ fn match_cb_instruction(opcode: u8) -> Instruction {
         0xFF => { Instruction::SET(7, Target::U8Register(TargetRegister::A)) }
         _ => { panic!("Invalid u16 opcode: {:02X}", opcode); }
     }
-}
+}

src/main.rs

@@ -41,7 +41,7 @@ enum CartridgeType{
 
 impl GameRom {
     fn load(filename: &str) -> GameRom {
-        
+
         let data = std::fs::read(filename).unwrap();
         let title_end = data[0x134..0x143].iter().position(|b| *b == 0x0).unwrap();
         let title = data[0x134..0x134 + title_end].to_vec().escape_ascii().to_string();
@@ -52,7 +52,7 @@ impl GameRom {
         let licensee = [data[0x144], data[0x145]];
         let sgb_flag = data[0x146];
         println!("{:x}", data[0x147]);
-        
+
         let cartridge_type = match data[0x147] {
             0x00 => CartridgeType::RomOnly,
             0x01 => CartridgeType::MBC1,
@@ -113,7 +113,7 @@ impl GameRom {
             }
             _ => {panic!("unsupported Cartridge type")}
         }
-        
+
 
     }
 }
@@ -152,14 +152,14 @@ impl GPU {
         // Tiles rows are encoded in two bytes with the first byte always
         // on an even address. Bitwise ANDing the address with 0xffe
         // gives us the address of the first byte.
-        // For example: `12 & 0xFFFE == 12` and `13 & 0xFFFE == 12`
+        // For example, `12 & 0xFFFE == 12` and `13 & 0xFFFE == 12`
         let normalized_index = index & 0xFFFE;
 
-        // First we need to get the two bytes that encode the tile row.
+        // First, we need to get the two bytes that encode the tile row.
         let byte1 = self.vram[normalized_index];
         let byte2 = self.vram[normalized_index + 1];
 
-        // A tiles is 8 rows tall. Since each row is encoded with two bytes a tile
+        // Tiles are 8 rows tall. Since each row is encoded with two bytes, a tile
         // is therefore 16 bytes in total.
         let tile_index = index / 16;
         // Every two bytes is a new row
@@ -167,19 +167,19 @@ impl GPU {
 
         // Now we're going to loop 8 times to get the 8 pixels that make up a given row.
         for pixel_index in 0..8 {
-            // To determine a pixel's value we must first find the corresponding bit that encodes
+            // To determine a pixel's value, we must first find the corresponding bit that encodes
-            // that pixels value:
+            // that pixel value:
             // 1111_1111
             // 0123 4567
             //
-            // As you can see the bit that corresponds to the nth pixel is the bit in the nth
+            // As you can see, the bit that corresponds to the nth pixel is the bit in the nth
             // position *from the left*. Bits are normally indexed from the right.
             //
             // To find the first pixel (a.k.a pixel 0) we find the left most bit (a.k.a bit 7). For
             // the second pixel (a.k.a pixel 1) we first the second most left bit (a.k.a bit 6) and
             // so on.
             //
-            // We then create a mask with a 1 at that position and 0s everywhere else.
+            // We then create a mask with a 1 at that position and 0 s everywhere else.
             //
             // Bitwise ANDing this mask with our bytes will leave that particular bit with its
             // original value and every other bit with a 0.
@@ -187,10 +187,10 @@ impl GPU {
             let lsb = byte1 & mask;
             let msb = byte2 & mask;
 
-            // If the masked values are not 0 the masked bit must be 1. If they are 0, the masked
+            // If the masked values are not 0, the masked bit must be 1. If they are 0, the masked
             // bit must be 0.
             //
-            // Finally we can tell which of the four tile values the pixel is. For example, if the least
+            // Finally, we can tell which of the four tile values the pixel is. For example, if the least
             // significant byte's bit is 1 and the most significant byte's bit is also 1, then we
             // have tile value `Three`.
             let value = match (lsb != 0, msb != 0) {
@@ -255,7 +255,7 @@ impl MemoryBus {
             CART_BEGIN ..= CART_END => {
                 self.rom.write_byte(address, value)
             }
-            _ => self.memory[address as usize] = value
+            _ => self.memory[address] = value
         }
     }
     fn read_u16(&self, address: u16) -> u16 {
@@ -445,7 +445,7 @@ impl CPU {
 
 
 
-    fn execute_next_instruction(&mut self) {
+    fn execute_next_instruction(&mut self) -> u8 {
         // if self.pc == 0xC {
         //     println!("clear vram Complete");
         // }
@@ -454,9 +454,10 @@ impl CPU {
         // }
         let inst = parse_instruction(self.bus.read_byte(self.pc), self.bus.read_byte(self.pc.wrapping_add(1)), self.bus.read_byte(self.pc.wrapping_add(2)));
         // println!("{:x} {:?} {:?}", self.pc, inst, self.registers.f);
-        
+
         let result = self.execute(inst);
-        self.pc = result.pc
+        self.pc = result.pc;
+        result.cycles
     }
 
     fn standard_inc_pc_and_timing(&mut self, target: Target) -> ExecutionReturn {
@@ -468,7 +469,7 @@ impl CPU {
                 ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 2}
             }
             Target::Immediate(_) => {
-                ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 3}
+                ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 2}
             },
         }
     }
@@ -618,13 +619,18 @@ impl CPU {
                 self.registers.f.subtract = true;
                 let (_, half_carry) = (value & 0xF).overflowing_sub(1);
                 self.registers.f.half_carry = half_carry;
-                self.standard_inc_pc_and_timing(target)
+                match target {
+                    Target::U8Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 1}}
+                    Target::U16Register(TargetU16Register::SP) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 2}}
+                    Target::U16Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 3}}
+                    Target::Immediate(_) => {panic!("Does not accept immediate target")}
+                }
             }
             Instruction::DECU16(register) => {
                 let value = self.get_u16_reg_value(register);
                 let new_value = value.wrapping_sub(1);
                 self.set_u16_reg_value(register, new_value);
-                ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 1}
+                ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 2}
             }
             Instruction::DI => {
                 self.interrupts_enabled = false;
@@ -635,7 +641,7 @@ impl CPU {
                 ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 1}
             }
             Instruction::HALT => {
-                ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 1}
+                ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 3}
             }
 
             Instruction::INC(target) => {
@@ -645,7 +651,12 @@ impl CPU {
                 self.registers.f.zero = new_value == 0;
                 self.registers.f.subtract = false;
                 self.registers.f.half_carry = (value & 0xF) + 1 > 0xF;
-                self.standard_inc_pc_and_timing(target)
+                match target {
+                    Target::U8Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 1}}
+                    Target::U16Register(TargetU16Register::SP) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 2}}
+                    Target::U16Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 3}}
+                    Target::Immediate(_) => {panic!("Does not accept immediate target")}
+                }
             }
             Instruction::INCU16(register) => {
                 let value = self.get_u16_reg_value(register);
@@ -666,7 +677,7 @@ impl CPU {
             }
             Instruction::JR(condition, offset) => {
                 if self.check_condition(condition) {
-                    ExecutionReturn{pc: self.pc.wrapping_add_signed(offset as i16).wrapping_add(2), cycles: 4}
+                    ExecutionReturn{pc: self.pc.wrapping_add_signed(offset as i16).wrapping_add(2), cycles: 3}
                 } else {
                     ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 2}
                 }
@@ -716,13 +727,21 @@ impl CPU {
                         let offset = self.get_target_value(target);
                         let address = 0xFF00 | offset as u16;
                         self.bus.write_byte(address, self.registers.a);
-                        self.standard_inc_pc_and_timing(target)
+                        match target {
+                            Target::U8Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 2}}
+                            Target::U16Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 3}}
+                            Target::Immediate(_) => {ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 3}}
+                        }
                     }
                     LoadTarget::CopyAPort(target) => {
                         let offset = self.get_target_value(target);
                         let address = 0xFF00 | offset as u16;
                         self.registers.a = self.bus.read_byte(address);
-                        self.standard_inc_pc_and_timing(target)
+                        match target {
+                            Target::U8Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 2}}
+                            Target::U16Register(_) => {ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 3}}
+                            Target::Immediate(_) => {ExecutionReturn{pc: self.pc.wrapping_add(2), cycles: 3}}
+                        }
                     }
                     LoadTarget::CopyAR16(source_register) => {
                         let address = self.get_u16_reg_value(source_register);
@@ -966,7 +985,7 @@ impl CPU {
                 self.rotate_inc_pc_and_timing(target)
             }
             Instruction::STOP(_) => {
-                ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 1}
+                ExecutionReturn{pc: self.pc.wrapping_add(1), cycles: 3}
             }
             Instruction::SUB(target) => {
                 let value = self.get_target_value(target);
@@ -1012,6 +1031,7 @@ fn main() {
 }
 
 fn run_instruction_tests() {
+
     for entry in glob("sm83/v1/*.json").unwrap() {
         println!("{:?}", entry);
         let json_file = std::fs::read(entry.unwrap()).unwrap();
@@ -1019,7 +1039,23 @@ fn run_instruction_tests() {
         for test in tests.as_array().unwrap() {
             // println!("{}", test["name"]);
             let mut gameboy = CPU::load_test(&test["initial"]);
-            gameboy.execute_next_instruction();
+
+            // Get the instruction before executing it
+            let pc = gameboy.pc;
+            let inst = parse_instruction(
+                gameboy.bus.read_byte(pc), 
+                gameboy.bus.read_byte(pc.wrapping_add(1)), 
+                gameboy.bus.read_byte(pc.wrapping_add(2))
+            );
+
+            // Execute the instruction and get the cycle count
+            let cycles = gameboy.execute_next_instruction();
+
+            // Verify the cycle count
+            let expected_cycles = test["cycles"].as_array().unwrap().len();
+            assert_eq!(cycles, expected_cycles as u8);
+
+            // Compare the final state
             gameboy.compare_state(&test["final"]);
         }
     }
@@ -1050,6 +1086,6 @@ fn run_gameboy() {
         if count % 1_000_000 == 0 {
             println!("PC: {:04X}, {count}", gameboy.pc);
         }
-        gameboy.execute_next_instruction()
+        gameboy.execute_next_instruction();
     }
-}
+}