fix #153 and fix #154

[heifisch]

No description provided.

[tahvane1]

First of all thanks for input!
I went through pull request briefly.

Few concerns:
About adding self._cmd_list. To me it seems that it only is used to filter out identical commands from list (regardless of order). Can this cause issues (like c1 c2 c1 c3 would end up c1 c2 c3 and potentially change functionality) and is it really needed? Program is also threaded. Will self._send_cmd and self.update_devices cause issues?

To me it seems that now self._active_devices is not anymore active devices but all devices (and device contains active flag, but there is already self._devices for that) which are then updated from self._active_devices_tmp (activations since last update). Could this actually be solved without adding self._active_devices_tmp at all (just removing from self._active_devices and not clearing all triggers)?

@mattsaxon do you have comments on this?

[heifisch]

First of all, sorry if the text is incomprehensible. Unfortunately I have to use Google translator.

It's quite possible that I didn't fully understand the original code, it's my first attempt at Python.

When analyzing the log file, I noticed that the status "Triggered detector (multiple)" made a lot of detailed requests. These looked a bit uncoordinated to me. That's why I reduced these requests first.
This allowed me to better recognize a structure in the log file.

A sequence would be e.g.:
Reception: "Triggered detector (multiple)"
Send: "Details"
Reception: "Device 1"
Send: "Details"
Reception: "Device 2"
Send: "Details"
Reception: "Device 3"
Send: "Details"
Reception: "accepted_prefix"

I didn't want to interrupt this sequence with another request.
That's why I increased "retries".
"self._cmd_list" only exists because you cannot check whether the command is already contained in "_cmd_q".
I couldn't think of anything else to trigger an update_devices at the end of the sequence.

Whether this is necessary or whether there is another way, I cannot say. I don't understand the original code well enough.

I couldn't see in the code how to set the state in self._devices. That's why I got stuck at self._active_devices.

So don't consider this solution perfect.
Maybe it's the wrong way, if it works in this case too.
I'm a beginner with Python!

[heifisch]

I'll try to answer the questions.

"self._cmd_list" is not necessary.
It then also works with retries = 2.

[tahvane1]

No worries.
I think we can't use these (and related code)

• self._cmd_list = []
• self._send_cmd = None
  as these can/will cause concurrency issues (modified/read by two different threads). All communication between threads should happen via queue or by using locks.

I also think that self.update_devices is not needed.

I think issue is now with this part that prevents sending "unnecessary" queries
elif activity == 0x10:
# permanent trigger during standard (unset) mode, e.g. a door open detector
activity_name = 'Triggered detector'
# something is active
if detail == 0x00:
# don't send query if we already have "triggered detector" displayed
if activity_name not in self.statustext.message or activity_name == self.statustext.message:
self._send_device_query()
else:
log = False

and this part
elif activity == 0x16:
activity_name = 'Triggered detector (multiple)'
# multiple things are active
if detail == 0x00:
# no details... ask..
self._send_device_query()
else:
self._activate_source(detail)
self._confirm_device_query()

I guess this PR fixes this issues via self._force_query which basically tells that if Jablotron sends "multiple detectors triggered" then ignore check in first function (so send always query). I'm just wondering what this multiple triggers detected returns in this case (in detail or after self._send_device_query()).

I will be on vacation for next couple of weeks, but we will get this fixed...

Mistake

[mattsaxon]

I will try to review this in the next couple of days to see if I can remember how all this was intended to work...... I agree the retries logic is a complicated area of the code. Whilst not the heat structured, I do remember that it was solving a real problem I had.

[fabianflanhardt]

Hi, any update on this? Got the same issue, too and the change here does seem to fix it somehow. Would be nice if you could pick it up again.

[raducotescu]

I'm running a locally modified version of 0.32 with this patch applied, but I'd like to be able to update via HACS to get the latest updates. I haven't noticed any issues with this code and I've been running it for some time now.

[mattsaxon]

@tahvane1 , my suggestion is we merge this and test in parallel to 0.34. This will also give us a solid baseline to work on issues #163

[mattsaxon]

I also suggest to release this as 0.35, so all 4 of us can test on the same code base.

[ondrejmirtes]

I've been also affected by #153. I've just tested this patch on my system and it definitely did something! Even with multiple windows opened (wireless sensors), any opening/closing is reported almost immediately.

But possibly there are some side effects - sometimes the integration will report a window being closed and opened again right afterwards, without me actually touching the window physically. This is how it looks like in the logbook:

This can mess up some automations based on these windows being closed/opened.

I have these wireless devices connected to my control panel:

• 8 glass break detectors
• 2 door sensors
• 10 window sensors

I'm also going to open some new issues this integration has when there are multiple active sensors. These has been discussed at length in #163 - in summary, the integration worked for me flawlessly for a few weeks, and then issues started popping up, all causing weird behaviour with multiple open sensors.

Seeing other issues, and this PR, looks like it was more unusual that the integration worked for me the first few weeks, rather than it stopped working afterwards. So I'm glad I'm not the only one with these problems, because I love this integration (when it works :)).

[ondrejmirtes]

I've opened the issues:

• Wireless keypad spammed when multiple wireless sensors are active #167
• Not able to arm/disarm alarm reliably when multiple wireless sensors are active #168

[mattsaxon]

Since this a PR, this should really be opened as a separate issue and assigned to @heifisch

[ondrejmirtes]

Alright, did just that: #169

[tahvane1]

@mattsaxon You can do release if you wish