Normalize unique ID in WLED#157901
Conversation
|
Hey there @frenck, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
There was a problem hiding this comment.
Pull request overview
This PR normalizes MAC addresses (unique IDs) in the WLED integration to lowercase format to fix issues where some users had uppercase MAC addresses stored in their configuration entries. The change includes duplicate detection, user-facing repair issues, and normalization in both existing entries and new config flows.
Key Changes
- Added MAC address normalization function to ensure consistent lowercase format
- Implemented duplicate entry detection with user-facing repair issues
- Applied normalization throughout config flow and coordinator initialization
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
homeassistant/components/wled/__init__.py |
Added duplicate detection logic, repair issue creation, and unique ID normalization for existing entries |
homeassistant/components/wled/coordinator.py |
Added normalize_mac_address() function and applied normalization when comparing device MAC to config entry |
homeassistant/components/wled/config_flow.py |
Applied MAC normalization in user and zeroconf config flows |
homeassistant/components/wled/strings.json |
Added repair issue descriptions for duplicate config entries |
tests/components/wled/conftest.py |
Added title to mock config entry fixture for testing |
tests/components/wled/test_init.py |
Added tests for duplicate detection and normalization scenarios |
tests/components/wled/test_config_flow.py |
Added parameterized tests to verify normalization in config flows |
deviantintegral
left a comment
deviantintegral
left a comment
There was a problem hiding this comment.
I left a few notes. Otherwise, I can test this live sometime tomorrow. Thanks for the quick PR!
| }, | ||
| "issues": { | ||
| "config_entry_unique_id_collision": { | ||
| "description": "There are multiple WLED config entries with the same device MAC addres.\nThe config entries are named {titles}.\n\nTo fix this error, [configure the integration]({configure_url}) and remove all except one of the duplicates.\n\nNote: Another group of duplicates may be revealed after removing these duplicates.", |
There was a problem hiding this comment.
Should this text remind users to look for disabled or ignored integrations too? Since they don't show up in the list at the integration level. I only figured it out because I knew enough to splunk through the underlying JSON.
There was a problem hiding this comment.
We can skip completely ignored entries. If the user ignore them, we should too.
This means I didn't test the repair workflow, but I'd say the PR as is fixes the issues I encountered well. Thanks! |
joostlek
left a comment
joostlek
left a comment
There was a problem hiding this comment.
I would say let's bump the minor version of the config entry and implement such thing in the migration of the entry.
Do we have any reason to believe people have duplicate entries? What about entity unique ids?
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
I thought about it, but it wouldn't allow us to handle conflicts because the migration can only be run once. Here this code needs to be run multiple times until the user fixes all the conflicts. I could just set a unique ID without checking for duplicates, but that will stop working in 2025.11. core/homeassistant/config_entries.py Line 2799 in d2c3543
Yes. Users in the discussion reported that they had duplicates because Zeroconf/DHCP discovery was likely creating entries with the wrong MAC address. and they created their entries manually and had duplicates, but that's just speculation, as I haven't been able to confirm this with certainty. I only saw that WLED always returned lowercase letters, and Discovery is another source for the MAC address, and I saw that it wasn't normalized. Here is normalization in WLED: An alternative is to completely drop the unique ID repair, because when we compare the MAC address, we normalize it anyway. It's not a big deal if someone created a config entry and has the unique ID in the wrong format, because now the integration will still work.
This shouldn't cause a problem as they are always set based on the values from the WLED API. core/homeassistant/components/wled/sensor.py Line 157 in 845c9ee |
| if len(duplicate_entries) - len(ignored_entries) > 1: | ||
| _LOGGER.warning( | ||
| "Found multiple WLED config entries with the same MAC address, cannot migrate to version 1.2" | ||
| ) | ||
| return False |
There was a problem hiding this comment.
What would the other ones be? mDNS? Maybe disabled? Which ones are ones we don't know what to do with?
There was a problem hiding this comment.
The problem is with those entries that were created with an incorrect unique ID, e.g. AABBCCDDEEFF. The WLED API always returns a lowercase value, so I don't know where the invalid entries came from or how many there are. My guess is that if someone managed to get two non-ignored entries to work, one will likely have no entities, because entities always assign a unique ID based on the API data. This seems to me to be a very special edge case and it won't happen in practice now.
|
Any ETA on when this will go out? I've had LED strips that I haven't been able to control for the past 4 days since this issue appeared. |
|
@altShiftDev Does deleting and re-adding the device solve the problem? Can you verify this? We don't have an ETA, but we're working hard on it. |
It likely would but it would also remove those entities from all scenes and automations. That's quite disruptive and I'd like to avoid doing so if this fix is around the corner. I can confirm that the right-click menu "reconfigure" does unfortunately not "reconfigure" anything but the IP address and gets tripped up at the mac address comparison. It would be extremely helpful to allow both IP and Mac address reconfiguring to avoid these lockouts in the future. I see no reason this can't be exposed to the user. 
|
Allowing users to edit the MAC address isn’t a good idea because the MAC is used as the It would also recreate the exact problems previous PR was designed to prevent. The original bug was that HA could accidentally associate a WLED entry with the wrong physical device when only the host/IP changed. This sometimes caused a single device to be linked to multiple config entries or made entities appear under the wrong controller. That is why explicit MAC verification was added in the first place: Letting the MAC be user-editable would effectively disable that protection and open the door to the same class of issues again. |
|
Respectfully I disagree. Allowing the power users to edit the MAC is a better idea than doing a blind string comparison without first sanitizing your inputs. It also allows users to fix their own issues instead of waiting on hotfixes when things break again in the future. Allowing explicit MAC verification is fine, enforcing it without bulletproof sanitizing of formats like uppercase/lowercase with and without colons is not fine.
There's no reason why allowing user-editable MAC addresses would recreate the past bug you're describing. I'm not saying to rollback the past protections, I'm asking you to allow power users to fix their own issues when things go wrong. |
|
If you need the change sooner, you can override the integration locally. Home Assistant supports loading a core integration as a custom component:
Alternatively, you can stay on the previous Home Assistant version that didn’t yet include MAC validation and wait until the fix is released. Both approaches let you continue using the setup you prefer while the upstream review runs its course. |
This folder doesn't exist, not sure if it's because I'm on the docker version or something else. It would be great if you accelerated this PR given it's broken the HA wled functionality for at least a few users. |
|
Here is folder with WLED component: https://github.com/home-assistant/core/tree/dev/homeassistant/components/wled For HAOS, the /config directory is accessible from Visual Studio Server or Terminal. For plain docker, the /config directory is mounted in user-controlled volume I'm doing my best to finish this PR, but I have to wait for the reviewers, who also have other challenges and have to divide their time between other components. |
Breaking change
Proposed change
Close: #157879
We recently started using unique IDs to verify that we were connecting to the correct device. However, it turned out that some users had their unique IDs set to uppercase. I'm not sure how this could have happened, as WLED always returns lowercase letters, but this prevented them from using this integration.
This PR normalizes WLED MAC addresses to lowercase to fix the issue. It adds a migration from version 1.1 to 1.2 that normalizes existing unique IDs and handles duplicate entries during the migration process.
Key changes:
Type of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: