[Nut-upsdev] [nut-commits] svn commit r1073 - in trunk: . drivers

[Arjen de Korte]

> I think having this logic buried within libhid/libusb
> (libusb:libusb_open(), line 179 to 206) is ultimately a mistake,
> albeit one that I am probably responsible for.

You introduced this in r197... :-)

> Would it make sense to
> confine libhid to low-level operations, and leave the decision of
> trying to reopen vs. retrying to open to the high-level driver, in
> this case usbhid-ups?

That is probably a good idea.

> I envision that the code in usbhid-ups:reconnect_ups() could be
> changed to something like:
>
> hd = HIDOpenDevice(&udev, &curDevice, reopen_matcher, MODE_REOPEN);
> if (hd == NULL) {
>    /* reopening failed, try opening new device */
>    hd = HIDOpenDevice(&udev, &curDevice, regex_matcher, MODE_OPEN);
> }
> if (hd == NULL) {
>    return 0;
> }

Good point.

> Note "regex_matcher", not "reopen_matcher", in the second call, and
> also the changed MODE flag.

In order for this to work, we would need to split these into

	reopen_matcher
and
	regex_matcher->subdriver_matcher

So the linking between these in upsdrv_initups() should be removed.

> Then all the complicated stuff could be removed from libhid/libusb
> (different non-zero return codes for matchers etc).

Makes a lot of sense.

> I am not sure the code in libusb_open() is correct anyway, as matching
> a chain of matchers should mean matching *all* the matchers contained
> therein, not just the first possible one. For example, after a positive
> reopen_match, one should still check regex_match also.

The latter makes no sense to me. An exact match is a subset of (hopefully
one) of the devices that match the regex matcher. If you already have an
exact match, you therefor are guaranteed to have a match on the regex
matcher, since that is how you got there in the first place through the
initial call to HIDOpenDevice() with MODE_OPEN. The added regex match
would effectively be a no-op then.

> In other words, we are dealing with a loop like this:
>
> For each device curDevice: {
>   Try exact match, stop if it succeeds.
>   Try regex match, stop if it succeeds.
> }

This is the present (and wrong) way.

> This means we match the first device for which an exact match *or* a
> regex match is found. In other words, if the device that got
> disconnected is on bus 5, and some other loosely matching device is on
> bus 3, then we'll open the new device on bus 3, rather than reopening
> the disconnected device on bus 5.

Very true, I will fix this later today. I'll need fix a memory leak in
libusb_open anyway. We leak curDevice->Vendor, curDevice->Product,
curDevice->Serial and curDevice->Bus each time we run HIDOpenDevice. At
the default rate of one attempt every two seconds (which will change to
two attempts after the change below), this won't take long to eat up all
memory if a device is detached.

Maybe we should cap the number of (consecutive) times we attempt to
reconnect and bail out if we see too many failures? Or would that be a bad
idea?

> What we should be doing instead is:
> For each device curDevice: {
>   Try exact match, stop if it succeeds.
> }
> For each device curDevice: {
>   Try regex match, stop if it succeeds.
> }
>
> I think moving this logic up into usbhid-ups would simplify things
> greatly.

Indeed.

Best regards, Arjen
-- 
Eindhoven - The Netherlands
Key fingerprint - 66 4E 03 2C 9D B5 CB 9B  7A FE 7E C1 EE 88 BC 57