search

SwiCago / HeatPump

Arduino library to control Mitsubishi Heat Pumps via connector cn105
GNU General Public License v3.0
834 stars 230 forks source link

autoUpdate doesn't (always) work as expected #79

Closed llemtt closed 6 years ago

llemtt commented 6 years ago

Hi,

if for whatever reason after a successful update the call to sync(RQST_PKT_SETTINGS) inside update() method fails

if(packetType == RCVD_PKT_UPDATE_SUCCESS) { // call sync() to get the latest settings from the heatpump, which should now have the updated settings sync(RQST_PKT_SETTINGS); ...

beiing that still stands wantedSettings != currentSettings any following sync() gets stuck into trying to update again and again...

It should be checked that sync(RQST_PKT_SETTINGS) is successful and take appropriate actions if not!

Cheers

SwiCago commented 6 years ago

The call to sync is not synchronous. It sends the packets to the HP for RQST_PKT_SETTINGS, the HP does not confirm wether or not it got the packets(there is no ack), but rather responds at a later time. When the response is finally sent back to us, we can process it. There is not reason to block or add additional checks. To be honest with you, even if it fails the one time and resends again, it should not matter and eventually it will sync. My setup of 6 units has been running w/o issues for over a year now. Yes, I have seen where an updated response was not instant, but that was also seen when using kumo cloud. That is just how these units were designed. No instant feedback. You if you like disable auto update.

llemtt commented 6 years ago

Yes but in my case is RQST_PKT_SETTINGS sending that fails...

134 else if(canSend(true)) { 135 byte packet[PACKET_LEN] = {}; 136 createInfoPacket(packet, packetType); 137 writePacket(packet, PACKET_LEN); 138 }

canSend(true) returns false!

and in that case as you can clearly see it creates a loop because the next call to sync() calls update() again...

Disabling auto update is a workaround but it would be better to fix the overall logic!

I have already badly patched code inside update() but that changes the sync() behavior from "not synchronous" to "synchronous" so it's not a solution I suggest.