On Fri, 13 Nov 2015 21:58:38 +0100 Bernhard Nortmann <[email protected]> wrote:
> This series of patches picks up on the discussion at > https://groups.google.com/forum/#!topic/linux-sunxi/lz0oQBwjex0 > It's based on first patches submitted there, but has been extensively > reworked to provide a flexible "progress framework" for USB uploads via > the sunxi-fel utility. > > The large number of patches is partly due to the fact that I've tried to > introduce several features in discrete steps, with a logical separation > between them. Patch 4/9 already provides a simple, "proof of concept" > progress bar display based on what was proposed initially. > > Other patches extend on that, while demonstrating that the framework > allows progress routines and sunxi-fel 'core' to be updated mostly > independent of each other. Hi, My opinion (as a user) is that the progress bar with the transfer speed and ETA looks great. Thanks for implementing it. > @Siarhei: > I'd like your comment on 2/9 "refactor aw_fel_write()", if that addresses > the concerns you've expressed regarding that function. Yes, the 2/9 patch is fine. Overall, I like the move of the progressbar handling to a separate source file. It's nice to have this stuff isolated from the rest of the code and used via a clearly documented API. The global state of the progressbar is also probably ok. Because we have only one stdout and we are unlikely to have multiple progressbars simultaneously. My biggest concern is the command line interface. It is basically stateless vs. stateful approach. You are in favour of allowing switches, which modify the state anywhere in the command line and then have regular commands changing behaviour, based on this state. I'm in favour of just adding more commands. It is easier to document these extended commands than all kind of permutations of active switches used together with commands. The current '-v' ('--verbose') switch is somewhat special because it is intended for debugging and does not need to be documented. It allows arbitrary human readable debugging messages coming to stderr, but we don't enforce any particular format of these message. The progressbar related switches are different and we need to document them. The "multiwrite" command does not offer any advantages over the "write" command unless it is used with an active progressbar. This all is somewhat similar to an old discussion about introducing a new "uboot" command versus adding a new switch to use it as a modifier for the existing "spl" command (in the form of "--exec spl"). We don't care about the performance of the "fill" and "clear" commands and don't really need a progressbar for them. The point is that these commands can be implemented as uploading and executing a memset function directly on the device. After doing this, the "fill" and "clear" commands will be faster than 1 GB/s. I can implement a patch for this. Regarding the code itself. Does it really make sense to have all these arguments for the callback function? Only the size of the new data chunk is necessary. And the rest can be deducted from the current progressbar state. For example, the sequence of calls would then look like ('progress_expect' could be renamed to 'progress_start' for better clarity): progress_expect(total_size) progress_update(chunk_size) progress_update(chunk_size) progress_update(chunk_size) progress_update(last_chunk_size) All the other information can be calculated inside of the progressbar code. In the progress_expect() function you can save the timestamp, clear the transferred bytes counter and save the total_size. Then in the progress_update() function you can update the transferred bytes counter and do other calculations. Or am I missing something? The current implementation seems to be overly complicated and even probably a bit buggy. The "multiwrite" command currently inserts line breaks on stdout between different files, which does not look good to me. Do we really want to have the SoC dependent chunk size? Normally we should expect the transfer speed to be somewhere between 500 KB/s and 1000 KB/s (the speeds below 500 KB/s can be improved and we are working on it). But even if we set the chunk size to only 128K, does it really have any negative impact on anything? About the boolean flag. You could probably just use the progress callback function pointer instead of it and lose nothing. And finally, the new progressbar feature fully replaces the old transfer speed calculation/printing that was done under the '-v' switch. So its removal can be probably done as a part of your patch set. I also think that we can just tag the 0.3 release right now. And then in a few days/weeks release the next 0.4 version with the progressbar feature and other improvements. Do you have any objections? -- Best regards, Siarhei Siamashka -- You received this message because you are subscribed to the Google Groups "linux-sunxi" group. To unsubscribe from this group and stop receiving emails from it, send an email to [email protected]. For more options, visit https://groups.google.com/d/optout.
