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

lv_fs Some Logic Needs Adjusting for WIN32? #7432

Open
vwheeler63 opened this issue Dec 10, 2024 · 20 comments · May be fixed by #7440
Open

lv_fs Some Logic Needs Adjusting for WIN32? #7432

vwheeler63 opened this issue Dec 10, 2024 · 20 comments · May be fixed by #7440

Comments

@vwheeler63
Copy link
Contributor

vwheeler63 commented Dec 10, 2024

Introduce the problem

The 3 examples in ./examples/others/file_explorer/ use incorrect path strings for the Windows platform, thus causing the examples to fail with run under lv_port_pc_visual_studio.

The Error

lv_file_explorer.* uses lv_fs.* internally, and so the driver-identifier-letter that it uses to prefix paths was (apparently) confused with the Windows drive letter, and so paths like "C:/Users/Public/Documents" were being passed during lv_file_explorer initialization, and if the program happened to be running on a different drive (other than C:), it didn't work because the initial "C:" in the path is in fact a driver-identifier-letter, and what follows it is the path passed to functions like lv_file_explorer_open_dir() => show_dir() => lv_fs_dir_open() => drv->dir_open_cb(drv, resolved_path.real_path), which only passed the "/Users/Public/Documents" to the function that actually opens the directory.

What they should have been passing is paths that looked like this: "C:C:/Users/Public/Documents" where the initial C is the driver-identifier-letter and the 2nd "C:" is part of the Windows path.

This confusion was probably brought about by the various places in the documentation and code where the driver-identifier-letter was erroneously being called the "drive letter" — which has very different meaning in a Windows, DOS, FAT or FAT32 context.

Proposal

Correct the path strings in the 3 lv_file_explorer examples, the mis-named code elements that cause confusion in Windows/DOS platform, as well as the mis-named macros in the lv_conf.h file.

@cheungxiongwei
Copy link

cheungxiongwei commented Dec 10, 2024

I'm also confused about this design, this is how I use it currently.

This is my configuration on Windows.

lv_conf.h

/** API for fopen, fread, etc. */
#define LV_USE_FS_STDIO 1
#if LV_USE_FS_STDIO
    #define LV_FS_STDIO_LETTER '/'     /**< Set an upper cased letter on which the drive will accessible (e.g. 'A') */
    #define LV_FS_STDIO_PATH ""         /**< Set the working directory. File/directory paths will be appended to it. */
    #define LV_FS_STDIO_CACHE_SIZE 4096    /**< >0 to cache this number of bytes in lv_fs_read() */
#endif

A feasible way

lv_obj_t *obj= lv_image_create(lv_screen_active());
lv_image_set_src(obj,"/C:/Users/root/Pictures/123.bmp"); 

// or
lv_image_set_src(obj,"/D:/Hub/cv.png"); 

Is there a better way to do this without adding the LV_FS_STDIO_LETTER character in front of the drive?

@liamHowatt
Copy link
Collaborator

Seems like we didn't think about Windows drive names conflicting with LVGL virtual drive letters.

Does it work to set LV_FS_WIN32_PATH to "C:" or "C:\" and LV_FS_WIN32_LETTER to 'C'?

A simple work-around to this would be to have lv_fs_resolve_path() keep the drive letter + ':' in the path if it was specified there in the first place (at least in the case of WIN32 platform).

Be careful that the dependents of lv_fs_resolve_path are. They may be expecting the drive letter to be removed.

in what context is it okay to remove the drive letter from the path and then attempt to open a directory without the drive letter in the path? I have not seen that logic before, and was just wondering if perhaps it was specific to some firmware platform, and whether it might deserve a re-evaluation.

Linux and Mac don't use drive letters :-). I can't think of any platforms other than Windows that use drive letters. LVGL interprets a leading [A-Z]: as a virtual drive letter. It conflicts with Windows and indeed makes it impossible to specify a Windows drive.

If you were to make the modification in your proposal and make C:\Users work, you would have to set LV_FS_WIN32_PATH to "", right?

@cheungxiongwei that looks correct to me. Does it work? You're using LVGL's support for "/" as a valid drive letter.

@liamHowatt
Copy link
Collaborator

liamHowatt commented Dec 10, 2024

It just occurred to me what the problem is here. I see that you have to do what cheungxiongwei did or else you cannot access different Windows drives with the win32 driver. At least there is a way to do it: cheungxiongwei's method. / is a valid drive letter.

Edit: You can also make the drive letter a normal letter like 'Z' and then access files like "Z:C:\....

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 10, 2024

@liamHowatt @cheungxiongwei My LV_FS_WIN32_LETTER is left at its default '\0', which seems like a natural place to start on WIN32 since all this mechanism should work in that configuration. Shouldn't it?

Another thought is if the

    void * dir_d = drv->dir_open_cb(drv, resolved_path.real_path);

call-back passed the whole resolved_path (or a pointer to it) rather than the string. In that way, it would be passing "all the data needed" for whatever platform to actually open the directory. The only thing I don't like about that is the WIN32 platform would have to RE-ASSEMBLE the path again, which seems backwards because "C:" is part of the path!

The more natural solution seems to be to NOT REMOVE the drive letter from the path in the lv_fs_resolve_path(). So the

    resolved_path_t resolved_path = lv_fs_resolve_path(path);

should not CHANGE the path, but only capture the drive letter.

The only burning question left is what was the designer's original design intention for lv_fs_resolve_path(path)? And we need @kisvegabor to get that question answered. In other words, what is the meaning of that resolved_path_t::letter field? And is the drive letter ever used in a context where it is not a legal part of the path?

Kind regards,
Vic

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 10, 2024

Linux and Mac don't use drive letters :-). I can't think of any platforms other than Windows that use drive letters. LVGL interprets a leading [A-Z]: as a virtual drive letter. It conflicts with Windows and indeed makes it impossible to specify a Windows drive.

To your point here, that calls for the DIFFERENCE in behavior then to ONLY be in the lv_fs_win32.c module then....

LV_FS_WIN32_PATH

Indeed, the design intention for that also should come into play, since it appears to be intended to be an "access limiter" for the lv_fs module, limiting the access to a specific directory tree. True?

Edit:

Edit: You can also make the drive letter a normal letter like 'Z' and then access files like "Z:C:...

I feel hesitant about that not knowing what a "virtual drive letter" is, but....

Perhaps this will help clarify:

What Windows (and DOS) does is that it keeps this data relationship: Default DRIVE goes somewhere in the execution environment (not an environment variable, but certainly part of the environment), and then each DRIVE in the environment has its own default directory and remembers it.

So you can be on drive E: and issue these commands:

> cd C:\default_dir_on_c
> cd D:\default_dir_on_d

and your current working directory is still the same directory on E, but the default directories on C: and D: did change, so when you do this:

> C:

your new current working directory is now: c:\default_dir_on_c.

@liamHowatt
Copy link
Collaborator

My LV_FS_WIN32_LETTER is left at its default '\0', which seems like a natural place to start on WIN32 since all this mechanism should work in that configuration. Shouldn't it?

You should be getting an error. It's an error to set the drive letter to something other than A-Z or /.

#if !LV_FS_IS_VALID_LETTER(LV_FS_WIN32_LETTER)
#error "Invalid drive letter"
#endif

lvgl/src/lv_conf_internal.h

Lines 4161 to 4162 in 2209e3a

/*Allow only upper case letters and '/' ('/' is a special case for backward compatibility)*/
#define LV_FS_IS_VALID_LETTER(l) ((l) == '/' || ((l) >= 'A' && (l) <= 'Z'))

@cheungxiongwei
Copy link

Seems like we didn't think about Windows drive names conflicting with LVGL virtual drive letters.

Does it work to set LV_FS_WIN32_PATH to "C:" or "C:\" and LV_FS_WIN32_LETTER to 'C'?

A simple work-around to this would be to have lv_fs_resolve_path() keep the drive letter + ':' in the path if it was specified there in the first place (at least in the case of WIN32 platform).

Be careful that the dependents of lv_fs_resolve_path are. They may be expecting the drive letter to be removed.

in what context is it okay to remove the drive letter from the path and then attempt to open a directory without the drive letter in the path? I have not seen that logic before, and was just wondering if perhaps it was specific to some firmware platform, and whether it might deserve a re-evaluation.

Linux and Mac don't use drive letters :-). I can't think of any platforms other than Windows that use drive letters. LVGL interprets a leading [A-Z]: as a virtual drive letter. It conflicts with Windows and indeed makes it impossible to specify a Windows drive.

If you were to make the modification in your proposal and make C:\Users work, you would have to set LV_FS_WIN32_PATH to "", right?

@cheungxiongwei that looks correct to me. Does it work? You're using LVGL's support for "/" as a valid drive letter.

Sorry for not describing it clearly.
Yes, the example I gave works fine. I just want to know if there is a better way to support multiple drives on Windows.

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 10, 2024

You should be getting an error. It's an error to set the drive letter to something other than A-Z or /.

Interesting.... I want to say, "Where I can learn about the design intention for lv_fs?" But then I realize that's probably going to come back to me to study the content of the lv_fs.c content and figure it out. I am forseeing suplements to fs.rst coming, because that's where I'm going next — to re-study.

Edit: I stand corrected: the lv_port_pc_visual_studio is indeed set up with LV_FS_WIN32_LETTER == 'C'. I was thinking of LV_FS_DEFAULT_DRIVE_LETTER, which is a different macro, and is indeed set as '\0'.

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 10, 2024

@kisvegabor @liamHowatt

Okay, so now that I think I understand that these two fields:

  • lv_fs_drv_t::letter
  • resolved_path_t::drive_letter

are meant to be a letter that represents the entire file system represented by the registered lv_fs_drv_t, and is not at all the "drive letter" used in in Windows, DOS and other FAT and FAT32 file systems. Please steer me if this is an incorrect conclusion.

If that is the case, then how to represent a path in the context of lv_fs is really:

    "C:dos_drive:\rooted\path\to\file"
or
    "C:dos_drive:relative\path\to\file"

where dos_drive is the Windows/DOS drive letter part of the path. And this is simply a way that LVGL "looks up" the file system driver (with all its function pointers), and that letter + ':' are removed from the path before the path is actually used to attempt to do things in the file system.

Am I correct? Either way, please steer me, because these understandings need to be carried into the documentation I am editing. Also, the examples for the file_explorer will need to be modified so it works when LV_USE_FS_WIN32 != 0 — which I can also take care of, as well as the documentation in the code surrounding the meaning of LV_FS_WIN32_LETTER, since to a Windows/DOS user, it looks very much like a drive letter... without further clarification, that is. Let me know, the sooner the better.

Kind regards,
Vic

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 10, 2024

P.S. I am suspecting the trouble I am having is that I erroneously conflated the meaning of LV_FS_WIN32_LETTER with the Windows/DOS drive letter which is part of the path. If so, this needs to be very explicit in the documentation -- which I can take care of once clarified in my own mind.

Edit: If this is the case, then it would seem that

  • resolved_path_t::drive_letter and
  • lv_fs_drv_t::letter

should instead both be called "identifier" or "virtual_drive_identifer" or something with a similar concept, or something that would help it to not be confused with Windows/DOS drive letters, would you agree?

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 10, 2024

Okay -- I think this is becoming clear now. The correct set of lines for the file_explorer examples then looks like this:

#if LV_USE_FS_WIN32
    lv_file_explorer_open_dir(file_explorer, "Z:C:/");
#if LV_FILE_EXPLORER_QUICK_ACCESS
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_HOME_DIR, "Z:C:/Users/Public/Desktop");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_VIDEO_DIR, "Z:C:/Users/Public/Videos");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_PICTURES_DIR, "Z:C:/Users/Public/Pictures");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_MUSIC_DIR, "Z:C:/Users/Public/Music");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_DOCS_DIR, "Z:C:/Users/Public/Documents");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_FS_DIR, "Z:C:/");
#endif

#else
...

where Z is whatever character is represented by macro LV_FS_WIN32_LETTER and accepted by the LV_FS_IS_VALID_LETTER() macro.

So if LV_FS_WIN32_LETTER was '/', then it would have to look like this:

#if LV_USE_FS_WIN32
    lv_file_explorer_open_dir(file_explorer, "/:C:/");
#if LV_FILE_EXPLORER_QUICK_ACCESS
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_HOME_DIR, "/:C:/Users/Public/Desktop");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_VIDEO_DIR, "/:C:/Users/Public/Videos");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_PICTURES_DIR, "/:C:/Users/Public/Pictures");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_MUSIC_DIR, "/:C:/Users/Public/Music");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_DOCS_DIR, "/:C:/Users/Public/Documents");
    lv_file_explorer_set_quick_access_path(file_explorer, LV_EXPLORER_FS_DIR, "/:C:/");
#endif

#else
...

And given these understandings, the (changed) examples are now working.

Okay -- I can document that.

@vwheeler63
Copy link
Contributor Author

@liamHowatt I updated the statement of the problem and the proposal accordingly.

@liamHowatt
Copy link
Collaborator

are meant to be a letter that represents the entire file system represented by the registered lv_fs_drv_t

Correct.

    "C:dos_drive:\rooted\path\to\file"
or
    "C:dos_drive:relative\path\to\file"

The part after the C: is what the OS's file open API sees. The question is whether dos_drive:\rooted\path\to\file or dos_drive:relative\path\to\file is correct for Windows/DOS which I do not know.

P.S. I am suspecting the trouble I am having is that I erroneously conflated the meaning of LV_FS_WIN32_LETTER with the Windows/DOS drive letter which is part of the path. If so, this needs to be very explicit in the documentation

I agree 😅

should instead both be called "identifier" or "virtual_drive_identifer" or something with a similar concept, or something that would help it to not be confused with Windows/DOS drive letters, would you agree?

Updating that name would require a lot of config names to change and I think it would break a lot of projects. I could see it happening in a new major release (v10). That's just my opinion though.

Okay -- I think this is becoming clear now. [...] And given these understandings, the (changed) examples are now working.

Nice, that looks correct to me.

Is everything clear?


Proposal

Correct the path strings in the 3 lv_file_explorer examples, the mis-named code elements that cause confusion in Windows/DOS platform, as well as the mis-named macros in the lv_conf.h file.

RE your new proposal: how exactly do you want to change the examples? I don't know if using these code snippets is a good idea because it's a very niche example. It's on Windows and they use the Windows root as the mount point.

@liamHowatt
Copy link
Collaborator

liamHowatt commented Dec 11, 2024

FYI I think a typical use case is to set LV_FS_WIN32_PATH to something like C:\Users\foo\Documents\my_project\assets so the graphics code has lv_image_set_src(img, "Z:cat.png"); which is more portable when a developer goes to move their code from simulator to device, as opposed to lv_image_set_src(img, "Z:C:\Users\foo\Documents\my_project\assets\cat.png");

I realize this contradicts #7431 (comment) 😆

@vwheeler63
Copy link
Contributor Author

vwheeler63 commented Dec 11, 2024

Good morning, Liam! ( @liamHowatt )

Updating that name would require a lot of config names to change

Interestingly it turns out that lv_fs_drv_t::letter is not misleading. It's not saying it belongs to a drive, so the mere fact that it is a member name makes it an attribute of the DRIVER! So without changing it, it still means "driver letter" which is okay.

What doesn't work (thankfully) turns out to be an internal field name (resolved_path_t::drive_letter is private to lv_fs.c) so no API is broken.

The one weird one that is misleading (leads to wrong train of thought and lost time digging in C code) is the macro name LV_FS_DEFAULT_DRIVE_LETTER and it is also found in the Kconfig file. I THINK I took care of these correctly though in my new PR #7440. Just adding the "R" in LV_FS_DEFAULT_DRIVER_LETTER takes care of the misleading factor.

RE your new proposal: how exactly do you want to change the examples?

Good question. Answer: with good, clear documentation and an explanatory comment in the code (since that is what I would need in order to not be led astray or think seeing "C:C:/Users..." is is a bug). There is a block of code that only applies to Windows and so simply inserting the "virtual" drive letters in front of the existing path makes it work, and the comment directs the user to the documentation for more details.

Thank You

Thank you for hanging in there with me on this one. :-) I appreciate the steering, and it puts it on record that there was something confusing that needed to be remedied.

Edit:

For whatever reason, your mermaid diagram is working, which makes me wonder if it has something to do with the version of the sphinxcontrib-mermaid extension being used to generate the docs....

@vwheeler63
Copy link
Contributor Author

FYI I think a typical use case is to

I probably should add this to the documentation....

@liamHowatt
Copy link
Collaborator

liamHowatt commented Dec 12, 2024

Updating that name would require a lot of config names to change

Interestingly it turns out that lv_fs_drv_t::letter is not misleading. It's not saying it belongs to a drive, so the mere fact that it is a member name makes it an attribute of the DRIVER! So without changing it, it still means "driver letter" which is okay.

What doesn't work (thankfully) turns out to be an internal field name (resolved_path_t::drive_letter is private to lv_fs.c) so no API is broken.

I don't know why but for some reason I thought you wanted to rename LV_FS_WIN32_LETTER. Sorry for my confusion. Internal/private names can change easily.

Re Kconfig, good catch!

Re the clear documentation, looks really solid at a glance! This will help a lot of people 😌

And thank you for not giving up while decrypting the meaning of all the FS details!

For whatever reason, your mermaid diagram is working, which makes me wonder if it has something to do with the version of the sphinxcontrib-mermaid extension being used to generate the docs....

I began to fix it and saw it working locally for me too, but not remotely here: https://docs.lvgl.io/master/details/main-components/fs.html

Here is my local version of the extension: sphinxcontrib-mermaid==0.9.2.

I believe we can simply do this

diff --git a/docs/requirements.txt b/docs/requirements.txt
index 138de0cec..c0774c1ad 100644
--- a/docs/requirements.txt
+++ b/docs/requirements.txt
@@ -10,7 +10,7 @@ sphinxcontrib-htmlhelp
 sphinxcontrib-jsmath
 sphinxcontrib-qthelp
 sphinxcontrib-serializinghtml
-sphinxcontrib-mermaid
+sphinxcontrib-mermaid==0.9.2
 sphinx-design
 sphinx-rtd-dark-mode
 typing-extensions

Or I will figure out what's not compliant about my Mermaid code in the latest version and correct it. I think that may be a better approach.

@vwheeler63
Copy link
Contributor Author

I believe we can simply do this

Good find, sir! Indeed, the one I have here locally is also sphinxcontrib-mermaid v0.9.2.

Or I will figure out what's not compliant about my Mermaid code in the latest version and correct it. I think that may be a better approach.

Let me know if/when you succeed! It will enable me to remove it from a list of things pending.

@liamHowatt
Copy link
Collaborator

I used pip install --upgrade sphinxcontrib-mermaid in my python venv to get v1.0.0 (was 0.9.2 before, which worked) and reproduced the issue. I could not figure out what was wrong so I opened a PR #7453 which simply pins the version back to 0.9.2 as discussed. I also opened an issue in the sphinxcontrib-mermaid GH repo.

@vwheeler63
Copy link
Contributor Author

I used pip install --upgrade sphinxcontrib-mermaid in my python venv to get v1.0.0 (was 0.9.2 before, which worked) and reproduced the issue. I could not figure out what was wrong so I opened a PR #7453 which simply pins the version back to 0.9.2 as discussed. I also opened an issue in the sphinxcontrib-mermaid GH repo.

Well done.

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 a pull request may close this issue.

3 participants