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

[Carlos Rodrigues]

On Nov 25, 2007 9:53 PM, Arjen de Korte <nut+devel at de-korte.org> wrote:
> Sure. As it used to be, the detection phase would try to get a status from
> the UPS five times. If it got three replies (or more), it would consider
> this a positive identification. After that, it would try to read a status
> once more and if that failed, it would abort. This was suboptimal for two
> reasons:
>
> 1) It is unreasonable to allow 2 out of 5 tries to fail in the detection
> phase, but terminate unconditionally if the sixth try failed.

Not really, because the point isn't getting a positive response, is
knowing if we always get a consistent response, and get an idea about
how reliabile is the communication with the UPS. And to know this, we
need a _fixed_ number of samples. If the idea were just to check if
there's a supported UPS on the other side of the cable, a single
success would be enough.

Having an idea of how reliable the communication is, in the least,
would allow the user to know about it an not be surprised if his box
suddenly shuts itself down one day because the driver lost
communications with the UPS.

> 2) If the first three attempts to read the Q1 status are successful, the
> outcome of the detection is already known, so reading two more is not
> going to change the outcome anymore.
>
> If a device was detected previously, it will be detected in the same (or
> less) attempts now. This hasn't changed. For devices that return a correct
> answer every time (many do) this means that the number of Q1 queries is
> cut in half.

Thats's an unnecessary optimization, and isn't fixing anything that's
broken. Driver startup time isn't important, as long as it is kept
reasonably short, which it already is (other drivers take much longer
to initialize and even those never got any complaints from users).

Most time is actually spent waiting between sending characters anyway
(and that could be reduced, if we could properly test with a
reasonable number of different models, but I digress).

> It was in the trunk. By changing it in the way I did, we can re-use the
> result from the third successful read later on and prevent situation #1
> altogether. It also makes sure we don't waste more time in the detection
> phase than absolutely needed.

Like I said, an unnecessary optimization.

> The addition of the buffer flushing was a bad idea after all and has been
> removed already.

Ok, I saw the later commit notifications (I was wondering when did I
add those ser_flush_io()'s...).

> I beg to differ here for reason #1. Also, duplication (almost) the same
> code is not a good idea in any driver.

It is duplicated now, but that won't always be the case. For instance,
there will probably be a need to check if the UPS always echoes back
unknown commands (to be able to properly notify the users about
unavailable instant commands). My UPS, for example, doesn't seem to be
doing this (it sometimes echoes back, and other times just stays
silent).

To do this, I'll have to add that function back...

I commit anything changes for now, however. (That would be pointless,
I have nothing to add yet.)

-- 
Carlos Rodrigues