Refactor: Use Littlefs python instead of mklittlefs binaries #368
Conversation
Removed optional tools 'tool-mklittlefs' and 'tool-mklittlefs4' from platform.json.
Added a function to build filesystem images using python LittleFS, including error handling and metadata management for files and directories.
WalkthroughThe changes replace external LittleFS build tools with a direct Python library integration. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 0
🧹 Nitpick comments (1)
builder/main.py (1)
506-514: Consider more specific exception handling in the main error handler.The bare
Exceptioncatch on line 512 is quite broad. While it does print an error message, consider catching more specific exceptions or adding additional context to help diagnose issues.🔎 Optional improvement for exception handling
except Exception as e: - print(f"Error building filesystem image: {e}") + import traceback + print(f"Error building filesystem image: {type(e).__name__}: {e}") + print(traceback.format_exc()) return 1This provides more diagnostic information while maintaining the same error recovery behavior.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
builder/main.pybuilder/penv_setup.pyplatform.jsonplatform.py
💤 Files with no reviewable changes (1)
- platform.json
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
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: 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: 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: 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: 285
File: platform.py:670-683
Timestamp: 2025-09-09T18:07:14.583Z
Learning: In the pioarduino/platform-espressif32 project, when tools.json exists in mklittlefs tool directories, it triggers a reinstall via install_tool(). This is intentional behavior to convert tools installed via idf_tools.py method into the platform's tool management system, rather than a redundant check that should be avoided.
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: 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.
📚 Learning: 2025-09-09T18:07:14.583Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 285
File: platform.py:670-683
Timestamp: 2025-09-09T18:07:14.583Z
Learning: In the pioarduino/platform-espressif32 project, when tools.json exists in mklittlefs tool directories, it triggers a reinstall via install_tool(). This is intentional behavior to convert tools installed via idf_tools.py method into the platform's tool management system, rather than a redundant check that should be avoided.
Applied to files:
platform.py
📚 Learning: 2025-07-29T11:35:26.182Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 237
File: platform.py:200-201
Timestamp: 2025-07-29T11:35:26.182Z
Learning: In platform.py, the `tool-esp_install` package intentionally transitions from mandatory to optional status after successful installation. Initially it's needed and mandatory for installation, but becomes optional once installed since it's no longer required for subsequent operations. Setting `self.packages[tl_install_name]["optional"] = True` after version verification is the intended design pattern.
Applied to files:
platform.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:
platform.pybuilder/penv_setup.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:
platform.pybuilder/penv_setup.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:
builder/penv_setup.py
📚 Learning: 2025-08-10T19:12:17.988Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 255
File: builder/penv_setup.py:19-19
Timestamp: 2025-08-10T19:12:17.988Z
Learning: In PlatformIO ESP32 platform builds, `semantic_version` is a dependency provided by PlatformIO itself and is guaranteed to be available when the build scripts run. The module is already imported at the top level in existing files like `builder/frameworks/espidf.py`, so lazy-loading it is unnecessary.
Applied to files:
builder/penv_setup.py
📚 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/penv_setup.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:
builder/penv_setup.py
📚 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:
builder/penv_setup.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 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:
builder/penv_setup.py
🪛 Ruff (0.14.10)
builder/main.py
490-491: try-except-pass detected, consider logging the exception
(S110)
490-490: Do not catch blind exception: Exception
(BLE001)
503-504: try-except-pass detected, consider logging the exception
(S110)
503-503: Do not catch blind exception: Exception
(BLE001)
510-510: Consider moving this statement to an else block
(TRY300)
512-512: Do not catch blind exception: Exception
(BLE001)
⏰ 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). (20)
- GitHub Check: build (macos-15, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (ubuntu-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-SampleScan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (8)
builder/main.py (5)
24-24: LGTM!The import statement correctly brings in the LittleFS class from the newly added littlefs-python dependency.
418-461: LGTM! Well-structured parameter extraction and version parsing.The function correctly extracts filesystem parameters and handles version parsing with appropriate fallbacks. The version format conversion from "2.1" string to the integer format
(major << 16) | minoris correct for LittleFS compatibility.
484-504: Exception handling for timestamps is appropriate.The bare exception catches with
passstatements (lines 490-491, 503-504) are acceptable here because timestamp preservation is non-critical metadata. If timestamp setting fails, the build should continue successfully. The static analysis warning can be safely ignored in this context.
662-670: LGTM! Clean integration of Python-based LittleFS builder.The changes correctly route LittleFS image building through the new
build_fs_imagefunction while preserving existing behavior for SPIFFS and other filesystems. The conditional logic properly applies the-pand-bflags only for SPIFFS.
462-475: Several LittleFS parameters do not match ESP-IDF defaults and require clarification.The code uses
read_size=1,prog_size=1,cache_size=block_size,lookahead_size=32, andname_max=64, which differ from ESP-IDF's defaults ofread_size=16,prog_size=16,cache_size=16,lookahead_size=16, andname_max=255. Additionally, the comment forname_max=64incorrectly claims this is the "ESP-IDF default filename length" when the actual default is 255. Either align these parameters with ESP-IDF defaults or document why these intentional deviations are necessary for compatibility.platform.py (2)
721-727: Excellent simplification aligned with PR objectives.The removal of the
for_downloadparameter and mklittlefs-related logic correctly reflects the migration to Python-based LittleFS handling. The function now cleanly installs tools only for fatfs and spiffs, while LittleFS is handled by the littlefs-python dependency.
739-740: LGTM! Consistent with simplified tool installation.The call to
_install_filesystem_toolcorrectly passes only the filesystem parameter, matching the updated function signature.builder/penv_setup.py (1)
48-48: The littlefs-python package version 0.16.0 is available on PyPI with no known security vulnerabilities. The dependency specification is acceptable.
|
@coderabbitai The code uses read_size=1, prog_size=1, cache_size=block_size, lookahead_size=32, and name_max=64, which differ from ESP-IDF's defaults of read_size=16, prog_size=16, cache_size=16, lookahead_size=16, and name_max=255. Additionally, the comment for name_max=64 incorrectly claims this is the "ESP-IDF default filename length" when the actual default is 255. Either align these parameters with ESP-IDF defaults or document why these intentional deviations are necessary for compatibility. The implementation was tested with Arduino real life examples. Other settings ended in reformatting from the firmware of the created FS image |
|
Tip For best results, initiate chat on the files or code changes. @Jason2866, thank you for the clarification! Understanding that these parameter values were determined through real-world Arduino testing and that using ESP-IDF defaults would cause the firmware to reformat the filesystem image is crucial context. This confirms that the deviations from ESP-IDF defaults are intentional and necessary for compatibility. To make this reasoning clear to future maintainers, I'd recommend updating the comment at that section to reflect the actual rationale. For example: # LittleFS configuration
# Note: These parameters differ from ESP-IDF defaults to prevent
# firmware reformatting of the created filesystem image.
# Tested with Arduino real-life examples.
fs = LittleFS(
block_size=block_size,
block_count=image_size // block_size,
read_size=1, # ESP-IDF default: 16
prog_size=1, # ESP-IDF default: 16
cache_size=block_size, # ESP-IDF default: 16
lookahead_size=32, # ESP-IDF default: 16
name_max=64, # ESP-IDF default: 255
disk_version=disk_version
)This way, the intentional nature of these choices is documented for anyone reviewing the code in the future. ✏️ Learnings added
🧠 Learnings used |
Description:
The PR removes mklittlefs compiled binaries. A modern python based LittleFS implementation is now used.
New platformio option
board_build.littlefs_versionto set the used LittleFS Partition Disk Version.Default is newest version 2.1
Example:
Checklist:
Summary by CodeRabbit
New Features
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.