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).
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughPlatformIO configuration refactored to centralize platform settings and monitor filters at the platform group level ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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. 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.)
|
@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).
| 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.)
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