Nut upsdev - Feb 2008 - Important regression in usbhid-ups (r1113)

while finishing some work on the HAL integration (I'm now mapping DBus
method with NUT commands, to allow at least the final UPS poweroff), I
was horrified to realize (and so late) the changes in
usbhid-ups->upsdrv_shutdown().

We've lost a lot there, but making a generic code, instead of keeping
the subdrivers delegation.

For example, the shutdown.return command (which is or should be the
standard UPS poweroff method) has to set both delay.shutdown *and*
delay.start does only set the latter.
It results in an immediate UPS poweroff... The same applies to
shutdown.return. And shutdown.stop should clear both shutdown and
start delay to be sure.

The reason I previously kept the subdrv delegation (apart from the
lack of visibility on other mfrs implementation) was the same as Arjen
comment: the var. and cmd dealing with shutdown clearly have to be
reworked since we have not a 1 to 1 mapping here.

This has made its way into 2.2.1 only (r1127) and the trunk (since r1113).

The fix will obviously have to go into 2.2.2, and the best would be to
keep the generic method (not subdrv delegation).

So we have 2 possibilities (non exclusive. We may apply (1) quickly,
while waiting for (2))
1) change upsdrv_shutdown() to proceed with setvar instead of (or
with) the current instcmd
2) modify the hid2nut mapping to better deal with the shutdown related
vars and commands.

Can you (all subdrivers maintainers) please ack this, make some tests
on your side and report back your feeling about that.

Arnaud
-- 
Linux / Unix Expert R&D - MGE Office Protection Systems -
http://www.mgeops.com
Network UPS Tools (NUT) Project Leader - http://www.networkupstools.org/
Debian Developer - http://people.debian.org/~aquette/
Free Software Developer - http://arnaud.quette.free.fr/

Hi Arnaud,

I noticed a similar problem: on APC, the new shutdown method actually
includes a delay of many seconds. Presumably the intende behavior was:
shutdown immediately, and return to power after n seconds. The
actual effect was the opposite: sit there for n seconds, then shut
down and return immediately.

The previous, model-specific shutdown routines had the advantage that
they actually did the correct thing for each model.

-- Peter

Arnaud Quette wrote:
> 
> while finishing some work on the HAL integration (I'm now mapping DBus
> method with NUT commands, to allow at least the final UPS poweroff), I
> was horrified to realize (and so late) the changes in
> usbhid-ups->upsdrv_shutdown().
> 
> We've lost a lot there, but making a generic code, instead of keeping
> the subdrivers delegation.
> 
> For example, the shutdown.return command (which is or should be the
> standard UPS poweroff method) has to set both delay.shutdown *and*
> delay.start does only set the latter.
> It results in an immediate UPS poweroff... The same applies to
> shutdown.return. And shutdown.stop should clear both shutdown and
> start delay to be sure.
> 
> The reason I previously kept the subdrv delegation (apart from the
> lack of visibility on other mfrs implementation) was the same as Arjen
> comment: the var. and cmd dealing with shutdown clearly have to be
> reworked since we have not a 1 to 1 mapping here.
> 
> This has made its way into 2.2.1 only (r1127) and the trunk (since r1113).
> 
> The fix will obviously have to go into 2.2.2, and the best would be to
> keep the generic method (not subdrv delegation).
> 
> So we have 2 possibilities (non exclusive. We may apply (1) quickly,
> while waiting for (2))
> 1) change upsdrv_shutdown() to proceed with setvar instead of (or
> with) the current instcmd
> 2) modify the hid2nut mapping to better deal with the shutdown related
> vars and commands.
> 
> Can you (all subdrivers maintainers) please ack this, make some tests
> on your side and report back your feeling about that.
> 
> Arnaud
> -- 
> Linux / Unix Expert R&D - MGE Office Protection Systems -
http://www.mgeops.com
> Network UPS Tools (NUT) Project Leader - http://www.networkupstools.org/
> Debian Developer - http://people.debian.org/~aquette/
> Free Software Developer - http://arnaud.quette.free.fr/
>

On Wed, Feb 20, 2008 at 12:04 PM, Arnaud Quette <aquette.dev at gmail.com>
wrote:
>  Can you (all subdrivers maintainers) please ack this, make some tests
>  on your side and report back your feeling about that.
I may be able to do some testing on the APC and MGE side, but I don't
think I maintain any of the usbhid-ups subdrivers. (I don't have any
PDC-compliant Tripp-Lite UPSes, and the tripplite_usb shutdown support
is not very well tested anyway.)

-- 
- Charles Lepple

Arnaud Quette wrote:
> while finishing some work on the HAL integration (I'm now mapping DBus
> method with NUT commands, to allow at least the final UPS poweroff), I
> was horrified to realize (and so late) the changes in
> usbhid-ups->upsdrv_shutdown().
> 
> We've lost a lot there, but making a generic code, instead of keeping
> the subdrivers delegation.
We haven't lost anything here. Instead, we now follow what is
documented. On shutdown, we will use the ondelay and offdelay settings
(as advertised in the manpage).
> For example, the shutdown.return command (which is or should be the
> standard UPS poweroff method) has to set both delay.shutdown *and*
> delay.start does only set the latter.
No, it sets both. First it writes the 'ondelay' time and if that is
successful, it proceeds to write the 'offdelay' time. If both are
successful (and ondelay > offdelay), you'll get an orderly shutdown.
> It results in an immediate UPS poweroff...
In that case, you must have overridden the 'offdelay' in ups.conf.
> The same applies to
> shutdown.return. And shutdown.stop should clear both shutdown and
> start delay to be sure.
Why?
> The reason I previously kept the subdrv delegation (apart from the
> lack of visibility on other mfrs implementation) was the same as Arjen
> comment: the var. and cmd dealing with shutdown clearly have to be
> reworked since we have not a 1 to 1 mapping here.
> 
> This has made its way into 2.2.1 only (r1127) and the trunk (since r1113).
> 
> The fix will obviously have to go into 2.2.2, and the best would be to
> keep the generic method (not subdrv delegation).
> 
> So we have 2 possibilities (non exclusive. We may apply (1) quickly,
> while waiting for (2))
> 1) change upsdrv_shutdown() to proceed with setvar instead of (or
> with) the current instcmd
I'm against that. The variables that command these timers are clearly
instcmd's, not variable settings.
> 2) modify the hid2nut mapping to better deal with the shutdown related
> vars and commands.
If you don't want to use the 'ondelay' or 'offdelay' values,
we can also
choose to ignore these. But this will also mean that we need to change
some manual pages.
> Can you (all subdrivers maintainers) please ack this, make some tests
> on your side and report back your feeling about that.
It works for me (with the default settings) so if it doesn't work for
you, what are the contents of 'ups.conf'?

Best regards, Arjen