gysmo38 / mitsubishi2MQTT

Mitsubishi to MQTT with ESP8266 module
GNU Lesser General Public License v2.1
371 stars 133 forks source link

Possible to get stuck with stale remote_temp value #167

Closed brackw closed 1 year ago

brackw commented 2 years ago

There's currently no fail-safe for reverting back to internal temp sensor once remote_temp has been sent. I don't believe the heatpumps will revert to internal without a power cycle or explicit packet.

If MQTT fails, the code should send 0 to the heatpump to resume using internal temperature.

This could be done a few ways, such as by setting a global remotetempset bool to true when the topic is published to, then adding a line to send 0 to the heatpump when during mqttconnect if its true, then setting it false again.

It would also be nice to revert to internal temperature if the remote_temp topic hasn't been published to in a significant time, such that a homeassistant or other mqtt device failure doesn't end up with chasing an unobtainable temp target.

mbbush commented 1 year ago

I like this idea, and I'm about to start using remote_temp for the first time on my units. I might write something like this, and if I do, I'll post a PR. I was thinking about doing it in home assistant, but I like the idea of doing it on the ESP even better.

I'm thinking something like:

I haven't looked at the code yet; is sending a remote temp of 0 the way to tell the unit to revert to internal temperature?

brackw commented 1 year ago

@mbbush I implemented this locally for my units after having an event knock mqtt offline, I've been meaning to update my branch with the changes (plus a cleanup) to do a PR but I have not gotten around to doing it properly yet since I worked on an older rev.

I pretty much did exactly what you suggested and it has worked well, sending a 0 brings the unit back to internal sensor without issue. I would be happy to send you my code if you want to implement it into yours before I get a chance to do it right.

mbbush commented 1 year ago

@brackw I'd love to look at your code, thanks! Is this what you did?

brackw commented 1 year ago

@mbbush Yes, that is what I am currently running on my units and seems to work as intended.

mbbush commented 1 year ago

@brackw The only part that doesn't seem clear to me is your changes in void readHeatPumpSettings(). What were those for?

brackw commented 1 year ago

@mbbush I was trying to band-aid fix #178 you can safely ignore those changes.

mbbush commented 1 year ago

I pulled your code, made two small but significant changes, and posted #201.

One change was changing the place in the loop where the check for a recent temperature update happens. You'd put it inside the mqtt sync loop, which is only called if the mqtt server is up. The mqtt server going down is probably one of the most common reasons for an external temperature sensor to fail to report, so it's important the check still happen in that case. Instead I put it in the heat pump state update loop, so it's executed every 30 seconds.

The other was a bug I saw related to disabling remote temperature sensing by mqtt. It looked like your code would take a 0 from the mqtt topic, but convert it to celsius before sending to the heat pump. If you use Fahrenheit as your local unit, this will send -18 to the heat pump instead of 0, which it probably won't know how to interpret.

Thanks for the solid start!

danielgoepp commented 1 year ago

Sorry to butt into the middle of the conversation here. I think this is directly related to something I wondered about this project, but don't understand yet (I just installed 4 of these last week). I see from the documentation:

topic/remote_temp/set also called "room_temp", the implementation defined in "HeatPump" seems not work in some models

I have the remote temp sensors that came with kumo cloud controllers. From what I can tell here, this is about how this app and the SwiCago library handle the same functionality of having a remote temp sensor. So is this about not connecting to those same sensors any more, but rather providing the same function to the unit via a senors updating data from an MQTT topic? If so, I'm VERY interested in getting this working. I have temp sensors all over the place pumping into MQTT and I run home assistant and Node-Red. In fact the rest of my home climate control (boiler) is raw sensors, MQTT and a olimex esp32-poe running ESPHome images to turn on and off valves and pumps :)

I would love to help in this if I can. I'm not necessarily in a place to dig into the code, but I can definitely be a tester and provide debug data. Heck, I might even poke my nose into the code a little to see what's up. Even if I can't help, I'm definitely watching here to see if this is related to what I want to do here.

mbbush commented 1 year ago

So is this about not connecting to those same sensors any more, but rather providing the same function to the unit via a senors updating data from an MQTT topic?

Yes, exactly. This specific issue is about adding a fallback to revert to the internal thermometer if it's been too long since a temperature reading came in via mqtt

danielgoepp commented 1 year ago

Fantastic, thank you! I was a bit confused by the documentation. I will definitely follow here. if there is anything I can do to help / test, I'd be happy to. Looks like you have a forked branch. Perhaps I'll pull yours and put it on one of my units.

danielgoepp commented 1 year ago

I know this is closed now, but I'll put my two cents in here also. It seems like the new default timeout is 5 minutes, which for a lot of folks is way too short. My temp sensors can go as long as an hour without reporting a temp change. I mean, the temp in my house doesn't change super fast ;) I'm just going to update and reflash, but figured I would let you know in case other folks using this see strange behavior with this new timeout and temp settings reverting when they might not expect. Thanks!

Allram commented 10 months ago

Thank you for writing down this @danielgoepp ! I have been struggling with this for the past weeks after i updated and see that the remote_temp reset's itself way to often, and i use Homey and Aqara devices which use hour(s) between updates :)

The timeout should be closer to 30minutes at least from my point of view. I will just make my own build with a longer timeout.