-
-
Notifications
You must be signed in to change notification settings - Fork 111
Enable builds with DMX Output support #260
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
base: mdev
Are you sure you want to change the base?
Conversation
DMX in/out
WalkthroughAdded platformio_override.ini defining a new PlatformIO environment [env:esp32dev_v4_dmx] that extends env:esp32dev and appends build_flags to set a custom release name and enable DMX support. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (3)
platformio_override.ini (3)
6-6
: Prefer simpler, portable quoting for WLED_RELEASE_NAME.Using single quotes around the string literal avoids escape noise and is the common pattern in PlatformIO files.
Apply this diff:
- -DWLED_RELEASE_NAME=\"mmwled-dmx\" + -D WLED_RELEASE_NAME='"mmwled-dmx"'
2-7
: Style consistency for -D usage and release artifact naming collision check.
- Minor: you mix
-DNAME=...
and-D NAME
. Both are valid; consider standardizing for consistency across envs.- If CI publishes OTA artifacts keyed by release name, ensure
"mmwled-dmx"
won’t collide with other envs (e.g., add board/flash hints if needed).If you want, I can align -D usage to your repo’s prevailing style; let me know which form you prefer.
2-3
: Extend the appropriate V4 base environmentThe project already defines a V4 variant (
esp32dev_V4_dio80
) with the proper flash/partition settings for your board. Since you’ve named your overrideesp32dev_v4_dmx
, you’ll likely want to inherit from that instead of the genericesp32dev
. Unless you really do intend to start from the stock settings, please update your override:• File: platformio_override.ini
• Section:[env:esp32dev_v4_dmx]
[env:esp32dev_v4_dmx] -extends = env:esp32dev +extends = env:esp32dev_V4_dio80If, however,
esp32dev
is indeed the correct base for your DMX variant, add a brief comment clarifying that choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
platformio_override.ini
(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-02T23:22:57.175Z
Learnt from: netmindz
PR: MoonModules/WLED-MM#248
File: platformio.ini:1613-1613
Timestamp: 2025-07-02T23:22:57.175Z
Learning: In WLED platformio.ini, the particle system disable flags (WLED_DISABLE_PARTICLESYSTEM1D and WLED_DISABLE_PARTICLESYSTEM2D) are intentionally varied across different build environments based on platform memory constraints. More memory-limited platforms (like ESP8266, ESP32-C3, ESP32-S2) disable both 1D and 2D particle systems, while platforms with more available memory (like esp32_4MB_V4_S with 4MB flash) may only disable the 1D version to preserve flash space while keeping 2D functionality available.
Applied to files:
platformio_override.ini
🔇 Additional comments (3)
platformio_override.ini (3)
4-5
: Good reuse of common flags.Inheriting
${common.build_flags}
keeps this env aligned with global defaults and reduces duplication. LGTM.
2-7
: Memory/feature balance sanity check (particles vs DMX).Enabling DMX may increase flash/IRAM usage. Per prior decisions in this repo, particle systems are selectively disabled on memory-limited targets. Verify that extending
env:esp32dev
plus enabling DMX does not push you over limits; if close, consider mirroring the particle system disable flags used for similar ESP32 variants.I can scan your base envs and suggest a matching set of feature flags if you confirm which board/flash size this env targets.
7-7
: DMX support libraries are already bundled and declared
TheWLED_ENABLE_DMX
flag pulls in the built-in DMX backends undersrc/dependencies/dmx/
(ESPDMX for ESP8266 and SparkFunDMX for ESP32). No externallib_deps
entry is required for DMX output. The DMX-input library is also declared viaDMXin_lib_deps = https://github.com/someweisguy/esp_dmx.git#47db25d
in yourplatformio_override.ini
entry.• Verified that all
#ifdef WLED_ENABLE_DMX
usages compile against the onboard DMX code.
• DMX-input support is pulled in byDMXin_lib_deps
.No further changes needed.
I haven't used DMX Output, so I do not know if there are any downsides to enabling DMX Outout in a build, if we were to offer this as a standard feature, we would probably also want to enable for S3 as well as classic ESP32 |
@@ -0,0 +1,7 @@ | |||
; === platformio_override.ini === | |||
[env:esp32dev_v4_dmx] | |||
extends = env:esp32dev |
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.
That is not one on the envs used for WLED-MM releases, also the platformio_override.ini is for local overrides, your change would need to be in the main platformio.ini to be part of the normal builds
@netmindz I'm wondering why this is not working? AFAIK (ok i was offline for many months) we have special MM configs for building with DMX out support. Is none of these working any more? |
We include dmx input, but not dmx output |
DMX in/out
Summary by CodeRabbit