SwiCago / HeatPump

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

IR changes+ Updates are not reflected. Advise required. #156

Closed JMan7777 closed 4 years ago

JMan7777 commented 4 years ago

Hello,

Thx a million for this great project.

I'm currently trying to use it to control my aircons (3 indoor models: MSY-GE10VA, MSY-GE13VA, MSY-GE24VA, connected to one outdoor multisplit unit) - All are Malaysia/Singapore models but I found on the web that the PCB's are the same as in the related Australian models). I can connect to the aircons and get the correct readings after ESP8266 startup.

Problem 1:

However, I'm still someone who is using the IR remote ;) as well but having trouble that IR changes get reflected in the heatpump settings.

In my setup function I'm doing something like this:

hp.connect(&Serial, true); if (hp.isConnected()) { hp.enableAutoUpdate(); hp.enableExternalUpdate(); hp.sync(); hp.getSettings(); }

and in the loop I always sync again: hp.sync(); heatpumpSettings settings = hp.getSettings();

However the 'settings' variable never reflects the IR changes. So if I for example change the temperature via IR the change never shows up in the 'settings' variable.

Problem 2:

If I update the settings via my code (not IR) and afterwards also call:

hp.setSettings(settings); hp.update();

,the update function returns true which means it was successful. Also physically the aircon settings changed on the aircon itself.

However afterwards in all loop rounds:

hp.sync(); heatpumpSettings settings = hp.getSettings();

the settings still never reflect the changes as well . The settings always contain the old settings before the change.

Problem 3:

If I try to turn off the AC via:

settings.power="OFF";

the final update function:

hp.setSettings(settings); hp.update();

always fails (returns false). Also AC will not turn off.

Please kindly advice.

Thx a lot.

Regards, Markus

SwiCago commented 4 years ago

@hawkefly are you still active in following this project. I know you had a similar issue and we resolved it. Can you share how exactly you use this feature (example code). I personally have never used this feature and have no need for it, don't even know where my remotes are LOL. This feature as you remember was added for someone who had a need for it and the you also had the same need. If you example is what @JMan7777 is doing, the maybe you could give latest a test for us, to see if a recent pull broke the feature. Your help would be appreciated. Thanks

JMan7777 commented 4 years ago

@SwiCago Thx a lot for helping out. Any idea on my Problem 2 and 3?

SwiCago commented 4 years ago

@JMan7777 this pull https://github.com/SwiCago/HeatPump/pull/147/files Might have broken it, someone tried to make it more responsive for something called esphome. . Here is the previous pull files https://github.com/sw-home/HeatPump/tree/2bbc2e9252f8b2c10b3e771d6841c8550624bb85 Download it and let me know if you still have the issue.

JMan7777 commented 4 years ago

@SwiCago Thx a million. Using this old version solved all my 3 problems. It only takes up to 1 min until the changes are reflected in the synced settings.

Unfortunately I'm loosing all the nice other updates made since the older version timestamp, like e.g. the isConnected() function which is really helpful.

Is there any way that you could make a version which satisfies both needs (pull 147 and my needs). I might not be the only one where the pull 147 is creating issues.

Thx again.

At least for now I can go ahead and finish up my Samsung Smartthings integration. FYI: I put the esp8266 in a small box with a SSD1306 OLED display, an RGB led and a reset button. I'm also creating a Smartthings device handler who will talk via JSON to the AC and with this have my smarthome integration ;). After it's all working I can share the code if someone is interested.

Cheers.

SwiCago commented 4 years ago

don't know . I need to understand exactly what the other pull broke exactly. Might even roll it back. I will tag them in this issue and see if they can help out. I really don't have time right now with this covid bullshit going on.

@sw-home , your last pull broke the above. Can you review and work with @JMan7777 as a tester to fix it. Otherwise I will need to rollback your pull.

JMan7777 commented 4 years ago

@SwiCago ,Thx again. @sw-home , FYI: I saw in your code that you are using the normal webserver implementation and understand that the sync delay is affecting the response time. I had the same issue and switched to ESPAsyncWebServer. With this you can sync in the loop (even with the delay) since your webserver response asynchronous. Without your pull 147 I have no issues getting the settings from the AC but with your changes I never get any settings data. Please kindly advise. Thx too.

sw-home commented 4 years ago

@JMan7777 my own application is based on @SwiCago 's Fancy_web example. It calls hp.sync() in the loop() and hp.getSettings() whenever requested to return the state. I get a correct state when powering up the ESP, setting changes are visible shortly after issuing them (it takes maybe a couple of seconds). Also no problem switching the unit off. I guess that my change somehow messed up some hidden timing issue. I'll try to figure out what could be wrong. Thanks for mentioning the async webserver, that might be a good workaround. For myself it might not solve everything unless I rewrite the whole application to be interrupt driven since I'm controlling other hardware too (I'm using the Heatpump for in-floor-heating).

SwiCago commented 4 years ago

@sw-home thanks for chiming in. I am all for responsive code, however these heatpumps are pretty picky. The official wifi dongle is even slower that this library lol. Anyhow, most of us do not use IR, so that part doesn't effect most, but some do. So we should try to figure out what went wrong. I as well use HP to control my hydronic heating, but I use MQTT for all that. Anyhow, have a look, maybe you can compare your change vs prior and see an easy fix. I would do it, but very busy at the moment with covid bullshit. Work with @JMan7777 to test, before new pull. Please use most current branch, so we don't go backwards

JMan7777 commented 4 years ago

@SwiCago , I will work together with @sw-home to find a solution. @sw-home , I'm currently still waiting for some level-converters for my new ESP32's (for TX & RX) as I'm not 100% trusting the Wemos D1 Mini clones I currently have (Direct connected without level converter). Once I have got my converters and have setup everything successful I will give the latest version a try again and let you know. I just want to ensure I'm not chasing a ghost here ;). Thx again. (By the way, I'm German too)

To both of you: Stay safe.

SwiCago commented 4 years ago

Vielen dank zusammen. Blieb sicher und schön Händchen waschen ;)

crnjan commented 4 years ago

@JMan7777 - you can try https://espeasy.readthedocs.io/en/latest/Plugin/P093.html#p093-page and should give instant update when operating AC with IR remote. You can download firmware from - https://www.dropbox.com/s/0cwteamx0a02xn2/ESPEasy_mega-20200305-57-PR_2879.zip?dl=0 and use ESP_Easy_mega-20200305-57-PR_2879_test_ESP8266_4M1M_VCC.bin that is in the zip for 4MB flash devices, as D1 Mini. Btw, I too use D1 mini clones in my two ACs for ~2 months now without any issues, FYI ...

JMan7777 commented 4 years ago

@crnjan , Thx for that. Unfortunately I'm not going to use any MQTT functionality. However, I will have a look at the P093_MitsubishiAC.ino file to see how the AC is accessed in the code. Thx again.

crnjan commented 4 years ago

Well MQTT is just one way, esp easy offers much more - udp, http, telnet, ... - please see here - https://espeasy.readthedocs.io/en/latest/Controller/_Controller.html.

The link to source code (I think the one in the doc is broken).

SwiCago commented 4 years ago

First sorry, I deleted my other comment. Under a lot of stress. @crnjan please don't suggest another library as a fix. I need @sw-home to work with @JMan7777 to figure out what went wrong with the pull. Now if @JMan7777 prefers to use an alternative library, fine. In that case this can be closed. I appreciate everyones help with testing and keeping the library alive and support as many HPs as possible. Cheers

JMan7777 commented 4 years ago

@SwiCago , don't worry. I know everyone is under stress at the current situation. I will wait for my parts to arrive and will then try to figure out whats causing my problems (with help of @sw-home , if required). For now I have no intentions to change to an alternative library as I really like to use your one.

As you said, at the moment there are much higher priorities than my issue here. So just let it open and I will respond as soon as I have news.

To all of you: Stay safe and at home.

crnjan commented 4 years ago

@SwiCago - you got me wrong - the code in EspEasy is a modified version of this library, mainly the delays were removed. If that would be (proven) beneficial here too, I can do PR here. I would actually like that the code in EspEasy plugin would be able to use this library directly ... so it's only one place we need to maintain (bugfixes, features, ...) and we all benefit from ...

The use case @JMan7777 is describing seems to work very well for me, therefore thought it might be good to consider integrate that part back here ...

My 2 cents.

Stay safe.

SwiCago commented 4 years ago

@JMan7777 @crnjan Thanks guys. Stay safe, crazy world we live in. No toilet paper and lots of dumb people not listening to officials.

sw-home commented 4 years ago

Watching the empty street I had a different idea: Since I've broken the tight connection between writing, delay and reading within the library, it now obviously depends on a continuously running loop() function.

Let's imagine for a moment that people's other firmware features / self added features within their loop() is blocking and using delay too. Then we have the problem that the library suffers from the lags in some way, maybe missing answers from the Heatpump or not sending out stuff right. So I now realize that my change is a bit a box of pandora... it allows better multitasking but also requires it.

So @JMan7777 : please review your other code... is it using delay()? In my projects I often toggle the board LED with every loop() run to visually verify that the loop is running continously and smoothly. Ideally you'd see a dimmed light with a 50% duty cycle. Any delays show up as extended bright or off periods.

A very user friendly solution might be to have two different function sets within the API: one that executes everything synchronously and stops the loop for whatever time it takes, and one that is non-blocking (but requires all other code to be non-blocking too).

JMan7777 commented 4 years ago

Hi @SwiCago , thx a lot for the great deep dive. Actually I was using some other functions (with delays) in my code inside the loop() function. I wasn't aware that sync() must run continuously without any delays. So, this might be my issue.

I will try to convert my delays into Arduino Ticker's and also move some other functions into Ticker's itself.

Just an idea:

If your code also could get rid of all delays() and making them maybe Ticker compatible, we could run the sync() and other of your functions in a Ticker itself.

Ticker also relies on SDK software timer functionality. Since this is a software timer, there are no guarantees regarding accuracy. Regarding things which you can call from ticker callbacks, there is no definitive list. You can not call delay or anything else which relies on coroutines (i.e. blocking IO methods of Serial and WiFi libraries), because ticker callbacks do not have their own continuation stack.

I'm still waiting for some parts but will keep you up-to-date,

Thx again.

SwiCago commented 4 years ago

@JMan7777 that is a good idea, however I wish to keep this library none arduino dependant. For the most part, most people use MQTT or async web with this library and it seems to work well for those purposes. Thanks for working together to figure out the issue. It seems like you guys figured it out.

JMan7777 commented 4 years ago

@SwiCago , I understand. I will go ahead and re-write my code. You only might think about to replace your blocking delays by using the non-blocking millis() function. Will keep you up-dated. Thanks again.

sw-home commented 4 years ago

That's basically what my change was all about. I removed the huge delay(1000) and implemented the wait using the millis (https://github.com/SwiCago/HeatPump/pull/147/files#diff-aa5d1dbc2ff977e82fbccafda04a670cL496) line 496. But then I need the loop to run in a timely manner.

SwiCago commented 4 years ago

@JMan7777 are you able to use latest branch with your code changes in your loop? If so, then we can close this.

JMan7777 commented 4 years ago

Hi @SwiCago , unfortunately the parts needed for my ESP32 still not arrived and I used my ESP8266 already for another project. I was able to rewrite my code (using now separate tasks via FreeRTOS) and the sync is now running approx. every 50-100ms.

I leave it up to you if you want to close the issue right now or if you want to wait until I tested. If you want to close it, I can re-open later if I still have issues.

Thx again.

JMan7777 commented 4 years ago

Hi, I have good news. After my re-write it's working now. Thanks for your great support. Once I finish my code and also the Samsung Smartthings handler I will share.

Cheers,

Markus

SwiCago commented 4 years ago

@JMan7777 great, so no bug. @sw-home thanks for being available.

JMan7777 commented 4 years ago

Hi.

As promised I started creating my own GitHub project for controlling my air conditioner's via Samsung's SmartThings Classic app.

If you are interested just have a look at:

Mitsubishi-Aircon-SmartThings

It's still WIP but gives you an idea what I'm working on.

Cheers,

Markus