Conversation
Use a custom linker section to hold dynamic arrays such as the usermod list. This provides an extensible solution for wider use in the future (such as FX or Bus drivers), as well as IDF v5 compatibility.
Break out the dynamic array macros to a re-usable component and fix the implementation.
Expand the parsing of custom_usermods to accept the same syntax as lib_deps. This allows external usermods to appear on the custom_usermods lines. Also includes comment handling. Co-Authored-By: Claude <noreply@anthropic.com>
WalkthroughImplements a linker-section based dynamic-array mechanism for usermod registration (headers, linker fragment, and post-build script), replaces .dtors registration with dynarray macros across usermod code, and overhauls PlatformIO usermod parsing and ELF-based validation to support external/custom usermods. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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.
Actionable comments posted: 5
🧹 Nitpick comments (1)
wled00/um_manager.cpp (1)
7-13: Update stale registration comments to match dynarray behavior.This comment block still describes
.dtorsas the general mechanism, but non-ESP8266 targets now use.dynarray.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wled00/um_manager.cpp` around lines 7 - 13, The comment above DECLARE_DYNARRAY(Usermod*, usermods) is outdated about using the '.dtors' segment; update it to describe current dynarray placement by mentioning that non-ESP8266 targets use the '.dynarray' section (while ESP8266 may still rely on '.dtors'), explain zero-length arrays get addresses but consume no flash and the numeric suffix ensures ordering via the linker, and keep reference to DECLARE_DYNARRAY and the usermods symbol so future readers know this macro now maps to the '.dynarray' mechanism on modern targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pio-scripts/dynarray.py`:
- Around line 14-16: The code naively calls linkflags.index("memory.ld") and
will raise if LINKFLAGS doesn't contain that exact token or contains a path
variant; update the logic around the linkflags/list mutation (the variables
linkflags, linker_script and the env.Replace(LINKFLAGS=linkflags) call) to:
ensure linkflags is a list, search for an element that endswith or contains
"memory.ld" (e.g., any(f for f in linkflags if "memory.ld" in f) ), and if found
insert linker_script after that element; if not found, append the linker_script
instead; finally call env.Replace(LINKFLAGS=linkflags).
In `@pio-scripts/load_usermods.py`:
- Around line 77-78: The current code truncates filenames at the first dot by
doing name.split('.')[0], which breaks names like "my.mod" and prevents
is_wled_module() from matching; change the return to preserve the full base name
without the final extension (e.g. use Path(name).stem or name.rsplit('.', 1)[0])
and keep the .strip() and None fallback, updating the expression that
assigns/returns name accordingly.
In `@platformio_override.sample.ini`:
- Line 554: The sample config currently references the external usermod repo
using a moving branch ref
"https://github.com/wled/wled-usermod-example.git#main"; change that to a fixed
tag or commit hash (for example replace "#main" with "#v1.0.0" or
"#<commit-hash>") so the external usermod URL is pinned and sample builds are
reproducible; update the URL string accordingly wherever
"https://github.com/wled/wled-usermod-example.git#main" appears.
In `@wled00/dynarray.h`:
- Line 17: DYNARRAY_MEMBER currently emits namespace-scope symbols; make them
internal by adding static to the declaration so symbols get internal linkage and
match DECLARE_DYNARRAY's static array markers—update the DYNARRAY_MEMBER macro
(the macro name DYNARRAY_MEMBER and its expansion that defines member_name in
the DYNARRAY_SECTION) to prepend static to the generated symbol declaration
while keeping the section attribute and used attribute unchanged.
In `@wled00/um_manager.cpp`:
- Around line 51-54: The serial debug block in readFromConfig() unconditionally
dereferences *x and always logs, which can access invalid memory when the
dynarray is empty and creates noisy serial output; remove the unconditional
dereference and noisy prints by guarding access with the usermods count (use
getCount() or check DYNARRAY_BEGIN(usermods) != DYNARRAY_END(usermods)) before
casting/reading *x, and wrap the remaining debug Serial.printf_P calls behind a
debug/build flag (or only run them when count>0) so they don't run in normal
builds; adjust references to DYNARRAY_BEGIN(usermods), usermods, x and
readFromConfig() accordingly.
---
Nitpick comments:
In `@wled00/um_manager.cpp`:
- Around line 7-13: The comment above DECLARE_DYNARRAY(Usermod*, usermods) is
outdated about using the '.dtors' segment; update it to describe current
dynarray placement by mentioning that non-ESP8266 targets use the '.dynarray'
section (while ESP8266 may still rely on '.dtors'), explain zero-length arrays
get addresses but consume no flash and the numeric suffix ensures ordering via
the linker, and keep reference to DECLARE_DYNARRAY and the usermods symbol so
future readers know this macro now maps to the '.dynarray' mechanism on modern
targets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 37b8449e-f0fe-46d1-a5be-59981f5b4465
📒 Files selected for processing (9)
pio-scripts/dynarray.pypio-scripts/load_usermods.pypio-scripts/validate_modules.pyplatformio.iniplatformio_override.sample.initools/dynarray_espressif32.ldwled00/dynarray.hwled00/fcn_declare.hwled00/um_manager.cpp
|
Using a consistent field for usermods will be a big improvement. Have you tested against the V5 branch? |
I just tested with my own v5 WIP (it's IMO quite a bit cleaner, but shares no history, so not really a good candidate to merge .. I'll still post it up shortly though). The |
|
I'll give this a test when I get a moment |
When LTO is enabled, the map file no longer provides a positive indication of whether a given object file has contributed to the final binary. Instead use nm to parse the debug data in the .elf file. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pio-scripts/load_usermods.py`:
- Around line 70-80: _predict_dep_name() currently only extracts a basename for
GitHub/GitLab/Bitbucket hosts and returns None for other URL hosts, causing
entries like symlink://, file:// and self-hosted URLs to be skipped; change the
logic so that after parsing the URL (urlparse(entry)) you always derive a
candidate name from the path (e.g., use
Path(parsed.path.rstrip('/')).name.strip()) when parsed.netloc is not one of
('github.com','gitlab.com','bitbucket.com') and still strip a trailing ".git" if
present; ensure the function returns that basename (not None) so entries are
added to _custom_usermod_names and is_wled_module() will treat them as WLED
modules.
In `@pio-scripts/validate_modules.py`:
- Around line 43-50: The call to subprocess.run for nm (using nm_path) doesn't
check result.returncode, so when nm exits non‑zero the code still consumes
result.stdout; update the logic after subprocess.run to check result.returncode
and treat any non-zero as a failure: log a warning including the exit code (and
stderr if available) and return the conservative set ({Path(b.build_dir).name
for b in module_lib_builders}) instead of proceeding to parse nm_output; keep
the existing except block for timeouts/OS errors and reference nm_path,
result.returncode, result.stderr, nm_output, and module_lib_builders when
implementing the check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 6f207ee6-63dc-4eca-a407-6c768da58a3b
📒 Files selected for processing (3)
pio-scripts/load_usermods.pypio-scripts/validate_modules.pywled00/um_manager.cpp
| if _URL_SCHEME_RE.match(entry): | ||
| parsed = urlparse(entry) | ||
| if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'): | ||
| parts = [p for p in parsed.path.split('/') if p] | ||
| if len(parts) >= 2: | ||
| name = parts[1] | ||
| else: | ||
| name = Path(parsed.path.rstrip('/')).name.strip() | ||
| if name.endswith('.git'): | ||
| name = name[:-4] | ||
| return name or None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd load_usermods.py -x wc -l {}Repository: wled/WLED
Length of output: 87
🏁 Script executed:
cat -n pio-scripts/load_usermods.py | head -150Repository: wled/WLED
Length of output: 6777
🏁 Script executed:
cat -n pio-scripts/load_usermods.py | sed -n '60,130p'Repository: wled/WLED
Length of output: 3194
🏁 Script executed:
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path
test_urls = [
"symlink:///path/to/usermod_name",
"file:///var/lib/repos/my_usermod",
"https://git.example.com/user/repo.git",
"git://self-hosted.org/usermods/cool-mod",
"https://github.com/user/my-usermod.git",
]
for url in test_urls:
parsed = urlparse(url)
# Current logic (lines 70-80) - only github/gitlab/bitbucket
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
current_name = parts[1]
else:
current_name = Path(parsed.path.rstrip('/')).name.strip()
else:
current_name = None
# Proposed logic (all schemes)
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
proposed_name = name or None
print(f"URL: {url}")
print(f" Current: {current_name}")
print(f" Proposed: {proposed_name}")
print()
EOFRepository: wled/WLED
Length of output: 463
🏁 Script executed:
cat -n pio-scripts/load_usermods.py | sed -n '140,210p'Repository: wled/WLED
Length of output: 3592
🏁 Script executed:
# Check if there's any validation or fallback that might handle unpredicted names
rg "library\.json" pio-scripts/ -A 3 -B 3Repository: wled/WLED
Length of output: 2087
🏁 Script executed:
# Check validate_modules.py to see how it uses WLED_MODULES
cat -n pio-scripts/validate_modules.py | head -60Repository: wled/WLED
Length of output: 3076
🏁 Script executed:
# Let's verify the proposed fix more carefully - ensure it handles all current code paths
cat -n pio-scripts/load_usermods.py | sed -n '70,93p'Repository: wled/WLED
Length of output: 1197
🏁 Script executed:
# Check if there are any tests or examples of custom_usermods usage
fd -t f "platformio.ini" -exec grep -l "custom_usermods" {} \;Repository: wled/WLED
Length of output: 303
🏁 Script executed:
fd platformio.ini -type fRepository: wled/WLED
Length of output: 223
🏁 Script executed:
# Check for any issues or PRs related to custom_usermods and name prediction
git log --oneline --all -- "pio-scripts/load_usermods.py" | head -20Repository: wled/WLED
Length of output: 940
🏁 Script executed:
# Let's verify the fix handles edge cases correctly
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path
test_cases = [
"symlink:///path/to/usermod_name",
"symlink:///path/to/usermod_name/",
"file:///var/lib/repos/my_usermod",
"file:///var/lib/repos/my_usermod.git",
"https://git.example.com/user/repo.git",
"git://self-hosted.org/usermods/cool-mod",
"https://github.com/user/my-usermod.git",
"symlink:///", # edge: empty path
"file://", # edge: no path
]
print("Testing proposed fix logic:")
for url in test_cases:
parsed = urlparse(url)
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
result = name or None
print(f"{url:50} → {result}")
EOFRepository: wled/WLED
Length of output: 640
🏁 Script executed:
# Let's check how PlatformIO resolves library names from deps to confirm behavior
rg "dep\.name" pio-scripts/load_usermods.py -B 2 -A 2Repository: wled/WLED
Length of output: 195
🏁 Script executed:
# Check if there's any fallback mechanism when a library can't be recognized by name
rg "library\.json.*name" pio-scripts/ -A 2 -B 2Repository: wled/WLED
Length of output: 483
🏁 Script executed:
# Verify the suggested fix doesn't have any syntax issues by testing it more thoroughly
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path
# Current code (lines 70-80)
def current_logic(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
return name or None
return None # Falls through for other schemes
# Proposed fix (restructured)
def proposed_logic(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
else:
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
return name or None
test_cases = [
"https://github.com/user/my-usermod.git", # Should work in both
"symlink:///path/to/usermod", # Should fail in current, work in proposed
"file:///path/my_usermod.git", # Should fail in current, work in proposed
]
for url in test_cases:
curr = current_logic(url)
prop = proposed_logic(url)
status = "✓" if curr == prop or (curr is None and prop is not None) else "✗"
print(f"{status} {url:50} current={curr:15} proposed={prop}")
EOFRepository: wled/WLED
Length of output: 307
🏁 Script executed:
# Fix the test to handle None values
python3 << 'EOF'
from urllib.parse import urlparse
from pathlib import Path
# Current code (lines 70-80)
def current_logic(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
return name or None
return None # Falls through for other schemes
# Proposed fix (restructured)
def proposed_logic(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
else:
name = Path(parsed.path.rstrip('/')).name.strip()
if name.endswith('.git'):
name = name[:-4]
return name or None
test_cases = [
"https://github.com/user/my-usermod.git",
"symlink:///path/to/usermod",
"file:///path/my_usermod.git",
]
print("Comparison of current vs proposed logic:")
for url in test_cases:
curr = current_logic(url)
prop = proposed_logic(url)
improved = "✓" if curr != prop else "="
print(f"{improved} {url:50}")
print(f" current: {str(curr):20} proposed: {str(prop)}")
EOFRepository: wled/WLED
Length of output: 414
🏁 Script executed:
# Now verify the impact: does is_wled_module rely on this name matching?
# Check how PlatformIO assigns names to library dependencies
rg "dep\.name" pio-scripts/ -B 3 -A 3Repository: wled/WLED
Length of output: 492
Generic URL dependencies bypass WLED-module classification.
When _URL_SCHEME_RE matches a URL with a hostname outside GitHub/GitLab/Bitbucket, _predict_dep_name() returns None, so the entry never enters _custom_usermod_names. This causes is_wled_module() to fail for symlink://, file://, and self-hosted URLs, even though they're passed to PlatformIO. Such usermods then miss include-path setup, the -g flag, and libArchive=false validation.
Extract basename for any URL scheme to ensure these external usermods receive the same WLED-specific treatment:
Suggested fix
if _URL_SCHEME_RE.match(entry):
parsed = urlparse(entry)
if parsed.netloc in ('github.com', 'gitlab.com', 'bitbucket.com'):
parts = [p for p in parsed.path.split('/') if p]
if len(parts) >= 2:
name = parts[1]
else:
name = Path(parsed.path.rstrip('/')).name.strip()
- if name.endswith('.git'):
- name = name[:-4]
- return name or None
+ else:
+ name = Path(parsed.path.rstrip('/')).name.strip()
+ if name.endswith('.git'):
+ name = name[:-4]
+ return name or NoneAlso applies to: 96-102, 114-125
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pio-scripts/load_usermods.py` around lines 70 - 80, _predict_dep_name()
currently only extracts a basename for GitHub/GitLab/Bitbucket hosts and returns
None for other URL hosts, causing entries like symlink://, file:// and
self-hosted URLs to be skipped; change the logic so that after parsing the URL
(urlparse(entry)) you always derive a candidate name from the path (e.g., use
Path(parsed.path.rstrip('/')).name.strip()) when parsed.netloc is not one of
('github.com','gitlab.com','bitbucket.com') and still strip a trailing ".git" if
present; ensure the function returns that basename (not None) so entries are
added to _custom_usermod_names and is_wled_module() will treat them as WLED
modules.
| result = subprocess.run( | ||
| [nm_path, "--defined-only", "-l", str(elf_path)], | ||
| capture_output=True, text=True, errors="ignore", timeout=120, | ||
| ) | ||
| nm_output = result.stdout | ||
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | ||
| secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True) | ||
| return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "validate_modules.py" -type fRepository: wled/WLED
Length of output: 86
🏁 Script executed:
cat -n pio-scripts/validate_modules.py | head -100Repository: wled/WLED
Length of output: 5057
🏁 Script executed:
# Look for the check_elf_modules function
rg -n "def check_elf_modules" pio-scripts/validate_modules.py -A 50Repository: wled/WLED
Length of output: 2785
🏁 Script executed:
# Verify subprocess.run() behavior by checking Python documentation/behavior
python3 << 'EOF'
import subprocess
import sys
# Check: does subprocess.run() raise on non-zero exit by default?
print("subprocess.run() behavior with check=False (default):")
print("- Does NOT raise exception on non-zero exit code")
print("- Returns CompletedProcess object with returncode attribute")
print("- stdout/stderr available regardless of exit code")
print()
# Show what happens with an nm-like scenario
result = subprocess.run(['sh', '-c', 'exit 5'], capture_output=True, text=True)
print(f"Example: exit code 5 with check=False")
print(f" returncode: {result.returncode}")
print(f" exception raised: No")
print(f" stdout available: {repr(result.stdout)}")
EOFRepository: wled/WLED
Length of output: 366
🏁 Script executed:
# Check if there are other subprocess calls in the file for pattern comparison
rg -n "subprocess\." pio-scripts/validate_modules.pyRepository: wled/WLED
Length of output: 164
Check nm exit code before consuming output.
subprocess.run() without check=True doesn't raise on non-zero exit—it returns CompletedProcess with the exit code in returncode. If nm fails but doesn't crash, the code consumes potentially empty or invalid stdout and proceeds to process it, leading to an empty confirmed set rather than the conservative pass. This creates asymmetric error handling: spawn/timeout failures return all modules, but tool execution failures return no modules.
Suggested fix
result = subprocess.run(
[nm_path, "--defined-only", "-l", str(elf_path)],
capture_output=True, text=True, errors="ignore", timeout=120,
)
+ if result.returncode != 0:
+ secho(
+ f"WARNING: nm failed with exit code {result.returncode}; skipping per-module validation",
+ fg="yellow",
+ err=True,
+ )
+ return {Path(b.build_dir).name for b in module_lib_builders}
nm_output = result.stdout📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result = subprocess.run( | |
| [nm_path, "--defined-only", "-l", str(elf_path)], | |
| capture_output=True, text=True, errors="ignore", timeout=120, | |
| ) | |
| nm_output = result.stdout | |
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | |
| secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True) | |
| return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass | |
| result = subprocess.run( | |
| [nm_path, "--defined-only", "-l", str(elf_path)], | |
| capture_output=True, text=True, errors="ignore", timeout=120, | |
| ) | |
| if result.returncode != 0: | |
| secho( | |
| f"WARNING: nm failed with exit code {result.returncode}; skipping per-module validation", | |
| fg="yellow", | |
| err=True, | |
| ) | |
| return {Path(b.build_dir).name for b in module_lib_builders} | |
| nm_output = result.stdout | |
| except (subprocess.TimeoutExpired, FileNotFoundError, OSError) as e: | |
| secho(f"WARNING: nm failed ({e}); skipping per-module validation", fg="yellow", err=True) | |
| return {Path(b.build_dir).name for b in module_lib_builders} # conservative pass |
🧰 Tools
🪛 Ruff (0.15.4)
[error] 43-43: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pio-scripts/validate_modules.py` around lines 43 - 50, The call to
subprocess.run for nm (using nm_path) doesn't check result.returncode, so when
nm exits non‑zero the code still consumes result.stdout; update the logic after
subprocess.run to check result.returncode and treat any non-zero as a failure:
log a warning including the exit code (and stderr if available) and return the
conservative set ({Path(b.build_dir).name for b in module_lib_builders}) instead
of proceeding to parse nm_output; keep the existing except block for timeouts/OS
errors and reference nm_path, result.returncode, result.stderr, nm_output, and
module_lib_builders when implementing the check.
Some small upgrades on the custom_usermods integration:
lib_depssyntax incustom_usermods.platformio_override.sample.iniNote that
lib_depssyntax uses newlines as delimiters instead of spaces as delimiters. For backwards compatibility, this implementation treats cases that don't look exactly likelib_depsstyle references as if they're space delimited usermod lists that should come from this repo. If we're up for breaking compatibility, the code would be much simpler ifcustom_usermodswas always newline delimited.Summary by CodeRabbit
New Features
Build System
Improvements