fix(radio): exFat directory handling#7485
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces ChangesexFAT Path Tracking Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@radio/src/gui/colorlcd/libui/file_browser.cpp`:
- Around line 133-135: In the FileBrowser constructor where currentPath is
initialized, normalize the input directory path by removing any trailing slashes
before assignment to ensure consistent path handling. This prevents double
slashes in full paths and ensures parent directory operations work correctly
from paths like "/A/". Apply the same normalization logic to the path operations
at lines 251-255 that are referenced in the comment to maintain consistency
across all path manipulations in the class.
In `@radio/src/gui/common/stdlcd/radio_sdmanager.cpp`:
- Around line 64-82: The sdManagerChdir function contains hardcoded assumptions
about root path structure, specifically checking sdManagerPath[1] in the descend
case and comparing sep == sdManagerPath in the parent case. Instead of these
hardcoded assumptions, import and use the ROOT_PATH constant from
radio/src/sdcard.h throughout the function. Replace all occurrences of the
hardcoded "/" with ROOT_PATH, and use strlen(ROOT_PATH) to determine root length
rather than assuming it is a single character. Update the root detection logic
in both the ".." case and the path building case to use the ROOT_PATH constant
and its computed length for robust path handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 75be2318-0371-4937-bb9d-3827f7485f67
📒 Files selected for processing (5)
radio/src/gui/colorlcd/libui/file_browser.cppradio/src/gui/colorlcd/libui/file_browser.hradio/src/gui/colorlcd/radio/radio_sdmanager.cppradio/src/gui/common/stdlcd/radio_sdmanager.cppradio/src/lib_file.cpp
|
Tested on a Zorro based on the main branch with FAT32, copy/paste and navigation all works normally. |
On exFAT-formatted SD cards the file browser was broken: the ".." (go up) entry never appeared, you couldn't navigate out of a sub-directory, and copy/paste targeted the wrong location. FAT32 cards were unaffected. As exemple, you could not open txt file for reading in SD manager.
Root cause
Two FatFs calls the browsers relied on don't work on exFAT, because exFAT directories carry no on-disk "." / ".." entries:
f_getcwd() always returns / — it rebuilds the path by walking up the ".." chain, which doesn't exist on exFAT
(FatFs documents this: "Cannot do getcwd on exFAT and returns root path").
This broke ".." detection, the current-path display, nested navigation and clipboard paths.
f_chdir("..") is a no-op — with no ".." entry, FatFs "stays there" (ff.c), so "go up" did nothing.
This fixes all of it for both the colorLCD and B&W SD managers.
I have taken great care of not breaking fat32, but I would appreciate some external tests
Summary by CodeRabbit
Release Notes