On Thu, 3 Jan 2013 21:37:18 +0100
Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at> 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.
--
Kind regards/Mit freundlichen Grüßen, Stefan Tauner
This can dramatically reduce the wait time on failures in some cases:
1. issue erase command that never succeeds but times out eventually
2. whole flash gets reread due to the erase not erasing as expected
3. next eraser is issued and flashrom waits for it to finish, although the
transaction is not even considered by the chip because it is still busy
4. whole chip gets reread...
we also can not really be sure which erase opcode succeed (if it does
eventuall) in this situation.
this patch shows said pedantry for the at45db family (which has not
been merged to svn as of this writing). if we want such a feature
(which costs some performance in non-error cases!) i would suggest
creating a is_ready function pointer in the struct flashchip to be
called in the generic code path and note like in this patch. this
might actually be a good idea anyway. i cant tell a useful use case
off the top of my head, do you?
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
---
at45db.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/at45db.c b/at45db.c
index 4ff25be..b41ec73 100644
--- a/at45db.c
+++ b/at45db.c
@@ -324,6 +324,12 @@ static int at45db_wait_ready (struct flashctx *flash, unsigned int us, unsigned
static int at45db_erase(struct flashctx *flash, uint8_t opcode, unsigned int at45db_addr, unsigned int stepsize, unsigned int retries)
{
+ int ret = at45db_wait_ready(flash, 0, 0);
+ if (ret != 0) {
+ msg_cerr("%s: chip not ready before sending erase command!\n", __func__);
+ return ret;
+ }
+
const uint8_t cmd[] = {
opcode,
(at45db_addr >> 16) & 0xff,
@@ -332,7 +338,7 @@ static int at45db_erase(struct flashctx *flash, uint8_t opcode, unsigned int at4
};
/* Send erase command. */
- int ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
+ ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
if (ret != 0) {
msg_cerr("%s: error sending erase command!\n", __func__);
return ret;
@@ -496,6 +502,12 @@ static int at45db_fill_buffer1(struct flashctx *flash, uint8_t *bytes, unsigned
static int at45db_commit_buffer1(struct flashctx *flash, unsigned int at45db_addr)
{
+ int ret = at45db_wait_ready(flash, 0, 0);
+ if (ret != 0) {
+ msg_cerr("%s: chip not ready before sending (write) commit command!\n", __func__);
+ return ret;
+ }
+
const uint8_t cmd[] = {
AT45DB_BUFFER1_PAGE_PROGRAM,
(at45db_addr >> 16) & 0xff,
@@ -504,7 +516,7 @@ static int at45db_commit_buffer1(struct flashctx *flash, unsigned int at45db_add
};
/* Send buffer to device. */
- int ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
+ ret = spi_send_command(flash, sizeof(cmd), 0, cmd, NULL);
if (ret != 0) {
msg_cerr("%s: error sending buffer to main memory command!\n", __func__);
return ret;
--
Kind regards, Stefan Tauner
Author: stefanct
Date: Mon Apr 1 02:45:45 2013
New Revision: 1662
URL: http://flashrom.org/trac/flashrom/changeset/1662
Log:
Introduce serialport_write_nonblock().
It seems useful to have a generic and platform-independent method to
read and write to a serial port without blocking. This is the write part.
This allows to get rid of the explicit temporary disabling of blocking I/O in
serprog's sp_synchronize().
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Acked-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Modified:
trunk/programmer.h
trunk/serial.c
trunk/serprog.c
Modified: trunk/programmer.h
==============================================================================
--- trunk/programmer.h Mon Apr 1 02:45:38 2013 (r1661)
+++ trunk/programmer.h Mon Apr 1 02:45:45 2013 (r1662)
@@ -660,6 +660,7 @@
/* expose serialport_shutdown as it's currently used by buspirate */
int serialport_shutdown(void *data);
int serialport_write(unsigned char *buf, unsigned int writecnt);
+int serialport_write_nonblock(unsigned char *buf, unsigned int writecnt, unsigned int timeout, unsigned int *really_wrote);
int serialport_read(unsigned char *buf, unsigned int readcnt);
int serialport_read_nonblock(unsigned char *c, unsigned int readcnt, unsigned int timeout, unsigned int *really_read);
Modified: trunk/serial.c
==============================================================================
--- trunk/serial.c Mon Apr 1 02:45:38 2013 (r1661)
+++ trunk/serial.c Mon Apr 1 02:45:45 2013 (r1662)
@@ -431,3 +431,75 @@
}
return ret;
}
+
+/* Tries up to timeout ms to write writecnt characters from the array starting at buf. Returns
+ * 0 on success, positive values on temporary errors (e.g. timeouts) and negative ones on permanent errors.
+ * If really_wrote is not NULL, this function sets its contents to the number of bytes written successfully. */
+int serialport_write_nonblock(unsigned char *buf, unsigned int writecnt, unsigned int timeout, unsigned int *really_wrote)
+{
+ int ret = 1;
+ /* disable blocked i/o and declare platform-specific variables */
+#ifdef _WIN32
+ DWORD rv;
+ COMMTIMEOUTS oldTimeout;
+ COMMTIMEOUTS newTimeout = {
+ .ReadIntervalTimeout = MAXDWORD,
+ .ReadTotalTimeoutMultiplier = 0,
+ .ReadTotalTimeoutConstant = 0,
+ .WriteTotalTimeoutMultiplier = 0,
+ .WriteTotalTimeoutConstant = 0
+ };
+ if(!GetCommTimeouts(sp_fd, &oldTimeout)) {
+ msg_perr_strerror("Could not get serial port timeout settings: ");
+ return -1;
+ }
+ if(!SetCommTimeouts(sp_fd, &newTimeout)) {
+ msg_perr_strerror("Could not set serial port timeout settings: ");
+ return -1;
+ }
+#else
+ ssize_t rv;
+ const int flags = fcntl(sp_fd, F_GETFL);
+ fcntl(sp_fd, F_SETFL, flags | O_NONBLOCK);
+#endif
+
+ int i;
+ int wr_bytes = 0;
+ for (i = 0; i < timeout; i++) {
+ msg_pspew("writecnt %d wr_bytes %d\n", writecnt, wr_bytes);
+#ifdef _WIN32
+ WriteFile(sp_fd, buf + wr_bytes, writecnt - wr_bytes, &rv, NULL);
+ msg_pspew("wrote %lu bytes\n", rv);
+#else
+ rv = write(sp_fd, buf + wr_bytes, writecnt - wr_bytes);
+ msg_pspew("wrote %zd bytes\n", rv);
+#endif
+ if ((rv == -1) && (errno != EAGAIN)) {
+ msg_perr_strerror("Serial port write error: ");
+ ret = -1;
+ break;
+ }
+ if (rv > 0) {
+ wr_bytes += rv;
+ if (wr_bytes == writecnt) {
+ msg_pspew("write successful\n");
+ ret = 0;
+ break;
+ }
+ }
+ internal_delay(1000); /* 1ms units */
+ }
+ if (really_wrote != NULL)
+ *really_wrote = wr_bytes;
+
+ /* restore original blocking behavior */
+#ifdef _WIN32
+ if (!SetCommTimeouts(sp_fd, &oldTimeout)) {
+ msg_perr_strerror("Could not restore serial port timeout settings: ");
+#else
+ if (fcntl(sp_fd, F_SETFL, flags) != 0) {
+#endif
+ return -1;
+ }
+ return ret;
+}
Modified: trunk/serprog.c
==============================================================================
--- trunk/serprog.c Mon Apr 1 02:45:38 2013 (r1661)
+++ trunk/serprog.c Mon Apr 1 02:45:45 2013 (r1662)
@@ -117,23 +117,19 @@
/* Synchronize: a bit tricky algorithm that tries to (and in my tests has *
* always succeeded in) bring the serial protocol to known waiting-for- *
- * command state - uses nonblocking read - rest of the driver uses *
+ * command state - uses nonblocking I/O - rest of the driver uses *
* blocking read - TODO: add an alarm() timer for the rest of the app on *
* serial operations, though not such a big issue as the first thing to *
* do is synchronize (eg. check that device is alive). */
static int sp_synchronize(void)
{
int i;
- int flags = fcntl(sp_fd, F_GETFL);
unsigned char buf[8];
- flags |= O_NONBLOCK;
- fcntl(sp_fd, F_SETFL, flags);
/* First sends 8 NOPs, then flushes the return data - should cause *
* the device serial parser to get to a sane state, unless if it *
* is waiting for a real long write-n. */
memset(buf, S_CMD_NOP, 8);
- if (write(sp_fd, buf, 8) != 8) {
- msg_perr("flush write: %s\n", strerror(errno));
+ if (serialport_write_nonblock(buf, 8, 1, NULL) != 0) {
goto err_out;
}
/* A second should be enough to get all the answers to the buffer */
@@ -147,8 +143,7 @@
for (i = 0; i < 8; i++) {
int n;
unsigned char c = S_CMD_SYNCNOP;
- if (write(sp_fd, &c, 1) != 1) {
- msg_perr("sync write: %s\n", strerror(errno));
+ if (serialport_write_nonblock(&c, 1, 1, NULL) != 0) {
goto err_out;
}
msg_pdbg(".");
@@ -165,9 +160,8 @@
if (ret > 0 || c != S_ACK)
continue;
c = S_CMD_SYNCNOP;
- if (write(sp_fd, &c, 1) != 1) {
- msg_perr("sync write: %s\n", strerror(errno));
- return 1;
+ if (serialport_write_nonblock(&c, 1, 1, NULL) != 0) {
+ goto err_out;
}
ret = serialport_read_nonblock(&c, 1, 500, NULL);
if (ret < 0)
@@ -179,9 +173,6 @@
goto err_out;
if (c != S_ACK)
break; /* fail */
- /* Ok, synchronized; back to blocking reads and return. */
- flags &= ~O_NONBLOCK;
- fcntl(sp_fd, F_SETFL, flags);
msg_pdbg("\n");
return 0;
}
Author: stefanct
Date: Mon Apr 1 02:45:38 2013
New Revision: 1661
URL: http://flashrom.org/trac/flashrom/changeset/1661
Log:
Replace sp_sync_read_timeout() with serialport_read_nonblock().
It seems useful to have a generic and platform-independent method to
read and write to a serial port without blocking. This is the read part.
It stores the current blocking properties before disabling blocking and
restores them after reading. The timeout is implemented as previously
by retrying every millisecond until the timeout is reached or enough
characters are available.
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Acked-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Modified:
trunk/programmer.h
trunk/serial.c
trunk/serprog.c
Modified: trunk/programmer.h
==============================================================================
--- trunk/programmer.h Mon Apr 1 02:45:32 2013 (r1660)
+++ trunk/programmer.h Mon Apr 1 02:45:38 2013 (r1661)
@@ -661,6 +661,7 @@
int serialport_shutdown(void *data);
int serialport_write(unsigned char *buf, unsigned int writecnt);
int serialport_read(unsigned char *buf, unsigned int readcnt);
+int serialport_read_nonblock(unsigned char *c, unsigned int readcnt, unsigned int timeout, unsigned int *really_read);
/* Serial port/pin mapping:
Modified: trunk/serial.c
==============================================================================
--- trunk/serial.c Mon Apr 1 02:45:32 2013 (r1660)
+++ trunk/serial.c Mon Apr 1 02:45:38 2013 (r1661)
@@ -361,3 +361,73 @@
return 0;
}
+
+/* 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. */
+int serialport_read_nonblock(unsigned char *c, unsigned int readcnt, unsigned int timeout, unsigned int *really_read)
+{
+ int ret = 1;
+ /* disable blocked i/o and declare platform-specific variables */
+#ifdef _WIN32
+ DWORD rv;
+ COMMTIMEOUTS oldTimeout;
+ COMMTIMEOUTS newTimeout = {
+ .ReadIntervalTimeout = MAXDWORD,
+ .ReadTotalTimeoutMultiplier = 0,
+ .ReadTotalTimeoutConstant = 0,
+ .WriteTotalTimeoutMultiplier = 0,
+ .WriteTotalTimeoutConstant = 0
+ };
+ if(!GetCommTimeouts(sp_fd, &oldTimeout)) {
+ msg_perr_strerror("Could not get serial port timeout settings: ");
+ return -1;
+ }
+ if(!SetCommTimeouts(sp_fd, &newTimeout)) {
+#else
+ ssize_t rv;
+ const int flags = fcntl(sp_fd, F_GETFL);
+ if (fcntl(sp_fd, F_SETFL, flags | O_NONBLOCK) != 0) {
+#endif
+ msg_perr_strerror("Could not set serial port timeout settings %s");
+ return -1;
+ }
+
+ int i;
+ int rd_bytes = 0;
+ for (i = 0; i < timeout; i++) {
+ msg_pspew("readcnt %d rd_bytes %d\n", readcnt, rd_bytes);
+#ifdef _WIN32
+ ReadFile(sp_fd, c + rd_bytes, readcnt - rd_bytes, &rv, NULL);
+ msg_pspew("read %lu bytes\n", rv);
+#else
+ rv = read(sp_fd, c + rd_bytes, readcnt - rd_bytes);
+ msg_pspew("read %zd bytes\n", rv);
+#endif
+ if ((rv == -1) && (errno != EAGAIN)) {
+ msg_perr_strerror("Serial port read error: ");
+ ret = -1;
+ break;
+ }
+ if (rv > 0)
+ rd_bytes += rv;
+ if (rd_bytes == readcnt) {
+ ret = 0;
+ break;
+ }
+ internal_delay(1000); /* 1ms units */
+ }
+ if (really_read != NULL)
+ *really_read = rd_bytes;
+
+ /* restore original blocking behavior */
+#ifdef _WIN32
+ if (!SetCommTimeouts(sp_fd, &oldTimeout)) {
+#else
+ if (fcntl(sp_fd, F_SETFL, flags) != 0) {
+#endif
+ msg_perr_strerror("Could not restore serial port timeout settings: %s\n");
+ ret = -1;
+ }
+ return ret;
+}
Modified: trunk/serprog.c
==============================================================================
--- trunk/serprog.c Mon Apr 1 02:45:32 2013 (r1660)
+++ trunk/serprog.c Mon Apr 1 02:45:38 2013 (r1661)
@@ -115,26 +115,6 @@
return sock;
}
-/* Returns 0 on success and places the character read into the location pointed to by c.
- * Returns positive values on temporary errors and negative ones on permanent errors.
- * An iteration of one loop takes up to 1ms. */
-static int sp_sync_read_timeout(unsigned int loops, unsigned char *c)
-{
- int i;
- for (i = 0; i < loops; i++) {
- ssize_t rv;
- rv = read(sp_fd, c, 1);
- if (rv == 1)
- return 0;
- if ((rv == -1) && (errno != EAGAIN)) {
- msg_perr("read: %s\n", strerror(errno));
- return -1;
- }
- usleep(1000); /* 1ms units */
- }
- return 1;
-}
-
/* Synchronize: a bit tricky algorithm that tries to (and in my tests has *
* always succeeded in) bring the serial protocol to known waiting-for- *
* command state - uses nonblocking read - rest of the driver uses *
@@ -174,12 +154,12 @@
msg_pdbg(".");
fflush(stdout);
for (n = 0; n < 10; n++) {
- int ret = sp_sync_read_timeout(50, &c);
+ int ret = serialport_read_nonblock(&c, 1, 50, NULL);
if (ret < 0)
goto err_out;
if (ret > 0 || c != S_NAK)
continue;
- ret = sp_sync_read_timeout(20, &c);
+ ret = serialport_read_nonblock(&c, 1, 20, NULL);
if (ret < 0)
goto err_out;
if (ret > 0 || c != S_ACK)
@@ -189,12 +169,12 @@
msg_perr("sync write: %s\n", strerror(errno));
return 1;
}
- ret = sp_sync_read_timeout(500, &c);
+ ret = serialport_read_nonblock(&c, 1, 500, NULL);
if (ret < 0)
goto err_out;
if (ret > 0 || c != S_NAK)
break; /* fail */
- ret = sp_sync_read_timeout(100, &c);
+ ret = serialport_read_nonblock(&c, 1, 100, NULL);
if (ret > 0 || ret < 0)
goto err_out;
if (c != S_ACK)
Author: stefanct
Date: Mon Apr 1 02:45:08 2013
New Revision: 1659
URL: http://flashrom.org/trac/flashrom/changeset/1659
Log:
serial.c: abstract system error printing.
Windows is awkward. The win32 API does not support errno/strerror as one
might expect. Introduce a new msg_* function that alleviates the pain a bit
(my head still hurts very badly).
Signed-off-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Acked-by: Stefan Tauner <stefan.tauner(a)student.tuwien.ac.at>
Modified:
trunk/serial.c
Modified: trunk/serial.c
==============================================================================
--- trunk/serial.c Wed Mar 27 14:00:23 2013 (r1658)
+++ trunk/serial.c Mon Apr 1 02:45:08 2013 (r1659)
@@ -104,17 +104,39 @@
};
#endif
+/* Uses msg_perr to print the last system error.
+ * Prints "Error: " followed first by \c msg and then by the description of the last error retrieved via
+ * strerror() or FormatMessage() and ending with a linebreak. */
+static void msg_perr_strerror(const char *msg)
+{
+ msg_perr("Error: %s", msg);
+#ifdef _WIN32
+ char *lpMsgBuf;
+ DWORD nErr = GetLastError();
+ FormatMessage(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, NULL, nErr,
+ MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (LPTSTR) &lpMsgBuf, 0, NULL);
+ msg_perr(lpMsgBuf);
+ /* At least some formatted messages contain a line break at the end. Make sure to always print one */
+ if (lpMsgBuf[strlen(lpMsgBuf)-1] != '\n')
+ msg_perr("\n");
+ LocalFree(lpMsgBuf);
+#else
+ msg_perr("%s\n", strerror(errno));
+#endif
+}
+
fdtype sp_openserport(char *dev, unsigned int baud)
{
#ifdef _WIN32
HANDLE fd;
char *dev2 = dev;
- if ((strlen(dev) > 3) && (tolower((unsigned char)dev[0]) == 'c') &&
+ 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("Error: Out of memory: %s\n", strerror(errno));
+ msg_perr_strerror("Out of memory: ");
return SER_INV_FD;
}
strcpy(dev2, "\\\\.\\");
@@ -125,12 +147,12 @@
if (dev2 != dev)
free(dev2);
if (fd == INVALID_HANDLE_VALUE) {
- msg_perr("Error: cannot open serial port: %s\n", strerror(errno));
+ msg_perr_strerror("Cannot open serial port: ");
return SER_INV_FD;
}
DCB dcb;
if (!GetCommState(fd, &dcb)) {
- msg_perr("Error: Could not fetch serial port configuration: %s\n", strerror(errno));
+ msg_perr_strerror("Could not fetch serial port configuration: ");
return SER_INV_FD;
}
switch (baud) {
@@ -146,7 +168,7 @@
dcb.Parity = NOPARITY;
dcb.StopBits = ONESTOPBIT;
if (!SetCommState(fd, &dcb)) {
- msg_perr("Error: Could not change serial port configuration: %s\n", strerror(errno));
+ msg_perr_strerror("Could not change serial port configuration: ");
return SER_INV_FD;
}
return fd;
@@ -155,7 +177,7 @@
int fd, i;
fd = open(dev, O_RDWR | O_NOCTTY | O_NDELAY);
if (fd < 0) {
- msg_perr("Error: cannot open serial port: %s\n", strerror(errno));
+ msg_perr_strerror("Cannot open serial port: ");
return SER_INV_FD;
}
fcntl(fd, F_SETFL, 0);