Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feature: filesystem #37

Merged
merged 30 commits into from
Mar 30, 2023
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
ef4a614
Initial wrappers for filesystem read/write
TsarFox Jan 14, 2023
abfd6dc
Implement wrapper for `buffered_file_stream_open`
TsarFox Jan 14, 2023
e1e37b5
Add example
TsarFox Jan 14, 2023
dab0d8a
Merge remote-tracking branch 'origin/main' into feat-filesystem
TsarFox Jan 14, 2023
2ca5f82
Use CStr to address null terminator issue; path needs prefix
TsarFox Jan 14, 2023
6f46b39
Rename module to `storage`
TsarFox Jan 26, 2023
ef3c8ae
Implement `fmt::Display` for `storage::Error`
TsarFox Jan 26, 2023
1ab7934
Re-implement `OpenOptions` as a struct
TsarFox Jan 26, 2023
abab55e
Remove extraneous call to `sys::buffered_file_stream_sync` in `Buffer…
TsarFox Jan 26, 2023
2543bd1
Use `stream_write` as opposed to `stream_insert` in `io::Write` imple…
TsarFox Jan 26, 2023
e3783b9
First attempt at having `storage` use the high-level `storage_file` API
TsarFox Jan 26, 2023
5d6cff1
Use integer types from `std::io::SeekFrom` for storage API
TsarFox Feb 11, 2023
f32fcc5
Default instance for File
TsarFox Feb 11, 2023
b01ddc0
Replace unwrap with returning Error::InvalidParameter
TsarFox Feb 11, 2023
70a60f3
Move traits to flipperzero::io
TsarFox Feb 11, 2023
f854179
Error handling for `File::write`
TsarFox Feb 11, 2023
207e4d4
Deal with nonsensical open modes in `open`
TsarFox Feb 11, 2023
10d4d74
Update example
TsarFox Feb 12, 2023
6cb0fcc
Better error handling for `read`
TsarFox Feb 12, 2023
502fd82
Apply suggestions from str4d's code review
TsarFox Feb 27, 2023
ee16dbc
Read/Write should check for errors unconditionally
TsarFox Feb 27, 2023
66aa317
error_code needs to be copied
TsarFox Feb 27, 2023
2a2b392
Address comments for Seek and other error handling updates
TsarFox Feb 27, 2023
7f7c03f
Address example syntax errors
TsarFox Feb 27, 2023
51f77df
Needs feedback: use Formatter::write_str in impl Display
TsarFox Feb 27, 2023
d728ac1
Update documentation
TsarFox Feb 27, 2023
6eaa44f
Hold onto UnsafeRecord for duration of File lifetime
TsarFox Feb 27, 2023
dd51cc4
Add `Error::WriteZero`
TsarFox Feb 27, 2023
ec770ac
impl fmt::Display for Error without `alloc`
TsarFox Mar 2, 2023
f17ab68
Merge branch 'main' into feat-filesystem
dcoles Mar 30, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
121 changes: 121 additions & 0 deletions crates/flipperzero/src/io.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
use core::ffi::CStr;
use core::fmt;

use flipperzero_sys as sys;

/// Stream and file system related error type
#[derive(Debug, Clone, Copy)]
pub enum Error {
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
Ok,
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
NotReady,
Exists,
NotExists,
InvalidParameter,
Denied,
InvalidName,
Internal,
NotImplemented,
AlreadyOpen,
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
}

impl Error {
pub fn to_sys(&self) -> sys::FS_Error {
match self {
Self::Ok => sys::FS_Error_FSE_OK,
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
Self::NotReady => sys::FS_Error_FSE_NOT_READY,
Self::Exists => sys::FS_Error_FSE_EXIST,
Self::NotExists => sys::FS_Error_FSE_NOT_EXIST,
Self::InvalidParameter => sys::FS_Error_FSE_INVALID_PARAMETER,
Self::Denied => sys::FS_Error_FSE_DENIED,
Self::InvalidName => sys::FS_Error_FSE_INVALID_NAME,
Self::Internal => sys::FS_Error_FSE_INTERNAL,
Self::NotImplemented => sys::FS_Error_FSE_NOT_IMPLEMENTED,
Self::AlreadyOpen => sys::FS_Error_FSE_ALREADY_OPEN,
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
}
}

pub fn from_sys(err: sys::FS_Error) -> Self {
match err {
sys::FS_Error_FSE_OK => Self::Ok,
sys::FS_Error_FSE_NOT_READY => Self::NotReady,
sys::FS_Error_FSE_EXIST => Self::Exists,
sys::FS_Error_FSE_NOT_EXIST => Self::NotExists,
sys::FS_Error_FSE_INVALID_PARAMETER => Self::InvalidParameter,
sys::FS_Error_FSE_DENIED => Self::Denied,
sys::FS_Error_FSE_INVALID_NAME => Self::InvalidName,
sys::FS_Error_FSE_INTERNAL => Self::Internal,
sys::FS_Error_FSE_NOT_IMPLEMENTED => Self::NotImplemented,
sys::FS_Error_FSE_ALREADY_OPEN => Self::AlreadyOpen,
_ => unimplemented!(),
}
}
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
}

impl fmt::Display for Error {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let msg = unsafe { CStr::from_ptr(sys::filesystem_api_error_get_desc(self.to_sys())) };
write!(f, "{}", msg.to_bytes().escape_ascii())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might want to use Formatter::write_str here, because Rust's formatting macros pull in 10+ KiB of formatting code even for a simple case like this one.

I didn't know about escape_ascii. That's very useful to know!

Copy link
Contributor Author

@TsarFox TsarFox Feb 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to need to ponder on this one. write! is somehow converting the EscapeAscii into a printable byte sequence without needing alloc. My naïve solution needs alloc, which I think isn't ideal. I'll head back to the drawing board

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out!

}
}

/// Trait comparable to `std::Read` for the Flipper stream API
pub trait Read {
fn read(&mut self, buf: &mut [u8]) -> Result<usize, Error>;
TsarFox marked this conversation as resolved.
Show resolved Hide resolved
}

/// Trait comparable to `std::Seek` for the Flipper stream API
pub trait Seek {
fn seek(&mut self, pos: SeekFrom) -> Result<usize, Error>;

fn rewind(&mut self) -> Result<(), Error> {
self.seek(SeekFrom::Start(0))?;
Ok(())
}

fn stream_len(&mut self) -> Result<usize, Error> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This API is not yet part of stable Rust (rust-lang/rust#59359), so we may not want to offer it here yet (or else be prepared to potentially alter it if/when std::io::Seek::stream_len is stabilised).

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The SDK exposes a stream_size function which serves a similar role.

So far I consider all APIs still subject to change. With the amount of churn in the Flipper Zero SDK, I haven't been willing to call anything stable yet. So if we need to alter it, then that's fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll defer to @dcoles for a final decision on whether to nix it - I don't have a dog in this race.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine to keep it as is.

let old_pos = self.stream_position()?;
let len = self.seek(SeekFrom::End(0))?;

// Avoid seeking a third time when we were already at the end of the
// stream. The branch is usually way cheaper than a seek operation.
if old_pos != len {
self.seek(SeekFrom::Start(
old_pos.try_into().map_err(|_| Error::InvalidParameter)?,
))?;
}

Ok(len)
}

fn stream_position(&mut self) -> Result<usize, Error> {
self.seek(SeekFrom::Current(0))
}
}

/// Trait comparable to `std::Write` for the Flipper stream API
pub trait Write {
fn write(&mut self, buf: &[u8]) -> Result<usize, Error>;
fn flush(&mut self) -> Result<(), Error>;

fn write_all(&mut self, mut buf: &[u8]) -> Result<(), Error> {
while !buf.is_empty() {
match self.write(buf) {
Ok(0) => {
// TODO
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having an equivalent to std::io::Error::WriteZero to return here, would require that Error::to_sys return Option<sys::FS_Error>, like std::io::Error::raw_os_error does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made the change and pushed it to this branch. It's a little cumbersome but.. so are a lot of the OS-adjacent API's in rust :)

Any thoughts on this, @dcoles?

}
Ok(n) => buf = &buf[n..],
Err(e) => return Err(e),
}
}
Ok(())
}
}

/// Enumeration of possible methods to seek within an I/O object.
///
/// It is used by the Seek trait.
pub enum SeekFrom {
Start(u64),
End(i64),
Current(i64),
}
2 changes: 2 additions & 0 deletions crates/flipperzero/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,6 @@ extern crate alloc;
pub mod dialogs;
pub mod furi;
pub mod gui;
pub mod io;
pub mod macros;
pub mod storage;
252 changes: 252 additions & 0 deletions crates/flipperzero/src/storage.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,252 @@
use core::ffi::{c_char, c_void, CStr};

use flipperzero_sys as sys;
use flipperzero_sys::furi::UnsafeRecord;

use crate::io::*;

const RECORD_STORAGE: *const c_char = sys::c_string!("storage");

#[derive(Debug, Default, Clone, Copy)]
pub struct OpenOptions {
access_mode: u8,
open_mode: u8,
}

impl OpenOptions {
pub fn new() -> Self {
Self::default()
}

fn from_parts(access_mode: u8, open_mode: u8) -> Self {
OpenOptions {
access_mode,
open_mode,
}
}

/// Read access
pub fn read(self, set: bool) -> Self {
OpenOptions::from_parts(
if set {
self.access_mode | sys::FS_AccessMode_FSAM_READ
} else {
self.access_mode & !sys::FS_AccessMode_FSAM_READ
},
self.open_mode,
)
}

/// Write access
pub fn write(self, set: bool) -> Self {
OpenOptions::from_parts(
if set {
self.access_mode | sys::FS_AccessMode_FSAM_WRITE
} else {
self.access_mode & !sys::FS_AccessMode_FSAM_WRITE
},
self.open_mode,
)
}

/// Open file, fail if file doesn't exist
pub fn open_existing(self, set: bool) -> Self {
OpenOptions::from_parts(
self.access_mode,
if set {
self.open_mode | sys::FS_OpenMode_FSOM_OPEN_EXISTING
} else {
self.open_mode & !sys::FS_OpenMode_FSOM_OPEN_EXISTING
},
)
}

/// Open file. Create new file if not exist
pub fn open_always(self, set: bool) -> Self {
OpenOptions::from_parts(
self.access_mode,
if set {
self.open_mode | sys::FS_OpenMode_FSOM_OPEN_ALWAYS
} else {
self.open_mode & !sys::FS_OpenMode_FSOM_OPEN_ALWAYS
},
)
}

/// Open file. Create new file if not exist. Set R/W pointer to EOF
pub fn open_append(self, set: bool) -> Self {
OpenOptions::from_parts(
self.access_mode,
if set {
self.open_mode | sys::FS_OpenMode_FSOM_OPEN_APPEND
} else {
self.open_mode & !sys::FS_OpenMode_FSOM_OPEN_APPEND
},
)
}

/// Creates a new file. Fails if the file is exist
pub fn create_new(self, set: bool) -> Self {
OpenOptions::from_parts(
self.access_mode,
if set {
self.open_mode | sys::FS_OpenMode_FSOM_CREATE_NEW
} else {
self.open_mode & !sys::FS_OpenMode_FSOM_CREATE_NEW
},
)
}

/// Creates a new file. If file exist, truncate to zero size
pub fn create_always(self, set: bool) -> Self {
OpenOptions::from_parts(
self.access_mode,
if set {
self.open_mode | sys::FS_OpenMode_FSOM_CREATE_ALWAYS
} else {
self.open_mode & !sys::FS_OpenMode_FSOM_CREATE_ALWAYS
},
)
}

pub fn open(self, path: &CStr) -> Result<File, Error> {
// It's possible to produce a nonsensical `open_mode` using the above
// operations, so we have some logic here to drop any extraneous
// information. The possible open modes form a partial order (for
// example, `create_new` is more specialized than `truncate`) so we
// search for the first "on" bit in this sequence, and use that as the
// open mode.
let canonicalized_open_mode = if self.open_mode & sys::FS_OpenMode_FSOM_CREATE_NEW != 0 {
sys::FS_OpenMode_FSOM_CREATE_NEW
} else if self.open_mode & sys::FS_OpenMode_FSOM_CREATE_ALWAYS != 0 {
sys::FS_OpenMode_FSOM_CREATE_ALWAYS
} else if self.open_mode & sys::FS_OpenMode_FSOM_OPEN_APPEND != 0 {
sys::FS_OpenMode_FSOM_OPEN_APPEND
} else if self.open_mode & sys::FS_OpenMode_FSOM_OPEN_ALWAYS != 0 {
sys::FS_OpenMode_FSOM_OPEN_ALWAYS
} else {
sys::FS_OpenMode_FSOM_OPEN_EXISTING
};

let f = File::new();
if unsafe {
sys::storage_file_open(
f.0,
path.as_ptr() as *const i8,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd recommend using core::ffi::c_char just to make this a bit more clear.

self.access_mode,
canonicalized_open_mode,
)
} {
Ok(f)
} else {
// Per docs, "you need to close the file even if the open operation
// failed," but this is handled by `Drop`.
Err(Error::from_sys(unsafe { sys::storage_file_get_error(f.0) }))
}
}
}

/// File stream with buffered read operations.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This documentation is out of date, since File is not buffered.

There's a few other mentions of "stream" that should also be corrected.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Going to update those comments but keep them unspecified since this /could/ be the API we use for actual stream objects later on

pub struct File(*mut sys::File);

impl File {
pub fn new() -> Self {
unsafe {
File(sys::storage_file_alloc(
UnsafeRecord::open(RECORD_STORAGE).as_ptr(),
))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storage_file_alloc holds onto the storage record, so we need to keep a valid UnsafeRecord on the File object, so we don't close the record prematurely.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shame on me for not reading the UnsafeRecord docs 🙁

}
}
}

impl Drop for File {
fn drop(&mut self) {
unsafe {
// `storage_file_close` calls `storage_file_sync`
// internally, so it's not necesssary to call it here.
sys::storage_file_close(self.0);
}
}
}

impl Read for File {
fn read(&mut self, buf: &mut [u8]) -> Result<usize, Error> {
let to_read = buf.len().try_into().map_err(|_| Error::InvalidParameter)?;
let bytes_read =
unsafe { sys::storage_file_read(self.0, buf.as_mut_ptr() as *mut c_void, to_read) };
if bytes_read == to_read || bytes_read < to_read && unsafe { sys::storage_file_eof(self.0) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sys::storage_file_read guaranteed to read as many bytes as it can? Most fread-style methods give no such guarantees, and indeed the only guarantee that std::io::Read::read(buf) gives is that if Ok(n) is returned, then 0 <= n <= buf.len().

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sys::storage_file_read guaranteed to read as many bytes as it can?

I wouldn't count on it. Most other places in the Flipper Zero firmware seem to loop on storage_file_read until they receive a 0, then check whether an error actually occurred using storage_file_ferror.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. I think it makes more sense to check for an error unconditionally, rather than trying to infer what happened based on the number of bytes read.

{
Ok(bytes_read as usize)
} else {
Err(Error::from_sys(unsafe {
sys::storage_file_get_error(self.0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Normally I would have a suspicion that the way we are supposed to use storage_file_get_error is to check it after every operation involving a file handle (i.e. if it returns sys::FS_Error_FSE_OK, that means the previous operation succeeded):

let bytes_read =
    unsafe { sys::storage_file_read(self.0, buf.as_mut_ptr() as *mut c_void, to_read) };
Error::from_sys(unsafe { sys::storage_file_get_error(self.0) }).map_or(Ok(bytes_read), Err)

And indeed, looking at how storage_file_get_error is used in flipperzero-firmware, you see that it is almost always of the form:

storage_SOME_METHOD(...);
if (storage_file_get_error(file) != FSE_OK) {...}

However, looking at how storage_file_read is used in flipperzero-firmware, it seems at a glance that boolean error conditions are produced via the same check as above (that as many bytes were read as were asked for). So IDK, we might want to ask the Flipper devs what their intentions are for storage_file_get_error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm happy to action on that; I can make a forum post

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}))
}
}
}

impl Seek for File {
fn seek(&mut self, pos: SeekFrom) -> Result<usize, Error> {
let (offset_type, offset) = match pos {
SeekFrom::Start(n) => (true, n.try_into().map_err(|_| Error::InvalidParameter)?),
SeekFrom::End(n) => (false, n.try_into().map_err(|_| Error::InvalidParameter)?),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting offset_type = false here will cause SeekFrom::End to be treated the same way as SeekFrom::Current (because offset_type is really from_start).

Instead, for SeekFrom::End we will need to measure the length of the file, and then use from_start = true and offset = file_length - n.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm realizing that this is all subtly incorrect if we're going off of the Rust semantics. In particular, the user should be able to specify a negative number for End or Current, but the current code doesn't allow that.

My problem is that the offset parameter in storage_file_seek is a u32 whereas the rest of the API treats the file offset as a u64, so it isn't super clear how to calculate offset = file_length - n such that it can be passed to storage_file_seek.

I'll see if there are any examples upstream

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't find many, but there's at least one example of necking storage_file_size down to a uint32_t in lib/toolbox/crc32_calc.c.

The LittleFS 'file max' is a 32-bit int and lfs_file_size returns a int32_t, so I'm tempted to say that we can assume storage_file_size and friends return a value in [0, u32::MAX]. Does that sound right? Or is it bad to assume that the firmware is always going to be on LittleFS

SeekFrom::Current(n) => (false, n.try_into().map_err(|_| Error::InvalidParameter)?),
};
unsafe {
if sys::storage_file_seek(self.0, offset, offset_type) {
Ok(sys::storage_file_tell(self.0)
.try_into()
.map_err(|_| Error::InvalidParameter)?)
} else {
Err(Error::from_sys(sys::storage_file_get_error(self.0)))
}
}
}

fn rewind(&mut self) -> Result<(), Error> {
self.seek(SeekFrom::Start(0)).map(|_| {})
}

fn stream_len(&mut self) -> Result<usize, Error> {
Ok(unsafe {
sys::storage_file_size(self.0)
.try_into()
.map_err(|_| Error::InvalidParameter)?
})
}

fn stream_position(&mut self) -> Result<usize, Error> {
Ok(unsafe {
sys::storage_file_tell(self.0)
.try_into()
.map_err(|_| Error::InvalidParameter)?
})
}
}

impl Write for File {
fn write(&mut self, buf: &[u8]) -> Result<usize, Error> {
let to_write = buf.len().try_into().map_err(|_| Error::InvalidParameter)?;
let bytes_written =
unsafe { sys::storage_file_write(self.0, buf.as_ptr() as *mut c_void, to_write) };
if bytes_written == to_write
|| bytes_written < to_write && unsafe { sys::storage_file_eof(self.0) }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same kind of comment here as for Read::read: is storage_file_write guaranteed to write as many bytes as it can, or only n <= buf.len()?

{
Ok(bytes_written as usize)
} else {
Err(Error::from_sys(unsafe {
sys::storage_file_get_error(self.0)
}))
}
}

fn flush(&mut self) -> Result<(), Error> {
Ok(())
}
}

impl Default for File {
fn default() -> Self {
Self::new()
}
}
Loading