Hi Bernhard,
On Thu, 19 Nov 2015 15:28:47 +0100
Bernhard Nortmann <[email protected]> wrote:
> 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]"?
Yes.
> 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().
Right. I just mean that we don't need special progressbar-enabled
variants of the "fill" and "clear" commands, so things do not get
out of control. If these switches were only intended to affect the
"write"/"multiwrite" commands, then just merging the switches into new
variants in the form of "write-with-some-extra-bells-and-whistles"
commands does not make anything more complicated. Moreover, as I
mentioned earlier, the "multiwrite" command makes little sense
without the progressbar feature because it can't offer any other
advantages over "write". When having a variety of commands, it
is also easier to deprecate and drop (or rename) some commands
in the future. If the user supplies an unsupported command, the
tool can just say that the command is not supported anymore. But
if we want to deprecate and drop some "switch + command" combination,
then everything becomes a lot more confusing.
There is also the "read" command, but I'm not sure if it needs
a progressbar either. Because I can't imagine it being ever used
to transfer a lot of data. Yes, we might implement something like
backing up the NAND flash in the future, but this will be probably
done by communicating with U-Boot (which can do the NAND reading
job) using the DFU protocol instead of FEL.
> > 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);
I don't quite understand. Why do we want to distinguish between single
and multi transfers? Can you provide an example?
> 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.
Oh, in fact that's one more thing that I really don't like. The tool
trying to be smart and deciding itself whether to show or skip a
progressbar for certain files may be very confusing for the users.
Especially if the threshold may vary depending on the transfer speeds,
which were supposed to be tabulated for each SoC in the original
patch set variant.
Basically, we have two distinct use cases:
1. The user is manually typing commands in the command line
2. The sunxi-fel tool is executed by a wrapper script or a GUI frontend
In the former case, we probably want to ensure that the user does not
need to type too much. That's why the '-v' and '--verbose' aliases
exist. And that's why the "uboot" command automatically schedules the
execution of U-Boot right before exit. The user simply does not have to
type too much. And for this use case, I actually proposed to still have
the '-p' switch supported, which would do a smart but predictable job
(combine back-to-back "write" commands and show a normal progressbar
for them).
In the latter case (wrapper scripts and frontends), we don't care much
about reducing typing but instead prefer better readability (that's why
the '--verbose' switch may be preferable there) and also perfect
control over the tool behaviour. If the script author wants to show a
shared progressbar for a selected set of files, then it can be done
via the "multiwrite-???" command. If the script author wants to show
separate progressbars for certain files or don't show it for some of
them at all, then this can be done too. The "quick" option would just
get in the way in a poorly predictable manner.
Also it looks like adding support for a trivial "echo" command might be
useful for adding extra annotations or markers between file transfers.
For example:
sunxi-fel uboot u-boot-sunxi-with-spl.bin \
echo "XXX" \
echo "0" \
echo "Uploading file A (1/2)" \
echo "XXX" \
write-with-dialog-gauge 0x40000000 file-a.bin \
echo "XXX" \
echo "0" \
echo "Uploading file B (2/2)" \
echo "XXX" \
write-with-dialog-gauge 0x41000000 file-b.bin \
| dialog --gauge "Starting upload" 6 70
> > 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.
How much of the performance are we losing exactly? The smaller chunk
size selection makes the progressbar much more lively, which is nice.
If the performance loss is less than 5%, then we probably don't care.
And in fact, this performance loss can be probably reduced/eliminated
if we move the USB transfer and progressbar updates into separate
threads (not that we have any need to do this right now).
> 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.
Let's just get rid of it for now.
> > 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
Thanks!
--
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.
