-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
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
Only declare lifx update failure after 3 attempts #90872
Conversation
These devices sometimes flakey and generate a lot of noise from drop outs since communication is UDP best-effort. We should only mark them unavailable if its not a momentary blip fixes #78876
Hey there @Djelibeybi, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
@@ -189,41 +192,54 @@ def async_get_entity_id(self, platform: Platform, key: str) -> str | None: | |||
async def _async_update_data(self) -> None: | |||
"""Fetch all device data from the api.""" | |||
async with self.lock: |
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 would remove this lock, btw. I added it in the previous integration's incarnation to prevent multiple discoveries from firing at the same time, but that's no longer an issue (or possible).
For reference, Photons uses a Semaphore instead of a lock to limit the number of packets "in-flight", i.e. awaiting a response, but that's not bulb specific. Photons is essentially just a very fancy way of getting the least amount of packets on the network to do the most amount of work across the largest number of devices.
# device.mac_addr is not the mac_address, its the serial number | ||
if self.device.mac_addr == TARGET_ANY: | ||
self.device.mac_addr = response.target_addr |
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.
Aside: we should move this into the callback of async_execute_lifx
so that it happens earlier.
The code as written never gets passed a single attempt before the update coordinator marks the update as failed (for me). I refactored it like so: lifx_raise_fallback...Djelibeybi:home-assistant-core:lifx_raise_fallback (which is NOT production ready because I'm using warning logs for debugging purposes). This refactor has not resulted in a single timeout at all. Not one in over 15 minutes of running. That's unprecedented with my fleet. Tests need to be updated, but they still all pass too. |
I'm happy to close this PR if you want to open another one. I'm working on release issues so it will be a while before I get back to this. |
Sure, that works. I'll fix up the tests shortly. |
I've just opened #90891 as a replacement for this one. |
closing in favor of #90891 |
Proposed change
These devices sometimes flakey and generate a lot of noise from drop outs since communication is UDP best-effort. We should only mark them unavailable if its not a momentary blip
fixes #78876
Type of change
Additional information
Checklist
black --fast 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
..coveragerc
.To help with the load of incoming pull requests: