From 46b26c08545fd7e7fececd0d9eafb4cc54c9da2d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Jo=C3=A3o=20Magalh=C3=A3es?= <joamag@gmail.com>
Date: Sun, 18 Feb 2024 23:17:45 +0000
Subject: [PATCH] chore: much improved error handling

Support for some basic Python testing.
Handling of loading error with proper error propagation.
---
 .gitignore                            |  3 ++
 .gitlab-ci.yml                        | 11 +++++++
 README.md                             |  6 ++++
 frontends/libretro/src/lib.rs         |  2 +-
 frontends/sdl/src/main.rs             | 17 ++++++-----
 setup.py                              |  1 +
 src/gb.rs                             | 39 +++++++++++++++++-------
 src/py.rs                             | 14 ++++++---
 src/python/boytacean/gb.py            |  6 +++-
 src/python/boytacean/test/__init__.py |  0
 src/python/boytacean/test/base.py     | 21 +++++++++++++
 src/test.rs                           | 43 +++++++++++++++++----------
 12 files changed, 125 insertions(+), 38 deletions(-)
 create mode 100644 src/python/boytacean/test/__init__.py
 create mode 100644 src/python/boytacean/test/base.py

diff --git a/.gitignore b/.gitignore
index c093af01..b61206a4 100644
--- a/.gitignore
+++ b/.gitignore
@@ -1,4 +1,5 @@
 *.pyc
+*.pyd
 
 .DS_Store
 
@@ -6,6 +7,8 @@ Cargo.lock
 
 /.vscode/settings.json
 
+/.eggs
+/.venv
 /.idea
 
 /ndk
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 40733f41..9b52b50d 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -79,6 +79,17 @@ test-rust:
     - rustc --version
     - cargo test
 
+test-pyo3:
+  stage: test
+  parallel:
+    matrix:
+      - RUST_VERSION: ["1.74.0"]
+  script:
+    - rustup toolchain install $RUST_VERSION
+    - rustup override set $RUST_VERSION
+    - rustc --version
+    - python setup.py test
+
 deploy-netlify-preview:
   stage: deploy
   script:
diff --git a/README.md b/README.md
index cf6459a7..044f1c28 100644
--- a/README.md
+++ b/README.md
@@ -61,6 +61,12 @@ cargo build
 pip install .
 ```
 
+or
+
+```bash
+python setup.py install
+```
+
 ### WASM for Node.js
 
 ```bash
diff --git a/frontends/libretro/src/lib.rs b/frontends/libretro/src/lib.rs
index cb8d838f..c1540645 100644
--- a/frontends/libretro/src/lib.rs
+++ b/frontends/libretro/src/lib.rs
@@ -385,7 +385,7 @@ pub unsafe extern "C" fn retro_load_game(game: *const RetroGameInfo) -> bool {
     instance.set_mode(mode);
     instance.reset();
     instance.load(true);
-    instance.load_cartridge(rom);
+    instance.load_cartridge(rom).unwrap();
     update_vars();
     true
 }
diff --git a/frontends/sdl/src/main.rs b/frontends/sdl/src/main.rs
index f7852aa2..1c073ab5 100644
--- a/frontends/sdl/src/main.rs
+++ b/frontends/sdl/src/main.rs
@@ -8,6 +8,7 @@ pub mod test;
 use audio::Audio;
 use boytacean::{
     devices::{printer::PrinterDevice, stdout::StdoutDevice},
+    error::Error,
     gb::{AudioProvider, GameBoy, GameBoyMode},
     info::Info,
     pad::PadKey,
@@ -225,7 +226,7 @@ impl Emulator {
         ));
     }
 
-    pub fn load_rom(&mut self, path: Option<&str>) {
+    pub fn load_rom(&mut self, path: Option<&str>) -> Result<(), Error> {
         let rom_path: &str = path.unwrap_or(&self.rom_path);
         let ram_path = replace_ext(rom_path, "sav").unwrap_or_else(|| "invalid".to_string());
         let rom = self.system.load_rom_file(
@@ -235,7 +236,7 @@ impl Emulator {
             } else {
                 None
             },
-        );
+        )?;
         println!(
             "========= Cartridge =========\n{}\n=============================",
             rom
@@ -253,12 +254,14 @@ impl Emulator {
             .to_str()
             .unwrap()
             .to_string();
+        Ok(())
     }
 
-    pub fn reset(&mut self) {
+    pub fn reset(&mut self) -> Result<(), Error> {
         self.system.reset();
         self.system.load(true);
-        self.load_rom(None);
+        self.load_rom(None)?;
+        Ok(())
     }
 
     pub fn apply_cheats(&mut self, cheats: &Vec<String>) {
@@ -418,7 +421,7 @@ impl Emulator {
                     Event::KeyDown {
                         keycode: Some(Keycode::R),
                         ..
-                    } => self.reset(),
+                    } => self.reset().unwrap(),
                     Event::KeyDown {
                         keycode: Some(Keycode::B),
                         ..
@@ -528,7 +531,7 @@ impl Emulator {
                         }
                         self.system.reset();
                         self.system.load(true);
-                        self.load_rom(Some(&filename));
+                        self.load_rom(Some(&filename)).unwrap();
                     }
                     _ => (),
                 }
@@ -989,7 +992,7 @@ fn main() {
     };
     let mut emulator = Emulator::new(game_boy, options);
     emulator.start(SCREEN_SCALE);
-    emulator.load_rom(Some(&args.rom_path));
+    emulator.load_rom(Some(&args.rom_path)).unwrap();
     emulator.apply_cheats(&args.cheats);
     emulator.toggle_palette();
 
diff --git a/setup.py b/setup.py
index 51b98c15..f4997018 100644
--- a/setup.py
+++ b/setup.py
@@ -35,6 +35,7 @@ setuptools.setup(
     keywords="gameboy emulator rust",
     url="https://boytacean.joao.me",
     packages=["boytacean"],
+    test_suite="boytacean.test",
     package_dir={"": os.path.normpath("src/python")},
     rust_extensions=[
         setuptools_rust.RustExtension(
diff --git a/src/gb.rs b/src/gb.rs
index 6386b228..26082896 100644
--- a/src/gb.rs
+++ b/src/gb.rs
@@ -426,9 +426,20 @@ impl GameBoy {
         let rom = self.rom().clone();
         self.reset();
         self.load(true);
-        self.load_cartridge(rom);
+        self.load_cartridge(rom).unwrap();
     }
 
+    /// Advance the clock of the system by one tick, this will
+    /// usually imply executing one CPU instruction and advancing
+    /// all the other components of the system by the required
+    /// amount of cycles.
+    ///
+    /// This method takes into account the current speed of the
+    /// system (single or double) and will execute the required
+    /// amount of cycles in the other components of the system
+    /// accordingly.
+    ///
+    /// The amount of cycles executed by the CPU is returned.
     pub fn clock(&mut self) -> u16 {
         let cycles = self.cpu_clock() as u16;
         let cycles_n = cycles / self.multiplier() as u16;
@@ -1081,12 +1092,16 @@ impl GameBoy {
         Ok(())
     }
 
-    pub fn load_cartridge(&mut self, rom: Cartridge) -> &mut Cartridge {
+    pub fn load_cartridge(&mut self, rom: Cartridge) -> Result<&mut Cartridge, Error> {
         self.mmu().set_rom(rom);
-        self.mmu().rom()
+        Ok(self.mmu().rom())
     }
 
-    pub fn load_rom(&mut self, data: &[u8], ram_data: Option<&[u8]>) -> &mut Cartridge {
+    pub fn load_rom(
+        &mut self,
+        data: &[u8],
+        ram_data: Option<&[u8]>,
+    ) -> Result<&mut Cartridge, Error> {
         let mut rom = Cartridge::from_data(data);
         if let Some(ram_data) = ram_data {
             rom.set_ram_data(ram_data)
@@ -1094,11 +1109,15 @@ impl GameBoy {
         self.load_cartridge(rom)
     }
 
-    pub fn load_rom_file(&mut self, path: &str, ram_path: Option<&str>) -> &mut Cartridge {
-        let data = read_file(path).unwrap();
+    pub fn load_rom_file(
+        &mut self,
+        path: &str,
+        ram_path: Option<&str>,
+    ) -> Result<&mut Cartridge, Error> {
+        let data = read_file(path)?;
         match ram_path {
             Some(ram_path) => {
-                let ram_data = read_file(ram_path).unwrap();
+                let ram_data = read_file(ram_path)?;
                 self.load_rom(&data, Some(&ram_data))
             }
             None => self.load_rom(&data, None),
@@ -1184,12 +1203,12 @@ impl GameBoy {
         }));
     }
 
-    pub fn load_rom_wa(&mut self, data: &[u8]) -> Cartridge {
-        let rom = self.load_rom(data, None);
+    pub fn load_rom_wa(&mut self, data: &[u8]) -> Result<Cartridge, String> {
+        let rom = self.load_rom(data, None).map_err(|e| e.to_string())?;
         rom.set_rumble_cb(|active| {
             rumble_callback(active);
         });
-        rom.clone()
+        Ok(rom.clone())
     }
 
     pub fn load_callbacks_wa(&mut self) {
diff --git a/src/py.rs b/src/py.rs
index 2e948c70..a9fb17fb 100644
--- a/src/py.rs
+++ b/src/py.rs
@@ -46,12 +46,18 @@ impl GameBoy {
             .map_err(PyErr::new::<PyException, _>)
     }
 
-    pub fn load_rom(&mut self, data: &[u8]) {
-        self.system.load_rom(data, None);
+    pub fn load_rom(&mut self, data: &[u8]) -> PyResult<()> {
+        self.system
+            .load_rom(data, None)
+            .map(|_| ())
+            .map_err(PyErr::new::<PyException, _>)
     }
 
-    pub fn load_rom_file(&mut self, path: &str) {
-        self.system.load_rom_file(path, None);
+    pub fn load_rom_file(&mut self, path: &str) -> PyResult<()> {
+        self.system
+            .load_rom_file(path, None)
+            .map(|_| ())
+            .map_err(PyErr::new::<PyException, _>)
     }
 
     pub fn read_memory(&mut self, addr: u16) -> u8 {
diff --git a/src/python/boytacean/gb.py b/src/python/boytacean/gb.py
index ecf2292c..5e2b3e33 100644
--- a/src/python/boytacean/gb.py
+++ b/src/python/boytacean/gb.py
@@ -2,7 +2,11 @@ from enum import Enum
 from contextlib import contextmanager
 from typing import Any, Iterable, Union, cast
 
-from PIL.Image import Image, frombytes
+try:
+    from PIL.Image import Image, frombytes
+except ImportError:
+    Image = Any
+    frombytes = Any
 
 from .palettes import PALETTES
 from .video import VideoCapture
diff --git a/src/python/boytacean/test/__init__.py b/src/python/boytacean/test/__init__.py
new file mode 100644
index 00000000..e69de29b
diff --git a/src/python/boytacean/test/base.py b/src/python/boytacean/test/base.py
new file mode 100644
index 00000000..487f0230
--- /dev/null
+++ b/src/python/boytacean/test/base.py
@@ -0,0 +1,21 @@
+import unittest
+
+from os.path import dirname, realpath, join
+
+from boytacean import GameBoy
+
+
+class BaseTest(unittest.TestCase):
+
+    def test_pocket(self):
+        CURRENT_DIR = dirname(realpath(__file__))
+        ROM_PATH = join(CURRENT_DIR, "../../../../res/roms/demo/pocket.gb")
+        FRAME_COUNT = 600
+        LOAD_GRAPHICS = False
+
+        gb = GameBoy(
+            apu_enabled=False, serial_enabled=False, load_graphics=LOAD_GRAPHICS
+        )
+        gb.load_rom(ROM_PATH)
+        for _ in range(FRAME_COUNT):
+            gb.next_frame()
diff --git a/src/test.rs b/src/test.rs
index ae82c98b..6ebd856c 100644
--- a/src/test.rs
+++ b/src/test.rs
@@ -1,5 +1,6 @@
 use crate::{
     devices::buffer::BufferDevice,
+    error::Error,
     gb::{GameBoy, GameBoyMode},
     ppu::FRAME_BUFFER_SIZE,
 };
@@ -25,12 +26,16 @@ pub fn build_test(options: TestOptions) -> GameBoy {
     game_boy
 }
 
-pub fn run_test(rom_path: &str, max_cycles: Option<u64>, options: TestOptions) -> GameBoy {
+pub fn run_test(
+    rom_path: &str,
+    max_cycles: Option<u64>,
+    options: TestOptions,
+) -> Result<GameBoy, Error> {
     let mut cycles = 0u64;
     let max_cycles = max_cycles.unwrap_or(u64::MAX);
 
     let mut game_boy = build_test(options);
-    game_boy.load_rom_file(rom_path, None);
+    game_boy.load_rom_file(rom_path, None)?;
 
     loop {
         cycles += game_boy.clock() as u64;
@@ -39,28 +44,32 @@ pub fn run_test(rom_path: &str, max_cycles: Option<u64>, options: TestOptions) -
         }
     }
 
-    game_boy
+    Ok(game_boy)
 }
 
-pub fn run_step_test(rom_path: &str, addr: u16, options: TestOptions) -> GameBoy {
+pub fn run_step_test(rom_path: &str, addr: u16, options: TestOptions) -> Result<GameBoy, Error> {
     let mut game_boy = build_test(options);
-    game_boy.load_rom_file(rom_path, None);
+    game_boy.load_rom_file(rom_path, None)?;
     game_boy.step_to(addr);
-    game_boy
+    Ok(game_boy)
 }
 
-pub fn run_serial_test(rom_path: &str, max_cycles: Option<u64>, options: TestOptions) -> String {
-    let mut game_boy = run_test(rom_path, max_cycles, options);
-    game_boy.serial().device().state()
+pub fn run_serial_test(
+    rom_path: &str,
+    max_cycles: Option<u64>,
+    options: TestOptions,
+) -> Result<String, Error> {
+    let mut game_boy = run_test(rom_path, max_cycles, options)?;
+    Ok(game_boy.serial().device().state())
 }
 
 pub fn run_image_test(
     rom_path: &str,
     max_cycles: Option<u64>,
     options: TestOptions,
-) -> [u8; FRAME_BUFFER_SIZE] {
-    let mut game_boy = run_test(rom_path, max_cycles, options);
-    *game_boy.frame_buffer()
+) -> Result<[u8; FRAME_BUFFER_SIZE], Error> {
+    let mut game_boy = run_test(rom_path, max_cycles, options)?;
+    Ok(*game_boy.frame_buffer())
 }
 
 #[cfg(test)]
@@ -75,7 +84,9 @@ mod tests {
             "res/roms/test/blargg/cpu/cpu_instrs.gb",
             0x0100,
             TestOptions::default(),
-        );
+        )
+        .unwrap();
+
         assert_eq!(result.cpu_i().pc(), 0x0100);
         assert_eq!(result.cpu_i().sp(), 0xfffe);
         assert_eq!(result.cpu_i().af(), 0x01b0);
@@ -96,7 +107,8 @@ mod tests {
             "res/roms/test/blargg/cpu/cpu_instrs.gb",
             Some(300000000),
             TestOptions::default(),
-        );
+        )
+        .unwrap();
         assert_eq!(result, "cpu_instrs\n\n01:ok  02:ok  03:ok  04:ok  05:ok  06:ok  07:ok  08:ok  09:ok  10:ok  11:ok  \n\nPassed all tests\n");
     }
 
@@ -106,7 +118,8 @@ mod tests {
             "res/roms/test/blargg/instr_timing/instr_timing.gb",
             Some(50000000),
             TestOptions::default(),
-        );
+        )
+        .unwrap();
         assert_eq!(result, "instr_timing\n\n\nPassed\n");
     }
 }
-- 
GitLab