[ECOS] tty.c driver bug?

[ECOS] tty.c driver bug?

[Patrick O'Grady]

Hi, All--

Hmm... sounds like we're all in sync, looking around at the serial port
drivers!  I think that I've found another bug--tell me what you think. The
page
http://sourceware.cygnus.com/ecos/docs-latest/ref/ecos-ref/tty-driver.html
shows some of the different TTY input and output modes which can be
selected...  it implies that CYG_TTY_IN_FLAGS_CR will translate '\r' into
'\n', and that CYG_TTY_IN_FLAGS_CRLF will translate the pair '\r\n' into a
single '\n'...  however, looking in
packages/io/serial/current/src/common/tty.c, about line 212, shows that
CYG_TTY_IN_FLAGS_CR is ignored, and that CYG_TTY_IN_FLAGS_CRLF will
generate a pair of '\n':

(Code snip:)

...
            } else if ((c == '\n') || (c == '\r')) {
                clen = 2;
                if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
                    cyg_io_write(chan, "\n\r", &clen);
                }
                if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_CRLF) {
                    c = '\n';  // Map CR -> LF
                }
                buf[size-1] = c;
                break;
            }
...

Looks like CYG_TTY_IN_FLAGS_CRLF should really be CYG_TTY_IN_FLAGS_CR, and
that a previous character should be stored for use with
CYG_TTY_IN_FLAGS_CRLF.  I'll use CYG_TTY_IN_FLAGS_CRLF for the time
being--all I really care about is when the user presses '\r'.  Might be a
good idea that CYG_TTY_IN_FLAGS_CRLF returns a single '\n' for either
'\r\n' or '\n\r'...  I've seen some lame systems which output those
characters in reverse.  Cheers!

-patrick

patrick@softprocess.com

Re: [ECOS] tty.c driver bug?

[Jonathan Larmour]

Prompted by Sergei's recent bug report, I don't recall this mail from
Patrick being resolved. Is that right?

I believe Patrick's analysis is correct, and the code still appears to be
the same, but I want to check that this wasn't resolved some other way
before I fix.

Jifl

Patrick O'Grady wrote:
> 
> Hi, All--
> 
> Hmm... sounds like we're all in sync, looking around at the serial port
> drivers!  I think that I've found another bug--tell me what you think. The
> page
> http://sourceware.cygnus.com/ecos/docs-latest/ref/ecos-ref/tty-driver.html
> shows some of the different TTY input and output modes which can be
> selected...  it implies that CYG_TTY_IN_FLAGS_CR will translate '\r' into
> '\n', and that CYG_TTY_IN_FLAGS_CRLF will translate the pair '\r\n' into a
> single '\n'...  however, looking in
> packages/io/serial/current/src/common/tty.c, about line 212, shows that
> CYG_TTY_IN_FLAGS_CR is ignored, and that CYG_TTY_IN_FLAGS_CRLF will
> generate a pair of '\n':
> 
> (Code snip:)
> 
> ...
>             } else if ((c == '\n') || (c == '\r')) {
>                 clen = 2;
>                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
>                     cyg_io_write(chan, "\n\r", &clen);
>                 }
>                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_CRLF) {
>                     c = '\n';  // Map CR -> LF
>                 }
>                 buf[size-1] = c;
>                 break;
>             }
> ...
> 
> Looks like CYG_TTY_IN_FLAGS_CRLF should really be CYG_TTY_IN_FLAGS_CR, and
> that a previous character should be stored for use with
> CYG_TTY_IN_FLAGS_CRLF.  I'll use CYG_TTY_IN_FLAGS_CRLF for the time
> being--all I really care about is when the user presses '\r'.  Might be a
> good idea that CYG_TTY_IN_FLAGS_CRLF returns a single '\n' for either
> '\r\n' or '\n\r'...  I've seen some lame systems which output those
> characters in reverse.  Cheers!
> 
> -patrick
> 
> patrick@softprocess.com

-- 
Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS  Tel: +44 (1223) 728762
"Plan to be spontaneous tomorrow."  ||  These opinions are all my own fault

Re: [ECOS] tty.c driver bug?

[Gary Thomas]

AFAIK, nothing was done about this.

On 05-Apr-00 Jonathan Larmour wrote:
> Prompted by Sergei's recent bug report, I don't recall this mail from
> Patrick being resolved. Is that right?
> 
> I believe Patrick's analysis is correct, and the code still appears to be
> the same, but I want to check that this wasn't resolved some other way
> before I fix.
> 
> Jifl
> 
> Patrick O'Grady wrote:
>> 
>> Hi, All--
>> 
>> Hmm... sounds like we're all in sync, looking around at the serial port
>> drivers!  I think that I've found another bug--tell me what you think. The
>> page
>> http://sourceware.cygnus.com/ecos/docs-latest/ref/ecos-ref/tty-driver.html
>> shows some of the different TTY input and output modes which can be
>> selected...  it implies that CYG_TTY_IN_FLAGS_CR will translate '\r' into
>> '\n', and that CYG_TTY_IN_FLAGS_CRLF will translate the pair '\r\n' into a
>> single '\n'...  however, looking in
>> packages/io/serial/current/src/common/tty.c, about line 212, shows that
>> CYG_TTY_IN_FLAGS_CR is ignored, and that CYG_TTY_IN_FLAGS_CRLF will
>> generate a pair of '\n':
>> 
>> (Code snip:)
>> 
>> ...
>>             } else if ((c == '\n') || (c == '\r')) {
>>                 clen = 2;
>>                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
>>                     cyg_io_write(chan, "\n\r", &clen);
>>                 }
>>                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_CRLF) {
>>                     c = '\n';  // Map CR -> LF
>>                 }
>>                 buf[size-1] = c;
>>                 break;
>>             }
>> ...
>> 
>> Looks like CYG_TTY_IN_FLAGS_CRLF should really be CYG_TTY_IN_FLAGS_CR, and
>> that a previous character should be stored for use with
>> CYG_TTY_IN_FLAGS_CRLF.  I'll use CYG_TTY_IN_FLAGS_CRLF for the time
>> being--all I really care about is when the user presses '\r'.  Might be a
>> good idea that CYG_TTY_IN_FLAGS_CRLF returns a single '\n' for either
>> '\r\n' or '\n\r'...  I've seen some lame systems which output those
>> characters in reverse.  Cheers!
>> 
>> -patrick
>> 
>> patrick@softprocess.com
> 
> -- 
> Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS  Tel: +44 (1223) 728762
> "Plan to be spontaneous tomorrow."  ||  These opinions are all my own fault
> 

Re: [ECOS] tty.c driver bug?

[Jonathan Larmour]

Gary Thomas wrote:
> 
> AFAIK, nothing was done about this.
> 
> On 05-Apr-00 Jonathan Larmour wrote:
> > Prompted by Sergei's recent bug report, I don't recall this mail from
> > Patrick being resolved. Is that right?
> >
> > Patrick O'Grady wrote:
> >>
http://sourceware.cygnus.com/ecos/docs-latest/ref/ecos-ref/tty-driver.html
> >> shows some of the different TTY input and output modes which can be
> >> selected...  it implies that CYG_TTY_IN_FLAGS_CR will translate '\r' into
> >> '\n', and that CYG_TTY_IN_FLAGS_CRLF will translate the pair '\r\n' into a
> >> single '\n'...  however, looking in
> >> packages/io/serial/current/src/common/tty.c, about line 212, shows that
> >> CYG_TTY_IN_FLAGS_CR is ignored, and that CYG_TTY_IN_FLAGS_CRLF will
> >> generate a pair of '\n':
[snip]
> >> Looks like CYG_TTY_IN_FLAGS_CRLF should really be CYG_TTY_IN_FLAGS_CR, and
> >> that a previous character should be stored for use with
> >> CYG_TTY_IN_FLAGS_CRLF.  I'll use CYG_TTY_IN_FLAGS_CRLF for the time
> >> being--all I really care about is when the user presses '\r'.

I think the attached patch should do the trick to fix this. Let me know
what you think Patrick.

>  Might be a
> >> good idea that CYG_TTY_IN_FLAGS_CRLF returns a single '\n' for either
> >> '\r\n' or '\n\r'...  I've seen some lame systems which output those
> >> characters in reverse.  Cheers!

Well, in that mode it's easiest just to pretend the '\r' didn't exist.
That's subtly different behaviour from treating it as '\n\r' because if we
did that, it would mean getting the '\n' and deliberately not returning
from tty_read() until we got the '\r'. We could add that complexity to cope
with such systems, but let's not - by keeping it this way, setting
CYG_TTY_IN_FLAGS_CRLF means it works when connected to systems that send
CRLF *and* ones that send just LF.

Jifl
-- 
Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS  Tel: +44 (1223) 728762
"Plan to be spontaneous tomorrow."  ||  These opinions are all my own fault
Index: tty.c
===================================================================
RCS file: /cvs/ecc/ecc/io/serial/current/src/common/tty.c,v
retrieving revision 1.11
diff -u -5 -p -c -r1.11 tty.c
*** tty.c	2000/04/05 18:55:01	1.11
--- tty.c	2000/04/07 03:03:58
*************** tty_read(cyg_io_handle_t handle, void *_
*** 208,240 ****
              *len = size;
              return res;
          }
          buf[size++] = c;
          if ((priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_BINARY) == 0) {
!             if ((c == '\b') || (c == 0x7F)) {
                  size -= 2;  // erase one character + 'backspace' char
                  if (size < 0) {
                      size = 0;
                  } else if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
                      clen = 3;
                      cyg_io_write(chan, "\b \b", &clen);
                  }
!             } else if ((c == '\n') || (c == '\r')) {
!                 clen = 2;
                  if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
!                     cyg_io_write(chan, "\r\n", &clen);
                  }
!                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_CRLF) {
                      c = '\n';  // Map CR -> LF
                  }
                  buf[size-1] = c;
!                 break;
!             } else {
                  if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
                      clen = 1;
                      cyg_io_write(chan, &c, &clen);
                  }
              }
          }
      }
      *len = size;
      return ENOERR;
--- 208,256 ----
              *len = size;
              return res;
          }
          buf[size++] = c;
          if ((priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_BINARY) == 0) {
!             switch (c) {
!             case '\b':    /* drop through */
!             case 0x7f:
                  size -= 2;  // erase one character + 'backspace' char
                  if (size < 0) {
                      size = 0;
                  } else if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
                      clen = 3;
                      cyg_io_write(chan, "\b \b", &clen);
                  }
!                 break;
!             case '\r':
!                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_CRLF) {
!                     /* Don't do anything because a '\n' will come next */
!                     break;
!                 }
!                 /* drop through */
!             case '\n':
                  if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
!                     if (priv->dev_info.tty_out_flags & CYG_TTY_OUT_FLAGS_CRLF) {
!                         clen = 2;
!                         cyg_io_write(chan, "\r\n", &clen);
!                     } else {
!                         clen = 1;
!                         cyg_io_write(chan, "\n", &clen);
!                     }
                  }
!                 if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_CR) {
                      c = '\n';  // Map CR -> LF
                  }
                  buf[size-1] = c;
!                 *len = size;
!                 return ENOERR;
!             default:
                  if (priv->dev_info.tty_in_flags & CYG_TTY_IN_FLAGS_ECHO) {
                      clen = 1;
                      cyg_io_write(chan, &c, &clen);
                  }
+                 break;
              }
          }
      }
      *len = size;
      return ENOERR;

Re: [ECOS] tty.c driver bug?

[Chris Gray]

On Fri, 7 Apr 2000, Jonathan Larmour wrote:

> Gary Thomas wrote:
> > > Patrick O'Grady wrote:
> >  Might be a
> > >> good idea that CYG_TTY_IN_FLAGS_CRLF returns a single '\n' for either
> > >> '\r\n' or '\n\r'...  I've seen some lame systems which output those
> > >> characters in reverse.  Cheers!
> 
> Well, in that mode it's easiest just to pretend the '\r' didn't exist.

Unless you also want to handle systems which output just '\r' (e.g. MacOS
uses \r internally as a record terminator.)

I happen to have a table of termios flgs open on my desk (I must have done
something pretty evil in my previous life), and I see options to map CR to
NL, map NL to CR, and ignore CR completely.  Another data point: method
readLine of Java class java/io/BufferedReader is defined to accept
'\n', '\r', or '\r\n' (but not the '\n\r' mutant). 

-- 

  Eur. Ing. Chris Gray MBCS C. Eng.       chris.gray@smartmove.be

Re: [ECOS] tty.c driver bug?

[Jonathan Larmour]

Chris Gray wrote:
> 
> On Fri, 7 Apr 2000, Jonathan Larmour wrote:
> 
> > Gary Thomas wrote:
> > > > Patrick O'Grady wrote:
> > >  Might be a
> > > >> good idea that CYG_TTY_IN_FLAGS_CRLF returns a single '\n' for either
> > > >> '\r\n' or '\n\r'...  I've seen some lame systems which output those
> > > >> characters in reverse.  Cheers!
> >
> > Well, in that mode it's easiest just to pretend the '\r' didn't exist.
> 
> Unless you also want to handle systems which output just '\r' (e.g. MacOS
> uses \r internally as a record terminator.)

In which case you should have set CYG_TTY_IN_FLAGS_CR! :-) No matter what
the setting of the flags, eCos's internal representation of "newline" is
'\n' only.
 
> I happen to have a table of termios flgs open on my desk (I must have done
> something pretty evil in my previous life)

First person to recommend porting termcap to eCos gets shot :-).

Ah, that's reminded me - I must get the docs updated.

Jifl
-- 
Red Hat, 35 Cambridge Place, Cambridge, UK. CB2 1NS  Tel: +44 (1223) 728762
"Plan to be spontaneous tomorrow."  ||  These opinions are all my own fault

Re: [ECOS] tty.c driver bug?

[Chris Gray]

On Fri, 7 Apr 2000, Jonathan Larmour wrote:

> First person to recommend porting termcap to eCos gets shot :-).

Won't be me, I can assure you.  What a @#$%^&* mess.> 

-- 

  Eur. Ing. Chris Gray MBCS C. Eng.       chris.gray@smartmove.be