Re: [Nut-upsdev] [PATCH/RFC 1/1] APC smart driver update and new features.
Citeren Michal Soltys sol...@ziu.info: 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 ? After some more thoughts, the first suggestion indeed seems the best. We really should allow people to override values, regardless of the fact that these may be writable. An override should really be that, unlike setting a default value where the value from the UPS would take precedence. The trunk version has now been changed to do just this and also adopts your idea of setting 'ignorelb' on a global level to ignore any set_status(LB) settings by drivers. It calculates this flag instead from battery.charge (battery.runtime) and battery.charge.low (battery.runtime.low) and also warns if insufficient data is available to do so. Thanks for your feedback. Best regards, Arjen -- Please keep list traffic on the list (off-list replies will be rejected) ___ Nut-upsdev mailing list Nut-upsdev@lists.alioth.debian.org http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
Re: [Nut-upsdev] [PATCH/RFC 1/1] APC smart driver update and new features.
Citeren rasengan rootish r...@relay.ppgk.com.pl: So basically what you have in mind is putting such checks in main.c's loop somewhere after upsdrv_updateinfo() call, and then updating ups.status ? Yes. That would also warrant adding options enabling such functionality (separately for charge and runtime), as not everyone might want it. No. If both 'battery.charge' and 'battery.charge.low' are present, there is no need to make this configurable. A UPS that supports them out of the box, will already set the LB flag (so there is no possibility to change that). If you don't want this added functionality, don't set a default value in 'ups.conf' (similar for runtime). In case you happen to have a UPS that does report both 'battery.charge' and 'battery.charge.low' but for some mysterious reason doesn't use that to set the LB flag, you can always use 'override.battery.charge.low=0' to disable it. I'll redo the patches with the new approach. Thanks. Best regards, Arjen -- Please keep list traffic on the list (off-list replies will be rejected) ___ Nut-upsdev mailing list Nut-upsdev@lists.alioth.debian.org http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
Re: [Nut-upsdev] [PATCH/RFC 1/1] APC smart driver update and new features.
On 11-01-26 16:36, Arjen de Korte wrote:
Citeren Michal Soltys sol...@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);
___
Nut-upsdev mailing list
Nut-upsdev@lists.alioth.debian.org
http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
Re: [Nut-upsdev] [PATCH/RFC 1/1] APC smart driver update and new features.
On 11-01-26 20:28, Arjen de Korte wrote:
Citeren Michal Soltys sol...@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 ?
___
Nut-upsdev mailing list
Nut-upsdev@lists.alioth.debian.org
http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
Re: [Nut-upsdev] [PATCH/RFC 1/1] APC smart driver update and new features.
Citeren Michal Soltys sol...@ziu.info: Summary: - new driver options: ignorelb, minbatt, mintime, wugrace, advorder - expanded: sdtype - few more types in compatibility table - minor fixes and adjustments First of all, thanks for the effort of working on this driver. The lack of an active developer for APC units has worried us for some time already, given the popularity of the brand. Most of the patches are good to go, with the exception of the 'minbatt' and 'mintime'. These are generic functions that I would prefer to see included in the main driver core, rather than in each driver individually. We already support adding missing constants (default.battery.charge.low) or overriding wrong ones (override.battery.charge.low) in 'ups.conf'. The only thing that is missing now in the driver core are the checks battery.charge battery.charge.low battery.runtime battery.runtime.low Adding the above is trivial and would then be usable for all drivers. Therefor I don't think it is a good idea to do this here. Best regards, Arjen -- Please keep list traffic on the list (off-list replies will be rejected) ___ Nut-upsdev mailing list Nut-upsdev@lists.alioth.debian.org http://lists.alioth.debian.org/mailman/listinfo/nut-upsdev
