-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add DALI support - As a light #5645
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: main
Are you sure you want to change the base?
Changes from 3 commits
eaecca9
0fc601c
23a9360
8394038
6bc19e9
0d77a27
670a6fd
71767b7
badfcfc
2daf17a
af4b39e
d459e6c
e68bd2a
50cd900
fb0c1df
9b4eafa
3007564
83e32ea
818c3cb
76a505f
76b1e0a
133aecb
fb7e309
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move to platformio_override.ini.sample @coderabbitai why didn't you flag this file?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 4594
Two clear problems with this file:
✏️ Learnings added
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need a proper solution for how you build PRs The sample file isn't included in the CI builds
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, it's a recurring topic and we don't have a good solution. Even when you use the VSCode UI, we'd still need a second file for custom buildenvs that should be included in the CI builds for the PR. @coderabbitai please open an issue ticket to keep track of this point.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 41
It captures the core problem (no clean way to get custom PlatformIO build envs into CI for PRs), the impact, a few possible directions to explore, and backlinks to this PR and the related docs issue. 🐇📋 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| [env:esp32s3dev_8MB_opi_dali_gear] | ||
| ;; ESP32-S3 dev board (8MB Flash, QSPI PSRAM) with dali_gear usermod | ||
| extends = env:esp32s3dev_8MB_qspi | ||
| custom_usermods = dali_gear | ||
| lib_deps = ${env:esp32s3dev_8MB_qspi.lib_deps} | ||
| https://github.com/netmindz/DALI-Lighting-Interface.git#fix/esp32-volatile-cast | ||
| build_flags = ${common.build_flags} ${esp32s3.build_flags} -D WLED_RELEASE_NAME=\"ESP32-S3_dali_gear\" | ||
| -D WLED_WATCHDOG_TIMEOUT=0 | ||
| -D ARDUINO_USB_CDC_ON_BOOT=0 ;; use UART0 via built-in JTAG/serial debug unit (303a:1001) | ||
| -DBOARD_HAS_PSRAM | ||
| -D WLED_DEBUG |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| { | ||
| "name": "dali_gear", | ||
| "build": { "libArchive": false }, | ||
| "dependencies": { | ||
| "qqqDALI": "https://github.com/netmindz/DALI-Lighting-Interface.git#fix/esp32-volatile-cast" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,140 @@ | ||||||||||||||
| # DALI Gear Usermod | ||||||||||||||
|
|
||||||||||||||
| Makes WLED act as a **DALI control gear** (IEC 62386) — i.e. a light that responds to commands from an external DALI master (wall dimmer, BMS, building automation system, etc.). | ||||||||||||||
|
|
||||||||||||||
| DALI (Digital Addressable Lighting Interface) is a standardised two-wire bus protocol for lighting control. This usermod puts WLED on the bus as a gear device: the DALI master sends brightness/on/off/colour commands, and WLED adjusts its LEDs accordingly. | ||||||||||||||
|
|
||||||||||||||
| > **ESP32 only.** The hardware timer API used for Manchester decoding is not available on ESP8266. | ||||||||||||||
|
|
||||||||||||||
| ## Hardware | ||||||||||||||
|
|
||||||||||||||
| You need a DALI bus interface circuit to convert between the DALI bus voltage (9.5–22.5 V) and the ESP32's 3.3 V GPIO levels. | ||||||||||||||
|
|
||||||||||||||
| ### Minimal DIY circuit (from [qqqlab/DALI-Lighting-Interface](https://github.com/qqqlab/DALI-Lighting-Interface)) | ||||||||||||||
|
|
||||||||||||||
| ``` | ||||||||||||||
| 3.3V ESP32 5.6V ___ | ||||||||||||||
| Zener +----|___|---- 12V Power Supply | ||||||||||||||
| ___ Diode | 220 Ω | ||||||||||||||
| RX ---+-----|___|---|>|----------+------------- DALI+ | ||||||||||||||
| | 10K | | ||||||||||||||
| +-+ | | ||||||||||||||
| | | 100K ___ |/ PNP DALI BUS | ||||||||||||||
| +-+ TX ---|___|----| Transistor | ||||||||||||||
| | 1K |\ | ||||||||||||||
| | V | ||||||||||||||
| GND --+---------------------------+------------- DALI- | ||||||||||||||
| ``` | ||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add a language tag to the fenced code block. Line 15 triggers markdownlint MD040. Add a fence language (e.g., Proposed change-```
+```text
3.3V ESP32 5.6V ___
Zener +----|___|---- 12V Power Supply
___ Diode | 220 Ω
RX ---+-----|___|---|>|----------+------------- DALI+
...
GND --+---------------------------+------------- DALI-🧰 Tools🪛 markdownlint-cli2 (0.22.1)[warning] 15-15: Fenced code blocks should have a language specified (MD040, fenced-code-language) 🤖 Prompt for AI AgentsThere was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A PNP would pull the GPIO above 12v, and there would be no isolation. The Waveshare has an NPN and an Opto, so the output should be normally at 0 v, and go to 3.3v during xmit. https://files.waveshare.com/wiki/Pico-DALI2/Pico-DALI2.pdf. I added an inverter for testing - and this fw release is not responding on the GPIO I selected ( 17 ). It is adjusting the light from Rx I set to 15. Pins are shown used by usermod - but only after a reboot. I tested a E7 command and verified the 02 response. Can you invert the output?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: Length of output: 906 🏁 Script executed: Length of output: 5446 🏁 Script executed: Length of output: 2795
1. 🐛
|
||||||||||||||
| Command (cmd byte) | Name | Required response |
|---|---|---|
0x90 |
QUERY STATUS | status byte |
0x91 |
QUERY CONTROL GEAR PRESENT | 0xFF ("Yes") |
0xA0 |
QUERY ACTUAL LEVEL | current arc level byte |
Without 0xFF to 0x91, many masters consider the gear absent and never proceed to 0xF7. These need to be added to the handleCommand() switch block alongside the existing DT8 cases.
4. ⚠️ ASCII circuit diagram (PNP safety concern)
You are right — the diagram sourced from the upstream qqqlab README shows a PNP transistor wired in a way that would expose the ESP32 GPIO to bus-level voltage without isolation. The README should be updated to either replace that diagram with one matching the actual Waveshare Pico-DALI2 (NPN + opto-isolator) or add a prominent warning that the qqqlab diagram is conceptual and not safe to build as-is.
Would you like me to open a GitHub issue to track these items (correct command byte, TX inversion config, missing gear-presence responses, circuit diagram correction) for the PR author @netmindz?
🐇 🔧
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.
Hi @mff-atx - Are you using the Pico-Dali2 board or the DIY circuit?
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
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.
Using the Pico-DALI2, qqqlab DIY circuit has many flaws, see update on https://github.com/mff-atx/DALI-Lighting-Interface/tree/patch-1
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 understand this change is temporary for testing, please remember to remove it once the code is final.
@coderabbitai why didn't you flag this change?
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.
@softhack007, you're right to call this out — I should have flagged it and I apologize for the miss. This is a significant change that warrants a review comment.The issue: The diff replaces the entire CI/release
default_envslist (21 environments covering ESP8266, ESP32, ESP32-S2, ESP32-S3, etc.) with a single development-only environmentesp32s3dev_8MB_opi_dali_gear. If merged as-is, this would break the CI/release build pipeline for WLED completely.Beyond the "must revert before merge" point, the WLED project convention is that personal build overrides belong in
platformio_override.ini(which is.gitignored), not inplatformio.ini. A developer testing only the DALI gear environment should instead add to their localplatformio_override.ini:This keeps
platformio.iniclean and avoids accidentally committing a broken CI configuration.As for why I didn't flag it: the PR is a draft, and I likely de-prioritized what appeared to be an intentional build environment customization in the context of a new usermod. That was a mistake — changes to
default_envsinplatformio.inithat gut the standard release environments should always be flagged regardless of draft status, since they're easy to forget and would be catastrophic if merged. I'll be sure to catch this pattern in future reviews.Would you like me to open a follow-up reminder issue to track reverting this before the PR is finalized?
🧠 Learnings used
Uh oh!
There was an error while loading. Please reload this page.
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.
@coderabbitai please add a learning, to make sure that changes like this always get highlighted.
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.
@softhack007, noted — I've added the learning now!(¬‿¬)🐰
✏️ Learnings added
🧠 Learnings used