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

> 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

On 10/3/07, Arjen de Korte <nut+devel at de-korte.org>
wrote:
> 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.
I can do this in "megatec.c" if necessary, just say the word.

And yes, the implementation has to be robust against garbage because
the hardware itself sometimes goes crazy and returns crap.

-- 
Carlos Rodrigues

On Wed, 03 Oct 2007 11:18:21 +0400, Arjen de Korte  
<nut+devel at de-korte.org> wrote:
>
>> 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.
>
Thanks!
I've overlooked it, will fix.
> 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).
>
Does it really complicate subdrivers? I think not.
I don't know whether flushing input buffers is terribly necessary in
agiler subdriver, but it's definitely not needed by krauler. And this
has nothing to do with megatec driver, it is an agiler device-specific
thing.

The benefit? I don't get messages like "get_data_krauler: no command
set"
while not breaking agiler subdriver which I cannot test. What else can I
do to keep it working? I just dont't like hacking it completely without
testing.

Btw, did you received any response from the owners of agiler devices?
I think you've missed Eric Raymond in the list. I'll contact him.
> 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.
>
Yes, I know about this. Ok, let's do it.
I plan to call reconnection function (which will try to reconnect once)
in ser_send_pace() and ser_get_line() if set/get_data_* reports that
connection is lost. Is this ok?

-- 
   Alexander