OStrama / weishaupt_modbus

This integration allows you to monitor and controll your weishaupt heatpump via modbus.
MIT License
10 stars 2 forks source link

HA crashes when host is unreachable #7

Open Cryotize opened 6 days ago

Cryotize commented 6 days ago

Describe the bug If the host (pump or whatever) is unreachable for whatever reason, it crashes my HA instance. I had to remove the integration via terminal in order to get HA working again. I don't know if this an error by this integration, or if HA should disable the integration by itself when it detects this kind of error.

Steps to reproduce the error Have the integration setup in HA, unplug your host so that it is unreachable, wait for HA to crash.

Expected behavior HA should continue working, even when the integration is failing.

Logs

homeassistant  | 2024-10-16 11:34:24.907 ERROR (MainThread) [pymodbus.logging] Connection to (10.10.11.101, 502) failed: timed out
homeassistant  | 2024-10-16 11:34:24.975 ERROR (MainThread) [pymodbus.logging] Connection to (10.10.11.101, 502) failed: [Errno 113] Host is unreachable
homeassistant  | 2024-10-16 11:34:12.617 ERROR (MainThread) [homeassistant.components.sensor] weishaupt_wbb: Error on device update!
homeassistant  | Traceback (most recent call last):
homeassistant  |   File "/usr/src/homeassistant/homeassistant/helpers/entity_platform.py", line 724, in _async_add_entity
homeassistant  |     await entity.async_device_update(warning=False)
homeassistant  |   File "/usr/src/homeassistant/homeassistant/helpers/entity.py", line 1300, in async_device_update
homeassistant  |     await self.async_update()
homeassistant  |   File "/config/custom_components/weishaupt_wbb/entities.py", line 156, in async_update
homeassistant  |     self._attr_native_value = self.translateVal
homeassistant  |                               ^^^^^^^^^^^^^^^^^
homeassistant  |   File "/config/custom_components/weishaupt_wbb/entities.py", line 171, in translateVal
homeassistant  |     val_x = self.calcTemperature(mbo_x.value) / 10
homeassistant  |             ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~
homeassistant  | TypeError: unsupported operand type(s) for /: 'NoneType' and 'int'
OStrama commented 5 days ago

I could reprodue the error. I also could remove the exception in my code. Version 0.0.7 will not show this anymore.

However, the complete HA hangs up when disconnecting the heat pump.

This happens, as far as I understand it for now, in the pymodbus and the websocket module. There seems to be a quite long timeout. I'll try to find out how to prevent this.

MadOne commented 4 days ago

You can specifiy the timeout for a modbus connection / request:

modbusobject.py line 31: self._ModbusClient = ModbusClient(host=self._ip, port=self._port, timeout=1) EDIT: 0.1 also works

But the problem still exists, because the integration does this for all entities (85 in my case)

I think the best approach would be to create one object that periodically pulls all modbus values. Looks like the update coordinator is what is designed for this purpose. https://developers.home-assistant.io/docs/integration_fetching_data/

thetschulian commented 3 days ago

Hey there, thanks for this great addon. I can confirm this error with a WBB20. I have the WBB20 in my checkmk monitoring, everytime the modbus tcp port is unreachable, the HA is not responding.

PS: i am a pretty new HA user

MadOne commented 3 days ago

Maybe something like this?

https://github.com/MadOne/weishaupt_modbus/blob/test/custom_components/weishaupt_modbus/heatpump.py

OStrama commented 3 days ago

Thanks for the tips and ideas!

Who calls the poll method?

(I also got some hints from the ModbusTCPClient developers to use the async methods. I think this would be the preferred way but I have to understand how to use them. )

MadOne commented 3 days ago

This is just a small proof if concept. According to the ha developer docs s update coordinator is designed to poll all values and update the entitys.

Just ket my know if you want me to further check this approach. I think this has several advanteges to not open a new Connection for every value.

Anyways switching to async is surely good. The integration will still get stuck in timeouts but not block the home Assistent main "thread".

I think the best solution would be a combination of both.

EDIT: I digged a bit into it and i think i found out that it is my fault. I designed the integration to be async. Therefore the external calls have to be async too or called through an HA helper. So we could either go back to a sync ingetration (not state of the art), call the modbus functions through an unblocking HA helper (hacky) or switch to async modbus functions (prefered).

MadOne commented 2 days ago

I managed to switch to the async modbus client: https://github.com/MadOne/weishaupt_modbus/commit/e1b531ce3255e932bcd0fe12cfa749980e1b6dc4

The HA Container automatically reformats the edited files on save. So there is some reformating. I think you can pull this commit if you are ok with that reformating. Otherwise it should be pretty easy to see what has to be changed.

EDIT: I can confirm HA is no longer blocked with a false / not responding heatpump ip.

EDIT2: There seems to be a problem with the calculated sensor.

OStrama commented 2 days ago

Thanks! Looks great. I'll pull the commit and integrate it.

I assume you mean "Wärmeleisrtung" when you talk aboput thew calculated sensor. What's the problem with it?

BTW: Is it possible to open a repository so that more than one person can work on it? In my opinion it would make sense to work together on one code base...

OStrama commented 2 days ago

... reformatting is no problem ;-). It's my 1st Python project after many years C/C++, so maybe my formatting is not the typical Python one...

MadOne commented 2 days ago

Thanks! Looks great. I'll pull the commit and integrate it.

I assume you mean "Wärmeleisrtung" when you talk aboput thew calculated sensor. What's the problem with it?

BTW: Is it possible to open a repository so that more than one person can work on it? In my opinion it would make sense to work together on one code base...

Yes "Wärmeleistung" was the problem. I didnt have time to to check what the problem is yet. I think there are some async and awaits missing. EDIT: Got it

I would really like to work with you on this integration. But be aware i am no programmer. I am a mechanical engineer and only doing this as a hobby. Im pretty new to python too.

... reformatting is no problem ;-). It's my 1st Python project after many years C/C++, so maybe my formatting is not the typical Python one...

The problem is, it makes pull requests and diffs more complicated to read. I am using the HomAssistant dev container, witch is verry convinient by the way. It uses RUFF for lintig and code formating. Maybe we could agree on using that?

If you still want to work with me you can add me under settings -> Collaborators.

OStrama commented 2 days ago

Yes, of course! Working together would be great! I added you as collaborator, so please feel free to contribute to this repo. I also changed to the Devcontainer and RUFF, so the diffs should be easy from now on..

OStrama commented 1 day ago

I changed modbus calls to async. Made some debugging (@property.setter does not work async etc..) and now it seems to run.

However, I get a lot of "Failed to connect". Any idea to avoid them?

MadOne commented 1 day ago

I also have them. I added some warnings myself while Debugging. Not sure if this is my debuging or not. Had no spare time today, hope i can look into tomorrow.

thetschulian commented 13 hours ago

side info: my heatpump becomes unavailable for a couple of minutes everytime e-heizung 1 changes from EIN to AUS. I contacted the support already since I guess its a bug in the firmware

OStrama commented 12 hours ago

side info: my heatpump becomes unavailable for a couple of minutes everytime e-heizung 1 changes from EIN to AUS. I contacted the support already since I guess its a bug in the firmware

interesting, I'll check it by manually switching on E1...

OStrama commented 12 hours ago

It was not only the warnings, I also have gaps in the data because the sensors become unavailable from time to time due to the connection errors. However, I made a test in reading sensor values according to https://developers.home-assistant.io/docs/integration_fetching_data/. In principle this works without errors and gaps. But: I only get the data from the sensors up to statistics. The last sensors in the list are missing. If I do not read them from Modbus but only return the addresses, I get the values. If I change the order, always the last few sensors are missing. So it seems to be a timing problem. Any idea how to get rid of it? If there's a solution I'll reconfigure the whole stuff in that way..

MadOne commented 4 hours ago

Maybe many connections in a short time?