Fixing trigger alert status and add some ancillary features by bigmoby · Pull Request #11 · RyuzakiKK/pyialarm

bigmoby · 2024-10-01T21:08:15Z

As reported in the subject has been fixed trigger status and retrieved zone_status. Added some other functionalities in order to improve the ialarm integration.

RyuzakiKK · 2024-10-03T11:57:40Z

Before I'll be able to review it, it would be great if you could keep pyialarm with your changes running for several hours, to ensure nothing breaks.

In the past I remember that the API of the alarm was very sensitive and susceptible to timing issues. E.g. multiple consecutive requests without being separated by a timeout eventually leaded the API to stop responding and the only way to recover it was to restart the alarm system.

andker87 · 2024-10-03T12:10:45Z

This is in fact the problem I have been experiencing with the ialarm-mqtt component.
My goal is to use the volumetric sensors also as occupancy sensors, to determine room occupancy. to do this I set a very low scan time (2 seconds), and so I was often running into this problem.
With the nodered flow, on the other hand, everything goes perfectly, and I have never had half a problem (I don't know if this can give you the info to approach the problem)

@RyuzakiKK I can pull up a test instance of home assistant with no problem, however, how can I test your component if it has not yet merged into HA?

bigmoby · 2024-10-03T13:22:54Z

Before I'll be able to review it, it would be great if you could keep pyialarm with your changes running for several hours, to ensure nothing breaks.

Hi @RyuzakiKK , at moment (I tested it for about a day) appear to me quite stable. Anyway I'll continue to run it in production

pyialarm/pyialarm.py

RyuzakiKK · 2024-10-05T13:37:33Z

pyialarm/pyialarm.py

+    async def _receive(self):
+        def raise_connection_error(msg: str):
+            self.sock.close()
+            raise ConnectionError(msg)
+
+        try:
+            if self.sock is None or self.sock.fileno() == -1:
+                raise_connection_error("Socket is not open")


If self.sock is None, we can't run self.sock.close().

What about using the pre-existing _close_connection() instead?

RyuzakiKK · 2024-10-05T13:39:46Z

pyialarm/pyialarm.py

+            with contextlib.suppress(OSError):
+                if self.sock:
+                    self.sock.close()


In which instances can this raise an OSError? I.e. why do we need to suppress it?

Also, if I'm not mistaken, when we are at this point, self.sock is already guaranteed that can't be None. So the if self.sock check can probably be avoided.

You're right! In order to suppress a Ruff hint, I modified the implementation in this incorrect way!

RyuzakiKK · 2024-10-05T13:39:57Z

pyialarm/pyialarm.py

+            with contextlib.suppress(OSError):
+                if self.sock:
+                    self.sock.close()


pyialarm/pyialarm.py

RyuzakiKK · 2024-10-05T13:48:40Z

pyialarm/pyialarm.py

+    def get_last_log_entries(self, log_list: list[LogEntryType]) -> list[LogEntryType]:
+        if not log_list:
+            return []
+        return log_list[:25]


Is this function needed? Doesn't seem to be used.

Ooops, I removed (and it's a mistake!) the log retrieval function! I would create a service in HA in order to have the log list.

Another nit is that the check if not log_list: should not be necessary. self.get_log() is never expected to return a None. We are guaranteed to receive either an empty list or a list of log entries.

I would create a service in HA in order to have the log list.

Can you elaborate a bit more on that point? I'm curious how the logs are going to be used in HA.

This function in the end is only doing a list slicing. Maybe we can let HA do it itself?

RyuzakiKK · 2024-10-05T14:07:06Z

pyialarm/pyialarm.py

+            raise ConnectionError(error_message)
+
+        if status in {self.ARMED_AWAY, self.ARMED_STAY}:
+            zone_status: list[ZoneStatusType] = await self.get_zone_status()


Here you are sending a GetAlarmStatus request and then a GetByWay back to back. When I initially attempted it I noticed that it made the alarm would eventually stop responding to the GetbyWay. That why in 2764bbd I was using a one second timeout.

If it works, great! If it doesn't, then we probably need to add some wait before sending the second request.

Mmm, very interesting! Honestly, I didn't notice any problems related to this, it seems to work without the timeout

RyuzakiKK · 2024-10-05T14:14:31Z

I'm not familiar with asyncio, so I can't really comment too much on it.

Ideally the PR could have been split in multiple part, a first one about the refactoring (e.g. using a const.py file, changing the strings from ' to ", using the f strings etc...), a second part about the transition to asyncio plus getting the detailed zone status, and finally a last part about the lint and automated tests.

But doing that now would probably require too much work, so likely not worth it. I'm just very glad you had the time to contribute those changes.
Thanks a lot!

RyuzakiKK · 2024-10-05T14:19:05Z

This is in fact the problem I have been experiencing with the ialarm-mqtt component. My goal is to use the volumetric sensors also as occupancy sensors, to determine room occupancy. to do this I set a very low scan time (2 seconds), and so I was often running into this problem. With the nodered flow, on the other hand, everything goes perfectly, and I have never had half a problem (I don't know if this can give you the info to approach the problem)

@RyuzakiKK I can pull up a test instance of home assistant with no problem, however, how can I test your component if it has not yet merged into HA?

To test it you could probably do something like this https://community.home-assistant.io/t/testing-pull-requests-via-custom-components/251587/2, where you download the updated pyialarm and place it in the custom components directory of HA.

However, I guess that we'd also need some minor changes to HA itself, because now the API functions are async.

@bigmoby am I right in assuming you were testing your changes to pyialarm manually? And not by updating the pyialarm component in a real HA instance?

bigmoby · 2024-10-06T07:19:22Z

I'm not familiar with asyncio, so I can't really comment too much on it.

Ideally the PR could have been split in multiple part, a first one about the refactoring (e.g. using a const.py file, changing the strings from ' to ", using the f strings etc...), a second part about the transition to asyncio plus getting the detailed zone status, and finally a last part about the lint and automated tests.

But doing that now would probably require too much work, so likely not worth it. I'm just very glad you had the time to contribute those changes. Thanks a lot!

Hi @RyuzakiKK , You're totally right! I should have broken the PR down into smaller parts for refactoring, switching to asyncio, and adding tests. It would have been much easier to follow. I got a bit carried away trying to fix everything at once, though. 😅 I'll try to do better in future PRs!

bigmoby · 2024-10-06T07:22:57Z

This is in fact the problem I have been experiencing with the ialarm-mqtt component. My goal is to use the volumetric sensors also as occupancy sensors, to determine room occupancy. to do this I set a very low scan time (2 seconds), and so I was often running into this problem. With the nodered flow, on the other hand, everything goes perfectly, and I have never had half a problem (I don't know if this can give you the info to approach the problem)
@RyuzakiKK I can pull up a test instance of home assistant with no problem, however, how can I test your component if it has not yet merged into HA?

To test it you could probably do something like this https://community.home-assistant.io/t/testing-pull-requests-via-custom-components/251587/2, where you download the updated pyialarm and place it in the custom components directory of HA.

However, I guess that we'd also need some minor changes to HA itself, because now the API functions are async.

@bigmoby am I right in assuming you were testing your changes to pyialarm manually? And not by updating the pyialarm component in a real HA instance?

Thanks for the testing suggestion! I'm currently testing the new library implementation by directly modifying the code in the Home Assistant core DEV branch and applying the changes there. This allows for quicker iteration and verification.

Today, I'm hoping to create the PR for the integration part as well. It's almost finalized!

bigmoby · 2024-10-06T07:32:52Z

I'll post the HA integration PR after this library (with new implementations) will be deployed in pypi.org of course :-)

RyuzakiKK · 2024-10-06T08:39:47Z

pyialarm/pyialarm.py

+    def _ensure_socket_is_open(self) -> None:
+        if self.sock is None or self.sock.fileno() == -1:
+            self._close_connection()

    def _close_connection(self) -> None:
-        if self.sock and self.sock.fileno() != 1:
+        if self.sock and self.sock.fileno() != -1:
            self.sock.close()


The implementation of _ensure_socket_is_open() seems off? I don't think it's expected to call _close_connection() when we already ensured the self.sock is None or without an open fileno.

If I'm not mistaken, probably you wanted to do something like the following?

def _is_socket_open(self) -> bool: return self.sock and self.sock.fileno() != -1 [...] async def ensure_connection_is_open(self) -> None: if not _is_socket_open(): self.sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM) [...] async def _receive(self): if not _is_socket_open(): raise_connection_error("Socket is not open")

RyuzakiKK · 2024-10-06T08:44:23Z

Thanks for the testing suggestion! I'm currently testing the new library implementation by directly modifying the code in the Home Assistant core DEV branch and applying the changes there. This allows for quicker iteration and verification.

Today, I'm hoping to create the PR for the integration part as well. It's almost finalized!

Thanks, having an easy to apply list of necessary changes in order to test your PR would be very useful.

As I mentioned earlier, the API of that alarm is quite fragile so it would be great if we could get some additional manual testings from other users as well, before pushing this change in HA.

bigmoby · 2024-10-07T16:54:33Z

Hi @RyuzakiKK
so, I think I finished to implement the updated integration:
https://github.com/bigmoby/core/tree/feature/new-ialarm-functionalities

Since the new version of the library (2.3.0?!) has not been released yet, it is necessary to copy the library files into the integration directory.

RyuzakiKK · 2024-10-08T16:03:59Z

Thanks @bigmoby.

@andker87 if you are able to run the HA version that @bigmoby linked above, and copying the updated pyialarm from https://github.com/bigmoby/pyialarm/tree/master, in theory you should be able to test it.

If we can get some additional confirmation that this is indeed working fine (especially when the alarm is set to home or away), then we can definitely proceed more comfortably, knowing that it is not causing regressions.

bigmoby · 2024-10-08T17:33:40Z

Hi @RyuzakiKK , hi @andker87
unfortunately after I merge my code in the core-forked repo I wasn't able to run dev container! ...very sad!!!
So...I created this repo for a custom component integration: https://github.com/bigmoby/ialarm_controller
At the moment, it seems that almost everything is working, except for the invocation of the get_log service.

andker87 · 2024-10-09T11:07:47Z

I installed the custom component from hacs, and in the dashboard I put the two alarm_control_panels in parallel view (the first from the node-red flow and the second from the custom_component).
when I change the status from the first alarm, the second one takes several seconds to receive the status, often going to unavailable and becoming available again after several seconds (but bringing back the correct status)
when I issue arm commands from the second, the first one gets the state, but the second one goes unavailable for a few seconds. the disarm command on the other hand does not success because of the missing code.

bigmoby · 2024-10-09T11:19:01Z

Yes @andker87, I unfortunately noticed it too. Maybe two components pinging the alarm control panel are causing it to go into crisis?

RyuzakiKK · 2024-10-13T10:22:45Z

@andker87 and @bigmoby, did you try to run only this component in a single instance? In that "normal" scenario, does it always works fine?

bigmoby · 2024-10-13T14:45:37Z

Hi @RyuzakiKK , with the latest release (v0.0.4) it's better managed the response message. At moment the single point of failure of the library is the _is_complete_message() function. I should work in order to optimize it and manage bad xml response like

2024-10-13 16:42:19.741 ERROR (MainThread) [pyasyncialarm.pyasyncialarm] XML Parsing error: unclosed token: line 1, column 264
2024-10-13 16:42:19.750 ERROR (MainThread) [pyasyncialarm.pyasyncialarm] OSError occurred: Received malformed XML response
2024-10-13 16:42:19.753 ERROR (MainThread) [custom_components.ialarm_controller.coordinator] Error fetching ialarm_controller data: Received malformed XML response

it's a WIP project :-)

bigmoby · 2024-10-15T04:48:24Z

Hi guys,
For me, now, the integration (the latest version) is quite stable. I think I'll stop the development (...at moment).

Alarm status is updated every 15 seconds:

alias: Aggiorna antifurto
description: Aggiorna antifurto
triggers:
  - seconds: /15
    trigger: time_pattern
conditions: []
actions:
  - target:
      entity_id: alarm_control_panel.ialarm
    data: {}
    action: homeassistant.update_entity
mode: single

And with this blueprint automation I also solved the alerting feature in my mobile device (I suggest it):

alias: Avviso allarme casa per telefono Fabio
description: ""
use_blueprint:
  path: vorion/actionable-notifications-for-android.yaml
  input:
    notify_device: a8f2df4749801e4a5f15e57217763c66
    trigger_entity: input_boolean.alarm_trigger
    notification_title: Attenzione allarme casa
    notification_message: Allarme antifurto in casa!
    persistent_notification: true
    action_1_title: Disattiva allarme
    first_action:
      - action: alarm_control_panel.alarm_disarm
        metadata: {}
        data:
          code: "xxxxxxx"
        target:
          device_id: [blablabla]
    action_2_title: Attiva allarme
    second_action:
      - action: alarm_control_panel.alarm_arm_away
        metadata: {}
        data:
          code: "xxxxxxx"
        target:
          device_id: [blablabla]

RyuzakiKK · 2024-10-19T10:00:48Z

For me, now, the integration (the latest version) is quite stable. I think I'll stop the development (...at moment).

Is this the same version in this PR or is your custom component different?

If it is working fine, then we can proceed to merge it and update the official integration in HA I guess.

bigmoby · 2024-10-19T16:39:13Z

Hi @RyuzakiKK, unfortunately no, this PR is now outdated. The library has changed dramatically, there're different returning type and the integration itself should be migrated (yesterday I released the version 1.0.0 with so many improvements...). How would you like to proceed? How can I help?

RyuzakiKK · 2024-10-19T16:50:37Z

I didn't check the differences with your current library version.
If you think it is a major rewrite and you want to take ownership of that library, one option is to publish it on pypi and then creating a PR for HA to point it to your lib instead of this pyialarm one.
The iAlarm system is installed at my parents house, which makes it not trivial for me testing it.

So, if anyone wants to take on the maintenance of the component, I'm more than happy.

Instead, if you prefer to keep your changes here, I guess you can create a new pull request (or update this one) and I'll try to do a quick review of it.

bigmoby · 2024-10-19T17:08:13Z

I had to fork your project and create another one, renaming it in order to release a new library on PyPI for the new integration I developed.
Don't worry @RyuzakiKK , just let me know what you prefer! I created this project for the library: https://github.com/bigmoby/pyasyncialarm and this one for the new integration (available on HACS): https://github.com/bigmoby/ialarm_controller.
Personally, it's working very well for me! The new integration brings several additional features that make it possible to forget about using the manufacturer's mobile app.
Since (hopefully!) I'll be replacing this alarm system soon (it's little more than a toy... what a bad purchase I made! ...Or at least I hope there isn’t something worse out there!), I think that personally following developments on Home Assistant Core isn't the best path for me. I'd lean towards keeping it on HACS. But if you'd like to merge the work into the Core repository, I'm available to help!

RyuzakiKK · 2024-10-19T18:59:40Z

I think that personally following developments on Home Assistant Core isn't the best path for me. I'd lean towards keeping it on HACS.

As you prefer. Given that you already created a forked project for the library, and also published it on pypi, if you want to update the "official" ha integration you can probably update the HA component with the new APIs and point it to your library, instead of mine.
Once HA core is updated, there isn't really any maintenance burden.

However, if you don't want to spend time updating the HA core (given the amount of work you already put in place), we can also leave it as-is. Maybe in the future removing pyialarm from HA core and just pointing users to your HACS one.

bigmoby · 2024-10-25T11:14:55Z

Hi @RyuzakiKK ,
unless this issue will be solved (home-assistant/core#127886) I'm not able to merge and test my implementation in the old integration.

fixing trigger alert status and add some ancillary features

f6d84ac

bigmoby mentioned this pull request Oct 1, 2024

Triggered status is not shown without the "include_memory_feature" option #9

Open

bigmoby marked this pull request as draft October 2, 2024 11:06

bigmoby added 6 commits October 2, 2024 19:29

fixing trigger alert status and add some ancillary features

14a0859

fixing trigger alert status and add some ancillary features

9d9ba66

fixing trigger alert status and add some ancillary features

f3188ed

fixing trigger alert status and add some ancillary features

46d9d9c

fixing trigger alert status and add some ancillary features

aa733ba

fixing trigger alert status and add some ancillary features

5eca60d

bigmoby marked this pull request as ready for review October 2, 2024 20:54

fixing trigger alert status and add some ancillary features

13b7b0e

bigmoby mentioned this pull request Oct 3, 2024

Add API to get zone configuration and status #8

Open

RyuzakiKK reviewed Oct 5, 2024

View reviewed changes

fixs after review

f38b8bd

RyuzakiKK reviewed Oct 6, 2024

View reviewed changes

fixs after review. 2nd round

5b7cf92

Comments

Conversation

bigmoby commented Oct 1, 2024

Uh oh!

RyuzakiKK commented Oct 3, 2024

Uh oh!

andker87 commented Oct 3, 2024

Uh oh!

bigmoby commented Oct 3, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyuzakiKK commented Oct 5, 2024

Uh oh!

RyuzakiKK commented Oct 5, 2024

Uh oh!

bigmoby commented Oct 6, 2024

Uh oh!

bigmoby commented Oct 6, 2024

Uh oh!

bigmoby commented Oct 6, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RyuzakiKK commented Oct 6, 2024

Uh oh!

bigmoby commented Oct 7, 2024

Uh oh!

RyuzakiKK commented Oct 8, 2024

Uh oh!

bigmoby commented Oct 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andker87 commented Oct 9, 2024

Uh oh!

bigmoby commented Oct 9, 2024

Uh oh!

RyuzakiKK commented Oct 13, 2024

Uh oh!

bigmoby commented Oct 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bigmoby commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RyuzakiKK commented Oct 19, 2024

Uh oh!

bigmoby commented Oct 19, 2024

Uh oh!

RyuzakiKK commented Oct 19, 2024

Uh oh!

bigmoby commented Oct 19, 2024

Uh oh!

RyuzakiKK commented Oct 19, 2024

Uh oh!

bigmoby commented Oct 25, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

bigmoby commented Oct 8, 2024 •

edited

Loading

bigmoby commented Oct 13, 2024 •

edited

Loading

bigmoby commented Oct 15, 2024 •

edited

Loading