use DRAM as fallback if PSRAM fails#5631
Conversation
WalkthroughMemory allocation preference flags are clarified in documentation. The ChangesPSRAM Memory Allocation Fallback
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wled00/fcn_declare.h (1)
530-530: 💤 Low valueConsider renaming flag to clarify fallback semantics.
The updated comment clarifies that
BFRALLOC_ENFORCE_PSRAMnow "falls back to DRAM if PSRAM fails," which is accurate. However, the term "ENFORCE" typically implies strict/mandatory behavior, while the new implementation allows fallback. This could cause confusion for developers expecting PSRAM-only allocation.Consider renaming to
BFRALLOC_REQUIRE_PSRAMorBFRALLOC_PSRAM_FIRSTto better convey the "attempt PSRAM, accept DRAM fallback" semantics, distinguishing it fromBFRALLOC_PREFER_PSRAM(which applies heuristics and may choose DRAM even when PSRAM is available).That said, if the naming is established and the updated documentation is deemed sufficient, this is acceptable as-is.
🤖 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 `@wled00/fcn_declare.h` at line 530, The macro name BFRALLOC_ENFORCE_PSRAM suggests mandatory PSRAM but the implementation allows falling back to DRAM; rename the macro to something clearer (e.g., BFRALLOC_PSRAM_FIRST or BFRALLOC_REQUIRE_PSRAM) and update its comment and all references/usages (search for BFRALLOC_ENFORCE_PSRAM in the codebase) so the symbol, its documentation, and any conditional logic reflect the "try PSRAM, accept DRAM fallback" semantics consistently.
🤖 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 `@wled00/fcn_declare.h`:
- Line 530: The macro name BFRALLOC_ENFORCE_PSRAM suggests mandatory PSRAM but
the implementation allows falling back to DRAM; rename the macro to something
clearer (e.g., BFRALLOC_PSRAM_FIRST or BFRALLOC_REQUIRE_PSRAM) and update its
comment and all references/usages (search for BFRALLOC_ENFORCE_PSRAM in the
codebase) so the symbol, its documentation, and any conditional logic reflect
the "try PSRAM, accept DRAM fallback" semantics consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 806b0897-1a1b-41ce-b29b-2b795b3317a3
📒 Files selected for processing (2)
wled00/fcn_declare.hwled00/util.cpp
softhack007
left a comment
There was a problem hiding this comment.
Looks good for me 👍
please also cherry-pick into 16_x after merge.
|
merged an cherry picked to 16_x |
fix for #5629
Summary by CodeRabbit