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 support for Mirkrotik Wireless Wire (wAP 60G) #9318

Merged
merged 26 commits into from Oct 20, 2018

Conversation

Projects
None yet
3 participants
@takenalias
Contributor

takenalias commented Oct 12, 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
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.

Fixes: #8933

takenalias added some commits Oct 12, 2018

@takenalias takenalias referenced this pull request Oct 12, 2018

Closed

MikroTik wAP 60G #8933

@laf

This comment has been minimized.

Member

laf commented Oct 12, 2018

Any reason you've done this as a new OS rather than just adding it to the existing routeros file?

@takenalias

This comment has been minimized.

Contributor

takenalias commented Oct 12, 2018

The wireless side of it used different OIDs to the rest of the Mikrotik stuff. Also the way the existing wireless sensor php one was working, as the data is stored in a table, it would pull indexes that have erroneous values.

It's a bit like the ubiquity airfibre. It's the same but different.

@laf

This comment has been minimized.

Member

laf commented Oct 13, 2018

The wireless side of it used different OIDs to the rest of the Mikrotik stuff

That doesn't matter. You can just add the new OIDs to routeros.

Also the way the existing wireless sensor php one was working, as the data is stored in a table, it would pull indexes that have erroneous values.

I don't understand what you mean by this.

Added support to existing Routeros.PHP
Upon talking to Laf, he advised to add support to existing php rather than create a new OS. Working hard I have done this method instead. I would like some one to test this who also has a normal mikrotik wifi AP to ensure I have not broken existing products.
@takenalias

This comment has been minimized.

Contributor

takenalias commented Oct 14, 2018

Ok So I worked on this some more, Seems to be working. I am sure glad my day job is not coding. See what everything thinks.

takenalias added some commits Oct 15, 2018

added state sensor support
Added state sensors for connection type and connection state.
@laf

A few changes need to be made on this now :)

@@ -54,7 +56,7 @@ public function discoverWirelessCcq()
$sensors = array();
foreach ($data as $index => $entry) {
// skip sensors with no data (nv2 should report 1 client, but doesn't report ccq)
if ($entry['mtxrWlApClientCount'] > 0 && $entry['mtxrWlApOverallTxCCQ'] == 0) {
if (($entry['mtxrWlApClientCount'] > 0 && $entry['mtxrWlApOverallTxCCQ'] == 0)) {

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

You don't need double brackets around this. Remove one of the sets.

);
}

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

You should do } else {

else{
$this->discoverSensor(

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

this needs a return before the $this

'frequency',
'mtxrWlApFreq',
'.1.3.6.1.4.1.14988.1.1.1.3.1.7.'
);

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

A lot of your formatting is now off. Please use four spaces for indentation so it's all aligned correctly. See travis for a list of issues: https://travis-ci.com/librenms/librenms/jobs/151701943#L721

@@ -153,7 +187,10 @@ public function discoverWirelessRate()
private function fetchData()
{
if (is_null($this->data)) {
$this->data = snmpwalk_cache_oid($this->getDevice(), 'mtxrWlApTable', array(), 'MIKROTIK-MIB');
$wl60 = snmpwalk_cache_oid($this->getDevice(), 'mtxrWl60GTable', array(), 'MIKROTIK-MIB');
$wl = snmpwalk_cache_oid($this->getDevice(), 'mtxrWlApTable', array(), 'MIKROTIK-MIB');

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

You can pass the previous array to snmpwalk_cache_oid so like:

            $wl = snmpwalk_cache_oid($this->getDevice(), 'mtxrWl60GTable', array(), 'MIKROTIK-MIB');
            $wl  = snmpwalk_cache_oid($this->getDevice(), 'mtxrWlApTable', $wl, 'MIKROTIK-MIB');
            $this->data = $wl;
@@ -174,8 +214,21 @@ private function discoverSensor($type, $oid, $num_oid_base)
'SSID: ' . $entry['mtxrWlApSsid'],
$entry[$oid]
);
}
}

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

} else {

@@ -1,15 +0,0 @@
os: routeros

This comment has been minimized.

@laf

laf Oct 15, 2018

Member

Why have you deleted this file?

This comment has been minimized.

@takenalias

takenalias Oct 15, 2018

Contributor

That was when I had modified it when creating the second second OS, as that wasn't needed I deleted that file so it would not be included in the PR

This comment has been minimized.

@laf

laf Oct 16, 2018

Member

Without that file, routeros will not work at all. You'll need to restore it.

takenalias added some commits Oct 16, 2018

Syntax fixes
Probably more to come. See what Travis says.

takenalias and others added some commits Oct 16, 2018

takenalias added some commits Oct 19, 2018

Show resolved Hide resolved LibreNMS/OS/Routeros.php

@laf laf removed the User-Pending label Oct 20, 2018

@laf

laf approved these changes Oct 20, 2018

LGTM

@laf laf merged commit e5f7969 into librenms:master Oct 20, 2018

2 of 3 checks passed

codeclimate 7 issues to fix
Details
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details

@takenalias takenalias deleted the takenalias:evan-patch2 branch Oct 21, 2018

@murrant

This comment has been minimized.

Member

murrant commented Oct 28, 2018

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-45-release-changelog-october-2018/6016/1

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