Speed up TP-Link lights #33606
Speed up TP-Link lights #33606
Conversation
probot-home-assistant
bot
added
integration: tplink
small-pr
|
Hey there @rytilahti, mind taking a look at this pull request as its been labeled with a integration ( |
|
I don't have access to any test devices, but my understanding is that this should work. If that's the case, there is no need to create a diff on settings to change. Simply checking if the current settings differ from the wanted ones should suffice (i.e., |
|
I'll dig one up out of the garage later this week if nobody has one available to test. |
|
@bramkragten is going to test it. |
|
Had a chance to test this on 0.108 and found there is a problem before the change |
|
After the change |
|
Summary: Before: After: |
|
Your current error on 108 is weird, there is a guard for it: core/homeassistant/components/tplink/light.py Lines 313 to 314 in cf563df Pushed a fix for the 2nd. |
|
No more log errors, brightness works, color temp. Setting hue/sat is broken. Tried multiple colors. Nothing happens |
|
Maybe color temp overrides the hue/sat? That would explain why it is not working when temp is also given. Anyway, you should probably enable debug logging for pyHS100 and check the logs to see what sort of input & output you are getting. Here's how it looks like in raw responses from a device (ignore that this is a different repo, the payloads are the same): https://github.com/python-kasa/python-kasa/blob/master/kasa/tests/fixtures/KL120(US)_1.0_real.json#L17 |
Did that. Yikes. This thing is a firehose |
|
Every 5 seconds |
|
No wonder it has to retry all the time. Its being bombarded with api requests |
|
Also makes 24 sysinfo api requests each restart |
|
Yes, each property access of the bulb device ( core/homeassistant/components/tplink/light.py Line 311 in 79acba2 That's one of the drivers for the new backend lib and this PR (the backend lib will simply cache the accesses to make it invisible for upstream users): #30719 - alas, its progress is currently stalled due to some necessary changes to support multiplug switches to avoid dropping support for those devices until I (or someone who wants to contribute) finds time to figure it all out. |
Codecov Report
@@ Coverage Diff @@
## dev #33606 +/- ##
==========================================
- Coverage 94.76% 94.75% -0.02%
==========================================
Files 780 858 +78
Lines 56315 60839 +4524
==========================================
+ Hits 53368 57646 +4278
- Misses 2947 3193 +246
Continue to review full report at Codecov.
|
|
@bdraco thanks for the comments. I've now converted it to int before we even pass it to LightState. Same with making sure color temp is set correctly in requested light state. I think that the flaw here is that we create light state objects, but really only need to send the parameters we want to change. ie, to turn off a light we just need to call with However, I also don't want to dig too much into this integration as I have better things to do. Really just wanted to make controlling it a little faster. |
|
I've got it cleaned up a bit. No more timeout errors |
|
Also every turn on is causing another update |
|
I'm going to borrow a non-colored bulb for testing with. It won't be here till this later this week though so leaving |
I have 2 bulbs (1 colored and 1 dimmable) and a switch in order, should arrive this week. |
Then there must be something wrong in the UI not following the given limits, I suppose... Or some change affected how it is being handled. |
I don't think its setting the constraints properly in your other branch. Let's reserve this conversation for the new PR when you send it in. I'd prefer not to add unrelated comments here as its going to trigger notifications for everyone who has contributed to this. |
|
Fair enough, I'll not pollute this PR anymore, but the PR will still be #30719 . I will just re-open it and fix the open issues when I find some time for it. However, the logic behind those limits has not changed (see https://github.com/home-assistant/core/pull/30719/files#diff-9a900d2f83693dd3e01d2894bf1c6bffL266-L283 in |
|
Fixed lights that do not support color |
|
|
|
I'm at the limit of what I can test. Let's wait for @bramkragten to get the devices in to validate this. |
|
I just got the devices, will test tonight |
|
🎉 That's a nice speed improvement! Way more stable now. Let's merge this, and after that, we can look at the async version or improve this integration a little more (I noticed that we only discover devices at startup for instance) |
|
Uhh, so, just a personal opinion, but if you really want to keep adding more hacky optimizations, I hope there'll be some leniency when reviewing that PR in the future. My plan is to simply ignore all the changes that has happened up to this PR, as I think otherwise doing a rebase will be such a waste of time and effort (assuming git rebase won't automatically handle these changes without extra effort). Therefore I'm pleading that you avoid doing any more optimization before we can manage to finalize that port & cleanup PR, thanks.. To be honest, I feel quite saddened by the amount of effort (or lack thereof) what was put into handling this properly: that PR has been active since January and provides the proper fix (and some cleanups to get rid of unnecessary code) for this, but here we concentrate on hacking around it. |
I don't know the history here, but if you have solved the performance issue by converting to async (and adding the caching) it seems like you can dump most of the code in there and make this much simpler. |
* Speed up TP-Link lights * Color temp kan be None * hs as int, force color temp=0 * Fix color temp? * Additional tplink cleanups to reduce api calls * Update test to return state, remove Throttle * Fix state restore on off/on * Fix lights without hue/sat Co-authored-by: J. Nick Koston <[email protected]>
|
this change broke my dimmers: tplink: Error on device update! |
|
@garyalbert. What is the model number of the device with the issue? |
|
@bdraco HS220 dimmers |
Prime now had one in stock. I’ll fix it when it gets here later this afternoon |
|
Please open an issue if you suspect a bug. Merged PRs should not be used for support or bug reports. Thanks! |
|
@garyalbert Please open an issue here https://github.com/home-assistant/core/issues/new?template=BUG_REPORT.md and be sure to include your yaml config if any. Thank you! |
|
Here's the issue if someone comes looking for it: #33896 |
Breaking change
Proposed change
Speed up changing the state of TP-Link lights by making a single request instead of doing one request per parameter that we want to change.
This PR is untested, looking for testers with TP-Link. @rytilahti maybe?
Type of change
Example entry for
configuration.yaml:# Example configuration.yamlAdditional 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.The integration reached or maintains the following Integration Quality Scale: