refactor: use python script for SPIFFS handling #371
Conversation
📝 WalkthroughWalkthroughAdds a native Python SPIFFS image generator and integrates it into the build pipeline (build/router/download targets), removes the external mkspiffs tool declaration and related auto-install logic, and adds an ESP32 SPIFFS example project with docs and test sketch. Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as Build System
participant FSRouter as build_fs_router
participant SpiffsFS as SpiffsFS
participant SpiffsBlock as SpiffsBlock
participant Output as Output Binary
Builder->>FSRouter: build_fs_router(target, source, env)
FSRouter->>FSRouter: Determine filesystem type (spiffs/littlefs/fatfs)
FSRouter->>SpiffsFS: instantiate builder + create_file(...) per source file
SpiffsFS->>SpiffsBlock: begin_obj(obj_id, size, name)
SpiffsFS->>SpiffsBlock: update_obj(contents)
SpiffsBlock->>SpiffsBlock: allocate/write pages, check is_full()
SpiffsFS->>SpiffsBlock: end_obj()
SpiffsFS->>Output: to_binary() → serialized SPIFFS image
Output-->>Builder: write target file
sequenceDiagram
participant Device as Target Device
participant Downloader as download_spiffs
participant Parser as SpiffsFS.from_binary
participant Extractor as SpiffsFS.extract_files
participant Disk as Local Unpack Dir
Device->>Downloader: read partition / flash region
Downloader->>Parser: pass binary image
Parser->>Parser: parse blocks/pages, reconstruct objects
Parser->>Extractor: extract files
Extractor->>Disk: write extracted files to unpack directory
Disk-->>Downloader: files available locally
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (11)
💤 Files with no reviewable changes (2)
🧰 Additional context used🧠 Learnings (25)📓 Common learnings📚 Learning: 2025-06-26T09:56:30.658ZApplied to files:
📚 Learning: 2025-12-27T12:14:31.697ZApplied to files:
📚 Learning: 2025-12-27T16:33:31.442ZApplied to files:
📚 Learning: 2025-12-27T12:16:48.649ZApplied to files:
📚 Learning: 2025-08-20T14:13:13.842ZApplied to files:
📚 Learning: 2025-09-23T14:15:00.476ZApplied to files:
📚 Learning: 2025-08-08T09:50:01.969ZApplied to files:
📚 Learning: 2025-12-23T17:13:00.004ZApplied to files:
📚 Learning: 2025-08-05T11:47:41.756ZApplied to files:
📚 Learning: 2025-08-08T09:55:50.842ZApplied to files:
📚 Learning: 2025-09-22T14:47:08.855ZApplied to files:
📚 Learning: 2025-09-23T14:15:00.476ZApplied to files:
📚 Learning: 2025-08-26T14:52:29.044ZApplied to files:
📚 Learning: 2025-12-27T12:14:31.697ZApplied to files:
📚 Learning: 2025-12-27T12:16:48.649ZApplied to files:
📚 Learning: 2025-07-13T20:03:29.695ZApplied to files:
📚 Learning: 2025-08-10T16:59:15.533ZApplied to files:
📚 Learning: 2025-10-19T17:30:42.403ZApplied to files:
📚 Learning: 2025-09-23T16:55:39.788ZApplied to files:
📚 Learning: 2025-09-05T11:58:49.681ZApplied to files:
📚 Learning: 2025-12-27T16:33:31.442ZApplied to files:
📚 Learning: 2025-09-23T12:35:35.508ZApplied to files:
📚 Learning: 2025-09-04T15:27:18.112ZApplied to files:
📚 Learning: 2025-11-14T14:46:51.257ZApplied to files:
🧬 Code graph analysis (1)builder/main.py (2)
🪛 markdownlint-cli2 (0.18.1)examples/arduino-spiffs/README.md21-21: Fenced code blocks should have a language specified (MD040, fenced-code-language) 70-70: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🪛 Ruff (0.14.10)builder/main.py601-601: Consider moving this statement to an (TRY300) 603-603: Do not catch blind exception: (BLE001) 1351-1351: Unused function argument: (ARG001) 1351-1351: Unused function argument: (ARG001) 1436-1436: Consider moving this statement to an (TRY300) 1438-1438: Do not catch blind exception: (BLE001) builder/spiffsgen.py55-55: Avoid specifying long messages outside the exception class (TRY003) 73-73: Value being cast to Remove unnecessary (RUF046) 104-107: Mutable class attributes should be annotated with (RUF012) 109-114: Mutable class attributes should be annotated with (RUF012) 297-297: Loop control variable Rename unused (B007) 322-322: Within an (B904) 322-322: Avoid specifying long messages outside the exception class (TRY003) 391-391: Avoid specifying long messages outside the exception class (TRY003) 403-403: Avoid specifying long messages outside the exception class (TRY003) 443-443: Within an (B904) 493-493: Avoid specifying long messages outside the exception class (TRY003) 591-591: Unpacked variable Prefix it with an underscore or any other dummy variable pattern (RUF059) 636-636: Loop control variable (B007) 751-751: Loop control variable Rename unused (B007) ⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (19)
🔇 Additional comments (16)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
examples/arduino-spiffs/README.md (2)
21-32: Minor: Add language specifier to fenced code block.The directory structure block is missing a language specifier. Using
textor leaving empty with just triple backticks satisfies linters.🔎 Suggested fix
-``` +```text arduino-spiffs/ ├── data/ # Files to be included in SPIFFS image
70-78: Minor: Add language specifier to expected output block.Same issue as above—add
textlanguage specifier for the expected serial output block.🔎 Suggested fix
-``` +```text SPIFFS mounted successfully Total space: 1507328examples/arduino-spiffs/src/spiffs_test.ino (1)
109-161: Consider adding data verification to the I/O benchmark.The current benchmark tests throughput but doesn't verify data integrity. For a more comprehensive example, consider:
- Initializing the buffer with known data (e.g., sequential pattern)
- After reading, verifying that the read data matches what was written
This would demonstrate proper testing practices and help catch potential filesystem corruption issues.
💡 Optional enhancement for data verification
void testFileIO(fs::FS &fs, const char *path) { Serial.printf("Testing file I/O with %s\r\n", path); static uint8_t buf[512]; + // Initialize buffer with test pattern + for (int j = 0; j < 512; j++) { + buf[j] = j & 0xFF; + } + size_t len = 0; File file = fs.open(path, FILE_WRITE); if (!file) { Serial.println("- failed to open file for writing"); return; } size_t i; Serial.print("- writing"); uint32_t start = millis(); for (i = 0; i < 2048; i++) { if ((i & 0x001F) == 0x001F) { Serial.print("."); } file.write(buf, 512); } Serial.println(""); uint32_t end = millis() - start; Serial.printf(" - %u bytes written in %lu ms\r\n", 2048 * 512, end); file.close(); + static uint8_t readBuf[512]; file = fs.open(path); start = millis(); end = start; i = 0; + bool dataValid = true; if (file && !file.isDirectory()) { len = file.size(); size_t flen = len; start = millis(); Serial.print("- reading"); while (len) { size_t toRead = len; if (toRead > 512) { toRead = 512; } - file.read(buf, toRead); + file.read(readBuf, toRead); + // Verify data integrity + for (size_t j = 0; j < toRead && dataValid; j++) { + if (readBuf[j] != (j & 0xFF)) { + dataValid = false; + } + } if ((i++ & 0x001F) == 0x001F) { Serial.print("."); } len -= toRead; } Serial.println(""); end = millis() - start; Serial.printf("- %u bytes read in %lu ms\r\n", flen, end); + Serial.printf("- data integrity: %s\r\n", dataValid ? "✓ PASS" : "✗ FAIL"); file.close(); } else { Serial.println("- failed to open file for reading"); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
examples/arduino-spiffs/data/partitions.csvis excluded by!**/*.csvexamples/arduino-spiffs/partitions.csvis excluded by!**/*.csv
📒 Files selected for processing (11)
builder/main.pybuilder/spiffsgen.pyexamples/arduino-spiffs/.gitignoreexamples/arduino-spiffs/README.mdexamples/arduino-spiffs/data/README.mdexamples/arduino-spiffs/data/platformio.iniexamples/arduino-spiffs/data/test.txtexamples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/src/spiffs_test.inoplatform.jsonplatform.py
💤 Files with no reviewable changes (2)
- platform.json
- platform.py
🧰 Additional context used
🧠 Learnings (24)
📓 Common learnings
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/main.py:49-50
Timestamp: 2025-09-22T14:47:08.855Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers that critical setup operations like platform.setup_python_env should crash immediately if they fail rather than having fallback mechanisms, as this ensures problems are detected early and clearly rather than being masked.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/penv_setup.py:612-647
Timestamp: 2025-09-23T16:55:39.788Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers no fallback mechanisms in penv setup functions. The penv virtual environment setup must work properly and should crash immediately rather than falling back to host Python environments, to avoid using "unknown env" configurations.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-12-23T17:13:00.004Z
Learning: In pioarduino/platform-espressif32 (builder/main.py), the LittleFS filesystem image builder uses parameters that intentionally differ from ESP-IDF defaults (read_size=1, prog_size=1, cache_size=block_size, lookahead_size=32, name_max=64 instead of ESP-IDF's 16, 16, 16, 16, 255 respectively). These values were determined through Arduino real-world testing and are necessary to prevent the firmware from reformatting the created filesystem image. Using ESP-IDF defaults causes reformatting issues.
📚 Learning: 2025-12-27T12:16:48.649Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 370
File: examples/arduino-fatfs/README.md:123-123
Timestamp: 2025-12-27T12:16:48.649Z
Learning: In the pioarduino/platform-espressif32 repository, README.md files under examples/* are treated as test data fixtures for testing and may not meet production documentation standards. When reviewing, do not require these example READMEs to have production-grade accuracy; prioritize production docs elsewhere and keep example READMEs lightweight and suitable for their test purposes.
Applied to files:
examples/arduino-spiffs/README.md
📚 Learning: 2025-12-27T12:14:31.697Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 370
File: examples/arduino-fatfs/data/README.md:218-224
Timestamp: 2025-12-27T12:14:31.697Z
Learning: In the pioarduino/platform-espressif32 project, files within `examples/*/data/` directories may be dummy test data or test fixtures used for filesystem image creation, and do not require the same accuracy or correctness standards as production documentation.
Applied to files:
examples/arduino-spiffs/README.mdexamples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.iniexamples/arduino-spiffs/src/spiffs_test.inoexamples/arduino-spiffs/.gitignoreexamples/arduino-spiffs/data/test.txt
📚 Learning: 2025-12-23T17:13:00.004Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-12-23T17:13:00.004Z
Learning: In pioarduino/platform-espressif32 (builder/main.py), the LittleFS filesystem image builder uses parameters that intentionally differ from ESP-IDF defaults (read_size=1, prog_size=1, cache_size=block_size, lookahead_size=32, name_max=64 instead of ESP-IDF's 16, 16, 16, 16, 255 respectively). These values were determined through Arduino real-world testing and are necessary to prevent the firmware from reformatting the created filesystem image. Using ESP-IDF defaults causes reformatting issues.
Applied to files:
examples/arduino-spiffs/README.mdexamples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.iniexamples/arduino-spiffs/src/spiffs_test.inobuilder/main.pybuilder/spiffsgen.py
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 199
File: examples/arduino-blink/platformio.ini:81-83
Timestamp: 2025-06-26T09:56:30.658Z
Learning: In examples/arduino-blink/platformio.ini, the different lib_ignore configurations across environments (some including wifi, some not, different syntax styles) are intentional test cases, not inconsistencies that need to be fixed.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.iniexamples/arduino-spiffs/.gitignoreexamples/arduino-spiffs/data/README.md
📚 Learning: 2025-08-20T14:13:13.842Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 263
File: platform.py:80-84
Timestamp: 2025-08-20T14:13:13.842Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to always override IDF_TOOLS_PATH environment variable with PlatformIO's core directory, rather than respecting any existing user-defined IDF_TOOLS_PATH values. This ensures consistent ESP32 tool management within PlatformIO.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.inibuilder/main.py
📚 Learning: 2025-09-23T14:15:00.476Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T14:15:00.476Z
Learning: In the pioarduino/platform-espressif32 project, espidf.py creates an extra/separate virtual environment from the penv setup, so cryptography version differences between penv_setup.py and espidf.py do not cause conflicts since they operate in isolated environments.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.inibuilder/main.py
📚 Learning: 2025-12-27T12:16:48.649Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 370
File: examples/arduino-fatfs/README.md:123-123
Timestamp: 2025-12-27T12:16:48.649Z
Learning: In the pioarduino/platform-espressif32 project, README.md files within `examples/*/` directories (e.g., examples/arduino-fatfs/README.md) may be dummy test data or test fixtures used for testing purposes, and do not require the same accuracy or correctness standards as production documentation.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.iniexamples/arduino-spiffs/src/spiffs_test.inoexamples/arduino-spiffs/.gitignoreexamples/arduino-spiffs/data/README.mdexamples/arduino-spiffs/data/test.txt
📚 Learning: 2025-08-08T09:50:01.969Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:50:01.969Z
Learning: Preference in pioarduino/platform-espressif32 (builder/frameworks/espidf.py): Avoid using {"name": "boot"} as a sentinel when resolving the default app partition offset. Instead, call parttool.py with --partition-boot-default directly (and optionally fall back to factory/ota_0) to prevent confusion with non-existent CSV names.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.ini
📚 Learning: 2025-09-23T14:15:00.476Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T14:15:00.476Z
Learning: In the pioarduino/platform-espressif32 project, espidf.py creates and manages its own separate IDF virtual environment (accessed via get_python_exe() from get_idf_venv_dir()). This IDF venv is completely isolated from the penv setup, so different cryptography versions between penv_setup.py and espidf.py don't cause conflicts as they exist in separate virtual environments.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.inibuilder/main.py
📚 Learning: 2025-08-10T19:13:25.599Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 255
File: builder/penv_setup.py:26-40
Timestamp: 2025-08-10T19:13:25.599Z
Learning: In the pioarduino/platform-espressif32 repository's builder/penv_setup.py file, semantic_version is imported but intentionally not listed in python_deps because it's a transitive dependency of platformio-core (version 2.10.*). When platformio is installed from the GitHub URL, semantic_version is automatically installed as one of its dependencies.
Applied to files:
examples/arduino-spiffs/platformio.ini
📚 Learning: 2025-08-05T11:47:41.756Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 246
File: builder/main.py:0-0
Timestamp: 2025-08-05T11:47:41.756Z
Learning: In the ESP32 platform builder, subprocess calls correctly use the virtual environment Python by explicitly passing PYTHON_EXE (which is set to the venv python path) as the first argument in the command array, rather than relying on environment variable propagation. The env=os.environ parameter only affects environment variables, not the executable path itself.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.inibuilder/main.py
📚 Learning: 2025-08-08T09:55:50.842Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:55:50.842Z
Learning: In pioarduino/platform-espressif32 (builder/frameworks/espidf.py, Python), for get_app_partition_offset: preferred behavior is to probe app/ota_0 first, then app/factory; avoid using the {"name":"boot"} sentinel; make probes non-fatal (don’t exit on missing partitions), optionally fall back to calling parttool.py with --partition-boot-default, then default to 0x10000.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.ini
📚 Learning: 2025-09-22T14:47:08.855Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/main.py:49-50
Timestamp: 2025-09-22T14:47:08.855Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers that critical setup operations like platform.setup_python_env should crash immediately if they fail rather than having fallback mechanisms, as this ensures problems are detected early and clearly rather than being masked.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.inibuilder/main.py
📚 Learning: 2025-08-26T14:52:29.044Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 268
File: builder/penv_setup.py:87-96
Timestamp: 2025-08-26T14:52:29.044Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 prefers to trust system executables when falling back to PATH-based command resolution (like "uv"), rather than implementing additional validation checks for security concerns.
Applied to files:
examples/arduino-spiffs/platformio.iniexamples/arduino-spiffs/data/platformio.ini
📚 Learning: 2025-11-14T14:46:51.257Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 338
File: builder/frameworks/component_manager.py:1312-1380
Timestamp: 2025-11-14T14:46:51.257Z
Learning: In the pioarduino/platform-espressif32 project, the `pioarduino-build.py` file has a fixed format that does not change, so exact string matching (e.g., `'CCFLAGS=['`) is acceptable and regex-based flexible matching is not necessary when modifying this file.
Applied to files:
examples/arduino-spiffs/data/platformio.ini
📚 Learning: 2025-09-04T15:27:18.112Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/component_manager.py:1047-1054
Timestamp: 2025-09-04T15:27:18.112Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers not to add guard checks for missing arduino_libs_mcu in backup operations, preferring to let the operation proceed rather than silently skip when the directory might be missing.
Applied to files:
examples/arduino-spiffs/data/platformio.ini
📚 Learning: 2025-09-23T12:35:35.508Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-09-23T12:35:35.508Z
Learning: For the pioarduino/platform-espressif32 project, Jason2866 has explicitly instructed to NEVER create issues or PRs outside of his repositories (pioarduino/*). All issue and PR creation must be strictly limited to his own repositories only, never in upstream or other repositories.
Applied to files:
examples/arduino-spiffs/data/platformio.ini
📚 Learning: 2025-07-13T20:03:29.695Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 225
File: builder/main.py:99-108
Timestamp: 2025-07-13T20:03:29.695Z
Learning: In the ESP32 platform builder, the subprocess call to get site-packages directory in setup_python_paths() function is intentionally left without error handling because if this basic Python operation fails, it indicates a fundamental system issue that would cause the entire build process to fail anyway.
Applied to files:
builder/main.py
📚 Learning: 2025-08-10T16:59:15.533Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 255
File: builder/penv_setup.py:265-275
Timestamp: 2025-08-10T16:59:15.533Z
Learning: In the ESP32 platform builder's penv_setup.py, the esptool_repo_path used in subprocess calls comes from PlatformIO's internal package management system (platform.get_package_dir()) which is a trusted source. Command injection concerns are not applicable here since this is build infrastructure code with controlled inputs, not user-facing code accepting arbitrary input.
Applied to files:
builder/main.py
📚 Learning: 2025-10-19T17:30:42.403Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-10-19T17:30:42.403Z
Learning: The tool-esp-rom-elfs package must remain in COMMON_IDF_PACKAGES in platform.py because espidf.py requires it to set the ESP_ROM_ELF_DIR environment variable for the ESP-IDF build system, independent of the exception decoder monitor filter usage.
Applied to files:
builder/main.py
📚 Learning: 2025-09-23T16:55:39.788Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 296
File: builder/penv_setup.py:612-647
Timestamp: 2025-09-23T16:55:39.788Z
Learning: In the pioarduino/platform-espressif32 project, Jason2866 prefers no fallback mechanisms in penv setup functions. The penv virtual environment setup must work properly and should crash immediately rather than falling back to host Python environments, to avoid using "unknown env" configurations.
Applied to files:
builder/main.py
📚 Learning: 2025-09-05T11:58:49.681Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 281
File: builder/frameworks/ulp.py:165-178
Timestamp: 2025-09-05T11:58:49.681Z
Learning: In the pioarduino/platform-espressif32 project, cmake executable paths in build scripts (like ulp.py) are provided as paths to the build engine (SCons/PlatformIO) rather than direct executable calls. The build engine handles cross-platform executable resolution automatically, so there's no need to add Windows .exe suffixes or special quoting - the current approach with str(Path(...) / "bin" / "cmake") works correctly across platforms.
Applied to files:
builder/main.py
📚 Learning: 2025-12-27T12:14:31.697Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 370
File: examples/arduino-fatfs/data/README.md:218-224
Timestamp: 2025-12-27T12:14:31.697Z
Learning: In the repository pioarduino/platform-espressif32, files under examples/*/data/ (e.g., README.md in that path) are likely dummy test data or fixtures for filesystem image creation and do not need to meet production documentation accuracy standards. When reviewing code or docs in these paths, treat them as non-production examples and focus on their functional intent for tests rather than full documentation quality.
Applied to files:
examples/arduino-spiffs/data/README.md
🪛 markdownlint-cli2 (0.18.1)
examples/arduino-spiffs/README.md
21-21: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
70-70: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.14.10)
builder/main.py
601-601: Consider moving this statement to an else block
(TRY300)
603-603: Do not catch blind exception: Exception
(BLE001)
1351-1351: Unused function argument: target
(ARG001)
1351-1351: Unused function argument: source
(ARG001)
1436-1436: Consider moving this statement to an else block
(TRY300)
1438-1438: Do not catch blind exception: Exception
(BLE001)
builder/spiffsgen.py
55-55: Avoid specifying long messages outside the exception class
(TRY003)
73-73: Value being cast to int is already an integer
Remove unnecessary int call
(RUF046)
104-107: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
109-114: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
297-297: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
322-322: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
322-322: Avoid specifying long messages outside the exception class
(TRY003)
391-391: Avoid specifying long messages outside the exception class
(TRY003)
403-403: Avoid specifying long messages outside the exception class
(TRY003)
443-443: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
493-493: Avoid specifying long messages outside the exception class
(TRY003)
591-591: Unpacked variable obj_type is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
616-616: Loop control variable obj_id not used within loop body
(B007)
633-633: Loop control variable span_ix not used within loop body
(B007)
748-748: Loop control variable dirs not used within loop body
Rename unused dirs to _dirs
(B007)
🔇 Additional comments (18)
examples/arduino-spiffs/.gitignore (1)
1-3: LGTM!Standard PlatformIO project ignores plus
unpacked_fs/for the SPIFFS extraction output directory, consistent with the_get_unpack_dirdefault inbuilder/main.py.examples/arduino-spiffs/data/test.txt (1)
1-2: LGTM!Simple test fixture for SPIFFS image creation testing. Based on learnings, files in
examples/*/data/are test data and don't require production accuracy.examples/arduino-spiffs/data/platformio.ini (1)
1-6: LGTM!This file resides in the
data/directory and serves as test data to be included in the SPIFFS image—not as the actual project configuration. Based on learnings, files inexamples/*/data/are test fixtures for filesystem image creation.examples/arduino-spiffs/data/README.md (1)
1-12: LGTM!Documentation for the SPIFFS test data directory. Per learnings, files in
examples/*/data/are test fixtures and don't require production documentation standards.builder/spiffsgen.py (1)
1-6: Espressif-originated SPIFFS generator integrated cleanly.This module is adapted from ESP-IDF's
spiffsgen.pyand provides complete SPIFFS image generation and extraction capabilities. The implementation correctly handles:
- Page/block layout and binary serialization
- Object lookup tables with magic number support
- File extraction with proper span ordering
builder/main.py (6)
54-61: LGTM! Dynamic SPIFFS module loading.The dynamic loading pattern using
importlib.utilis consistent with the existing board script loading approach in this file. Registering the module insys.modulesprevents redundant reloading.
549-561: Config precedence is correctly implemented.The loop order
["common", "env:" + env["PIOENV"]]with sequential overwrites ensures environment-specific settings take precedence over common settings—opposite but equivalent to the break-on-first-match pattern used elsewhere.
778-790: Clean filesystem router implementation.The router cleanly dispatches to the appropriate builder based on filesystem type, improving maintainability over inline conditionals.
1351-1361: LGTM! SPIFFS download implementation.The function correctly:
- Uses
[0x82]filter for SPIFFS partition subtype- Reads SPIFFS configuration consistently with
build_spiffs_image- Uses the shared
_download_partition_imagehelperThe unused
targetandsourceparameters are required by the SCons action signature.
792-806: LGTM! LDF targets correctly updated.The
download_spiffstarget is correctly added to the set of filesystem targets that disable Library Dependency Finder, consistent with other download targets.
1789-1795: LGTM! Platform target correctly registered.The
download_spiffstarget is registered withNonedependencies, correctly indicating no build is required before downloading the filesystem.examples/arduino-spiffs/README.md (1)
1-247: Comprehensive SPIFFS example documentation.The README provides thorough coverage of SPIFFS usage including build commands, troubleshooting, configuration options, and a comparison table with other filesystems. Based on learnings, example READMEs are test fixtures and don't require production documentation standards.
examples/arduino-spiffs/platformio.ini (1)
1-6: No action needed. Thepartitions.csvfile exists in the example directory and correctly defines a SPIFFS partition at offset0x290000with size0x170000. The platformio.ini configuration is valid.examples/arduino-spiffs/src/spiffs_test.ino (5)
1-11: LGTM! Well-structured configuration.The includes are correct for SPIFFS usage, and the test configuration flags are clearly documented. Defaulting
FORMAT_SPIFFStofalseis a good safety measure to prevent accidental data loss.
12-107: LGTM! Clean filesystem utility functions.All utility functions demonstrate proper error handling patterns:
- Validate file/directory open operations
- Check return values for filesystem operations
- Close file handles appropriately
- Provide clear diagnostic output
This is excellent educational example code for SPIFFS usage.
163-231: LGTM! Comprehensive test coverage.The test suite functions demonstrate excellent testing practices:
testExistingFilessafely checks file existence before readingtestWriteOperationsincludes verification steps after each operationtestFileSystemprovides clear diagnostic output with formatted space usageThis provides a thorough validation of SPIFFS functionality.
237-324: LGTM! Excellent test orchestration and user guidance.The
setup()function demonstrates best practices for example code:
- Clear progress indicators throughout execution
- Helpful error messages with troubleshooting steps (lines 268-272)
- Conditional test execution based on configuration flags
- User-friendly completion message with next-step instructions (lines 314-318)
- Intentionally keeps SPIFFS mounted in
loop()for interactive useThis is exemplary educational code that guides users through SPIFFS testing while providing actionable feedback.
1-324: Excellent SPIFFS example sketch!This is well-crafted example code that effectively demonstrates ESP32 SPIFFS functionality. The sketch provides:
✅ Comprehensive coverage: filesystem info, directory listing, CRUD operations, and I/O benchmarking
✅ Robust error handling: validates operations and provides clear diagnostic output
✅ User-friendly design: helpful error messages, configurable tests, and actionable next steps
✅ Educational value: demonstrates proper SPIFFS API usage patterns for Arduino/ESP32The code is production-ready for its intended purpose as an example/test fixture and aligns well with the PR's objective of demonstrating Python-based SPIFFS tooling integration.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
@coderabbitai about your comments of the spiffs arduino example. The example is only for testing the general SPIFFS FS function. Not ment for something usefull |
|
Tip For best results, initiate chat on the files or code changes. @Jason2866, thank you for the clarification! I understand that the Arduino SPIFFS example is purely for testing the general SPIFFS filesystem functionality and not intended as a production-ready reference. I've noted this for future reviews and won't apply the same standards as I would for production code. ✏️ Learnings added
🧠 Learnings used |
Checklist:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.