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

ict-pdu os: Fixed polling for state sensor. #8558

Merged
merged 4 commits into from Apr 15, 2018

Conversation

Projects
None yet
4 participants
@vivia11
Contributor

vivia11 commented Apr 13, 2018

Removed if/else statement. Code was reading an index that was out of bounds (for sensor_index greater than 0) resulting in sensor_value of 0 which did not map to any state.

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

@laf

This comment has been minimized.

Member

laf commented Apr 13, 2018

Any context around why this needs changing?

@vivia11

This comment has been minimized.

Contributor

vivia11 commented Apr 13, 2018

image

The if/else structure was causing the odd behaviour seen above. The first fuse port was correctly assigned a sensor_value, but all other fuse ports (due to the else structure) was given a sensor_value of 0 which does not map to anything.

The code was attempting to append '.0' to the oid and read the snmp value - but no such index exists. Not sure why the if/else structure was originally necessary, as the polling works fine without the if/else as seen below.

image

@laf

This comment has been minimized.

Member

laf commented Apr 13, 2018

Luckily the chap who added this support used our pastebin service and posted his walk in the issue. I used it to test and you are correct this does fix it - however, removing the file also seems to. Can you check that and if so just remove the file please.

@vivia11

This comment has been minimized.

Contributor

vivia11 commented Apr 13, 2018

Removing the file also works for me.

@murrant

This comment has been minimized.

Member

murrant commented Apr 14, 2018

Any chance you could add test data for this device?

https://docs.librenms.org/Developing/os/Test-Units/#example-workflow

@laf

This comment has been minimized.

Member

laf commented Apr 15, 2018

I've added test data.

@laf laf added the Device 🖥 label Apr 15, 2018

@scrutinizer-notifier

This comment has been minimized.

scrutinizer-notifier commented Apr 15, 2018

The inspection completed: No new issues

@laf

laf approved these changes Apr 15, 2018

LGTM + tested

@laf laf merged commit 146fc98 into librenms:master Apr 15, 2018

2 checks passed

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

@vivia11 vivia11 deleted the vivia11:ict-pdu branch Apr 16, 2018

TheMysteriousX added a commit to TheMysteriousX/librenms that referenced this pull request May 20, 2018

device: Fixed polling for state sensor on ict-pdu (librenms#8558)
* Remove if/else for polling. Gives incorrct index.

* delete ict-pdu file for state polling

* Added test data

@lock lock bot locked as resolved and limited conversation to collaborators Jun 15, 2018

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