Skip to content

eaton-ats16-nm2-mib.c: fix mul coef. for ambient.temperature and humidity#1173

Merged
aquette merged 3 commits intonetworkupstools:masterfrom
FrancoisRegisDegott-eaton:patch-1
Nov 15, 2021
Merged

eaton-ats16-nm2-mib.c: fix mul coef. for ambient.temperature and humidity#1173
aquette merged 3 commits intonetworkupstools:masterfrom
FrancoisRegisDegott-eaton:patch-1

Conversation

@FrancoisRegisDegott-eaton
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@aquette aquette left a comment

Choose a reason for hiding this comment

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

Lgtm. Would be worth testing on more than 1 unit to be sure

@jimklimov
Copy link
Copy Markdown
Member

Does this apply to all devices? IIRC at least in some other drivers, we had a problem of multipliers adjusted between firmware versions, so we have to either require that hardware is flashed to newest FW, or to adapt in the driver to devices in the field, by some mapping functions etc. like "if (version < X) then ... else..."

I do hope this one is not a case like that? :)

@aquette aquette requested a review from jimklimov November 10, 2021 07:11
@FrancoisRegisDegott-eaton
Copy link
Copy Markdown
Contributor Author

I agree. But its not easy to check regression related to fw/MIB version...
Here is the upsc fingerprint for the 2.1.5 fw... (useless info removed)
ambient.humidity: 4.50
ambient.humidity.high: 90
ambient.humidity.low: 10
ambient.temperature: 2.6
ambient.temperature.high: 80
ambient.temperature.low: 0
device.mfr: EATON
device.model: Eaton ATS 208V 20A
device.part: EATS220
device.serial: GA07H14138
device.type: ats
driver.name: snmp-ups
driver.version: 2.7.4.1
driver.version.data: eaton_ats16_nm2 MIB 0.18
driver.version.internal: 1.16
ups.firmware: 00.00.0010
ups.firmware.aux: 2.1.5
ups.status: OL

@jimklimov
Copy link
Copy Markdown
Member

For the data dump above, is the humidity really "4.5%" as in a desert (and not a moderate 45% in the range okay for electronic devices -- too dry means lots of static!), and temperature a freezing "2.6" not "26" degrees (Celsius, I suppose)?

Combining the two... many people died in Sahara unexpectedly freezing at nights, not hiding in shadows during the day, but those nights also allowed them to collect condensed water so it was not too dry at that time ;)

@jimklimov
Copy link
Copy Markdown
Member

Also, whatever coefficient is used, shouldn't "high" and "low" use the same?

I wonder if those high watermarks can tell us whether to divide by 10 (something over 100% certainly should be, at least...)

@jimklimov
Copy link
Copy Markdown
Member

And regarding FW/MIB versions, can you get in touch with HW team or their changelogs, I suppose there would be a couple of values (or ranges) in the end to check for?..

@jimklimov jimklimov requested a review from aquette November 10, 2021 16:51
Copy link
Copy Markdown
Member

@jimklimov jimklimov left a comment

Choose a reason for hiding this comment

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

Per comments above, seems to me that additional work is needed here. Or please prove me wrong :)

@aquette
Copy link
Copy Markdown
Member

aquette commented Nov 15, 2021

hereby confirming that Jeff's above dump is on ATS16 with NM2 and EMP002 backward compat (exposed like the legacy EMP001).
With Jeff's patch, we do well have the right values.
Also confirming that ATS16 with EMP001 are not impacted:
ambient.humidity: 31.10
ambient.humidity.high: 90
ambient.humidity.low: 5
ambient.temperature: 22.8
ambient.temperature.high: 40
ambient.temperature.low: 5
device.mfr: EATON
device.model: Eaton ATS 230V 16A
device.part: EATS16N
...
driver.version.data: eaton_ats16_nmc MIB 0.18

Finally confirming that the new Eaton Sensor MIB exposes temperature and humidity in "tenthOfDegXXX" (Kelvin, Celsius or Farhenheit), as per TemperatureUnitType OID

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants