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

[Jonathan Larmour <jifl at eCosCentric dot com>]

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 :-(.

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

Jifl
--
eCosCentric    http://www.eCosCentric.com/    The eCos and RedBoot experts
--[ "You can complain because roses have thorns, or you ]--
--[  can rejoice because thorns have roses." -Lincoln   ]-- Opinions==mine