Merge pull request #99 from alloncm/fix/window_crashes

Fix/window crashes

Bug fixes including mem corrupt and panics

gb/src/sdl/sdl_pull_audio_device.rs

@@ -10,7 +10,7 @@ const BUFFERS_NUMBER:usize = 3;
 struct UserData{
     rx: Receiver<usize>,
     current_buf: Option<usize>,
-    current_buf_index:usize,
+    current_buf_byte_index:usize,
 }
 
 pub struct SdlPullAudioDevice<AR:AudioResampler>{
@@ -31,7 +31,7 @@ impl<AR:AudioResampler> ResampledAudioDevice<AR> for SdlPullAudioDevice<AR>{
         let(s,r) = bounded(BUFFERS_NUMBER - 1);
         let data = Box::new(UserData{
             current_buf:Option::None,
-            current_buf_index:0,
+            current_buf_byte_index:0,
             rx:r
         });
 
@@ -103,22 +103,21 @@ unsafe extern "C" fn audio_callback(userdata:*mut c_void, buffer:*mut u8, length
     }
 
     let samples = &*((safe_userdata.current_buf.unwrap()) as *const [Sample;BUFFER_SIZE]);
-    let samples_size = (samples.len() * std::mem::size_of::<Sample>()) - safe_userdata.current_buf_index;
-    let samples_ptr = (samples.as_ptr() as *mut u8).add(safe_userdata.current_buf_index);
+    let samples_size = (samples.len() * std::mem::size_of::<Sample>()) - safe_userdata.current_buf_byte_index;
+    let samples_ptr = (samples.as_ptr() as *mut u8).add(safe_userdata.current_buf_byte_index);
     std::ptr::copy_nonoverlapping(samples_ptr, buffer, std::cmp::min(length, samples_size));
 
     if length > samples_size && safe_userdata.rx.is_empty(){
         safe_userdata.current_buf = Option::None;
-        safe_userdata.current_buf_index = 0;
-        std::ptr::write_bytes(buffer.add(samples.len() as usize), 0, length  - samples_size);
+        safe_userdata.current_buf_byte_index = 0;
+        std::ptr::write_bytes(buffer.add(samples_size), 0, length  - samples_size);
     }
     else if length > samples_size{
         safe_userdata.current_buf = Option::None;
-        safe_userdata.current_buf_index = 0;
+        safe_userdata.current_buf_byte_index = 0;
         audio_callback(userdata, buffer.add(samples_size), (length - samples_size) as i32);
     }
     else{
-        safe_userdata.current_buf_index = length;
+        safe_userdata.current_buf_byte_index = length;
     }
-}
-
+}

lib_gb/src/mmu/carts/mbc1.rs

@@ -27,7 +27,7 @@ impl Mbc for Mbc1{
 
     fn read_current_bank(&self, address:u16)->u8{
         let bank:u16 = self.get_current_rom_bank() as u16;
-        return self.program[(ROM_BANK_SIZE * bank + address) as usize];
+        return self.program[ROM_BANK_SIZE as usize * bank as usize + address as usize];
     }
 
     fn write_rom(&mut self, address: u16, value: u8){
@@ -41,13 +41,18 @@ impl Mbc for Mbc1{
     }
 
     fn read_external_ram(&self, address: u16)->u8{
+        if self.ram.is_empty(){
+            return 0xFF;
+        }
         let bank:u16 = self.get_current_ram_bank() as u16;
         return self.ram[(bank * RAM_BANK_SIZE + address) as usize];
     }
 
     fn write_external_ram(&mut self, address: u16, value: u8){
-        let bank:u16 = self.get_current_ram_bank() as u16;
-        self.ram[(bank * RAM_BANK_SIZE + address) as usize] = value;
+        if self.ram.len() > 0{
+            let bank:u16 = self.get_current_ram_bank() as u16;
+            self.ram[(bank * RAM_BANK_SIZE + address) as usize] = value;   
+        }
     }
 }
 

lib_gb/src/ppu/fifo/background_fetcher.rs

@@ -45,14 +45,7 @@ impl BackgroundFetcher{
 
     pub fn fetch_pixels(&mut self, vram:&VRam, lcd_control:u8, ly_register:u8, window_pos:&Vec2<u8>, bg_pos:&Vec2<u8>){
         self.has_wy_reached_ly = self.has_wy_reached_ly || ly_register == window_pos.y;
-        let last_rendering_status = self.rendering_window;
         self.rendering_window = self.is_rendering_wnd(lcd_control, window_pos);
-
-        // In case I was rendering a background pixel need to reset the state of the fetcher 
-        // (and maybe clear the fifo but right now Im not doing it since im not sure what about the current_x_pos var)
-        if self.rendering_window && !last_rendering_status{
-            self.fetcher_state_machine.reset();
-        }
 
         match self.fetcher_state_machine.current_state(){
             FetchingState::FetchTileNumber=>{
@@ -88,20 +81,28 @@ impl BackgroundFetcher{
             FetchingState::Push if self.fifo.len() == 0 => {
                 if lcd_control & BIT_0_MASK == 0{
                     self.fifo.fill(&EMPTY_FIFO_BUFFER);
+                    self.current_x_pos += SPRITE_WIDTH;
                 }
                 else{
                     let low_data = self.fetcher_state_machine.data.low_tile_data;
                     let high_data = self.fetcher_state_machine.data.high_tile_data;
-                    let mut buffer:[u8;SPRITE_WIDTH as usize] = [0;SPRITE_WIDTH as usize];
-                    for i in 0..buffer.len(){
+                    for i in (0..FIFO_SIZE).rev(){
                         let mask = 1 << i;
                         let mut pixel = (low_data & mask) >> i;
                         pixel |= ((high_data & mask) >> i) << 1;
-                        buffer[(buffer.len() - 1 - i) as usize] = pixel;
+                        self.fifo.push(pixel);
+                        self.current_x_pos += 1;
+
+                        let last_rendering_status = self.rendering_window;
+                        self.rendering_window = self.is_rendering_wnd(lcd_control, window_pos);            
+                        // In case I was rendering a background pixel need to reset the state of the fetcher 
+                        // (and maybe clear the fifo but right now Im not doing it since im not sure what about the current_x_pos var)
+                        if self.rendering_window && !last_rendering_status{
+                            self.fetcher_state_machine.reset();
+                            return;
+                        }
                     }
-                    self.fifo.fill(&buffer);
                 }
-                self.current_x_pos += SPRITE_WIDTH;
             }
             _ => {}
         }

lib_gb/src/ppu/fifo/sprite_fetcher.rs

@@ -71,11 +71,11 @@ impl SpriteFetcher{
                 if oam_attribute.flip_x{
                     for i in (0 + skip_x)..SPRITE_WIDTH as usize{
                         let pixel = Self::get_decoded_pixel(i, low_data, high_data);
-                        if i + skip_x >= start_x {
+                        if i >= start_x {
                             self.fifo.push((pixel, self.current_oam_entry));
                         }
-                        else if self.fifo[i + skip_x].0 == 0{
-                            self.fifo[i+ skip_x] = (pixel, self.current_oam_entry);
+                        else if self.fifo[i].0 == 0{
+                            self.fifo[i] = (pixel, self.current_oam_entry);
                         }
                     }
                 }

lib_gb/src/ppu/gb_ppu.rs

@@ -159,7 +159,7 @@ impl<GFX:GfxDevice> GbPpu<GFX>{
             (HBLANK_M_CYCLES_LENGTH as u32 * 154 ) - self.m_cycles_passed as u32
         }
         else{
-            ((self.lyc_register - self.ly_register) as u32 * HBLANK_M_CYCLES_LENGTH as u32) - self.m_cycles_passed as u32
+            ((self.lyc_register - self.ly_register) as u32 * HBLANK_M_CYCLES_LENGTH as u32) - (self.m_cycles_passed as u32 % HBLANK_M_CYCLES_LENGTH as u32)
         };
 
         return t_cycles_to_next_stat_change;

lib_gb/src/utils/fixed_size_queue.rs

@@ -3,38 +3,38 @@ pub struct FixedSizeQueue<T:Copy, const SIZE:usize>{
     // Im modifing it but not increasing its allocated size once its allocated so I hope this will work for me
     // and I wont get weird memory issues
     _data: Vec<T>,   // This field is not use directly only through pointers aquired in the new() function
-    end_data_pointer: *const T,
-    start_data_pointer: *const T,
+    end_alloc_pointer: *const T,
+    start_alloc_pointer: *const T,
     data_pointer: *mut T,
     base_data_pointer: *mut T,
     length: usize,
 }
 
-impl<T:Copy, const SIZE:usize> FixedSizeQueue<T, SIZE>{
+impl<T:Copy + Default, const SIZE:usize> FixedSizeQueue<T, SIZE>{
     pub fn new()->Self{
         let data = Vec::with_capacity(SIZE);
         let mut s = Self{
             _data: data,
             length:0,
             base_data_pointer: std::ptr::null_mut(),
             data_pointer: std::ptr::null_mut(),
-            end_data_pointer: std::ptr::null_mut(),
-            start_data_pointer: std::ptr::null_mut(),
+            end_alloc_pointer: std::ptr::null_mut(),
+            start_alloc_pointer: std::ptr::null_mut(),
         };
 
         s.base_data_pointer = s._data.as_mut_ptr();
         s.data_pointer = s._data.as_mut_ptr();
-        s.start_data_pointer = s._data.as_ptr();
-        unsafe{s.end_data_pointer = s._data.as_ptr().add(SIZE)};
+        s.start_alloc_pointer = s._data.as_ptr();
+        unsafe{s.end_alloc_pointer = s._data.as_ptr().add(SIZE)};
 
         return s;
     }
 
     pub fn push(&mut self, t:T){
         if self.length < SIZE{
             unsafe{
-                if self.data_pointer == self.end_data_pointer as *mut T{
-                    self.data_pointer = self.start_data_pointer as *mut T;
+                if self.data_pointer == self.end_alloc_pointer as *mut T{
+                    self.data_pointer = self.start_alloc_pointer as *mut T;
                 }
                 *self.data_pointer = t;
                 self.data_pointer = self.data_pointer.add(1);
@@ -51,8 +51,8 @@ impl<T:Copy, const SIZE:usize> FixedSizeQueue<T, SIZE>{
             unsafe{
                 let t = *self.base_data_pointer;
                 self.base_data_pointer = self.base_data_pointer.add(1);
-                if self.base_data_pointer == self.end_data_pointer as *mut T{
-                    self.base_data_pointer = self.start_data_pointer as *mut T;
+                if self.base_data_pointer == self.end_alloc_pointer as *mut T{
+                    self.base_data_pointer = self.start_alloc_pointer as *mut T;
                 }
 
                 self.length -= 1;
@@ -65,8 +65,8 @@ impl<T:Copy, const SIZE:usize> FixedSizeQueue<T, SIZE>{
 
     pub fn clear(&mut self){
         self.length = 0;
-        self.data_pointer = self.start_data_pointer as *mut T;
-        self.base_data_pointer = self.start_data_pointer as *mut T;
+        self.data_pointer = self.start_alloc_pointer as *mut T;
+        self.base_data_pointer = self.start_alloc_pointer as *mut T;
     }
 
     pub fn len(&self)->usize{
@@ -75,43 +75,42 @@ impl<T:Copy, const SIZE:usize> FixedSizeQueue<T, SIZE>{
 
     pub fn fill(&mut self, value:&[T;SIZE]){
         unsafe{
+            self.base_data_pointer = self.start_alloc_pointer as *mut T;
             std::ptr::copy_nonoverlapping(value.as_ptr(), self.base_data_pointer, SIZE);
             self.length = SIZE;
-            self.data_pointer = self.end_data_pointer.sub(1) as *mut T;
+            self.data_pointer = self.end_alloc_pointer as *mut T;
         }
     }
 }
 
-impl<T:Copy, const SIZE:usize> std::ops::Index<usize> for FixedSizeQueue<T, SIZE>{
-    type Output = T;
-
-    fn index(&self, mut index: usize) -> &Self::Output {
+impl<T:Copy, const SIZE:usize> FixedSizeQueue<T, SIZE>{
+    #[inline] 
+    fn get_index_ptr(&self, index:usize)->*const T{
         if index < self.length{
             unsafe{
-                if self.base_data_pointer.add(index) >= self.end_data_pointer as *mut T{
-                    index -= self.end_data_pointer.offset_from(self.base_data_pointer) as usize;
+                if self.base_data_pointer.add(index) >= self.end_alloc_pointer as *mut T{
+                    let wrap_offset = self.end_alloc_pointer.offset_from(self.base_data_pointer) as usize;
+                    return self.start_alloc_pointer.add(index - wrap_offset);
+                }
+                else{
+                    return self.base_data_pointer.add(index);
                 }
-                // casting a *mut T to a &T
-                return &*(self.base_data_pointer.add(index));
             }
         }
-
         std::panic!("Index is out of range");
     }
 }
 
-impl<T:Copy, const SIZE:usize> std::ops::IndexMut<usize> for FixedSizeQueue<T, SIZE>{
-    fn index_mut(&mut self, mut index: usize) -> &mut Self::Output {
-        if index < self.length{
-            unsafe{
-                if self.base_data_pointer.add(index) >= self.end_data_pointer as *mut T{
-                    index -= self.end_data_pointer.offset_from(self.base_data_pointer) as usize;
-                }
-                // casting a *mut T to a &mut T
-                return &mut *(self.base_data_pointer.add(index));
-            }
-        }
+impl<T:Copy, const SIZE:usize> std::ops::Index<usize> for FixedSizeQueue<T, SIZE>{
+    type Output = T;
 
-        std::panic!("Index is out of range");
+    fn index(&self, index: usize) -> &Self::Output {
+        unsafe{&*(self.get_index_ptr(index))}
+    }
+}
+
+impl<T:Copy, const SIZE:usize> std::ops::IndexMut<usize> for FixedSizeQueue<T, SIZE>{
+    fn index_mut(&mut self, index: usize) -> &mut Self::Output {
+        unsafe{&mut *(self.get_index_ptr(index) as *mut T)}
     }
 }

lib_gb/tests/fixed_size_queue_tests.rs

@@ -84,4 +84,43 @@ fn panic_on_fifo_set_index_out_of_range(){
 
     //should panic
     fifo[2] = 4;
+}
+
+#[test]
+fn fill_fills_the_fifo(){
+    let mut fifo = FixedSizeQueue::<u8, 8>::new();
+    fifo.push(1);
+    fifo.push(1);
+    fifo.push(1);
+
+    fifo.remove();
+    fifo.remove();
+
+    fifo.fill(&[0;8]);
+
+    assert_eq!(fifo.len(), 8);
+    for i in 0..8{
+        assert_eq!(fifo[i], 0);
+    }
+}
+
+#[test]
+fn fifo_index_check_happyflow(){
+    let mut fifo = FixedSizeQueue::<u8, 8>::new();
+    for i in 0..8{
+        fifo.push(i);
+    }
+    for _ in 0..6{
+        fifo.remove();
+    }
+    for i in 0..6{
+        fifo.push(i);
+    }
+
+    assert_eq!(fifo[0], 6);
+    assert_eq!(fifo[1], 7);
+    assert_eq!(fifo[2], 0);
+    assert_eq!(fifo[3], 1);
+    assert_eq!(fifo[4], 2);
+    assert_eq!(fifo[5], 3);
 }