add example demonstrating advanced PlatformIO environment inheritance #4618
Conversation
WalkthroughThe changes introduce a new environment configuration Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (1)
platformio_override.sample.ini (1)
539-544: Typo in Documentation CommentsThere's a small typo in the header comment at line 540: “Inhering” should be corrected to “Inheriting” for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio_override.sample.ini(1 hunks)
🔇 Additional comments (1)
platformio_override.sample.ini (1)
546-570: Advanced Environment Inheritance ConfigurationThe new
[env:esp32_eth_custom]block is well implemented and thoroughly documented. It correctly uses theextendsdirective to inherit from the upstreamenv:esp32_ethenvironment, and the separation betweenbuild_unflags(to remove conflicting upstream definitions) andbuild_flags(to add custom overrides) is clear and appropriately commented. This approach will help users override specific build flags without manually copying all upstream values, aligning well with the PR objectives.
Arcitec
force-pushed
the
advanced-pio-overrides
branch
from
ab69673 to
378fa59
Compare
|
Thanks Mr. Bot for finding a small comment typo. Fixed. :) |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
platformio_override.sample.ini (2)
552-561: Appropriate Use ofbuild_unflagsto Prevent Duplicate Flags
The configuration usesbuild_unflagsto remove specific upstream flag definitions (i.e.-D WLED_RELEASE_NAME="ESP32_Ethernet"and-D RLYPIN=-1). The detailed warning comments also help future maintainers understand the importance of specifying exact flag values. Consider adding a brief inline note indicating that any upstream changes must be reviewed here.
562-568: Overriding Upstream Flags withbuild_flags
The use ofbuild_flagsto append custom definitions (changing the release name to"ESP32_Ethernet_Custom"and settingRLYPINto24) correctly demonstrates the override approach. For added clarity, you might consider an inline comment explaining that these new definitions are intended to replace the unflagged upstream values. For example:-build_flags = ${env:esp32_eth.build_flags} - -D WLED_RELEASE_NAME=\"ESP32_Ethernet_Custom\" - -D RLYPIN=24 +build_flags = ${env:esp32_eth.build_flags} + # Override upstream release name and relay pin settings: + -D WLED_RELEASE_NAME=\"ESP32_Ethernet_Custom\" + -D RLYPIN=24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio_override.sample.ini(1 hunks)
🔇 Additional comments (2)
platformio_override.sample.ini (2)
539-544: Clear Documentation for Advanced Example Description
The added comment block clearly explains the advanced inheritance mechanism. It effectively informs users how to override specific upstream parameters without manual duplication.
546-551: New Environment Declaration and Inheritance
The new[env:esp32_eth_custom]environment is correctly declared and set to inherit fromenv:esp32_eth. This provides a solid basis for users to build upon the core configuration while applying custom overrides.
Adds a self-documented example demonstrating how a user can inherit from a core WLED configuration and add their own values without needing to copy all of the upstream values manually. This new method simplifies configuration maintenance for the users.
Arcitec
force-pushed
the
advanced-pio-overrides
branch
from
378fa59 to
c0d6cfc
Compare
|
thanks, @willmmiles can you check the example is correct please? I am not too familiar with the inner workings of envs. |
|
Hey Dede! :) I agreed with Mr. Bot about adding two more comment lines to super clearly delineate where the old values should be unset/set. Unfortunately I had pushed like 10 seconds before I saw your comment. Apologies. Well, the diff is here (GitHub adds "Compare" links to the PR discussion history for any force pushes which is why I usually force push for minor changes): As for checking if the example is correct, first go into VSCode preferences ( You can then see that the old flags are removed and the new flags are set. I spent about 2 hours going through PlatformIO's code (seeing how |
|
Here are some of the references I used (useful for history/PR documentation):
|
|
For what it's worth, my own opinion: That being said, the change is benign and only boils down to the question: Are we willing to deal with users complaining/asking why their custom builds fail out of the sudden when they pull from upstream since they followed instructions provided by WLED? If the answer is yes then I'd suggest to add a disclaimer that environments may change without notice and such change may render derived environment non-functional. |
|
@blazoncek It's already marked as an advanced method in the first line of the example. :) I agree 100% that there should be a clear warning in the example code which explains that the chosen upstream environment may change or become unmaintained in the future. I will add that! That "feature" is the entire reason for its existence though: Seeing a good release build (such as "ESP32_Ethernet") and tweaking a few WLED defaults for yourself. For most users, such a usage will not need any maintenance at all because they'll only be setting a few pin numbers, default ethernet board model, etc. The alternative is to copy-paste an entire definition and maintain it manually and relentlessly. That's a lot more work for the user and will still break identically when there's big compiler variable name/value changes in future WLED updates. So there's no improvement to be gained by doing the manual copy-paste method, in this use-case (taking a good release build and just tweaking a few flags). I just need to explain more clearly the upsides and downsides of this approach in the comments itself. |
What is the difference compared to extending existing core environment? You will still need to "maintain it manually and relentlessly". I understand that this approach may work for you (until we change |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
platformio_override.sample.ini (1)
562-570: Comprehensive Documentation forbuild_unflags
The detailed comments clearly explain how thebuild_unflagsdirective operates — emphasizing the need to specify EXACT values to unset and warning about potential duplicate definitions if upstream flags change. This level of documentation is helpful for advanced users. Consider including a brief note or reference link to PlatformIO’s documentation onbuild_unflagsfor additional clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
platformio_override.sample.ini(1 hunks)
🔇 Additional comments (3)
platformio_override.sample.ini (3)
556-560: Environment Inheritance: Valid Use ofextends
The new[env:esp32_eth_custom]environment correctly extendsenv:esp32_eth, ensuring that all upstream settings are inherited while allowing overrides. This helps reduce duplication and future maintenance by automatically incorporating upstream changes.
571-574: Accurate Unsetting of Conflicting Flags
The unflag directives correctly list the flags to be removed:
-D WLED_RELEASE_NAME="ESP32_Ethernet"-D RLYPIN=-1
Ensure that these values precisely match those defined inenv:esp32_ethso that only the conflicting instances are removed. This review action might be verified by comparing with the upstream settings during a verbose build.
575-584: Correct Overriding of Build Flags
The overriding of build flags is well structured. By inheriting all upstream flags via${env:esp32_eth.build_flags}and then appending new customizations:
-D WLED_RELEASE_NAME="ESP32_Ethernet_Custom"-D RLYPIN=24(indicating the new relay pin)-D WLED_ETH_DEFAULT=4-D DATA_PINS=16,3,1,4
the configuration provides clear and maintainable customizations. The inline comments further aid users in understanding the purpose of each flag. Verify these values against your hardware setup and intended customization via a verbose build process.
There's a huge difference in user maintenance burden. Here's the upstream release config that this technique is based on: [env:esp32_eth]
board = esp32-poe
platform = ${esp32.platform}
platform_packages = ${esp32.platform_packages}
upload_speed = 921600
custom_usermods = audioreactive
build_unflags = ${common.build_unflags}
build_flags = ${common.build_flags} ${esp32.build_flags} -D WLED_RELEASE_NAME=\"ESP32_Ethernet\" -D RLYPIN=-1 -D WLED_USE_ETHERNET -D BTNPIN=-1
; -D WLED_DISABLE_ESPNOW ;; ESP-NOW requires wifi, may crash with ethernet only
lib_deps = ${esp32.lib_deps}
board_build.partitions = ${esp32.default_partitions}The user can copy-paste that huge text blob and maintain it themselves, sure:
By instead deriving from your known-good release config, the user only has to maintain the exact flags they are modifying. That's great, as long as they are happy about being automatically derived from your release config and any future changes it gets - which is... the entire point of this technique. So I am definitely happy about being derived from
Me too. I have never proposed that this is a "general" suggestion for people. It's marked as ADVANCED in the first word of the first line of the example. I've also added a big, thick warning now to explain the user's own responsibilities if they use this technique. For those who know what this technique is for, it's a perfect technique. It automatically follows the WLED release settings and lets users tweak a handful of values they wanted to change, to set new "firmware flash" defaults for things like pin numbers, ethernet board (which is very useful to write into the firmware, to not have to manually connect to the WiFi to change the board model every time you flash an update via USB). And if WLED ever changes the settings in I have expanded the "add flags" example to clearly demonstrate the kind of flags users like me are setting with this technique. It's all harmless stuff like pin numbers and ethernet board model. Just minor default-tweaks for upstream releases. And with the new warning that explains that the user cannot complain to you if upstream defaults change in the future, I think it's ready for inclusion. :) |
Arcitec
force-pushed
the
advanced-pio-overrides
branch
from
3615b95 to
f0bcbaa
Compare
|
Thank you for raising this @Arcitec I agree that we want to make it easy for users to base their builds of a known good set of values, which may change and we don't want them to have to track these changes so extends is the right approach I need to take some time to review our current config as well as your pr so that the best path for the user is also the easy path |
|
@netmindz That's excellent to hear! If you have any concerns or questions, I'm here for you and ready to revise anything! :) The general approach requires all 3 parts, due to how PlatformIO is designed:
To verify that the correct flags are being applied by this technique, here's some instructions to do a verbose build to see all flags being applied onto each file by the compiler: Also remember to erase your buildcache to force rebuilds after each test. I think it defaulted to Hope this helps! Let me know if there's any questions. :) |
Adds a self-documented example demonstrating how a user can inherit from a core WLED configuration and add their own values without needing to copy all of the upstream values manually. This new method simplifies configuration maintenance for the users.
Summary by CodeRabbit