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

Improve Storage part 2 #2849

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

networkfusion
Copy link
Member

@networkfusion networkfusion commented Dec 21, 2023

Description

  • Align names for drive defines.
  • Take into account all drives when enumerating.
  • Improve checks to only need a single char for drive letter.
  • Rename the nanoHal header to move away from "Windows" naming.
  • Remove a duplicate include.

Each change is kept seperate incase of individual merge.

Motivation and Context

Code Maintainability.

How Has This Been Tested?

Screenshots

Types of changes

  • Improvement (non-breaking change that improves a feature, code or algorithm)
  • Bug fix (non-breaking change which fixes an issue with code or algorithm)
  • New feature (non-breaking change which adds functionality to code)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Config and build (change in the configuration and build system, has no impact on code or features)
  • Dev Containers (changes related with Dev Containers, has no impact on code or features)
  • Dependencies (update dependencies and changes associated, has no impact on code or features)
  • Documentation (changes or updates in the documentation, has no impact on code or features)

Checklist

  • My code follows the code style of this project (only if there are changes in source code).
  • My changes require an update to the documentation (there are changes that require the docs website to be updated).
  • I have updated the documentation accordingly (the changes require an update on the docs in this repo).
  • I have read the CONTRIBUTING document.
  • I have tested everything locally and all new and existing tests passed (only if there are changes in source code).

could possibly optimize out memcpy, but that is for later.
Further improve file name
Copy link
Member

@josesimoes josesimoes left a comment

Choose a reason for hiding this comment

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

I have no problem with the rename to move away from Windows to Persistent.
Now about the defines for the drives name: that's another story.
These were using Index0...1 etc on purpose as those are the order they show when enumerated by FATFS.

Hard coding this to (for example) USB_DRIVE_LETTER is assuming that there will be an USB MSD on the system and that it will be at INDEX 1, which may not be the case. There can be an SD Card drive at INDEX 0. Or not...

Any renaming seen necessary here should follow an agnostic naming pattern.

@networkfusion
Copy link
Member Author

I have no problem with the rename to move away from Windows to Persistent. Now about the defines for the drives name: that's another story. These were using Index0...1 etc on purpose as those are the order they show when enumerated by FATFS.

Hard coding this to (for example) USB_DRIVE_LETTER is assuming that there will be an USB MSD on the system and that it will be at INDEX 1, which may not be the case. There can be an SD Card drive at INDEX 0. Or not...

Any renaming seen necessary here should follow an agnostic naming pattern.

My concept towards inprovements here are this:
We are not Windows and cannot expect a user to enumerate an environment, so the filesystem should always be constent.
FatFS does not expect an enumeration, and we are probably fighting against it.
The naming was inconsistent, We should choose one or the other.

Personally (as a developer) I (think) I would prefer a (say) 3 letter drive name that relates to the drive (like linux) rather than a 1 letter name that is based on DOS. I note that this needs us to deprecate (rather than remove).

But, would add that some of the changes in this PR are helpful and optimize code. Just need split them.

networkfusion and others added 5 commits January 15, 2024 17:05
Automated fixes for code style.
…3d99-1b61-4b86-b7ec-cf78dfdbc883

Code style fixes for nanoframework/nf-interpreter PR#2849
@networkfusion
Copy link
Member Author

@josesimoes hopefully the changes should go some way towards your comments. Let me know what else.

@networkfusion
Copy link
Member Author

Unfortunatly, I missed the fact that DRIVE_LETTER_LENGTH is used extensively with windows storage... so probably back to the drawing board.

networkfusion and others added 3 commits January 15, 2024 19:08
Can be used as future reference.
Automated fixes for code style.
…1c5c-6115-4d92-994d-6898cbf8f3e1

Code style fixes for nanoframework/nf-interpreter PR#2849
@networkfusion networkfusion changed the title Improve Storage Improve Storage part 2 Jan 15, 2024
@networkfusion networkfusion mentioned this pull request Jan 15, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants