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
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"
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
-
Carl-Daniel Hailfinger -
Michael Karcher