Weird unaligned write in embedded Rust
Opened this issue · 2 comments
Oakchris1955 commented
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
allocfeature, 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 commented
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 commented
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?