Skip to content

Dynarray fixes#5502

Open
willmmiles wants to merge 6 commits intowled:mainfrom
willmmiles:dynarray-fixes
Open

Dynarray fixes#5502
willmmiles wants to merge 6 commits intowled:mainfrom
willmmiles:dynarray-fixes

Conversation

@willmmiles
Copy link
Copy Markdown
Member

@willmmiles willmmiles commented Apr 13, 2026

Bring in the dynarray/usermod improvements from the V5 work in progress. This improves the system to support LTO and framework = arduino, esp-idf, as well as fixes some issues with dynarray linkage on RISCV platforms.

#5497, but for main - with one more commit to finish cleaning up some of the messaging.

Summary by CodeRabbit

  • Refactor
    • Unified dynamic array handling across platforms with a cross-platform linker script injection approach, replacing platform-specific implementations.
    • Enhanced module validation system to use compilation-unit analysis for more accurate module detection in builds.
    • Streamlined build infrastructure for improved consistency and maintainability across different hardware platforms.

willmmiles and others added 6 commits April 12, 2026 11:38
Rather than append a linker file, we edit the upstream supplied ones to
add our section to the binaries.  Works better on all platforms.

Co-Authored-By: Claude <noreply@anthropic.com>
Use readelf instead of nm for great speed.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 13, 2026

Walkthrough

Refactors the dynamic array linker integration from platform-specific scripts to cross-platform injection into rodata regions. Simultaneously replaces module validation from ELF symbol-based detection using nm to DWARF compilation-unit analysis using readelf. Unifies ESP8266 and ESP32 dynarray section handling to use a common .dynarray section.

Changes

Cohort / File(s) Summary
Dynarray Linker Integration
pio-scripts/dynarray.py, tools/dynarray_espressif32.ld, wled00/dynarray.h
Replaces platform-specific linker-script approach with fragment injection at rodata markers. Adds inject_before_marker() to patch linker scripts in-place. For ESP32, locates and patches sections.ld; for ESP8266, patches local.eagle.app.v6.common.ld post-action. Removes platform-specific .dynarray section definitions and unifies DYNARRAY_SECTION macro across ESP32/ESP8266.
Module Validation
pio-scripts/validate_modules.py
Replaces nm --defined-only symbol-based validation with readelf --debug-dump=info DWARF compilation-unit analysis. Introduces _get_readelf_path(), tracks CU boundaries, matches DW_AT_name/DW_AT_comp_dir against module sources. Changes count_usermod_objects() to compute count from sentinel address spans in .dynarray.usermods.* sections instead of section occurrence counting.

Sequence Diagram(s)

sequenceDiagram
    participant Build as Build System
    participant Script as dynarray.py
    participant FS as File System
    participant Linker as Linker (ld)
    
    Build->>Script: Execute build script with PIOPLATFORM
    Script->>Script: Determine platform (espressif32/8266)
    
    alt ESP32 Path
        Script->>FS: Search LIBPATH for sections.ld
        alt Found in LIBPATH
            Script->>FS: Copy sections.ld to $BUILD_DIR/dynarray_sections.ld
            Script->>FS: Patch: inject before _rodata_end marker
            Script->>Script: Rewrite LINKFLAGS (-Tsections.ld → absolute path)
        else Not Found
            Script->>Script: Register post-action callback
            Script->>FS: Patch $BUILD_DIR/sections.ld (after generation)
        end
    else ESP8266 Path
        Script->>Script: Register post-action callback
        Script->>FS: Patch $BUILD_DIR/ld/local.eagle.app.v6.common.ld
        Script->>FS: Inject before _irom0_text_end marker
    end
    
    Script->>Linker: Pass modified LINKFLAGS with injected fragments
    Linker->>FS: Link ELF with patched linker scripts
Loading
sequenceDiagram
    participant Build as Build System
    participant Script as validate_modules.py
    participant Readelf as readelf Tool
    participant ELF as ELF File
    participant Modules as Module Builders
    
    Build->>Script: Validate modules in ELF
    Script->>Readelf: Execute readelf --debug-dump=info on ELF
    Readelf->>ELF: Parse DWARF compilation units
    Readelf->>Script: Return DWARF CU headers (DW_AT_name, DW_AT_comp_dir)
    
    Script->>Script: Iterate CU boundaries
    Script->>Script: Track compilation unit metadata
    Script->>Script: Match DW_AT_comp_dir against module source paths
    alt Virtual path (/IDF_PROJECT)
        Script->>Script: Fallback to $PROJECT_DIR substitution
    end
    
    Script->>Modules: Compare matched paths against module_lib_builders
    Script->>Script: Compute confirmed module set
    Script->>Script: Return confirmed modules
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

  • custom_usermod improvements #5403: Both PRs modify the same dynarray/linker integration and module validation code paths (pio-scripts/dynarray.py, validate_modules.py, tools/dynarray_espressif32.ld, wled00/dynarray.h), indicating synchronized or iterative refactoring of this subsystem.

Suggested reviewers

  • DedeHai
  • softhack007
  • netmindz
🚥 Pre-merge checks | ✅ 1 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 77.78% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Dynarray fixes' is overly vague and does not convey the specific nature or scope of the changes. While it mentions dynarray, it lacks clarity about what fixes are being made or why they matter. Consider a more specific title like 'Refactor dynarray injection to use linker script fragments' or 'Improve dynarray cross-platform support with linker script injection' to better describe the core changes.
✅ Passed checks (1 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
pio-scripts/validate_modules.py (1)

112-112: Consider documenting the 32-bit target assumption.

The hardcoded ENTRY_SIZE = 4 is correct for current ESP8266/ESP32 targets (both 32-bit), but the assumption could be made explicit for future maintainability.

📝 Suggested documentation
-    ENTRY_SIZE = 4  # sizeof(Usermod*) on 32-bit targets
+    ENTRY_SIZE = 4  # sizeof(Usermod*); all supported WLED platforms are 32-bit
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@pio-scripts/validate_modules.py` at line 112, The hardcoded ENTRY_SIZE = 4
assumes 32-bit targets; update the code around ENTRY_SIZE to document that
assumption explicitly (e.g., "ENTRY_SIZE = 4  # sizeof(Usermod*) on 32-bit
targets (ESP8266/ESP32)"). Also add a short note about future-proofing: either
compute the pointer size dynamically (using struct.calcsize('P')) or add a
runtime assertion/guard that raises a clear error if the platform pointer size
is not 4, so callers of ENTRY_SIZE (the ENTRY_SIZE symbol) will behave correctly
on non-32-bit platforms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@pio-scripts/validate_modules.py`:
- Line 112: The hardcoded ENTRY_SIZE = 4 assumes 32-bit targets; update the code
around ENTRY_SIZE to document that assumption explicitly (e.g., "ENTRY_SIZE = 4 
# sizeof(Usermod*) on 32-bit targets (ESP8266/ESP32)"). Also add a short note
about future-proofing: either compute the pointer size dynamically (using
struct.calcsize('P')) or add a runtime assertion/guard that raises a clear error
if the platform pointer size is not 4, so callers of ENTRY_SIZE (the ENTRY_SIZE
symbol) will behave correctly on non-32-bit platforms.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: f57b7275-5745-45ee-a088-10b393377fdb

📥 Commits

Reviewing files that changed from the base of the PR and between 9651061 and 6d7c1d0.

📒 Files selected for processing (4)
  • pio-scripts/dynarray.py
  • pio-scripts/validate_modules.py
  • tools/dynarray_espressif32.ld
  • wled00/dynarray.h
💤 Files with no reviewable changes (2)
  • tools/dynarray_espressif32.ld
  • wled00/dynarray.h

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant