Flag "battery_voltage_reports_one_pack" in nutdrv_qx is broken

[dennypage]

The battery_voltage_reports_one_pack flag was introduced in response to issue #1279. It's unclear why this flag was introduced, as the prior method of always multiplying the reported voltage by the number of packs was functional. Having the flag be false is the same end result as battery.packs=1, which should always be the case if the battery_voltage_reports_one_pack flag is not set.

Regardless, the battery_voltage_reports_one_pack flag is broken as a result of commit [6fbb680].

The problem is the access method used. The current code uses getval() to examine the setting. However, since battery_voltage_reports_one_pack is a flag rather than a variable, getval() will always return null. Function testvar() must be used instead.

PR #2324 addresses this simple issue. However, it might be worth considering the removal of the flag completely as battery.packs=1 represents the same functionality. If you would like me to prepare an alternate PR for this approach, please let me know.

[jimklimov]

Good catch, thanks!

Regarding the value, at least conceptually, IIRC the idea was that some UPSes report the voltage of the battery pack (e.g. 12V nominal of a PbAc 6-cell assembly), some report the overall voltage of their 2 or 4 battery packs (e.g. 24nominal/28-ish max or 48/56-ish V) in larger UPSes that house several battery bricks inside, and some apparently have some way of reporting a cell (small single-digit volts; say a typical PbAc cell has 2V and 6 of those are in one pack).

IIRC the flag was about this: if the device reports, say, 2V as its native voltage, we could multiply that by the amount of cells per pack to get 12V in one pack, and e.g. by 4 "battery.packs" to know the resulting voltage as applicable to that device's overall power cell combination topology.

I am not sure OTOH if that idea was completed (e.g. with a setting to tell the amount of cells in a pack), though, now that I'm trying to remember those PRs (not at a computer now).

[dennypage]

Yes, some UPSs report the voltage of a single pack. However, that was previously handled with the number of packs setting in the same way that it is handled in the now obsolete blazer driver.

The conditional in qx_multiply_battvolt() tells the tale:

if (!battery_voltage_reports_one_pack || batt.packs < 2)
        /* (We assume by default that) this device already reports
         * the sum-total voltage for the battery assembly - so just
         * pass it on unmodified.

The effective else clause returns the voltage multiplied by the number of battery packs.

The battery_voltage_reports_one_pack flag is not used anywhere else, so I honestly don't see a reason for it to exist.