Merge pull request #5 from aqueductfluidics/eeprom-api

Extend EEPROM API with bounds check, slice methods

Cargo.toml

@@ -35,6 +35,7 @@ keywords = [
 
 [dependencies]
 cortex-m = "0.7"
+embedded-storage = "0.3"
 
 [dependencies.cortex-m-rt]
 version = "0.7"

bin/eeprom.c

@@ -28,7 +28,10 @@
  * SOFTWARE.
  */
 
-// To configure the EEPROM size, edit E2END in avr/eeprom.h.
+// To configure the EEPROM size, edit E2END in
+// the Rust module. The meaning of that E2END Rust
+// constant is the same as the meaning in the
+// official module.
 //
 // Generally you should avoid editing this code, unless you really
 // know what you're doing.
@@ -41,11 +44,6 @@
 // values from the Teensy cores project.
 #define FLASH_BASEADDR 0x601F0000
 #define FLASH_SECTORS  15
-#define E2END 0x437	// From avr/eeprom.h in upstream.
-
-#if E2END > (255*FLASH_SECTORS-1)
-#error "E2END is set larger than the maximum possible EEPROM size"
-#endif
 
 // Conversation about how this code works & what the upper limits are
 // https://forum.pjrc.com/threads/57377?p=214566&viewfull=1#post214566
@@ -81,7 +79,6 @@ uint8_t eeprom_read_byte(const uint8_t *addr_ptr)
 	const uint16_t *p, *end;
 	uint8_t data=0xFF;
 
-	if (addr > E2END) return 0xFF;
 	if (!initialized) eeprom_initialize();
 	sector = (addr >> 2) % FLASH_SECTORS;
 	offset = (addr & 3) | (((addr >> 2) / FLASH_SECTORS) << 2);
@@ -104,7 +101,6 @@ void eeprom_write_byte(uint8_t *addr_ptr, uint8_t data)
 	uint8_t olddata=0xFF;
 	uint8_t buf[256];
 
-	if (addr > E2END) return;
 	if (!initialized) eeprom_initialize();
 
 	sector = (addr >> 2) % FLASH_SECTORS; 

examples/eeprom.rs

@@ -19,6 +19,7 @@ use teensy4_bsp as bsp;
 
 const PERIOD_MS: u32 = 1_000;
 const EEPROM_RW_ADDRESS: usize = 42;
+const EEPROM_RW_SLICE_ADDRESS: usize = 314;
 const EEPROM_PERSISTENCE_ADDRESS: usize = 137;
 const EEPROM_SENTINEL: u8 = 42;
 
@@ -32,7 +33,7 @@ fn main() -> ! {
     let mut led = bsp::configure_led(pins.p13);
 
     let mut eeprom = bsp::Eeprom::new().unwrap();
-    if EEPROM_SENTINEL == eeprom.read_byte(EEPROM_PERSISTENCE_ADDRESS) {
+    if EEPROM_SENTINEL == eeprom.read_byte(EEPROM_PERSISTENCE_ADDRESS).unwrap() {
         // Run this example at least once. Then, power cycle your Teensy4. You
         // should see this branch of code is hit (manifests as the LED turns on
         // immediately, instead of after one second.)
@@ -41,15 +42,50 @@ fn main() -> ! {
     } else {
         log::warn!("No EEPROM_SENTINEL");
     }
-    eeprom.write_byte(EEPROM_PERSISTENCE_ADDRESS, EEPROM_SENTINEL);
+    eeprom
+        .write_byte(EEPROM_PERSISTENCE_ADDRESS, EEPROM_SENTINEL)
+        .unwrap();
 
     loop {
         systick.delay_ms(PERIOD_MS);
         led.toggle();
 
-        let mut rw_val = eeprom.read_byte(EEPROM_RW_ADDRESS);
+        let mut rw_val = eeprom.read_byte(EEPROM_RW_ADDRESS).unwrap();
         log::info!("Incrementing {rw_val}...");
         rw_val = rw_val.wrapping_add(1);
-        eeprom.write_byte(EEPROM_RW_ADDRESS, rw_val);
+        eeprom.write_byte(EEPROM_RW_ADDRESS, rw_val).unwrap();
+
+        if rw_val % 7 == 0 {
+            log::warn!(
+                "Intentionally reading out of bounds... {:?}",
+                eeprom.read_byte(0xDEADBEEF)
+            );
+            log::warn!(
+                "Intentionally writing out of bounds... {:?}",
+                eeprom.read_byte(0xDEADBEEF)
+            );
+            log::warn!("Intentionally reading slice of bounds... {:?}", {
+                let mut buffer = [0u8; 7];
+                eeprom.read_bytes_exact(1080, &mut buffer)
+            });
+            log::warn!("Intentionally writing slice of bounds... {:?}", {
+                let buffer = [0u8; 7];
+                eeprom.write_bytes_exact(1080, &buffer)
+            });
+        }
+
+        if rw_val % 3 == 0 {
+            let mut buffer = [0u8; 7];
+            eeprom
+                .read_bytes_exact(EEPROM_RW_SLICE_ADDRESS, &mut buffer)
+                .unwrap();
+            log::info!("Incrementing buffer {:?}", buffer);
+            buffer
+                .iter_mut()
+                .for_each(|item| *item = item.wrapping_add(1));
+            eeprom
+                .write_bytes_exact(EEPROM_RW_SLICE_ADDRESS, &buffer)
+                .unwrap();
+        }
     }
 }

src/eeprom.rs

@@ -2,24 +2,78 @@
 //!
 //! EEPROM emulation uses a small (~1KiB) region of flash to persist data.
 
-use core::sync::atomic::{AtomicBool, Ordering};
+use core::{
+    ffi::c_void,
+    sync::atomic::{AtomicBool, Ordering},
+};
 
 #[cfg_attr(target_arch = "arm", link(name = "t4eeprom"))]
 extern "C" {
     pub fn eeprom_initialize();
     pub fn eeprom_read_byte(addr_ptr: *const u8) -> u8;
     pub fn eeprom_write_byte(addr_ptr: *const u8, data: u8);
-    // TODO the rest...
+    pub fn eeprom_read_block(buf: *mut c_void, addr_ptr: *const c_void, len: u32);
+    pub fn eeprom_write_block(buf: *const c_void, addr_ptr: *mut c_void, len: u32);
 }
 
 static TAKEN: AtomicBool = AtomicBool::new(false);
 
+/// Possible errors encountered when interacting with EEPROM.
+#[non_exhaustive]
+#[derive(Debug)]
+pub enum EepromError {
+    /// The operation extends beyond the EEPROM range.
+    ///
+    /// See [`EEPROM_CAPACITY`] for more information.
+    OutOfRange,
+}
+
+/// The EEPROM capacity, in bytes.
+///
+/// All values for `index` supplied to [`Eeprom`] should be less
+/// than this value. Otherwise, you'll observe [`EepromError::OutOfRange`].
+pub const EEPROM_CAPACITY: usize = E2END + 1;
+const E2END: usize = 0x437;
+
+type Result<T> = core::result::Result<T, EepromError>;
+
+/// This simulates the bounds check that was implemented in
+/// the official `eeprom.c` module. Unit tests demonstrate
+/// that it meets the behaviors documented in [`EEPROM_CAPACITY`].
+const fn bounds_check_scalar(addr: usize) -> Result<()> {
+    if addr > E2END {
+        Err(EepromError::OutOfRange)
+    } else {
+        Ok(())
+    }
+}
+
+fn bounds_check_slice<T>(addr: usize, slice: &[T]) -> Result<()> {
+    bounds_check_scalar(addr)?;
+    bounds_check_scalar((addr + slice.len()).saturating_sub(1))?;
+    Ok(())
+}
+
 /// Provides read/write access to EEPROM
 ///
 /// There's only one of these available in a given program.
+/// It implements some of the basic `embedded_storage` traits;
+/// consider using these for generality.
 pub struct Eeprom(());
 
 impl Eeprom {
+    // The safety of this implementation depends on
+    //
+    // 1. Bounds checks happening before we read / write to
+    //    emulated EEPROM.
+    // 2. These functions are only accessed from one execution context.
+    //
+    // We ensure 2 by construction of `new()`. We ensure 1 by construction
+    // of I/O methods.
+    //
+    // Refer to this notice when you see undocumented `unsafe` code in
+    // this block.
+
     /// Create an `Eeprom` that controls I/O with the EEPROM emulation region
     ///
     /// Returns `None` if the `Eeprom` has already been created.
@@ -32,16 +86,104 @@ impl Eeprom {
             Some(Eeprom(()))
         }
     }
-    /// Read a byte from the EEPROM emulation region
+    /// Read a byte from the EEPROM emulation region.
+    pub fn read_byte(&self, index: usize) -> Result<u8> {
+        bounds_check_scalar(index)?;
+        Ok(unsafe { eeprom_read_byte(index as _) })
+    }
+    /// Write a byte into the EEPROM emulated region.
+    pub fn write_byte(&mut self, index: usize, byte: u8) -> Result<()> {
+        bounds_check_scalar(index)?;
+        Ok(unsafe { eeprom_write_byte(index as _, byte) })
+    }
+
+    /// Read the exact number of bytes required to fill `buffer`, starting from `index`.
     ///
-    /// TODO what happens when out of range? Should handle here...
-    pub fn read_byte(&self, index: usize) -> u8 {
-        unsafe { eeprom_read_byte(index as _) }
+    /// If the operation would exceed the EEPROM range, this method returns an error
+    /// and does nothing. This includes the case when `buffer` is empty. Otherwise,
+    /// if `buffer` is non-empty, it will be filled with the contents of storage.
+    pub fn read_bytes_exact(&self, index: usize, buffer: &mut [u8]) -> Result<()> {
+        bounds_check_slice(index, buffer)?;
+        if !buffer.is_empty() {
+            unsafe {
+                eeprom_read_block(
+                    buffer.as_mut_ptr() as *mut _,
+                    index as _,
+                    buffer.len() as u32,
+                )
+            }
+        }
+        Ok(())
     }
-    /// Write a byte into the EEPROM emulated region
+
+    /// Write all of `buffer` to storage, starting at `index`.
     ///
-    /// TODO what happens when out of range? Should handle here...
-    pub fn write_byte(&mut self, index: usize, byte: u8) {
-        unsafe { eeprom_write_byte(index as _, byte) };
+    /// If the operation would exceed the EEPROM range, this method returns an error
+    /// and does nothing. This includes the case when `buffer` is empty. Otherwise,
+    /// if `buffer` is non-empty, its contents are written to storage.
+    pub fn write_bytes_exact(&mut self, index: usize, buffer: &[u8]) -> Result<()> {
+        bounds_check_slice(index, buffer)?;
+        if !buffer.is_empty() {
+            unsafe {
+                eeprom_write_block(buffer.as_ptr() as *const _, index as _, buffer.len() as u32)
+            }
+        }
+        Ok(())
+    }
+}
+
+impl embedded_storage::ReadStorage for Eeprom {
+    type Error = EepromError;
+    fn read(&mut self, offset: u32, bytes: &mut [u8]) -> Result<()> {
+        self.read_bytes_exact(offset as usize, bytes)
+    }
+    fn capacity(&self) -> usize {
+        EEPROM_CAPACITY
+    }
+}
+
+impl embedded_storage::Storage for Eeprom {
+    fn write(&mut self, offset: u32, bytes: &[u8]) -> core::result::Result<(), Self::Error> {
+        self.write_bytes_exact(offset as usize, bytes)
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::{bounds_check_scalar, bounds_check_slice};
+
+    #[test]
+    fn scalar_ok() {
+        assert!(bounds_check_scalar(0).is_ok());
+        assert!(bounds_check_scalar(1079).is_ok());
+        assert!(bounds_check_scalar(1080).is_err());
+        assert!(bounds_check_scalar(9999).is_err());
+    }
+
+    #[test]
+    fn slice_ok() {
+        assert!(bounds_check_slice(0, &[1]).is_ok());
+        assert!(bounds_check_slice(1079, &[1]).is_ok());
+        assert!(bounds_check_slice(1080, &[1]).is_err());
+
+        let buffer = [0u8; 13];
+        assert!(bounds_check_slice(1070, &buffer).is_err());
+        assert!(bounds_check_slice(1068, &buffer).is_err());
+        assert!(bounds_check_slice(1067, &buffer).is_ok());
+        assert!(bounds_check_slice(314, &buffer).is_ok());
+
+        static LARGE_BUFFER: [u8; 1080] = [0; 1080];
+        assert!(bounds_check_slice(0, &LARGE_BUFFER).is_ok());
+        assert!(bounds_check_slice(1, &LARGE_BUFFER).is_err());
+
+        static ALMOST_LARGE_BUFFER: [u8; 1079] = [0; 1079];
+        assert!(bounds_check_slice(0, &ALMOST_LARGE_BUFFER).is_ok());
+        assert!(bounds_check_slice(1, &ALMOST_LARGE_BUFFER).is_ok());
+        assert!(bounds_check_slice(2, &ALMOST_LARGE_BUFFER).is_err());
+
+        // Though there's nothing to read or write, we'll still
+        // signal an error.
+        const EMPTY: &[()] = &[];
+        assert!(bounds_check_slice(1080, EMPTY).is_err());
     }
 }

src/lib.rs

@@ -92,7 +92,7 @@ mod rt;
 pub mod usb;
 
 mod eeprom;
-pub use eeprom::Eeprom;
+pub use eeprom::{Eeprom, EepromError, EEPROM_CAPACITY};
 #[cfg(all(target_arch = "arm", feature = "rt"))]
 pub use rt::{dtcm_heap_start, heap_len, heap_start};