This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: Setting parity for remote serial
- From: Joel Brobecker <brobecker at adacore dot com>
- To: Yurij Grechishhev <yurij dot grechishhev at gmail dot com>
- Cc: gdb-patches at sourceware dot org
- Date: Mon, 23 Mar 2015 06:11:47 -0700
- Subject: Re: Setting parity for remote serial
- Authentication-results: sourceware.org; auth=none
- References: <CAAyhtXSnc_frS-2PKQDt_wDL+=FN9GPkJeWRo_Yn=ez1CYL4uw at mail dot gmail dot com> <20130926165939 dot GA5440 at adacore dot com> <CAAyhtXTmrJ04BVhziaFnogGyWLz7+G+Qwbc9UnHJkrEbDgTjFw at mail dot gmail dot com> <20131004073447 dot GB3326 at adacore dot com> <20150225151646 dot GA30181 at adacore dot com> <CAAyhtXRC8DFk0SdfLoQvhWk_+h5AFZW2QmXQ6RcdCVC9Asx9Vw at mail dot gmail dot com> <20150227081642 dot GE31815 at adacore dot com> <CAAyhtXSsfZss0UqLTjG0sWN-T0k-XLkKLtvz-ea-QaAbrZFCEw at mail dot gmail dot com> <20150317145620 dot GB7494 at adacore dot com> <CAAyhtXQUqokKS8CvqwK063DmzG1=OY2KJxg25fZWUDH4_3tK3Q at mail dot gmail dot com>
> 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