add speed to mem variant folder for S3 #124
Conversation
WalkthroughThe changes introduce extraction and use of memory variant frequency (MEM_VARIANT_FREQ) during build phases for esp32s3 targets. Build configuration is updated with new memory/frequency permutations, and copy scripts are modified to append the extracted frequency to memory configuration paths. Changes
Sequence DiagramsequenceDiagram
participant Build as build.sh
participant Config as configs/builds.json
participant CopyLib as tools/copy-libs.sh
participant CopyMem as tools/copy-mem-variant.sh
Build->>Config: Read esp32s3 idf_libs & mem_variants
Build->>Build: Extract MEM_VARIANT_FREQ from [1] element
Build->>Build: Export MEM_VARIANT_FREQ + log frequency
rect rgba(100, 150, 255, 0.1)
Note over CopyLib,CopyMem: Memory Configuration Processing
Build->>CopyLib: Pass MEM_VARIANT_FREQ
Build->>CopyMem: Pass MEM_VARIANT_FREQ
CopyLib->>CopyLib: Append MEM_VARIANT_FREQ to MEMCONF (if esp32s3)
CopyLib->>CopyLib: Add freq suffix to memory paths
CopyMem->>CopyMem: Append MEM_VARIANT_FREQ to MEMCONF (if esp32s3)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Poem
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: 0
🧹 Nitpick comments (2)
tools/copy-libs.sh (1)
471-471: Complex PlatformIO config expressions—readability concern.Lines 471 and 496 add very long Python ternary expressions to generate memory-type suffixes for include and lib paths. The logic is correct (conditionally append frequency for esp32s3), but the single-line nested ternary is hard to parse visually and maintain.
Consider breaking these into intermediate variables for clarity:
memory_type = board_config.get("build.arduino.memory_type", board_config.get("build.flash_mode", "dio") + "_qspi") if board_config.get("build.mcu") == "esp32s3": freq_suffix = "_" + board_config.get("build.f_boot", board_config.get("build.f_flash", "80000000L") ).replace("000000L", "m") memory_path = memory_type + freq_suffix else: memory_path = memory_typeThis would make the intent clearer and easier to debug if board config keys are missing or unexpected.
Also applies to: 496-496
build.sh (1)
217-217: Address shellcheck SC2155: Declare and assign separately to avoid masking return values.Using
export VAR=$(command)can hide failures in the command. Ifjqencounters an error or returns unexpected output (e.g., "null" for a missing array element), the export still succeeds, leaving MEM_VARIANT_FREQ in an inconsistent state.While the downstream conditional checks (
[ -n "$MEM_VARIANT_FREQ" ]) guard against empty strings, they don't guard against the literal string "null" if jq fails silently.Split the declaration and assignment to surface errors more clearly:
# Line 217 (IDF Libs) MEM_VARIANT_FREQ=$(echo "$target_json" | jq -r '.idf_libs[1]') export MEM_VARIANT_FREQ # Line 247 (Mem Variants) MEM_VARIANT_FREQ=$(echo "$mem_conf" | jq -r '.[1]') export MEM_VARIANT_FREQThis way, if
jqfails or returns null, you can add explicit error handling or validation.Also applies to: 247-247
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
build.sh(2 hunks)configs/builds.json(1 hunks)tools/copy-libs.sh(3 hunks)tools/copy-mem-variant.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Jason2866
Repo: Jason2866/esp32-arduino-lib-builder PR: 121
File: tools/compile_slave.sh:48-50
Timestamp: 2025-07-31T10:57:07.367Z
Learning: In the esp32-arduino-lib-builder project's tools/compile_slave.sh script, the coprocessor firmware version header file uses non-standard IDF version style definitions with a "_1" suffix (PROJECT_VERSION_MAJOR_1, PROJECT_VERSION_MINOR_1, PROJECT_VERSION_PATCH_1) instead of the usual IDF format without the suffix. This is intentional and correct according to maintainer Jason2866.
📚 Learning: 2025-07-31T10:57:07.367Z
Learnt from: Jason2866
Repo: Jason2866/esp32-arduino-lib-builder PR: 121
File: tools/compile_slave.sh:48-50
Timestamp: 2025-07-31T10:57:07.367Z
Learning: In the esp32-arduino-lib-builder project's tools/compile_slave.sh script, the coprocessor firmware version header file uses non-standard IDF version style definitions with a "_1" suffix (PROJECT_VERSION_MAJOR_1, PROJECT_VERSION_MINOR_1, PROJECT_VERSION_PATCH_1) instead of the usual IDF format without the suffix. This is intentional and correct according to maintainer Jason2866.
Applied to files:
tools/copy-libs.shbuild.shtools/copy-mem-variant.shconfigs/builds.json
📚 Learning: 2025-07-30T15:26:30.575Z
Learnt from: Jason2866
Repo: Jason2866/esp32-arduino-lib-builder PR: 121
File: .github/workflows/parallel_build.yaml:85-108
Timestamp: 2025-07-30T15:26:30.575Z
Learning: In the esp32-arduino-lib-builder project's parallel_build.yaml workflow, the coprocessor_fw_version.txt file is correctly accessed at the path "framework-arduinoespressif32/tools/slave_firmware/coprocessor_fw_version.txt" despite the mv command moving slave firmware files, according to maintainer Jason2866. The current implementation works as intended.
Applied to files:
tools/copy-libs.sh
🪛 Shellcheck (0.11.0)
build.sh
[warning] 217-217: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 247-247: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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). (2)
- GitHub Check: Build Libs for esp32s3
- GitHub Check: Build Slave Firmware
🔇 Additional comments (6)
tools/copy-mem-variant.sh (1)
14-17: Conditional MEMCONF suffix logic looks correct.The frequency suffix is properly appended to MEMCONF only when IDF_TARGET is esp32s3 and MEM_VARIANT_FREQ is non-empty. This integrates cleanly with the downstream mkdir/mv operations that use MEMCONF as a directory path.
tools/copy-libs.sh (1)
16-19: MEMCONF suffix logic matches copy-mem-variant.sh—looks good.Same conditional append of MEM_VARIANT_FREQ for esp32s3 targets. Properly prevents unintended suffix for other targets.
configs/builds.json (2)
168-168: Verify intent of idf_libs frequency change for esp32s3.The idf_libs frequency for esp32s3 changed from
120mto80m. This is a downgrade in build configuration speed. Confirm this is intentional (e.g., for stability, compatibility, or resource management).Provide context: Is this frequency reduction intentional? Does it align with the PR objective ("add speed to mem variant folder")? If speed-related, this might be a preparatory change for the new mem_variants setup.
175-181: mem_variants expansion for esp32s3—data structure is consistent and coherent.The esp32s3 mem_variants expanded from 2 to 5 entries, providing richer frequency/RAM-type combinations:
- Added 80m variants (both opi_ram and qio_ram)
- Retained and reordered 120m variants
- All follow the consistent
[flash_mode, frequency, ram_type]structureThis aligns with the PR objective of adding frequency-based memory variant folders. The data structure is ready for build.sh extraction of
.[1](frequency) and downstream MEMCONF suffix appending.build.sh (2)
245-248: No issues found - code assumption verified.All esp32s3 mem_variants follow the
[flash_mode, frequency, ram_type]structure with frequency consistently at index [1]. The extraction in build.sh is safe and correct.
214-218: No issues found; the code is safe.The extraction at line 217 correctly assumes
.idf_libs[1]exists and contains frequency data. Verification confirms:
- All targets have at least 2 elements in
idf_libs(element [1] is frequency for all)- esp32s3 has 3 elements, others have 2
- The extraction is properly guarded with
if [ "$target" = "esp32s3" ]at line 216- No other unsafe
idf_libsaccess patterns exist in build.sh
Summary by CodeRabbit