Nut upsdev - Jun 2007 - tripplite_usb.c reconnection

On 6/21/07, Arjen de Korte <nut+devel at de-korte.org>
wrote:
> Charles,
>
> The same remarks as for the reconnect_ups() patch for usbhid-ups.c go for
> the function with the same name in tripplite_usb.c. This is also blocking
> and thereby locking up the communication between driver and server. I
> don't think this is intentional, since the MAX_RECONNECT_TRIES in
> usb_comm_fail() is effectively a no-op then. I also seems to duplicate
> some things in usb_comm_fail(), like upsdrv_cleanup().
Thanks for pointing this out - I had forgotten that this code was
pretty much copied fron usbhid-ups/newhidups, and thus needed to be
fixed as well.

However, I'm not sure I caught the resolution of the problem. Should I
just take out the retry? The retry logic is a little simpler in
tripplite_usb in that reconnect_ups() only gets called in one place
(usb_comm_fail()).

I would almost prefer to have the driver not do any reconnection, but
have the hotplug rule start the driver whenever something is plugged
in (similar to HAL, but not necessarily dependent on it). This
eliminates any polling, and when there is nothing connected, it is
clearly visible to the server. Since USB is more stateful than RS-232,
I think this simplifies a lot of the corner cases.

-- 
- Charles Lepple

>> The same remarks as for the reconnect_ups() patch for usbhid-ups.c go
>> for
>> the function with the same name in tripplite_usb.c. This is also
>> blocking
>> and thereby locking up the communication between driver and server. I
>> don't think this is intentional, since the MAX_RECONNECT_TRIES in
>> usb_comm_fail() is effectively a no-op then. I also seems to duplicate
>> some things in usb_comm_fail(), like upsdrv_cleanup().
>
> Thanks for pointing this out - I had forgotten that this code was
> pretty much copied fron usbhid-ups/newhidups, and thus needed to be
> fixed as well.
>
> However, I'm not sure I caught the resolution of the problem. Should I
> just take out the retry? The retry logic is a little simpler in
> tripplite_usb in that reconnect_ups() only gets called in one place
> (usb_comm_fail()).
Well, basically you should never hang around in upsdrv_updateinfo(). So if
something isn't working out as expected, instead of calling sleep and
retrying, you should return back to main() and wait for the next time
upsdrv_updateinfo() gets called again to try again. So any while loop with
a sleep in it is out.
> I would almost prefer to have the driver not do any reconnection, but
> have the hotplug rule start the driver whenever something is plugged
> in (similar to HAL, but not necessarily dependent on it). This
> eliminates any polling, and when there is nothing connected, it is
> clearly visible to the server. Since USB is more stateful than RS-232,
> I think this simplifies a lot of the corner cases.
I'm already working on that for the server part, which is actually looking
pretty good now. Problem is that I can't seem to manage SVN properly from
my end, it refuses to accept any changes to the ServerConf branch I
created for this purpose, so I guess I'll remove that and commit it again.
There are still some things to iron out. Hot(un)plugging of driver and
letting the server know what is going on is ready, but I have some
problems with figuring out how the (upsmon) client should know which UPSes
to connect to (it should probably connect to all locally connected ones).
Probably the new 'upsc -l' could serve that purpose with a little shell
scripting, although I prefer a solution that would simply default to this
behaviour in the absence of MONITOR lines in 'upsmon.conf'.

Best regards, Arjen