Update the code style of the save module to better match agb's philosophy.

agb/src/lib.rs

@@ -168,7 +168,6 @@ pub use agb_fixnum as fixnum;
 pub mod hash_map;
 /// Simple random number generator
 pub mod rng;
-/// Implements save games.
 pub mod save;
 mod single;
 /// Implements sound output.

agb/src/save/mod.rs

@@ -1,9 +1,5 @@
 //! Module for reading and writing to save media.
 //!
-//! This module provides both specific interfaces that directly access particular
-//! types of save media, and an abstraction layer that allows access to all kinds
-//! of save media using a shared interface.
-//!
 //! ## Save media types
 //!
 //! There are, broadly speaking, three different kinds of save media that can be
@@ -35,11 +31,6 @@
 //! * For 512 byte EEPROM, call [`use_eeprom_512b`].
 //! * For 8 KiB EEPROM, call [`use_eeprom_8k`].
 //!
-//! Then, call [`set_timer_for_timeout`] to set the timer you intend to use to
-//! track the timeout that prevents errors with the save media from hanging your
-//! game. For more information on GBA timers, see the [`timer`](`crate::timer`)
-//! module's documentation.
-//!
 //! TODO Update example
 //! ```rust,norun
 //! # use gba::save;
@@ -49,11 +40,11 @@
 //!
 //! ## Using save media
 //!
-//! To access save media, use the [`SaveAccess::new`] method to create a new
+//! To access save media, use the [`SaveData::new`] method to create a new
-//! [`SaveAccess`] object. Its methods are used to read or write save media.
+//! [`SaveData`] object. Its methods are used to read or write save media.
 //!
-//! Reading data from the savegame is simple. Use [`read`](`SaveAccess::read`)
+//! Reading data from the savegame is simple. Use [`read`] to copy data from an
-//! to copy data from an offset in the savegame into a buffer in memory.
+//! offset in the savegame into a buffer in memory.
 //!
 //! TODO Update example
 //! ```rust,norun
@@ -64,8 +55,8 @@
 //! ```
 //!
 //! Writing to save media requires you to prepare the area for writing by calling
-//! the [`prepare_write`](`SaveAccess::prepare_write`) method before doing the
+//! the [`prepare_write`] method to return a [`SavePreparedBlock`], which contains
-//! actual write commands with the [`write`](`SaveAccess::write`) method.
+//! the actual [`write`] method.
 //!
 //! TODO Update example
 //! ```rust,norun
@@ -84,48 +75,43 @@
 //!
 //! Because writes can only be prepared on a per-sector basis, a clear on a range
 //! of `4000..5000` on a device with 4096 byte sectors will actually clear a range
-//! of `0..8192`. Use [`sector_size`](`SaveAccess::sector_size`) to find the
+//! of `0..8192`. Use [`sector_size`] to find the sector size, or [`align_range`]
-//! sector size, or [`align_range`](`SaveAccess::align_range`) to directly
+//! to directly calculate the range of memory that will be affected by the clear.
-//! calculate the range of memory that will be affected by the clear.
+//!
+//! [`read`]: SaveData::read
+//! [`prepare_write`]: SaveData::prepare_write
+//! [`write`]: SavePreparedBlock::write
+//! [`sector_size`]: SaveAccess::sector_size
+//! [`align_range`]: SaveAccess::align_range
 //!
 //! ## Performance and Other Details
 //!
-//! Because `prepare_write` does nothing on non-flash chips, it would not cause
+//! The performance characteristics of the media types are as follows:
-//! correctness issues to ignore it. Even so, it is recommend to write code to
-//! use the `prepare_write` function regardless of the save media, as it has
-//! minimal runtime cost on other save media types. If needed, you can check if
-//! `prepare_write` is required by calling the
-//! (`requires_prepare_write`)(`SaveAccess::requires_prepare_write`) method.
 //!
-//! Some memory types have a `sector_size` above `1`, but do not use
+//! * SRAM is simply a form of battery backed memory, and has no particular
-//! `prepare_write`. This indicates that the media type has sectors that must
+//!   performance characteristics.  Reads and writes at any alignment are
-//! be rewritten all at once, instead of supporting the separate erase/write
+//!   efficient. Furthermore, no timer is needed for accesses to this type of
-//! cycles that flash media does. Writing non-sector aligned memory will be
+//!   media. `prepare_write` does not immediately erase any data.
-//! slower on such save media, as the implementation needs to read the old
+//! * Non-Atmel flash chips have a sector size of 4096 bytes. Reads and writes
-//! contents into a buffer before writing to avoid data loss.
+//!   to any alignment are efficient, however, `prepare_write` will erase all
-//!
+//!   data in an entire sector before writing.
-//! To summarize, for all supported media types:
+//! * Atmel flash chips have a sector size of 128 bytes. Reads to any alignment
-//!
+//!   are efficient, however, unaligned writes are extremely slow.
-//! * SRAM does not require `prepare_write` and has no sectors to align to. Reads
+//!   `prepare_write` does not immediately erase any data.
-//!   and writes at any alignment are efficient. Furthermore, it does not require
+//! * EEPROM has a sector size of 8 bytes. Unaligned reads and writes are
-//!   a timer to be set with [`set_timer_for_timeout`].
+//!   slower than aligned writes, however, this is easily mitigated by the
-//! * Non-Atmel flash chips requires `prepare_write`, and have sectors of 4096
+//!   small sector size.
-//!   bytes. Atmel flash chips instead do not require `prepare_write`, and instead
-//!   have sectors of 128 bytes. You should generally try to use `prepare_write`
-//!   regardless, and write in blocks of 128 bytes if at all possible.
-//! * EEPROM does not require `prepare_write` and has sectors of 8 bytes.
 
-use core::cell::Cell;
 use core::ops::Range;
-use bare_metal::Mutex;
+use crate::sync::{Mutex, RawMutexGuard};
+use crate::timer::Timer;
 
 mod asm_utils;
 //mod setup;
-//mod utils;
+mod utils;
 
 //pub use asm_utils::*;
 //pub use setup::*;
-//pub use utils::*;
 
 //pub mod eeprom;
 //pub mod flash;
@@ -171,6 +157,7 @@ pub enum Error {
 
 /// Information about the save media used.
 #[derive(Clone, Debug)]
+#[non_exhaustive]
 pub struct MediaInfo {
   /// The type of save media installed.
   pub media_type: MediaType,
@@ -181,75 +168,49 @@ pub struct MediaInfo {
   pub sector_shift: usize,
   /// The size of the save media, in sectors.
   pub sector_count: usize,
-  /// Whether the save media type requires the use of the
+  /// Whether the save media type requires media be prepared before writing.
-  /// [`prepare_write`](`SaveAccess::prepare_write`) function before a block of
+  pub uses_prepare_write: bool,
-  /// memory can be overwritten.
-  pub requires_prepare_write: bool,
 }
 
 /// A trait allowing low-level saving and writing to save media.
-///
+trait RawSaveAccess: Sync {
-/// It exposes an interface mostly based around the requirements of reading and
-/// writing flash memory, as those are the most restrictive.
-///
-/// This interface treats memory as a continuous block of bytes for purposes of
-/// reading, and as an array of sectors .
-pub trait RawSaveAccess: Sync {
-  /// Returns information about the save media used.
   fn info(&self) -> Result<&'static MediaInfo, Error>;
-
-  /// Reads a slice of memory from save media.
-  ///
-  /// This will attempt to fill `buffer` entirely, and will error if this is
-  /// not possible. The contents of `buffer` are unpredictable if an error is
-  /// returned.
   fn read(&self, offset: usize, buffer: &mut [u8]) -> Result<(), Error>;
-
-  /// Verifies that the save media has been successfully written, comparing
-  /// it against the given buffer.
   fn verify(&self, offset: usize, buffer: &[u8]) -> Result<bool, Error>;
-
-  /// Prepares a given span of sectors for writing. This may permanently erase
-  /// the current contents of the sector on some save media.
   fn prepare_write(&self, sector: usize, count: usize) -> Result<(), Error>;
-
-  /// Writes a buffer to the save media.
-  ///
-  /// The sectors you are writing to must be prepared with a call to the
-  /// `prepare_write` function beforehand, or else the contents of the save
-  /// media may be unpredictable after writing.
   fn write(&self, offset: usize, buffer: &[u8]) -> Result<(), Error>;
 }
 
-/// Contains the current save media implementation.
+static CURRENT_SAVE_ACCESS: Mutex<Option<&'static dyn RawSaveAccess>> = Mutex::new(None);
-static CURRENT_SAVE_ACCESS: Mutex<Cell<Option<&'static dyn RawSaveAccess>>> =
-  Mutex::new(Cell::new(None));
 
-/// Sets the save media implementation in use.
+fn set_save_implementation(access_impl: &'static dyn RawSaveAccess) {
-pub fn set_save_implementation(access: Option<&'static dyn RawSaveAccess>) {
+  let mut access = CURRENT_SAVE_ACCESS.lock();
-  crate::interrupt::free(|c| {
+  assert!(access.is_none(), "Cannot initialize the savegame engine more than once.");
-    CURRENT_SAVE_ACCESS.borrow(c).set(access)
+  *access = Some(access_impl);
-  })
 }
 
-/// Gets the save media implementation in use.
+fn get_save_implementation() -> Option<&'static dyn RawSaveAccess> {
-pub fn get_save_implementation() -> Option<&'static dyn RawSaveAccess> {
+  *CURRENT_SAVE_ACCESS.lock()
-  crate::interrupt::free(|c| {
-    CURRENT_SAVE_ACCESS.borrow(c).get()
-  })
 }
 
 /// Allows reading and writing of save media.
 #[derive(Copy, Clone)]
-pub struct SaveAccess {
+pub struct SaveData {
+  lock: RawMutexGuard<'static>,
   access: &'static dyn RawSaveAccess,
   info: &'static MediaInfo,
+  timeout: utils::Timeout,
 }
-impl SaveAccess {
+impl SaveData {
   /// Creates a new save accessor around the current save implementaiton.
-  pub fn new() -> Result<SaveAccess, Error> {
+  fn new(timer: Option<Timer>) -> Result<SaveData, Error> {
     match get_save_implementation() {
-      Some(access) => Ok(SaveAccess { access, info: access.info()? }),
+      Some(access) => Ok(SaveData {
+        lock: utils::lock_media()?,
+        access,
+        info: access.info()?,
+        timeout: utils::Timeout::new(timer),
+      }),
       None => Err(Error::NoMedia),
     }
   }
@@ -275,21 +236,31 @@ impl SaveAccess {
     self.info.sector_count << self.info.sector_shift
   }
 
+  fn check_bounds(&self, range: Range<usize>) -> Result<(), Error> {
+    if range.start >= self.len() || range.end >= self.len() {
+      Err(Error::OutOfBounds)
+    } else {
+      Ok(())
+    }
+  }
+  fn check_bounds_len(&self, offset: usize, len: usize) -> Result<(), Error> {
+    self.check_bounds(offset..(offset + len))
+  }
+
   /// Copies data from the save media to a buffer.
+  ///
+  /// If an error is returned, the contents of the buffer are unpredictable.
   pub fn read(&self, offset: usize, buffer: &mut [u8]) -> Result<(), Error> {
+    self.check_bounds_len(offset, buffer.len())?;
     self.access.read(offset, buffer)
   }
 
   /// Verifies that a given block of memory matches the save media.
   pub fn verify(&self, offset: usize, buffer: &[u8]) -> Result<bool, Error> {
+    self.check_bounds_len(offset, buffer.len())?;
     self.access.verify(offset, buffer)
   }
 
-  /// Returns whether this save media requires the use of [`SaveAccess::prepare_write`].
-  pub fn requires_prepare_write(&self) -> bool {
-    self.info.requires_prepare_write
-  }
-
   /// Returns a range that contains all sectors the input range overlaps.
   ///
   /// This can be used to calculate which blocks would be erased by a call
@@ -305,41 +276,69 @@ impl SaveAccess {
   /// This will erase any data in any sector overlapping the input range. To
   /// calculate which offset ranges would be affected, use the
   /// [`align_range`](`SaveAccess::align_range`) function.
-  pub fn prepare_write(&self, range: Range<usize>) -> Result<(), Error> {
+  pub fn prepare_write(&mut self, range: Range<usize>) -> Result<SavePreparedBlock, Error> {
-    if self.info.requires_prepare_write {
+    self.check_bounds(range.clone())?;
-      let range = self.align_range(range);
+    if self.info.uses_prepare_write {
+      let range = self.align_range(range.clone());
       let shift = self.info.sector_shift;
-      self.access.prepare_write(range.start >> shift, range.len() >> shift)
+      self.access.prepare_write(range.start >> shift, range.len() >> shift)?;
-    } else {
+    }
-      Ok(())
+    Ok(SavePreparedBlock {
+      parent: self,
+      range
+    })
   }
 }
 
+/// A block of save memory that has been prepared for writing.
+pub struct SavePreparedBlock<'a> {
+  parent: &'a mut SaveData,
+  range: Range<usize>,
+}
+impl<'a> SavePreparedBlock<'a> {
   /// Writes a given buffer into the save media.
   ///
-  /// If [`requires_prepare_write`](`SaveAccess::requires_prepare_write`) returns
+  /// Multiple overlapping writes to the same memory range without a separate
-  /// `true`, you must call [`prepare_write`](`SaveAccess::prepare_write`) on the
+  /// call to `prepare_write` will leave the save data in an unpredictable
-  /// range you intend to write for this to function correctly. The contents of
+  /// state. If an error is returned, the contents of the save media is
-  /// the save media are unpredictable if you do not.
+  /// unpredictable.
   pub fn write(&self, offset: usize, buffer: &[u8]) -> Result<(), Error> {
-    self.access.write(offset, buffer)
+    if buffer.len() == 0 {
+      Ok(())
+    } else if !self.range.contains(&offset) ||
+        !self.range.contains(&(offset + buffer.len() - 1)) {
+      Err(Error::OutOfBounds)
+    } else {
+      self.parent.access.write(offset, buffer)
+    }
   }
 
   /// Writes and validates a given buffer into the save media.
   ///
-  /// If [`requires_prepare_write`](`SaveAccess::requires_prepare_write`) returns
-  /// `true`, you must call [`prepare_write`](`SaveAccess::prepare_write`) on the
-  /// range you intend to write for this to function correctly. The contents of
-  /// the save media will be unpredictable if you do not.
-  ///
   /// This function will verify that the write has completed successfully, and
   /// return an error if it has not done so.
+  ///
+  /// Multiple overlapping writes to the same memory range without a separate
+  /// call to `prepare_write` will leave the save data in an unpredictable
+  /// state. If an error is returned, the contents of the save media is
+  /// unpredictable.
   pub fn write_and_verify(&self, offset: usize, buffer: &[u8]) -> Result<(), Error> {
     self.write(offset, buffer)?;
-    if !self.verify(offset, buffer)? {
+    if !self.parent.verify(offset, buffer)? {
       Err(Error::WriteError)
     } else {
       Ok(())
     }
   }
 }
+
+/// Allows access to the cartridge's save data.
+pub struct SaveManager;
+impl SaveManager {
+  pub fn access() -> Result<SaveData, Error> {
+    SaveData::new(None)
+  }
+  pub fn access_with_timer(timer: Timer) -> Result<SaveData, Error> {
+    SaveData::new(Some(timer))
+  }
+}

agb/src/save/utils.rs

@@ -0,0 +1,54 @@
+//! A package containing useful utilities for writing save accessors.
+
+use super::Error;
+use crate::sync::{RawMutex, RawMutexGuard};
+use crate::timer::{Timer, Divider};
+
+/// A timeout type used to prevent hardware errors in save media from hanging
+/// the game.
+pub struct Timeout {
+  timer: Option<Timer>,
+}
+impl Timeout {
+  /// Creates a new timeout from the timer passed to [`set_timer_for_timeout`].
+  ///
+  /// ## Errors
+  ///
+  /// If another timeout has already been created.
+  #[inline(never)]
+  pub fn new(timer: Option<Timer>) -> Self {
+    Timeout { timer }
+  }
+
+  /// Starts this timeout.
+  pub fn start(&mut self) {
+    if let Some(timer) = &mut self.timer {
+      timer.set_divider(Divider::Divider1024);
+      timer.set_interrupt(false);
+      timer.set_overflow_amount(0xFFFF);
+      timer.set_cascade(false);
+      timer.set_enabled(true);
+    }
+  }
+
+  /// Returns whether a number of milliseconds has passed since the last call
+  /// to [`Timeout::start()`].
+  pub fn check_timeout_met(&self, check_ms: u16) -> bool {
+    if let Some(timer) = &self.timer {
+      check_ms * 17 < timer.value()
+    } else {
+      false
+    }
+  }
+}
+
+/// Tries to obtain a lock on the global lock for save operations.
+///
+/// This is used to prevent problems with stateful save media.
+pub fn lock_media() -> Result<RawMutexGuard<'static>, Error> {
+  static LOCK: RawMutex = RawMutex::new();
+  match LOCK.try_lock() {
+    Some(x) => Ok(x),
+    None => Err(Error::MediaInUse),
+  }
+}