Hi Siarhei!
Am 19.11.2015 um 09:04 schrieb Siarhei Siamashka:
[...]
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").
If I understand you correctly, you'd do away with the "-p" and "-g[g]"
switches and instead use dedicated "[multi]progress" and "[multi][x]gauge"
commands, behaving like what would now be achieved with one of the
progress options combined with "write" or "multi[write]"?
It's easy enough to switch over to that, and stick with the special
treatment of the leading "-v" option (for extra/debugging output).
@Alex, what do you think of this?
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.
This is why they currently enforce `false` on aw_write_buffer().
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.
I agree that it should be possible to set the total size (only) once. Due
to the way the numbers passed to the callback function are modified, I'd
still want a way to tell if there's a "multi"-transfer pending - but that
could be done nicely using something like:
void progress_start(size_t total_bytes, bool multi);
Getting rid of the "quick" might be a bit tricky. Currently it's there to
allow suppressing those small (and therefore very fast) transfers that
users mostly won't be interested in when using the "classic" 'write'
command (boot script, DTB). If we discard that feature (i.e. simply show
progress updates for each and every transfer) or work around that some way,
it gets obsolete too.
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?
For those SoCs that could "do better" with larger chunk size, it does have
a small (but negligible) impact on performance / peek transfer rate, due
to frequent progress updates. I'd be fine with a larger default, too - like
the 512 KiB I selected initially; just wanted to "play it safe" for those
oddball devices somehow ending up with sluggish transfers. If we agree that
these might be lacking a bit in terms of (progress) update frequency, then
I'm happy to get rid of a SoC-based chunk selection.
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.
That probably makes sense, yes.
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?
No real objections here. Actually I'd like to get in a small patch that
fixes
some trivial issues (multiple includes, and an outdated reference to a
U-Boot
header) for 0.3. We might also prepare for the progress patches by already
introducing <stdbool.h> and converting existing code to proper boolean types
- what do you think?
Regards, B. Nortmann
--
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 linux-sunxi+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
