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

Add a proper File class #287

Merged
merged 46 commits into from
Oct 8, 2024
Merged

Add a proper File class #287

merged 46 commits into from
Oct 8, 2024

Conversation

flo12356
Copy link
Contributor

@flo12356 flo12356 commented May 4, 2024

Description

Add class File as part of the wrapper for littlefs.

Fixes parts of #273
Fixes parts of #186
Fixes parts of #209

@flo12356 flo12356 requested a review from PatrickKa May 4, 2024 20:00
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2024

Codecov Report

Attention: Patch coverage is 0.69444% with 143 lines in your changes missing coverage. Please review.

Project coverage is 85.72%. Comparing base (ef20651) to head (d4e7a26).

Files with missing lines Patch % Lines
Sts1CobcSw/FileSystem/LfsWrapper.cpp 0.00% 99 Missing ⚠️
Tests/UnitTests/LfsWrapper.test.cpp 0.00% 39 Missing ⚠️
Sts1CobcSw/FileSystem/LfsWrapper.ipp 0.00% 4 Missing ⚠️
Sts1CobcSw/FileSystem/LfsWrapper.hpp 0.00% 1 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #287       +/-   ##
===========================================
- Coverage   97.69%   85.72%   -11.97%     
===========================================
  Files          33       37        +4     
  Lines        1039     1184      +145     
  Branches       25       48       +23     
===========================================
  Hits         1015     1015               
- Misses         24      169      +145     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PatrickKa PatrickKa changed the title Add a proper C++ wrapper for littlefs WIP: Add a proper C++ wrapper for littlefs May 18, 2024
@PatrickKa PatrickKa changed the title WIP: Add a proper C++ wrapper for littlefs Add class File Aug 7, 2024
@PatrickKa PatrickKa changed the title Add class File Add a proper file class Aug 7, 2024
@PatrickKa PatrickKa force-pushed the littlefs-wrapper-273 branch 2 times, most recently from 95a1b61 to 51d8ccd Compare August 12, 2024 18:38
Copy link
Contributor

@PatrickKa PatrickKa left a comment

Choose a reason for hiding this comment

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

  • add_compile_definitions(USE_NO_RODOS) in the top-level CML file seems wrong

  • Read/Write() should be like fram::ReadFrom/WriteTo(), i.e. they should take byte spans:

    template<std::size_t extent>
    [[nodiscard]] auto Read(std::span<Byte, extent> data) const -> Result<int>;
    template<std::size_t extent>
    [[nodiscard]] auto Write(std::span<const Byte, extent> data) -> Result<int>;
  • Rename Read/WriteInternal() to just Read/Write(). The private functions have different parameters, so there is no reason to give them different names for disambiguation (function overloading FTW).

  • Use the same order for the public and private Read/Write() functions

Sts1CobcSw/FileSystem/LfsWrapper.ipp Outdated Show resolved Hide resolved
Sts1CobcSw/Outcome/CMakeLists.txt Outdated Show resolved Hide resolved
@PatrickKa PatrickKa changed the title Add a proper file class Add a proper File class Sep 21, 2024
flo12356 and others added 19 commits October 5, 2024 14:07
The TEST_CASE()s and other magic from Catch2 made it harder to debug.
This requires addinge the members path_ and openFlags_.
When using designated initializes to initialize a struct, it is
perfectly valid not to specify all fields. The unspecified fields are
initialized with default member initializers where provided, and empty
list-initialization otherwise.
The normal lfs_file_open() uses malloc() to allocate the file buffer.
Since we do not want to use the heap, we use lfs_file_opencfg() and
provide a statically allocated buffer instead.
flo12356 and others added 16 commits October 5, 2024 14:30
This reverts commit 1d0bd2f.

In this special case the Swap() function is an abomination, and the
previous version with the Move() function is better.
- Rename `Move()` to `MoveConstructFrom()`
- Make the class const-correct
- Remove `std::cout` and `iostream`
- Make cosmetic changes
They were used to test stuff during development but are now obsolete.
The longest path should be "/programs/65536.zip.lock" which is only 25
characters long, so we can save quite a bit of memory when we reduce the
max path length from the default 256 to just 30.
- Always pass paths as `Path` aka `etl::string` and use `c_str()` to
  ensure that the littlefs functions get null-terminated strings
- Move member variables which are only used in `CreateLockFile()` to
  that function
Eventually, the `File` class will need semaphores and therefore require
the main function from Rodos. I converted it now, because that allows me
to get rid of this `USE_NO_RODOS` nonsense in `Outcome.hpp` too.
Copy link
Contributor

@PatrickKa PatrickKa left a comment

Choose a reason for hiding this comment

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

  • Make CreateLockFile() and Close() const
  • In Open() when calling CreateLockFile() either use OUTCOME_TRY(file.CreateLockFile()); or create (and return) a new, specific error code for this, like fileLocked
  • Instead of removing the lock file in MoveConstructFrom() by calling Close() and creating it again later, we should add a private CloseAndKeepLockFile() function that we call instead. Close() should then also call this function and additionally remove the lock file.

@PatrickKa PatrickKa merged commit a21effb into master Oct 8, 2024
6 checks passed
@PatrickKa PatrickKa deleted the littlefs-wrapper-273 branch October 8, 2024 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants