Refactor: Hybrid Compile sdkconfig handling#477
Conversation
Refactor PSRAM detection logic and configure PSRAM frequency only if supported.
|
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:
📝 WalkthroughWalkthroughThe change refactors ESP-IDF sdkconfig generation to detect board-specific configurations more comprehensively, moves PSRAM detection earlier in the process with improved heuristics, makes PSRAM frequency flag generation conditional on PSRAM presence, and adds ESP32-specific bootloader WP pin handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 257-258: The PSRAM detection wrongly assumes extra_flags is a
list; coerce board.get("build.extra_flags", []) to a list when it's a string
before iterating so has_psram is computed correctly. Update the code around
extra_flags / has_psram in builder/frameworks/espidf.py to mirror the handling
used in generate_board_specific_config(): if extra_flags is a str, convert it to
a single-element list (or split it the same way generate_board_specific_config
does) and then compute has_psram = any("-DBOARD_HAS_PSRAM" in flag for flag in
extra_flags).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d227663a-8b1d-460e-9ab4-0e77637e6449
📒 Files selected for processing (1)
builder/frameworks/espidf.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
builder/frameworks/espidf.py (1)
453-459: Minor inconsistency: Default value differs fromhas_board_specific_config().At line 454, the default is
""(empty string), while at line 257 inhas_board_specific_config(), the default is[](empty list). Although both work correctly due to theisinstance()checks, using a consistent default would improve maintainability.♻️ Optional: Use consistent default value
# Check for PSRAM support based on board flags (needed before frequency config) - extra_flags = board.get("build.extra_flags", "") + extra_flags = board.get("build.extra_flags", [])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 453 - 459, The code checks board.get("build.extra_flags", "") and treats it as str or list; to be consistent with has_board_specific_config(), change the default from "" to [] so both use an empty list default; update the call in the block that sets extra_flags and the subsequent isinstance checks around has_psram to work with the new default (keep the current logic that handles str vs list) and reference the same "build.extra_flags" key used in has_board_specific_config().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 251-252: The current check uses flash_mode =
board.get("build.flash_mode", "dio") and then if flash_mode: return True which
is always true; instead detect whether flash_mode was explicitly set in the
board manifest by testing the presence of the key (e.g., use board.get("build")
and check "flash_mode" in that dict or call board.has_key / board.get_raw
depending on API) and only return True when the manifest contains an explicit
"build.flash_mode"; update the conditional in the function that uses flash_mode
(the block with "if flash_mode: return True" in builder/frameworks/espidf.py) so
the subsequent PSRAM and memory-type logic (lines handling psram, flash_type
checks) can run when flash_mode is not explicitly provided, or alternatively
remove the early return and adjust the docstring to reflect that all boards
trigger sdkconfig generation—use the symbols flash_mode and
board.get/build.flash_mode to locate the code to change.
---
Nitpick comments:
In `@builder/frameworks/espidf.py`:
- Around line 453-459: The code checks board.get("build.extra_flags", "") and
treats it as str or list; to be consistent with has_board_specific_config(),
change the default from "" to [] so both use an empty list default; update the
call in the block that sets extra_flags and the subsequent isinstance checks
around has_psram to work with the new default (keep the current logic that
handles str vs list) and reference the same "build.extra_flags" key used in
has_board_specific_config().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 31a35503-c089-4e79-85c9-64ade5776bc6
📒 Files selected for processing (1)
builder/frameworks/espidf.py
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
builder/frameworks/espidf.py (2)
758-777: Deduplication is still partial for flags not present in template lines.Current logic deduplicates only when a source sdkconfig line is matched. Duplicate custom flags that don’t exist in the template can still be appended multiple times at Line 783 onward. Pre-dedup
idf_config_flagsby flag name (keep last) before the template loop.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 758 - 777, The code currently only removes duplicates when a template line matches; before iterating the template you should pre-deduplicate idf_config_flags by flag name so only the last occurrence is kept: build a map keyed by extract_flag_name(idf_config_flags[i].replace("'", "")) and retain the last index/value, then reconstruct idf_config_flags from that map (preserving the chosen last entries) so later logic that pops and appends (references: idf_config_flags, extract_flag_name, last_match_idx, flag_replaced, dst.write) cannot append duplicate custom flags that never matched a template line.
657-661: Avoid duplicating WP defaults already provided by board sdkconfig.
boards/esp32-solo1.jsonalready defines these two flags; unconditional append here can produce duplicate entries. Consider skipping auto-add when boardespidf.custom_sdkconfigalready sets either WP flag.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@builder/frameworks/espidf.py` around lines 657 - 661, The code unconditionally appends WP defaults when mcu == "esp32" which can duplicate flags already defined by the board's espidf.custom_sdkconfig; modify the mcu == "esp32" block to first inspect the board's espidf.custom_sdkconfig (and/or existing board_config_flags) for the two entries ("# CONFIG_BOOTLOADER_SPI_CUSTOM_WP_PIN is not set" and "CONFIG_BOOTLOADER_SPI_WP_PIN=7") and only extend board_config_flags with each entry if it is not already present in custom_sdkconfig or board_config_flags; reference the mcu check and the board_config_flags list in your change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@builder/frameworks/espidf.py`:
- Around line 79-86: The code calls mcu.lower() without ensuring mcu is set,
which can raise AttributeError; update the block around mcu and idf_variant so
you validate mcu (from board.get("build.mcu", None)) before calling .lower(): if
mcu is missing or falsy, raise a clear ValueError (or RuntimeError) with a
concise message about the missing build.mcu in the board manifest instead of
letting .lower() fail, and only assign idf_variant = mcu.lower() when mcu is a
non-empty string.
- Around line 453-474: The PSRAM detection is overbroad because it checks "opi"
anywhere in the raw memory_type string; update the logic in the block that
computes has_psram (using variables memory_type and extra_flags) to first ensure
memory_type is a string, split it on "_" and inspect the psram_memory_type part
(e.g., second segment) rather than the whole string, and only set has_psram=True
when that parsed psram_memory_type equals or contains "opi" or "psram"; keep the
other checks (build.psram_type and PSRAM in extra_flags) intact.
---
Nitpick comments:
In `@builder/frameworks/espidf.py`:
- Around line 758-777: The code currently only removes duplicates when a
template line matches; before iterating the template you should pre-deduplicate
idf_config_flags by flag name so only the last occurrence is kept: build a map
keyed by extract_flag_name(idf_config_flags[i].replace("'", "")) and retain the
last index/value, then reconstruct idf_config_flags from that map (preserving
the chosen last entries) so later logic that pops and appends (references:
idf_config_flags, extract_flag_name, last_match_idx, flag_replaced, dst.write)
cannot append duplicate custom flags that never matched a template line.
- Around line 657-661: The code unconditionally appends WP defaults when mcu ==
"esp32" which can duplicate flags already defined by the board's
espidf.custom_sdkconfig; modify the mcu == "esp32" block to first inspect the
board's espidf.custom_sdkconfig (and/or existing board_config_flags) for the two
entries ("# CONFIG_BOOTLOADER_SPI_CUSTOM_WP_PIN is not set" and
"CONFIG_BOOTLOADER_SPI_WP_PIN=7") and only extend board_config_flags with each
entry if it is not already present in custom_sdkconfig or board_config_flags;
reference the mcu check and the board_config_flags list in your change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5578e1ec-9202-4af4-8a46-f913896ee96a
📒 Files selected for processing (1)
builder/frameworks/espidf.py
Added error handling for missing 'build.mcu' field in board manifest.
Add linker script for ESP32 when PSRAM is not configured.
|
@coderabbitai full review |
1 participant
Checklist:
Summary by CodeRabbit
Bug Fixes
Refactor
Bug Fix / Chore