feat(fs): default drive letter + ESP FS docs #6367
Conversation
| @@ -20,7 +20,7 @@ | |||
| #define DIR FF_DIR /* ESP IDF typedefs `DIR` as `FF_DIR` in its version of ff.h. Use `FF_DIR` in LVGL too */ | |||
| #endif | |||
|
|
|||
| #if LV_FS_FATFS_LETTER == '\0' | |||
| #if (LV_FS_FATFS_LETTER < 'A') || (LV_FS_FATFS_LETTER > 'Z') | |||
There was a problem hiding this comment.
Why do we need to add this restriction?
There was a problem hiding this comment.
lv_fs also needs to support '/' as the drive letter, for example: lv_fs_open(&fa, "/test_files/readtest.txt", LV_FS_MODE_RD);
There was a problem hiding this comment.
I wanted to make the code consistent with the error message and the documentation (which states the [A-Z]: format).
Using / as drive letter should not be encouraged as it can be confused with an absolute path. I also feel like pitfalls will occur if we want to make it work with LV_FS_DEFAULT_DRIVE_LETTER.
And I think the intention behind default drive letter was to elegantly handle situations like this, but correct me @kisvegabor
There was a problem hiding this comment.
This is done to be compatible with absolute paths. It has been used on our platform for quite a long time, and I believe there is no problem with the current design.
There was a problem hiding this comment.
See this change: a708450
There was a problem hiding this comment.
The goal of "default driver letter" feature is to better support absolute and "desktop" paths.
I'm assuming a driver is registered for letter / on your platform at the moment. The same driver has to be registered for - let's say - A, then LV_FS_DEFAULT_DRIVE_LETTER set to be A, then all the paths in your code should work the same way as before.
This allows replacing the workaround of using / as a drive letter with a more clear and verbose solution.
There was a problem hiding this comment.
If I understand correct this is what happens if you are using / as driver letter:
/path/to/files: is a relative path//path/to/files: is an absolute path
If this is the case then it seems a little but hacky. Using / as driver letter might work, but according to the docs it should have been an upper case letter. How complicated would it be for you to migrate to this new approac?
There was a problem hiding this comment.
//path/to/files: is an absolute path
Aren't all LV FS paths absolute relative to the driver mount point?
To me it just looks like the : is optional for any drive letter or / and / does not get any special treatment.
IMO it's okay to lock down the drive letters to [A-Z] especially now that a default drive letter is being added.
|
We need some feedback on this pull request. Now we mark this as "stale" because there was no activity here for 14 days. Remove the "stale" label or comment else this will be closed in 7 days. |
| @@ -20,7 +20,7 @@ | |||
| #define DIR FF_DIR /* ESP IDF typedefs `DIR` as `FF_DIR` in its version of ff.h. Use `FF_DIR` in LVGL too */ | |||
| #endif | |||
|
|
|||
| #if LV_FS_FATFS_LETTER == '\0' | |||
| #if (LV_FS_FATFS_LETTER < 'A') || (LV_FS_FATFS_LETTER > 'Z') | |||
There was a problem hiding this comment.
//path/to/files: is an absolute path
Aren't all LV FS paths absolute relative to the driver mount point?
To me it just looks like the : is optional for any drive letter or / and / does not get any special treatment.
IMO it's okay to lock down the drive letters to [A-Z] especially now that a default drive letter is being added.
| path++; /*Ignore the driver letter*/ | ||
| if(*path == ':') path++; | ||
| if(lv_fs_path_has_drive(path)) | ||
| path += 2; |
There was a problem hiding this comment.
Looks like it will break file paths missing the :, like A/path/to/file. I think this is ok.
This PR introduces the following changes:
LV_FS_DEFAULT_DRIVE_LETTERconfiguration, which allows skipping the drive letter form file paths