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 LFN support #155

Draft
wants to merge 5 commits into
base: develop
Choose a base branch
from

Conversation

beregond
Copy link

No description provided.

@thejpster
Copy link
Member

Thank you for this pull request!

I have some comments.

  • The nested array that was added uses 416 bytes of stack, on top of the heapless String of length 255. Passing the headless::String in would allow the caller to decide how much space they are prepared to use, plus UTF-8 is a more efficient encoding than an array of char and we can avoid the duplication.
  • I didn't see where this checks the LFN hash and sequence number to check the LFN is valid.
  • We probably also need to invalidate LFN entries when we delete a file.
  • LFNs are equally valid on FAT16 and we should handle both.
  • We're going to need some tests ;)

I like the approach of doing the dir listing first. That makes sense.

@beregond
Copy link
Author

Yes, I will provide code for fat16 as well as all the tests - for now it is a little mess (as I'm running this against actual SD card to understand fully what I'm trying to implement), but will do 👍

@jonathanpallant
Copy link
Collaborator

You inspired me to have a go at this myself - see https://github.com/rust-embedded-community/embedded-sdmmc-rs/tree/add-lfn-support

@beregond
Copy link
Author

Nice! Therefore I will continue to work on this branch until I loose interest - I treat this as educational thing as well (and definitely will take some ideas from your branch, I see solutions for problems that I've been stuck on a bit)

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