fhandler_serial.cc: MARK and SPACE parity for serial port

fhandler_serial.cc: MARK and SPACE parity for serial port

[Marek Smetana]

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
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)
   /* -------------- 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;
   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
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.  */

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

---

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

[Corinna Vinschen]

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

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

[Corinna Vinschen]

Oh, btw...

On Jan 28 11:08, Corinna Vinschen via Cygwin-patches wrote:
> Hi Marek,
> 
> thanks for the patch.  [...]
> > 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.  */

Why did you choose such a big value here?  Wouldn't it be nicer just to
follow up with 

  #define CMSPAR 0x10000

or am I missing something here?

Also, on second thought I think CMSPAR should follow CRTSCTS, a few
lines below, because of its numerical value higher than CRTSCTS.


Thanks,
Corinna

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

[Brian Inglis]

On 2021-01-28 03:14, Corinna Vinschen via Cygwin-patches wrote:
> On Jan 28 11:08, Corinna Vinschen via Cygwin-patches wrote:
>> Hi Marek,
>> thanks for the patch.  [...]
>>> 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.  */
> 
> Why did you choose such a big value here?  Wouldn't it be nicer just to
> follow up with
> 
>    #define CMSPAR 0x10000
> 
> or am I missing something here?

GLIBC/Linux compatibility:
https://sourceware.org/git/?p=glibc.git&a=search&h=HEAD&st=grep&s=define+CMSPAR

sysdeps/unix/sysv/linux/alpha/bits/termios-baud.h	
   23 #ifdef __USE_MISC
   24 # define CBAUD  0000037
   25 # define CBAUDEX 0000000
   26 # define CMSPAR   010000000000          /* mark or space (stick) parity */
   27 # define CRTSCTS  020000000000          /* flow control */
   28 #endif
sysdeps/unix/sysv/linux/bits/termios-baud.h	
   23 #ifdef __USE_MISC
   24 # define CBAUD   000000010017 /* Baud speed mask (not in POSIX).  */
   25 # define CBAUDEX 000000010000 /* Extra baud speed mask, included in CBAUD.
   26                                  (not in POSIX).  */
   27 # define CIBAUD  002003600000 /* Input baud rate (not used).  */
   28 # define CMSPAR  010000000000 /* Mark or space (stick) parity.  */
   29 # define CRTSCTS 020000000000 /* Flow control.  */
   30 #endif
sysdeps/unix/sysv/linux/powerpc/bits/termios-baud.h	
   23 #ifdef __USE_MISC
   24 # define CBAUD  0000377
   25 # define CBAUDEX 0000020
   26 # define CMSPAR   010000000000          /* mark or space (stick) parity */
   27 # define CRTSCTS  020000000000          /* flow control */
   28 #endif
sysdeps/unix/sysv/linux/sparc/bits/termios-baud.h	
   23 #ifdef __USE_MISC
   24 # define CBAUD   0x0000100f
   25 # define CBAUDEX 0x00001000
   26 # define CIBAUD  0x100f0000     /* input baud rate (not used) */
   27 # define CMSPAR  0x40000000     /* mark or space (stick) parity */
   28 # define CRTSCTS 0x80000000     /* flow control */
   29 #endif

> Also, on second thought I think CMSPAR should follow CRTSCTS, a few
> lines below, because of its numerical value higher than CRTSCTS.

GLIBC/Linux normally has it lower:
$ grep -C2 'define\s\+CMSPAR' /usr/include/**/*.h
/usr/include/asm-generic/termbits.h:
#define  B4000000 0010017
#define CIBAUD	  002003600000	/* input baud rate */
#define CMSPAR	  010000000000	/* mark or space (stick) parity */
#define CRTSCTS	  020000000000	/* flow control */

--
/usr/include/i386-linux-gnu/bits/termios.h:
#ifdef __USE_MISC
# define CIBAUD	  002003600000		/* input baud rate (not used) */
# define CMSPAR   010000000000		/* mark or space (stick) parity */
# define CRTSCTS  020000000000		/* flow control */
#endif
-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in binary units and prefixes, physical quantities in SI.]

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

[Brian Inglis]

On 2021-01-28 10:17, Brian Inglis wrote:
> On 2021-01-28 03:14, Corinna Vinschen via Cygwin-patches wrote:
>> On Jan 28 11:08, Corinna Vinschen via Cygwin-patches wrote:
>>> Hi Marek,
>>> thanks for the patch.  [...]
>>>> 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.  */
>>
>> Why did you choose such a big value here?  Wouldn't it be nicer just to
>> follow up with
>>
>>    #define CMSPAR 0x10000
>>
>> or am I missing something here?
> 
> GLIBC/Linux compatibility:
> https://sourceware.org/git/?p=glibc.git&a=search&h=HEAD&st=grep&s=define+CMSPAR
> 
> sysdeps/unix/sysv/linux/alpha/bits/termios-baud.h
>    23 #ifdef __USE_MISC
>    24 # define CBAUD  0000037
>    25 # define CBAUDEX 0000000
>    26 # define CMSPAR   010000000000          /* mark or space (stick) parity */
>    27 # define CRTSCTS  020000000000          /* flow control */
>    28 #endif
> sysdeps/unix/sysv/linux/bits/termios-baud.h
>    23 #ifdef __USE_MISC
>    24 # define CBAUD   000000010017 /* Baud speed mask (not in POSIX).  */
>    25 # define CBAUDEX 000000010000 /* Extra baud speed mask, included in CBAUD.
>    26                                  (not in POSIX).  */
>    27 # define CIBAUD  002003600000 /* Input baud rate (not used).  */
>    28 # define CMSPAR  010000000000 /* Mark or space (stick) parity.  */
>    29 # define CRTSCTS 020000000000 /* Flow control.  */
>    30 #endif
> sysdeps/unix/sysv/linux/powerpc/bits/termios-baud.h
>    23 #ifdef __USE_MISC
>    24 # define CBAUD  0000377
>    25 # define CBAUDEX 0000020
>    26 # define CMSPAR   010000000000          /* mark or space (stick) parity */
>    27 # define CRTSCTS  020000000000          /* flow control */
>    28 #endif
> sysdeps/unix/sysv/linux/sparc/bits/termios-baud.h
>    23 #ifdef __USE_MISC
>    24 # define CBAUD   0x0000100f
>    25 # define CBAUDEX 0x00001000
>    26 # define CIBAUD  0x100f0000     /* input baud rate (not used) */
>    27 # define CMSPAR  0x40000000     /* mark or space (stick) parity */
>    28 # define CRTSCTS 0x80000000     /* flow control */
>    29 #endif
> 
>> Also, on second thought I think CMSPAR should follow CRTSCTS, a few
>> lines below, because of its numerical value higher than CRTSCTS.
> 
> GLIBC/Linux normally has it lower:
> $ grep -C2 'define\s\+CMSPAR' /usr/include/**/*.h
> /usr/include/asm-generic/termbits.h:
> #define  B4000000 0010017
> #define CIBAUD      002003600000    /* input baud rate */
> #define CMSPAR      010000000000    /* mark or space (stick) parity */
> #define CRTSCTS      020000000000    /* flow control */
> 
> -- 
> /usr/include/i386-linux-gnu/bits/termios.h:
> #ifdef __USE_MISC
> # define CIBAUD      002003600000        /* input baud rate (not used) */
> # define CMSPAR   010000000000        /* mark or space (stick) parity */
> # define CRTSCTS  020000000000        /* flow control */
> #endif

Linux may have inherited this from early Unix, UART, or serial I/O mux docs as 
they use octal, support split input/output speeds, are 16 bit aligned, and some 
of the fields are direct from UART register docs:

  3:0  output baud rate enum
  6:4  output data and stop bit lengths in UART line control register low bits
11:7  output parity and other control and/or status
12    output baud rate high speed extension
15:13 unused
19:16 input baud rate enum
22:20 input data and stop bit lengths in UART line control register low bits
27:23 input parity and other control and/or status
28    input baud rate high speed extension
29    unused
30    mark or space (stick) parity
31    flow control

Cygwin currently supports only up to B3000000 0x100f and cannot easily support:

#define  B3500000 0010016
#define  B4000000 0010017

as bit 3 is already and differently used.

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in binary units and prefixes, physical quantities in SI.]

fhandler_serial.cc: MARK and SPACE parity for serial port

[Marek Smetana]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

Hi,

I have modified the patch as recommended and am sending it as an attachment.

The CMSPAR value is the same as in "asm / termbits.h" on Linux.

Patches  sent by me are licensed under the 2-clause BSD license.

Best regards

Marek



[-- Attachment #2: serial_parity.patch --]
[-- Type: text/plain, Size: 1613 bytes --]

 winsup/cygwin/fhandler_serial.cc    | 11 ++++++++++-
 winsup/cygwin/include/sys/termios.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git c/winsup/cygwin/fhandler_serial.cc w/winsup/cygwin/fhandler_serial.cc
index fd5b45899..e0257302c 100644
--- c/winsup/cygwin/fhandler_serial.cc
+++ w/winsup/cygwin/fhandler_serial.cc
@@ -727,7 +727,12 @@ fhandler_serial::tcsetattr (int action, const struct termios *t)
   /* -------------- 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;
+    }
   else
     state.Parity = NOPARITY;
 
@@ -1068,6 +1073,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 c/winsup/cygwin/include/sys/termios.h w/winsup/cygwin/include/sys/termios.h
index 17e8d83a3..e4465fca3 100644
--- c/winsup/cygwin/include/sys/termios.h
+++ w/winsup/cygwin/include/sys/termios.h
@@ -206,6 +206,7 @@ POSIX commands */
 
 #define CRTSXOFF 0x04000
 #define CRTSCTS	 0x08000
+#define CMSPAR	 0x40000000 /* Mark or space (stick) parity.  */
 
 /* lflag bits */
 #define ISIG	0x0001

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

[Corinna Vinschen]

On Jan 29 23:06, Marek Smetana via Cygwin-patches wrote:
> Hi,
> 
> I have modified the patch as recommended and am sending it as an attachment.

Great, but can you please attach the entire output of
`git format-patch -1' including the commit message?


Thanks,
Corinna

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

[Marek Smetana]

[-- Attachment #1: Type: text/plain, Size: 80 bytes --]

I'm Sorry, this is my first patch using the mailing list.

Best regards

Marek


[-- Attachment #2: 0001-fhandler_serial.cc-MARK-and-SPACE-parity-for-serial-.patch --]
[-- Type: text/plain, Size: 1858 bytes --]

From be77a27bd40da56064f12a72c18d434ddb6403dc Mon Sep 17 00:00:00 2001
From: Marek Smetana <mara.smetana@gmail.com>
Date: Mon, 1 Feb 2021 22:02:14 +0100
Subject: [PATCH] fhandler_serial.cc: MARK and SPACE parity for serial port

---
 winsup/cygwin/fhandler_serial.cc    | 11 ++++++++++-
 winsup/cygwin/include/sys/termios.h |  1 +
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/winsup/cygwin/fhandler_serial.cc b/winsup/cygwin/fhandler_serial.cc
index fd5b45899..e0257302c 100644
--- a/winsup/cygwin/fhandler_serial.cc
+++ b/winsup/cygwin/fhandler_serial.cc
@@ -727,7 +727,12 @@ fhandler_serial::tcsetattr (int action, const struct termios *t)
   /* -------------- 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;
+    }
   else
     state.Parity = NOPARITY;
 
@@ -1068,6 +1073,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
index 17e8d83a3..e4465fca3 100644
--- a/winsup/cygwin/include/sys/termios.h
+++ b/winsup/cygwin/include/sys/termios.h
@@ -206,6 +206,7 @@ POSIX commands */
 
 #define CRTSXOFF 0x04000
 #define CRTSCTS	 0x08000
+#define CMSPAR	 0x40000000 /* Mark or space (stick) parity.  */
 
 /* lflag bits */
 #define ISIG	0x0001
-- 
2.30.0

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

[Brian Inglis]

On 2021-02-01 14:26, Marek Smetana via Cygwin-patches wrote:
> I'm Sorry, this is my first patch using the mailing list.

I too struggled for ages getting these right;
first set up your ~/.gitconfig something like:

$ grep -v '^#' ~/.gitconfig
[user]
         name = ...
         email = ...

[sendemail]
         bcc = ...
         smtpServer = ...
         confirm = always

[core]
         editor = /usr/bin/gvim -f

When you commit your changes, the first line of the commit message becomes your 
patch file name and email subject, so keep it < 63, leave a blank line, then 
expand on the details in subsequent lines < 80, so I find it easier to run:

$ git diff @^ > COMMIT_MSG

hack away at that then:

$ git commit -F COMMIT_MSG ...

$ git format-patch --to=cygwin-patches@cygwin.com -1

which will write a file like:

0001-fhandler_serial.cc-MARK-and-SPACE-parity-for-serial-port.patch

hack away at that to improve it then:

$ git send-email --to=cygwin-patches@cygwin.com \
	0001-fhandler_serial.cc-MARK-and-SPACE-parity-for-serial-port.patch

and answer the confirmation message if you enabled that above.

Does anyone know of a way to set different default patch email addresses for 
newlib/ and winsup/ trees?

-- 
Take care. Thanks, Brian Inglis, Calgary, Alberta, Canada

This email may be disturbing to some readers as it contains
too much technical detail. Reader discretion is advised.
[Data in binary units and prefixes, physical quantities in SI.]

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

[Corinna Vinschen]

On Feb  1 22:26, Marek Smetana via Cygwin-patches wrote:
> I'm Sorry, this is my first patch using the mailing list.

No worries.  Patch pushed.


Thanks,
Corinna