Allow lmsensor fanspeeds of 0 to be discovered. #3616

Merged
merged 2 commits into from Jun 10, 2016

Projects

None yet

5 participants

@keeperofdakeys
Contributor

Devices that have a fan that spins down are currently undiscovered during discovery. Allow the fanspeed to be 0 to fix this.

@Rosiak
Contributor
Rosiak commented Jun 8, 2016

I agree on the issue raised, not sure of the impact tho' @librenms/reviewers ?

@keeperofdakeys
Contributor

I have a device that has a fan that isn't always on, only being spun up when it gets hot. Without this patch, it only gets discovered when it's spinning, and gets undiscovered when not spinning. This patch allows fan speeds of 0 to be discovered.

@paulgear
Member
paulgear commented Jun 8, 2016

Patch seems reasonable to me; I guess we need to test whether the discovery of missing fan sensors still works correctly.

@laf
Member
laf commented Jun 9, 2016

Think this should just be if ($current >= 0) { imho.

Aside from that I'd say +1 for a merge.

@keeperofdakeys keeperofdakeys I agree to the conditions of the Contributor Agreement contained
in doc/General/Contributing.md.
871f2d7
@keeperofdakeys
Contributor

When no sensor info is found, snmp_get returns false. Unfortunately $current >= 0 fails in this case.

php > print var_dump(false >= 0);
bool(true)

I've also attached my agreement to the Contributor Agreement.

@murrant murrant commented on the diff Jun 9, 2016
includes/discovery/sensors/fanspeeds/lmsensors.inc.php
@@ -19,7 +19,7 @@
$oid = '1.3.6.1.4.1.2021.13.16.3.1.3.'.$index;
$current = snmp_get($device, $oid, '-Oqv', 'LM-SENSORS-MIB');
$descr = trim(str_ireplace('fan-', '', $descr));
- if ($current > '0') {
+ if ($current !== false && $current >= 0) {
@murrant
murrant Jun 9, 2016 Contributor

Just remove the && $current >= 0 and test that.

@keeperofdakeys
keeperofdakeys Jun 9, 2016 Contributor

Does it make sense for a fan to return a negative rpm value?

@murrant
murrant Jun 10, 2016 edited Contributor

BTW, it doesn't matter that false >= 0 is true, because that means false !== false already failed the check. So it is fine as stands.

@murrant
Contributor
murrant commented Jun 10, 2016

I finally got a chance to test.
Looks good there are a couple scenarios where you could get a fan to show up when you don't want it, but I don't think that would happen in the real world. (lmFanSensorsDevice defined for non-existent device and $current returns 0)

@murrant murrant merged commit 879abfb into librenms:master Jun 10, 2016

1 of 2 checks passed

Auto-Deploy Triggered
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