search

tchellomello / python-amcrest

A Python 2.7/3.x module for Amcrest and Dahua Cameras using the SDK HTTP API.
GNU General Public License v2.0
214 stars 74 forks source link

Add is_alarm_triggered to check for AlarmLocal events #144

Closed lonestriker closed 4 years ago

lonestriker commented 4 years ago

Helper function to check for AlarmLocal condition. AlarmLocal is similar to the VideoMotion event but triggers on what the camera considers a local alarm rather than just video motion. For the Amcrest AD110 Video Doorbell, AlarmLocal seems to be what's used for it's combined PIR motion + VideoMotion alerts that are sent to the smartphone app. VideoMotion is too noisy and triggers any time there is enough motion in the video image itself, without needing any actual PIR-triggered motion.

coveralls commented 4 years ago

Coverage Status

Coverage increased (+0.006%) to 31.722% when pulling 0b09db17e4e6164738d2d7d11dace381b579bafe on lonestriker:master into 6db2d9cac09df0414b8ad29c04c276368e348210 on tchellomello:master.

lonestriker commented 4 years ago

It's out of habit and readability. But yes, it's the same without the bool() so I would not object if it's removed. The other function that does the same thing should be changed to match the same convention as well (condense the if/else into the single return). I didn't want to make just stylistic changes to the other code for my PR.

Thanks!

Hien

On Tue, Jan 21, 2020 at 3:25 PM Phil Bruckner notifications@github.com wrote:

@pnbruckner requested changes on this pull request.

In src/amcrest/event.py https://github.com/tchellomello/python-amcrest/pull/144#discussion_r369250467 :

@@ -144,6 +144,11 @@ def is_motion_detected(self): return False return True

  • @property
  • def is_alarm_triggered(self):
  • event = self.event_channels_happened('AlarmLocal')
  • return bool('channels' in event)

Why not just return 'channels' in event? The result of that expression would already be a bool.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tchellomello/python-amcrest/pull/144?email_source=notifications&email_token=ACTGFGODHE54LBRIC4DQH7LQ65R2XA5CNFSM4KITCDLKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCSRJZRA#pullrequestreview-346201284, or unsubscribe https://github.com/notifications/unsubscribe-auth/ACTGFGLKVP2PWI3OVIIQLZLQ65R2XANCNFSM4KITCDLA .

pnbruckner commented 4 years ago

I guess as long as you have a reason. It's not my style, but probably not worth changing at this point. And, yes, the other method probably shouldn't be the way it is, but that was before my time.

I'll go ahead and approve the change and merge it. Let me know if you need a release, or if this can just wait for the next release, whenever that might be.