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

[Alexander I. Gordeev]

On Wed, Oct 03, 2007 at 01:09:25PM +0200, Arjen de Korte wrote:
> > Does it really complicate subdrivers? I think not.
> 
> Yes. Until this change, the subdrivers had a really clean, simple
> interface - reading and writing. You added another function to the agiler
> subdriver, which largely duplicates an existing one and isn't required at
> this (low) level.
> 
> After Carlos has added the ser_flush_io() function to the megatec.c
> subdriver, we will probably need the following:
> 
> int ser_flush_io(int upsfd)
> {
> 	int	i = 0;
> 
> 	while (subdriver->get_data(buf, buflen) > 0) {
> 		if (++i > 10) {
> 			break;
> 		}
> 	}
> 	return 0;
> }
> 
> Although this will be basically a no-op for the Krauler subdriver, this is
> certainly not worth an additional subdriver function.
> 

You are right it's better to use ser_flush_io for this.
But I still disagree on not having a special subdriver function to
handle this. (Of course, the current ones should be renamed to, say,
flush_<subdriver>).

> > 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.
> 
> I know.
> 

So it's better to keep it inside the subdriver.

> > 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.
> 
> Don't mistake upsdebugx() messages for error messages. It is telling you
> just that no command is set. This is *not* an error and therefor doesn't
> need fixing. It is perfectly OK to call the get_data_krauler() function
> without setting a command (robustness). Users will never see the output
> from the debugging command.
> 

I don't agree with you.
Yes, it certainly is not an error, it is useless but tolerated debug message.
This is like arguing about compiler warnings: some people tolerate them and
some try to eliminate them all. Well, I'm the latter one. This is just a
point of view.
I think this message will make more unclearness (for everybody not in the
context) than having subdriver-specific functions. I am even going to comment
them :)

> > Btw, did you received any response from the owners of agiler devices?
i> 
> Yes. The updated agiler subdriver works as expected and is now also
> included in nut-2.2.1-pre.
> 

Great.

[...]
> > 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?
> 
> It depends on how you do this, but essentially this is what needs to be
> done. Note that there are quite a number of error codes that need to be
> dealt with:
> 
> 	-EBUSY (other driver tried to claim this interface)
> 	-EPERM
> 	-EPIPE
> 	-ENODEV
> 	-EACCES
> 	-EIO
> 	-ENOENT
> 
> Best regards, Arjen
> 

Thanks for the note!
I'll first study other usb drivers.

--
  Alexander