For examples/arduino_emulator, removed dependency on arduino-esp32#446
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a minimal FS (filesystem) shim into the Arduino emulator example so ESPAsyncWebServer can compile/link in the host build without cloning the ESP32 Arduino core.
Changes:
- Adds
FS.h / FS.cpp / FSImpl.h(sourced from arduino-pico) intoexamples/arduino_emulator/. - Updates the emulator example CMake to build against the local FS shim instead of
.ci/arduino-esp32FS sources/headers. - Simplifies the GitHub Actions workflow by removing the
arduino-esp32clone step.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| examples/arduino_emulator/README_FS.md | Documents why FS shim files are included and their provenance. |
| examples/arduino_emulator/FSImpl.h | Adds FS implementation interface types required by FS wrapper. |
| examples/arduino_emulator/FS.h | Adds the FS wrapper API used by ESPAsyncWebServer/static file serving. |
| examples/arduino_emulator/FS.cpp | Implements FS wrapper methods and adds a DEBUGV() no-op. |
| examples/arduino_emulator/CMakeLists.txt | Switches host build to compile/include the local FS shim. |
| .github/workflows/build-arduino-emulator.yml | Removes dependency on cloning arduino-esp32 for FS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| #include <memory> | ||
| #include <Arduino.h> | ||
| #include <../include/time.h> // See issue #6714 |
|
|
||
| #include <stddef.h> | ||
| #include <stdint.h> | ||
| #include <FS.h> |
| int File::available() { | ||
| if (!_p) { | ||
| return false; | ||
| } | ||
|
|
||
| return _p->size() - _p->position(); | ||
| } |
| String File::readString() { | ||
| String ret; | ||
| ret.reserve(size() - position()); | ||
| uint8_t temp[256]; | ||
| int countRead; | ||
| do { | ||
| countRead = read(temp, sizeof(temp)); | ||
| ret.concat(temp, countRead); | ||
| } while (countRead > 0); | ||
| return ret; | ||
| } |
| } | ||
|
|
||
|
|
||
| #if defined(FS_FREESTANDING_FUNCTIONS) |
| FSImplPtr p = fs._impl; | ||
| if (!p || !p->mount()) { | ||
| DEBUGV("FSImpl mount failed\r\n"); | ||
| return false; | ||
| } | ||
|
|
||
| !make sure mountPoint has trailing '/' here |
| for (MountEntry* entry = s_mounted; entry; entry = entry->next) { | ||
| size_t offset = entry->path.length(); | ||
| if (strstr(path, entry->path.c_str())) { | ||
| File result = entry->fs->open(path + offset); |
| ESPAsyncWebServer's dependency on the FS class for serving static | ||
| file. Arduino-Emulator does not implement the FS class, which is a |
There was a problem hiding this comment.
@MitchBradley we might want to exclude the new files from cpplint check in the GitHub workflow. mistake - example folder not considerd
|
@me-no-dev FYI something you might be interested in (my comment plus @MitchBradley 's answer): #423 (comment) A recent change in Arduino FS impl that now depends on FreeRTOS, which makes the FS impl unusable with Arduino Emulator. Since ESPAsycnWebServer depends on FS lib, we then depend on FreeRTOS, which was not the case before (only AsyncTCP depended on FreeRTOS). |
|
I'm not entusiastic about implementing the copilot suggestions, since these versions of FS.cpp, FS.h, and FSImpl.h are almost identical to the corresponding files in arduinopico - and those files, in turn, are almost identical to the original FS file in ESP8266 arduino. This does not seem like the right place to fix those problems; it seems better to mimic those other files to make it clear that there is nothing new here. |
Oh yes we have to ignore them! copilot automatically reviews PRs lol... But in this case we have to leave the files as-is. |
|
Hi @MitchBradley I pinged @me-no-dev about this issue and Arduino Core Team raised a PR (espressif/arduino-esp32#12670) to make the inclusion optional. So that should solve the issue. I think Lucas pinged you in the other PR (#423 (comment)). |
|
That esp fix seems fine to me. |
|
@mathieucarbou @MitchBradley I've optimized the PR a little. PTAL is it's still a proper fix. |
The Arduino Emulator example program had a dependency on the not-quite-standard
FS class that was being satisfied by cloning the arduino-esp32 repo and compiling
only FS.cpp therefrom. The problem is that v3.3.9 of that repo introduces some locking
in the included FSImpl.h, which creates a dependency on FreeRTOS.
The solution is to extract only the necessary FS files into the example source directory,
but from the rp2040 arduino core that does not have the FreeRTOS dependency.
An alternative approach would be to try and incorporate the FS class into Arduino-Emulator,
but that would be a substantial effort due to other file-related mismatches in Arduino-Emulator's
API surface.