Re: fhandler_serial.cc: MARK and SPACE parity for serial port

[Corinna Vinschen <corinna-cygwin@cygwin.com>]

Hi Marek,

thanks for the patch.  This is a patch adding functionality so it's not
trivial.  Would you mind to express your willingness to put this patch
as well as further patches under 2-clause BSD license per the
"Before you get started" section of https://cygwin.com/contrib.html?

A few minor problems with this patch.

First of all, your MUA apparently broke the inline patch.  There are
line breaks in the patch, unexpected by git am, and the spacing in the
termios.h hunk is all wrong, see below.

If you can't change that in your MUA, please attach your patches as
plain text attachements, that usually helps.

On Jan 27 21:30, Marek Smetana via Cygwin-patches wrote:
> Hi,
> 
> This patch add MARK and SPACE parity support to serial port
> 
> ---
>  winsup/cygwin/fhandler_serial.cc    | 9 ++++++++-
>  winsup/cygwin/include/sys/termios.h | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/winsup/cygwin/fhandler_serial.cc
> b/winsup/cygwin/fhandler_serial.cc

Wrong line break

> index fd5b45899..23d69eca5 100644
> --- a/winsup/cygwin/fhandler_serial.cc
> +++ b/winsup/cygwin/fhandler_serial.cc
> @@ -727,7 +727,10 @@ fhandler_serial::tcsetattr (int action, const struct
> termios *t)

Wrong line break

>    /* -------------- Set parity ------------------ */
> 
>    if (t->c_cflag & PARENB)
> -    state.Parity = (t->c_cflag & PARODD) ? ODDPARITY : EVENPARITY;
> +    if(t->c_cflag & CMSPAR)
> +      state.Parity = (t->c_cflag & PARODD) ? MARKPARITY : SPACEPARITY;
> +    else
> +      state.Parity = (t->c_cflag & PARODD) ? ODDPARITY : EVENPARITY;

Please put the nested if/else into curly braces so the potential
for later changes breaking the if/else chain is reduced.

>    else
>      state.Parity = NOPARITY;
> 
> @@ -1068,6 +1071,10 @@ fhandler_serial::tcgetattr (struct termios *t)
>      t->c_cflag |= (PARENB | PARODD);
>    if (state.Parity == EVENPARITY)
>      t->c_cflag |= PARENB;
> +  if (state.Parity == MARKPARITY)
> +    t->c_cflag |= (PARENB | PARODD | CMSPAR);
> +  if (state.Parity == SPACEPARITY)
> +    t->c_cflag |= (PARENB | CMSPAR);
> 
>    /* -------------- Parity errors ------------------ */
> 
> diff --git a/winsup/cygwin/include/sys/termios.h
> b/winsup/cygwin/include/sys/termios.h

Wrong line break

> index 17e8d83a3..933851c21 100644
> --- a/winsup/cygwin/include/sys/termios.h
> +++ b/winsup/cygwin/include/sys/termios.h
> @@ -185,6 +185,7 @@ POSIX commands */
>  #define PARODD 0x00200
>  #define HUPCL 0x00400
>  #define CLOCAL 0x00800
> +#define CMSPAR  0x40000000 /* Mark or space (stick) parity.  */

Spacing here is completely off, probably due to your MUA.  Please note
that every definition is followed by a TAB and a SPACE for historical
reasons.  Ideally you do the same for the new CMSPAR definition.

> 
>  /* Extended baud rates above 37K. */
>  #define CBAUDEX 0x0100f
> 
> ---

Other than these minor formatting issues, your patch looks good,
so I'm looking forward to the fixed version.


Thanks,
Corinna