Weird unaligned write in embedded Rust

[Oakchris1955]

I have written a Flash struct that implements the Read, Write and Read traits for the Raspberry Pico's flash memory. I am trying to format the volume using fatfs::format_volume with the following options:

let options = fatfs::FormatVolumeOptions::new()
    .bytes_per_sector(4096)
    .bytes_per_cluster(4096);

and after a while, my write function returns an error that "The cursor is not aligned to SECTOR_SIZE" (that error type is mine). Furthermore, it seems like this specific error occurs after the library zeroes a bunch of memory at the beginning of the flash, when it tries to write the buffer [255, 255] to the address 4097, just 1 byte off from the start of the 2nd sector.

Additional info

• I am not doing any kind of buffering, everything is flushed immediately
• I am using C bindings to the RP2040 ROM functions described in section 2.8.3.1.3, page 136 of the RP2040 datasheet
• I have enabled the alloc feature, but that doesn't seem to change the program's behaviour
• Another issue I have found is that the size of the buffer data to write isn't a multiple of SECTOR_SIZE. Is this intended behavior?

Code

use alloc::format;
use core::{cmp, slice};

// custom error types
use crate::error;
use error::{Error, ErrorKind, FlashErrorKind};

use fatfs::{self, SeekFrom};
// ROM functions
use rp_pico::hal::rom_data::{
    connect_internal_flash, flash_enter_cmd_xip, flash_exit_xip, flash_flush_cache,
    flash_range_erase, flash_range_program,
};

// SECTOR_SIZE is already a multiple of PAGE_SIZE (256), so we use that instead
pub const SECTOR_SIZE: usize = 4096;
pub const SECTOR_ERASE: u8 = 0x20;
pub const XIP_BASE: usize = 0x1000_0000;

pub struct Flash<'a> {
    pub slice: &'a [u8],
    pub offset: usize,
}

impl<'a> Flash<'a> {
    /// Returns `None` if offset isn't aligned to a sector
    pub fn new(offset: usize, len: usize) -> Option<Self> {
        let slice = unsafe { slice::from_raw_parts((XIP_BASE + offset) as *const u8, len) };
        let flash = Flash { slice, offset: 0 };

        if flash.is_aligned()
            && slice.len() % SECTOR_SIZE == 0
            && slice.as_ptr() as usize >= XIP_BASE
        {
            Some(flash)
        } else {
            None
        }
    }

    pub fn len(&self) -> usize {
        self.slice.len()
    }

    fn is_aligned(&self) -> bool {
        // Below we are not using that pointer to read/write anything, so this is actually safe
        unsafe { self.slice.as_ptr().add(self.offset) }.align_offset(SECTOR_SIZE) == 0
    }

    fn offset_from_xip_base(&self) -> usize {
        self.slice.as_ptr() as usize - XIP_BASE
    }
}

/// Copy the contents of `src` to `dest` and return how many bytes were read
///
/// Note: unlike the `copy_from_slice` function, this doesn't panic;
/// instead, this will cap at the when an end-of-slice is reached for either `src` or `dst`
fn slice_copy<T>(src: &[T], dest: &mut [T]) -> usize
where
    T: Copy,
{
    let src_len = src.len();
    let dest_len = dest.len();
    let size_cap = cmp::min(src_len, dest_len);

    // Perform copy operation
    dest[..size_cap].copy_from_slice(&src[..size_cap]);

    return size_cap;
}

#[derive(Debug)]
pub struct IoError(Error);

impl From<Error> for IoError {
    fn from(value: Error) -> Self {
        Self(value)
    }
}

impl fatfs::IoBase for Flash<'_> {
    type Error = IoError;
}

impl fatfs::IoError for IoError {
    fn is_interrupted(&self) -> bool {
        self.0.kind() == ErrorKind::Interrupted
    }

    fn new_unexpected_eof_error() -> Self {
        Self(Error::new(
            ErrorKind::UnexpectedEof,
            "Encountered unexpected EOF while doing FS operations",
        ))
    }

    fn new_write_zero_error() -> Self {
        Self(Error::new(
            ErrorKind::WriteZero,
            "A FS operation could not be completed because a call to .write() returned Ok(0)",
        ))
    }
}

impl fatfs::Read for Flash<'_> {
    // `io::Result` always returns `Ok`, so you can safely unwrap this
    fn read(&mut self, buf: &mut [u8]) -> Result<usize, Self::Error> {
        defmt::debug!("Read {} bytes from offset {}", buf.len(), self.offset);
        if self.offset >= self.slice.len() {
            unreachable!()
        } else {
            let bytes_read = slice_copy(&self.slice[self.offset..], buf);
            self.offset += bytes_read;
            Ok(bytes_read)
        }
    }
}

impl fatfs::Seek for Flash<'_> {
    fn seek(&mut self, pos: fatfs::SeekFrom) -> Result<u64, Self::Error> {
        defmt::debug!(
            "current pos: {}, seekfrom: {:?}",
            self.offset,
            defmt::Debug2Format(&pos)
        );
        match pos {
            SeekFrom::Start(offset) => {
                let offset: usize = offset.try_into().map_err(|_err| {
                    Error::new(
                        ErrorKind::TryFromIntError,
                        "New offset must fit into a usize",
                    )
                })?;

                if offset < self.slice.len() {
                    self.offset = offset;
                } else {
                    return Err(Error::new(
                        ErrorKind::FlashRelated(FlashErrorKind::MovedPast),
                        "Moved past end of managed flash",
                    )
                    .into());
                }
            }
            SeekFrom::Current(offset) => {
                let offset: i64 =
                    offset
                        .checked_add_unsigned(self.offset as u64)
                        .ok_or(Error::new(
                            ErrorKind::CheckedOverflow,
                            "Numeric overflow between addition of i64 and usize",
                        ))?;

                match u64::try_from(offset) {
                    Ok(offset) => self.offset = self.seek(SeekFrom::Start(offset))? as usize,
                    Err(_err) => {
                        return Err(Error::new(
                            ErrorKind::FlashRelated(FlashErrorKind::MovedBefore),
                            "Moved before start of managed flash",
                        )
                        .into())
                    }
                };
            }
            SeekFrom::End(offset) => {
                let offset: i64 =
                    offset
                        .checked_add_unsigned(self.slice.len() as u64)
                        .ok_or(Error::new(
                            ErrorKind::CheckedOverflow,
                            "Numeric overflow between addition of i64 and usize",
                        ))?
                        - 1;

                match u64::try_from(offset) {
                    Ok(offset) => self.offset = self.seek(SeekFrom::Start(offset))? as usize,
                    Err(_err) => {
                        return Err(Error::new(
                            ErrorKind::FlashRelated(FlashErrorKind::MovedBefore),
                            "Moved before start of managed flash",
                        )
                        .into())
                    }
                };
            }
        }

        defmt::debug!("new pos: {}", self.offset);

        Ok(self.offset as u64)
    }
}

impl fatfs::Write for Flash<'_> {
    // Please note that if writing the entire buffer would exceed the limits
    // of the managed flash space, everything UP TO that point will be written
    #[inline(never)]
    #[link_section = ".data.ram_func"]
    fn write(&mut self, buf: &[u8]) -> Result<usize, Self::Error> {
        defmt::debug!("{}: {:?}", self.offset, buf);
        if !self.is_aligned() {
            Err(Error::new(
                ErrorKind::FlashRelated(FlashErrorKind::Unaligned),
                "The cursor is not aligned to SECTOR_SIZE",
            )
            .into())
        }
        // the issue with the buf len not being a multiple of SECTOR_SIZE
        /*else if buf.len() % SECTOR_SIZE != 0 {
            Err(Error::new(
                ErrorKind::FlashRelated(FlashErrorKind::InvalidBufferSize),
                format!(
                    "The length of the input buffer ({}) isn't a multiple of SECTOR SIZE ({})",
                    buf.len(),
                    SECTOR_SIZE
                ),
            )
            .into())
        } */
        else {
            // Prevent flash memory overflow by taking a slice of the input buffer
            let init_buf_slice = &buf[..cmp::min(self.slice.len() - self.offset, buf.len())];

            // the issue with the buf len not being a multiple of SECTOR_SIZE
            // Zero out remaining bytes so that our buffer is divisible by SECTOR_SIZE
            let bytes_needed =
                init_buf_slice.len().next_multiple_of(SECTOR_SIZE) - init_buf_slice.len();
            let mut filler_vec: heapless::Vec<u8, SECTOR_SIZE> = heapless::Vec::new();
            filler_vec.resize(bytes_needed, 0).unwrap();

            let buf_vec = [init_buf_slice, filler_vec.as_slice()].concat();
            let buf_slice = buf_vec.as_slice();

            // Variation of what is described in https://github.com/rp-rs/rp-hal/issues/257
            unsafe {
                cortex_m::interrupt::free(|_cs| {
                    connect_internal_flash();
                    flash_exit_xip();

                    // Erase the data in that region
                    flash_range_erase(
                        self.offset_from_xip_base() as u32,
                        buf_slice.len(),
                        SECTOR_SIZE as u32,
                        0xD8,
                    );

                    // Reprogram the data in that region
                    flash_range_program(
                        self.offset_from_xip_base() as u32,
                        buf_slice.as_ptr(),
                        buf_slice.len(),
                    );

                    flash_flush_cache();
                    flash_enter_cmd_xip();
                });
            }

            self.offset += buf_slice.len();
            Ok(buf.len())
        }
    }

    // Anything written to the flash is flushed immediately
    fn flush(&mut self) -> Result<(), Self::Error> {
        Ok(())
    }
}

[rafalh]

This is intended behavior. Implementation assumes IO has some kind of buffering and allows reading/writing arbitrary number of bytes at unaligned addresses.
In your case it probably makes sense to add a buffer that has size of exactly one sector to your Flash struct and use it in Read and Write trait implementations. You can take https://github.com/rafalh/rust-fscommon/blob/master/src/buf_stream.rs as example although it doesn't seem to care about address alignment.

[Oakchris1955]

This is intended behavior.

In that case, shouldn't this behavior be documented somewhere in the docs, especially considering that this is the only FATFS library in crates.io?