Skip to content
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

Fix unique identifiers where multiple IKEA Tradfri gateways are in use #136060

Merged
merged 28 commits into from
Mar 3, 2025

Conversation

cs12ag
Copy link
Contributor

@cs12ag cs12ag commented Jan 20, 2025

Resolving issue #134497

Proposed change

This PR fixes a bug whereby users with multiple Tradfri gateways configured in the Tradfri integration will see inconsistent / randomly changing lists of devices associated to those gateways in Home Assistant.

This PR also introduces a migration function within the init.py file, to correct installations where multiple gateways have been configured in the Tradfri integration. The migration script does two things:

  • removes erroneously-added config-entry references from the associated device's 'config_entries' list; and
    • (this happens as a result of erroneously matching a device on gateway A using identifiers derived from gateway B using the current version of entity.py)
  • updates the 'identifiers' of existing Tradfri devices in the device registry to be genuinely unique within the device registry, so that devices with clashing identifiers can be added as expected when initialising the integration (rather than erroneously updating an existing device).

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New integration (thank you!)
  • New feature (which adds functionality to an existing integration)
  • Deprecation (breaking change to happen in the future)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Additional information

Checklist

  • The code change is tested and works locally.
  • Local tests pass. Your PR cannot be merged unless tests pass
  • There is no commented out code in this PR.
  • I have followed the development checklist
  • I have followed the perfect PR recommendations
  • The code has been formatted using Ruff (ruff format homeassistant tests)
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

If the code communicates with devices, web services, or third-party tools:

  • The manifest file has all fields filled out correctly.
    Updated and included derived files by running: python3 -m script.hassfest.
  • New or updated dependencies have been added to requirements_all.txt.
    Updated by running python3 -m script.gen_requirements_all.
  • For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

cs12ag added 5 commits January 2, 2025 22:18
Added migration function to execute upon initialisation, to:
a) remove the erroneously-added config)_entry added to the device (gateway B gets added as a config_entry to a device associated to gateway A), and
b) swap out the non-unique identifiers for genuinely unique identifiers.
…itly executing migrate_entity_unique_ids() from __init__.py)
Copy link
Member

@joostlek joostlek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So wait, what do you mean the problem is? Say I have 2 bridges. can the devices be used using both bridges (as in like a mesh?)?

Or are the device_ids just single numbers or so, first one gets 0, second 1, etc

@cs12ag
Copy link
Contributor Author

cs12ag commented Feb 5, 2025

@joostlek You can't have a single device paired to more than one gateway. The problem occurs in this HA integration when you try to use 2 or more Tradfri gateways, because of a combination of the way this integration was written and the way the Tradfri gateway firmware works.

In my specific example, the two gateways I've got have multiple devices associated to each of them. The pytradfri (not 100% name is correct) library responsible for polling the gateways simply responds with the list of unique identifiers that the gateway has assigned to each of its paired devices. My assumption is that because the gateways are the same hardware and firmware versions, they are using the same starting-point in the sequence used to assign unique IDs to devices as each device is paired to the gateway. Then, because of the code-issue seen (that this PR attempts to resolve), the devices are added to the device-library using the gateway's unique identifier with no other context. This has led to a situation whereby multiple devices in the device-library have identical identifiers, despite technically being different devices associated to different gateways. I have discovered that the devices will display in HA as the device associated to whichever gateway in the Tradfri integration that was most-recently (re)loaded.

Hopefully this example demonstrates the problem:
Gateway 1 has lamp with ID '1' paired to it.
Gateway 2 has switch with ID '1' paired to it.
There will be two separate devices in the device-library with the set of identifiers ['Tradfri','1'] on them. If I view the list of devices in this integration in the HA UI -
If I loaded gateway 1 most-recently in the integration, I'll only see the lamp device
If I loaded gateway 2 most-recently in the integration, I'll only see the switch device
I should be seeing both of the devices in the device list for this integration.

My guess is that when the original version of this integration was written, it was not tested with multiple gateways. My resolution is to add each gateway's own unique ID into the identifiers in the device-library, like ['Tradfri','gateway1-1']. The change corrects this going forward but I also wrote a migration method so that identifiers of all existing devices associated to the Tradfri integration were corrected and made unique, without having to delete the integration and its devices and start over.

Edit: realised I never at any point thanked you for taking a look, so - thanks! 👍

@home-assistant
Copy link

home-assistant bot commented Feb 5, 2025

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

@home-assistant home-assistant bot marked this pull request as draft February 5, 2025 17:10
@cs12ag
Copy link
Contributor Author

cs12ag commented Feb 5, 2025

To set expectations, I'll make changes over the next few days.

@cs12ag
Copy link
Contributor Author

cs12ag commented Feb 10, 2025

So I've attempted to upgrade the pytradfri dependency as part of this fix, and run into a bunch of issues as a result. I suspect there might be an issue with the pytradfri library: home-assistant-libs/pytradfri#986
I'll wait until I hear back on that issue before deciding how to continue here.

@cs12ag
Copy link
Contributor Author

cs12ag commented Feb 16, 2025

For this PR, the pytradfri dependency was restored to 9.0.1. There will be a forthcoming PR to change the tradfri component to support the latest version of pytradfri, as per home-assistant-libs/pytradfri#986

@cs12ag cs12ag marked this pull request as ready for review February 16, 2025 15:21
@home-assistant home-assistant bot requested a review from joostlek February 16, 2025 15:21
@home-assistant home-assistant bot marked this pull request as draft February 16, 2025 16:18
@MartinHjelmare MartinHjelmare added this to the 2025.3.0 milestone Mar 2, 2025
@MartinHjelmare MartinHjelmare changed the title Create unique identifiers where multiple IKEA Tradfri gateways are in use Fix unique identifiers where multiple IKEA Tradfri gateways are in use Mar 2, 2025
@MartinHjelmare MartinHjelmare marked this pull request as draft March 2, 2025 21:55
@MartinHjelmare MartinHjelmare self-assigned this Mar 2, 2025
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I noticed we didn't have coverage for some lines.

@MartinHjelmare MartinHjelmare removed this from the 2025.3.0 milestone Mar 2, 2025
@cs12ag cs12ag marked this pull request as ready for review March 2, 2025 23:11
@home-assistant home-assistant bot requested a review from MartinHjelmare March 2, 2025 23:11
@MartinHjelmare MartinHjelmare marked this pull request as draft March 2, 2025 23:18
@cs12ag cs12ag marked this pull request as ready for review March 3, 2025 09:49
@home-assistant home-assistant bot requested a review from MartinHjelmare March 3, 2025 09:49
@MartinHjelmare MartinHjelmare marked this pull request as draft March 3, 2025 12:15
@cs12ag cs12ag marked this pull request as ready for review March 3, 2025 12:45
@home-assistant home-assistant bot requested a review from MartinHjelmare March 3, 2025 12:45
Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@MartinHjelmare MartinHjelmare added this to the 2025.3.0 milestone Mar 3, 2025
@MartinHjelmare MartinHjelmare merged commit ee486c2 into home-assistant:dev Mar 3, 2025
32 checks passed
@cs12ag
Copy link
Contributor Author

cs12ag commented Mar 4, 2025

Thanks for your help on getting this to completion @MartinHjelmare 👍

@MartinHjelmare
Copy link
Member

Great work! 👍

frenck pushed a commit that referenced this pull request Mar 4, 2025
#136060)

* Create unique identifiers where multiple gateways are in use

Resolving issue #134497

* Added migration function to __init__.py

Added migration function to execute upon initialisation, to:
a) remove the erroneously-added config)_entry added to the device (gateway B gets added as a config_entry to a device associated to gateway A), and
b) swap out the non-unique identifiers for genuinely unique identifiers.

* Added tests to simulate migration from bad data scenario (i.e. explicitly executing migrate_entity_unique_ids() from __init__.py)

* Ammendments suggested in first review

* Changes after second review

* Rewrite of test_migrate_config_entry_and_identifiers after feedback

* Converted migrate function into major version, updated tests

* Finalised variable naming convention per feedback, added test to validate config entry migrated to v2

* Hopefully final changes for cosmetic / comment stucture

* Further code-coverage in test_migrate_config_entry_and_identifiers()

* Minor test corrections

* Added test for non-tradfri identifiers
@github-actions github-actions bot locked and limited conversation to collaborators Mar 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

IKEA TRADFRI integration appears unable to handle multiple gateways
4 participants