Skip to content

enhance fs detection from dl image#374

Merged
Jason2866 merged 1 commit into
developfrom
enh_fs_detect
Dec 31, 2025
Merged

enhance fs detection from dl image#374
Jason2866 merged 1 commit into
developfrom
enh_fs_detect

Conversation

@Jason2866
Copy link
Copy Markdown

@Jason2866 Jason2866 commented Dec 31, 2025

Checklist:

  • The pull request is done against the latest develop branch
  • Only relevant files were touched
  • Only one feature/fix was added per PR, more changes are allowed when changing boards.json
  • I accept the CLA

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced filesystem type detection during downloads to more reliably identify LittleFS, FAT, and SPIFFS formats with improved accuracy and multi-step verification, providing detailed detection results.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Dec 31, 2025

📝 Walkthrough

Walkthrough

The PR rewrites filesystem type detection in builder/main.py's download_fs_action function to implement multi-step detection. It now checks for LittleFS magic at offset 8, scans multiple FAT boot sector offsets (0, 4096, 8192), and falls back to partition-subtype logic. The enhanced approach replaces simple subtype-based detection with offset-aware verification.

Changes

Cohort / File(s) Summary
Filesystem Type Detection Enhancement
builder/main.py
Rewrites download_fs_action filesystem type detection with multi-step logic: checks LittleFS magic signature at superblock offset 8, scans multiple FAT boot sector offsets with byte-per-sector verification, and falls back to partition-subtype classification (0x81 fatfs, 0x82 spiffs/littlefs, 0x83 littlefs) with fallback warning. Maintains existing behavior for path preparation and extraction while printing detailed detection results.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

Poem

🐰 Bytes we scan, offsets explored,
LittleFS magic, FAT restored,
Nine sixteen sectors, we now detect,
Filesystems found—each one correct! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'enhance fs detection from dl image' directly describes the main change: improving filesystem type detection from downloaded images.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@Jason2866
Copy link
Copy Markdown
Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Dec 31, 2025

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
builder/main.py (1)

1522-1570: Excellent enhancement to filesystem detection!

The multi-step detection logic is well-designed and robust:

  • LittleFS magic check at offset 8 is straightforward and reliable
  • FAT detection validates boot signature (0x55AA), checks for filesystem markers (FAT/MSDOS/MSWIN), and confirms valid bytes_per_sector values
  • Checking multiple FAT offsets (0, 4096, 8192) properly handles wear-leveling scenarios
  • Appropriate length checks prevent out-of-bounds access
  • Fallback to partition subtype provides a safety net when signature-based detection fails

The handling of subtype 0x82 ambiguity (can be SPIFFS or LittleFS) is reasonable, relying on the earlier LittleFS magic check to have caught LittleFS filesystems.

Optional: Add explanatory comment for header size

Consider adding a brief comment explaining the 16384-byte header read size:

-    # Read header for detailed filesystem detection
+    # Read header for detailed filesystem detection
+    # 16KB covers FAT boot sectors at offsets up to 8192 plus 512-byte boot sector
     with open(fs_file, 'rb') as f:
         header = f.read(16384)  # Read more to check for offset FAT
📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da4c1ec and 8badb74.

📒 Files selected for processing (1)
  • builder/main.py
🧰 Additional context used
🧠 Learnings (2)
📓 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: 371
File: builder/spiffsgen.py:620-629
Timestamp: 2025-12-27T16:33:38.710Z
Learning: In the pioarduino/platform-espressif32 project, when extracting SPIFFS files in builder/spiffsgen.py, Jason2866 prefers to check if rel_path is empty after lstrip('/') and skip such files with a warning message, rather than attempting to handle empty parent directories in os.makedirs calls. This treats empty paths as invalid/corrupted data.
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: 0
File: :0-0
Timestamp: 2025-12-28T16:31:36.068Z
Learning: In pioarduino/platform-espressif32 (builder/main.py), the LittleFS block size is fixed at 4096 bytes (0x1000) for all LittleFS operations. Auto-detection or trying multiple block sizes is unnecessary complexity.
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.
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-12-23T17:13:00.012Z
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.
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.
📚 Learning: 2025-12-23T17:13:00.012Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 0
File: :0-0
Timestamp: 2025-12-23T17:13:00.012Z
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:

  • builder/main.py

@Jason2866 Jason2866 merged commit 20457f9 into develop Dec 31, 2025
68 checks passed
@Jason2866 Jason2866 deleted the enh_fs_detect branch December 31, 2025 16:03
@coderabbitai coderabbitai Bot mentioned this pull request May 6, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant