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

[Michal Soltys]

On 11-01-26 16:36, Arjen de Korte wrote:
> Citeren Michal Soltys <soltys at ziu.info>:
>
>> I think I found one bug with flag handling though:
>>
>> dstate_setflags() uses = when setting new flags. This has a side
>> effect, that any later setflags() call will override earlier flags (as
>> expected), but that also includes the immutable flag (and that we
>> don't want, as it will enable the ups to e.g. poll and update the
>> variable at will afterwards). It could be fixed in a few ways:
>>
>> 1) make sure each driver reads the variable first and preserves
>> immutable flag (probably lots of fixing all over the place)
>> 2) dstate_setflags() could use an extra argument to specify if to
>> preserve or not
>> 3) make that an immutable flag once set, cannot be unset (simple,
>> probably good solution, as immutable flags come from override.*)
>> 4) something else
>>
>> I'd choose #3, but ...
>
> The ST_FLAG_IMMUTABLE is only supposed to be used for read-only values
> from the UPS. For those variables, dstate_setflags() must not be used
> (it is intended to set ST_FLAG_RW and sometimes in addition,
> ST_FLAG_STRING).
>

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

Then

1) config parser would interpret and set the immutable flag on 
battery.minutes.low
2) apc driver during capabiliity detection would find it and set RW 
flag, as the value can be read and also programmed in eeprom (few 
discrete values in case of that particular apc unit)

When I added checks for asserting LB state basing on runtime/battery 
left, this of course created conflict - as the driver removed the 
immutable flag and happily kept polling the value - so 0 or any other 
value had no effect whatsoever.

> If a writeable value is wrong, you should not override it in ups.conf,
> but rather change it.

That's not always possible (as above - 0 can't be set at all) or 
flexible enough.

In the code, the only meaning of immutable flag - from what I saw - is 
simply that dstate_setinfo() refuses to change it:

                 /* changes should be ignored */
                 if (node->flags & ST_FLAG_IMMUTABLE) {
                         return 0;       /* no change */
                 }

It doesn't conflict in any way with ability to write or read it (to/from 
the actual ups unit)

Either way, simple fix with which everything works fine :

-       sttmp->flags = flags;
+       sttmp->flags = flags | (sttmp->flags & ST_FLAG_IMMUTABLE);