GuillaumeWaignier / fibaro

Some fibaro quickApp
Apache License 2.0
13 stars 4 forks source link

Thermostat setpoints not updating properly when using °F. Problems updating the setpoints in auto mode update steps. #10

Open PeterV989 opened 9 months ago

PeterV989 commented 9 months ago

I finally took the time to update my version of this package on my Z-Box hub. In the process of troubleshooting I added to the header of main.lua to enable it to be run within VS Code using fibemu from @jangabrielson https://github.com/jangabrielsson/fibemu.git.

The first issue concerns the conversion of °F to °C during the setpoint updates. The call to NestThermostat:setCoolingThermostatSetpoint() and NestThermostat:setCoolingThermostatSetpoint() receive two arguments. The second argument is the temperature unit ('C' or 'F') of the value passed in the first argument. I added a parameter to retrieve this argument and used it to adjust the setpoint when the value is 'F'.

The second issue concerns the two step update to both setpoints when the thermostat is in AUTO mode. The Nest Thermostat requires both setpoints to be set in AUTO mode, but the updates are made in two steps. When the HEAT setpoint is to be updated and the HEAT setpoint is higher than the COOL setpoint, the API throws an error. I fixed this by forcing the COOL setpoint to be 2° higher than the HEAT setpoint during this step. Likewise, when the COOL setpoint is being updated and the HEAT setpoint is higher than the COOL setpoint, the HEAT setpoint is forced 2° cooler than the COOL setpoint. When the user has made the correct settings, everything will work out just fine. But the UI does not guard against this condition. When the user wants to be stupid, she/he will end up with the heating and cooling to be within 1° of each other regardless of what she/he has tried to do. The Google Nest app performs the same thing within the UI but I think the spread is a more reasonable 3° difference.

I created a fork of this repo, fixed the header of main.lua, and then created a branch Fix-set-temperature-in-Auto-mode to make my other corrections. I also took the liberty to proofread the README.md file and fix some minor typos and grammar (no offense).

**** Jan. 20, 2024 13:45 Changed the adjustment from 1° to 2°. There was an error thrown when it was 1°.

PeterV989 commented 9 months ago

After looking really hard at this issue I have come to the realization that there have been issues with the updating of setpoints when mode is AUTO. My synchronous brain finally saw what was happening right in front of me all this time. There are two events sent from the UI when the user hits Set. The first event (which appears to be the "Set Heating Setpoint" event) only sends the heating setpoint (duh). But Nest requires that both setpoints be set. The second event (the "Set Cooling Setpoint" event) sends the cooling setpoint. But, during the processing of the first event, a call to the Nest API to update BOTH setpoints, begins and that process goes dormant waiting on the resolution of that API call. Only after the API call will the heating setpoint get updated in the device properties. All of that makes perfect sense. But in my previous, misguided assumptions, I failed to account for that. Thus, when the updating of the heat setpoint goes dormant, the updating of the cool setpoint (probably) begins with the event being received. [The whole architecture of the system is cooperative multiprocessing so only one task can be serviced by the system at any given time.] My lame brain forgot that point and just blithely grabbed the current heat setpoint, failing to account for the fact that the heat setpoint is in the process of being updated and has not been changed in the device properties. I don't want to ACTUALLY change the device property until hearing back from the API call with a success response. There seem to be two scenarios I witness. If I change a single setpoint which doesn't need to affect the other setpoint, I only see a single event. Thus I cannot defer the update until the second setpoint update event occurs. But Nest requires the heat setpoint to be at least 2°C lower than the cool setpoint or it burps with an error response. I found a kludge workaround for this but I'm not enthused by its use long-term. There are two properties called heatThermostatSetpointFuture and coolThermostatSetpointFuture that appear to be unused (the operative word there is appear). I'm thinking I could use them to store the new setpoint before the API call and clear it out after the completion of the API call. Then, during the other event (which occurs while the API call is processing) I could grab that setpoint and use it in the second API call. I hate this approach because it makes two API calls before the two setpoints are correctly set. Granted, they are only seconds apart but it is so inelegant. I'm looking at other things to see if there's a better, more correct, approach to this dilemma. I wanted to keep you informed and get any insights you might have.

PeterV989 commented 9 months ago

I am ready to submit these changes for your testing & approval. Because the updates to the two setpoints overlap, I inserted some flags in each update method. These flags indicate that an update is occurring for one of the setpoints. Then, if the other setpoint begins to process its own update, it tests to see if the first setpoint is in process. If it is in process, instead of setting an in-process flag, it sets a pending flag and stores the new setpoint and units for later retrieval. While the first setpoint is processing, if it sees the other setpoint is pending, it will use the new values and send them both to the cloud. This way there don't have to be two updates. On the other hand, if the second setpoint is not pending, it uses the old value and sends it.

Another consideration I ran into is that when the user adjusts the setpoints on the thermostat device QA, there doesn't seem to be any guarantee that the cooling temperature is warmer than the heating temperature in the UI. The climate panel seems to handle that in the UI so it will never need to trigger this fail safe. When the heating setpoint is being updated, it makes sure the cooling setpoint is at least 2.3°C hotter than the heating setpoint. Likewise, when the cooling setpoint is being adjusted, it makes sure the heating setpoint is at least 2.3°C cooler than the cooling setpoint. This is the equivalent of about 4°F adjustment. I found anything less than the 2.3°C would generate an update error during the API call. The net result will be that the final setpoints will reflect this correction and will depend on which setpoint was updated last.

On a side note, I use the fibemu emulator, by Jan Gabrielsson (https://github.com/jangabrielsson/fibemu), running in VS Code. The preamble is in the main.lua code. Since it is all stored as comments, there is no harm in having it there. But, if there are performance or size issues with it being there you can strip them out. I marked the end of this block of comments just above your original header.

I hope this will work satisfactorily. I learned a lot about lua and async processes (and pulled some hair out in the process). If there is something totally boneheaded in this, let me know so I can fix it. You can find it in my fork under the branch Fix-set-temperature-in-Auto-mode.