arduino-esp32
arduino-esp32 copied to clipboard
fix(littlefs): Use VFSImpl::exists() to avoid false error log
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
| Messages | |
|---|---|
| :book: | You might consider squashing your 3 commits (simplifying branch history). |
π Hello BlueAndi, we appreciate your contribution to this project!
Click to see more instructions ...
This automated output is generated by the PR linter DangerJS, which checks if your Pull Request meets the project's requirements and helps you fix potential issues.
DangerJS is triggered with each push event to a Pull Request and modify the contents of this comment.
Please consider the following:
- Danger mainly focuses on the PR structure and formatting and can't understand the meaning behind your code or changes.
- Danger is not a substitute for human code reviews; it's still important to request a code review from your colleagues.
- Addressing info messages (π) is strongly recommended; they're less critical but valuable.
- To manually retry these Danger checks, please navigate to the Actions tab and re-run last Danger workflow.
Review and merge process you can expect ...
We do welcome contributions in the form of bug reports, feature requests and pull requests.
1. An internal issue has been created for the PR, we assign it to the relevant engineer.
2. They review the PR and either approve it or ask you for changes or clarifications.
3. Once the GitHub PR is approved we do the final review, collect approvals from core owners and make sure all the automated tests are passing.
- At this point we may do some adjustments to the proposed change, or extend it by adding tests or documentation.
4. If the change is approved and passes the tests it is merged into the default branch.
Generated by :no_entry_sign: dangerJS against 22dfbfeea5bf51e0a89a1539b99df3f5737c1153
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.
| Memory | FLASH [bytes] | FLASH [%] | RAM [bytes] | RAM [%] | ||||
|---|---|---|---|---|---|---|---|---|
| Target | DEC | INC | DEC | INC | DEC | INC | DEC | INC |
| ESP32S3 | :green_heart: -84 | 0 | :green_heart: -0.02 | 0.00 | 0 | 0 | 0.00 | 0.00 |
| ESP32S2 | :green_heart: -76 | 0 | :green_heart: -0.02 | 0.00 | 0 | 0 | 0.00 | 0.00 |
| ESP32C3 | :green_heart: -48 | 0 | :green_heart: -0.01 | 0.00 | 0 | 0 | 0.00 | 0.00 |
| ESP32C6 | :green_heart: -48 | 0 | :green_heart: -0.02 | 0.00 | 0 | 0 | 0.00 | 0.00 |
| ESP32H2 | :green_heart: -50 | 0 | :green_heart: -0.02 | 0.00 | 0 | 0 | 0.00 | 0.00 |
| ESP32 | :green_heart: -84 | 0 | :green_heart: -0.02 | 0.00 | 0 | 0 | 0.00 | 0.00 |
Click to expand the detailed deltas report [usage change in BYTES]
| Target | ESP32S3 | ESP32S2 | ESP32C3 | ESP32C6 | ESP32H2 | ESP32 | ||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Example | FLASH | RAM | FLASH | RAM | FLASH | RAM | FLASH | RAM | FLASH | RAM | FLASH | RAM |
| FFat/examples/FFat_Test | :green_heart: -56 | 0 | :green_heart: -56 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | :green_heart: -56 | 0 |
| FFat/examples/FFat_time | :green_heart: -56 | 0 | :green_heart: -56 | 0 | 0 | 0 | 0 | 0 | - | - | :green_heart: -56 | 0 |
| LittleFS/examples/LITTLEFS_test | :green_heart: -80 | 0 | :green_heart: -72 | 0 | :green_heart: -42 | 0 | :green_heart: -42 | 0 | :green_heart: -50 | 0 | :green_heart: -80 | 0 |
| LittleFS/examples/LITTLEFS_time | :green_heart: -84 | 0 | :green_heart: -76 | 0 | :green_heart: -48 | 0 | :green_heart: -48 | 0 | - | - | :green_heart: -84 | 0 |
Test Results
β56 filesβ β-ββ83βββ56 suitesβ β-β83βββ4m 53s :stopwatch: - 1h 37m 52s β21 tests β-βββ9βββ21 :white_check_mark: β-βββ9ββ0 :zzz: Β±0ββ0 :x: Β±0β 135 runsβ β-β168ββ135 :white_check_mark: β-β168ββ0 :zzz: Β±0ββ0 :x: Β±0β
Results for commit 22dfbfee.βΒ± Comparison against base commit 396def3f.
This pull request removes 9 tests.
performance.coremark.test_coremark β test_coremark
performance.fibonacci.test_fibonacci β test_fibonacci
performance.psramspeed.test_psramspeed β test_psramspeed
performance.ramspeed.test_ramspeed β test_ramspeed
performance.superpi.test_superpi β test_superpi
test_touch_errors
test_touch_interrtupt
test_touch_read
validation.periman.test_periman β test_periman
:recycle: This comment has been updated with latest results.
I think the proper action here would be to make the error a warning or even debug.
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 exists will stop functioning correctly. The log is there to let you know that the file was not found and will not be created. If the log is the issue, then that is where it should be fixed.
If you remove the method, then exists will stop functioning correctly.
Do you recall what is the issue with ::exists in the base class, https://github.com/espressif/arduino-esp32/blob/0539ebf322cf9b0620d4a8c357ddc284edc87f64/libraries/FS/src/vfs_api.cpp#L104 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 https://github.com/espressif/arduino-esp32/issues/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.
If you remove the method, then
existswill stop functioning correctly.
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 exists() to always return true.
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 VFSImpl::exists() in place of fs->exists() which currently throws the error. I've made several attempt at implementing it but it's always returning false even if the file exists. This is what I had to do to get it to compile so I'm thinking I've botched this up and perhaps someone can help fix it who knows better.
bool IHPFirmware::doesFirmwareFileExist() {
VFSImpl* vfsImpl = reinterpret_cast<VFSImpl*>(this->deps.fs);
return vfsImpl->VFSImpl::exists(this->firmwarePath);
}
I confirmed that I'm passing the same path as I was using with fs->exists()
Sorry for asking here but I wasn't sure where else would be appropriate to ask