Nut upsdev - Nov 2013 - [PATCH] al175: updated driver, please restore it

Arjen,

On Fri, Feb 06, 2009 at 05:40:56PM +0100, Arjen de Korte
wrote:
> Citeren Kirill Smelkov <kirr at mns.spb.ru>:
>
>> I'm a bit overworked right now, so I'll try to have a rest and
will
>> reply with a fresh head.
>
> If you need any help in this area, please say so and I'll be willing to
> invest some time in this to get you started.
Sorry for so loooooong delay and thank you.

I just wanted to say that I'm still here -- I decided to get back to
this issue when I'll have AL175 hardware at hand, so that I can test
reworked driver. Otherwise, I'm risking too much to break our existing
setups (located some 1000s km from main office).

Unfortunately, it could take a long time until live AL175 arives :(


Hope I'll do it finally,
Kirill

On Tue, Jan 27, 2009 at 04:39:13PM +0100, Arnaud Quette
wrote:
> Hi Kirill,
> 
> 2009/1/27 Kirill Smelkov <kirr at mns.spb.ru>
> 
> > On Tue, Jan 13, 2009 at 05:58:23PM +0300, Kirill Smelkov wrote:
> > > Arjen, Arnaud,
> > > first of all, I'm sorry for my late reply.
> > >
> > > If it's not too late, here is updated al175:
> > >
> > >
> > > On Fri, Dec 26, 2008 at 11:37:04PM +0100, Arjen de Korte wrote:
> > > > Citeren Kirill Smelkov <kirr at mns.spb.ru>:
> > > >
> > > > [...]
> > > >
> > > >> Yes, "All the world is not an x86 Linux box,"
and I've tried to make
> > all the
> > > >> world happy.
> > > >
> > > > I raised several other issues as well, of which the most
important one
> > > > isn't addressed. The driver still uses 'alloca'
which isn't portable
> > (it
> > > > is not in POSIX). I also indicated how this could be solved
by using
> > > > automatic variables. In any case I'm not conviced there
is no
> > > > alternative here, so this is a show stopper for now.
> > >
> > > I've reworked it not to use alloca and to use auto-variables
+ xmalloc
> > > in one place.
> > >
> > > Now it usually looks like
> > >
> > >     raw_data    reply;
> > >     byte_t      reply_buf[11];
> > >
> > >     ...
> > >
> > >     raw_alloc_onstack(&reply, reply_buf);
> > >
> > > > Also something I really don't like in this driver is the
use of
> > alarm().
> > > > Although this is presently used in the 'net-xml'
driver (in case an
> > older
> > > > neon library is detected), there is no need for this here.
We have
> > > > timeouts on *all* ser_get* functions and these *must* be
used instead.
> > > > Note that with using timeouts I mean something different
than setting
> > > > them to '999' seconds and setting an alarm() to
breakout early. Unless
> > > > there is a really good reason to use an alarm() (ie, there
is no
> > > > alternative), this shouldn't be used.
> > >
> > > I've used alarm in the first place for the following reason:
> > >
> > > Let's look at e.g. upsdrv_updateinfo() -- it reads several
al175
> > > registers, and each read_register consists of a chat between host
and
> > > al175.
> > >
> > > And I want the whole transaction to be less than say 5 seconds.
What
> > > should I do? manually calculate read/write time budget left for
_each_
> > > read or write?
> > >
> > > I think this is not practical.
> > >
> > > That's why I simply
> > >
> > > 1. arm the timer via alarm(T) to fire after T seconds
> > > 2. do all the I/O, logic, etc...
> > > 3. if I/O would stale at *any* place, it will be *always* aborted
right
> > >    after T seconds since the transaction start (thanks to alarm)
> > > 4. after all the I/O transaction is done -- reset the timer.
> > >
> > > And exactly for this reason 999 is used as infinity which means
'much
> > > more longer than _any_ T we'll pass to alarm() ever'. If
there is a way
> > > to actually pass 'please never timeout on select',
I'd be happy to
> > > rework the code.
> > >
> > > Also according to mans, alarm(2) is in SVr4, POSIX.1-2001 and
4.3BSD.
> > > That's why I think using alarm() makes sense.
> > >
> > > But if there is a similiar functionality in NUT to set whole
transaction
> > > timeout, I'll be happy to rework the code to use what NUT
provides.
> > >
> > > > Even in the updated driver, I see quite a couple of
fprintf() lines.
> > > > Although these are in most (all?) cases encased in a 'if
> > > > (nut_debug_level > 0)', we have the upsdebug*
functions for that. Do
> > > > yourself (and your fellow developers) a favor and use those
instead.
> > >
> > > > Another thing related to this is the XTRACE macro. This will
only use a
> > > > single (1) debug level to show output. Please use a layered
approach,
> > > > where various levels of debugging information are used.
> > >
> > > I've split debug/trace output into several levels:
> > >
> > >     1       /* user-level trace (status, instcmd, etc */
> > >     2       /* proto handling info  */
> > >     3       /* IO trace             */
> > >
> > >
> > > so with -D the user will see only high level stuff, with -DD --
> > > high-level info + any information regarding COMLI proto handling,
and
> > > with -DDD -- everything includiong full IO trace.
> > >
> > > Can't test it because I don't have the hardware now, but
I hope this is
> > > ok.
> > >
> > > (one place still uses fprintf(stderr, ...) but that's because
it is
> > >  difficult to construct one line through several calls to
upsdebug,
> > >  although sure this is doable, like upsdebug_hex does. Mine has
pretty
> > >  printing for control ascii characters, and I'd better leave
it there,
> > >  because as I recall this was usefull for debugging. I hope this
is not
> > >  a show-stopper.)
> > >
> > >
> > > > It also helps understanding the driver if you keep the
number of
> > > > macro's limited.
> > >
> > > The reason XTRACE was introduced in the first place is to
automatically
> > > put a function name as prefix into trace output through the use
of
> > > __FUNCTION__. if it is (again) a protability problem, we could
always
> > > workaround this as follows (taken from gcc manual):
> > >
> > >
> > >      #if __STDC_VERSION__ < 199901L
> > >      # if __GNUC__ >= 2
> > >      #  define __func__ __FUNCTION__
> > >      # else
> > >      #  define __func__ "<unknown>"
> > >      # endif
> > >      #endif
> > >
> > >
> > >
> > > Here is the interdiff wrt recent version as posted to the list,
and the
> > > full patch:
> >
> > [...]
> >
> > Arnaud, Arjen,
> >
> > So what would we do about this patch?
> >
> > I've tried to address the issues you've raised, and to
describe my
> > rationale behind alarm().
> >
> > If it can't go in -- that's ok with me, but could you please
at least
> > tell me what you objections (if any) are now.
> >
> 
> sorry, both Arjen and I were caught by real life!
> 
> we've had a discussion not long ago about your driver.
> the conclusion is that there are still show stoppers for including it back
> in the tree.
> so it's a no-go for 2.4.0!
> Remember that our aim is not to bother drivers developers, but to ensure
> that their work will be maintainable and maintained even if they
> disappear...
> 
> Arjen should get back to you soon and point/help you in doing what is
> needed.
> Though he already stated most (if not all) the points in the previous
mails.
> In the meantime, 2.2.2 will still be available, and you can always use a
> patched 2.4.0 including al175
> 
> so, keep your motivation, we'll end up in succeeding ;-)

On Fri, May 08, 2009 at 10:38:07AM +0400, Kirill Smelkov
wrote:
> Arjen,
> 
> On Fri, Feb 06, 2009 at 05:40:56PM +0100, Arjen de Korte wrote:
> > Citeren Kirill Smelkov <kirr at mns.spb.ru>:
> >
> >> I'm a bit overworked right now, so I'll try to have a rest
and will
> >> reply with a fresh head.
> >
> > If you need any help in this area, please say so and I'll be
willing to
> > invest some time in this to get you started.
> 
> Sorry for so loooooong delay and thank you.
> 
> I just wanted to say that I'm still here -- I decided to get back to
> this issue when I'll have AL175 hardware at hand, so that I can test
> reworked driver. Otherwise, I'm risking too much to break our existing
> setups (located some 1000s km from main office).
> 
> Unfortunately, it could take a long time until live AL175 arives :(
> 
> 
> Hope I'll do it finally,
> Kirill

Arnaud, Arjen, everyone,

If it still makes sense, I'm here again, with reworked al175 driver.

I've tried to clean it up, but unfortunately no longer have hardware to
test and will not have any in foreseeable future, so the driver was
reworked to meet the project code quality criteria, without testing on
real hardware.  Some bugs may have crept in.

Please find main description in github issue tracker

    https://github.com/networkupstools/nut/issues/69

and, for completness, both patches attached to this mail.


Thanks beforehand,
Kirill
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-common-upsdebug_ascii-to-dump-a-message-in-ascii.patch
Type: text/x-diff
Size: 3695 bytes
Desc: not available
URL:
<http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20131118/b15b631e/attachment-0002.patch>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-al175.patch
Type: text/x-diff
Size: 40599 bytes
Desc: not available
URL:
<http://lists.alioth.debian.org/pipermail/nut-upsdev/attachments/20131118/b15b631e/attachment-0003.patch>