<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". :)
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 :) };
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);
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?
- The callers have a predefined knowledge about what flashrom does and approximate from that (e.g., a "parent" progress bar that shows the overall progress in the case of a flashrom -w in 3 giant steps similar to what many other programs do (e.g., gparted). The disadvantage is that this is completely non-linear speed and there need to exist prior knowledge in the caller.
- Additionally to the "total" values of the current stage we could also give a total total. This would solve the problem with the preknowledge (unless for some reasons the total number of stages are needed). The problem is that I think we won't have this information at all times where we need to make these calls. We could update this information only sometimes (and use -1 or 0 to indicate no change), or alternatively, we could have another dedicated callback to describe the overall process. NB: the total amount of net bytes transferred might not always equal <number of stages>*<bytes of any single stage> (e.g., write-only locks -> initial read and verification are impossible thus skipped but we still might need to do a blind write).
- Go fully berserk and add another callback that informs the application of the overall plan/layout of stages flashrom will do.