Add additional features to SAF Tehnika #4666

Merged
merged 6 commits into from Oct 6, 2016

Projects

None yet

4 participants

@guillemmateos
Contributor

Improve support for SAF Tehnika products by drawing additional information

@guillemmateos guillemmateos Improve support for SAF Tehnika products by drawing additional releva…
…nt information
5395759
@guillemmateos guillemmateos Add missing new line at the enf of file saf-mib.inc.php
21eede3
@laf laf changed the title from master to Add additional features to SAF Tehnika Sep 29, 2016
@guillemmateos guillemmateos * Rename some files to organize things properly
* Remove modulation graphing as it does not work as expected
* Include graphing for Temp and Voltage
* Include current ACM capacity
e307963
@guillemmateos guillemmateos * Prettify graphs
262a8be
@guillemmateos guillemmateos * Improve alignment of comments on graphs
5df10f4
+
+require 'includes/graphs/common.inc.php';
+
+$rrdfilename = $config['rrd_dir'].'/'.$device['hostname'].'/saf.rrd';
@laf
laf Oct 3, 2016 Member

Should be using rrd_name() to generate the filename.

+
+$rrdfilename = $config['rrd_dir'].'/'.$device['hostname'].'/saf.rrd';
+
+if (file_exists($rrdfilename)) {
@laf
laf Oct 3, 2016 Member

Need to use rrdtool_check_rrd_exists() to check for rrd files - rrdcached based ones may not have local files

+
+require 'includes/graphs/common.inc.php';
+
+$rrdfilename = $config['rrd_dir'].'/'.$device['hostname'].'/saf.rrd';
@laf
laf Oct 3, 2016 Member

Should be using rrd_name() to generate the filename.

+
+$rrdfilename = $config['rrd_dir'].'/'.$device['hostname'].'/saf.rrd';
+
+if (file_exists($rrdfilename)) {
@laf
laf Oct 3, 2016 Member

Need to use rrdtool_check_rrd_exists() to check for rrd files - rrdcached based ones may not have local files

+
+$rrdfilename = $config['rrd_dir'].'/'.$device['hostname'].'/saf.rrd';
+
+if (file_exists($rrdfilename)) {
@laf
laf Oct 3, 2016 Member

Need to use rrdtool_check_rrd_exists() to check for rrd files - rrdcached based ones may not have local files

+
+$rrdfilename = $config['rrd_dir'].'/'.$device['hostname'].'/saf.rrd';
+
+if (file_exists($rrdfilename)) {
@laf
laf Oct 3, 2016 Member

Need to use rrdtool_check_rrd_exists() to check for rrd files - rrdcached based ones may not have local files

+ d_echo($oids."\n");
+
+ if (!empty($oids)) {
+ echo 'SAF Temperature ';
@laf
laf Oct 3, 2016 Member

Why not move this to the next if (!empty($oids)) {

@guillemmateos
guillemmateos Oct 3, 2016 Contributor

Agreed! Just based my work to much on previous files from other brands and i should have been trying to improve the code as a whole! Will arrange it ASAP. Same for other comments regarding voltage and temperature discovery files

+ echo 'SAF Temperature ';
+ }
+
+ $divisor = 1;
@laf
laf Oct 3, 2016 Member

May as well move these two lines into the if (!empty($oids)) {

+ list(,$current) = explode(' ', $oids);
+ $index = $oid;
+ $descr = 'System Temp';
+ discover_sensor($valid['sensor'], 'temperature', $device, $oid, $index, $type, $descr, $divisor, '1', null, null, null, null, $current);
@laf
laf Oct 3, 2016 Member

When storing $oid this needs to be the numerical value like .1.3.6.1.2.3.4.5.6.

@guillemmateos
guillemmateos Oct 3, 2016 Contributor

I'm not sure I understand this one. So isn't using the MIB name the right way to do it if it does exist? or when you refer to numeric oid, you mean only when calling the discover_sensor and snmp_get is ok with MIB name?

@laf
laf Oct 6, 2016 Member

yeah snmp_get is ok but storing the OID needs to be numerical.

+<?php
+
+if ($device['os'] == 'saf') {
+ $oid = '1.3.6.1.4.1.7571.100.1.1.5.15.1.2.0';
@laf
laf Oct 3, 2016 Member

This OID needs to start with a ., i.e .1.3.6

+ d_echo($oids."\n");
+
+ if (!empty($oids)) {
+ echo 'SAF Voltage ';
@laf
laf Oct 3, 2016 Member

Move this + the other two variables into the if (!empty($oids)) { check

includes/polling/wifi.inc.php
@@ -16,6 +16,11 @@
include 'includes/polling/mib/siklu-mib.inc.php';
}
+ if ($device['os'] == 'saf') {
+ echo "It is SAF\n";
@laf
laf Oct 3, 2016 Member

Something like echo 'SAF Wireless' . PHP_EOL; would be much better.

@guillemmateos
guillemmateos Oct 3, 2016 Contributor

I just followed what had been written before on the echo. There is a number of if's and all them work exactly in the same way, but i agree we should change them to what you suggest, and probably do else if's instead of just a ton of if's considering a device is never gonna be 5 different brands. What do you think?

@laf
laf Oct 3, 2016 Member

Agreed 100%, mind updating to do that?

@guillemmateos
guillemmateos Oct 3, 2016 Contributor

Sure. I'll update that. Maybe better if I do it in a different pull request?

@laf
laf Oct 3, 2016 Member

No it's fine here, we squash on merge anyway

@laf laf added the Blocker label Oct 3, 2016
@guillemmateos guillemmateos * Correct some issues based on @laf suggestions
a93cee7
@scrutinizer-notifier

The inspection completed: 7 new issues

@laf laf removed the Blocker label Oct 6, 2016
@laf
laf approved these changes Oct 6, 2016 View changes
@laf laf added the Graphs label Oct 6, 2016
@laf laf merged commit c73ed98 into librenms:master Oct 6, 2016

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment