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 HikVision-DS Cameras #9980

Merged
merged 2 commits into from Mar 27, 2019

Conversation

Projects
None yet
5 participants
@spencerbutler
Copy link
Contributor

commented Mar 17, 2019

fixes #9950

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@spencerbutler spencerbutler referenced this pull request Mar 17, 2019

Closed

HikVision DS-2CD2185FWD-I #9950

3 of 3 tasks complete

@spencerbutler spencerbutler force-pushed the spencerbutler:feature/lnms-9950 branch from 0a56176 to b7b4408 Mar 18, 2019

oops, didn't check tests first

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

@spencerbutler The mempool poller doesn't seem to be working in tests. After that is fixed this looks good to go.

@laf

This comment has been minimized.

Copy link
Member

commented Mar 18, 2019

Any reason this hasn't just been added to the hikvision support that exists rather than a new OS?

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

Any reason this hasn't just been added to the hikvision support that exists rather than a new OS?

@laf because the hikvision.yaml uses a different enterprise OID for their NVRs. I can combine them, if that is a better solution.

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Mar 18, 2019

@spencerbutler The mempool poller doesn't seem to be working in tests. After that is fixed this looks good to go.

@murrant all tests are now passing.

@murrant

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

@spencerbutler do they use different mibs too?

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@spencerbutler do they use different mibs too?

I will investigate and report back.

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

@spencerbutler do they use different mibs too?

I will investigate and report back.

https://p.libren.ms/view/b02e1efa
Using the snmprec file from librenms-snmpsim for the NVR device (the one in tests/snmpsim is the same OID but only 8 lines) it is clear these 2 variants want different MIB files. I'll see if I can locate the proper MIB files for the NVR model. I did not see any polling data when I added this device to my test instance. I will dig further.

Does it make sense to define these as os: hikvision variants: nvr,cam?

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Mar 19, 2019

https://p.libren.ms/view/7c490aa7

I had the MIB (HIKVISION-MIB) but didn't add it because it's broken. After commenting out a couple dozen lines, I was able to get it to return some usable information for the NVR device (enterprise 50001). I might as well just rewrite the $#@! MIBs...

Added support for HikVision-DS Cameras
clean up

removed pointless file, cleaned up code

fixed failing unit tests, a bit of cleanup

WIP - will rebase

split hikvision into hikvision-nvr an hikvision-cam

@spencerbutler spencerbutler force-pushed the spencerbutler:feature/lnms-9950 branch from 3d32c69 to 34cd8cb Mar 20, 2019

@spencerbutler

This comment has been minimized.

Copy link
Contributor Author

commented Mar 21, 2019

As far as I'm concerned, this PR is done. Feel free to tell me I'm wrong, and more needs to be done. Just don't leave out the "this needs to be done" bit.

@jozefrebjak

This comment has been minimized.

Copy link
Contributor

commented Mar 22, 2019

It is working for my NVR, i think it will be fine to change a logo of hikvision too. And what I am looking, there is possibilty to add some sensors for NVR. When it will be this merged, than i would like to add some sensors for NVR.

@murrant murrant merged commit 7bfe0bc into librenms:master Mar 27, 2019

1 of 2 checks passed

codeclimate 2 issues to fix
Details
license/cla Contributor License Agreement is signed.
Details

@spencerbutler spencerbutler deleted the spencerbutler:feature/lnms-9950 branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.