| Submitter | Stefan Tauner |
|---|---|
| Date | 2013-07-10 19:21:09 |
| Message ID | <1373484071-549-6-git-send-email-stefan.tauner@student.tuwien.ac.at> |
| Download | mbox | patch |
| Permalink | /patch/3982/ |
| State | New |
| Headers | show |
Comments
Am 10.07.2013 21:21 schrieb Stefan Tauner: > Because they share almost all code, combine them into serialport_rw > and export the old functions as wrappers. This makes reads "empty reads"- > aware and programmers able to detect such problems even on "blocking" > reads. Also, this fixes error detection on Windows. > > Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> > --- > serial.c | 83 ++++++++++++++++++++++++++++++---------------------------------- > 1 file changed, 39 insertions(+), 44 deletions(-) > > diff --git a/serial.c b/serial.c > index 1b394cd..5f74fd1 100644 > --- a/serial.c > +++ b/serial.c > @@ -328,66 +330,59 @@ int serialport_shutdown(void *data) > return 0; > } > > -int serialport_write(unsigned char *buf, unsigned int writecnt) > +#define EMPTY_TRIES 10 A total of 10 ms timeout is too fast for the Bus Pirate and some slower serprog implementations AFAICS. I'd prefer a 1 s timeout before signaling an error. That should be sufficient, and callers are supposed to check for errors, so the 1 s timeout won't happen multiple times. > +/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf. > + * Tries up to \c EMPTY_TRIES times with a timeout of \c ms ms between each try. */ > +static int serialport_rw(unsigned char *buf, unsigned int todo, bool outdir, unsigned int ms) > { > -#ifdef _WIN32 > - DWORD tmp = 0; > -#else > - ssize_t tmp = 0; > -#endif > - unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */ > + const char * const op = outdir ? "write" : "read"; > + bool err = false; > + unsigned int empty_tries = EMPTY_TRIES; > > - while (writecnt > 0) { > + while (todo > 0) { > #ifdef _WIN32 > - WriteFile(sp_fd, buf, writecnt, &tmp, NULL); > + DWORD cur = 0; > + if (outdir) > + err = !WriteFile(sp_fd, buf, todo, &cur, NULL); > + else > + err = !ReadFile(sp_fd, buf, todo, &cur, NULL); Should we check for cur == 0 here? Can that happen? > #else > - tmp = write(sp_fd, buf, writecnt); > -#endif > - if (tmp == -1) { > - msg_perr("Serial port write error!\n"); > + ssize_t cur = 0; > + if (outdir) > + err = (cur = write(sp_fd, buf, todo)) < 0; OK. > + else > + err = (cur = read(sp_fd, buf, todo)) < 0; Should be <= 0 instead of < 0 AFAICS. See the POSIX man page of read(): "When attempting to read from an empty pipe or FIFO: If no process has the pipe open for writing, read() shall return 0 to indicate end-of-file." > +#endif > + if (err) { > + msg_perr("Serial port %s error!\n", op); > return 1; > } > - if (!tmp) { > - msg_pdbg2("Empty write\n"); > - empty_writes--; > - programmer_delay(500); > - if (empty_writes == 0) { > - msg_perr("Serial port is unresponsive!\n"); > + if (cur == 0) { > + msg_pdbg2("Empty %s.\n", op); > + empty_tries--; > + programmer_delay(ms * 1000); > + if (empty_tries == 0) { > + msg_perr("Serial port is unresponsive or disappeared!\n"); > return 1; > } > + continue; > } > - writecnt -= tmp; > - buf += tmp; > + todo -= cur; > + buf += cur; > + empty_tries = EMPTY_TRIES; > } > > return 0; > } > > -int serialport_read(unsigned char *buf, unsigned int readcnt) > +int serialport_write(unsigned char *buf, unsigned int writecnt) > { > -#ifdef _WIN32 > - DWORD tmp = 0; > -#else > - ssize_t tmp = 0; > -#endif > - > - while (readcnt > 0) { > -#ifdef _WIN32 > - ReadFile(sp_fd, buf, readcnt, &tmp, NULL); > -#else > - tmp = read(sp_fd, buf, readcnt); > -#endif > - if (tmp == -1) { > - msg_perr("Serial port read error!\n"); > - return 1; > - } > - if (!tmp) > - msg_pdbg2("Empty read\n"); > - readcnt -= tmp; > - buf += tmp; > - } > + return serialport_rw(buf, writecnt, true, 1); > +} > > - return 0; > +int serialport_read(unsigned char *buf, unsigned int readcnt) > +{ > + return serialport_rw(buf, readcnt, false, 10); > } > > /* Tries up to timeout ms to read readcnt characters and places them into the array starting at c. Returns Regards, Carl-Daniel
On Fri, 19 Jul 2013 01:57:30 +0200 Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006@gmx.net> wrote: > Am 10.07.2013 21:21 schrieb Stefan Tauner: > > Because they share almost all code, combine them into serialport_rw > > and export the old functions as wrappers. This makes reads "empty reads"- > > aware and programmers able to detect such problems even on "blocking" > > reads. Also, this fixes error detection on Windows. > > > > Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> > > --- > > serial.c | 83 ++++++++++++++++++++++++++++++---------------------------------- > > 1 file changed, 39 insertions(+), 44 deletions(-) > > > > diff --git a/serial.c b/serial.c > > index 1b394cd..5f74fd1 100644 > > --- a/serial.c > > +++ b/serial.c > > @@ -328,66 +330,59 @@ int serialport_shutdown(void *data) > > return 0; > > } > > > > -int serialport_write(unsigned char *buf, unsigned int writecnt) > > +#define EMPTY_TRIES 10 > > A total of 10 ms timeout is too fast for the Bus Pirate and some slower > serprog implementations AFAICS. I'd prefer a 1 s timeout before > signaling an error. That should be sufficient, and callers are supposed > to check for errors, so the 1 s timeout won't happen multiple times. I was under the impression that reads and writes are blocking, hence that these timeout values do not determine the upper bound of the execution time of one operation. I came to that conclusion because it is the behavior of my hardware (serprog via cdc-acm) and software (native linux and wine). I also tested it on native windows. On non-windows we open the device with O_NDELAY which seems to have been different to O_NONBLOCK in the distant past but is now semantically equivalent on most platforms. However, only O_NONBLOCK seems to be in POSIX and I would like to switch to that. That being said, apparently we are relying on not too well specified behavior. AFAIK the tty devices are "character special" files/devices. The standard specifies the following: "When opening a block special or character special file that supports non-blocking opens: - If O_NONBLOCK is set, the open() function shall return without blocking for the device to be ready or available. *Subsequent behavior of the device is device-specific*." (emphasis mine, and I am very certain from the other context that this means if reads and writes are blocking or not) So... before I think and write about what we could do, I would like to hear what you have to say about it. > > +/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf. > > + * Tries up to \c EMPTY_TRIES times with a timeout of \c ms ms between each try. */ > > +static int serialport_rw(unsigned char *buf, unsigned int todo, bool outdir, unsigned int ms) > > { > > -#ifdef _WIN32 > > - DWORD tmp = 0; > > -#else > > - ssize_t tmp = 0; > > -#endif > > - unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */ > > + const char * const op = outdir ? "write" : "read"; > > + bool err = false; > > + unsigned int empty_tries = EMPTY_TRIES; > > > > - while (writecnt > 0) { > > + while (todo > 0) { > > #ifdef _WIN32 > > - WriteFile(sp_fd, buf, writecnt, &tmp, NULL); > > + DWORD cur = 0; > > + if (outdir) > > + err = !WriteFile(sp_fd, buf, todo, &cur, NULL); > > + else > > + err = !ReadFile(sp_fd, buf, todo, &cur, NULL); > > Should we check for cur == 0 here? Can that happen? we do after the ifdefs (marked below) for all platforms. > > #else > > - tmp = write(sp_fd, buf, writecnt); > > -#endif > > - if (tmp == -1) { > > - msg_perr("Serial port write error!\n"); > > + ssize_t cur = 0; > > + if (outdir) > > + err = (cur = write(sp_fd, buf, todo)) < 0; > > OK. > > > > + else > > + err = (cur = read(sp_fd, buf, todo)) < 0; > > Should be <= 0 instead of < 0 AFAICS. See the POSIX man page of read(): > "When attempting to read from an empty pipe or FIFO: If no process has > the pipe open for writing, read() shall return 0 to indicate end-of-file." A character special file is not a pipe. But even then... we would receive empty reads and timeout quickly. No harm done AFAICS. > > +#endif > > + if (err) { > > + msg_perr("Serial port %s error!\n", op); > > return 1; > > } > > - if (!tmp) { > > - msg_pdbg2("Empty write\n"); > > - empty_writes--; > > - programmer_delay(500); > > - if (empty_writes == 0) { > > - msg_perr("Serial port is unresponsive!\n"); > > + if (cur == 0) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + msg_pdbg2("Empty %s.\n", op); > > + empty_tries--; > > + programmer_delay(ms * 1000); > > + if (empty_tries == 0) { > > + msg_perr("Serial port is unresponsive or disappeared!\n"); > > return 1; > > } > > + continue; > > }
Patch
diff --git a/serial.c b/serial.c index 1b394cd..5f74fd1 100644 --- a/serial.c +++ b/serial.c @@ -3,6 +3,7 @@ * * Copyright (C) 2009 Urja Rannikko <urjaman@gmail.com> * Copyright (C) 2009,2010 Carl-Daniel Hailfinger + * Copyright (C) 2013 Stefan Tauner * * This program is free software; you can redistribute it and/or modify * it under the terms of the GNU General Public License as published by @@ -28,6 +29,7 @@ #include <sys/stat.h> #include <errno.h> #include <inttypes.h> +#include <stdbool.h> #ifdef _WIN32 #include <conio.h> #else @@ -328,66 +330,59 @@ int serialport_shutdown(void *data) return 0; } -int serialport_write(unsigned char *buf, unsigned int writecnt) +#define EMPTY_TRIES 10 +/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf. + * Tries up to \c EMPTY_TRIES times with a timeout of \c ms ms between each try. */ +static int serialport_rw(unsigned char *buf, unsigned int todo, bool outdir, unsigned int ms) { -#ifdef _WIN32 - DWORD tmp = 0; -#else - ssize_t tmp = 0; -#endif - unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */ + const char * const op = outdir ? "write" : "read"; + bool err = false; + unsigned int empty_tries = EMPTY_TRIES; - while (writecnt > 0) { + while (todo > 0) { #ifdef _WIN32 - WriteFile(sp_fd, buf, writecnt, &tmp, NULL); + DWORD cur = 0; + if (outdir) + err = !WriteFile(sp_fd, buf, todo, &cur, NULL); + else + err = !ReadFile(sp_fd, buf, todo, &cur, NULL); #else - tmp = write(sp_fd, buf, writecnt); -#endif - if (tmp == -1) { - msg_perr("Serial port write error!\n"); + ssize_t cur = 0; + if (outdir) + err = (cur = write(sp_fd, buf, todo)) < 0; + else + err = (cur = read(sp_fd, buf, todo)) < 0; +#endif + if (err) { + msg_perr("Serial port %s error!\n", op); return 1; } - if (!tmp) { - msg_pdbg2("Empty write\n"); - empty_writes--; - programmer_delay(500); - if (empty_writes == 0) { - msg_perr("Serial port is unresponsive!\n"); + if (cur == 0) { + msg_pdbg2("Empty %s.\n", op); + empty_tries--; + programmer_delay(ms * 1000); + if (empty_tries == 0) { + msg_perr("Serial port is unresponsive or disappeared!\n"); return 1; } + continue; } - writecnt -= tmp; - buf += tmp; + todo -= cur; + buf += cur; + empty_tries = EMPTY_TRIES; } return 0; } -int serialport_read(unsigned char *buf, unsigned int readcnt) +int serialport_write(unsigned char *buf, unsigned int writecnt) { -#ifdef _WIN32 - DWORD tmp = 0; -#else - ssize_t tmp = 0; -#endif - - while (readcnt > 0) { -#ifdef _WIN32 - ReadFile(sp_fd, buf, readcnt, &tmp, NULL); -#else - tmp = read(sp_fd, buf, readcnt); -#endif - if (tmp == -1) { - msg_perr("Serial port read error!\n"); - return 1; - } - if (!tmp) - msg_pdbg2("Empty read\n"); - readcnt -= tmp; - buf += tmp; - } + return serialport_rw(buf, writecnt, true, 1); +} - return 0; +int serialport_read(unsigned char *buf, unsigned int readcnt) +{ + return serialport_rw(buf, readcnt, false, 10); } /* Tries up to timeout ms to read readcnt characters and places them into the array starting at c. Returns
Because they share almost all code, combine them into serialport_rw and export the old functions as wrappers. This makes reads "empty reads"- aware and programmers able to detect such problems even on "blocking" reads. Also, this fixes error detection on Windows. Signed-off-by: Stefan Tauner <stefan.tauner@student.tuwien.ac.at> --- serial.c | 83 ++++++++++++++++++++++++++++++---------------------------------- 1 file changed, 39 insertions(+), 44 deletions(-)