Enhance Filesystem partition detection#490
Conversation
Amp-Thread-ID: https://ampcode.com/threads/T-019dfe38-e3be-7192-85d9-8dc4b23baeb3 Co-authored-by: Amp <amp@ampcode.com> # Conflicts: # builder/penv_setup.py
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughEnhanced filesystem partition detection in ChangesFilesystem Partition Detection
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
builder/main.py (1)
1206-1207: ⚡ Quick winConstants are function-scoped, not module-level — contradicts the PR summary.
The summary lists
KNOWN_FS_SUBTYPESandDATA_PARTITION_TYPEas new public module-level constants, but they are defined inside_download_partition_imageso they are re-created on every call and are not reusable from other call sites. In particular,download_fs_action(Lines 1673–1679) re-hardcodes0x81/0x82/0x83, which is exactly the duplication these constants were meant to remove.♻️ Proposed fix: hoist to module scope and reuse
@@ filesystem = board.get("build.filesystem", "littlefs") + +# Known filesystem subtypes for ESP32 data partitions: +# 0x81 = FAT, 0x82 = SPIFFS, 0x83 = LittleFS +# All other data subtypes (e.g. 0x00 ota, 0x01 phy, 0x02 nvs, +# 0x03 coredump, 0x04 nvs_keys, 0x05 efuse, 0x06 undefined) must +# be excluded so that e.g. a coredump partition is not mistakenly +# treated as a filesystem partition. +KNOWN_FS_SUBTYPES = (0x81, 0x82, 0x83) +DATA_PARTITION_TYPE = 0x01@@ def _download_partition_image(env, fs_type_filter=None): - # Known filesystem subtypes for ESP32 data partitions: - # 0x81 = FAT, 0x82 = SPIFFS, 0x83 = LittleFS - # All other data subtypes (e.g. 0x00 ota, 0x01 phy, 0x02 nvs, - # 0x03 coredump, 0x04 nvs_keys, 0x05 efuse, 0x06 undefined) must - # be excluded so that e.g. a coredump partition is not mistakenly - # treated as a filesystem partition. - KNOWN_FS_SUBTYPES = (0x81, 0x82, 0x83) - DATA_PARTITION_TYPE = 0x01 # Ensure upload port is setAnd in
download_fs_action, you can then drop the magic numbers in favor of the named constants.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@builder/main.py` around lines 1206 - 1207, KNOWN_FS_SUBTYPES and DATA_PARTITION_TYPE are currently declared inside _download_partition_image so they are recreated per-call and not available module-wide; hoist these constants to the module scope as public constants (retain the uppercase names KNOWN_FS_SUBTYPES and DATA_PARTITION_TYPE) and update all call sites—specifically remove the hardcoded 0x81/0x82/0x83 in download_fs_action and reference KNOWN_FS_SUBTYPES instead, and use DATA_PARTITION_TYPE where appropriate—so the values are defined once and reused across _download_partition_image and download_fs_action.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@builder/main.py`:
- Around line 1206-1207: KNOWN_FS_SUBTYPES and DATA_PARTITION_TYPE are currently
declared inside _download_partition_image so they are recreated per-call and not
available module-wide; hoist these constants to the module scope as public
constants (retain the uppercase names KNOWN_FS_SUBTYPES and DATA_PARTITION_TYPE)
and update all call sites—specifically remove the hardcoded 0x81/0x82/0x83 in
download_fs_action and reference KNOWN_FS_SUBTYPES instead, and use
DATA_PARTITION_TYPE where appropriate—so the values are defined once and reused
across _download_partition_image and download_fs_action.
Description:
Related issue (if applicable): fixes #
Checklist:
Summary by CodeRabbit