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

device: Rittal IT Chiller / Carel pCOweb card #7826

Merged
merged 11 commits into from Dec 30, 2017

Conversation

Projects
None yet
4 participants
@FTBZ
Copy link
Contributor

FTBZ commented Nov 30, 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.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926


Note that the MIB file is ugly, but I've finally found a solution that's compatible with the YAML during the discovery AND the polling.

  • Added support for the Rittal IT Chiller by the pCOweb card
  • Added documentation about the pCOweb card
  • Corrected a bad SQL schema file that I've injected in a past commit

screen shot 2017-11-30 at 15 12 26

FTBZ added some commits Nov 30, 2017

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Nov 30, 2017

Two suggestions off the top:

  1. E.E.R. should not be abbreviated in the graph heading
  2. E.E.R. should probably be a new sensor type. (see an example here: https://github.com/librenms/librenms/pull/7714/files)
@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 1, 2017

I've a problem with the cooling graphs. The values appear in the device Overview (disco and poll seem to be working), but the Free Cooling Capacity and Total cooling Capacity graphs are not working. It's showing -nanW as value. Compressor Cooling Capacity is working fine. Idea?

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 1, 2017

Post the output of ./poller.php -h HOSTNAME -d -m sensors

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 1, 2017

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 2, 2017

That output seems to suggest the data is recording ok. In the device overview page, next to the graph for this sensor it also displays the text value, is this correct?

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 6, 2017

The numbers in the overview are always correct. But the graph does strange things (only the graph for a few minutes):
screen shot 2017-12-06 at 08 20 48

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 6, 2017

We do only allow a range of -20000 - 20000 for sensors, I wonder if it's going beyond that.

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 6, 2017

Correct, the value is 29300W at the moment. (27700W in the first screen shot)

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 6, 2017

If those are genuine values then you need to edit: https://github.com/librenms/librenms/blob/master/includes/polling/functions.inc.php#L169

And change those two values to something else. Maybe 100000 for now. I'm not sure dropping the limits is a good idea as it's at least a safe guard for bad data being recorded.

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 6, 2017

Nice, it's working. The cooling value should be in kW and not W in my opinion, hard to change now.

FTBZ and others added some commits Dec 6, 2017

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 7, 2017

I've made some changes but looks good to me.

Question though, what's eer and does it not belong as a sensor?

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 8, 2017

EER -> Energy Efficiency Ratio for cooling systems. It tells you if you're efficient or not. Yes, it can be a sensor but not existing at this time.

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 8, 2017

Then we need to shift that into sensors, if you want me to do that give me a shout. We've got a few PRs recently that add this type of support.

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 8, 2017

If you can take care of it, it would be nice. I watched the PR but I'm not comfortable with it (not merged at this time). Another thing that's missing it's a sensor for water flow rate in l/m. If you can create a PR with only one sensor, I can use it as an example to create one of the two.

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 9, 2017

We've got an other PR in adding more sensors, will do it after that to save merge conflicts.

@murrant

This comment has been minimized.

Copy link
Member

murrant commented Dec 13, 2017

@FTBZ Other PR is now merged :)

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 13, 2017

Can this PR be merged like this and I check the two new sensors in a separate one?

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 15, 2017

Sure, just needs a rebase to fix the merge conflict :)

@laf

This comment has been minimized.

Copy link
Member

laf commented Dec 15, 2017

One last question before merging, is this the best name for this OS? Seems very hardware specific.

@FTBZ

This comment has been minimized.

Copy link
Contributor

FTBZ commented Dec 16, 2017

The name was chosen by @murrant to track all device of this kind.

@scrutinizer-notifier

This comment has been minimized.

Copy link

scrutinizer-notifier commented Dec 30, 2017

The inspection completed: No new issues

@laf laf merged commit dd8849d into librenms:master Dec 30, 2017

2 checks passed

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

This comment has been minimized.

Copy link

lock bot commented May 16, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed.

@lock lock bot locked as resolved and limited conversation to collaborators May 16, 2018

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