kukulich / home-assistant-jablotron100

Home Assistant custom component for JABLOTRON 100+ alarm system
MIT License
65 stars 24 forks source link

fix: call async methods on hass asyncio loop + add poetry for easier development #81

Closed zlymeda closed 10 months ago

zlymeda commented 11 months ago

Fixes #80

I did lots of debugging, I added more debug statements to the reader and writer functions and I discovered that the stream was never closed and that my writer was stuck at sending the authorization packet.

def callback(_) -> None:
    stream.close()

try:
    async_call_later(self._hass, 0.1, callback)
except Exception as ex:
    pass

I found a few places where we called an async function on a worker thread and that caused errors as the async function needs to be processed on a thread that owns the loop.

I also added poetry, as I find it easy to install it for local testing. Let me know if you don't want that.

Thank you

kukulich commented 11 months ago

We cannot remove the 1 second timeout - it wiľ break installation of other people

zlymeda commented 11 months ago

We cannot remove the 1 second timeout - it wiľ break installation of other people

do you know why it is necessary? it was not called for me for some reason, I think because of calling it in wrong thread. so I can run it similarly as the other fixes.

I was just wondering what is the reason for that timeout.

zlymeda commented 11 months ago

hi @kukulich I posted an update. however, I am not sure if it is a good idea to close a stream in the main "UI" thread let me know what you think

kukulich commented 11 months ago

There was problem that without the timeout the stream was closed to early and the sent packet was not delivered to Jablotron.

zlymeda commented 11 months ago

@kukulich and what do you think about doing a sleep in the current thread - we are not on the "UI" thread so we can sleep and close, instead of running the close later on the "UI" thread.

like so:

stream = self._open_write_stream()
stream.write(packet)

time.sleep(1)
stream.close()
kukulich commented 11 months ago

I think this version was used earlier but HA started to report it as problematic codde.

zlymeda commented 11 months ago

I think this version was used earlier but HA started to report it as problematic codde.

ok, I will try to update it on my side and test it to see if there are issues. I could see this being a problem only if this was run on the "UI" thread

zlymeda commented 11 months ago

@kukulich OK, so I know what was the problem. the _send_packet_by_stream is called both on the main thread as well as on the worker threads. it is okay to do sleep and close it on the current thread if we are not on a main thread.

let me know what you think. I tested this and integration loads find, I tried arming/disarming as well as motion sensors and smoke detectors seem to be working and reporting fine.

kukulich commented 11 months ago

@zlymeda is it possible to test just the changes that are necessary to fix the bug?

zlymeda commented 11 months ago

@kukulich ok done, I updated it and squash the commits. can you take another look pls?

kukulich commented 10 months ago

I've tested it in my environment and it looks there's no problem so I'll merge it. Thanks.