On Thu, 17 Aug 2017 10:23:54 +0100 Richard Hughes hughsient@gmail.com wrote:
On 17 August 2017 at 10:18, Stefan Tauner stefan.tauner@gmx.at wrote:
However, this is inevitable sometimes as explained before... so maybe we need some additional message passing indicating the different reasons for a "change of plans" so that the UI can react? (which is exactly the type of complication I wanted to avoid ;)
I think a status enum of something like RESET would be enough to show something hand-wavey in the UI, e.g. "Sorry for the delay, trying harder..."
Am I the only one seeing a contradiction to what you wrote half an hour ago (below)? Did I miss something?
On Thu, 17 Aug 2017 09:58:25 +0100 Richard Hughes hughsient@gmail.com wrote:
If the percentage does go backwards then we need to give the user a really good reason of why, otherwise users *will* file issues and bugs about it.
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.
Richard.
On 17.08.2017 11:30, Stefan Tauner wrote:
On Thu, 17 Aug 2017 10:23:54 +0100 Richard Hughes hughsient@gmail.com wrote:
On 17 August 2017 at 10:18, Stefan Tauner stefan.tauner@gmx.at wrote:
However, this is inevitable sometimes as explained before... so maybe we need some additional message passing indicating the different reasons for a "change of plans" so that the UI can react? (which is exactly the type of complication I wanted to avoid ;)
I think a status enum of something like RESET would be enough to show something hand-wavey in the UI, e.g. "Sorry for the delay, trying harder..."
Let's call this statement RESET.
Am I the only one seeing a contradiction to what you wrote half an hour ago (below)? Did I miss something?
You might be, see below.
On Thu, 17 Aug 2017 09:58:25 +0100 Richard Hughes hughsient@gmail.com wrote:
If the percentage does go backwards then we need to give the user a really good reason of why, otherwise users *will* file issues and bugs about it.
Let me translate that for you:
A := the percentage does go backwards B := we need to give the user a really good reason
He said A -> B.
Having a RESET enum value allows us to fulfill B, thus:
RESET -> B
which suffices to say:
RESET -> (A -> B)
I don't see a contradiction ;)
Nico
<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.
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
On 17 August 2017 at 23:19, Nico Huber nico.h@gmx.de wrote:
typedef void(flashrom_progress_event_cb)(enum flashrom_progress_event event, char *reason, void *user_data);
`char *reason`? see above.
I use libraries with const char* enums; and although it makes it easy to add states the calling program never knows what to expect -- "add" or "ADD", "add foo" or "add: foo" -- I'd really much prefer this to be tightly controlled with an enum.
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?
I think it's fine.
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, that's ok, on the assumption we know what to do -- perhaps not all chips need erasing...
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.
libflashrom would be called by fwupd, which would then proxy on a percentage to the GUI, so I think it's fine that fwupd can control what's happening. That's pretty much what it does for the other userspace flash plugins.
Richard
-
Nico Huber -
Nico Huber -
Richard Hughes -
Stefan Tauner