Re: [Fwd: termios - bad behaviour with VMIN]

[Gary Thomas <gary at mlbassoc dot com>]

On Thu, 2003-03-20 at 20:22, Jonathan Larmour wrote:
> Gary Thomas wrote:
> > No response to this either.  Jonathan - I am most interested in this.  
> 
> Sorry, firstly I was away all of last week, and then I had already started 
> looking at this yesterday and then started again today, but then my e-mail 
> went wrong and I couldn't send anything most of the day :-(.
> 

Understood (and after this morning's trouble with disk drives, I
*really* understand).

> > which tells me that if I have a termios device in non-canonical
> > mode and I set VMIN, then a read to that device will not be
> > satisfied until at least VMIN characters arrive.  Currently,
> > this does not happen; it returns 0, with no error, if there
> > are no characters available.
> 
> I can see that would be the case, yes. The driver was originally written 
> only to support MIN = 0, TIME = 0. Recently I tried to get MIN > 0 
> working, but was doing it blind and waiting for someone else to confirm 
> (which they didn't, but I thought I'd found the bug so checked it in). But 
> clearly it still wasn't right.
> 
> > This patch fixes it and gives what I think is proper behaviour:
> > 
> > Index: io/serial/current/src/common/termiostty.c
> > ===================================================================
> > RCS file: /misc/cvsfiles/ecos/packages/io/serial/current/src/common/termiostty.c,v
> > retrieving revision 1.4
> > diff -u -5 -p -r1.4 termiostty.c
> > --- io/serial/current/src/common/termiostty.c	14 Feb 2003 11:50:12 -0000	1.4
> > +++ io/serial/current/src/common/termiostty.c	9 Mar 2003 15:39:58 -0000
> > @@ -659,11 +659,18 @@ termios_read(cyg_io_handle_t handle, voi
> >          cyg_uint32 dbc_len = sizeof( dev_buf_conf );
> >          res = cyg_io_get_config( chan,
> >                                   CYG_IO_GET_CONFIG_SERIAL_BUFFER_INFO,
> >                                   &dev_buf_conf, &dbc_len );
> >          CYG_ASSERT( res == ENOERR, "Query buffer status failed!" );
> > -        *len = *len < dev_buf_conf.rx_count ? *len : dev_buf_conf.rx_count;
> > +        if (dev_buf_conf.rx_count > 0) {
> > +            // Adjust length to be max characters currently available
> > +            *len = *len < dev_buf_conf.rx_count ? *len : dev_buf_conf.rx_count;
> > +        } else if (t->c_cc[VMIN] == 0) {
> > +            // No chars available - don't block
> > +            *len = 0;
> > +            return ENOERR;
> > +        }
> 
> This isn't quite right. You've got to respect VMIN != 0 no matter what in 
> non-canonical mode. The way this is written, it can return fewer than VMIN 
> bytes if there are more than 0 but fewer than VMIN bytes available in the 
> buffer at the time it's called. This might not affect your app of course.
> 
> But the solution is tricky, because we then only want to read a maximum of 
> *len bytes out of the lower level serial buffer, but not return until 
> there are more than VMIN of them in there, and for VMIN > *len we need to 
> take a different approach to the way the code works now.
> 
> Unfortunately the way the cyg_io interface is structured I can't see any 
> simple way to do this without either having to create and manage a new 
> intermediate buffer to store characters, or perhaps add a special 
> set_config primitive that allows us to block until a certain number of 
> bytes are in the buffer. The latter isn't very clean, but would save a lot 
> of hassle and would probably be what I'd choose.
> 
> I was thinking about trying to solve this, but I think it'll take a bit 
> long for something I'm not actively meant to be working near anyway. So 
> I'll let you choose between fixing, bugzilla'ing, or adding a big FIXME 
> with all the above in a comment :-). Actually, if you bugzilla, a single 
> line FIXME with the bug number would probably be a good idea - no need for 
> a ChangeLog or ecos-patches message for something that trivial.
> 
> [ I've also noticed that MAX_INPUT should be defined in limits.h to be the 
> size of the input serial buffer, which unfortunately has to be determined 
> programmatically beacuse it requires a call to serial_get_config with 
> CYG_IO_GET_CONFIG_SERIAL_BUFFER_INFO. I'll just bugzilla that not myself. ]
> 
> > @@ -752,14 +759,11 @@ termios_read(cyg_io_handle_t handle, voi
> >                  }
> >                  if ( t->c_iflag & INLCR )
> >                      c = '\r';
> >                  returnnow = true; // FIXME: true even for INLCR?
> >              } // else if
> > -        } else { // non-canonical mode
> > -            if ( t->c_cc[ VMIN ] && (size+1 >= t->c_cc[ VMIN ]) )
> > -                returnnow = true;
> > -        } // else
> > +        } // if 
> >  
> >  #ifdef CYGSEM_IO_SERIAL_TERMIOS_USE_SIGNALS
> >          if ( (t->c_lflag & ISIG) && (t->c_cc[ VINTR ] == c) ) {
> >              discardc = true;
> >              if ( 0 == (t->c_lflag & NOFLSH) )
> > @@ -789,10 +793,16 @@ termios_read(cyg_io_handle_t handle, voi
> >              if ( t->c_lflag & ECHO ) {
> >                  clen = 1;
> >                  // FIXME: what about error or non-blocking?
> >                  termios_write( handle, &c, &clen );
> >              }
> > +        }
> > +
> > +        if ( (t->c_lflag & ICANON) == 0 ) {
> > +            // Check to see if read has been satisfied
> > +            if ( t->c_cc[ VMIN ] && (size >= t->c_cc[ VMIN ]) )
> > +                returnnow = true;
> >          }
> 
> This bit makes sense.

It's also the part that fixed the bug I was after.

I'll see what I can do to help you iron out the other parts as well.
If you want, I can back out the changes at the top which are arguably
incorrect.

-- 
------------------------------------------------------------
Gary Thomas                 |
MLB Associates              |  Consulting for the
+1 (970) 229-1963           |    Embedded world
http://www.mlbassoc.com/    |
email: <gary at mlbassoc dot com>  |
gpg: http://www.chez-thomas.org/gary/gpg_key.asc
------------------------------------------------------------