[Nut-upsdev] [PATCH/RFC 1/1] APC smart driver update and new features.

[Michal Soltys]

On 11-01-26 20:28, Arjen de Korte wrote:
> Citeren Michal Soltys <soltys at ziu.info>:
>
>> The point is about override.* values, as they are implemented through
>> immutable flag. If I added for example
>>
>> [apctest]
>> override.battery.minutes.low = 0
>> or
>> override.battery.minutes.low = -1
>
> How is this supposed to work? I can imagine shutting down earlier than
> whatever buildin runtime is configured in the UPS, but not later. If the
> UPS uses battery.runtime.low=120, there is no point overriding this with
> a lower value, because when the buildin value is reached, the UPS will
> already set the LB flag. We use battery.runtime(.low) by the way.
>

One of the main points of the patch I sent was to let the user be in 
control of the shutdown (in context of apc smart units), basing the 
decision /only/ on two polled values: if supported by ups - 
battery.charge (I think all apc units in smart can do that, regardless 
of their age) and if supported by ups - battery.runtime. User would 
supply 'minbatt' and 'mintime' to control the thresholds, that would 
cause the LB to be asserted - *ignoring* what ups thinks internally 
about LB (as I've found it, over the years, unreliable in many cases) - 
thanks to "ignorelb" option.

You suggested to not do it at driver level, but in main.c - using 
battery.{runtime,charge}.low - that can be overriden through "override." 
prefix (if necessary). You also suggested to use e.g. 
override.battery.charge.low = 0, should user want to disable it for some 
reason. That could all work fine, except that under current rules - they 
can't be overridden, if they can be programmed (rw are not meant to be 
immutable). APC units (dunno if all) won't tolerate battery.runtime.low = 0.

Using battery.runtime.low as an example - it's really RO variable during 
normal operation, and RW only during eeprom programming (the allowed 
values are few discrete settings for apc units, if applicable). ignorelb 
+ override.battery.charge.low = 0 (or as in original patch - ignorelb + 
minbatt = 0) would disable this check, and let user control the ups 
basing the decision only on e.g. battery.charge.

>
> If a driver already supports this value, it will also act upon it. If
> you set the immutable flag on this value, NUT might display this, but
> the UPS would still set the LB flag if the battery.runtime would drop
> below the internal representation of battery.runtime.low.

That's why "ignorelb" option was added to apcsmart driver (see above and 
original commit message for the rationale) - this allows the control by 
polling the charge/runtime, using /only/ user supplied thresholds for LB 
decision. ups' internal decision (based on calibration, firmware, 
battery age/quality AND battery.runtime.low (if applicable) eeprom 
variable) is ignored.

That's why I also originally opted for minbatt/mintime, instead of going 
for generic main.c level checks from the start.

>>
>> - sttmp->flags = flags;
>> + sttmp->flags = flags | (sttmp->flags & ST_FLAG_IMMUTABLE);
>
> No. A variable that is R/W can *never* be immutable.
>

Then, the other options, that is if:

- additional LB checks are to remain in main.c
- new apcsmart's "ignorelb" option to have the desired effect
- the checks are to use well known battery.{runtime,charge}.low, and be 
universal at the same time

the likely options are:

- handle this at driver level - e.g. preserve the immutable flag if 
already set without setting rw (if necessary, e.g. in apcsmart if 
ignorelb is set), ignore polling, adjust the code so it works fine, etc.
- go back to minbatt/mintime
- add e.g. ST_FLAG_USER_OVERRIDE which would be used for any (and only 
for) override.* in ups.conf (instead of ST_FLAG_USER_IMMUTABLE), while 
not being encumbered by "must not be rw" constraint

The 1st option looks good ?