Refactor LIFX discovery to make it faster and more reliable

[Djelibeybi]

Signed-off-by: Avi Miller me@dje.li

Breaking change

This PR changes the network adapters used to discover LIFX devices.
Instead of enabling all adapters on which LIFX devices appear, this
PR switches to only using the adapter(s) enabled in Home Assistant.

Proposed change

Currently there is a race condition that occurs when bulbs respond
multiple times to the discovery broadcast and the integration tries
to register the same bulb that's already mid-registering.

This PR adds an in-flight discovery throttle that only allows one
direct connection per bulb at a time to avoid that race condition.

This is faster because there is a limit to how many packets a bulb
can process per second. So, the fewer packets we send, the faster
the bulb can process the ones it receives.

It's more reliable because there are less packets on the network,
less broadcasts over wifi and thus less overall traffic. This allows
us to reduce the amount of message retries as well.

Finally, it eliminates the "does not generate unique ID" error that
was caused by multiple discoveries trying to register the same entity
at the same time but out of order.

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)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes Platform lifx does not generate unique IDs #69168
• This PR is related to issue:
• Link to documentation pull request:

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
• The code has been formatted using Black (black --fast homeassistant tests)
• Tests have been added to verify that the new code works.

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

• Documentation added/updated for www.home-assistant.io

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.
• Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

• No score or internal
• 🥈 Silver
• 🥇 Gold
• 🏆 Platinum

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[Djelibeybi]

I also took the opportunity to reword the various debug log statements so that the connectivity lifecycle is more obvious if you have debug logs enabled. This includes changing the entity.who property to use the MAC address instead of the IP address for identification as IP addresses change, but MAC addresses don't.

[Djelibeybi]

This may resolve #68943 as well.

[Djelibeybi]

I've also added additional LIFX models to the homekit discovery list. The model names were provided by the nice folks over at LIFX who are literally and figuratively keeping the lights on right now.

[amelchio]

Thank you for picking up the LIFX integration. Have you considered signing up as code owner?

[Djelibeybi]

Let me think about that for a bit. Is there anywhere I can read up about what's involved and what the expectations are from the maintainers?

[Djelibeybi]

@amelchio @bdraco here are the logs that show the huge improvement between the current and updated startup reliability and time for my ~60 LIFX bulb install. I think we should merge this so that we improve things immediately and I'll keep working on it to streamline and optimise it too.
home-assistant-current.log
home-assistant-updated.log

[Djelibeybi]

Have you considered signing up as code owner?

Would that allow me to replace aiolifx with something else that may or may not be Photons?

[bdraco]

Thank you for your contribution thus far! 🎖 Since this is a significant contribution, we would appreciate you'd added yourself to the list of code owners for this integration. ❤️

Please, add your GitHub username to the manifest.json of this integration.

For more information about "code owners", see: Architecture Decision Record 0008: Code owners.

[Djelibeybi]

Please, add your GitHub username to the manifest.json of this integration.

Done.

[bdraco]

The changes look good but I'm traveling until Wednesday so I can't test them with my LIFX devices

[Djelibeybi]

The changes look good but I'm traveling until Wednesday so I can't test them with my LIFX devices

This has been an issue for a while now, so a few more days isn't going to make that much difference. :)

[bdraco]

Ideally we merge this before beta cutoff (sometime before Wed morning) so we can get feedback in beta.

We can always revert if it is a problem.

@Djelibeybi Would you please move the manifest changes into a new PR so in the rare event we do have to revert it, we won't revert them as well.

[Djelibeybi]

@bdraco Yep, I can do that.

[Djelibeybi]

Manifest changes spun out into #70614

[bdraco]

I ordered a few more LIFX devices and a switch so I'll be able to test on Wednesday when I get back to Texas

[Djelibeybi]

@bdraco I've officially asked the LIFX team to update the public protocol with the new switch messages: LIFX/public-protocol#6 and I've been talking to folks at LIFX about this directly too. I also have an alternative integration that uses Photons instead of aiolifx that I've been working on for a while now, mostly as a learning exercise to get my head around mapping Home Assistant's opinions with Photon's opinions.

[bdraco]

I'll check back on this tomorrow as I want to make sure Anders has a chance to look at it before hitting merge.

[amelchio]

Would that allow me to replace aiolifx with something else that may or may not be Photons?

Yeah. Well, it's almost the other way around: doing a big change like that requires you to sign up as code owner to make sure you are notified of the inevitable fallout.

As a code owner, it's somewhat expected that you track new issues. But you're a volunteer and nobody will hold it against you if you cannot donate enough time to resolve everything.

There is no actual rule but in practise being a code owner also means that you get priority in reviews since people will assume that a) you know what you are doing and b) you will be around to fix up any mistakes.

[Djelibeybi]

@bdraco @amelchio I'd appreciate if you could sanity check the code I borrowed from the yeelight integration to detect active interfaces is appropriately used here. It works for me and boy, is discovery even faster with only a single active interface. Thanks for the pointers!

[amelchio]

I have a couple more comments, just no time to make them right now. But we'll get this into the beta, don't worry about that.

[amelchio]

This looks good to me now. Just a few minor comments that you can consider but I am not insisting on any change.

I think you should remove the second paragraph of the Breaking Change note though, it is a bit confusing and contradictory (anything vs. unlikely/most) to me and I don't think it adds much anyway.

[amelchio]

@bdraco Are we good to go here or did you want to do tests with your bulbs first?

[bdraco]

I'm traveling and will test it when I get back home tomorrow. Let's merge this now so we get feedback in beta.

I'll ping @Djelibeybi tomorrow if I find anything needs to be adjusted.

[bdraco]

Tested with my devices. Works great. No longer missing devices when I remove and readd the config entry