Skip to content

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)

@thejpster
Copy link
Member

Please do feel free to work on this. Perhaps there's a better API than the one we have. But I'll close this PR in the meantime.

@thejpster thejpster closed this Nov 13, 2024
@beregond beregond deleted the add/lfn-support branch November 23, 2024 13:47
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