Re: [flashrom] [PATCH 0/7] Port serprog to Windows 2.0

[Stefan Tauner]

On Thu,  3 Jan 2013 21:37:18 +0100
Stefan Tauner  wrote:
> Developing cross-platform is great!
> If you look at 1/7 and have to puke, please don't blame me.
> I got the win32 calls right eventually and tested this with wine
> and my serprog flasher (which uses serial over USB).
> I repeat: I used serial over USB inside wine to talk serprog. haha.
> Tests with programmers using serial.c (e.g. buspirate) on all platforms
> warmly welcomed.
>
> Stefan Tauner (7):
>   serial.c: abstract system error printing.
>   serial.c: round baudrates to valid ones.
>   Replace sp_sync_read_timeout() with serialport_read_nonblock().
>   Introduce serialport_write_nonblock().
>   Replace native calls in serprog with wrapper calls.
>   serial.c: be more pedantic.
>   Enable serprog on Windows.

Self-acked and committed in r1659 to r1665 after testing it on real
windows too.

[PATCH] Combine serialport_write and serialport_read into serialport_rw.

[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.

Signed-off-by: Stefan Tauner 
---
 serial.c |   72 ++++++++++++++++++++++++++++----------------------------------
 1 file changed, 33 insertions(+), 39 deletions(-)

diff --git a/serial.c b/serial.c
index 49484f5..6679b7c 100644
--- a/serial.c
+++ b/serial.c
@@ -28,6 +28,7 @@
 #include <sys/stat.h>
 #include <errno.h>
 #include <inttypes.h>
+#include <stdbool.h>
 #ifdef _WIN32
 #include <conio.h>
 #else
@@ -328,68 +329,61 @@ int serialport_shutdown(void *data)
    return 0;
 }

-int serialport_write(unsigned char *buf, unsigned int writecnt)
+/* Writes (if \c outdir is true) or reads \c todo bytes from/into \c buf.
+ * Tries up to 10 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)
 {
+	const char * const op = outdir ? "write" : "read";
 #ifdef _WIN32
-	DWORD tmp = 0;
+	DWORD cur = 0;
 #else
-	ssize_t tmp = 0;
+	ssize_t cur = 0;
 #endif
-	unsigned int empty_writes = 250; /* results in a ca. 125ms timeout */
+	unsigned int empty_tries = 10;

-	while (writecnt > 0) {
+	while (todo > 0) {
 #ifdef _WIN32
-		WriteFile(sp_fd, buf, writecnt, &tmp, NULL);
+		if (outdir)
+			WriteFile(sp_fd, buf, todo, &cur, NULL);
+		else
+			ReadFile(sp_fd, buf, todo, &cur, NULL);
 #else
-		tmp = write(sp_fd, buf, writecnt);
+		if (outdir)
+			cur = write(sp_fd, buf, todo);
+		else
+			cur = read(sp_fd, buf, todo);
 #endif
-		if (tmp == -1) {
-			msg_perr("Serial port write error!\n");
+		if (cur == -1) {
+			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) {
+		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!\n");
    			return 1;
    		}
    	}
-		writecnt -= tmp;
-		buf += tmp;
+		todo -= cur;
+		buf += cur;
    }

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
  * 0 on success, positive values on temporary errors (e.g. timeouts) and negative ones on permanent errors.
  * If really_read is not NULL, this function sets its contents to the number of bytes read successfully. */

[PATCH 0/7] Port serprog to Windows 2.0

[Carl-Daniel Hailfinger]

Am 01.04.2013 02:59 schrieb Stefan Tauner:
> On Thu,  3 Jan 2013 21:37:18 +0100
> Stefan Tauner  wrote:
> > Developing cross-platform is great!
> > If you look at 1/7 and have to puke, please don't blame me.
> > I got the win32 calls right eventually and tested this with wine
> > and my serprog flasher (which uses serial over USB).
> > I repeat: I used serial over USB inside wine to talk serprog. haha.
> > Tests with programmers using serial.c (e.g. buspirate) on all platforms
> > warmly welcomed.
> >
> > Stefan Tauner (7):
> >   serial.c: abstract system error printing.
> >   serial.c: round baudrates to valid ones.
> >   Replace sp_sync_read_timeout() with serialport_read_nonblock().
> >   Introduce serialport_write_nonblock().
> >   Replace native calls in serprog with wrapper calls.
> >   serial.c: be more pedantic.
> >   Enable serprog on Windows.
>
> Self-acked and committed in r1659 to r1665 after testing it on real
> windows too.

Thanks a lot for this patch set! I saw the horror of handling the
equivalent of strerror() on Windows, and it made me shudder.

Regards,
Carl-Daniel

[PATCH] Introduce serialport_config().

[Stefan Tauner]

This allows to easily reconfigure a serial port as needed in
the Bus Pirate speedup.

Also, initialize sp_fd and fix baud rate setting on windows.

Signed-off-by: Stefan Tauner 
---
 programmer.h |    1 +
 serial.c     |  109 ++++++++++++++++++++++++++++++++--------------------------
 2 files changed, 62 insertions(+), 48 deletions(-)

diff --git a/programmer.h b/programmer.h
index a22bdd2..bb19b2a 100644
--- a/programmer.h
+++ b/programmer.h
@@ -655,6 +655,7 @@ typedef int fdtype;

void sp_flush_incoming(void);
 fdtype sp_openserport(char *dev, unsigned int baud);
+int serialport_config(fdtype fd, unsigned int baud);
 void __attribute__((noreturn)) sp_die(char *msg);
 extern fdtype sp_fd;
 /* expose serialport_shutdown as it's currently used by buspirate */
diff --git a/serial.c b/serial.c
index 49484f5..562a95a 100644
--- a/serial.c
+++ b/serial.c
@@ -39,7 +39,7 @@
 #include "flash.h"
 #include "programmer.h"

-fdtype sp_fd;
+fdtype sp_fd = SER_INV_FD;

void __attribute__((noreturn)) sp_die(char *msg)
 {
@@ -158,72 +158,45 @@ static void msg_perr_strerror(const char *msg)
 #endif
 }

-fdtype sp_openserport(char *dev, unsigned int baud)
+int serialport_config(fdtype fd, unsigned int baud)
 {
-#ifdef _WIN32
-	HANDLE fd;
-	char *dev2 = dev;
-	if ((strlen(dev) > 3) &&
-	    (tolower((unsigned char)dev[0]) == 'c') &&
-	    (tolower((unsigned char)dev[1]) == 'o') &&
-	    (tolower((unsigned char)dev[2]) == 'm')) {
-		dev2 = malloc(strlen(dev) + 5);
-		if (!dev2) {
-			msg_perr_strerror("Out of memory: ");
-			return SER_INV_FD;
-		}
-		strcpy(dev2, "\\.\");
-		strcpy(dev2 + 4, dev);
-	}
-	fd = CreateFile(dev2, GENERIC_READ | GENERIC_WRITE, 0, NULL,
-			OPEN_EXISTING, 0, NULL);
-	if (dev2 != dev)
-		free(dev2);
-	if (fd == INVALID_HANDLE_VALUE) {
-		msg_perr_strerror("Cannot open serial port: ");
-		return SER_INV_FD;
+	if (fd == SER_INV_FD) {
+		msg_perr("%s: File descriptor is invalid.\n", __func__);
+		return 1;
    }
+
+#ifdef _WIN32
    DCB dcb;
    if (!GetCommState(fd, &dcb)) {
    	msg_perr_strerror("Could not fetch original serial port configuration: ");
-		goto out_close;
+		return 1;
    }
    const struct baudentry *entry = round_baud(baud);
-	dcb.BaudRate = entry->baud;
+	dcb.BaudRate = entry->flag;
    dcb.ByteSize = 8;
    dcb.Parity = NOPARITY;
    dcb.StopBits = ONESTOPBIT;
    if (!SetCommState(fd, &dcb)) {
    	msg_perr_strerror("Could not change serial port configuration: ");
-		goto out_close;
+		return 1;
    }
    if (!GetCommState(fd, &dcb)) {
    	msg_perr_strerror("Could not fetch new serial port configuration: ");
-		goto out_close;
+		return 1;
    }
    msg_pdbg("Baud rate is %ld.\n", dcb.BaudRate);
-	return fd;
-out_close:
-	CloseHandle(sp_fd);
-	return SER_INV_FD;
 #else
    struct termios wanted, observed;
-	int fd;
-	fd = open(dev, O_RDWR | O_NOCTTY | O_NDELAY);
-	if (fd < 0) {
-		msg_perr_strerror("Cannot open serial port: ");
-		return SER_INV_FD;
-	}
    fcntl(fd, F_SETFL, 0);
    if (tcgetattr(fd, &observed) != 0) {
    	msg_perr_strerror("Could not fetch original serial port configuration: ");
-		goto out_close;
+		return 1;
    }
    wanted = observed;
    const struct baudentry *entry = round_baud(baud);
    if (cfsetispeed(&wanted, entry->flag) != 0 || cfsetospeed(&wanted, entry->flag) != 0) {
    	msg_perr_strerror("Could not set serial baud rate: ");
-		goto out_close;
+		return 1;
    }
    wanted.c_cflag &= ~(PARENB | CSTOPB | CSIZE | CRTSCTS);
    wanted.c_cflag |= (CS8 | CLOCAL | CREAD);
@@ -232,11 +205,11 @@ out_close:
    wanted.c_oflag &= ~OPOST;
    if (tcsetattr(fd, TCSANOW, &wanted) != 0) {
    	msg_perr_strerror("Could not change serial port configuration: ");
-		goto out_close;
+		return 1;
    }
    if (tcgetattr(fd, &observed) != 0) {
    	msg_perr_strerror("Could not fetch new serial port configuration: ");
-		goto out_close;
+		return 1;
    }
    if (observed.c_cflag != wanted.c_cflag ||
        observed.c_lflag != wanted.c_lflag ||
@@ -244,14 +217,54 @@ out_close:
        observed.c_oflag != wanted.c_oflag ||
        cfgetispeed(&observed) != cfgetispeed(&wanted)) {
    	msg_perr("%s: Some requested options did not stick.\n", __func__);
-		goto out_close;
+		return 1;
    }
-	msg_pdbg("Baud rate is %d.\n", entry->baud);
-	return fd;
+	msg_pdbg("Baud rate is %d now.\n", entry->baud);
+#endif
+	return 0;
+}

-out_close:
-	close(sp_fd);
-	return SER_INV_FD;
+fdtype sp_openserport(char *dev, unsigned int baud)
+{
+	fdtype fd;
+#ifdef _WIN32
+	char *dev2 = dev;
+	if ((strlen(dev) > 3) &&
+	    (tolower((unsigned char)dev[0]) == 'c') &&
+	    (tolower((unsigned char)dev[1]) == 'o') &&
+	    (tolower((unsigned char)dev[2]) == 'm')) {
+		dev2 = malloc(strlen(dev) + 5);
+		if (!dev2) {
+			msg_perr_strerror("Out of memory: ");
+			return SER_INV_FD;
+		}
+		strcpy(dev2, "\\.\");
+		strcpy(dev2 + 4, dev);
+	}
+	fd = CreateFile(dev2, GENERIC_READ | GENERIC_WRITE, 0, NULL,
+			OPEN_EXISTING, 0, NULL);
+	if (dev2 != dev)
+		free(dev2);
+	if (fd == INVALID_HANDLE_VALUE) {
+		msg_perr_strerror("Cannot open serial port: ");
+		return SER_INV_FD;
+	}
+	if (serialport_config(fd, baud) != 0) {
+		CloseHandle(fd);
+		return SER_INV_FD;
+	}
+	return fd;
+#else
+	fd = open(dev, O_RDWR | O_NOCTTY | O_NDELAY);
+	if (fd < 0) {
+		msg_perr_strerror("Cannot open serial port: ");
+		return SER_INV_FD;
+	}
+	if (serialport_config(fd, baud) != 0) {
+		close(fd);
+		return SER_INV_FD;
+	}
+	return fd;
 #endif
 }