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

newdevice: APC Environmental monitoring units #5140 #5356

Merged
merged 9 commits into from
Jan 18, 2017

Conversation

laf
Copy link
Member

@laf laf commented Jan 9, 2017

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.

Fixes: #5140

Few things.

  1. These devices allow users to specify the data is in Fahrenheit rather than Celsius so I've added a function to convert.

  2. The includes/polling/sensors/X/ folders were named wrong so have been moved. They don't follow the plural like disco but use the name registered in discover_sensor() which is the singular!

  3. Added a pre-cache for sensor polling.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

$descr = $temp['emsProbeStatusProbeName'];
$low_limit = $temp['emsProbeStatusProbeMinHumidityThresh'];
$low_warn_limit = $temp['emsProbeStatusProbeLowHumidityThresh'];
$high_limit = $temp['emsProbeStatusProbeMaxHumidityThresh'];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device I tested against doesn't send any info back for:
emsProbeStatusProbeMaxHumidityThresh
emsProbeStatusProbeMinHumidityThresh

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

does it not then just auto calculate?

foreach ($emu2_temp as $id => $temp) {
if (isset($temp['emsProbeStatusProbeHumidity']) && $temp['emsProbeStatusProbeHumidity'] > 0) {
$index = $temp['emsProbeStatusProbeIndex'];
$oid = '.1.3.6.1.4.1.318.1.1.10.3.13.1.1.3.' . $index;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I belive this should be .1.3.6.1.4.1.318.1.1.10.3.13.1.1.6.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well spotted, updated now

$index = $temp['emsProbeStatusProbeIndex'];
$oid = '.1.3.6.1.4.1.318.1.1.10.3.13.1.1.3.' . $index;
$descr = $temp['emsProbeStatusProbeName'];
$low_limit = fahrenheit_to_celsius($emu2_temp_scale, $temp['emsProbeStatusProbeMinTempThresh']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The device I tested against doesn't send any info back for:
emsProbeStatusProbeMaxTempThresh
emsProbeStatusProbeMinTempThresh

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

@Rosiak
Copy link
Member

Rosiak commented Jan 9, 2017

Disco: http://pastebin.com/2hgQtvVA
Poll: http://pastebin.com/8QCDH7sX

To sum up, missing min/max messes up the thresholds, polling doesn't reflect the Fahrenheit conversion.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

@laf
Copy link
Member Author

laf commented Jan 10, 2017

Forgot to push this last night. Added a function to set the default value to null based on the minimum we expect so if none of the thresholds are valid (i.e 0 according to your output) then we default to null so they are excluded.

The Fahrenheit was fixed in another commit but you were on the older one hence it didn't work for you.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

@laf
Copy link
Member Author

laf commented Jan 12, 2017

bump

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

@laf
Copy link
Member Author

laf commented Jan 15, 2017

fixed rebase.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

@Rosiak
Copy link
Member

Rosiak commented Jan 16, 2017

@last thing:
Device I have access to does not respond on emsProbeStatusProbeMaxHumidityThresh or emsProbeStatusProbeMinHumidityThresh which messes up the limit's.

Other than that I think we are good to go!

@laf
Copy link
Member Author

laf commented Jan 16, 2017

Thanks, I've moved set_null to inside discover_sensor now so any invalid high/low limits will be nulled out.

@LibreNMS-CI
Copy link

Auto-Deploy finished, Test PR at http://5356.ci.librenms.org or https://5356.ci.librenms.org

@scrutinizer-notifier
Copy link

The inspection completed: 3 updated code elements

@Rosiak
Copy link
Member

Rosiak commented Jan 16, 2017

Still an issue, you are aware that it was Humidity that was wrong, right?
Temp polling/warnings is perfectly fine :)

@laf
Copy link
Member Author

laf commented Jan 16, 2017

Yes but the call is now in discover_sensor() so cleans anything. Up what's actually wrong and where's the debug on the latest commit?

@laf
Copy link
Member Author

laf commented Jan 17, 2017

Can we look at merging this. The issue for @Rosiak is not from this PR, defaults for humidity sensors with no thresholds are 70/70

@laf laf merged commit 8557f2d into librenms:master Jan 18, 2017
@laf laf deleted the issue-5140 branch January 18, 2017 08:48
@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants