-
Notifications
You must be signed in to change notification settings - Fork 7.5k
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
fix(littlefs): Use VFSImpl::exists() to avoid false error log #10217
Conversation
👋 Hello BlueAndi, we appreciate your contribution to this project! Click to see more instructions ...
Review and merge process you can expect ...
|
Memory usage test (comparing PR against master branch)The table below shows the summary of memory usage change (decrease - increase) in bytes and percentage for each target.
Click to expand the detailed deltas report [usage change in BYTES]
|
Test Results 56 files - 83 56 suites - 83 4m 53s ⏱️ - 1h 37m 52s Results for commit 22dfbfe. ± Comparison against base commit 396def3. This pull request removes 9 tests.
♻️ This comment has been updated with latest results. |
dc67023
to
cd80754
Compare
I think the proper action here would be to make the error a warning or even debug. |
This could be done additional. But I would not expext any log by using exists. |
If you remove the method, then |
Do you recall what is the issue with ::exists in the base class, arduino-esp32/libraries/FS/src/vfs_api.cpp Line 104 in 0539ebf
for LittleFS? |
I do not recall, because LittleFS was not implemented by us, but I see it being implemented much the same for the other file systems. Issue here seems the same as #6749 though for FFat. @lbernstone do you recall why FFat needs to open the file to check if it exists? If the issue is the error log, I do agree that it's a bit to much to issue an error, where a warning or even debug will suffice for the cases where one will need to get more information. |
Didn't realize this in my tests. Whats exactly is the problem by using base class exists method? |
@lucasssvaz can you please test this same solution with LittleFS, FFat and SPIFFS? |
I experiment the same issue in arduino esp32 version 3.0.4 as component idf with version 5.1.4 |
Same here. Been struggling with the and then realized is just a log thing "Seems". Anyway I think supporting littleFS is kinda important. |
@me-no-dev Tested for LittleFS, SPIFFS and FFat. This solution works for LittleFS and FFat. For SPIFFS this error does not appear and when trying this solution it causes |
I have a situation where I'm not able to update the library yet so I was trying to implement a code fix to directly use
I confirmed that I'm passing the same path as I was using with Sorry for asking here but I wasn't sure where else would be appropriate to ask |
Description of Change
Use exists() from VFSImpl base class and avoid using open() to check whether the file exists, because open() will log an error in case a file doesn't exists which is confusing, but might be relevant in case a developer uses open() directly and expects that the file is there.
Related links
Closes #7615