HybridCompile: Refactor sdkconfig generation from boards json and add P4 #316
Conversation
|
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 WalkthroughAdds PSRAM frequency configuration ( Changes
Sequence DiagramsequenceDiagram
participant User as Build System
participant espidf as ESPiDF Builder
participant Board as Board Config
participant SDK as SDK Flags
User->>espidf: generate_board_specific_config()
espidf->>Board: Extract MCU, f_cpu, f_psram, f_flash
Board-->>espidf: Board settings
alt ESP32-P4
espidf->>espidf: Parse f_psram (or fallback to f_flash)
espidf->>SDK: Emit ESP_DEFAULT_CPU_FREQ_MHZ
espidf->>SDK: Emit CONFIG_ESP32P4_DEFAULT_CPU_FREQ
Note over espidf: Separate Flash & PSRAM modes<br/>(no f_boot override)
else Other MCUs
espidf->>espidf: Parse f_boot for Flash/PSRAM
espidf->>SDK: Emit MCU-specific DEFAULT flags
Note over espidf: Unified frequency handling<br/>with f_boot override
end
alt PSRAM Detected
espidf->>espidf: Validate frequency thresholds
espidf->>SDK: Set PSRAM mode (HEX/OPI or QIO/QSPI)
espidf->>SDK: Apply experimental features if needed
else No PSRAM
espidf->>SDK: Explicitly disable SPIRAM
end
espidf-->>User: Configured SDK flags
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes The three board JSON changes are straightforward repetitive additions, but the espidf.py modifications introduce significant logic density with MCU-aware branching, frequency validation, PSRAM mode selection (HEX/OPI vs QIO/QSPI), specialized ESP32-P4 handling, and extended ULP gating. Multiple independent reasoning paths and configuration chains require careful review. Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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: 2
🧹 Nitpick comments (2)
builder/frameworks/espidf.py (2)
364-381: Harden CPU‑freq flag emission; avoid non‑existent Kconfig symbolsYou emit both CONFIG_ESP_DEFAULT_CPU_FREQ_MHZ_{x} and CONFIG_{MCU}DEFAULT_CPU_FREQ{x}. For values like 360, some MCUs/IDF versions may not define these symbols, causing noisy/ignored entries. Gate by a per‑MCU supported list (include P4: 300/360/400, etc.) and only emit the MCU‑specific toggles that exist.
Would you confirm the allowed CPU freq symbols for each MCU/IDF you support? If helpful, I can provide a patch that maps per‑MCU lists and falls back to generic flags when unknown.
462-466: Tiny cleanup: f-string without placeholdersThis f-string has no interpolations.
Apply this diff:
- print(f"Warning: Invalid frequency values, skipping frequency configuration") + print("Warning: Invalid frequency values, skipping frequency configuration")Based on static analysis hints.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
boards/esp32-p4-evboard.json(1 hunks)boards/esp32-p4.json(1 hunks)boards/m5stack-tab5-p4.json(1 hunks)builder/frameworks/espidf.py(6 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
builder/frameworks/espidf.py
437-437: Do not use bare except
(E722)
437-438: try-except-pass detected, consider logging the exception
(S110)
445-445: Local variable esptool_flash_freq is assigned to but never used
Remove assignment to unused variable esptool_flash_freq
(F841)
465-465: f-string without any placeholders
Remove extraneous f prefix
(F541)
⏰ 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). (5)
- GitHub Check: build (macos-15, examples/espidf-hello-world)
- GitHub Check: build (macos-15, examples/espidf-arduino-matter-light)
- GitHub Check: build (macos-15, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-blink)
- GitHub Check: build (windows-latest, examples/arduino-wifiscan)
🔇 Additional comments (6)
boards/m5stack-tab5-p4.json (1)
11-11: LGTM: PSRAM frequency declared200 MHz PSRAM matches P4 expectations and will be picked up by the builder.
builder/frameworks/espidf.py (4)
498-505: Verify PSRAM speed options include 200M for P4Emits CONFIG_SPIRAM_SPEED=200 and CONFIG_SPIRAM_SPEED_200M=y when f_psram=200MHz. Confirm the IDF Kconfig exposes 200M for P4 on the versions you target.
If needed, I can add a guard to clamp to available options per MCU/IDF.
592-597: Good: explicitly disable SPIRAM when not presentPrevents accidental partial enablement and keeps sdkconfig clean.
602-612: OPI flash defaults look correctFor OPI flash, forcing DOUT boot and enabling OCT + DTR sampling matches IDF guidance.
Confirm boards that truly use OPI set either build.boot=opi or flash_mode=opi; otherwise this block won’t engage.
2565-2568: ULP gating updated to include P4 — LGTMMatches current SoC capabilities; excludes C2/C3/C5/H2.
boards/esp32-p4-evboard.json (1)
9-9: LGTM: explicit PSRAM frequency for P4 verifiedValue 200000000L is correct for P4 HEX PSRAM. ESP-IDF v5.x/v6.x confirms CONFIG_SPIRAM_SPEED_200M support for this target, and the builder framework correctly consumes the build.f_psram setting.
Removed unnecessary comments and fixed print statement formatting.
Checklist:
Summary by CodeRabbit
New Features
Refactor