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

[Arjen de Korte]

> Author: agordeev-guest
> Date: Wed Oct  3 00:09:21 2007
> New Revision: 1136
>
> Log:
>  - drivers/megatec_usb.c: added ability to do subdriver-specific
>    initialization, and moved some shared code to agiler init function
>    since krauler doesn't need it.

[...]

I'm not very thrilled about this patch:

1) Down at the subdriver level, one shouldn't use ser_get_line() anymore,
but usb->get_interrupt() instead or alternatively, get_data_agiler().
Hopping back-and-forth between various layers in the code is bad coding
style as it makes the code difficult to understand.

2) So what is the benefit of this patch? It seem to try to optimize
something away that complicates the subdrivers and really provides no
runtime advantages (the ser_open() function is only run once).

3) Something you're probably not responsible for, but if flushing the
input buffers is needed, this is something that should be done in main
driver body (megatec.c). The driver doesn't flush I/O buffers, so the
implementation of the Megatec protocol is apparently robust enough to
tolerate occasional garbage characters in the buffers.

Therefor, I think flushing the buffers in the subdrivers was a bad idea in
the way it was implemented before (and even more in the way it is now).

If you have some time available to work on this driver, a thing that *is*
definitly lacking in the existing megatec_usb.c, is a proper reconnect
function (and error handling that is needed to notice when this should be
called). The driver as it is won't tolerate the occasional disconnect you
may see on USB ports. If for some reason, the devices are enumerated
again, the driver will not find the UPS again until it is started again.
See for an example how to solve this, the tripplite_usb driver.

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