[Nut-upsdev] [nut-commits] svn commit r1043 - in trunk: . docs

[Arjen de Korte]

>> I'm very reluctant to add all sorts of status flags to 'ups.status' (in
>> fact, I still doubt if we should have CHRG and DISCHRG in there). Where
>> would we stop?
> I think a useful condition for inclusion in ups.status is that it is a
> status, and that it is somewhat standardized across UPS.

This is not enough, IMHO. There must be a reason to aggregate this in
'ups.status' and not make it a separate variable. We could also create a
separate variable 'battery.charge.state' for instance, that could have the
values 'empty', 'discharging', 'charging' or 'full'. The only reason I
documented CHRG and DISCHRG in 'docs/new-drivers.txt' is that we also use
this status in the HAL drivers, so that it makes sense to formalize them.

> I think every UPS has some sort of indicator of whether the battery is
> charging.

Megatec doesn't have this (quite a popular protocol) and for contact
closure types, this isn't available either, to name a few. They are plent
of other bitmapped variables (note the alarms I recently added) that could
also be added. For instance, you could also map 'VoltageOutOfRange' in
'ups.status' instead of the separate NUT item 'input.transfer.reason'. So
a flag must have some importance, otherwise we could map all booleans in
'ups.status', which is certainly not what we want.

> It may or may not be used by clients.

Here is my main concern. For inclusion in 'ups.status' I would suggest
that we only put the bare minimum information in there, to make sure that
we don't have to send and parse long replies for status information.

> Whether the FullyCharged and FullyDischarged flags should be included
> (e.g. as FULL and EMPTY) depends on whether there are any meaningful UPS
> out there who actually use these flags.

There probably are. However, my Evolution 650 has a status 'EmergencyStop'
and probably most MGE units will have that too. Should we include that in
'ups.status' too? I don't think so.

>> I propose that we map 'FullyCharged=1' to '!chrg' and
>> 'FullyDischarged=1' to '!dischrg'. By putting these at the end of the
>> list
>> of 'ups.status' info elements, they would clear the Charging and
>> Discharging status, without setting these. So 'FullyCharged=0' would not
>> mean the UPS is charging, nor would 'FullyDischarged=0' mean that it is
>> discharging.
>
> This is a possibility, and probably a good one. But the logic must go in
> ups_status_set(), and not in the ups.status info elements in the
> subdrivers. This is because the order of the ups.status elements does
> not determine the order in which bits in the ups_status variable are
> set and cleared. Information may arrive asynchronously (during an
> interrupt transfer) or in random order, depending on what's included
> in a long walk, quick walk, or whatever.

If you include these variables in the quick walk and process whatever is
received through interrupt transfers before doing the quick walk, you can
be sure that they are processed in the order listed.

> This is the reason for the separation of "driver-internal" status bits
> (defined in status_bit_t), vs. "NUT" status (defined in
> docs/new-drivers.txt). The former should record strictly what the UPS
> reports, without any interpretation. In the latter, several flags
> might be combined into one. So the existing internal FULLYCHARGED and
> CHRG bits should be retained with their present meaning, and should be
> mapped to the NUT status CHRG.

That would be an option too. In that case I suggest that we use a special
variable name for that, instead of "ups.status" that we have now for
instance. This to make sure that the HIDpaths that are on that line don't
deal with the NUT variable 'ups.status' but will be processed later on.

> In addition, this allows subdrivers to collect all available
> information, without worrying whether this should go in ups.status,
> ups.alarm, or simply be ignored. Unknown stuff doesn't need to be
> commented out. All decisions about what actually to report can be done
> in ups_status_set() in a non-subdriver-specific manner.

Agreed.

Best regards, Arjen