Add download_littlefs target and function#369
Conversation
Updated the switch_off_ldf function to include 'download_littlefs' in the targets. Added a new function 'download_littlefs' to download and extract the LittleFS filesystem from the device.
📝 WalkthroughWalkthroughAdded a new public build target and function Changes
Sequence DiagramsequenceDiagram
autonumber
participant CLI as PlatformIO CLI
participant Builder as Build Target (download_littlefs)
participant Device as Target Device / Serial
participant Parser as Partition Table Parser
participant Extractor as LittleFS Extractor
participant Disk as Local Disk
rect rgba(200,230,255,0.25)
CLI->>Builder: invoke download_littlefs(target, source, env)
end
rect rgba(220,255,200,0.18)
Builder->>Device: request partition table bytes
Device-->>Builder: return partition table blob
Builder->>Parser: parse table, find LittleFS (0x82/0x83)
Parser-->>Builder: partition start & size
end
rect rgba(255,240,200,0.18)
Builder->>Device: request image bytes (start,size)
Device-->>Builder: stream image bytes
Builder->>Extractor: mount/inspect image, enumerate files
Extractor-->>Builder: file list & contents
Builder->>Disk: write files to unpack dir
Disk-->>Builder: extraction complete
end
note right of Builder: Errors at any step raise\nexceptions and abort extraction
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~20 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: 1
🧹 Nitpick comments (3)
builder/main.py (3)
1040-1086: Ensurefs.unmount()is called even on exception.If an exception occurs during file extraction (lines 1049-1074),
fs.unmount()at line 1076 is skipped, potentially leaving the filesystem in an inconsistent state. Usetry/finallyfor proper cleanup.🔎 Proposed fix
fs.context.buffer = bytearray(fs_data) fs.mount() - # Extract all files - file_count = 0 - print("\nExtracted files:") - for root, dirs, files in fs.walk("/"): - ... - - fs.unmount() - print(f"\nSuccessfully extracted {file_count} file(s) to {unpack_dir}") - return 0 + try: + # Extract all files + file_count = 0 + print("\nExtracted files:") + for root, dirs, files in fs.walk("/"): + # ... extraction logic ... + + print(f"\nSuccessfully extracted {file_count} file(s) to {unpack_dir}") + return 0 + finally: + fs.unmount() except Exception as e:
977-993: Minor cleanup: unused variables and f-strings without placeholders.Per static analysis:
- Line 982:
page_sizeis assigned but never used—remove or use it- Lines 978, 988, 993: f-strings without placeholders can be regular strings
🔎 Proposed cleanup
if fs_subtype not in [0x82, 0x83]: - print(f"Error: Unsupported filesystem partition type") + print("Error: Unsupported filesystem partition type") return 1 block_size = 0x1000 # 4KB - page_size = 0x100 # 256 bytes print(f"Found filesystem partition (subtype {hex(fs_subtype)}):") print(f" Start: {hex(fs_start)}") print(f" Size: {hex(fs_size)} ({fs_size} bytes)") print(f" Block size: {hex(block_size)}") - print(f"Note: This tool only supports LittleFS extraction") + print("Note: This tool only supports LittleFS extraction") # Download filesystem image fs_file = build_dir / f"downloaded_fs_{hex(fs_start)}_{hex(fs_size)}.bin" - print(f"\nDownloading filesystem from device...") + print("\nDownloading filesystem from device...")
1026-1030: Consider movingshutilimport to module level.The
shutilimport at line 1028 is inside the function. While this works for lazy loading, sinceshutilis a lightweight stdlib module andrmtreeis used in this commonly executed path, moving it to the top-level imports would be more conventional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/main.py
🧰 Additional context used
🧠 Learnings (1)
📓 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: 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.
🪛 Ruff (0.14.10)
builder/main.py
880-880: Unused function argument: target
(ARG001)
880-880: Unused function argument: source
(ARG001)
922-922: subprocess call: check for execution of untrusted input
(S603)
924-924: f-string without any placeholders
Remove extraneous f prefix
(F541)
929-929: Do not catch blind exception: Exception
(BLE001)
966-966: Local variable fs_type_name is assigned to but never used
Remove assignment to unused variable fs_type_name
(F841)
978-978: f-string without any placeholders
Remove extraneous f prefix
(F541)
982-982: Local variable page_size is assigned to but never used
Remove assignment to unused variable page_size
(F841)
988-988: f-string without any placeholders
Remove extraneous f prefix
(F541)
993-993: f-string without any placeholders
Remove extraneous f prefix
(F541)
1009-1009: subprocess call: check for execution of untrusted input
(S603)
1016-1016: Do not catch blind exception: Exception
(BLE001)
1078-1078: Consider moving this statement to an else block
(TRY300)
1080-1080: 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 (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-hello-world)
- GitHub Check: build (ubuntu-latest, examples/espidf-exceptions)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-http-request)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-rmt-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-NimBLE-SampleScan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (3)
builder/main.py (3)
530-543: LGTM!The addition of
"download_littlefs"to thefs_targetsset correctly disables LDF scanning for this new target, consistent with other filesystem operations.
1325-1331: LGTM!The platform target registration correctly specifies
Nonefor dependencies since this target downloads from the device rather than building from source.
947-967: Partition parsing reads 3 bytes instead of 4 for offset/size.ESP-IDF partition entries use 4-byte little-endian values for both offset and size fields. The current code reads only 3 bytes (
entry[2:5]andentry[6:9]), which works for partitions under 16MB but will fail for larger configurations.🔎 Proposed fix
# Check for SPIFFS (0x82) or LITTLEFS (0x83) if part_type in [0x82, 0x83]: - fs_start = int.from_bytes(entry[2:5], byteorder='little', signed=False) - fs_size = int.from_bytes(entry[6:9], byteorder='little', signed=False) + fs_start = int.from_bytes(entry[2:6], byteorder='little', signed=False) + fs_size = int.from_bytes(entry[6:10], byteorder='little', signed=False) fs_subtype = part_type - fs_type_name = "LittleFS" if part_type == 0x83 else "SPIFFS" breakAlso,
fs_type_nameis assigned but never used—consider removing it per static analysis hint.⛔ Skipped due to learnings
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: 252 File: builder/frameworks/espidf.py:1382-1388 Timestamp: 2025-08-08T10:04:46.044Z Learning: When discussing espidf.py get_app_partition_offset behavior with Jason2866, they prefer the OTA-first then factory order and consider using the "boot" sentinel undesirable. Clarifications must be evidence-based, citing that get_partition_info calls env.Exit(1) on missing partitions, which can preempt fallbacks if used directly.Learnt from: Jason2866 Repo: pioarduino/platform-espressif32 PR: 252 File: builder/frameworks/espidf.py:1382-1388 Timestamp: 2025-08-08T09:44:18.366Z Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.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.Learnt from: Jason2866 Repo: pioarduino/platform-espressif32 PR: 199 File: builder/main.py:221-226 Timestamp: 2025-06-26T09:55:55.788Z Learning: In the _update_max_upload_size function in builder/main.py, the `sizes` variable that maps partition subtypes to their sizes should not be removed despite appearing unused to static analysis tools. The variable serves a purpose that's not immediately obvious from the code structure.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.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
Comment out the 'arduino-zigbee-switch' example in the workflow.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
builder/main.py (2)
921-931: Unreachable code:check=Truealready raises on failure.This issue was already identified in a previous review. With
check=True, theif result.returncode != 0block at lines 923-925 is unreachable becausesubprocess.runwill raiseCalledProcessErroron failure.
1008-1018: Unreachable code:check=Truealready raises on failure.This issue was already identified in a previous review. With
check=True, theif result.returncode != 0block at lines 1010-1012 is unreachable becausesubprocess.runwill raiseCalledProcessErroron failure.
🧹 Nitpick comments (1)
builder/main.py (1)
966-966: Remove unused variables.Lines 966 (
fs_type_name) and 982 (page_size) are assigned but never used. Consider removing them to reduce clutter.🔎 Proposed fix
fs_start = int.from_bytes(entry[2:5], byteorder='little', signed=False) fs_size = int.from_bytes(entry[6:9], byteorder='little', signed=False) fs_subtype = part_type - fs_type_name = "LittleFS" if part_type == 0x83 else "SPIFFS" breakblock_size = 0x1000 # 4KB - page_size = 0x100 # 256 bytes print(f"Found filesystem partition (subtype {hex(fs_subtype)}):")Also applies to: 982-982
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
builder/main.py
🧰 Additional context used
🧠 Learnings (5)
📓 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: 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-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:
builder/main.py
📚 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:
builder/main.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/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
🪛 Ruff (0.14.10)
builder/main.py
880-880: Unused function argument: target
(ARG001)
880-880: Unused function argument: source
(ARG001)
922-922: subprocess call: check for execution of untrusted input
(S603)
924-924: f-string without any placeholders
Remove extraneous f prefix
(F541)
929-929: Do not catch blind exception: Exception
(BLE001)
966-966: Local variable fs_type_name is assigned to but never used
Remove assignment to unused variable fs_type_name
(F841)
978-978: f-string without any placeholders
Remove extraneous f prefix
(F541)
982-982: Local variable page_size is assigned to but never used
Remove assignment to unused variable page_size
(F841)
988-988: f-string without any placeholders
Remove extraneous f prefix
(F541)
993-993: f-string without any placeholders
Remove extraneous f prefix
(F541)
1009-1009: subprocess call: check for execution of untrusted input
(S603)
1016-1016: Do not catch blind exception: Exception
(BLE001)
1078-1078: Consider moving this statement to an else block
(TRY300)
1080-1080: 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). (15)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (macos-15, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (macos-15, examples/espidf-storage-sdcard)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (4)
builder/main.py (4)
532-537: LGTM! Clean integration of the new target.The addition of
download_littlefsto the LDF-disabled targets is consistent with the filesystem-related operations pattern.
880-900: LGTM! Function setup is clean and follows SCons conventions.The unused
targetandsourceparameters are part of the standard SCons target function signature.
1022-1087: LGTM! Comprehensive filesystem extraction implementation.The LittleFS mounting and extraction logic is well-implemented with:
- Proper cleanup of existing unpack directory
- Direct buffer assignment for mounting downloaded images
- Complete file and directory extraction with progress feedback
- Comprehensive error handling with helpful user guidance
1325-1331: LGTM! Platform target registration is correct.The target is properly registered with no build dependencies, which is appropriate for a download operation.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
builder/main.py (1)
1017-1083: Filesystem extraction logic looks solid.The LittleFS mounting and extraction implementation correctly:
- Cleans and prepares the unpack directory
- Mounts the downloaded image using littlefs-python
- Recursively extracts all files and directories
- Provides clear progress output and error messages
The
return 0at line 1075 could optionally be moved to anelseblock for better structure, but the current form is acceptable:Optional: Move success return to else block
fs.unmount() print(f"\nSuccessfully extracted {file_count} file(s) to {unpack_dir}") - return 0 - except Exception as e: print(f"Error: Failed to extract LittleFS filesystem: {e}") print("This tool only supports LittleFS. If you have SPIFFS, please convert to LittleFS.") print("Make sure the device has a valid LittleFS filesystem.") import traceback traceback.print_exc() return 1 + else: + return 0
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/examples.ymlbuilder/main.py
🧰 Additional context used
🧠 Learnings (6)
📓 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: 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-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:
builder/main.py
📚 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:
builder/main.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/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-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:
.github/workflows/examples.yml
🪛 Ruff (0.14.10)
builder/main.py
880-880: Unused function argument: target
(ARG001)
880-880: Unused function argument: source
(ARG001)
922-922: subprocess call: check for execution of untrusted input
(S603)
924-924: f-string without any placeholders
Remove extraneous f prefix
(F541)
929-929: Do not catch blind exception: Exception
(BLE001)
976-976: f-string without any placeholders
Remove extraneous f prefix
(F541)
985-985: f-string without any placeholders
Remove extraneous f prefix
(F541)
990-990: f-string without any placeholders
Remove extraneous f prefix
(F541)
1006-1006: subprocess call: check for execution of untrusted input
(S603)
1013-1013: Do not catch blind exception: Exception
(BLE001)
1075-1075: Consider moving this statement to an else block
(TRY300)
1077-1077: 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 (windows-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-riscv)
- GitHub Check: build (macos-15, examples/espidf-ulp)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (windows-latest, examples/espidf-http-request)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-SampleScan)
- GitHub Check: build (macos-15, examples/arduino-zigbee-light)
- GitHub Check: build (macos-15, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-peripherals-uart)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-usb-keyboard)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
🔇 Additional comments (3)
.github/workflows/examples.yml (1)
22-22: LGTM - Example disabled in CI matrix.The Zigbee switch example has been commented out, removing it from the test matrix while preserving the line for future reference.
builder/main.py (2)
530-543: LGTM - Correctly extends LDF disabling to the new target.The
download_littlefstarget has been appropriately added to the set of filesystem operations that don't require library dependency scanning, optimizing build performance for download operations.
1322-1328: LGTM - Platform target correctly registered.The
download_littlefstarget is properly registered with no build dependencies, making it available aspio run -t download_littlefsto download and extract the filesystem without requiring a build first.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
builder/main.py (1)
880-1075: Excellent work addressing the previous critical partition parsing issue!The partition table parsing at lines 943-959 now correctly uses 4-byte fields for offset (
entry[2:6]) and size (entry[6:10]), which matches the ESP32 partition table binary format. The unreachable exception handlers have also been removed.Optional enhancement: Add partition type check
For additional robustness, consider verifying that
entry[0] == 0x01(data partition type) before checking the subtype. While unlikely to cause issues in practice (only data partitions use subtypes 0x82/0x83), it would make the parsing more strictly correct:for entry in entries: if len(entry) < 32: continue + part_type = entry[0] part_subtype = entry[1] # Check for SPIFFS (0x82) or LITTLEFS (0x83) - if part_subtype in [0x82, 0x83]: + if part_type == 0x01 and part_subtype in [0x82, 0x83]: fs_start = int.from_bytes(entry[2:6], byteorder='little', signed=False) fs_size = int.from_bytes(entry[6:10], byteorder='little', signed=False) fs_subtype = part_subtype break
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/examples.ymlbuilder/main.py
🧰 Additional context used
🧠 Learnings (8)
📓 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.
📚 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:
.github/workflows/examples.yml
📚 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:
builder/main.py
📚 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:
builder/main.py
📚 Learning: 2025-08-08T10:04:46.044Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T10:04:46.044Z
Learning: When discussing espidf.py get_app_partition_offset behavior with Jason2866, they prefer the OTA-first then factory order and consider using the "boot" sentinel undesirable. Clarifications must be evidence-based, citing that get_partition_info calls env.Exit(1) on missing partitions, which can preempt fallbacks if used directly.
Applied to files:
builder/main.py
📚 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:
builder/main.py
📚 Learning: 2025-08-08T09:44:18.366Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 252
File: builder/frameworks/espidf.py:1382-1388
Timestamp: 2025-08-08T09:44:18.366Z
Learning: In builder/frameworks/espidf.py, get_partition_info treats pt_params["name"] == "boot" as a sentinel to pass --partition-boot-default to parttool.py; "boot" is not expected to be a CSV partition name.
Applied to files:
builder/main.py
📚 Learning: 2025-06-26T09:55:55.788Z
Learnt from: Jason2866
Repo: pioarduino/platform-espressif32 PR: 199
File: builder/main.py:221-226
Timestamp: 2025-06-26T09:55:55.788Z
Learning: In the _update_max_upload_size function in builder/main.py, the `sizes` variable that maps partition subtypes to their sizes should not be removed despite appearing unused to static analysis tools. The variable serves a purpose that's not immediately obvious from the code structure.
Applied to files:
builder/main.py
🪛 Ruff (0.14.10)
builder/main.py
880-880: Unused function argument: target
(ARG001)
880-880: Unused function argument: source
(ARG001)
922-922: subprocess call: check for execution of untrusted input
(S603)
926-926: Do not catch blind exception: Exception
(BLE001)
970-970: f-string without any placeholders
Remove extraneous f prefix
(F541)
979-979: f-string without any placeholders
Remove extraneous f prefix
(F541)
984-984: f-string without any placeholders
Remove extraneous f prefix
(F541)
1000-1000: subprocess call: check for execution of untrusted input
(S603)
1004-1004: Do not catch blind exception: Exception
(BLE001)
1066-1066: Consider moving this statement to an else block
(TRY300)
1068-1068: 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). (15)
- GitHub Check: build (macos-15, examples/espidf-peripherals-uart)
- GitHub Check: build (macos-15, examples/espidf-arduino-littlefs)
- GitHub Check: build (macos-15, examples/espidf-blink)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-rmt-blink)
- GitHub Check: build (windows-latest, examples/arduino-NimBLE-SampleScan)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (ubuntu-latest, examples/espidf-ulp-lp)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
🔇 Additional comments (3)
.github/workflows/examples.yml (1)
22-22: LGTM - Example temporarily disabled.The Zigbee switch example has been commented out from the CI matrix, which is a straightforward way to disable testing without removing the entry.
builder/main.py (2)
532-537: LGTM - LDF correctly disabled for new target.The addition of
download_littlefsto the filesystem targets that disable Library Dependency Finder is appropriate, as this operation doesn't require library compilation.
1313-1319: LGTM - Platform target correctly registered.The
download_littlefstarget is properly registered withNoneas the target (no build dependency), which is appropriate for a download-only operation. The registration follows the established pattern for other platform targets.
1 participant
Description:
Updated the switch_off_ldf function to include
download_littlefsin the targets. Added a new functiondownload_littlefsto download and extract the LittleFS filesystem from the device.Checklist:
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.