From 2b4c4459e05184d6954d51ec3e7802f88e3576f7 Mon Sep 17 00:00:00 2001 From: Corwin Date: Sat, 17 Feb 2024 02:34:39 +0000 Subject: [PATCH 01/10] use portable atomics and other similar libraries --- agb/Cargo.toml | 4 +- agb/examples/output.rs | 8 +- agb/examples/wave.rs | 10 +- agb/src/dma.rs | 2 +- agb/src/interrupt.rs | 66 +++--- agb/src/lib.rs | 2 +- agb/src/rng.rs | 17 +- agb/src/save/flash.rs | 18 +- agb/src/save/mod.rs | 14 +- agb/src/save/utils.rs | 10 +- agb/src/sound/mixer/sw_mixer.rs | 10 +- agb/src/sync/locks.rs | 223 -------------------- agb/src/sync/mod.rs | 136 ++++++++++-- agb/src/sync/statics.rs | 337 ------------------------------ agb/tests/save_test_common/mod.rs | 6 +- 15 files changed, 202 insertions(+), 661 deletions(-) delete mode 100644 agb/src/sync/locks.rs delete mode 100644 agb/src/sync/statics.rs diff --git a/agb/Cargo.toml b/agb/Cargo.toml index e6d35195..f824bd11 100644 --- a/agb/Cargo.toml +++ b/agb/Cargo.toml @@ -19,9 +19,11 @@ agb_sound_converter = { version = "0.19.1", path = "../agb-sound-converter" } agb_macros = { version = "0.19.1", path = "../agb-macros" } agb_fixnum = { version = "0.19.1", path = "../agb-fixnum" } agb_hashmap = { version = "0.19.1", path = "../agb-hashmap" } -bare-metal = "1" bilge = "0.2" qrcodegen-no-heap = "1.8" +portable-atomic = { version = "1.6.0", default-features = false, features = ["unsafe-assume-single-core"] } +once_cell = { version = "1.19.0", default-features = false, features = ["critical-section"] } +critical-section = { version = "1.1.2", features = ["restore-state-u16"] } [package.metadata.docs.rs] default-target = "thumbv4t-none-eabi" diff --git a/agb/examples/output.rs b/agb/examples/output.rs index 8aabc69d..15ca4ee8 100644 --- a/agb/examples/output.rs +++ b/agb/examples/output.rs @@ -1,17 +1,17 @@ #![no_std] #![no_main] -use agb::sync::Static; +use portable_atomic::{AtomicU32, Ordering}; -static COUNT: Static = Static::new(0); +static COUNT: AtomicU32 = AtomicU32::new(0); #[agb::entry] fn main(_gba: agb::Gba) -> ! { let _a = unsafe { agb::interrupt::add_interrupt_handler(agb::interrupt::Interrupt::VBlank, |_| { - let cur_count = COUNT.read(); + let cur_count = COUNT.load(Ordering::SeqCst); agb::println!("Hello, world, frame = {}", cur_count); - COUNT.write(cur_count + 1); + COUNT.store(cur_count + 1, Ordering::SeqCst); }) }; loop {} diff --git a/agb/examples/wave.rs b/agb/examples/wave.rs index 1e9cf223..4b71be9c 100644 --- a/agb/examples/wave.rs +++ b/agb/examples/wave.rs @@ -9,9 +9,9 @@ use agb::{ tiled::{RegularBackgroundSize, TileFormat}, }, fixnum::FixedNum, - interrupt::{free, Interrupt}, + interrupt::Interrupt, }; -use bare_metal::{CriticalSection, Mutex}; +use critical_section::{CriticalSection, Mutex}; struct BackCosines { cosines: [u16; 32], @@ -37,7 +37,7 @@ fn main(mut gba: agb::Gba) -> ! { let _a = unsafe { agb::interrupt::add_interrupt_handler(Interrupt::HBlank, |key: CriticalSection| { - let mut back = BACK.borrow(key).borrow_mut(); + let mut back = BACK.borrow_ref_mut(key); let deflection = back.cosines[back.row % 32]; ((0x0400_0010) as *mut u16).write_volatile(deflection); back.row += 1; @@ -49,8 +49,8 @@ fn main(mut gba: agb::Gba) -> ! { loop { vblank.wait_for_vblank(); - free(|key| { - let mut back = BACK.borrow(key).borrow_mut(); + critical_section::with(|key| { + let mut back = BACK.borrow_ref_mut(key); back.row = 0; time += 1; for (r, a) in back.cosines.iter_mut().enumerate() { diff --git a/agb/src/dma.rs b/agb/src/dma.rs index 8135a499..a16fc021 100644 --- a/agb/src/dma.rs +++ b/agb/src/dma.rs @@ -191,7 +191,7 @@ pub(crate) fn dma3_exclusive(f: impl FnOnce() -> R) -> R { const DMA1_CTRL_HI: MemoryMapped = unsafe { MemoryMapped::new(dma_control_addr(1) + 2) }; const DMA2_CTRL_HI: MemoryMapped = unsafe { MemoryMapped::new(dma_control_addr(2) + 2) }; - crate::interrupt::free(|_| { + critical_section::with(|_| { let dma0_ctl = DMA0_CTRL_HI.get(); let dma1_ctl = DMA1_CTRL_HI.get(); let dma2_ctl = DMA2_CTRL_HI.get(); diff --git a/agb/src/interrupt.rs b/agb/src/interrupt.rs index c30c4b45..8caab48c 100644 --- a/agb/src/interrupt.rs +++ b/agb/src/interrupt.rs @@ -1,9 +1,10 @@ use core::{cell::Cell, marker::PhantomPinned, pin::Pin}; use alloc::boxed::Box; -use bare_metal::CriticalSection; +use critical_section::{CriticalSection, RawRestoreState}; +use portable_atomic::{AtomicBool, AtomicUsize, Ordering}; -use crate::{display::DISPLAY_STATUS, memory_mapped::MemoryMapped, sync::Static}; +use crate::{display::DISPLAY_STATUS, memory_mapped::MemoryMapped}; #[derive(Clone, Copy)] pub enum Interrupt { @@ -234,7 +235,7 @@ fn interrupt_to_root(interrupt: Interrupt) -> &'static InterruptRoot { /// * The closure must be static because forgetting the interrupt handler would /// cause a use after free. /// -/// [`CriticalSection`]: bare_metal::CriticalSection +/// [`CriticalSection`]: critical_section::CriticalSection /// /// # Examples /// @@ -242,7 +243,7 @@ fn interrupt_to_root(interrupt: Interrupt) -> &'static InterruptRoot { /// # #![no_std] /// # #![no_main] /// # fn foo() { -/// use bare_metal::CriticalSection; +/// use critical_section::CriticalSection; /// use agb::interrupt::{add_interrupt_handler, Interrupt}; /// // Safety: doesn't allocate /// let _a = unsafe { @@ -257,7 +258,7 @@ pub unsafe fn add_interrupt_handler( handler: impl Fn(CriticalSection) + Send + Sync + 'static, ) -> InterruptHandler { fn do_with_inner(interrupt: Interrupt, inner: Pin>) -> InterruptHandler { - free(|_| { + critical_section::with(|_| { let root = interrupt_to_root(interrupt); root.add(); let mut c = root.next.get(); @@ -283,32 +284,23 @@ pub unsafe fn add_interrupt_handler( do_with_inner(interrupt, inner) } -/// How you can access mutexes outside of interrupts by being given a -/// [`CriticalSection`] -/// -/// [`CriticalSection`]: bare_metal::CriticalSection -pub fn free(mut f: F) -> R -where - F: FnOnce(CriticalSection) -> R, -{ - let enabled = INTERRUPTS_ENABLED.get(); +struct MyCriticalSection; +critical_section::set_impl!(MyCriticalSection); - disable_interrupts(); +unsafe impl critical_section::Impl for MyCriticalSection { + unsafe fn acquire() -> RawRestoreState { + let irq = INTERRUPTS_ENABLED.get(); + INTERRUPTS_ENABLED.set(0); + irq + } - // prevents the contents of the function from being reordered before IME is disabled. - crate::sync::memory_write_hint(&mut f); - - let mut r = f(unsafe { CriticalSection::new() }); - - // prevents the contents of the function from being reordered after IME is re-enabled. - crate::sync::memory_write_hint(&mut r); - - INTERRUPTS_ENABLED.set(enabled); - r + unsafe fn release(token: RawRestoreState) { + INTERRUPTS_ENABLED.set(token); + } } -static NUM_VBLANKS: Static = Static::new(0); // overflows after 2.27 years -static HAS_CREATED_INTERRUPT: Static = Static::new(false); +static NUM_VBLANKS: AtomicUsize = AtomicUsize::new(0); // overflows after 2.27 years +static HAS_CREATED_INTERRUPT: AtomicBool = AtomicBool::new(false); #[non_exhaustive] pub struct VBlank { @@ -320,29 +312,28 @@ impl VBlank { /// interrupt syscall. #[must_use] pub fn get() -> Self { - if !HAS_CREATED_INTERRUPT.read() { + if !HAS_CREATED_INTERRUPT.swap(true, Ordering::SeqCst) { // safety: we don't allocate in the interrupt let handler = unsafe { add_interrupt_handler(Interrupt::VBlank, |_| { - NUM_VBLANKS.write(NUM_VBLANKS.read() + 1); + NUM_VBLANKS.store(NUM_VBLANKS.load(Ordering::SeqCst) + 1, Ordering::SeqCst); }) }; core::mem::forget(handler); - - HAS_CREATED_INTERRUPT.write(true); } VBlank { - last_waited_number: Cell::new(NUM_VBLANKS.read()), + last_waited_number: Cell::new(NUM_VBLANKS.load(Ordering::SeqCst)), } } /// Pauses CPU until vblank interrupt is triggered where code execution is /// resumed. pub fn wait_for_vblank(&self) { let last_waited_number = self.last_waited_number.get(); - self.last_waited_number.set(NUM_VBLANKS.read() + 1); + self.last_waited_number + .set(NUM_VBLANKS.load(Ordering::SeqCst) + 1); - if last_waited_number < NUM_VBLANKS.read() { + if last_waited_number < NUM_VBLANKS.load(Ordering::SeqCst) { return; } @@ -383,4 +374,11 @@ mod tests { "interrupt table should be able to store gamepak interrupt" ); } + + #[test_case] + fn interrupts_disabled_in_critical_section(_gba: &mut crate::Gba) { + critical_section::with(|_| { + assert_eq!(INTERRUPTS_ENABLED.get(), 0); + }); + } } diff --git a/agb/src/lib.rs b/agb/src/lib.rs index 0febf2b5..9d5f72ed 100644 --- a/agb/src/lib.rs +++ b/agb/src/lib.rs @@ -175,7 +175,7 @@ mod single; /// Implements sound output. pub mod sound; /// A module containing functions and utilities useful for synchronizing state. -pub mod sync; +mod sync; /// System BIOS calls / syscalls. pub mod syscall; /// Interactions with the internal timers diff --git a/agb/src/rng.rs b/agb/src/rng.rs index b98ad6ce..320d2955 100644 --- a/agb/src/rng.rs +++ b/agb/src/rng.rs @@ -1,8 +1,4 @@ -use core::cell::RefCell; - -use bare_metal::Mutex; - -use crate::interrupt::free; +use portable_atomic::{AtomicU128, Ordering}; /// A fast pseudo-random number generator. Note that the output of the /// random number generator for a given seed is guaranteed stable @@ -58,13 +54,18 @@ impl Default for RandomNumberGenerator { } } -static GLOBAL_RNG: Mutex> = - Mutex::new(RefCell::new(RandomNumberGenerator::new())); +static GLOBAL_RNG: AtomicU128 = + AtomicU128::new(unsafe { core::mem::transmute(RandomNumberGenerator::new().state) }); /// Using a global random number generator, provides the next random number #[must_use] pub fn gen() -> i32 { - free(|cs| GLOBAL_RNG.borrow(cs).borrow_mut().gen()) + let data: u128 = GLOBAL_RNG.load(Ordering::SeqCst); + let data_u32: [u32; 4] = unsafe { core::mem::transmute(data) }; + let mut rng = RandomNumberGenerator { state: data_u32 }; + let value = rng.gen(); + GLOBAL_RNG.store(unsafe { core::mem::transmute(rng.state) }, Ordering::SeqCst); + value } #[cfg(test)] diff --git a/agb/src/save/flash.rs b/agb/src/save/flash.rs index 08b55a38..95e7d5e7 100644 --- a/agb/src/save/flash.rs +++ b/agb/src/save/flash.rs @@ -5,11 +5,13 @@ // TODO: Setup cartridge read timings for faster Flash access. +use once_cell::sync::OnceCell; +use portable_atomic::{AtomicU8, Ordering}; + use crate::memory_mapped::{MemoryMapped, MemoryMapped1DArray}; use crate::save::asm_utils::*; use crate::save::utils::Timeout; use crate::save::{Error, MediaInfo, MediaType, RawSaveAccess}; -use crate::sync::{InitOnce, Static}; use core::cmp; // Volatile address ports for flash @@ -45,14 +47,14 @@ fn issue_flash_command(c2: u8) { } /// A simple thing to avoid excessive bank switches -static CURRENT_BANK: Static = Static::new(!0); +static CURRENT_BANK: AtomicU8 = AtomicU8::new(!0); fn set_bank(bank: u8) -> Result<(), Error> { if bank == 0xFF { Err(Error::OutOfBounds) - } else if bank != CURRENT_BANK.read() { + } else if bank != CURRENT_BANK.load(Ordering::SeqCst) { issue_flash_command(CMD_SET_BANK); FLASH_PORT_BANK.set(bank); - CURRENT_BANK.write(bank); + CURRENT_BANK.store(bank, Ordering::SeqCst); Ok(()) } else { Ok(()) @@ -239,10 +241,12 @@ impl FlashChipType { } } } -static CHIP_INFO: InitOnce<&'static ChipInfo> = InitOnce::new(); + +static CHIP_INFO: OnceCell<&'static ChipInfo> = OnceCell::new(); + fn cached_chip_info() -> Result<&'static ChipInfo, Error> { CHIP_INFO - .try_get(|| -> Result<_, Error> { Ok(FlashChipType::detect()?.chip_info()) }) + .get_or_try_init(|| -> Result<_, Error> { Ok(FlashChipType::detect()?.chip_info()) }) .cloned() } @@ -380,7 +384,7 @@ impl ChipInfo { buf: &[u8], timeout: &mut Timeout, ) -> Result<(), Error> { - crate::interrupt::free(|_| { + critical_section::with(|_| { issue_flash_command(CMD_WRITE); for i in 0..128 { FLASH_DATA.set(offset + i, buf[i]); diff --git a/agb/src/save/mod.rs b/agb/src/save/mod.rs index 915f89ed..7adb1d71 100644 --- a/agb/src/save/mod.rs +++ b/agb/src/save/mod.rs @@ -86,7 +86,7 @@ //! size. use crate::save::utils::Timeout; -use crate::sync::{Mutex, RawMutexGuard}; +use crate::sync::{Lock, RawLockGuard}; use crate::timer::Timer; use core::ops::Range; @@ -179,7 +179,7 @@ trait RawSaveAccess: Sync { fn write(&self, offset: usize, buffer: &[u8], timeout: &mut Timeout) -> Result<(), Error>; } -static CURRENT_SAVE_ACCESS: Mutex> = Mutex::new(None); +static CURRENT_SAVE_ACCESS: Lock> = Lock::new(None); fn set_save_implementation(access_impl: &'static dyn RawSaveAccess) { let mut access = CURRENT_SAVE_ACCESS.lock(); @@ -196,7 +196,7 @@ fn get_save_implementation() -> Option<&'static dyn RawSaveAccess> { /// Allows reading and writing of save media. pub struct SaveData { - _lock: RawMutexGuard<'static>, + _lock: RawLockGuard<'static>, access: &'static dyn RawSaveAccess, info: &'static MediaInfo, timeout: utils::Timeout, @@ -356,19 +356,19 @@ mod marker { #[inline(always)] pub fn emit_eeprom_marker() { - crate::sync::memory_read_hint(&EEPROM); + core::hint::black_box(&EEPROM); } #[inline(always)] pub fn emit_sram_marker() { - crate::sync::memory_read_hint(&SRAM); + core::hint::black_box(&SRAM); } #[inline(always)] pub fn emit_flash_512k_marker() { - crate::sync::memory_read_hint(&FLASH512K); + core::hint::black_box(&FLASH512K); } #[inline(always)] pub fn emit_flash_1m_marker() { - crate::sync::memory_read_hint(&FLASH1M); + core::hint::black_box(&FLASH1M); } } diff --git a/agb/src/save/utils.rs b/agb/src/save/utils.rs index ea12ee27..c83727ae 100644 --- a/agb/src/save/utils.rs +++ b/agb/src/save/utils.rs @@ -1,8 +1,10 @@ //! A package containing useful utilities for writing save accessors. use super::Error; -use crate::sync::{RawMutex, RawMutexGuard}; -use crate::timer::{Divider, Timer}; +use crate::{ + sync::{RawLock, RawLockGuard}, + timer::{Divider, Timer}, +}; /// A timeout type used to prevent hardware errors in save media from hanging /// the game. @@ -50,8 +52,8 @@ impl Drop for Timeout { } } -pub fn lock_media_access() -> Result, Error> { - static LOCK: RawMutex = RawMutex::new(); +pub fn lock_media_access() -> Result, Error> { + static LOCK: RawLock = RawLock::new(); match LOCK.try_lock() { Some(x) => Ok(x), None => Err(Error::MediaInUse), diff --git a/agb/src/sound/mixer/sw_mixer.rs b/agb/src/sound/mixer/sw_mixer.rs index e1d6c743..2466428c 100644 --- a/agb/src/sound/mixer/sw_mixer.rs +++ b/agb/src/sound/mixer/sw_mixer.rs @@ -4,7 +4,7 @@ use core::pin::Pin; use alloc::boxed::Box; use alloc::vec::Vec; -use bare_metal::{CriticalSection, Mutex}; +use critical_section::{CriticalSection, Mutex}; use super::hw::LeftOrRight; use super::{hw, Frequency}; @@ -13,7 +13,6 @@ use super::{SoundChannel, SoundPriority}; use crate::InternalAllocator; use crate::{ fixnum::Num, - interrupt::free, interrupt::{add_interrupt_handler, InterruptHandler}, timer::Divider, timer::Timer, @@ -395,11 +394,11 @@ impl MixerBuffer { } fn should_calculate(&self) -> bool { - free(|cs| self.state.borrow(cs).borrow().should_calculate()) + critical_section::with(|cs| self.state.borrow_ref_mut(cs).should_calculate()) } fn swap(&self, cs: CriticalSection) { - let buffer = self.state.borrow(cs).borrow_mut().playing_advanced(); + let buffer = self.state.borrow_ref_mut(cs).playing_advanced(); let left_buffer = buffer; // SAFETY: starting pointer is fine, resulting pointer also fine because buffer has length buffer_size() * 2 by construction @@ -435,7 +434,8 @@ impl MixerBuffer { } } - let write_buffer = free(|cs| self.state.borrow(cs).borrow_mut().active_advanced()); + let write_buffer = + critical_section::with(|cs| self.state.borrow_ref_mut(cs).active_advanced()); unsafe { agb_rs__mixer_collapse( diff --git a/agb/src/sync/locks.rs b/agb/src/sync/locks.rs deleted file mode 100644 index 2c2cd952..00000000 --- a/agb/src/sync/locks.rs +++ /dev/null @@ -1,223 +0,0 @@ -use crate::sync::Static; -use core::cell::UnsafeCell; -use core::mem::MaybeUninit; -use core::ops::{Deref, DerefMut}; -use core::ptr; -use core::sync::atomic::{compiler_fence, Ordering}; - -#[inline(never)] -fn already_locked() -> ! { - panic!("IRQ and main thread are attempting to access the same Mutex!") -} - -/// A mutex that prevents code from running in both an IRQ and normal code at -/// the same time. -/// -/// Note that this does not support blocking like a typical mutex, and instead -/// mainly exists for memory safety reasons. -pub struct RawMutex(Static); -impl RawMutex { - /// Creates a new lock. - #[must_use] - pub const fn new() -> Self { - RawMutex(Static::new(false)) - } - - /// Locks the mutex and returns whether a lock was successfully acquired. - fn raw_lock(&self) -> bool { - if self.0.replace(true) { - // value was already true, oops. - false - } else { - // prevent any weird reordering, and continue - compiler_fence(Ordering::Acquire); - true - } - } - - /// Unlocks the mutex. - fn raw_unlock(&self) { - compiler_fence(Ordering::Release); - if !self.0.replace(false) { - panic!("Internal error: Attempt to unlock a `RawMutex` which is not locked.") - } - } - - /// Returns a guard for this lock, or panics if there is another lock active. - pub fn lock(&self) -> RawMutexGuard<'_> { - self.try_lock().unwrap_or_else(|| already_locked()) - } - - /// Returns a guard for this lock, or `None` if there is another lock active. - pub fn try_lock(&self) -> Option> { - if self.raw_lock() { - Some(RawMutexGuard(self)) - } else { - None - } - } -} -unsafe impl Send for RawMutex {} -unsafe impl Sync for RawMutex {} - -impl Default for RawMutex { - fn default() -> Self { - Self::new() - } -} - -/// A guard representing an active lock on an [`RawMutex`]. -pub struct RawMutexGuard<'a>(&'a RawMutex); -impl<'a> Drop for RawMutexGuard<'a> { - fn drop(&mut self) { - self.0.raw_unlock(); - } -} - -/// A mutex that protects an object from being accessed in both an IRQ and -/// normal code at once. -/// -/// Note that this does not support blocking like a typical mutex, and instead -/// mainly exists for memory safety reasons. -pub struct Mutex { - raw: RawMutex, - data: UnsafeCell, -} -impl Mutex { - /// Creates a new lock containing a given value. - #[must_use] - pub const fn new(t: T) -> Self { - Mutex { - raw: RawMutex::new(), - data: UnsafeCell::new(t), - } - } - - /// Returns a guard for this lock, or panics if there is another lock active. - pub fn lock(&self) -> MutexGuard<'_, T> { - self.try_lock().unwrap_or_else(|| already_locked()) - } - - /// Returns a guard for this lock or `None` if there is another lock active. - pub fn try_lock(&self) -> Option> { - if self.raw.raw_lock() { - Some(MutexGuard { - underlying: self, - ptr: self.data.get(), - }) - } else { - None - } - } -} -unsafe impl Send for Mutex {} -unsafe impl Sync for Mutex {} - -/// A guard representing an active lock on an [`Mutex`]. -pub struct MutexGuard<'a, T> { - underlying: &'a Mutex, - ptr: *mut T, -} -impl<'a, T> Drop for MutexGuard<'a, T> { - fn drop(&mut self) { - self.underlying.raw.raw_unlock(); - } -} -impl<'a, T> Deref for MutexGuard<'a, T> { - type Target = T; - fn deref(&self) -> &Self::Target { - unsafe { &*self.ptr } - } -} -impl<'a, T> DerefMut for MutexGuard<'a, T> { - fn deref_mut(&mut self) -> &mut Self::Target { - unsafe { &mut *self.ptr } - } -} - -enum Void {} - -/// A helper type that ensures a particular value is only initialized once. -pub struct InitOnce { - is_initialized: Static, - value: UnsafeCell>, -} - -impl InitOnce { - /// Creates a new uninitialized object. - #[must_use] - pub const fn new() -> Self { - InitOnce { - is_initialized: Static::new(false), - value: UnsafeCell::new(MaybeUninit::uninit()), - } - } - - /// Gets the contents of this state, or initializes it if it has not already - /// been initialized. - /// - /// The initializer function is guaranteed to only be called once. - /// - /// This function disables IRQs while it is initializing the inner value. - /// While this can cause audio skipping and other similar issues, it is - /// not normally a problem as interrupts will only be disabled once per - /// `InitOnce` during the life cycle of the program. - pub fn get(&self, initializer: impl FnOnce() -> T) -> &T { - match self.try_get(|| -> Result { Ok(initializer()) }) { - Ok(v) => v, - _ => unimplemented!(), - } - } - - /// Gets the contents of this state, or initializes it if it has not already - /// been initialized. - /// - /// The initializer function is guaranteed to only be called once if it - /// returns `Ok`. If it returns `Err`, it will be called again in the - /// future until an attempt at initialization succeeds. - /// - /// This function disables IRQs while it is initializing the inner value. - /// While this can cause audio skipping and other similar issues, it is - /// not normally a problem as interrupts will only be disabled once per - /// `InitOnce` during the life cycle of the program. - pub fn try_get(&self, initializer: impl FnOnce() -> Result) -> Result<&T, E> { - unsafe { - if !self.is_initialized.read() { - // We disable interrupts to make this simpler, since this is likely to - // only occur once in a program anyway. - crate::interrupt::free(|_| -> Result<(), E> { - // We check again to make sure this function wasn't called in an - // interrupt between the first check and when interrupts were - // actually disabled. - if !self.is_initialized.read() { - // Do the actual initialization. - ptr::write_volatile((*self.value.get()).as_mut_ptr(), initializer()?); - self.is_initialized.write(true); - } - Ok(()) - })?; - } - compiler_fence(Ordering::Acquire); - Ok(&*(*self.value.get()).as_mut_ptr()) - } - } -} - -impl Default for InitOnce { - fn default() -> Self { - Self::new() - } -} - -impl Drop for InitOnce { - fn drop(&mut self) { - if self.is_initialized.read() { - // drop the value inside the `MaybeUninit` - unsafe { - ptr::read((*self.value.get()).as_ptr()); - } - } - } -} -unsafe impl Send for InitOnce {} -unsafe impl Sync for InitOnce {} diff --git a/agb/src/sync/mod.rs b/agb/src/sync/mod.rs index 826a8326..769a6500 100644 --- a/agb/src/sync/mod.rs +++ b/agb/src/sync/mod.rs @@ -1,29 +1,123 @@ -mod locks; -mod statics; +use core::cell::UnsafeCell; +use core::ops::{Deref, DerefMut}; -pub use locks::*; -pub use statics::*; +use portable_atomic::{AtomicBool, Ordering}; -use core::arch::asm; - -/// Marks that a pointer is read without actually reading from this. -/// -/// This uses an [`asm!`] instruction that marks the parameter as being read, -/// requiring the compiler to treat this function as if anything could be -/// done to it. -#[inline(always)] -pub fn memory_read_hint(val: *const T) { - unsafe { asm!("/* {0} */", in(reg) val, options(readonly, nostack)) } +#[inline(never)] +fn already_locked() -> ! { + panic!("IRQ and main thread are attempting to access the same Lock!") } -/// Marks that a pointer is read or written to without actually writing to it. +/// A lock that prevents code from running in both an IRQ and normal code at +/// the same time. /// -/// This uses an [`asm!`] instruction that marks the parameter as being read -/// and written, requiring the compiler to treat this function as if anything -/// could be done to it. -#[inline(always)] -pub fn memory_write_hint(val: *mut T) { - unsafe { asm!("/* {0} */", in(reg) val, options(nostack)) } +/// Note that this does not support blocking like a typical mutex, and instead +/// mainly exists for memory safety reasons. +pub struct RawLock(AtomicBool); +impl RawLock { + /// Creates a new lock. + #[must_use] + pub const fn new() -> Self { + RawLock(AtomicBool::new(false)) + } + + /// Locks the lock and returns whether a lock was successfully acquired. + fn raw_lock(&self) -> bool { + if self.0.swap(true, Ordering::Acquire) { + // value was already true, oops. + false + } else { + // prevent any weird reordering, and continue + true + } + } + + /// Unlocks the lock. + fn raw_unlock(&self) { + if !self.0.swap(false, Ordering::Release) { + panic!("Internal error: Attempt to unlock a `RawLock` which is not locked.") + } + } + + /// Returns a guard for this lock, or `None` if there is another lock active. + pub fn try_lock(&self) -> Option> { + if self.raw_lock() { + Some(RawLockGuard(self)) + } else { + None + } + } +} +unsafe impl Send for RawLock {} +unsafe impl Sync for RawLock {} + +/// A guard representing an active lock on an [`RawLock`]. +pub struct RawLockGuard<'a>(&'a RawLock); +impl<'a> Drop for RawLockGuard<'a> { + fn drop(&mut self) { + self.0.raw_unlock(); + } +} + +/// A lock that protects an object from being accessed in both an IRQ and +/// normal code at once. +/// +/// Note that this does not support blocking like a typical mutex, and instead +/// mainly exists for memory safety reasons. +pub struct Lock { + raw: RawLock, + data: UnsafeCell, +} +impl Lock { + /// Creates a new lock containing a given value. + #[must_use] + pub const fn new(t: T) -> Self { + Lock { + raw: RawLock::new(), + data: UnsafeCell::new(t), + } + } + + /// Returns a guard for this lock, or panics if there is another lock active. + pub fn lock(&self) -> LockGuard<'_, T> { + self.try_lock().unwrap_or_else(|| already_locked()) + } + + /// Returns a guard for this lock or `None` if there is another lock active. + pub fn try_lock(&self) -> Option> { + if self.raw.raw_lock() { + Some(LockGuard { + underlying: self, + ptr: self.data.get(), + }) + } else { + None + } + } +} +unsafe impl Send for Lock {} +unsafe impl Sync for Lock {} + +/// A guard representing an active lock on an [`Lock`]. +pub struct LockGuard<'a, T> { + underlying: &'a Lock, + ptr: *mut T, +} +impl<'a, T> Drop for LockGuard<'a, T> { + fn drop(&mut self) { + self.underlying.raw.raw_unlock(); + } +} +impl<'a, T> Deref for LockGuard<'a, T> { + type Target = T; + fn deref(&self) -> &Self::Target { + unsafe { &*self.ptr } + } +} +impl<'a, T> DerefMut for LockGuard<'a, T> { + fn deref_mut(&mut self) -> &mut Self::Target { + unsafe { &mut *self.ptr } + } } /// An internal function used as a temporary hack to get `compiler_fence` diff --git a/agb/src/sync/statics.rs b/agb/src/sync/statics.rs deleted file mode 100644 index e7828c6c..00000000 --- a/agb/src/sync/statics.rs +++ /dev/null @@ -1,337 +0,0 @@ -use core::arch::asm; -use core::cell::UnsafeCell; -use core::mem; -use core::ptr; - -/// The internal function for replacing a `Copy` (really `!Drop`) value in a -/// [`Static`]. This uses assembly to use an `stmia` instruction to ensure -/// an IRQ cannot occur during the write operation. -unsafe fn transfer(dst: *mut T, src: *const T) { - let align = mem::align_of::(); - let size = mem::size_of::(); - - if size == 0 { - // Do nothing with ZSTs. - } else if size <= 16 && align % 4 == 0 { - // We can do an 4-byte aligned transfer up to 16 bytes. - transfer_align4_thumb(dst, src); - } else if size <= 40 && align % 4 == 0 { - // We can do the same up to 40 bytes, but we need to switch to ARM. - transfer_align4_arm(dst, src); - } else if size <= 2 && align % 2 == 0 { - // We can do a 2-byte aligned transfer up to 2 bytes. - asm!( - "ldrh {2},[{0}]", - "strh {2},[{1}]", - in(reg) src, in(reg) dst, out(reg) _, - ); - } else if size == 1 { - // We can do a simple byte copy. - asm!( - "ldrb {2},[{0}]", - "strb {2},[{1}]", - in(reg) src, in(reg) dst, out(reg) _, - ); - } else { - // When we don't have an optimized path, we just disable IRQs. - crate::interrupt::free(|_| ptr::write_volatile(dst, ptr::read_volatile(src))); - } -} - -#[allow(unused_assignments)] -unsafe fn transfer_align4_thumb(mut dst: *mut T, mut src: *const T) { - let size = mem::size_of::(); - - if size <= 4 { - // We use assembly here regardless to just do the word aligned copy. This - // ensures it's done with a single ldr/str instruction. - asm!( - "ldr {2},[{0}]", - "str {2},[{1}]", - inout(reg) src, in(reg) dst, out(reg) _, - ); - } else if size <= 8 { - // Starting at size == 8, we begin using ldmia/stmia to load/save multiple - // words in one instruction, avoiding IRQs from interrupting our operation. - asm!( - "ldmia {0}!, {{r2-r3}}", - "stmia {1}!, {{r2-r3}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, - ); - } else if size <= 12 { - asm!( - "ldmia {0}!, {{r2-r4}}", - "stmia {1}!, {{r2-r4}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, - ); - } else if size <= 16 { - asm!( - "ldmia {0}!, {{r2-r5}}", - "stmia {1}!, {{r2-r5}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, - ); - } else { - unimplemented!("This should be done via transfer_arm."); - } -} - -#[instruction_set(arm::a32)] -#[allow(unused_assignments)] -unsafe fn transfer_align4_arm(mut dst: *mut T, mut src: *const T) { - let size = mem::size_of::(); - - if size <= 16 { - unimplemented!("This should be done via transfer_thumb."); - } else if size <= 20 { - // Starting at size == 16, we have to switch to ARM due to lack of - // accessible registers in THUMB mode. - asm!( - "ldmia {0}!, {{r2-r5,r7}}", - "stmia {1}!, {{r2-r5,r7}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r7") _, - ); - } else if size <= 24 { - asm!( - "ldmia {0}!, {{r2-r5,r7-r8}}", - "stmia {1}!, {{r2-r5,r7-r8}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r7") _, - out("r8") _, - ); - } else if size <= 28 { - asm!( - "ldmia {0}!, {{r2-r5,r7-r9}}", - "stmia {1}!, {{r2-r5,r7-r9}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r7") _, - out("r8") _, out("r9") _, - ); - } else if size <= 32 { - asm!( - "ldmia {0}!, {{r2-r5,r7-r10}}", - "stmia {1}!, {{r2-r5,r7-r10}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r7") _, - out("r8") _, out("r9") _, out("r10") _, - ); - } else if size <= 36 { - asm!( - "ldmia {0}!, {{r2-r5,r7-r10,r12}}", - "stmia {1}!, {{r2-r5,r7-r10,r12}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r7") _, - out("r8") _, out("r9") _, out("r10") _, out("r12") _, - ); - } else if size <= 40 { - asm!( - "ldmia {0}!, {{r2-r5,r7-r10,r12,r14}}", - "stmia {1}!, {{r2-r5,r7-r10,r12,r14}}", - inout(reg) src, inout(reg) dst, - out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r7") _, - out("r8") _, out("r9") _, out("r10") _, out("r12") _, out("r14") _, - ); - } else { - // r13 is sp, and r15 is pc. Neither are usable - unimplemented!("Copy too large for use of ldmia/stmia."); - } -} - -/// The internal function for swapping the current value of a [`Static`] with -/// another value. -unsafe fn exchange(dst: *mut T, src: *const T) -> T { - let align = mem::align_of::(); - let size = mem::size_of::(); - if size == 0 { - // Do nothing with ZSTs. - ptr::read(dst) - } else if size <= 4 && align % 4 == 0 { - // Swap a single word with the SWP instruction. - let val = ptr::read(src as *const u32); - let new_val = exchange_align4_arm(dst, val); - ptr::read(&new_val as *const _ as *const T) - } else if size == 1 { - // Swap a byte with the SWPB instruction. - let val = ptr::read(src as *const u8); - let new_val = exchange_align1_arm(dst, val); - ptr::read(&new_val as *const _ as *const T) - } else { - // fallback - crate::interrupt::free(|_| { - let cur = ptr::read_volatile(dst); - ptr::write_volatile(dst, ptr::read_volatile(src)); - cur - }) - } -} - -#[instruction_set(arm::a32)] -unsafe fn exchange_align4_arm(dst: *mut T, i: u32) -> u32 { - let out; - asm!("swp {2}, {1}, [{0}]", in(reg) dst, in(reg) i, lateout(reg) out); - out -} - -#[instruction_set(arm::a32)] -unsafe fn exchange_align1_arm(dst: *mut T, i: u8) -> u8 { - let out; - asm!("swpb {2}, {1}, [{0}]", in(reg) dst, in(reg) i, lateout(reg) out); - out -} - -/// A helper that implements static variables. -/// -/// It ensures that even if you use the same static variable in both an IRQ -/// and normal code, the IRQ will never observe an invalid value of the -/// variable. -/// -/// This type only works with owned values. If you need to work with borrows, -/// consider using [`sync::Mutex`](`crate::sync::Mutex`) instead. -/// -/// ## Performance -/// -/// Writing or reading from a static variable is efficient under the following -/// conditions: -/// -/// * The type is aligned to 4 bytes and can be stored in 40 bytes or less. -/// * The type is aligned to 2 bytes and can be stored in 2 bytes. -/// * The type is can be stored in a single byte. -/// -/// Replacing the current value of the static variable is efficient under the -/// following conditions: -/// -/// * The type is aligned to 4 bytes and can be stored in 4 bytes or less. -/// * The type is can be stored in a single byte. -/// -/// When these conditions are not met, static variables are handled using a -/// fallback routine that disables IRQs and does a normal copy. This can be -/// dangerous as disabling IRQs can cause your program to miss out on important -/// interrupts such as V-Blank. -/// -/// Consider using [`sync::Mutex`](`crate::sync::Mutex`) instead if you need to -/// use a large amount of operations that would cause IRQs to be disabled. Also -/// consider using `#[repr(align(4))]` to force proper alignment for your type. -pub struct Static { - data: UnsafeCell, -} -impl Static { - /// Creates a new static variable. - pub const fn new(val: T) -> Self { - Static { - data: UnsafeCell::new(val), - } - } - - /// Replaces the current value of the static variable with another, and - /// returns the old value. - #[allow(clippy::needless_pass_by_value)] // critical for safety - pub fn replace(&self, val: T) -> T { - unsafe { exchange(self.data.get(), &val) } - } - - /// Extracts the interior value of the static variable. - pub fn into_inner(self) -> T { - self.data.into_inner() - } -} -impl Static { - /// Writes a new value into this static variable. - pub fn write(&self, val: T) { - unsafe { transfer(self.data.get(), &val) } - } - - /// Reads a value from this static variable. - pub fn read(&self) -> T { - unsafe { - let mut out: mem::MaybeUninit = mem::MaybeUninit::uninit(); - transfer(out.as_mut_ptr(), self.data.get()); - out.assume_init() - } - } -} -impl Default for Static { - fn default() -> Self { - Static::new(T::default()) - } -} -unsafe impl Send for Static {} -unsafe impl Sync for Static {} - -#[cfg(test)] -mod test { - use crate::interrupt::Interrupt; - use crate::sync::Static; - use crate::timer::Divider; - use crate::Gba; - - macro_rules! generate_concurrency_test { - ($count:literal, $gba:ident) => {{ - (|gba: &mut Gba| { - const SENTINEL: [u32; $count] = [0x12345678; $count]; - static VALUE: Static<[u32; $count]> = Static::new(SENTINEL); - - // set up a timer and an interrupt that uses the timer - let mut timer = gba.timers.timers().timer2; - timer.set_cascade(false); - timer.set_divider(Divider::Divider1); - timer.set_overflow_amount(1049); - timer.set_interrupt(true); - timer.set_enabled(true); - - let _int = unsafe { - crate::interrupt::add_interrupt_handler(Interrupt::Timer2, |_| { - VALUE.write(SENTINEL); - }) - }; - - // the actual main test loop - let mut interrupt_seen = false; - let mut no_interrupt_seen = false; - for i in 0..250000 { - // write to the static - let new_value = [i; $count]; - VALUE.write(new_value); - - // check the current value - let current = VALUE.read(); - if current == new_value { - no_interrupt_seen = true; - } else if current == SENTINEL { - interrupt_seen = true; - } else { - panic!("Unexpected value found in `Static`."); - } - - // we return as soon as we've seen both the value written by the main thread - // and interrupt - if interrupt_seen && no_interrupt_seen { - timer.set_enabled(false); - return; - } - - if i % 8192 == 0 && i != 0 { - timer.set_overflow_amount(1049 + (i / 64) as u16); - } - } - panic!("Concurrency test timed out: {}", $count) - })($gba); - }}; - } - - #[test_case] - fn write_read_concurrency_test(gba: &mut Gba) { - generate_concurrency_test!(1, gba); - generate_concurrency_test!(2, gba); - generate_concurrency_test!(3, gba); - generate_concurrency_test!(4, gba); - generate_concurrency_test!(5, gba); - generate_concurrency_test!(6, gba); - generate_concurrency_test!(7, gba); - generate_concurrency_test!(8, gba); - generate_concurrency_test!(9, gba); - generate_concurrency_test!(10, gba); - } -} diff --git a/agb/tests/save_test_common/mod.rs b/agb/tests/save_test_common/mod.rs index 3f7c2049..3e9f937b 100644 --- a/agb/tests/save_test_common/mod.rs +++ b/agb/tests/save_test_common/mod.rs @@ -1,10 +1,10 @@ use agb::save::{Error, MediaInfo}; -use agb::sync::InitOnce; use core::cmp; +use once_cell::sync::OnceCell; fn init_sram(gba: &mut agb::Gba) -> &'static MediaInfo { - static ONCE: InitOnce = InitOnce::new(); - ONCE.get(|| { + static ONCE: OnceCell = OnceCell::new(); + ONCE.get_or_init(|| { crate::save_setup(gba); gba.save.access().unwrap().media_info().clone() }) From 9fbd4200891e85363e561635ef9c077d9de0c0df Mon Sep 17 00:00:00 2001 From: Corwin Date: Sat, 17 Feb 2024 16:25:02 +0000 Subject: [PATCH 02/10] add test that atomic read / write works --- agb/src/interrupt.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/agb/src/interrupt.rs b/agb/src/interrupt.rs index 8caab48c..5049d07b 100644 --- a/agb/src/interrupt.rs +++ b/agb/src/interrupt.rs @@ -364,6 +364,8 @@ pub fn profiler(timer: &mut crate::timer::Timer, period: u16) -> InterruptHandle #[cfg(test)] mod tests { + use portable_atomic::AtomicU8; + use super::*; #[test_case] @@ -381,4 +383,14 @@ mod tests { assert_eq!(INTERRUPTS_ENABLED.get(), 0); }); } + + #[test_case] + fn atomic_check(_gba: &mut crate::Gba) { + static ATOMIC: AtomicU8 = AtomicU8::new(8); + + for i in 0..=255 { + ATOMIC.store(i, Ordering::SeqCst); + assert_eq!(ATOMIC.load(Ordering::SeqCst), i); + } + } } From 10575f28a6942b29e6e76abde2dd6932d4dd3b56 Mon Sep 17 00:00:00 2001 From: Corwin Date: Sat, 17 Feb 2024 16:25:09 +0000 Subject: [PATCH 03/10] delay cell --- agb/src/save/flash.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/agb/src/save/flash.rs b/agb/src/save/flash.rs index 95e7d5e7..f0b0f65b 100644 --- a/agb/src/save/flash.rs +++ b/agb/src/save/flash.rs @@ -47,8 +47,8 @@ fn issue_flash_command(c2: u8) { } /// A simple thing to avoid excessive bank switches -static CURRENT_BANK: AtomicU8 = AtomicU8::new(!0); fn set_bank(bank: u8) -> Result<(), Error> { + static CURRENT_BANK: AtomicU8 = AtomicU8::new(!0); if bank == 0xFF { Err(Error::OutOfBounds) } else if bank != CURRENT_BANK.load(Ordering::SeqCst) { @@ -242,9 +242,13 @@ impl FlashChipType { } } -static CHIP_INFO: OnceCell<&'static ChipInfo> = OnceCell::new(); - fn cached_chip_info() -> Result<&'static ChipInfo, Error> { + static CHIP_INFO: OnceCell<&'static ChipInfo> = OnceCell::new(); + + for _ in 0..100 { + unsafe { core::arch::asm!("nop") }; + } + CHIP_INFO .get_or_try_init(|| -> Result<_, Error> { Ok(FlashChipType::detect()?.chip_info()) }) .cloned() From fb247aa8f2f5497b663f2690c3082cfeb52bd740 Mon Sep 17 00:00:00 2001 From: Corwin Date: Sat, 17 Feb 2024 16:38:22 +0000 Subject: [PATCH 04/10] move single file module out of directory --- agb/src/{sync/mod.rs => sync.rs} | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) rename agb/src/{sync/mod.rs => sync.rs} (87%) diff --git a/agb/src/sync/mod.rs b/agb/src/sync.rs similarity index 87% rename from agb/src/sync/mod.rs rename to agb/src/sync.rs index 769a6500..d614662f 100644 --- a/agb/src/sync/mod.rs +++ b/agb/src/sync.rs @@ -142,3 +142,28 @@ impl<'a, T> DerefMut for LockGuard<'a, T> { #[no_mangle] #[inline(always)] pub unsafe extern "C" fn __sync_synchronize() {} + +#[cfg(test)] +mod tests { + use once_cell::sync::OnceCell; + + #[derive(Default)] + #[allow(dead_code)] + struct Storage([u32; 16 / 4]); + + #[test_case] + fn check_init_once(_: &mut crate::Gba) { + static CELL: OnceCell = OnceCell::new(); + + core::hint::black_box(CELL.get_or_init(Default::default)); + } + + #[test_case] + fn check_init_once_many(_: &mut crate::Gba) { + static CELL: OnceCell = OnceCell::new(); + + for _ in 0..1000 { + core::hint::black_box(CELL.get_or_init(Default::default)); + } + } +} From 7ade79e132034d7ab93486fe043e5fa881efb729 Mon Sep 17 00:00:00 2001 From: Corwin Date: Sat, 17 Feb 2024 20:04:03 +0000 Subject: [PATCH 05/10] update other projects --- examples/hyperspace-roll/Cargo.toml | 1 + examples/hyperspace-roll/src/save.rs | 16 +++++++--------- examples/the-dungeon-puzzlers-lament/Cargo.toml | 1 + examples/the-dungeon-puzzlers-lament/src/save.rs | 12 ++++++------ 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/examples/hyperspace-roll/Cargo.toml b/examples/hyperspace-roll/Cargo.toml index d84b04d0..7ed75561 100644 --- a/examples/hyperspace-roll/Cargo.toml +++ b/examples/hyperspace-roll/Cargo.toml @@ -6,6 +6,7 @@ edition = "2021" [dependencies] agb = { version = "0.19.1", path = "../../agb" } +portable-atomic = { version = "1.6.0", default-features = false } [profile.dev] opt-level = 3 diff --git a/examples/hyperspace-roll/src/save.rs b/examples/hyperspace-roll/src/save.rs index e2154a87..1554e195 100644 --- a/examples/hyperspace-roll/src/save.rs +++ b/examples/hyperspace-roll/src/save.rs @@ -1,8 +1,8 @@ use agb::save::{Error, SaveManager}; -use agb::sync::Static; use agb::Gba; +use portable_atomic::{AtomicU32, Ordering}; -static HIGH_SCORE: Static = Static::new(0); +static HIGH_SCORE: AtomicU32 = AtomicU32::new(0); pub fn init_save(gba: &mut Gba) -> Result<(), Error> { gba.save.init_sram(); @@ -21,24 +21,22 @@ pub fn init_save(gba: &mut Gba) -> Result<(), Error> { access.read(1, &mut buffer)?; let high_score = u32::from_le_bytes(buffer); - if high_score > 100 { - HIGH_SCORE.write(0) - } else { - HIGH_SCORE.write(high_score) - } + let score = if high_score > 100 { 0 } else { high_score }; + + HIGH_SCORE.store(score, Ordering::SeqCst); } Ok(()) } pub fn load_high_score() -> u32 { - HIGH_SCORE.read() + HIGH_SCORE.load(Ordering::SeqCst) } pub fn save_high_score(save: &mut SaveManager, score: u32) -> Result<(), Error> { save.access()? .prepare_write(1..5)? .write(1, &score.to_le_bytes())?; - HIGH_SCORE.write(score); + HIGH_SCORE.store(score, Ordering::SeqCst); Ok(()) } diff --git a/examples/the-dungeon-puzzlers-lament/Cargo.toml b/examples/the-dungeon-puzzlers-lament/Cargo.toml index 90644ac3..9865ba2a 100644 --- a/examples/the-dungeon-puzzlers-lament/Cargo.toml +++ b/examples/the-dungeon-puzzlers-lament/Cargo.toml @@ -10,6 +10,7 @@ edition = "2021" agb = { version = "0.19.1", path = "../../agb" } agb_tracker = { version = "0.19.1", path = "../../tracker/agb-tracker", default-features = false, features = ["xm"] } slotmap = { version = "1", default-features = false } +portable-atomic = { version = "1.6.0", default-features = false } [profile.dev] opt-level = 3 diff --git a/examples/the-dungeon-puzzlers-lament/src/save.rs b/examples/the-dungeon-puzzlers-lament/src/save.rs index a47a510d..d6830b9d 100644 --- a/examples/the-dungeon-puzzlers-lament/src/save.rs +++ b/examples/the-dungeon-puzzlers-lament/src/save.rs @@ -1,10 +1,10 @@ use agb::{ save::{Error, SaveManager}, - sync::Static, Gba, }; +use portable_atomic::{AtomicU32, Ordering}; -static MAXIMUM_LEVEL: Static = Static::new(0); +static MAXIMUM_LEVEL: AtomicU32 = AtomicU32::new(0); pub fn init_save(gba: &mut Gba) -> Result<(), Error> { gba.save.init_sram(); @@ -24,9 +24,9 @@ pub fn init_save(gba: &mut Gba) -> Result<(), Error> { let max_level = u32::from_le_bytes(buffer); if max_level > 100 { - MAXIMUM_LEVEL.write(0) + MAXIMUM_LEVEL.store(0, Ordering::SeqCst) } else { - MAXIMUM_LEVEL.write(max_level) + MAXIMUM_LEVEL.store(max_level, Ordering::SeqCst) } } @@ -34,13 +34,13 @@ pub fn init_save(gba: &mut Gba) -> Result<(), Error> { } pub fn load_max_level() -> u32 { - MAXIMUM_LEVEL.read() + MAXIMUM_LEVEL.load(Ordering::SeqCst) } pub fn save_max_level(save: &mut SaveManager, level: u32) -> Result<(), Error> { save.access()? .prepare_write(1..5)? .write(1, &level.to_le_bytes())?; - MAXIMUM_LEVEL.write(level); + MAXIMUM_LEVEL.store(level, Ordering::SeqCst); Ok(()) } From d6b32e511db7b7f8b6f223cbe8d61ad767cae6cb Mon Sep 17 00:00:00 2001 From: Corwin Date: Tue, 9 Apr 2024 21:57:26 +0100 Subject: [PATCH 06/10] pub them in external mod --- agb/src/lib.rs | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/agb/src/lib.rs b/agb/src/lib.rs index 9d5f72ed..26109560 100644 --- a/agb/src/lib.rs +++ b/agb/src/lib.rs @@ -189,6 +189,12 @@ pub use no_game::no_game; pub(crate) mod arena; mod global_asm; +pub mod external { + pub use critical_section; + pub use once_cell; + pub use portable_atomic; +} + pub use {agb_alloc::ExternalAllocator, agb_alloc::InternalAllocator}; #[cfg(not(any(test, feature = "testing")))] From 32eb34f22620c8e9f207fe33b7c92e91aef7daa9 Mon Sep 17 00:00:00 2001 From: Corwin Date: Tue, 9 Apr 2024 21:58:49 +0100 Subject: [PATCH 07/10] fix panic renderer --- agb/src/panics_render.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/agb/src/panics_render.rs b/agb/src/panics_render.rs index e8edd5ca..190463e9 100644 --- a/agb/src/panics_render.rs +++ b/agb/src/panics_render.rs @@ -6,13 +6,13 @@ use crate::{ backtrace, display::{bitmap3::Bitmap3, busy_wait_for_vblank, HEIGHT, WIDTH}, dma::dma3_exclusive, - interrupt, mgba, syscall, + mgba, syscall, }; mod text; pub fn render_backtrace(trace: &backtrace::Frames, info: &PanicInfo) -> ! { - interrupt::free(|_cs| { + critical_section::with(|_cs| { dma3_exclusive(|| { // SAFETY: This is not fine, but we're crashing anyway. The loop at the end should stop anything bad happening let mut gba = unsafe { crate::Gba::new_in_entry() }; From c632eb4ea32cdd9e02374227c8c5a20592661015 Mon Sep 17 00:00:00 2001 From: Corwin Date: Tue, 9 Apr 2024 22:03:33 +0100 Subject: [PATCH 08/10] type use of rng --- agb/src/rng.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/agb/src/rng.rs b/agb/src/rng.rs index 320d2955..069dc5f7 100644 --- a/agb/src/rng.rs +++ b/agb/src/rng.rs @@ -54,8 +54,9 @@ impl Default for RandomNumberGenerator { } } -static GLOBAL_RNG: AtomicU128 = - AtomicU128::new(unsafe { core::mem::transmute(RandomNumberGenerator::new().state) }); +static GLOBAL_RNG: AtomicU128 = AtomicU128::new(unsafe { + core::mem::transmute::<[u32; 4], u128>(RandomNumberGenerator::new().state) +}); /// Using a global random number generator, provides the next random number #[must_use] @@ -64,7 +65,10 @@ pub fn gen() -> i32 { let data_u32: [u32; 4] = unsafe { core::mem::transmute(data) }; let mut rng = RandomNumberGenerator { state: data_u32 }; let value = rng.gen(); - GLOBAL_RNG.store(unsafe { core::mem::transmute(rng.state) }, Ordering::SeqCst); + GLOBAL_RNG.store( + unsafe { core::mem::transmute::<[u32; 4], u128>(rng.state) }, + Ordering::SeqCst, + ); value } From a0895635abff3eba57dca725ce7aa18d50ef2ad2 Mon Sep 17 00:00:00 2001 From: Corwin Date: Tue, 9 Apr 2024 22:08:55 +0100 Subject: [PATCH 09/10] use portable atomic from agb --- examples/hyperspace-roll/Cargo.toml | 1 - examples/hyperspace-roll/src/save.rs | 2 +- examples/the-dungeon-puzzlers-lament/Cargo.toml | 1 - examples/the-dungeon-puzzlers-lament/src/save.rs | 2 +- 4 files changed, 2 insertions(+), 4 deletions(-) diff --git a/examples/hyperspace-roll/Cargo.toml b/examples/hyperspace-roll/Cargo.toml index 7ed75561..d84b04d0 100644 --- a/examples/hyperspace-roll/Cargo.toml +++ b/examples/hyperspace-roll/Cargo.toml @@ -6,7 +6,6 @@ edition = "2021" [dependencies] agb = { version = "0.19.1", path = "../../agb" } -portable-atomic = { version = "1.6.0", default-features = false } [profile.dev] opt-level = 3 diff --git a/examples/hyperspace-roll/src/save.rs b/examples/hyperspace-roll/src/save.rs index 1554e195..ada5cf6a 100644 --- a/examples/hyperspace-roll/src/save.rs +++ b/examples/hyperspace-roll/src/save.rs @@ -1,6 +1,6 @@ +use agb::external::portable_atomic::{AtomicU32, Ordering}; use agb::save::{Error, SaveManager}; use agb::Gba; -use portable_atomic::{AtomicU32, Ordering}; static HIGH_SCORE: AtomicU32 = AtomicU32::new(0); diff --git a/examples/the-dungeon-puzzlers-lament/Cargo.toml b/examples/the-dungeon-puzzlers-lament/Cargo.toml index 9865ba2a..90644ac3 100644 --- a/examples/the-dungeon-puzzlers-lament/Cargo.toml +++ b/examples/the-dungeon-puzzlers-lament/Cargo.toml @@ -10,7 +10,6 @@ edition = "2021" agb = { version = "0.19.1", path = "../../agb" } agb_tracker = { version = "0.19.1", path = "../../tracker/agb-tracker", default-features = false, features = ["xm"] } slotmap = { version = "1", default-features = false } -portable-atomic = { version = "1.6.0", default-features = false } [profile.dev] opt-level = 3 diff --git a/examples/the-dungeon-puzzlers-lament/src/save.rs b/examples/the-dungeon-puzzlers-lament/src/save.rs index d6830b9d..5923fa5c 100644 --- a/examples/the-dungeon-puzzlers-lament/src/save.rs +++ b/examples/the-dungeon-puzzlers-lament/src/save.rs @@ -1,8 +1,8 @@ +use agb::external::portable_atomic::{AtomicU32, Ordering}; use agb::{ save::{Error, SaveManager}, Gba, }; -use portable_atomic::{AtomicU32, Ordering}; static MAXIMUM_LEVEL: AtomicU32 = AtomicU32::new(0); From 413a88948814cde74e5788ea95001b42a92c87b7 Mon Sep 17 00:00:00 2001 From: Corwin Date: Tue, 9 Apr 2024 22:17:31 +0100 Subject: [PATCH 10/10] add changelog entry --- CHANGELOG.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d50f6adc..b99110fa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,10 +22,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - Many macros now emit statics rather than consts OR can be used as statics OR - have had examples changed to use statics. You should use statics where possble + have had examples changed to use statics. You should use statics where possible for assets as consts can lead to them being included multiple times in the ROM. - Fixnums are now implemented with `num_traits` trait definitions. +- Rather than having our own sync with Statics, use the standard portable + atomics crate. These are reexported for convenience. ## [0.19.1] - 2024/03/06