Request for comments: Improve file system access

[TheSlowGrowth]

I took a look at the current file system support in libDaisy, and I'd like to suggest an improved API. Before I get to work writing the code, I wanted to post here and get some feedback and collect some ideas.

Here's what I'd like to achieve:

• A type-safe file system API that is safe to use: e.g. there should be no way you could attempt to open a file before a filesystem is mounted
• Mockable and unit test friendly: The same file system types you would use in production code should also be mockable for unit testing. This would ensure that it's easy and fast to write unit tested applications
• Be independent from the fatfs API: I'd like to abstract away the fatfs functions because they're inherently unsafe and not mockable.
• Be abstract: I want to be able to write higher-level code that accesses the file system without actually needing to know what underlying hardware is being used. This includes classes like WavFileReader or serializers / file stream classes, etc.
• Easy to use: Iterators to scan throught files and folders; RAII based file access that automatically closes the file when done

Viewed from an application, this is what I imagine such a filesystem API to look like:

// mount SD card and obtain a FileSystemHandle
FileSystemHandle sdCard = FatFSInterface::InitAndMount(FatFSInterface::Media::SD);
// The FileSystemHandle is just that - a handle. It can be duplicated, copied, etc. and distributed to all
// places that want to access this particular filesystem, independently of what type of hardware it's using.
// It also provides global stats about the file system, e.g. free disk space, etc.
const size_t sdCardSpaceLeft = sdCard.GetFreeDiskSpace();

// Files are accessed by requesting them from the FileSystemHandle, ensuring that you can't actually 
// try to access a file without ever mounting a filesystem.
{
    ReadOnlyFile myFile = sdCard.OpenFileForReading("/path/to/file.txt");
    std::array<uint8_t, 100> buffer;
    const size_t numBytesRead = myFile.Read(buffer);
    // here myFile goes out of scope, automatically closing the file
}
{
    ReadWriteFile myFile = sdCard.OpenFileForWriting("/path/to/file.txt");
    const size_t numBytesWritten = myFile.Write("Some text");
    // here myFile goes out of scope, automatically closing the file
}
// ReadOnlyFile and WriteOnlyFile are non-copyable (but move-able?), making sure no two places can concurrently 
// access the same file.

// This is how you'd iterate over a directory, returning all files that end on *.txt
DirectoryIterator rootFolderIterator = sdCard.IterateDirectory("/").FilteredByFileEnding(".txt");
for (const auto& fileIterator : rootFolderIterator) 
{
    const Path path = fileIterator.GetPath();
    ReadOnlyFile myFile = fileIterator.OpenForReading();
    // do something with the file
}

This all works internally by providing a File-IO layer abstraction that the FileSystemHandle and the other classes use to actually access the hardware. Think of it like a wrapper around fatfs functions. There would be ONE instance of such an abstraction layer, per file system. The implementation of FatFSInterface::InitAndMount() would look something like this:

FileSystemHandle FatFSInterface::InitAndMount(FatFSInterface::Media media)
{
    // init the hardware (if not done already)
    // initialize the IO layer instance (a member of FatFSInterface)
    ioLayer_[mediaIndex].Init(/* ... */);
    // create a FileSystemHandle, passing the IO Layer in the constructor
    return FileSystemHandle(ioLayer[mediaIndex]); // C++17 version
    return { ioLayer[mediaIndex] }; // C++14 version
} 

while the FileSystemHandle essentially just redirects calls to the IO Layer:

FileSystemHandle::FileSystemHandle(FileIoLayer& ioLayer) : ioLayer_(ioLayer) {}

size_t FileSystemHandle::GetFreeDiskSpace() const
{
    return ioLayer_.GetFreeDiskSpace();
}

ReadOnlyFile FileSystemHandle::OpenFileForReading(const char* path)
{
    return ReadOnlyFile(ioLayer_, path);
}

Similarly, the ReadOnlyFile or the DirectoryIterator would take an IO Layer as a constructor argument. This makes sure that we can mock the IO Layer in Unit tests, meaning that we can do proper unit testing without having to hide fatfs-code behind #ifndef UNIT_TEST.

Long story short: I have some of this already sketched out, but I'd like some feedback. Would this be something you'd like to see in libDaisy? What would you improve? How would we best handle errors (SD card unplugged at runtime, etc) in a typesafe way?

[stephenhensley]

I definitely like the idea of this. Are you thinking that this would be a layer on top of fatfs (on the hardware, with whatever alternative for mock in unit tests), or would this actually be a new filesystem to replace fatfs? As those would be significantly different scopes.

Wrt to your "abstract" point -- as of the latest update that added FatFsInterface, the actual fatfs usage itself does become independent of any hardware connections. Some of the legacy objects (WavFileReader, etc.) haven't been thoroughly updated to deal with that yet.

As for error handling, this is something that I've been wanting to resolve for the existing media as well.

With USB you can take advantage of I/O level connect/disconnect callbacks due to the nature of the connection. With SDMMC, without a detect pin wired up (which the vertical connector on most of our official boards doesn't have), you have to actually try, and fail to have a transaction. That could just be a simple GetStatus or something, but its still a bit of overhead.

As for what happens to your return types when an error occurs: at the cost of some extra ram, we could store an error code within the ReadOnlyFile, and other types. That way other functions, or the user could check if ReadOnlyFile.isValid() or whatever.

I'm not sure that's the best way to do it, but but I'm definitely interested in what other people think as well.

[TheSlowGrowth]

Are you thinking that this would be a layer on top of fatfs

Yes definitely. For the production code, fatfs functions would be wrapped into the FileIoLayer (or whatever we end up calling it). For test code, that layer would be replaced with a mock.

as of the latest update that added FatFsInterface, the actual fatfs usage itself does become independent of any hardware connections

Yers, and that in itself is a huge step. I'd like to take it further and abstract away the fatfs calls. That would allow us to write higher level code that can work on multiple types of storages, e.g. you could imagine a patch manager class that writes to a ReadWriteFile object. We could then use this patch storage class with SD cards, USB drives (those would be using the fatfs-based low level file io) or the QSPI flash (which could have its own file io layer). The "high level" patch storage could be properly unit tested, of course.