Open
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>
Contributor
WalkthroughImplements a dynamic array mechanism for usermod registration using linker scripts and platform-specific sections, replacing static linker-section-based registration. Includes new build script logic for custom usermod handling, platform-specific linker scripts, and refactored usermod registration and iteration. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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
Comment on lines
+77
to
+78
| name = Path(parsed.path.rstrip('/')).name | ||
| return name.split('.')[0].strip() or None |
Contributor
There was a problem hiding this comment.
Avoid truncating dependency names at the first dot.
Line 78 can mispredict names like my.mod, which may prevent is_wled_module() from recognizing the dependency.
Proposed fix
- name = Path(parsed.path.rstrip('/')).name
- return name.split('.')[0].strip() or None
+ name = Path(parsed.path.rstrip('/')).name.strip()
+ if name.endswith('.git'):
+ name = name[:-4]
+ return name or None📝 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.
Suggested change
| name = Path(parsed.path.rstrip('/')).name | |
| return name.split('.')[0].strip() or None | |
| name = Path(parsed.path.rstrip('/')).name.strip() | |
| if name.endswith('.git'): | |
| name = name[:-4] | |
| return name or None |
🤖 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 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.
Member
|
Using a consistent field for usermods will be a big improvement. Have you tested against the V5 branch? |
Member
Author
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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
Release Notes
New Features
Build System
Improvements