search

jeffschubert / homebridge-daikin-oneplus

Homebridge plugin to control a Daikin One+ thermostat
Apache License 2.0
18 stars 7 forks source link

Home app shows old value after change #2

Closed jeffschubert closed 2 years ago

jeffschubert commented 2 years ago

Describe The Bug: After changing the target temperature or other values via the Home app, it may show the old value for a few seconds before updating to the expected value.

To Reproduce: Go to the thermostat in the Home app. Change the target temperature. Note that it may show the new temperature momentarily but then reverts to the old temp for several seconds before finally changing to the new temperature for good.

Expected behavior: Always show the new value after changing it.

FabianFrank commented 2 years ago

This is because _devices in DaikinApi acts as a cache that is updated in the direction thermostat -> homebridge once every 10 seconds by the getData() method. To update the temperature in response to a user input in Homekit the method setTargetTemp updates the thermostat by calling the Daikin REST API. However, it doesn't update the _devices data structure. So for up to 10 seconds (until whenever the next getData() call comes around) calls to getTargetTemp, which reads from _devices without calling the Daikin API, will return the old (and now wrong) value. An easy way to fix this would be to update _devices when the set... methods are called. However, I think there might be more to this and a strategy where the Daikin API is called less frequent seems desirable. What about updating _devices for the requested device on demand in the get... methods if the data is older than 5s? For background updates (I guess automations that wait for temperature changes etc might need this?) it could then update all thermostats only once per minute. The reason I bring this up is that I've noticed that this plugin causes quite a lot of network traffic, 500-100MB per hour, with 2 thermostats. What do you think?

jeffschubert commented 2 years ago

@FabianFrank thanks for the input and ideas. I do need to rework the HomeKit<->Thermostat syncing. My intention was for it to only pull the current state from the thermostat/Api on the refresh interval (10s by default). So if it is actually doing it every second, that's definitely not desirable. The payload the Api returns is a json object with nearly 1000+ properties, so it's not insignificant bandwidth to sync up from the thermostat. I've been meaning to try and sniff the thermostat traffic to see if it uses a more efficient method of talking with the Api.

FabianFrank commented 2 years ago

Sorry 1s was a typo earlier, I've corrected it to 10s. The JSON payload that Daikin returns for each thermostat is indeed pretty large.

FabianFrank commented 2 years ago

So I looked around a bit. I'm able to run their Daikin One Home iOS app on my M1 mac and just from measuring its network consumption with Activity Monitor it is roughly where I'd expect it to be if it used the same API, i.e. if I trigger a refresh in the UI it consumes close to the same amount of data as if I make 2 /deviceData/requests by hand. The iOS app fails to connect if you try to run it through a debugging proxy like Charles using a self-signed cert, so unfortunately I didn't see the actual traffic. That said I poked around in the Android app (fabian@pagefault.de if you want to chat details) and I also only found the same call to /deviceData/ that returns a lot of JSON. Frankly, the app is pretty sluggish so it could really be just fetching everything at once. This is just after running it a few minutes in the background: image

As for the actual problem of stale/old values being returned/displayed in the Home app, I think I know what's going on now. When I call POST /deviceData/:deviceId followed right after by GET /deviceData/:deviceId the change is not yet reflected in the GET response, so unfortunately we won't be able to just clear the cache and count on the next fetch being up to date, but have to actually maintain local state. So far I've removed the setTimeout loops in all the accessories like DaikinOnePlusThermostat and replaced it with a callback subscription to DaikinApi. I'll see if I can get a commit that I can share later of a working version. That said....

@jeffschubert What did you think about my PRs? The merging is getting a bit out of hand - I'm running a combined branch of all of them on my own homebridge right now - so would be awesome to know if you are interested in merging and collaborating?

jeffschubert commented 2 years ago

@FabianFrank I've skimmed the PRs just now and like what I see. I'll try to take a closer look tonight and merge them in. This has been my first foray into typescript and Homebridge development so your input is definitely welcomed.

FabianFrank commented 2 years ago

@jeffschubert Awesome thanks. Here's what I have so far: https://github.com/FabianFrank/homebridge-daikin-oneplus/commit/1de028b6fea38dcb456b0094c7f1809deb3ea258. It works well enough in that things seem stable and settings are no longer bouncing back and forth.

jeffschubert commented 2 years ago

@FabianFrank I think I got all your PRs merged in without breaking anything. Let me know if I missed anything. I like what I'm seeing on your data polling refactor. It's the direction my brain was telling me I needed to go, but my lack of typescript knowledge was preventing me from implementing thus far.

FabianFrank commented 2 years ago

@jeffschubert awesome, thanks! I've switched back to master, rebased my polling changes and made a PR for them just now. so far it's running fine. :)