Skip to content
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

autoUpdate and externalUpdate - ignored packets cause requested settings to be clobbered #192

Closed
prashker opened this issue Apr 12, 2023 · 9 comments

Comments

@prashker
Copy link
Contributor

Consider the following current "state" of HVAC/HeatPump library:

hp.enableAutoUpdate();
hp.enableExternalUpdate();
currentSettings.temperature = 17

Requesting the following call
hp.setTemperature(18) makes our wantedSettings.temperature = 18;

That would be handled in the autoupdate block of code and applied accordingly.


My understanding of the code is, if the packet that requests a temperature/state change was to fail/be ignored for some reason, this block of code would be problematic

HeatPump/src/HeatPump.cpp

Lines 635 to 639 in 1abc49a

// if this is the first time we have synced with the heatpump, set wantedSettings to receivedSettings
if(firstRun || (autoUpdate && externalUpdate)) {
wantedSettings = currentSettings;
firstRun = false;
}

Presumably this code exists to:

  • Assumes that any requested settings sent to HVAC are received and applied
  • If wantedSettings differ from currentSettings, then the end user must have changed them externally

There seem to be some circumstances in which the HVAC does not "take" settings. I've experienced this with my PVA (also being discussed in projects that leverage HeatPump.h such as gysmo38/mitsubishi2MQTT#190)

I am finding it difficult to find a clear way to reproduce the "ignored" requests to change HVAC state, but it seems I am not the only one affected by this issue.

Would it make sense to add some additional logic (if possible) that confirms wantedSettings were actually autoUpdated successfully, before considering any external updates?

It appears the library also has some form of "retry" mechanism, IF external updates are disabled, wantedSettings wouldn't equal currentSettings, and it would basically try again see:

HeatPump/src/HeatPump.cpp

Lines 179 to 181 in 1abc49a

else if(autoUpdate && !firstRun && wantedSettings != currentSettings && packetType == PACKET_TYPE_DEFAULT) {
update();
}

@GhyslainBruno
Copy link

Hi,

I'm starting a zoning project using Mitsubishi heatpump, MQTT, HA and this library helps a lot.

However, just like some people in here, I'm experiencing this same issue, which is really complicating things.

I'm planing on doing such a thing (check if wanted settings are same as wanted settings) using NodeJS for now, as I'm much more experienced with JS for now. Would be awesome to have it in the lib :-D

Anyway, thanks to all of you guys

prashker added a commit to prashker/HeatPump that referenced this issue Apr 17, 2023
Attempts to address SwiCago#192

All updates to wanted settings keep a timestamp of when the requested change was done

External Updates are only respected 10  seconds after that
prashker added a commit to prashker/HeatPump that referenced this issue Apr 17, 2023
Attempts to address SwiCago#192

All updates to wanted settings keep a timestamp of when the requested change was done

External Updates are only respected ~30  seconds after that
@prashker
Copy link
Contributor Author

prashker commented Apr 17, 2023

I created a fork (https://github.com/prashker/HeatPump/tree/ignore_external_updates_grace_period) of this to test some logic I had rumbling in my head. I have nowhere near enough time to properly debug this - I hacked together something I thought would work.

Basically, every call to the setXXX series of functions, will update a lastWanted time.

  • setPowerSetting
  • setModeSetting
  • setTemperature
  • setFanSpeed
  • setVaneSetting
  • setWideVaneSetting

Any wanted settings made in the last 30 seconds will trump any external changes (configurable via AUTOUPDATE_GRACE_PERIOD_IGNORE_EXTERNAL_UPDATES_MS) which have the potential to clobber unapplied wantedSettings.

There's an obvious edge case here where you actually do an external change within 30 seconds of a library-spawned change (ex: use the remote or use the thermostat control panel) - I leave it up to others to decide what is best behavior here.

@GhyslainBruno Consider recompiling HeatPump using my fork and see if this helps.

@GhyslainBruno
Copy link

Just to be sure if I understand right, I have 2 questions:

  1. The code you added will not update external changes within 30sec, but after that, it will right ?

  2. I am using SwiCago library with gysmo38/mitsubishi2MQTT one. When you say "external updates", you don't mean MQTT updates right ? (just to be sure ^^)

For my use case I have to be able to retrieve external changes (through wall remote), as I guess, a lot of people :-). But a 30sec delay is acceptable I guess.

I'll give it a try as soon as I have some time to do so and I'll let you know what the results are.

What kind of AC do you use @prashker ? The one I use is a duct PEAD SM50JA and beside that, everything is nicely working.

In case you're interested in what I'm doing right now, here is a little personal reminder I use : https://gitlab.com/ghyslainbruno/zoning.
Right now, the retry NodeJS code is kind of working, but not a very clean way to handle this issue.

Anyway, thanks for your time and your work !

@prashker
Copy link
Contributor Author

@GhyslainBruno

  1. External updates are ignored for 30 seconds after making a setXXX() call via HeatPump library.
  2. External update means "not programmatically" aka any manual changes with your remote or thermostat console.

I tested with lower thresholds, around 10 seconds, but I wasn't 100% sure if it was working - 30 seconds I haven't had any issue in a week+ of testing.

I have a whole home PVA-A36AA7 with PAR-40MAA remote control.

@logon84
Copy link

logon84 commented Apr 26, 2023

I did my own fix for this today, maybe is still soon but for the moment all the times I tried, the hvac got the settings properly. I changed line 180 of Heatpump.cpp from "update();" to:
while(!update()) {
delay(10);
}

I'll write again in a few days to confirm or discard this fix.

@prashker
Copy link
Contributor Author

Neat attempt @logon84 - keep us posted!

I didn't want to dabble into building my own auto-retry mechanism, mostly out of lack of confidence. If there was a reproducible way induce a failed settings change, it would be easier to debug.

@notentirelysure85
Copy link

I created a fork (https://github.com/prashker/HeatPump/tree/ignore_external_updates_grace_period) of this to test some logic I had rumbling in my head. I have nowhere near enough time to properly debug this - I hacked together something I thought would work.

Basically, every call to the setXXX series of functions, will update a lastWanted time.

* `setPowerSetting`

* `setModeSetting`

* `setTemperature`

* `setFanSpeed`

* `setVaneSetting`

* `setWideVaneSetting`

Any wanted settings made in the last 30 seconds will trump any external changes (configurable via AUTOUPDATE_GRACE_PERIOD_IGNORE_EXTERNAL_UPDATES_MS) which have the potential to clobber unapplied wantedSettings.

There's an obvious edge case here where you actually do an external change within 30 seconds of a library-spawned change (ex: use the remote or use the thermostat control panel) - I leave it up to others to decide what is best behavior here.

@GhyslainBruno Consider recompiling HeatPump using my fork and see if this helps.

I have been running your fork for the past two weeks. I have three MSZ-AP heatpumps running https://github.com/gysmo38/mitsubishi2MQTT/ and I haven't had a single instance of the settings bouncing back. Great work!

@SwiCago
Copy link
Owner

SwiCago commented Jun 16, 2023

@prashker , over the years so many people have mucked with external updates and auto updates .In some cases it works great and in others it does not. I do not think MITSU ever thought we would try to control these units from 2 sources. In my case I do not. All my remotes have gone in a junk drawer LOL. I use automation, in combination of an android app to override settings.

As for auto update and external update.
The way I remember it, if external update is enabled, then as soon as the setting are received from HP the MCU settings are overwritten. If auto update is enable, then what ever is sent to unit via remote will get overwritten by MCU, so as to prevent anyone else messing with settings.
Example: Say you own a business and employees or have a house full of children that keep changing the settings with remote. Well, by enabling auto update, you will always overwrite what ever they sent. SO if the set AC down to 60F and you have it set in automation at 73F, on next auto update the unit is put back to 73F.

That is what auto update is for. As soon as MCU and HP are out of sync, MCU resends what it knows is true.
With external update, what ever HP send MCU reset MCU settings and keeps them in sync.
You do not use both the settings together. It is one or the other.

Hope that helps.

If you feel there is still an issue and have a solution, you can always make a pull for review. But I feel this is not used often. Far easier to use an APP than those stupid remotes or wall units.

@SwiCago SwiCago closed this as completed Jun 16, 2023
@prashker
Copy link
Contributor Author

@SwiCago thank you for responding. I am coming from a whole home heatpump solution, so I am not too well versed in a business scenario or multi-heat pump scenario. ExternalUpdates implies autoUpdate in the codebase.

Let's bring this back to the problem space:

  1. Right now, as it stands, update() can fail - I haven't found a way to reproduce it - this is the core problem that most users using this library are facing. Programmatic changes are not being applied.
  2. autoUpdate will attempt to reapply settings in a subsequent update() call - eventually "succeeding" and masking the problem of a failed update() call.
  3. Only when externalUpdates are enabled, are people seeing changes not getting applied.

Most users are solving this by bake their own retry mechanism - if the changes requested weren't applied, try applying them again.

Since I don't have enough skill to figure out a way to reproduce and fix why update() sometimes fail, I simply just prioritize APP changes for 30 seconds after an APP change is done. For autoUpdate only users, this does not change any behavior (failed updates will continue to auto-retry), for externalUpdate users, this gives a more reliable experience - as it will give 30 seconds for the app initiated changes to succeed.

Will submit my pull request as for past 2 months I have not had any anxiety, and have had a consistent experience that I can properly automate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants