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 5 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
301 changes: 301 additions & 0 deletions crates/flipperzero/src/filesystem.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,301 @@
use core::ffi::{c_char, CStr};

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

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

/// Stream and file system related error type
#[derive(Debug, Clone, Copy)]
pub enum Error {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would also be nice to implement the Display trait that calls filesystem_api_error_get_desc.

Ok,
NotReady,
Exists,
NotExists,
InvalidParameter,
Denied,
InvalidName,
Internal,
NotImplemented,
AlreadyOpen,
}

impl Error {
pub fn to_sys(&self) -> sys::FS_Error {
match self {
Self::Ok => sys::FS_Error_FSE_OK,
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,
}
}

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!(),
}
}
}

/// Trait comparable to `std::Read` for the Flipper stream API
pub trait Read {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder what's the best way of handling std traits like this. Perhaps we put them under a flipperzero::io module?

The problem with implementing traits like Read, Write and Seek here is that we might want to use these traits for other purposes (e.g. sockets). Having several copies of this trait takes away a lot of the benefits of having the traits.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That sounds like a good idea to me. I can't really think of any sensible alternatives.

Should I go ahead and lift the traits, or do you think it's worth trying to get more consensus?

Copy link
Collaborator

Choose a reason for hiding this comment

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

That seems reasonable. We can put them under something like flipperzero::io similar to the standard library.

If it doesn't work out, it's OK to break the API at the next major release. I'm trying to avoid doing it too often, but given the Flipper Zero SDK API is far from stable, I don't think folks will mind.

fn read(&mut self, buf: &mut [u8]) -> Result<usize, Error>;
}

/// Enumeration of possible methods to seek within an I/O object.
///
/// It is used by the Seek trait.
pub enum SeekFrom {
Start(i32),
End(i32),
Current(i32),
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I would recommend going with u64/i64 here.

I know the Flipper Zero API doesn't allow seeking by anything greater than a u32, but a u64 matches the return types of storage_file_tell/storage_file_size and means a little less casting is required.

std::io::SeekFrom::Start uses an u64 rather than an i64. The other two are i64 to allow seeking backwards.

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 a bit new to Rust API design -- what do you reckon the behavior should be if we seek with a u64 or i64 that doesn't fit into a u32?

Intuition says to return a Result::Err, but I'm concerned that breaks the orthogonality with sys::FS_Error.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the conversion to u32 with try_into fails, I'd suggest returning Err(Error::InvalidParameter). It's fine to mention that seeks are limited to 4 GiB in the documentation.

If it does become a real problem, we could probably emulate it with multiple seeks or ask for a native 64-bit seek call.

}

/// 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> {
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 as i32))?;
}

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
}
Ok(n) => buf = &buf[n..],
Err(e) => return Err(e),
}
}
Ok(())
}
}

pub struct OpenOptions(u8, u8);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should implement at least Clone and Debug.


impl OpenOptions {
pub fn new() -> Self {
Self(0, 0)
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 assuming that .0 is Access Mode and .1 is Open Mode.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. I'll make that a struct to make the intent more clear

}

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

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

/// Open file, fail if file doesn't exist
pub fn open_existing(self, set: bool) -> Self {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a shame that std::fs::OpenOptions doesn't quite map 1:1 with OpenMode.

In some sense the flags map better to the File::open, File::create and File::create_new functions, since despite being bit-flags, it doesn't make sense to ever set two of them simultaneously (what does FSOM_CREATE_NEW | FSOM_OPEN_EXISTING mean?).

It might be best to use the standard std::fs::OpenOptions attributes, checking them in order and choosing the first one which is set:

  • create_newFSOM_CREATE_NEW
  • truncateFSOM_CREATE_ALWAYS
  • appendFSOM_OPEN_APPEND
  • createFSOM_OPEN_ALWAYS
  • [default] → FSOM_OPEN_EXISTING

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be sure I understand, you're saying that the module should prevent nonsensical states like FSOM_CREATE_NEW | FSOM_CREATE_ALWAYS by enforcing mutual exclusivity? Like how create_new overrides any previous calls to create or truncate in std::fs::OpenOptions?

(If that's the case, would we want to choose the first one that's set, or the last one that's set?)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Exactly. If there's nonsensical values, the best thing we can do is have an API where it's impossible to create those nonsensical values.

We'd want the first one that matches in the list above, since it goes from most specialised (create_new) to least (no OpenOptions set). For example, if create_new is set, then it doesn't really matter what the values of truncate, append or create are—It's going to create a file, which is technically already truncated and you're appending since there can't possibly be any existing data because we're creating a new one.

OpenOptions(
self.0,
if set {
self.1 | sys::FS_OpenMode_FSOM_OPEN_EXISTING
} else {
self.1 & !sys::FS_OpenMode_FSOM_OPEN_EXISTING
},
)
}

/// Open file. Create new file if not exist
pub fn open_always(self, set: bool) -> Self {
OpenOptions(
self.0,
if set {
self.1 | sys::FS_OpenMode_FSOM_OPEN_ALWAYS
} else {
self.1 & !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(
self.0,
if set {
self.1 | sys::FS_OpenMode_FSOM_OPEN_APPEND
} else {
self.1 & !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(
self.0,
if set {
self.1 | sys::FS_OpenMode_FSOM_CREATE_NEW
} else {
self.1 & !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(
self.0,
if set {
self.1 | sys::FS_OpenMode_FSOM_CREATE_ALWAYS
} else {
self.1 & !sys::FS_OpenMode_FSOM_CREATE_ALWAYS
},
)
}

pub fn open(self, path: &CStr) -> Result<BufferedFile, Error> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should probably return a File (storage_file_open) rather than a Stream (file_stream_open/buffered_file_stream_open).

Streams are a higher level primitive built on-top of File, somewhat like how FILE (fopen) is built on an OS file descriptor (open). Having access to these low-level handles are important in certain situations (for example, if you wanted to implement BufRead or needed to pass it to a C-API).

Streams look like a quite useful abstraction (and saves wasting space re-implementing that functionality), but probably makes sense to do separately from this PR.

let f = BufferedFile::new();
if unsafe {
sys::buffered_file_stream_open(f.0, path.as_ptr() as *const i8, self.0, self.1)
} {
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::buffered_file_stream_get_error(f.0)
}))
}
}
}

/// File stream with buffered read operations.
pub struct BufferedFile(*mut sys::Stream);

impl BufferedFile {
pub fn new() -> Self {
unsafe {
BufferedFile(sys::buffered_file_stream_alloc(
UnsafeRecord::open(RECORD_STORAGE).as_ptr(),
))
}
}
}

impl Drop for BufferedFile {
fn drop(&mut self) {
unsafe {
sys::buffered_file_stream_sync(self.0);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like buffered_file_stream_close calls buffered_file_stream_sync internally, so no need to explicitly call it.

sys::buffered_file_stream_close(self.0);
}
}
}

impl Read for BufferedFile {
fn read(&mut self, buf: &mut [u8]) -> Result<usize, Error> {
Ok(unsafe { sys::stream_read(self.0, buf.as_mut_ptr(), buf.len()) })
}
}

impl Seek for BufferedFile {
fn seek(&mut self, pos: SeekFrom) -> Result<usize, Error> {
let (offset_type, offset) = match pos {
SeekFrom::Start(n) => (sys::StreamOffset_StreamOffsetFromStart, n),
SeekFrom::End(n) => (sys::StreamOffset_StreamOffsetFromEnd, n),
SeekFrom::Current(n) => (sys::StreamOffset_StreamOffsetFromCurrent, n),
};
unsafe {
if sys::stream_seek(self.0, offset, offset_type) {
Ok(sys::stream_tell(self.0))
} else {
Err(Error::from_sys(sys::buffered_file_stream_get_error(self.0)))
}
}
}

fn rewind(&mut self) -> Result<(), Error> {
unsafe { sys::stream_rewind(self.0) };
Ok(())
}

fn stream_len(&mut self) -> Result<usize, Error> {
Ok(unsafe { sys::stream_size(self.0) })
}

fn stream_position(&mut self) -> Result<usize, Error> {
Ok(unsafe { sys::stream_tell(self.0) })
}
}

impl Write for BufferedFile {
fn write(&mut self, buf: &[u8]) -> Result<usize, Error> {
if unsafe { sys::stream_insert(self.0, buf.as_ptr(), 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.

You probably want stream_write. It appears that stream_insert deletes the previous contents.

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, that's what I get for not reading the docs!

Right now my diff is

@@ -305,13 +305,7 @@ impl Seek for BufferedFile {
 
 impl Write for BufferedFile {
     fn write(&mut self, buf: &[u8]) -> Result<usize, Error> {
-        if unsafe { sys::stream_insert(self.0, buf.as_ptr(), buf.len()) } {
-            Ok(buf.len())
-        } else {
-            Err(Error::from_sys(unsafe {
-                sys::buffered_file_stream_get_error(self.0)
-            }))
-        }
+        Ok(unsafe { sys::stream_write(self.0, buf.as_ptr(), buf.len()) })
     }
 
     fn flush(&mut self) -> Result<(), Error> {

which doesn't seem right. I'll do a bit of digging to see what sort of error reporting there is for that interface.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suspect the Stream API is modelled on C's stdio. The behaviour for fwrite is that if the returned size is less than the size argument provided (or returns zero), then an error might have occurred. In this case you need to use feof or ferror to tell if it's an actual error, or you just hit EOF.

So, probably a similar logic applies for stream_write.

Ok(buf.len())
} else {
Err(Error::from_sys(unsafe {
sys::buffered_file_stream_get_error(self.0)
}))
}
}

fn flush(&mut self) -> Result<(), Error> {
if unsafe { sys::buffered_file_stream_sync(self.0) } {
Ok(())
} else {
Err(Error::from_sys(unsafe {
sys::buffered_file_stream_get_error(self.0)
}))
}
}
}
1 change: 1 addition & 0 deletions crates/flipperzero/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
extern crate alloc;

pub mod dialogs;
pub mod filesystem;
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 calling this module storage to match the service that exposes these APIs.

pub mod furi;
pub mod gui;
pub mod macros;
23 changes: 23 additions & 0 deletions examples/hello-filesystem/.cargo/config.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
[target.thumbv7em-none-eabihf]
rustflags = [
# CPU is Cortex-M4 (STM32WB55)
"-C", "target-cpu=cortex-m4",

# Size optimizations
"-C", "panic=abort",
"-C", "debuginfo=0",
"-C", "opt-level=z",

# LTO helps reduce binary size
"-C", "embed-bitcode=yes",
"-C", "lto=yes",

# Linker flags for relocatable binary
"-C", "link-args=--script=flipperzero-rt.ld --Bstatic --relocatable --discard-all --strip-all --lto-O3 --lto-whole-program-visibility",

# Required to link with `lld`
"-Z", "no-unique-section-names=yes",
]

[build]
target = "thumbv7em-none-eabihf"
6 changes: 6 additions & 0 deletions examples/hello-filesystem/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
# Generated by Cargo
# will have compiled files and executables
/target/

# These are backup files generated by rustfmt
**/*.rs.bk
30 changes: 30 additions & 0 deletions examples/hello-filesystem/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

19 changes: 19 additions & 0 deletions examples/hello-filesystem/Cargo.toml
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
[package]
name = "hello-filesystem"
version = "0.1.0"
edition = "2021"
rust-version = "1.64.0"
autobins = false
autoexamples = false
autotests = false
autobenches = false

[[bin]]
name = "hello-filesystem"
bench = false
test = false

[dependencies]
flipperzero = { version = "0.6.0-alpha", path = "../../crates/flipperzero" }
flipperzero-sys = { version = "0.6.0-alpha", path = "../../crates/sys" }
flipperzero-rt = { version = "0.6.0-alpha", path = "../../crates/rt" }
Loading