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

Added support for new ISAPI urls #49

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

Conversation

ayushpratap
Copy link

get_device_status
get_device_time
get_get_upnp_ports_status

get_device_status
get_device_time
get_get_upnp_ports_status
@ayushpratap
Copy link
Author

I have tested these changes with two Hikvision DVRs I have with, cameras of Hikvision, CP Plus, Dahua.,

@mezz64
Copy link
Owner

mezz64 commented Jan 11, 2020

I'm on board with the new endpoints, but things need streamlined a bit. The current intent of the library is that initialize is always called first which should define some of the items you are repeating in each function (digest check in particular). I also don't see the need to add the beautiful soup dependency when everything can be done with the existing XML tree functions.

@ayushpratap
Copy link
Author

ayushpratap commented Jan 13, 2020

@mezz64 Okay we can streamline the library. I used beautiful soup because of its flexibility and ease of use.
Drop me a mail with your approach on ayushs56@gmail and we can move forward from there

@ayushpratap
Copy link
Author

I have removed the BeautifulSoup dependency from the new added URLs

Copy link
Owner

@mezz64 mezz64 left a comment

Choose a reason for hiding this comment

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

I did a more thorough review to help let you know what changes need made to bring these additions in-line with the rest of the code base.

@@ -242,6 +242,7 @@ def initialize(self):
device_info = self.get_device_info()

if device_info is None:
print('Hello this is it')
Copy link
Owner

Choose a reason for hiding this comment

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

Remove.

Copy link
Author

Choose a reason for hiding this comment

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

Removed this unwanted print statement.

@@ -401,6 +402,7 @@ def get_device_info(self):
except (requests.exceptions.RequestException,
requests.exceptions.ConnectionError) as err:
_LOGGING.error('Unable to fetch deviceInfo, error: %s', err)
print('This is test')
Copy link
Owner

Choose a reason for hiding this comment

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

Remove.

Copy link
Author

Choose a reason for hiding this comment

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

Removed this unwanted print statement

@@ -430,6 +432,182 @@ def get_device_info(self):
_LOGGING.error('There was a problem: %s', err)
return None

"""
This function is added by Ayush Pratap Singh ([email protected])
Copy link
Owner

Choose a reason for hiding this comment

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

Attribution comments aren't needed. You'll be the owner on the merged PR.

Copy link
Author

Choose a reason for hiding this comment

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

Removed attribution comment

"""Parse deviceInfo into dictionary."""
device_status = {}
url = '%s/ISAPI/System/status' % self.root_url
using_digest = False
Copy link
Owner

Choose a reason for hiding this comment

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

Resetting the digest flag isn't needed. It will be set once the initialize function is called.

Copy link
Author

Choose a reason for hiding this comment

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

Removed resetting of digest flag


try:
response = self.hik_request.get(url, timeout=CONNECT_TIMEOUT)
if response.status_code == requests.codes.unauthorized:
Copy link
Owner

Choose a reason for hiding this comment

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

Use the using_digest variable to determine what auth type to use.

# Seems to be difference between camera and nvr, they can't seem to
# agree if they should 404 or 401 first
if not using_digest and response.status_code == requests.codes.unauthorized:
_LOGGING.debug('Basic authentication failed. Using digest.')
Copy link
Owner

Choose a reason for hiding this comment

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

Again, use the variable to decide.

try:
tree = ET.fromstring(response.text)

device_status['currentdevicetime'] = tree[0].text.strip()
Copy link
Owner

Choose a reason for hiding this comment

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

Use the element_query function to find the fields you want. This allows the XML ordering to change and you can still return the correct result.

_LOGGING.error('There was a problem: %s', err)
return None

def get_device_time(self):
Copy link
Owner

Choose a reason for hiding this comment

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

Implement same changes on this function as the previous one.

Get the Ports using the endpoint : /ISAPI/System/Network/UPnP/ports/status
"""
#TODO : This needs to be implemented
def get_upnp_ports_status(self):
Copy link
Owner

Choose a reason for hiding this comment

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

Implement same changes on this function as the previous one.

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.

None yet

2 participants