Bug 91710

Description of problem:

This is from a discussion on ecos-patches around 21/03/03:

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