feature: filesystem

[TsarFox]

Hello!

I've been working on some wrappers for the stream and file system APIs. This is still draft-state but I'm opening a MR now to make the effort known and garner some comments on approach.

BLUF: These commits provide an interface reminiscent of std::io for the Flipper Zero file system APIs, enabling application developers to interact with files on e.g., the SD card.

[dcoles]

Hi @TsarFox!

Thanks for putting together this PR. It would be fantastic to have a wrapper around the filesystem APIs.

Sounds like you have the right idea by trying to roughly align with the std::fs and std::io modules.

[dcoles]

I'd recommend calling this module storage to match the service that exposes these APIs.

[dcoles]

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.

[TsarFox]

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.

[dcoles]

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.

[dcoles]

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.

[TsarFox]

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?

[dcoles]

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.

[dcoles]

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

[dcoles]

This should implement at least Clone and Debug.

[dcoles]

I'm assuming that .0 is Access Mode and .1 is Open Mode.

[TsarFox]

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

[dcoles]

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

[dcoles]

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

[TsarFox]

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.

[dcoles]

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.

[dcoles]

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_new → FSOM_CREATE_NEW
• truncate → FSOM_CREATE_ALWAYS
• append → FSOM_OPEN_APPEND
• create → FSOM_OPEN_ALWAYS
• [default] → FSOM_OPEN_EXISTING

[TsarFox]

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?)

[dcoles]

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.

[dcoles]

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.

[TsarFox]

Thank you for the feedback!

Switching over to the file_stream_* API led to the need to cast more, so I'm less certain that this is correct, but I'll try to reason through it in the next couple of days

[dcoles]

No problem. I'll take another look over the code shortly (getting a bit late tonight).

[str4d]

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.

[TsarFox]

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?

[str4d]

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().

[dcoles]

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.

[TsarFox]

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.

[str4d]

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.

[TsarFox]

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

[TsarFox]

https://forum.flipperzero.one/t/detecting-errors-in-the-storage-api/13441

[str4d]

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.

[TsarFox]

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

[TsarFox]

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

[str4d]

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()?

[TsarFox]

@str4d This was edifying -- I really appreciate the thorough review! I'll address the rest of your comments tomorrow

[dcoles]

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!

[TsarFox]

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

[TsarFox]

Figured it out!

[dcoles]

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.

[dcoles]

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

[dcoles]

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.

[TsarFox]

Shame on me for not reading the UnsafeRecord docs 🙁

[dcoles]

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

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

[TsarFox]

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

[dcoles]

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.

[dcoles]

I'm fine to keep it as is.