Re: Setting parity for remote serial

Joel Brobecker
2015-03-23T13:11:55Z

> Unfortunately, now I don't have the ability to fully-test it with
> windows (but I was checking initial version one year ago, with real
> hardware). Now it has been compiled by using mingw with
> --host=i586-mingw32msvc.

No problem - the code looked about right to me, and we will let those
who do work on Windows take care of fixing any bugs if they use this
feature and encounter a bug.

> diff --git a/gdb/ChangeLog b/gdb/ChangeLog
> index ca8bbaf..02f9ddc 100644
> --- a/gdb/ChangeLog
> +++ b/gdb/ChangeLog
> @@ -1,3 +1,37 @@
> +2015-03-21 Yurij Grechishhev <yurij.grechishhev@gmail.com>
> +
> + * NEWS: Mention set/show serial parity command.
> + * monitor.c (monitor_open): Call serial_setparity.
> + * remote.c (remote_open_1): Likewise.
> + * ser-base.c (ser_base_serparity): New function.
> + * ser-base.h (ser_base_setparity): Add declaration.
> + * ser-go32.c (dos_ops): Add dos_noop field.

The name of the struct field is actually "setparity" (instead of
"dos_noop").

> + * ser-mingw.c (ser_windows_raw): Remove state.fParity and
> + state.Parity definitions.

"Do not set state.fParity and state.Parity". You remmoved assignments,
rather than definitions.

> + (ser_windows_setparity): New function.
> + (hardwire_ops): Add ser_windows_setparity.
> + (tty_ops): Add NULL for setparity field.
> + (pipe_ops): Add ser_base_setparity.
> + (tcp_ops): Likewise.
> + * ser-pipe.c (pipe_ops): Likewise.
> + * ser-tcp.c (tcp_ops): Likewise.
> + * ser-unix.c (hardwire_setparity): Add declaration.
> + (hardwire_raw): Don't reset PARENB flag.
> + (hardwire_setparity): New function.
> + (hardwire_ops): Add hardwire_setparity.
> + * serial.c (serial_setparity): New function.
> + (serial_parity): New global.
> + (parity_none, parity_odd, parity_even, parity_enums, parity):
> + New static globals.

Wrong indentation of "New static globals". I suspect that this is
because you used 8 spaces instead of a tab to indent that line?

> + (set_parity): New function.
> + (_initialize_serial): Add set/show serial parity commands.
> + * serial.h (GDBPARITY_NONE): Define.
> + (GDBPARITY_ODD): Define.
> + (GDBPARITY_EVEN): Define.
> + (serial_setparity) Add declaration.
> + (serial_ops): Add setparity entry.

Let's say...

(struct serial_ops): Add setparity field

... instead.

> + * target.h (serial_parity): Add declaration.

The rest are minor comments:

> + internal_warning (__FILE__, __LINE__, "Incorrect parity value:
> %d", parity);

This line is too long. It shouldn't exceed 79 characters.

internal_warning (__FILE__, __LINE__, "Incorrect parity value: %d", parity);

Use instead:

internal_warning (__FILE__, __LINE__,
"Incorrect parity value: %d", parity);

Also, watch out for the fact that your mailer appears to have wrapped
the line. I suggest you switch to sending the patch either using
"git send-email" (the recommended method), or else sending the patch
as an attachment ("git format-patch" + attach to email).

> @@ -409,7 +410,7 @@ hardwire_raw (struct serial *scb)
> state.termios.c_iflag = 0;
> state.termios.c_oflag = 0;
> state.termios.c_lflag = 0;
> - state.termios.c_cflag &= ~(CSIZE | PARENB);
> + state.termios.c_cflag &= ~(CSIZE);

You don't really need the parentheses anymore -> ~CSIZE

> state.termios.c_cflag |= CLOCAL | CS8;
> #ifdef CRTSCTS
> /* h/w flow control. */
> @@ -432,7 +433,7 @@ hardwire_raw (struct serial *scb)
> state.termio.c_iflag = 0;
> state.termio.c_oflag = 0;
> state.termio.c_lflag = 0;
> - state.termio.c_cflag &= ~(CSIZE | PARENB);
> + state.termio.c_cflag &= ~(CSIZE);

Likewise.

> @@ -893,6 +894,50 @@ hardwire_setstopbits (struct serial *scb, int num)
> return set_tty_state (scb, &state);
> }
>
> +/* Implement the "setparity" serial_ops callback. */
> +
> +static int
> +hardwire_setparity (struct serial *scb, int parity)
> +{
> + struct hardwire_ttystate state;
> + int newparity = 0;
> +
> + if (get_tty_state (scb, &state))
> + return -1;
> +
> + switch (parity)
> + {
> + case GDBPARITY_NONE:
> + newparity = 0;
> + break;
> + case GDBPARITY_ODD:
> + newparity = PARENB | PARODD;
> + break;
> + case GDBPARITY_EVEN:
> + newparity = PARENB;
> + break;
> + default:
> + internal_warning (__FILE__, __LINE__, "Incorrect parity value:
> %d", parity);

Same as above - line is too long.

> +/* See serial.h. */
> +
> +int
> +serial_setparity (struct serial *scb, int parity)
> +{
> + return scb->ops->setparity (scb, parity);

The indentation is wrong (we use a 2-space indentation). Hence:

int
serial_setparity (struct serial *scb, int parity)
{
return scb->ops->setparity (scb, parity);
}

--
Joel