src/apu.rs (1)

950-960: Compatibility audio_buffer() copies — acceptable, but note the cost.

Returning an owned VecDeque provides a safe API but incurs O(n) copy. Callers on hot paths should prefer audio_buffer_iter() (or the eager path in GameBoy) to avoid allocations.

src/gb.rs (2)

425-429: Owned audio buffer in trait simplifies lifetimes but increases copies.

The API change to return VecDeque<i16> avoids borrow issues but forces allocations. If you want the best of both worlds, consider adding a secondary method (non-breaking) to the trait that exposes an iterator or slice pair for zero-copy consumers while retaining this owned fallback.

---

824-833: Avoid the intermediate Vec in audio_buffer_eager to cut one copy.

You can extend the cache directly from the iterator, eliminating the temporary samples Vec.

Apply this diff:

-    // collect samples first to avoid borrowing issues
-    let samples: Vec<i16> = self.apu().audio_buffer_iter().collect();
-    self.audio_buffer_cache.clear();
-    self.audio_buffer_cache.extend(samples);
+    // extend cache directly from iterator to avoid an extra allocation/copy
+    self.audio_buffer_cache.clear();
+    self.audio_buffer_cache.extend(self.apu().audio_buffer_iter());

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

src/mmu.rs (2)

394-396: Echo RAM read path ignores SVBK; 0xF000–0xFDFF must mirror selected WRAM bank (CGB).

Currently, the entire 0xE000–0xFDFF range is indexed with (addr & 0x1fff), which always targets bank 0. On CGB, 0xF000–0xFDFF must mirror the currently selected WRAM bank (SVBK), like 0xD000–0xDFFF. This breaks titles that use banked WRAM through the echo window.

Fix by splitting the range to mirror 0xC000/0xD000 behavior:

• 0xE000–0xEFFF → bank 0 (addr & 0x0fff)
• 0xF000–0xFDFF → selected bank (self.ram_offset + (addr & 0x0fff))

Also correct the comment which states “(4 KB)”; the echo region is 0x1E00 bytes mirrored.

Apply this diff:

-                // 0xE000-0xFDFF - Echo RAM (4 KB)
-                0xe000..=0xfdff => self.ram[(addr & 0x1fff) as usize],
+                // 0xE000-0xEFFF - Echo RAM (bank 0 mirror)
+                0xe000..=0xefff => self.ram[(addr & 0x0fff) as usize],
+                // 0xF000-0xFDFF - Echo RAM (selected bank mirror)
+                0xf000..=0xfdff => self.ram[(self.ram_offset + (addr & 0x0fff)) as usize],

---

530-532: Echo RAM write path has the same bank mapping bug; mirror selected bank for 0xF000–0xFDFF.

Writes to 0xF000–0xFDFF should target the currently selected WRAM bank (SVBK), not bank 0. Mirror the read-path fix for correctness.

Apply this diff:

-                // 0xE000-0xFDFF - Echo RAM (4 KB)
-                0xe000..=0xfdff => self.ram[(addr & 0x1fff) as usize] = value,
+                // 0xE000-0xEFFF - Echo RAM (bank 0 mirror)
+                0xe000..=0xefff => self.ram[(addr & 0x0fff) as usize] = value,
+                // 0xF000-0xFDFF - Echo RAM (selected bank mirror)
+                0xf000..=0xfdff => {
+                    self.ram[(self.ram_offset + (addr & 0x0fff)) as usize] = value
+                },

src/mmu.rs (5)

372-377: Doc wording nit: “switch statement” → “match expression.”

Rust uses match expressions; consider updating the comment to avoid confusion.

-    /// prediction hints and a switch statement to handle
+    /// prediction hints and a match expression to handle
-    /// the different memory regions.
+    /// the different memory regions.

---

475-477: KEY1 (0xFF4D) read: consider returning 0xFF on DMG for accuracy.

On DMG, KEY1 is unmapped/unused; many implementations return 0xFF. Optional, but improves compatibility with DMG-only titles/tests.

-            0xff4d => (if self.switching { 0x01 } else { 0x00 }) | ((self.speed as u8) << 7) | 0x7e,
+            0xff4d => {
+                if self.mode == GameBoyMode::Cgb {
+                    (if self.switching { 0x01 } else { 0x00 }) | ((self.speed as u8) << 7) | 0x7e
+                } else {
+                    0xff
+                }
+            }

---

600-602: KEY1 (0xFF4D) write: optionally ignore writes on DMG.

Writing KEY1 is CGB-only; on DMG it should be ignored. Not critical, but improves behavioral fidelity.

-            0xff4d => self.switching = value & 0x01 == 0x01,
+            0xff4d => {
+                if self.mode == GameBoyMode::Cgb {
+                    self.switching = value & 0x01 == 0x01;
+                }
+            }

---

658-667: Minor: read_many could take &self (not &mut self).

read_many only calls self.read(&self), so it doesn’t need &mut. Keeping it &self can reduce borrow restrictions in callers. Purely optional.

-    pub fn read_many(&mut self, addr: u16, count: u16) -> Vec<u8> {
+    pub fn read_many(&self, addr: u16, count: u16) -> Vec<u8> {

---

372-399: Add tests to lock the Echo RAM mirroring semantics.

Once the Echo fixes are applied, consider adding regression tests that assert:

• 0xE000–0xEFFF mirrors bank 0 (0xC000–0xCFFF)
• 0xF000–0xFDFF mirrors selected bank (0xD000–0xDFFF), honoring 0xFF70 (SVBK)

Example to add under the existing #[cfg(test)] mod tests:

#[test]
fn test_echo_ram_mirrors_bank0_and_selected_bank() {
    let mut mmu = Mmu::default();
    mmu.allocate_cgb();
    mmu.set_mode(GameBoyMode::Cgb);
    mmu.ppu.set_gb_mode(GameBoyMode::Cgb);

    // Bank 0 mirror: C000 <-> E000
    mmu.write(0xc456, 0x12);
    assert_eq!(mmu.read(0xe456), 0x12);
    mmu.write(0xe789, 0x34);
    assert_eq!(mmu.read(0xc789), 0x34);

    // Select bank 3 and test D000 <-> F000 mirror
    mmu.write(0xff70, 0x03); // SVBK = 3
    mmu.write(0xd123, 0xab);
    assert_eq!(mmu.read(0xf123), 0xab);
    mmu.write(0xf123, 0xcd);
    assert_eq!(mmu.read(0xd123), 0xcd);

    // Switch bank and ensure the echo mirrors the new bank
    mmu.write(0xff70, 0x02); // SVBK = 2
    mmu.write(0xd123, 0x55);
    assert_eq!(mmu.read(0xf123), 0x55);
}

Also applies to: 510-536

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled by default for public repositories
• Linear integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.