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

newdevice: Add Support for cambium 58500 #7998

Merged
merged 16 commits into from Jan 16, 2018

Conversation

Projects
None yet
5 participants
@pheinrichs
Contributor

pheinrichs commented Jan 2, 2018

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

pheinrichs added some commits Jan 2, 2018

Add Support for cambium-ptp500
Added mibs for ptp500, ptp300
Update label on 500 to 500/300
300 and 500 use the same mibs
'ssr',
$this->getDeviceId(),
$ssr,
'ptp500',

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

Should set this to something better like the others. Bit more descriptive and unique.

-
oid: transmitModulationMode
num_oid: .1.3.6.1.4.1.17713.5.12.9.
index: 0

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

You should use something better for the index in all of these. 'transmitModulationMode.{{ $index }}' is what we would normally do.

icon: cambium
mib_dir: cambium\500
over:
- { graph: device_bits, text: 'Device Traffic' }

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

You can add some of the wireless graphs here to make it a little more informative for users. Max 4 I think.

text: Cambium PTP 300/500
type: wireless
icon: cambium
mib_dir: cambium\500

This comment has been minimized.

@laf

laf Jan 5, 2018

Member

Should this not be cambium/500?

Request changes
Added overhead graph's to all current ptp devices
@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Jan 8, 2018

@laf I'm a little confused why the CI is failing with the snmp tests for airos-af. The tests passed on my local machine(although i don't have the past three days of PR's on my branch)
I've looked through the discovery and it looks ok, do you see something I've done? Should I add another condition for discovery?

Fixed... forgot to uppercase 'ID' in discovery ¯_(ツ)_/¯

@laf

This comment has been minimized.

Member

laf commented Jan 8, 2018

Thanks for sorting this out @pheinrichs. Looks good to me now.

Would be helpful if you could add some additional test data so that we can make sure things don't get broke later on: https://docs.librenms.org/#Developing/os/Test-Units/#example-workflow

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Jan 9, 2018

@laf Gladly, It's only doing discovery by sysObjectID right now(the exact OID) so there wouldn't be any variants to detect other than the specific ones. What else should i throw in to test?

@murrant

This comment has been minimized.

Member

murrant commented Jan 9, 2018

@pheinrichs save-test-data.php will collect data for all (supported) modules, not just os discovery.

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Jan 10, 2018

@murrant We may have to get someone with these devices to do so then. I don't have any available devices to run this on specifically.

@Linux-Maverick

This comment has been minimized.

Linux-Maverick commented Jan 10, 2018

@pheinrichs I performed a github-apply and tried doing some manual rediscovery. PTP800 seemed to come up ok but not the PTP600. On my local server I then replaced all instances of "17713.1" with "17713.6" contained in LibreNMS/OS/Ptp600.php and includes/definitions/discovery/ptp600.yaml. I also created a mibs/cambium/600 directory and uploaded the missing PTP600 v1 and v2 MIB and txt files. Now they at least look right after discovery. I'm currently watching the data being collected to see if it is correct.

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Jan 11, 2018

@Linux-Maverick Thanks for checking this. For some reason i thought they were already added. I'll update this right away!

@Linux-Maverick

This comment has been minimized.

Linux-Maverick commented Jan 11, 2018

@pheinrichs Great work, Paul! I have a few more suggested changes based on my testing. Bear in mind I'm focusing on the PTP600 and PTP800 so I can't speak to the other models but they probably apply as well:

  • The TX, RX and Aggregate rate values reported by the radios are given in kbps. The OS files should reflect this by changing the "multiplier, divisor" entries from 1, 100 to 1000, 1 to give the final value in bps. I've tested this on my local files and it works.
  • While you're in that area of the OS files, you might want to edit the typo so that the label for the graph is 'Aggregate'. :)
  • The Signal Strength Ratio is reported by the radios in tenths of a dB. The OS files need to have a "multiplier, divisor" entry of "1, 10" appended to the WirelessSensor declaration to view this in dB. I've changed this on my local files, it works, but the data looks rather boring (steady at 30.0 dB) on my setup because I'm only using single polarity antennas. ;)

Have you considered collecting / graphing 'Vector Error'?

--Brett

@pheinrichs

This comment has been minimized.

Contributor

pheinrichs commented Jan 11, 2018

@Linux-Maverick Great work, i'll fix the above! As for the vector error it's not a bad idea but currently I don't think there are any real wireless sensors that 'fit' the category. So possibly in another PR we can add a new sensor to categorise this correctly.

@Linux-Maverick

This comment has been minimized.

Linux-Maverick commented Jan 11, 2018

@pheinrichs Hey, this looks good! Doing away with the modulation mode was a good idea; it doesn't graph all that well. I've pulled it down, discovered, and run for a little bit and like what I see!

@laf

laf approved these changes Jan 11, 2018

@laf

This comment has been minimized.

Member

laf commented Jan 11, 2018

So I'm fine with this now. The only thing is the mib file names need to be fixed. @murrant has just added a script you can use (you'll need to rebase this to get it) then run php ./scripts/rename-mibs.php mibs/cambium/300/ mibs/cambium/400/ mibs/cambium/600/

If you could also commit chmod +x scripts/rename-mibs.php that would be ace.

@murrant

This comment has been minimized.

Member

murrant commented Jan 12, 2018

Heh, I did not +x the rename script because I thought it wouldn't be used very often and that prevents it from tab completing. I was probably wrong. 😃

But you need to also add #!/usr/bin/env php to the top of the file.

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Jan 12, 2018

The inspection completed: 2 new issues, 11 updated code elements

@laf laf merged commit d4bd566 into librenms:master Jan 16, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@laf

This comment has been minimized.

Member

laf commented Jan 16, 2018

Thanks very much as always @pheinrichs :)

@laf

This comment has been minimized.

Member

laf commented Jan 16, 2018

Btw, you hadn't created the json data from the test data, only just spotted that. The snmprec files also don't contain all the info from the support you added like wireless sensors.

Would you be able to submit a new PR with the additional data in the snmprec? Running ./scripts/save-test-data.php -o ptp500 after should be enough to generate the json data.

@lock

This comment has been minimized.

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.