[Nut-upsdev] [Nut-upsuser] Tripplite OMNIVS1500 Woes

[Arjen de Korte]

> [did you mean to reply off-list? I don't mind having this discussion
> on the nut-upsdev list.]

I did mean to reply off-list. But I'll take it to the list now, leaving
whatever I quoted in here.

>>>> Here is the most persistant error type I get when running the
>>>> Driver in
>>>> Debug mode (I am using the latest trunk SVN):
>>>
>>> You may need to pass one or two more -D options to see what is really
>>> going on. I am suspicious of any "error code 0" messages because "0"
>>> usually means "no error".
>>
>> I have one remark here when it comes to the 'tripplite_usb' code.
>> The UPS
>> seems to reconnect after replugging the power (which is unsurprising,
>> since this usually generates lots of EMI). But during the
>> reconnection,
>> you call upsdrv_initinfo() again. I don't think that is a good idea.
>>
>> The upsdrv_initinfo() function is meant for detecting the UPS type/
>> model
>> and is basically the last time a driver can decide that it is
>> unsuitable
>> to monitor a UPS and bail out. After that (and running
>> upsdrv_updateinfo()
>> once), a socket for the 'upsd' server is created and from then on,
>> it is
>> expected that the driver socket will remain available as long as
>> NUT is
>> running. Which means that if you fail to (re)connect to the UPS, you
>> should call dstate_datastale() and/or upslogx() instead of
>> terminating the
>> driver, no matter how many communication failures you might see
>> (ever).
>> What's wrong in the driver now, is that by running upsdrv_initinfo
>> () again
>> when reconnecting, you introduce the possibility that if there is a
>> communications problem or the driver reads enough garbage
>> characters to
>> miss detecting it on the first try, it will bail out with fatalx().
>> Ouch!
>> This problem is aggrevated by the fact that upsdrv_initinfo() never
>> retries on failures.
>>
>> I don't think it is needed to run upsdrv_initinfo() when
>> reconnecting in
>> the first place. After all, the initialization ran before, so
>> unless the
>> user removed the UPS from the system, we can expect that it will
>> eventually pop up again. So basically, you already have all the
>> information that upsdrv_initinfo() provides. I wouldn't care for
>> the edge
>> cases where people swap models without stopping the driver. It
>> wouldn't
>> work in the current implementation of this driver anyway, since
>> reconnecting to a different model (with possibly different variable
>> set)
>> would also require dstate_delinfo() all information that is no longer
>> available. We do that in the 'usbhid-ups' driver, but this requires
>> a lot
>> of bookkeeping to prevent first deleting and then creating the same
>> variable again. Not recommended.
>
> I don't claim to completely understand the reconnection code - I
> believe Arnaud was pushing for it, and I modeled the tripplite_usb
> reconnection code after whatever usbhid-ups (newhidups) was using at
> the time (read: copied).

I understand, it looks pretty familiar... :-)

> The one bit of functionality that I would like to eventually have in
> the driver is the case where several units with the same VID/PID are
> connected, and we need to read the unit ID to distinguish between
> them. (Unfortunately, it's not something that we could put into the
> matcher.)

If I understand correctly, this requires sending the UPS a command to read
the ID. In that case, it *can't* be done (sadly enough). The only
information you can read from a USB device without claiming it, is the
stuff we use in the existing matchers. Everything else requires claiming a
device first. But in the process this will remove the claim any other
process may have. Since there is no way to tell which device will be tried
first, this will lead to an endless mess of disconnect/connect attempts if
two or more drivers are running. We tried this with the 'usbhid-ups'
driver, but in practice you can only use this if you have only one UPS
connected (in which case this is moot since there will be no confusion).
The only thing that might solve the problem with devices with identical
VID:PID, is adding the physical port in the matcher. As far as I know,
this isn't possible with the existing libusb-0.1 library and not with the
future libusb-1.0 library either.

> Of course, that wouldn't preclude the removal of the fatalx() call.

In order to keep things clear, it is probably best to only call fatalx()
from the upsdrv_initups() and upsdrv_initinfo() functions and certainly
not from helper functions.

> So if I read this right, do you think that upsdrv_initinfo() *should*
> retry, or should retrying be relegated to the reconnection code,
> since it shouldn't call upsdrv_initinfo() on disconnection?

Drivers must never call upsdrv_*() functions themselves, these should be
reserved to the interface with the main driver body.

It probably wouldn't hurt if upsdrv_initinfo() would retry, but keep the
number of retries limited (three times). What we don't want is that the
first failed attempt terminates the driver, but it should not retry
forever (if the UPS is unavailable we want to know it when starting up).

The reconnection code should indeed handle everything by itself (or
through helper functions).

Best regards, Arjen
--