On 17.08.2017 14:09, Stefan Tauner wrote:
<missed the ML in the reply, sorry!>
On Thu, 17 Aug 2017 11:53:58 +0200 Nico Huber nico.huber@secunet.com wrote:
B := we need to give the user a really good reason
Having a RESET enum value allows us to fulfill B, thus:
RESET -> B
OK, that's where I don't agree(d) then. I was expecting a "real good reason" originating in libflashrom and not being some pre-defined obvious string like "things are complicated". :)
There is only one reason in flashrom internally: an erase failed. I suppose that can be covered by a pre-defined string.
On Thu, 17 Aug 2017 10:42:14 +0100 Richard Hughes hughsient@gmail.com wrote:
On 17 August 2017 at 10:30, Stefan Tauner stefan.tauner@gmx.at wrote:
Am I the only one seeing a contradiction to what you wrote half an hour ago (below)? Did I miss something?
Well, what I was trying to say (perhaps badly) that if we must "go backwards" for a very good technical reason then we need to communicate this to the user. In an ideal world we'd just be able to write 0..100 then verify 0..100, but in the real worlds we have blocks, erases, retries and all that jazz. So if we do need to handle a "retry" we need a way to map the event to something we can show in the UI to explain what we're doing.
But to do that no additional enum is necessary? The event of a reset can easily be determined if the last state is known. If we want to make the life of the caller easier and notify it of these events we should not use the "enum flashrom_progress_stage" parameter for it or rename it. I'd rather separate these two things completely like this:
enum flashrom_progress_stage { FLASHROM_PROGRESS_READ, FLASHROM_PROGRESS_WRITE, FLASHROM_PROGRESS_VERIFY, };
enum flashrom_progress_event { FLASHROM_PROGEV_RESET_STEP, /* If and when we might back off a little without resetting the whole stage. */ I think this is not implemented yet but we might want to retry writing single blocks if they fail for example. FLASHROM_PROGEV_RESET_STAGE, /* If and when we need to restart the current stage */ < Not implemented yet either but I forsee this being implemented in case of timeouts or connection problems. FLASHROM_PROGEV_RESET_PROCESS, /* If and when we need to restart the whole process */ < The typical write/erase fail -> re-read of whole chip case FLASHROM_PROGEV_SKIP_{STEP, STAGE}, /* If and when we skip a step, or possibly even stage */ < Is this useful at all? Probably not for ordinary progress bars but if you think about something looking like filesystem defragmentation visualization this could be worth it and ignoring is easy :) };
Not exactly what I had in mind when I said the interface shouldn't be "driven by internals" but, IMHO, this clearly goes in the wrong direc- tion. Not only the interface would be harder to understand and use, I also don't want to picture how the code would look like internally when we have not only progress calls but also progress event calls at every corner.
typedef void(flashrom_progress_cb)(enum flashrom_progress_stage stage, uint64_t completed, unit64_t total, void *user_data); typedef void(flashrom_progress_event_cb)(enum flashrom_progress_event event, char *reason, void *user_data);
`char *reason`? see above.
flashrom_set_progress_callbacks(flashrom_progress_cb *cb_progress, flashrom_progress_event_cb *cb_event, void *user_data); < We might need to add a flashrom context here? No idea how the current staging implementation works but eventually we want to fix the hidden internal state...
What's missing above is an indicator of the total progress. Essentially this scheme breaks down the total execution into stages and gives only feedback on the progress of them individually. To construct a useful overall progress indication the caller is required to have knowledge about the composition and numbers of stages forming the total operation. How could this be solved?
In a simplified version (let's assume resets don't count), we could simply approximate everything with the size of the flash chip (or size of included regions if a layout is used). So total amount of progress for a read/write/verify would be three times the size. Read and verify would be linear, write rather bumpy, but does it matter?
Any way to smoothen the write progress comes at the cost of an unpredic- table total. We just can't decide the amount to be written before the initial read.
Which reminds me: we haven't discussed alternatives to the `enum flashrom_progress_stage` yet. I said I'm not convinced of it and I'm even less tonight. The alternative would be to make the whole process more controllable by the caller.
Richard, would it be feasible for your GUI visualization if you'd do the steps separately? e.g. you could make a backup, then you could pass the backup back to libflashrom together with the new data to be written (which would allow us to skip the usual read before write) and then finally call a function to verify everything. So basically you'd have three calls into libflashrom which take some time and would show three times a progress bar. If you wanted to show a total progress you'd have to do that manually then.
Nico