refactor platformio.ini to eliminate copy-paste duplication#5544
refactor platformio.ini to eliminate copy-paste duplication#5544
Conversation
Centralise platform, platform_packages, build_unflags, lib_deps and monitor_filters into the per-chipset abstract sections ([esp8266], [esp32], [esp32s2], [esp32c3], [esp32s3]) and have concrete envs inherit via extends rather than repeating the same values. - [esp8266]: add platform, platform_packages, monitor_filters - [esp32]: add monitor_filters - [esp32s2]: add monitor_filters - [esp32c3]: add monitor_filters - [esp32s3]: add upload_speed, monitor_filters - esp8266 envs (nodemcuv2, esp8266_2m, esp01_1m_full): extends = esp8266 - esp32dev: extends = esp32 - esp32dev_8M, esp32dev_16M: extends = env:esp32dev - esp32_eth, usermods: extends = esp32 / env:esp32dev - esp32_wrover: drop redundant build_unflags, lib_deps (already extends esp32_idf_V4) - esp32c3dev: drop redundant platform, platform_packages, framework, build_unflags, lib_deps, board_build.partitions (already extends esp32c3) - esp32s3 envs: extends = esp32s3 - lolin_s2_mini: extends = esp32s2 No functional changes. Verified with pio run across all five chipset families (ESP8266, ESP32, C3, S3, S2).
WalkthroughCentralizes monitor_filters and shared platform settings into platform groups ( ChangesPlatformIO configuration refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
| lib_deps = | ||
| ${esp32_idf_V4.lib_deps} | ||
| board_build.partitions = ${esp32.large_partitions} ;; default partioning for 8MB flash - can be overridden in build envs | ||
| upload_speed = 921600 |
There was a problem hiding this comment.
maybe comment out the upload speed flag. 920kbaud might be too much for boards where the UART-to-USB module gets connected by flimsy dupond wires only.
There was a problem hiding this comment.
My $0.02, but I'd prefer to default to the higher speed -- it's much more convenient and failures are easily caught. If a user is doing a custom build and needs to lower the speed, it's straightforward to do in the override file. Try the fast speed first and lower if you have to, instead of the other way around.
(AFAIK these settings don't affect users using the web installer or the like -- just people doing custom builds.)
There was a problem hiding this comment.
What happens if you have no value?
|
@netmindz I generally like the idea to use "extends = " for reducing the number of copied entries in buildenvs. Actually, I've proposed a similar approach some time ago, but then there was user feedback that people prefer to see "full entries", and "extends = " also created new pitfalls and makes it harder to understand which flags are actually passed to the compiler. Not sure if this has changed in the meantime? |
| platform = ${common.platform_wled_default} | ||
| platform_packages = ${common.platform_packages} |
There was a problem hiding this comment.
We should move all the ESP8266 platform definitions in to this section instead of common, since they're not common for all environments. Yes, this will potentially be a breaking change for user platformio_override.ini files for anyone building custom ESP8266 environments; I think that's reasonable for 17.0, to keep our definitions clean (and prepare for a proper HAL).
There was a problem hiding this comment.
now is the time to break things, at the very start of the development cycle
| lib_deps = | ||
| ${esp32_idf_V4.lib_deps} | ||
| board_build.partitions = ${esp32.large_partitions} ;; default partioning for 8MB flash - can be overridden in build envs | ||
| upload_speed = 921600 |
There was a problem hiding this comment.
My $0.02, but I'd prefer to default to the higher speed -- it's much more convenient and failures are easily caught. If a user is doing a custom build and needs to lower the speed, it's straightforward to do in the override file. Try the fast speed first and lower if you have to, instead of the other way around.
(AFAIK these settings don't affect users using the web installer or the like -- just people doing custom builds.)
I think it's fine so long as we keep it to just one (2 at max) levels of extends. Most things defined at the chipset level, the env is then only setting things like the |
Per review feedback: platform_wled_default, platform_packages, and the
arduino_core_* aliases are ESP8266-specific and don't belong in [common].
Note: this is a breaking change for custom platformio_override.ini files
that reference ${common.platform_wled_default} or ${common.platform_packages};
those should be updated to use the ${esp8266.*} prefix.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
platformio.ini (1)
160-651:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMaintainer approval is required before merge for this file.
This
platformio.inirefactor looks consistent, but it must receive explicit maintainer/WLED-org approval before merging (global build config gate).As per coding guidelines: “Modifications to
platformio.inimust be explicitly approved by a maintainer or WLED organization member” and “Changes to platformio.ini require maintainer approval”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platformio.ini` around lines 160 - 651, This change modifies global build configuration in platformio.ini (sections like [esp8266], platform_wled_default, build_flags, and various [env:*] entries) and therefore requires explicit maintainer/WLED-org approval before merging; get a named maintainer to review and add an explicit approval comment or GitHub review approval on the PR, update the PR description to call out "platformio.ini changes require maintainer approval" and reference the maintainer's sign-off, and do not merge until that approval is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@platformio.ini`:
- Around line 160-651: This change modifies global build configuration in
platformio.ini (sections like [esp8266], platform_wled_default, build_flags, and
various [env:*] entries) and therefore requires explicit maintainer/WLED-org
approval before merging; get a named maintainer to review and add an explicit
approval comment or GitHub review approval on the PR, update the PR description
to call out "platformio.ini changes require maintainer approval" and reference
the maintainer's sign-off, and do not merge until that approval is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b74fa100-3d15-43da-b2ec-b9b042a39063
📒 Files selected for processing (2)
platformio.iniplatformio_override.sample.ini
3 participants
Centralise platform, platform_packages, build_unflags, lib_deps and monitor_filters into the per-chipset abstract sections ([esp8266], [esp32], [esp32s2], [esp32c3], [esp32s3]) and have concrete envs inherit via extends rather than repeating the same values.
No functional changes. Verified with pio run across all five chipset families (ESP8266, ESP32, C3, S3, S2).
Summary by CodeRabbit
Refactor
Chores