Remove option to manage your own interrupts in the mixer (#361)

Simplify the audio mixer to always use the interrupt system which fixes
a soundness issue (pun intended) if you forget the mixer but not the
interrupt handler.

- [x] Changelog updated / no changelog update needed

CHANGELOG.md

@@ -11,6 +11,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Changed
 - Text renderer can now be re-used which is useful for rpg style character/word at a time text boxes.
+- Audio now automatically uses interrupts, so you can remove the `setup_interrupt_handler` or `after_vblank` calls to the mixer.
 - If a vblank happens outside of `wait_for_vblank`, then next call will immediately return.
 
 ### Fixed

agb/Cargo.toml

@@ -12,7 +12,8 @@ opt-level = 3
 debug = true
 
 [profile.release]
-lto = true
+opt-level = "s"
+lto = "thin"
 debug = true
 
 [features]

agb/examples/mixer_32768.rs

@@ -39,11 +39,12 @@ fn main(mut gba: Gba) -> ! {
 
     let timer_controller = gba.timers.timers();
     let mut timer = timer_controller.timer2;
+    let mut timer2 = timer_controller.timer3;
     timer.set_enabled(true);
+    timer2.set_cascade(true).set_enabled(true);
 
     let mut mixer = gba.mixer.mixer(Frequency::Hz32768);
     mixer.enable();
-    let _interrupt = mixer.setup_interrupt_handler();
 
     let mut channel = SoundChannel::new(CRAZY_GLUE);
     channel.stereo();
@@ -57,14 +58,22 @@ fn main(mut gba: Gba) -> ! {
         vblank_provider.wait_for_vblank();
         bg.commit(&mut vram);
 
-        let before_mixing_cycles = timer.value();
+        let before_mixing_cycles_high = timer2.value();
+        let before_mixing_cycles_low = timer.value();
+
         mixer.frame();
-        let after_mixing_cycles = timer.value();
+
+        let after_mixing_cycles_low = timer.value();
+        let after_mixing_cycles_high = timer2.value();
 
         frame_counter = frame_counter.wrapping_add(1);
 
         if frame_counter % 128 == 0 && !has_written_frame_time {
-            let total_cycles = after_mixing_cycles.wrapping_sub(before_mixing_cycles) as u32;
+            let before_mixing_cycles =
+                ((before_mixing_cycles_high as u32) << 16) + before_mixing_cycles_low as u32;
+            let after_mixing_cycles =
+                ((after_mixing_cycles_high as u32) << 16) + after_mixing_cycles_low as u32;
+            let total_cycles = after_mixing_cycles.wrapping_sub(before_mixing_cycles);
 
             let percent = (total_cycles * 100) / 280896;
 

agb/examples/mixer_basic.rs

@@ -51,6 +51,5 @@ fn main(mut gba: Gba) -> ! {
 
         mixer.frame();
         vblank_provider.wait_for_vblank();
-        mixer.after_vblank();
     }
 }

agb/examples/stereo_sound.rs

@@ -32,7 +32,6 @@ fn main(mut gba: Gba) -> ! {
 
     writeln!(&mut writer, "Let it in by Josh Woodward").unwrap();
 
-
     writer.commit();
 
     bg.commit(&mut vram);
@@ -58,7 +57,6 @@ fn main(mut gba: Gba) -> ! {
         bg.commit(&mut vram);
 
         let before_mixing_cycles = timer.value();
-        mixer.after_vblank();
         mixer.frame();
         let after_mixing_cycles = timer.value();
 

agb/src/sound/mixer/hw.rs

@@ -46,23 +46,23 @@ pub(super) enum LeftOrRight {
     Right,
 }
 
-pub(super) fn enable_dma_for_sound(sound_memory: &[i8], lr: LeftOrRight) {
+pub(super) fn enable_dma_for_sound(sound_memory: *const i8, lr: LeftOrRight) {
     match lr {
         LeftOrRight::Left => enable_dma1_for_sound(sound_memory),
         LeftOrRight::Right => enable_dma2_for_sound(sound_memory),
     }
 }
 
-fn enable_dma1_for_sound(sound_memory: &[i8]) {
+fn enable_dma1_for_sound(sound_memory: *const i8) {
     DMA1_CONTROL.set(0);
-    DMA1_SOURCE_ADDR.set(sound_memory.as_ptr() as u32);
+    DMA1_SOURCE_ADDR.set(sound_memory as u32);
     DMA1_DEST_ADDR.set(FIFO_A_DEST_ADDR);
     DMA1_CONTROL.set(DMA_CONTROL_SETTING_FOR_SOUND);
 }
 
-fn enable_dma2_for_sound(sound_memory: &[i8]) {
+fn enable_dma2_for_sound(sound_memory: *const i8) {
     DMA2_CONTROL.set(0);
-    DMA2_SOURCE_ADDR.set(sound_memory.as_ptr() as u32);
+    DMA2_SOURCE_ADDR.set(sound_memory as u32);
     DMA2_DEST_ADDR.set(FIFO_B_DEST_ADDR);
     DMA2_CONTROL.set(DMA_CONTROL_SETTING_FOR_SOUND);
 }

agb/src/sound/mixer/mod.rs

@@ -45,11 +45,13 @@
 //!
 //! ## Doing the per-frame work
 //!
-//! Then, you have a choice of whether you want to use interrupts or do the buffer swapping
+//! Despite being high performance, the mixer still takes a sizable portion of CPU time (6-10%
-//! yourself after a vblank interrupt. If you are using 32768Hz as the frequency of your
+//! depending on number of channels and frequency) to do the per-frame tasks, so should be done
-//! files, you _must_ use the interrupt version.
+//! towards the end of the frame time (just before waiting for vblank) in order to give as much
+//! time during vblank as possible for rendering related tasks.
 //!
-//! Without interrupts:
+//! In order to avoid skipping audio, call the [`Mixer::frame()`] function at least once per frame
+//! as shown below:
 //!
 //! ```rust,no_run
 //! # #![no_std]
@@ -61,35 +63,9 @@
 //! // Somewhere in your main loop:
 //! mixer.frame();
 //! vblank.wait_for_vblank();
-//! mixer.after_vblank();
 //! # }
 //! ```
 //!
-//! Or with interrupts:
-//!
-//! ```rust,no_run
-//! # #![no_std]
-//! # #![no_main]
-//! use agb::sound::mixer::Frequency;
-//! # fn foo(gba: &mut agb::Gba) {
-//! let mut mixer = gba.mixer.mixer(agb::sound::mixer::Frequency::Hz32768);
-//! let vblank = agb::interrupt::VBlank::get();
-//! // outside your main loop, close to initialisation
-//! // you must assign this to a variable (not to _ or ignored) or rust will immediately drop it
-//! // and prevent the interrupt handler from firing.
-//! let _mixer_interrupt = mixer.setup_interrupt_handler();
-//!
-//! // inside your main loop
-//! mixer.frame();
-//! vblank.wait_for_vblank();
-//! # }
-//! ```
-//!
-//! Despite being high performance, the mixer still takes a sizable portion of CPU time (6-10%
-//! depending on number of channels and frequency) to do the per-frame tasks, so should be done
-//! towards the end of the frame time (just before waiting for vblank) in order to give as much
-//! time during vblank as possible for rendering related tasks.
-//!
 //! ## Loading a sample
 //!
 //! To load a sample, you must have it in `wav` format (both stereo and mono work) at exactly the
@@ -159,7 +135,7 @@ pub enum Frequency {
     Hz10512,
     /// 18157Hz
     Hz18157,
-    /// 32768Hz - note that this option requires interrupts for buffer swapping
+    /// 32768Hz
     Hz32768,
 }
 

agb/src/sound/mixer/sw_mixer.rs

@@ -1,4 +1,5 @@
 use core::cell::RefCell;
+use core::pin::Pin;
 
 use alloc::boxed::Box;
 use alloc::vec::Vec;
@@ -74,17 +75,22 @@ extern "C" {
 /// loop {
 ///    mixer.frame();
 ///    vblank.wait_for_vblank();
-///    mixer.after_vblank();   
 /// }
 /// # }
 /// ```
 pub struct Mixer {
-    buffer: MixerBuffer,
+    interrupt_timer: Timer,
+    // SAFETY: Has to go before buffer because it holds a reference to it
+    _interrupt_handler: InterruptHandler<'static>,
+
+    buffer: Pin<Box<MixerBuffer, InternalAllocator>>,
     channels: [Option<SoundChannel>; 8],
     indices: [i32; 8],
     frequency: Frequency,
 
-    timer: Timer,
+    working_buffer: Box<[Num<i16, 4>], InternalAllocator>,
+
+    fifo_timer: Timer,
 }
 
 /// A pointer to a currently playing channel.
@@ -112,13 +118,46 @@ pub struct ChannelId(usize, i32);
 
 impl Mixer {
     pub(super) fn new(frequency: Frequency) -> Self {
+        let buffer = Box::pin_in(MixerBuffer::new(frequency), InternalAllocator);
+
+        // SAFETY: you can only ever have 1 Mixer at a time
+        let fifo_timer = unsafe { Timer::new(0) };
+
+        // SAFETY: you can only ever have 1 Mixer at a time
+        let mut interrupt_timer = unsafe { Timer::new(1) };
+        interrupt_timer
+            .set_cascade(true)
+            .set_divider(Divider::Divider1)
+            .set_interrupt(true)
+            .set_overflow_amount(frequency.buffer_size() as u16);
+
+        let buffer_pointer_for_interrupt_handler: &MixerBuffer = &buffer;
+
+        // SAFETY: dropping the lifetime, sound because interrupt handler dropped before the buffer is
+        //         In the case of the mixer being forgotten, both stay alive so okay
+        let buffer_pointer_for_interrupt_handler: &MixerBuffer =
+            unsafe { core::mem::transmute(buffer_pointer_for_interrupt_handler) };
+        let interrupt_handler = add_interrupt_handler(interrupt_timer.interrupt(), |cs| {
+            buffer_pointer_for_interrupt_handler.swap(cs);
+        });
+
+        set_asm_buffer_size(frequency);
+
+        let mut working_buffer =
+            Vec::with_capacity_in(frequency.buffer_size() * 2, InternalAllocator);
+        working_buffer.resize(frequency.buffer_size() * 2, 0.into());
+
         Self {
             frequency,
-            buffer: MixerBuffer::new(frequency),
+            buffer,
             channels: Default::default(),
             indices: Default::default(),
 
-            timer: unsafe { Timer::new(0) },
+            interrupt_timer,
+            _interrupt_handler: interrupt_handler,
+
+            working_buffer: working_buffer.into_boxed_slice(),
+            fifo_timer,
         }
     }
 
@@ -127,71 +166,13 @@ impl Mixer {
     /// You must call this method in order to start playing sound. You can do as much set up before
     /// this as you like, but you will not get any sound out of the console until this method is called.
     pub fn enable(&mut self) {
-        hw::set_timer_counter_for_frequency_and_enable(&mut self.timer, self.frequency.frequency());
+        hw::set_timer_counter_for_frequency_and_enable(
+            &mut self.fifo_timer,
+            self.frequency.frequency(),
+        );
         hw::set_sound_control_register_for_mixer();
-    }
 
-    /// Do post-vblank work. You can use either this or [`setup_interrupt_handler()`](Mixer::setup_interrupt_handler),
+        self.interrupt_timer.set_enabled(true);
-    /// but not both. Note that this is not available if using 32768Hz sounds since those require more irregular timings.
-    ///
-    /// # Example
-    ///
-    /// ```rust,no_run
-    /// # #![no_std]
-    /// # #![no_main]
-    /// # use agb::sound::mixer::*;
-    /// # use agb::*;
-    /// # fn foo(gba: &mut Gba) {
-    /// # let mut mixer = gba.mixer.mixer(agb::sound::mixer::Frequency::Hz10512);
-    /// # let vblank = agb::interrupt::VBlank::get();
-    /// loop {
-    ///     mixer.frame();
-    ///     vblank.wait_for_vblank();
-    ///     mixer.after_vblank();   
-    /// }
-    /// # }
-    /// ```
-    #[cfg(not(feature = "freq32768"))]
-    pub fn after_vblank(&mut self) {
-        free(|cs| self.buffer.swap(cs));
-    }
-
-    /// Use timer interrupts to do the timing required for ensuring the music runs smoothly.
-    ///
-    /// Note that if you set up an interrupt handler, you should not call [`after_vblank`](Mixer::after_vblank) any more
-    /// You are still required to call [`frame`](Mixer::frame).
-    ///
-    /// This is required if using 32768Hz music, but optional for other frequencies.
-    ///
-    /// # Example
-    ///
-    /// ```rust,no_run
-    /// # #![no_std]
-    /// # #![no_main]
-    /// # use agb::sound::mixer::*;
-    /// # use agb::*;
-    /// # fn foo(gba: &mut Gba) {
-    /// # let mut mixer = gba.mixer.mixer(agb::sound::mixer::Frequency::Hz10512);
-    /// # let vblank = agb::interrupt::VBlank::get();
-    /// // you must set this to a named variable to ensure that the scope is long enough
-    /// let _mixer_interrupt = mixer.setup_interrupt_handler();
-    ///
-    /// loop {
-    ///    mixer.frame();
-    ///    vblank.wait_for_vblank();
-    /// }
-    /// # }
-    /// ```
-    pub fn setup_interrupt_handler(&self) -> InterruptHandler<'_> {
-        let mut timer1 = unsafe { Timer::new(1) };
-        timer1
-            .set_cascade(true)
-            .set_divider(Divider::Divider1)
-            .set_interrupt(true)
-            .set_overflow_amount(self.frequency.buffer_size() as u16)
-            .set_enabled(true);
-
-        add_interrupt_handler(timer1.interrupt(), move |cs| self.buffer.swap(cs))
     }
 
     /// Do the CPU intensive mixing for the next frame's worth of data.
@@ -214,7 +195,6 @@ impl Mixer {
     /// loop {
     ///     mixer.frame();
     ///     vblank.wait_for_vblank();
-    ///     mixer.after_vblank(); // optional, only if not using interrupts
     /// }
     /// # }
     /// ```
@@ -224,7 +204,7 @@ impl Mixer {
         }
 
         self.buffer
-            .write_channels(self.channels.iter_mut().flatten());
+            .write_channels(&mut self.working_buffer, self.channels.iter_mut().flatten());
     }
 
     /// Start playing a given [`SoundChannel`].
@@ -340,8 +320,6 @@ impl SoundBuffer {
 }
 
 struct MixerBuffer {
-    buffers: [SoundBuffer; 3],
-    working_buffer: Box<[Num<i16, 4>], InternalAllocator>,
     frequency: Frequency,
 
     state: Mutex<RefCell<MixerBufferState>>,
@@ -350,6 +328,7 @@ struct MixerBuffer {
 struct MixerBufferState {
     active_buffer: usize,
     playing_buffer: usize,
+    buffers: [SoundBuffer; 3],
 }
 
 /// Only returns a valid result if 0 <= x <= 3
@@ -365,38 +344,31 @@ const fn mod3_estimate(x: usize) -> usize {
 
 impl MixerBufferState {
     fn should_calculate(&self) -> bool {
-        mod3_estimate(self.active_buffer + 1) != mod3_estimate(self.playing_buffer)
+        mod3_estimate(self.active_buffer + 1) != self.playing_buffer
     }
 
-    fn playing_advanced(&mut self) -> usize {
+    fn playing_advanced(&mut self) -> *const i8 {
         self.playing_buffer = mod3_estimate(self.playing_buffer + 1);
-        self.playing_buffer
+        self.buffers[self.playing_buffer].0.as_ptr()
     }
 
-    fn active_advanced(&mut self) -> usize {
+    fn active_advanced(&mut self) -> *mut i8 {
         self.active_buffer = mod3_estimate(self.active_buffer + 1);
-        self.active_buffer
+        self.buffers[self.active_buffer].0.as_mut_ptr()
     }
 }
 
 impl MixerBuffer {
     fn new(frequency: Frequency) -> Self {
-        let mut working_buffer =
-            Vec::with_capacity_in(frequency.buffer_size() * 2, InternalAllocator);
-        working_buffer.resize(frequency.buffer_size() * 2, 0.into());
-
         MixerBuffer {
+            state: Mutex::new(RefCell::new(MixerBufferState {
+                active_buffer: 0,
+                playing_buffer: 0,
                 buffers: [
                     SoundBuffer::new(frequency),
                     SoundBuffer::new(frequency),
                     SoundBuffer::new(frequency),
                 ],
-
-            working_buffer: working_buffer.into_boxed_slice(),
-
-            state: Mutex::new(RefCell::new(MixerBufferState {
-                active_buffer: 0,
-                playing_buffer: 0,
             })),
 
             frequency,
@@ -410,18 +382,20 @@ impl MixerBuffer {
     fn swap(&self, cs: CriticalSection) {
         let buffer = self.state.borrow(cs).borrow_mut().playing_advanced();
 
-        let (left_buffer, right_buffer) = self.buffers[buffer]
+        let left_buffer = buffer;
-            .0
+        // SAFETY: starting pointer is fine, resulting pointer also fine because buffer has length buffer_size() * 2 by construction
-            .split_at(self.frequency.buffer_size());
+        let right_buffer = unsafe { buffer.add(self.frequency.buffer_size()) };
 
         hw::enable_dma_for_sound(left_buffer, LeftOrRight::Left);
         hw::enable_dma_for_sound(right_buffer, LeftOrRight::Right);
     }
 
-    fn write_channels<'a>(&mut self, channels: impl Iterator<Item = &'a mut SoundChannel>) {
+    fn write_channels<'a>(
-        set_asm_buffer_size(self.frequency);
+        &self,
-
+        working_buffer: &mut [Num<i16, 4>],
-        self.working_buffer.fill(0.into());
+        channels: impl Iterator<Item = &'a mut SoundChannel>,
+    ) {
+        working_buffer.fill(0.into());
 
         for channel in channels {
             if channel.is_done {
@@ -451,7 +425,7 @@ impl MixerBuffer {
                     unsafe {
                         agb_rs__mixer_add_stereo(
                             channel.data.as_ptr().add(channel.pos.floor()),
-                            self.working_buffer.as_mut_ptr(),
+                            working_buffer.as_mut_ptr(),
                             channel.volume,
                         );
                     }
@@ -462,7 +436,7 @@ impl MixerBuffer {
                     unsafe {
                         agb_rs__mixer_add(
                             channel.data.as_ptr().add(channel.pos.floor()),
-                            self.working_buffer.as_mut_ptr(),
+                            working_buffer.as_mut_ptr(),
                             playback_speed,
                             left_amount,
                             right_amount,
@@ -474,12 +448,10 @@ impl MixerBuffer {
             channel.pos += playback_speed * self.frequency.buffer_size();
         }
 
-        let write_buffer_index = free(|cs| self.state.borrow(cs).borrow_mut().active_advanced());
+        let write_buffer = free(|cs| self.state.borrow(cs).borrow_mut().active_advanced());
-
-        let write_buffer = &mut self.buffers[write_buffer_index].0;
 
         unsafe {
-            agb_rs__mixer_collapse(write_buffer.as_mut_ptr(), self.working_buffer.as_ptr());
+            agb_rs__mixer_collapse(write_buffer, working_buffer.as_ptr());
         }
     }
 }

examples/hyperspace-roll/src/lib.rs

@@ -142,7 +142,6 @@ pub fn main(mut gba: agb::Gba) -> ! {
 
     let mut mixer = gba.mixer.mixer(Frequency::Hz32768);
     mixer.enable();
-    let _interrupt_handler = mixer.setup_interrupt_handler();
 
     let sfx = Sfx::new(&mut mixer);
 

examples/the-hat-chooses-the-wizard/src/lib.rs

@@ -832,7 +832,6 @@ pub fn main(mut agb: agb::Gba) -> ! {
             music_box.before_frame(&mut mixer);
             mixer.frame();
             vblank.wait_for_vblank();
-            mixer.after_vblank();
 
             level_display::write_level(
                 &mut world_display,
@@ -848,7 +847,6 @@ pub fn main(mut agb: agb::Gba) -> ! {
             music_box.before_frame(&mut mixer);
             mixer.frame();
             vblank.wait_for_vblank();
-            mixer.after_vblank();
 
             let map_current_level = current_level;
             let mut background = InfiniteScrolledMap::new(
@@ -894,21 +892,18 @@ pub fn main(mut agb: agb::Gba) -> ! {
                 music_box.before_frame(&mut mixer);
                 mixer.frame();
                 vblank.wait_for_vblank();
-                mixer.after_vblank();
             }
 
             while level.background.init_foreground(&mut vram) != PartialUpdateStatus::Done {
                 music_box.before_frame(&mut mixer);
                 mixer.frame();
                 vblank.wait_for_vblank();
-                mixer.after_vblank();
             }
 
             for _ in 0..20 {
                 music_box.before_frame(&mut mixer);
                 mixer.frame();
                 vblank.wait_for_vblank();
-                mixer.after_vblank();
             }
 
             object.commit();
@@ -930,7 +925,6 @@ pub fn main(mut agb: agb::Gba) -> ! {
                             music_box.before_frame(&mut mixer);
                             mixer.frame();
                             vblank.wait_for_vblank();
-                            mixer.after_vblank();
                             object.commit();
                         }
                         break;
@@ -944,7 +938,6 @@ pub fn main(mut agb: agb::Gba) -> ! {
                 music_box.before_frame(&mut mixer);
                 mixer.frame();
                 vblank.wait_for_vblank();
-                mixer.after_vblank();
                 object.commit();
             }
 

examples/the-hat-chooses-the-wizard/src/splash_screen.rs

@@ -41,10 +41,6 @@ pub fn show_splash_screen(
 
     vblank.wait_for_vblank();
 
-    if let Some(ref mut mixer) = mixer {
-        mixer.after_vblank();
-    }
-
     for y in 0..20u16 {
         for x in 0..30u16 {
             map.set_tile(
@@ -63,10 +59,6 @@ pub fn show_splash_screen(
         }
 
         vblank.wait_for_vblank();
-
-        if let Some(ref mut mixer) = mixer {
-            mixer.after_vblank();
-        }
     }
 
     map.commit(vram);
@@ -83,17 +75,13 @@ pub fn show_splash_screen(
         ) {
             break;
         }
-        if let Some(ref mut mixer) = mixer {
+        if let Some(mixer) = &mut mixer {
-            if let Some(ref mut music_box) = music_box {
+            if let Some(music_box) = &mut music_box {
                 music_box.before_frame(mixer);
             }
             mixer.frame();
         }
         vblank.wait_for_vblank();
-
-        if let Some(ref mut mixer) = mixer {
-            mixer.after_vblank();
-        }
     }
 
     map.hide();

examples/the-purple-night/src/lib.rs

@@ -85,7 +85,6 @@ impl<'a> Level<'a> {
         let mut between_updates = || {
             sfx.frame();
             vblank.wait_for_vblank();
-            sfx.after_vblank();
         };
 
         backdrop.init(vram, start_pos, &mut between_updates);
@@ -2283,7 +2282,6 @@ fn game_with_level(gba: &mut agb::Gba) {
         start_at_boss = loop {
             sfx.frame();
             vblank.wait_for_vblank();
-            sfx.after_vblank();
             object.commit();
             match game.advance_frame(&object, &mut vram, &mut sfx) {
                 GameStatus::Continue => {}

examples/the-purple-night/src/sfx.rs

@@ -39,10 +39,6 @@ impl<'a> Sfx<'a> {
         self.mixer.frame();
     }
 
-    pub fn after_vblank(&mut self) {
-        self.mixer.after_vblank();
-    }
-
     pub fn stop_music(&mut self) {
         if let Some(bgm) = &self.bgm {
             let channel = self.mixer.channel(bgm).unwrap();