Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add door control API #90

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

drewclauson
Copy link

@drewclauson drewclauson commented Jun 3, 2021

Opening up PR to have a place to discuss adding in these capabilities.

  • Add API requests for basic door operations
  • Add Tests
  • Any further integration to work with HA
  • Squash commits?
  • Rebase after Improve typing #85 is merged.
  • Add event support
  • More event tests

@Kane610
Copy link
Owner

Kane610 commented Jun 4, 2021

Awesome! I'll have a look during the weekend

Copy link
Owner

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall it looks great, good job!

Is the whole door control API covered by your implementation? It seems some commands also can provide additional attributes such as AccessTime and OpenTooLongTime

Don't forget to look at events for door state changes so we get that warm fuzzy feeling of push notifications :)

axis/door_control.py Show resolved Hide resolved
axis/door_control.py Outdated Show resolved Hide resolved
axis/door_control.py Outdated Show resolved Hide resolved
tests/test_door_control.py Outdated Show resolved Hide resolved
@drewclauson
Copy link
Author

Don't forget to look at events for door state changes so we get that warm fuzzy feeling of push notifications :)

Yes, thanks for the reminder. I just discovered how this works through ONVIF, but need to head to bed for tonight. That part will be a bit easier than I originally thought. At first glance (and I'm new to ONVIF), but looks like adding some notification filters / topics.

@Kane610
Copy link
Owner

Kane610 commented Jun 8, 2021

Don't forget to look at events for door state changes so we get that warm fuzzy feeling of push notifications :)

Yes, thanks for the reminder. I just discovered how this works through ONVIF, but need to head to bed for tonight. That part will be a bit easier than I originally thought. At first glance (and I'm new to ONVIF), but looks like adding some notification filters / topics.

Hopefully it's as easy as extending https://github.com/Kane610/axis/blob/master/axis/event_stream.py with a new class

@drewclauson
Copy link
Author

drewclauson commented Jun 8, 2021

Don't forget to look at events for door state changes so we get that warm fuzzy feeling of push notifications :)

Yes, thanks for the reminder. I just discovered how this works through ONVIF, but need to head to bed for tonight. That part will be a bit easier than I originally thought. At first glance (and I'm new to ONVIF), but looks like adding some notification filters / topics.

Hopefully it's as easy as extending https://github.com/Kane610/axis/blob/master/axis/event_stream.py with a new class

I think this is the case and I've added the right topics, but when I run __main__ and watch the events come in as I trigger actions manually on the device, I'm not seeing anything related to the topics I added. Should the debug output log out all events that come in from the device or just ones that are in set up event_stream.py? Do you have any other tools that you use to capture events from a device for troubleshooting or anything like that?

@Kane610
Copy link
Owner

Kane610 commented Jun 8, 2021

I think this is the case and I've added the right topics, but when I run __main__ and watch the events come in as I trigger actions manually on the device, I'm not seeing anything related to the topics I added. Should the debug output log out all events that come in from the device or just ones that are in set up event_stream.py? Do you have any other tools that you use to capture events from a device for troubleshooting or anything like that?

I don't remember 😂 .

Have you enabled events with main?

parser.add_argument("--events", action="store_true")

This one should print all events
LOGGER.info(f"{action} {event}")

You can put a print here to debug

self.event(self.data)

@drewclauson
Copy link
Author

I think this is the case and I've added the right topics, but when I run __main__ and watch the events come in as I trigger actions manually on the device, I'm not seeing anything related to the topics I added. Should the debug output log out all events that come in from the device or just ones that are in set up event_stream.py? Do you have any other tools that you use to capture events from a device for troubleshooting or anything like that?

I don't remember 😂 .

Have you enabled events with main?

parser.add_argument("--events", action="store_true")

This one should print all events

LOGGER.info(f"{action} {event}")

You can put a print here to debug

self.event(self.data)

LOL and yes, I have --events enabled. I'm wondering if the device isn't sending all the events and I'm missing a configuration requirement for RSTP/ONVIF on the device. I'll keep digging. :-)

@Kane610
Copy link
Owner

Kane610 commented Jun 8, 2021

LOL and yes, I have --events enabled. I'm wondering if the device isn't sending all the events and I'm missing a configuration requirement for RSTP/ONVIF on the device. I'll keep digging. :-)

AFAIK it should support metadata events

@drewclauson
Copy link
Author

drewclauson commented Jun 9, 2021

I think I have events all configured, just need to add tests. Also, I did finally figure out how to get the events to log out when running with --events. I suspect it was actually a firewall issue as I was accessing my door controller through a firewall. Still more work to do, but I'm figuring some things out.

Remind me - I can't find the comment you previously made - is this the right spot to load/configure/include/not-sure-the-right-word the door_control.py stuff?

https://github.com/drewclauson/axis/blob/fed1fdfd71815f42299a95aaa78088d253209103/axis/vapix.py#L144

Or do I need to include something like how the light_control does it?

https://github.com/drewclauson/axis/blob/fed1fdfd71815f42299a95aaa78088d253209103/axis/vapix.py#L184

@Kane610
Copy link
Owner

Kane610 commented Jun 9, 2021

Remind me - I can't find the comment you previously made - is this the right spot to load/configure/include/not-sure-the-right-word the door_control.py stuff?

https://github.com/drewclauson/axis/blob/fed1fdfd71815f42299a95aaa78088d253209103/axis/vapix.py#L144

Or do I need to include something like how the light_control does it?

https://github.com/drewclauson/axis/blob/fed1fdfd71815f42299a95aaa78088d253209103/axis/vapix.py#L184

It depends ;)

If the oldest device available (A1001 in this case) supports API discovery in its latest release, then it is fine to keep it to just be API discovery. But if the A1001 has a too old firmware to work with API discovery then you need to support the older mechanism to identify if it is supported or not. Hope that is clear

@Kane610
Copy link
Owner

Kane610 commented Jun 15, 2021

Hey! Press the re-request review button when you want my attention.

@Kane610
Copy link
Owner

Kane610 commented Jun 21, 2021

Hey @drewclauson, everything ok?

@drewclauson
Copy link
Author

drewclauson commented Jun 21, 2021 via email

@Kane610
Copy link
Owner

Kane610 commented Jun 21, 2021

Take your time! I will go on vacation next week so might be a bit less active at times

@drewclauson
Copy link
Author

I'll be coming back to this soon - had a death in the family that made things very busy for a month.

@Kane610
Copy link
Owner

Kane610 commented Jul 23, 2021

I'll be coming back to this soon - had a death in the family that made things very busy for a month.

My condolences.

I appreciate the status update but don't feel rushed that I'm expecting preogress. I have a couple of bug reports that I will try to solve before going back to working on Axis integration.

@drewclauson drewclauson requested a review from Kane610 July 31, 2021 18:38
@drewclauson
Copy link
Author

drewclauson commented Jul 31, 2021

@Kane610 - I came back to this and fixed the failing flake8 statement. I think this encapsulates the functionality that I think would be beneficial for an A1001 door controller integration, but there might be other API calls that could be helpful. The majority of the Axis door controller API has to do with configuring the controller and I focused on the ones that are more about the day to day operation of the door (lock/unlock/access/etc). My specific use case is just to be able to have a button in HA that will let me click it to open it temporarily, but I tried to go much further outside that into the other API operations.

Let me know what you think! I'm finally back at a place where I can put a bit more time into it.... If I can get past the flake8 guardian 😂

@Kane610
Copy link
Owner

Kane610 commented Aug 5, 2021

I'll have a look during the next week. I think regardless of what additional apis to support to keep the PR to one api.

What I think would be interesting is card reader / access panel support. Possibly user support.

Copy link
Owner

@Kane610 Kane610 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, work came back with a wham! after vacation and forgot about this

"""Door Control API.

The Axis Door control API makes it possible to control the behavior and functionality
of physical access controls in the Axis devices (e.g. A1001, A1601)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove device examples, access control is enough, I think video door stations might be applicable here as well and I don't want to keep updating the list.

"""Initialize door control manager."""
super().__init__({}, request, URL, Door)

# TODO: Question: Is this used to get status information for the door? Or to update the object properties?
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Time to remove it? ;)

"""Capabilities of Door."""
return self.raw["Capabilities"]

async def is_capable_of(self, capability: str) -> bool:
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats the reasoning behind this? When will this be used? It doesnt seem to have test coverage when looking for the method

@@ -4,6 +4,8 @@

DAYNIGHT_INIT = b'<?xml version="1.0" encoding="UTF-8"?>\n<tt:MetadataStream xmlns:tt="http://www.onvif.org/ver10/schema">\n<tt:Event><wsnt:NotificationMessage xmlns:tns1="http://www.onvif.org/ver10/topics" xmlns:tnsaxis="http://www.axis.com/2009/event/topics" xmlns:wsnt="http://docs.oasis-open.org/wsn/b-2" xmlns:wsa5="http://www.w3.org/2005/08/addressing"><wsnt:Topic Dialect="http://docs.oasis-open.org/wsn/t-1/TopicExpression/Simple">tns1:VideoSource/tnsaxis:DayNightVision</wsnt:Topic><wsnt:ProducerReference><wsa5:Address>uri://1c8ae81b-3b00-46cf-bf76-79cc3fa533dc/ProducerReference</wsa5:Address></wsnt:ProducerReference><wsnt:Message><tt:Message UtcTime="2019-02-06T18:58:51.007104Z" PropertyOperation="Initialized"><tt:Source><tt:SimpleItem Name="VideoSourceConfigurationToken" Value="1"/></tt:Source><tt:Key></tt:Key><tt:Data><tt:SimpleItem Name="day" Value="1"/></tt:Data></tt:Message></wsnt:Message></wsnt:NotificationMessage></tt:Event></tt:MetadataStream>\n'

DOOR_MODE_INIT = b'<?xml version="1.0" encoding="UTF-8"?>\n<tt:MetadataStream xmlns:tt="http://www.onvif.org/ver10/schema">\n <tt:Event><wsnt:NotificationMessage xmlns:tns1="http://www.onvif.org/ver10/topics" xmlns:tnsaxis="http://www.axis.com/2009/event/topics" xmlns:wsnt="http://docs.oasis-open.org/wsn/b-2" xmlns:wsa5="http://www.w3.org/2005/08/addressing"><wsnt:Topic Dialect="http://docs.oasis-open.org/wsn/t-1/TopicExpression/Simple">tns1:Door/State/DoorMode</wsnt:Topic><wsnt:ProducerReference><wsa5:Address>uri://755cc9bb-cf3a-410b-bd1b-0ec97c6d6256/ProducerReference</wsa5:Address></wsnt:ProducerReference><wsnt:Message><tt:Message UtcTime="2020-09-05T04:25:51.692744Z" PropertyOperation="Initialized"><tt:Source><tt:SimpleItem Name="DoorToken" Value="Axis-5fba94a4-8601-4627-bdda-cc408f69e026"/></tt:Source><tt:Key></tt:Key><tt:Data><tt:SimpleItem Name="state" Value="Accessed"/></tt:Data></tt:Message></wsnt:Message></wsnt:NotificationMessage></tt:Event></tt:MetadataStream>\n'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you generate additional events? The better coverage and examples the easier it will be to maintain

assert item.door_name == "Test Door 1"
assert item.door_description == "Test Door 1 Description"

# TODO: Check to see that comparing dict using "==" actually does what I want it to do
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dictionaries are sorted since python 3.6 I think so this should be fine

@@ -55,66 +56,66 @@ def event_manager(axis_device) -> EventManager:
[
(FIRST_MESSAGE, {}),
(
PIR_INIT,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whats happening with this extra indentation?

@@ -12,6 +12,7 @@
FIRST_MESSAGE,
AUDIO_INIT,
DAYNIGHT_INIT,
DOOR_MODE_INIT,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add coverage for all events



@respx.mock
async def test_update(door_control):
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All previous tests have a @pytest.mark.asyncio descriptor, if you can pin point what dependency you have in your system that rids that dependency please add it to this PR so I can remove the descriptor.

@Kane610
Copy link
Owner

Kane610 commented Sep 20, 2021

Hey @drewclauson everything ok?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants