Add a proper File class (#287)

CMakePresets.json

@@ -71,8 +71,9 @@
         "W_NULL": "-Wnull-dereference -Wzero-as-null-pointer-constant -Wstrict-null-sentinel",
         "W_CLASS": "-Wsuggest-override -Woverloaded-virtual",
         "W_COND": "-Wduplicated-branches -Wduplicated-cond",
-        "W_REST": "-Wimplicit-fallthrough=5 -Wshadow -Wlogical-op -Wformat=2 -Wundef -Wno-long-long",
-        "WARNINGS": "$env{W_BASIC} $env{W_CONVERSION} $env{W_CAST} $env{W_NULL} $env{W_CLASS} $env{W_COND} $env{W_REST}"
+        "W_NO": "-Wno-missing-field-initializers -Wno-long-long",
+        "W_REST": "-Wimplicit-fallthrough=5 -Wshadow -Wlogical-op -Wformat=2 -Wundef",
+        "WARNINGS": "$env{W_BASIC} $env{W_CONVERSION} $env{W_CAST} $env{W_NULL} $env{W_CLASS} $env{W_COND} $env{W_NO} $env{W_REST}"
       }
     },
     {
@@ -81,7 +82,7 @@
       "inherits": "warnings-unix",
       "cacheVariables": {
         "CMAKE_CXX_FLAGS": "$env{WARNINGS}",
-        "CMAKE_CXX_FLAGS_DEBUG": "-Og -DDEBUG",
+        "CMAKE_CXX_FLAGS_DEBUG": "-O0 -g -DDEBUG",
         "CMAKE_CXX_FLAGS_RELEASE": "-Os"
       }
     },

Sts1CobcSw/FileSystem/CMakeLists.txt

@@ -1,10 +1,12 @@
 target_sources(Sts1CobcSw_FileSystem PRIVATE LfsWrapper.cpp)
-target_link_libraries(Sts1CobcSw_FileSystem PUBLIC littlefs::littlefs Sts1CobcSw_Outcome)
-target_link_libraries(Sts1CobcSw_FileSystem PRIVATE Sts1CobcSw_Serial)
+target_link_libraries(
+    Sts1CobcSw_FileSystem PUBLIC etl::etl littlefs::littlefs Sts1CobcSw_Outcome
+                                 Sts1CobcSw_ProgramId Sts1CobcSw_Serial
+)
 
 if(CMAKE_SYSTEM_NAME STREQUAL Generic)
     target_sources(Sts1CobcSw_FileSystem PRIVATE FileSystem.cpp LfsFlash.cpp)
-    target_link_libraries(Sts1CobcSw_FileSystem PRIVATE Sts1CobcSw_Periphery)
+    target_link_libraries(Sts1CobcSw_FileSystem PRIVATE rodos::rodos Sts1CobcSw_Periphery)
 else()
     target_sources(Sts1CobcSw_FileSystem PRIVATE LfsRam.cpp)
 endif()

Sts1CobcSw/FileSystem/ErrorsAndResult.hpp

@@ -24,6 +24,9 @@ enum class ErrorCode
     noMemory = LFS_ERR_NOMEM,           // No more memory available
     noAttribute = LFS_ERR_NOATTR,       // No data/attr available
     nameTooLong = LFS_ERR_NAMETOOLONG,  // File name too long
+    fileNotOpen = 1,
+    unsupportedOperation,
+    fileLocked,
 };
 
 

Sts1CobcSw/FileSystem/LfsFlash.cpp

@@ -1,13 +1,14 @@
 //! @file
-//! @brief  Implement a flash storage device for littlefs.
+//! @brief  Implement a flash memory device for littlefs.
 
-#include <Sts1CobcSw/FileSystem/LfsStorageDevice.hpp>  // IWYU pragma: associated
+#include <Sts1CobcSw/FileSystem/LfsMemoryDevice.hpp>  // IWYU pragma: associated
 #include <Sts1CobcSw/Periphery/Flash.hpp>
 #include <Sts1CobcSw/Serial/Byte.hpp>
 
 #include <rodos_no_using_namespace.h>
 
 #include <algorithm>
+#include <array>
 #include <span>
 
 
@@ -29,9 +30,14 @@ auto Lock(lfs_config const * config) -> int;
 auto Unlock(lfs_config const * config) -> int;
 
 
-auto readBuffer = flash::Page{};
+auto readBuffer = std::array<Byte, lfsCacheSize>{};
 auto programBuffer = decltype(readBuffer){};
-auto lookaheadBuffer = flash::Page{};
+auto lookaheadBuffer = std::array<Byte, 64>{};  // NOLINT(*magic-numbers)
+
+// littlefs requires the lookaheadBuffer size to be a multiple of 8
+static_assert(lookaheadBuffer.size() % 8 == 0);  // NOLINT(*magic-numbers)
+// littlefs requires the cacheSize to be a multiple of the read_size and prog_size, i.e., pageSize
+static_assert(lfsCacheSize % flash::pageSize == 0);
 
 lfs_config const lfsConfig = lfs_config{.context = nullptr,
                                         .read = &Read,
@@ -51,7 +57,7 @@ lfs_config const lfsConfig = lfs_config{.context = nullptr,
                                         .read_buffer = readBuffer.data(),
                                         .prog_buffer = programBuffer.data(),
                                         .lookahead_buffer = lookaheadBuffer.data(),
-                                        .name_max = LFS_NAME_MAX,
+                                        .name_max = maxPathLength,
                                         .file_max = LFS_FILE_MAX,
                                         .attr_max = LFS_ATTR_MAX,
                                         .metadata_max = flash::sectorSize,

Sts1CobcSw/FileSystem/LfsMemoryDevice.hpp

@@ -0,0 +1,15 @@
+#pragma once
+
+#include <littlefs/lfs.h>
+
+
+namespace sts1cobcsw::fs
+{
+inline constexpr auto lfsCacheSize = 256;
+// Our longest path should be strlen("/programs/65536.zip.lock") = 25 characters long
+inline constexpr auto maxPathLength = 30;
+extern lfs_config const lfsConfig;
+
+
+auto Initialize() -> void;
+}

Sts1CobcSw/FileSystem/LfsRam.cpp

@@ -1,9 +1,9 @@
 //! @file
-//! @brief Simulate a storage device for littlefs in RAM.
+//! @brief Simulate a memory device for littlefs in RAM.
 //!
 //! This is useful for testing the file system without using a real flash memory.
 
-#include <Sts1CobcSw/FileSystem/LfsStorageDevice.hpp>  // IWYU pragma: associated
+#include <Sts1CobcSw/FileSystem/LfsMemoryDevice.hpp>  // IWYU pragma: associated
 #include <Sts1CobcSw/Serial/Byte.hpp>
 
 #include <algorithm>
@@ -35,9 +35,14 @@ constexpr auto sectorSize = 4 * 1024;
 constexpr auto memorySize = 128 * 1024 * 1024;
 
 auto memory = std::vector<Byte>();
-auto readBuffer = std::array<Byte, pageSize>{};
+auto readBuffer = std::array<Byte, lfsCacheSize>{};
 auto programBuffer = decltype(readBuffer){};
-auto lookaheadBuffer = std::array<Byte, pageSize>{};
+auto lookaheadBuffer = std::array<Byte, 64>{};  // NOLINT(*magic-numbers)
+
+// littlefs requires the lookaheadBuffer size to be a multiple of 8
+static_assert(lookaheadBuffer.size() % 8 == 0);  // NOLINT(*magic-numbers)
+// littlefs requires the cacheSize to be a multiple of the read_size and prog_size, i.e., pageSize
+static_assert(lfsCacheSize % pageSize == 0);
 
 lfs_config const lfsConfig = lfs_config{.context = nullptr,
                                         .read = &Read,
@@ -57,7 +62,7 @@ lfs_config const lfsConfig = lfs_config{.context = nullptr,
                                         .read_buffer = readBuffer.data(),
                                         .prog_buffer = programBuffer.data(),
                                         .lookahead_buffer = lookaheadBuffer.data(),
-                                        .name_max = LFS_NAME_MAX,
+                                        .name_max = maxPathLength,
                                         .file_max = LFS_FILE_MAX,
                                         .attr_max = LFS_ATTR_MAX,
                                         .metadata_max = sectorSize,

Sts1CobcSw/FileSystem/LfsWrapper.cpp

@@ -1,4 +1,3 @@
-#include <Sts1CobcSw/FileSystem/LfsStorageDevice.hpp>
 #include <Sts1CobcSw/FileSystem/LfsWrapper.hpp>
 #include <Sts1CobcSw/Outcome/Outcome.hpp>
 
@@ -10,13 +9,221 @@ namespace sts1cobcsw::fs
 auto lfs = lfs_t{};
 
 
-[[nodiscard]] auto Mount() -> Result<void>
+// FIXME: For some reason this allocates 1024 bytes on the heap. With LFS_NO_MALLOC defined, it
+// crashes with a SEGFAULT.
+auto Mount() -> Result<void>
 {
     auto error = lfs_mount(&lfs, &lfsConfig);
     if(error == 0)
     {
         return outcome_v2::success();
     }
+    error = lfs_format(&lfs, &lfsConfig);
+    if(error != 0)
+    {
+        return static_cast<ErrorCode>(error);
+    }
+    error = lfs_mount(&lfs, &lfsConfig);
+    if(error == 0)
+    {
+        return outcome_v2::success();
+    }
     return static_cast<ErrorCode>(error);
 }
+
+
+auto Unmount() -> Result<void>
+{
+    auto error = lfs_unmount(&lfs);
+    if(error != 0)
+    {
+        return static_cast<ErrorCode>(error);
+    }
+    return outcome_v2::success();
+}
+
+
+auto Open(Path const & path, unsigned int flags) -> Result<File>
+{
+    auto file = File();
+    // We need to create a temporary with Path(path) here since append() modifies the object and we
+    // don't want to change the original path
+    file.lockFilePath_ = Path(path).append(".lock");
+    auto createLockFileResult = file.CreateLockFile();
+    if(createLockFileResult.has_error())
+    {
+        return ErrorCode::fileLocked;
+    }
+    auto error = lfs_file_opencfg(
+        &lfs, &file.lfsFile_, path.c_str(), static_cast<int>(flags), &file.lfsFileConfig_);
+    if(error == 0)
+    {
+        file.path_ = path;
+        file.openFlags_ = flags;
+        file.isOpen_ = true;
+        return file;
+    }
+    return static_cast<ErrorCode>(error);
+}
+
+
+File::File(File && other) noexcept
+{
+    MoveConstructFrom(&other);
+}
+
+
+auto File::operator=(File && other) noexcept -> File &
+{
+    if(this != &other)
+    {
+        MoveConstructFrom(&other);
+    }
+    return *this;
+}
+
+
+File::~File()
+{
+    // Only close the file if it is not in a default initialized or moved-from state
+    if(not path_.empty())
+    {
+        (void)Close();
+    }
+}
+
+
+auto File::Size() const -> Result<int>
+{
+    if(not isOpen_)
+    {
+        return ErrorCode::fileNotOpen;
+    }
+    auto size = lfs_file_size(&lfs, &lfsFile_);
+    if(size >= 0)
+    {
+        return size;
+    }
+    return static_cast<ErrorCode>(size);
+}
+
+
+auto File::Close() const -> Result<void>
+{
+    if(not isOpen_)
+    {
+        return outcome_v2::success();
+    }
+    auto error = lfs_remove(&lfs, lockFilePath_.c_str());
+    if(error != 0)
+    {
+        return static_cast<ErrorCode>(error);
+    }
+    return CloseAndKeepLockFile();
+}
+
+
+// This function handles the weird way in which lfs_file_t objects are moved. It is more efficient
+// than using the usual copy-and-swap idiom, because the latter would require to open and close the
+// file more often.
+auto File::MoveConstructFrom(File * other) noexcept -> void
+{
+    if(other->path_.empty())
+    {
+        return;
+    }
+    auto error = lfs_file_opencfg(&lfs,
+                                  &lfsFile_,
+                                  other->path_.c_str(),
+                                  static_cast<int>(other->openFlags_),
+                                  &lfsFileConfig_);
+    if(error == 0)
+    {
+        path_ = other->path_;
+        lockFilePath_ = other->lockFilePath_;
+        openFlags_ = other->openFlags_;
+        isOpen_ = true;
+    }
+    (void)other->CloseAndKeepLockFile();
+    other->path_ = "";
+    other->lockFilePath_ = "";
+    other->openFlags_ = 0;
+    other->lfsFile_ = {};
+}
+
+
+auto File::CreateLockFile() const noexcept -> Result<void>
+{
+    auto lfsLockFile = lfs_file_t{};
+    auto lockFileBuffer = std::array<Byte, lfsCacheSize>{};
+    auto lfsLockFileConfig = lfs_file_config{.buffer = lockFileBuffer.data()};
+    auto error = lfs_file_opencfg(&lfs,
+                                  &lfsLockFile,
+                                  lockFilePath_.c_str(),
+                                  static_cast<unsigned int>(LFS_O_RDWR) | LFS_O_CREAT | LFS_O_EXCL,
+                                  &lfsLockFileConfig);
+    if(error == 0)
+    {
+        error = lfs_file_close(&lfs, &lfsLockFile);
+    }
+    if(error == 0)
+    {
+        return outcome_v2::success();
+    }
+    return static_cast<ErrorCode>(error);
+}
+
+
+auto File::Read(void * buffer, std::size_t size) const -> Result<int>
+{
+    if(not isOpen_)
+    {
+        return ErrorCode::fileNotOpen;
+    }
+    if((openFlags_ & LFS_O_RDONLY) == 0U)
+    {
+        return ErrorCode::unsupportedOperation;
+    }
+    auto nReadBytes = lfs_file_read(&lfs, &lfsFile_, buffer, size);
+    if(nReadBytes >= 0)
+    {
+        return nReadBytes;
+    }
+    return static_cast<ErrorCode>(nReadBytes);
+}
+
+
+auto File::Write(void const * buffer, std::size_t size) -> Result<int>
+{
+    if(not isOpen_)
+    {
+        return ErrorCode::fileNotOpen;
+    }
+    if((openFlags_ & LFS_O_WRONLY) == 0U)
+    {
+        return ErrorCode::unsupportedOperation;
+    }
+    auto nWrittenBytes = lfs_file_write(&lfs, &lfsFile_, buffer, size);
+    if(nWrittenBytes >= 0)
+    {
+        return nWrittenBytes;
+    }
+    return static_cast<ErrorCode>(nWrittenBytes);
+}
+
+
+auto File::CloseAndKeepLockFile() const -> Result<void>
+{
+    if(not isOpen_)
+    {
+        return outcome_v2::success();
+    }
+    auto error = lfs_file_close(&lfs, &lfsFile_);
+    if(error != 0)
+    {
+        return static_cast<ErrorCode>(error);
+    }
+    isOpen_ = false;
+    return outcome_v2::success();
+}
 }