[flashrom] [PATCH][RFC] Progress bar

[Carl-Daniel Hailfinger]

I wanted to get some discussion about a progress bar going. I'm not
really happy with the current patch, and it will screw up in various
places if its design is not improved, but hey... it works. Sort of.

The biggest problem is deciding which operations should print a progress
bar.
Write? Sure.
Erase? Sure. But how do you handle a one-sector erase in the middle of
some write operation? You can't print two progress bars at the same time.
Read? Makes sense as well. But how do you handle the reads used for
verification of erase/write? The two-bars problem strikes again.

We could add a bar_type parameter to each progressbar call, and make
sure nested calls don't cause confusion. That would solve the issues above.

Another problem: How often do we call the progress bar update? Once per
byte? Per sector? Per arbitrary unit? Will the division and
multiplication performed on every progress bar update affect
performance, especially on architectures where such operations are
sloooooooooow?

This patch also starts progress bars at all the wrong places. Still, it
should give you an impression of what's needed.

Test with
flashrom -p dummy -c MX25L1005 -f -r foo.rom -V
flashrom -p dummy -c Am29LV081B -f -r foo.rom -V
or any read or erase action on real hardware.

Signed-off-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

Index: flashrom-progressbar/flash.h
===================================================================
--- flashrom-progressbar/flash.h	(Revision 1072)
+++ flashrom-progressbar/flash.h	(Arbeitskopie)
@@ -588,6 +588,10 @@
 int check_erased_range(struct flashchip *flash, int start, int len);
 int verify_range(struct flashchip *flash, uint8_t *cmpbuf, int start, int len, char *message);
 int need_erase(uint8_t *have, uint8_t *want, int len, enum write_granularity gran);
+void progressbar_start(struct flashchip *flash);
+void progressbar_update(struct flashchip *flash, int bytepos);
+void progressbar_done(struct flashchip *flash);
+void progressbar_abort(struct flashchip *flash);
 char *strcat_realloc(char *dest, const char *src);
 void print_version(void);
 void print_banner(void);
Index: flashrom-progressbar/spi25.c
===================================================================
--- flashrom-progressbar/spi25.c	(Revision 1072)
+++ flashrom-progressbar/spi25.c	(Arbeitskopie)
@@ -892,6 +892,7 @@
 	 * (start + len - 1) / page_size. Since we want to include that last
 	 * page as well, the loop condition uses <=.
 	 */
+	progressbar_start(flash);
 	for (i = start / page_size; i <= (start + len - 1) / page_size; i++) {
 		/* Byte position of the first byte in the range in this page. */
 		/* starthere is an offset to the base address of the chip. */
@@ -900,6 +901,7 @@
 		lenhere = min(start + len, (i + 1) * page_size) - starthere;
 		for (j = 0; j < lenhere; j += chunksize) {
 			toread = min(chunksize, lenhere - j);
+			progressbar_update(flash, starthere + j);
 			rc = spi_nbyte_read(starthere + j, buf + starthere - start + j, toread);
 			if (rc)
 				break;
@@ -908,6 +910,10 @@
 			break;
 	}
 
+	if (!rc)
+		progressbar_done(flash);
+	else
+		progressbar_abort(flash);
 	return rc;
 }
 
Index: flashrom-progressbar/flashrom.c
===================================================================
--- flashrom-progressbar/flashrom.c	(Revision 1072)
+++ flashrom-progressbar/flashrom.c	(Arbeitskopie)
@@ -544,8 +544,12 @@
 
 int read_memmapped(struct flashchip *flash, uint8_t *buf, int start, int len)
 {
+	progressbar_start(flash);
+	/* chip_readn does not take a struct flashchip argument, and this means
+	 * we can't print progress.
+	 */
 	chip_readn(buf, flash->virtual_memory + start, len);
-		
+	progressbar_done(flash);
 	return 0;
 }
 
@@ -1193,7 +1197,7 @@
 			continue;
 		}
 		found = 1;
-		msg_cdbg("trying... ");
+		progressbar_start(flash);
 		for (i = 0; i < NUM_ERASEREGIONS; i++) {
 			/* count==0 for all automatically initialized array
 			 * members so the loop below won't be executed for them.
@@ -1201,8 +1205,7 @@
 			for (j = 0; j < eraser.eraseblocks[i].count; j++) {
 				start = done + eraser.eraseblocks[i].size * j;
 				len = eraser.eraseblocks[i].size;
-				msg_cdbg("0x%06x-0x%06x, ", start,
-					     start + len - 1);
+				progressbar_update(flash, start);
 				ret = eraser.block_erase(flash, start, len);
 				if (ret)
 					break;
@@ -1212,10 +1215,12 @@
 			done += eraser.eraseblocks[i].count *
 				eraser.eraseblocks[i].size;
 		}
-		msg_cdbg("\n");
 		/* If everything is OK, don't try another erase function. */
-		if (!ret)
+		if (!ret) {
+			progressbar_done(flash);
 			break;
+		} else
+			progressbar_abort(flash);
 	}
 	if (!found) {
 		msg_cerr("ERROR: flashrom has no erase function for this flash chip.\n");
@@ -1230,6 +1235,43 @@
 	return ret;
 }
 
+static int progressbar_pos = 0;
+
+void progressbar_start(struct flashchip *flash)
+{
+	msg_cdbg("|");
+	progressbar_pos = 0;
+}
+
+/**
+ * Print a progress bar with 64 dots. This allows fitting the progress bar in
+ * an 80 column screen next to some other output.
+ */
+void progressbar_update(struct flashchip *flash, int bytepos)
+{
+	int newpos;
+	/* This will be simpler once total_size is in bytes.
+	 * bytepos + 1 to get the full progress bar on the highest address
+	 * which is flash->total_size * 1024 - 1.
+	 */
+	newpos = (bytepos + 1) * 64 / (flash->total_size * 1024);
+	for (; progressbar_pos < newpos; progressbar_pos++)
+		msg_cdbg(".");
+}
+
+void progressbar_done(struct flashchip *flash)
+{
+	progressbar_update(flash, flash->total_size * 1024 - 1);
+	msg_cdbg("|");
+}
+
+void progressbar_abort(struct flashchip *flash)
+{
+	for (; progressbar_pos < 64; progressbar_pos++)
+		msg_cdbg("!");
+	msg_cdbg("|");
+}
+
 void emergency_help_message(void)
 {
 	msg_gerr("Your flash chip is in an unknown state.\n"


-- 
http://www.hailfinger.org/