Make remove of component dsp possible by adding to lib_ignore
#284
Conversation
Updated DSP component protection logic to allow removal if 'dsp' is explicitly ignored.
Add method to retrieve original lib_ignore entries.
Refactor _remove_ignored_lib_includes method for clarity and consistency. Improve logging and maintain structure in the component manager.
Comment out DSP component protection logic.
|
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 You can disable this status message by setting the WalkthroughRefactors ComponentManager internals: lazy-loaded Arduino paths, batched library-ignore removals, BT/DSP mapping and protection optimizations, and structural validation changes. Adds Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Build as Build System
participant CM as ComponentManager
participant Map as Mapper
participant Batch as BatchRemover
participant Validator as Validator
User->>Build: start build
Build->>CM: resolve libraries & includes
CM->>Map: map library name -> include path (fast-paths for BT/DSP)
Map-->>CM: include path(s)
CM->>Batch: compile removal patterns (_get_compiled_patterns)
Batch-->>CM: filtered CPPPATH / includes (batch removals)
CM->>Validator: _validate_changes(structure markers)
Validator-->>CM: validation result
CM-->>Build: final includes/CPPPATH
Build-->>User: build proceeds
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
✨ Finishing Touches🧪 Generate unit tests
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 (6)
builder/frameworks/component_manager.py (6)
773-775: BLE aliases mapping looks good.Adding simpleble and esp-nimble-cpp → bt will prevent accidental removal when bt is needed. Consider optionally adding common variants to be more forgiving.
Apply if desired:
'esp32_ble_arduino': 'bt', 'simpleble': 'bt', 'esp-nimble-cpp': 'bt', + 'esp_nimble_cpp': 'bt', + 'nimble_arduino': 'bt',
829-834: DSP aliases → espressif__esp-dsp: good coverage.Minor: include the hyphenated key for directory-name fallbacks.
'tinyusb': 'arduino_tinyusb', 'dsp': 'espressif__esp-dsp', 'esp_dsp': 'espressif__esp-dsp', 'dsps': 'espressif__esp-dsp', 'fft2r': 'espressif__esp-dsp', - 'dsps_fft2r': 'espressif__esp-dsp' + 'dsps_fft2r': 'espressif__esp-dsp', + 'esp-dsp': 'espressif__esp-dsp'
894-902: Direct mapping additions are appropriate.Same nit: add the hyphenated esp-dsp to mirror user input.
'esp-nimble-cpp': 'bt', 'dsp': 'espressif__esp-dsp', 'esp_dsp': 'espressif__esp-dsp', 'dsps': 'espressif__esp-dsp', 'fft2r': 'espressif__esp-dsp', - 'dsps_fft2r': 'espressif__esp-dsp' + 'dsps_fft2r': 'espressif__esp-dsp', + 'esp-dsp': 'espressif__esp-dsp'
912-917: Docstring mismatches behavior (DSP no longer protected).Update to avoid confusion.
- all ignored libraries. Implements protection for BT/BLE and DSP - components when dependencies are detected. Uses multiple regex + all ignored libraries. Implements protection for BT/BLE components + when dependencies are detected. Uses multiple regex patterns to catch different include path formats.
943-947: Remove commented-out DSP protection block.Dead code invites misunderstanding; the docstring change above clarifies intent.
-# # Hard protection for DSP components -# if lib_name.lower() in ['dsp', 'esp_dsp', 'dsps', 'fft2r', 'dsps_fft2r']: -# self.logger.log_change(f"Protected DSP component: {lib_name}") -# continue
1019-1029: Backup guard: align with repo preference and other class behavior.Prior project learning indicates a preference to not early-return on missing arduino_libs_mcu during backups; BackupManager already relies on existence checks. For consistency, consider dropping this guard here too.
if "arduino" not in self.config.env.subst("$PIOFRAMEWORK"): return - - if not self.config.arduino_libs_mcu: - return build_py_path = str(Path(self.config.arduino_libs_mcu) / "pioarduino-build.py") backup_path = str(Path(self.config.arduino_libs_mcu) / f"pioarduino-build.py.{self.config.mcu}") if os.path.exists(build_py_path) and not os.path.exists(backup_path): shutil.copy2(build_py_path, backup_path)Note: This suggestion references your stated preference in earlier PRs; confirm you still want this behavior in this context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
builder/frameworks/component_manager.py(6 hunks)examples/arduino-rmt-blink/platformio.ini(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/espidf.py:1121-1139
Timestamp: 2025-09-05T12:20:25.789Z
Learning: In the pioarduino/platform-espressif32 project, the LibraryIgnoreHandler._get_lib_ignore_entries() private method already includes BT/BLE protection logic internally, so there's no need to add additional BT/BLE dependency checking when using this method in get_lib_ignore_components().
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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.
📚 Learning: 2025-09-05T12:20:25.789Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/espidf.py:1121-1139
Timestamp: 2025-09-05T12:20:25.789Z
Learning: In the pioarduino/platform-espressif32 project, the LibraryIgnoreHandler._get_lib_ignore_entries() private method already includes BT/BLE protection logic internally, so there's no need to add additional BT/BLE dependency checking when using this method in get_lib_ignore_components().
Applied to files:
builder/frameworks/component_manager.py
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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-rmt-blink/platformio.ini
⏰ 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). (12)
- GitHub Check: build (macos-15, examples/espidf-ulp-lp)
- GitHub Check: build (macos-15, examples/espidf-arduino-C6-ULP-blink)
- GitHub Check: build (macos-15, examples/espidf-arduino-blink)
- GitHub Check: build (macos-15, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-peripherals-usb)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/tasmota)
- GitHub Check: build (windows-latest, examples/espidf-hello-world)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
🔇 Additional comments (1)
examples/arduino-rmt-blink/platformio.ini (1)
14-14: LGTM: dsp added to lib_ignore across ESP32 envs.Matches the PR goal and keeps example envs as intended test cases (per prior project learnings). No issues spotted.
Also applies to: 31-31, 48-48, 65-65, 82-82
|
@coderabbitai make suggestions for speed optimize |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll analyze the code changes to provide specific speed optimization suggestions. Let me examine the implementation details first. 🏁 Script executed: Unrecoverable error: Failed to clone repository Let me examine the files directly to understand the current implementation and provide speed optimization suggestions. 🏁 Script executed: Unrecoverable error: Failed to clone repository @Jason2866 Based on the PR changes described in the AI summary, I can provide several speed optimization suggestions for the component manager functionality: Performance Optimization Suggestions1. Optimize Library Mapping LookupsThe PR adds multiple new DSP and Bluetooth mappings. Consider:
2. Cache Library Include Path ResolutionsSince
3. Batch Processing for lib_ignoreWith DSP components now processed through lib_ignore:
4. Optimize Direct Mapping OperationsFor the expanded
5. Reduce String Operations
6. Performance MonitoringSince per-library logging was removed:
Would you like me to focus on any specific area or provide code examples for implementing these optimizations? 🧠 Learnings used |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
builder/frameworks/component_manager.py (1)
298-306: Update default ESP-IDF version to ≥5.5.0
Change the defaultidfdependency inbuilder/frameworks/component_manager.pyfrom">=5.1"to">=5.5.0", matching Arduino-ESP32 v3.3.0’s ESP-IDF v5.5.0 baseline.
🧹 Nitpick comments (10)
builder/frameworks/component_manager.py (10)
181-186: Avoid duplicating backup logic; delegate to BackupManager.This block calls a local backup helper; similar backup code exists in multiple classes. Prefer a single implementation (BackupManager) to keep behavior consistent.
389-391: Typo in comment.“entrys” → “entries”.
- if not component: # Skip empty entrys + if not component: # Skip empty entries
443-454: Backup method here diverges from centralized approach and past preference—please confirm.
- This re-implements backup logic already present elsewhere.
- It also early-returns if
arduino_libs_mcuis empty; previous maintainer preference was to avoid extra guards around this (PR #281 learning). Confirm desired behavior.
507-516: Regex join for batch CPPPATH removal: solid idea; consider a small hardening.Escape set is correct. Optionally anchor to line starts when feasible to reduce over-match risk.
527-529: Don’t swallow exceptions; log them.At least log the error; aligns with static-analysis hint.
- except Exception: - pass + except Exception as e: + print(f"[ComponentManager] Error updating build file during CPPPATH cleanup: {e!s}")
617-637: lib_ignore normalization looks fine; consider de-dup and stable ordering (optional).Since you later store into a set, returning a de-duped, sorted list can make logs deterministic.
- return cleaned_entries + return sorted(set(cleaned_entries))
851-857: Add more DSP aliases to mapping.Handle users specifying registry or fs-style names directly.
'dsp': 'espressif__esp-dsp', 'esp_dsp': 'espressif__esp-dsp', 'dsps': 'espressif__esp-dsp', 'fft2r': 'espressif__esp-dsp', 'dsps_fft2r': 'espressif__esp-dsp', - 'esp-dsp': 'espressif__esp-dsp' + 'esp-dsp': 'espressif__esp-dsp', + 'espressif/esp-dsp': 'espressif__esp-dsp', + 'espressif__esp-dsp': 'espressif__esp-dsp'
885-899: Fast-path DSP/BT conversion: include registry/fs variants.Covers the common user inputs without falling back to generic cleanup.
- dsp_patterns = { - 'dsp', 'esp_dsp', 'dsps', 'fft2r', 'dsps_fft2r', 'esp-dsp' - } + dsp_patterns = { + 'dsp', 'esp_dsp', 'dsps', 'fft2r', 'dsps_fft2r', 'esp-dsp', + 'espressif/esp-dsp', 'espressif__esp-dsp' + }
1036-1038: Use explicit conversion flag and include exception type.Improves clarity and satisfies linter.
- except Exception as e: - self.logger.log_change(f"Error processing libraries: {str(e)}") + except Exception as e: + self.logger.log_change(f"Error processing libraries: {e!s} ({e.__class__.__name__})")
1086-1099: Validation guard is minimal; optional extra check.Consider a quick parse check (e.g.,
ast.parse) when Python is available to reduce risk of breaking the build script.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
builder/frameworks/component_manager.py(22 hunks)examples/arduino-blink/platformio.ini(7 hunks)examples/arduino-rmt-blink/platformio.ini(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/espidf.py:1121-1139
Timestamp: 2025-09-05T12:20:25.789Z
Learning: In the pioarduino/platform-espressif32 project, the LibraryIgnoreHandler._get_lib_ignore_entries() private method already includes BT/BLE protection logic internally, so there's no need to add additional BT/BLE dependency checking when using this method in get_lib_ignore_components().
📚 Learning: 2025-06-26T09:56:30.658Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#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-rmt-blink/platformio.iniexamples/arduino-blink/platformio.ini
📚 Learning: 2025-09-05T12:20:25.789Z
Learnt from: Jason2866
PR: pioarduino/platform-espressif32#281
File: builder/frameworks/espidf.py:1121-1139
Timestamp: 2025-09-05T12:20:25.789Z
Learning: In the pioarduino/platform-espressif32 project, the LibraryIgnoreHandler._get_lib_ignore_entries() private method already includes BT/BLE protection logic internally, so there's no need to add additional BT/BLE dependency checking when using this method in get_lib_ignore_components().
Applied to files:
examples/arduino-blink/platformio.inibuilder/frameworks/component_manager.py
🪛 Ruff (0.12.2)
builder/frameworks/component_manager.py
527-528: try-except-pass detected, consider logging the exception
(S110)
527-527: Do not catch blind exception: Exception
(BLE001)
636-636: Consider moving this statement to an else block
(TRY300)
638-638: Do not catch blind exception: Exception
(BLE001)
665-665: Do not catch blind exception: Exception
(BLE001)
1036-1036: Do not catch blind exception: Exception
(BLE001)
1037-1037: Use explicit conversion flag
Replace with conversion flag
(RUF010)
⏰ 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/espidf-peripherals-uart)
- GitHub Check: build (macos-15, examples/espidf-http-request)
- GitHub Check: build (macos-15, examples/espidf-coap-server)
- GitHub Check: build (windows-latest, examples/arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-zigbee-switch)
- GitHub Check: build (windows-latest, examples/espidf-storage-sdcard)
- GitHub Check: build (macos-15, examples/arduino-matter-light)
- GitHub Check: build (windows-latest, examples/espidf-exceptions)
- GitHub Check: build (macos-15, examples/espidf-arduino-blink)
- GitHub Check: build (windows-latest, examples/espidf-arduino-littlefs)
- GitHub Check: build (windows-latest, examples/arduino-zigbee-light)
- GitHub Check: build (ubuntu-latest, examples/espidf-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/espidf-arduino-matter-light)
- GitHub Check: build (ubuntu-latest, examples/arduino-wifiscan)
- GitHub Check: build (ubuntu-latest, examples/arduino-blink)
- GitHub Check: build (ubuntu-latest, examples/tasmota)
🔇 Additional comments (5)
examples/arduino-blink/platformio.ini (1)
17-17: Approve DSP lib_ignore additions across all envs. No DSP headers detected in the examples after addingdsptolib_ignore.examples/arduino-rmt-blink/platformio.ini (1)
14-14: Approve dsp added to lib_ignore in RMT blink examples
Scan of examples/arduino-rmt-blink for ESP-DSP symbols (esp_dsp, dsps_, fft2r, dsps_fft2r, esp-dsp) returned no hits—ignoring dsp is safe.builder/frameworks/component_manager.py (3)
55-79: Lazy-loaded Arduino paths: good improvement.Removes repeated lookups; simple and safe.
558-584: Precomputed protection sets: LGTM.Fast O(1) lookups for critical/BT keywords are appropriate.
1001-1005: BT/BLE protection toggle: OK; ensure no double-protection elsewhere.Given prior note that BT/BLE protection existed upstream, confirm this isn’t duplicating logic in other call paths.
Description:
Related issue (if applicable): fixes #
Checklist:
Summary by CodeRabbit
New Features
Bug Fixes / Performance
Documentation