Patchwork [RFC] Progress bar

login
register
about
Submitter Carl-Daniel Hailfinger
Date 2010-07-09 19:55:40
Message ID <4C377EBC.1040005@gmx.net>
Download mbox | patch
Permalink /patch/1601/
State RFC
Headers show

Comments

Carl-Daniel Hailfinger - 2010-07-09 19:55:40
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@gmx.net>
Michael Karcher - 2010-07-11 17:37:08
Some random thoughts about progress printing in flashrom.

Am Freitag, den 09.07.2010, 21:55 +0200 schrieb Carl-Daniel Hailfinger:
> The biggest problem is deciding which operations should print a progress
> bar.
The first thing we have to be aware of is that there are two kinds of
progress printing that seems very similar, but are in fact completely
different when you get at implementing them. Being aware of this
distinction should help answering some of the questions.

A) The first type of progress bar ist just what one naively thinks of
when one heres "progress bar", i.e. 0%=start of "operation", 100%=end of
"operation". You "just" have to define what you mean by "operation", and
how to map the steps of the operation to percentage.

B) The second type of progress bar is a "flash status map". You probably
know the kind of disk map displayed by the defragmentation tools, I mean
something like that. So we can display flash areas as "no rewrite
needed", "sucessfully flashed", "erased sucessfully", "currently
modified block" and so on, for example by different letters or colors
representing the different states.

It might turn out that in fact we want both type-A (simple UI) and
type-B (sophisticated GUI, having the status map and a type-A progress
bar) progress indication, but as far as I can see this means we have to
implement both seperately, and not have just one type of progress
indication and try to fudge the one type of output from code that calls
status updates for the other type.

> Write? Sure.
> Erase? Sure. But how do you handle a one-sector erase in the middle of
> some write operation?
One-sector-erases don't just happen in the middle of write, but they are
planned from the beginning. If you want to go to mere progress printing,
the "operation" defined above must include both erasing and writing, so
that erasing is a part of the "operation". This has the consequence that
the "current address" is not directly proportional to the progress bar
state, as the "operation" is not just doing one thing for each addresss.

If you are deciding to implement type-B address map printing, this
question of course does not apply at all.

> Read? Makes sense as well. But how do you handle the reads used for
> verification of erase/write? The two-bars problem strikes again.
Same comment as above. A "in-total" progress bar needs to consider for
each "block" (probably erase-block) the steps of "erase", "erase check",
"write", "verify" and scale the progress appropriately. Or one
recolors/overwrites "erasing" marks to "erased OK" marks.

> 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?
Of course calling progress update per byte is nonsense. We need a bigger
unit. For some operations like mmapped reads (block copies) that are
currently implemented atomic, progress updating seems to make no sense.
Most flash operations are considerably slower than progress bar updates
(think page program, sector erase, chip erase). The only operation that
suffers from progress printing would be byte program/byte read. Maybe
even the 5-byte stuff some SPI masters have at maximum.

One approach to solve the problem of "how do we avoid much too many
multiplication/division operations" is to calculate the position of the
next change too when a change of the progress bar is printed, and ignore
all progess values (maybe addresses, but see above that addresses might
not be what we really want) below the calculated next-change point. This
avoids all calculations until needed.

Regards,
  Michael Karcher

Patch

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"