-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert usermods to static libraries #4480
base: main
Are you sure you want to change the base?
Conversation
Redesign the usermod system so that usermods are implemented as PlatformIO libraries instead of headers. This permits them to call for dependencies, and eliminates the compiler flags for enabling each one, allowing the build cache to behave better. The usermod list is built using some linker magic to construct a static list in ROM memory. This eliminates the need for wasting SRAM on something fixed at build time.
Look for 'usermod_v2_x' as well. This could be removed later if we clean up the folder names.
Borrowed library definition from @netmindz's work on Aircoookie#4476.
Annoyingly, PlatformIO requires locally-defined properties to be prefixed with 'custom_' ; I'd've liked it to just be 'usermods' but no such luck. :( |
platformio.ini
Outdated
board_build.partitions = ${esp32.default_partitions} ;; default partioning for 4MB Flash - can be overridden in build envs | ||
# additional build flags for audioreactive - must be applied globally | ||
AR_build_flags = -D sqrt_internal=sqrtf ;; -fsingle-precision-constant ;; forces ArduinoFFT to use float math (2x faster) | ||
AR_lib_deps = kosme/arduinoFFT @ 2.0.1 ;; for pre-usermod-library platformio_override compatibility |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are updating to be a library, why do we still need the old AR_lib_deps
info as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Compatibility with old platformio_override files that reference it. If we remove it, git pull
breaks platformIO completely until you clean all references out of your overrides.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If folk have platform_override that is trying to use AR, is having AR_build_flags
that that doesn't have the old define just going to cause confusion? They will think they have the flags, but now that's not enough to actually enable
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this would be a breaking change that will need to be staged appropriately. Unfortunately if your platformio file is outright invalid, PlatformIO in VSCode doesn't tell you what's wrong, it just blankly refuses to operate with no error message. I found it more useful to at least keep the tool operating while I was correcting my build definitions, but that's just my $0.02.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll have a bit of a play around with this over the next few days
It looks like a great start, the key things in my mind however are what extra changes we can make both in terms of code, documentation and release management to help make the transition as painless as possible
One quick question - how essential is the rename from .h to .cpp ?
@@ -1,482 +0,0 @@ | |||
#include "wled.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it better to delete this file or retain but with message about the new format - What would be the best way to highlight the changes to developers of the may forks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good question! I'd figured the deletion will create an obvious merge conflict, so it's clear that something must be done. Personally I'd rather not leave a .cpp file that the build tooling will have to grovel over only for it to do nothing.
Yes. Unfortunately it looks like the docs aren't in the code proper (beyond the examples), which is a little frustrating when different versions might have different approaches. :(
Some .cpp file has to instantiate the module object; and this must be in the library, or you're back to all the problems with |
How about a migration script that looks for an existing .h of a usermod, moves with git and then automatically added the extra static field and call to register? |
Are you aware of the build failure when using the audioreactive usermod? @willmmiles
|
Not impossible. but nontrivial - the script would have to pick out the class name and any constructor arguments. We'd probably need to call on |
platformio.ini
Outdated
@@ -424,23 +420,23 @@ build_flags = ${common.build_flags} ${esp8266.build_flags} -D WLED_RELEASE_NAME= | |||
board = esp32dev | |||
platform = ${esp32.platform} | |||
platform_packages = ${esp32.platform_packages} | |||
custom_usermods = audioreactive | |||
build_unflags = ${common.build_unflags} | |||
build_flags = ${common.build_flags} ${esp32.build_flags} -D WLED_RELEASE_NAME=\"ESP32\" #-D WLED_DISABLE_BROWNOUT_DET | |||
${esp32.AR_build_flags} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any way to move the build_flags used for the usermod into it's library.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In most cases, yes.
In the specific case of AR, the build flags aren't actually for the AR module, they're for the FFT library dependency. Unfortunately, the only way to affect some dependent library's build is to do it at the project level. PlaformIO doesn't let one library define flags for one of its dependencies. https://community.platformio.org/t/setting-flags-defines-for-building-a-librarys-dependency/36744
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, maybe there is a way, using platformio hook scripts in the library. Working on it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are certainly usermods that use more typical build_flags that affect the usermod itself, but perhaps these still need to stay in the main platformio.ini if they are actual optional flags the user chooses to set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed! It turns out there is a way though platformio hook scripts. 8527d23
Yup, looking in to it - seems like some sort of case sensitivity thing. |
I'm just knocking up a quick PoC of a script. If nothing else it will help show how many of the usermods in the official AC repo do not follow proper naming convention etc |
Yup, a breaking API change like this is a good opportunity for cleanup. Honestly I'd figured on just going through them by hand, there aren't so many it's a huge problem. With a solution like this in hand, I was hoping to ultimately create a "core" modules folder with the modules that this team is directly supporting; and then work towards migrating anything with a "WLED_DISABLE_X" flag in to becoming a core module instead. (Yes, I recognize that will likely require some expansion of the module API to handle specialized timing and polling requirements; I think that's probably a good thing, as if we have one use case for it, there may be others.) Further, the linker section approach demonstrated here for the static usermod list could also be used later to make static library-based FX or bus type lists, too, allowing modules to include new FX or bus types without needing any RAM to build a runtime data structure. One step at a time, though! |
I didn't want to mess up your main PR with my attempts at automatic migration, so I've popped into it's own PR for now #4481 These are the ones that don't follow any common naming WARNING: usermods/ADS1115_v2/ADS1115_v2.h missing |
Fixed the naming of a few, so now just WARNING: Artemis_reciever possibly still v1 usermod |
I think it might be because we are missing the |
Confirmed. If I hack wled.h to define LOROL_LITTLEFS the problem goes away. Do you have a wled_custom.h or something locally? |
When you switch platform from 3.5.0 to 6.3.x, Lorol's LittleFS will not be needed any more. |
When processing usermods, update their include path properties before the LDF runs, so it can see through wled.h.
Move the "all usermods" logic in to the platformio script, so the 'usermods' environment can be built in any checkout without extra setup commands.
all_usermods = [f for f in usermod_dir.iterdir() if f.is_dir() and f.joinpath('library.json').exists()] | ||
|
||
if env['PIOENV'] == "usermods": | ||
# Add all usermods | ||
env.GetProjectConfig().set(f"env:usermods", 'custom_usermods', " ".join([f.name for f in all_usermods])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The downside to this change is that you have no control of what you are building
For testing I was setting my local platform_override.ini to only list the mods I was trying to build.
I guess I can still do that, but just need to name my env somehting like usermods_local
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not thrilled at magic environment names, but I think it's important to be able to locally replicate what the CI system does without having to run shell commands to update local state after every git pull
. I'm open to alternative suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly less need for an "all in one" build if we just let the CI handle a matrix build for usermods, then just define the one we are working on locally in our own platform_override ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I stand by my opinion from above -- I think it's important that what the CI system does should be easily and directly reproducible by developers. I don't think it's appropriate to have to hand hack custom files in your personal workspace to replicate what the CI does; that's just asking to get it wrong.
From a practical perspective, I'd also like it to be simple to validate my local changes before pushing to the CI. If we depend on matrix builds, that becomes almost impossible. Id be stuck waiting on the CI system any time I want to have some confidence I haven't inadvertently broken the build of some random usermod.
Finally, I don't know much about GitHub CI; I'm concerned that adding dozens of extra builds per run would be costly.
Any ideas why I'm now seeing errors for missing |
Looks like the load_usermods.py doesn't have any reference to wled00, unlike the original libray.json. No idea if this is correct, but it makes the error for away @willmmiles
|
Nope, I'm talking rubbish. Every time you do a build after a full clean you get the missing wled.h, however just clicking build again fixes it |
Yeah, there's something wrong with building the dependency list; it's sometimes not locating the usermods early enough, so the patch-up process isn't able to configure them. I can't replicate it on my system, even in a fresh container, but the CI does it repeatably, so I was debugging in a separate branch. I'll update when I've got it sorted out. |
What OS and platformIO version are you on? I'm using linux and 6.1.4 and is is totally repeatable. Full clean, Build - error no wled.h, Build - OK |
Hm, maybe that's it and it was changed in a newer PlatformIO -- library management is definitely an area of active development. I'm using whatever came down with the devcontainer; currently it's PlatformIO 6.1.17b2. Once we've got a working solution, I'm going to see about opening a discussion on PlatformIO's own issue board about our "modules" use case and see what the devs there think about it. |
Should now work for new *and* old versions of PlatformIO!
- Check after including wled.h - Use WLED_DISABLE_MQTT instead of WLED_ENABLE_MQTT
A better solution for cross-module communication is required!
Define a couple pins, leave a note where the usermod list comes from.
This is now managed centrally.
Known issues in the current state:
|
Redesign the usermod intergration so that usermods are implemented as PlatformIO libraries instead of headers. This permits them to call for dependencies, and eliminates the compiler flags for enabling each one, allowing the build cache to operate consistently regardless of the selected modules.
The usermod list is built using some linker magic to construct a static list in ROM memory. This eliminates the need for wasting SRAM on something well known at build time.
To use this integration:
.h
should be moved to a.cpp
, with a static instantiation of the module class at the endREGISTER_USERMOD()
to add it to the compiled-in usermod listcustom_usermods = audioreactive animartrix
builds in the "audioreactive" and "animartrix" usermodsThe
custom_usermods
property is implemented via a platformio hook script that constructs the required symlink lines and injects them in to the build environment.Currently I have converted 'auto_save', 'audioreactive', and 'animartrix' (based on #4476) to library-type as proof-of-concept.