Nut upsdev - Apr 2014 - usbhid-ups pull request for consideration

I've added a pull request with changes to drivers/libusb and
drivers/usbhid-ups mainly. They are to resolve issues I encountered when
setting up a Tripp Lite SMART1500LCDT, but I think they're generally useful
to the community. Here is the pull request comment I sent:

https://github.com/networkupstools/nut/pull/122

Had trouble getting nut to work for my new Tripp Lite SMART1500LCDT. So in
best open source fashion, I hacked at it till it worked. Couple things in
this pull request:

1. Scale values for some of the SMART1500LCD parameters.

2. With my hardware and kernel, usbhid-ups wouldn't run more than 10
minutes before exiting with a fatal error. This seems to be related to
holding usb_claim_interface() the entire life of the program. I've
refactored claim_interface/release_interface calls to be around only when
work is being done. Now usbhid-ups has run for 24+ hours without issue.

3. The very nature of USB in a home means that sometimes the driver will
lose communications with the UPS (accidently unplug the hub, needed a port
for a camera, etc). This shouldn't be a fatal error. I've tried to
detect
these situations and let usbhid-ups retry gracefully. Also, at startup the
UPS no longer has to be immediately present. It will keep retrying
upsdrv_initups() until it finds one (only implemented for usbhid-ups, but
someone could do the work for other drivers).
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20140419/5b0eac6b/attachment.html>

Just to put another cent of thought in the bucket:

On Sat, Apr 19, 2014 at 8:23 AM, Stephen J. Butler <stephen.butler at
gmail.com
> wrote:
> 3. The very nature of USB in a home means that sometimes the driver will
> lose communications with the UPS (accidently unplug the hub, needed a port
> for a camera, etc). This shouldn't be a fatal error. I've tried to
detect
> these situations and let usbhid-ups retry gracefully. Also, at startup the
> UPS no longer has to be immediately present. It will keep retrying
> upsdrv_initups() until it finds one (only implemented for usbhid-ups, but
> someone could do the work for other drivers).
>
This is a good idea in any situation, even in a data-center setting the
connection to the ups may be lost at times (disconnection, ups failure,
etc.) the connection must be retried consistently with some fixed delay to
avoid overloading the system. There should be a way to alert about this to
the users but that's somewhat outside the scope of the nut project itself.

Baruch
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20140419/bd30e838/attachment.html>

Hi Stephen,

It's been a little busy at home, so I haven't had a chance to go over
the pull request in depth, but I didn't want you to think it hadn't been
considered.

On Apr 19, 2014, at 1:23 AM, Stephen J. Butler wrote:
> I've added a pull request with changes to drivers/libusb and
drivers/usbhid-ups mainly. They are to resolve issues I encountered when setting
up a Tripp Lite SMART1500LCDT, but I think they're generally useful to the
community. Here is the pull request comment I sent:
> 
> https://github.com/networkupstools/nut/pull/122
> 
> Had trouble getting nut to work for my new Tripp Lite SMART1500LCDT. So in
best open source fashion, I hacked at it till it worked. Couple things in this
pull request:
> 
> 1. Scale values for some of the SMART1500LCD parameters.
Thanks!
> 2. With my hardware and kernel, usbhid-ups wouldn't run more than 10
minutes before exiting with a fatal error. This seems to be related to holding
usb_claim_interface() the entire life of the program. I've refactored
claim_interface/release_interface calls to be around only when work is being
done. Now usbhid-ups has run for 24+ hours without issue.
Given how long we have been running with code that claims the interface at
driver startup, I am a wary about changing this behavior. Have you had a chance
to raise this with the Linux USB folks? I don't see any reason why claiming
the interface for an indefinite amount of time shouldn't be expected to
work.

We will, of course, need to test this on other models of UPS which are supported
by usbhid-ups.

Assuming for a moment that the fatal error is due to a regression in the kernel,
are there other general advantages to only claiming when needed?
> 3. The very nature of USB in a home means that sometimes the driver will
lose communications with the UPS (accidently unplug the hub, needed a port for a
camera, etc). This shouldn't be a fatal error. I've tried to detect
these situations and let usbhid-ups retry gracefully. Also, at startup the UPS
no longer has to be immediately present. It will keep retrying upsdrv_initups()
until it finds one (only implemented for usbhid-ups, but someone could do the
work for other drivers).
To be honest, the current approach to reattaching to USB devices has always
bothered me. We have had a few hard-to-find memory management bugs in the
reattach code over the years, and there are race conditions when a USB device
doesn't cleanly reconnect. Ensuring complete testing coverage over all of
those reconnection cases is much harder.

I would be a lot more excited about retesting all of this if we could somehow
leverage udev (or equivalent, on non-Linux) to make this event-driven. Then, the
individual drivers don't have to deal with reconnection, and when the UPS
disappears for any reason, the driver simply exits. The problem for libusb-0.1
is that I don't know of a clean way to pass a udev device specification to
usb_open(). (Insert rant about most USB devices not having unique serial numbers
in their device descriptors.)

Thoughts? Has anyone implemented a similar udev-to-application handoff in
libusb-1.x?

-- 
Charles Lepple
clepple at gmail

On Thu, Apr 24, 2014 at 7:46 AM, Charles Lepple <clepple at gmail.com>
wrote:
> On Apr 19, 2014, at 1:23 AM, Stephen J. Butler wrote:
> > 2. With my hardware and kernel, usbhid-ups wouldn't run more than
10
> minutes before exiting with a fatal error. This seems to be related to
> holding usb_claim_interface() the entire life of the program. I've
> refactored claim_interface/release_interface calls to be around only when
> work is being done. Now usbhid-ups has run for 24+ hours without issue.
>
> Given how long we have been running with code that claims the interface at
> driver startup, I am a wary about changing this behavior. Have you had a
> chance to raise this with the Linux USB folks? I don't see any reason
why
> claiming the interface for an indefinite amount of time shouldn't be
> expected to work.
>
No, I haven't. I'm not entirely convinced it's the kernel or libusb
causing
my problems. I half believe it could be some other userland program
running. For example, I had no idea I had dbus running upower before
debugging this issue.

I didn't go as far as doing a stap on it. The more I thought of it, the
more I decided that usbhid-ups should allow other programs on the system to
claim the device if they want.

> We will, of course, need to test this on other models of UPS which are
> supported by usbhid-ups.
>
> Assuming for a moment that the fatal error is due to a regression in the
> kernel, are there other general advantages to only claiming when needed?

I noticed when hacking around in libusb.c that there was a comment in
libusb_close() about not calling usb_release_interface() because it hangs.
That doesn't seem right to me, and I couldn't find any other libusb
examples that advocated claiming the device the lifetime of the program. I
wonder if it wasn't an early symptom of this.

As far as testing, it does seem like a risky change.

> I would be a lot more excited about retesting all of this if we could
> somehow leverage udev (or equivalent, on non-Linux) to make this
> event-driven. Then, the individual drivers don't have to deal with
> reconnection, and when the UPS disappears for any reason, the driver simply
> exits. The problem for libusb-0.1 is that I don't know of a clean way
to
> pass a udev device specification to usb_open(). (Insert rant about most USB
> devices not having unique serial numbers in their device descriptors.)
>
> Thoughts? Has anyone implemented a similar udev-to-application handoff in
> libusb-1.x?

Nope, no udev experience. And certainly not on other platforms.
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20140425/2c6c3650/attachment.html>