Subject: [PATCH sunxi-tools 1/1] option --progress: shows progress bar for large transfers

[kapla...@gmail.com]

Hi,

I added an option --progress (or -p for short) to display a progress bar during large transfers.

Hope it is useful.
Alex

---
 fel.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 59 insertions(+), 21 deletions(-)

diff --git a/fel.c b/fel.c
index e9f6450..27d30de 100644
--- a/fel.c
+++ b/fel.c
@@ -64,6 +64,7 @@ static int AW_USB_FEL_BULK_EP_OUT;
 static int AW_USB_FEL_BULK_EP_IN;
 static int timeout = 60000;
 static int verbose = 0; /* Makes the 'fel' tool more talkative if non-zero */
+static int progress = 0; /* Makes the 'fel' tool show a progress bar when transferring large files */
 static uint32_t uboot_entry = 0; /* entry point (address) of U-Boot */
 static uint32_t uboot_size  = 0; /* size of U-Boot binary */
 
@@ -79,11 +80,34 @@ static void pr_info(const char *fmt, ...)
 
 static const int AW_USB_MAX_BULK_SEND = 4 * 1024 * 1024; // 4 MiB per bulk request
 
-void usb_bulk_send(libusb_device_handle *usb, int ep, const void *data, int length)
+typedef void (*progress_cb_t)(int total,int sent,int len);
+
+void progress_bar(int total,int sent,int len)
+{
+    if (progress && (len<total)) {
+        int   w = 60;
+        float r = ((float)sent)/total;
+        int   x = w * r;
+        int   i;
+
+        fprintf(stderr,"\r%3d%% [", (int)(r*100) );
+
+        for (i=0;i<x;i++) {
+            fprintf(stderr,"=");
+        }
+        for (i=x;i<w;i++) {
+            fprintf(stderr," ");
+        }
+
+        fprintf(stderr,"] ");
+    }
+}
+
+void usb_bulk_send(libusb_device_handle *usb, int ep, const void *data, int length, progress_cb_t progress_cb)
 {
-    int rc, sent;
+    int rc, sent, total=length, len;
     while (length > 0) {
-        int len = length < AW_USB_MAX_BULK_SEND ? length : AW_USB_MAX_BULK_SEND;
+        len = length < AW_USB_MAX_BULK_SEND ? length : AW_USB_MAX_BULK_SEND;
         rc = libusb_bulk_transfer(usb, ep, (void *)data, len, &sent, timeout);
         if (rc != 0) {
             fprintf(stderr, "libusb usb_bulk_send error %d\n", rc);
@@ -91,6 +115,10 @@ void usb_bulk_send(libusb_device_handle *usb, int ep, const void *data, int leng
         }
         length -= sent;
         data += sent;
+
+        if (progress_cb) {
+            progress_cb(total, total-length, len);
+        }
     }
 }
 
@@ -156,7 +184,7 @@ void aw_send_usb_request(libusb_device_handle *usb, int type, int length)
     req.length = req.length2 = htole32(length);
     req.request = htole16(type);
     req.unknown1 = htole32(0x0c000000);
-    usb_bulk_send(usb, AW_USB_FEL_BULK_EP_OUT, &req, sizeof(req));
+    usb_bulk_send(usb, AW_USB_FEL_BULK_EP_OUT, &req, sizeof(req), NULL);
 }
 
 void aw_read_usb_response(libusb_device_handle *usb)
@@ -166,17 +194,17 @@ void aw_read_usb_response(libusb_device_handle *usb)
     assert(strcmp(buf, "AWUS") == 0);
 }
 
-void aw_usb_write(libusb_device_handle *usb, const void *data, size_t len)
+void aw_usb_write(libusb_device_handle *usb, const void *data, size_t len, progress_cb_t progress_cb)
 {
     aw_send_usb_request(usb, AW_USB_WRITE, len);
-    usb_bulk_send(usb, AW_USB_FEL_BULK_EP_OUT, data, len);
+    usb_bulk_send(usb, AW_USB_FEL_BULK_EP_OUT, data, len, progress_cb);
     aw_read_usb_response(usb);
 }
 
-void aw_usb_read(libusb_device_handle *usb, const void *data, size_t len)
+void aw_usb_read(libusb_device_handle *usb, const void *data, size_t len, progress_cb_t progress_cb)
 {
     aw_send_usb_request(usb, AW_USB_READ, len);
-    usb_bulk_send(usb, AW_USB_FEL_BULK_EP_IN, data, len);
+    usb_bulk_send(usb, AW_USB_FEL_BULK_EP_IN, data, len, progress_cb);
     aw_read_usb_response(usb);
 }
 
@@ -199,19 +227,19 @@ void aw_send_fel_request(libusb_device_handle *usb, int type, uint32_t addr, uin
     req.request = htole32(type);
     req.address = htole32(addr);
     req.length = htole32(length);
-    aw_usb_write(usb, &req, sizeof(req));
+    aw_usb_write(usb, &req, sizeof(req), NULL);
 }
 
 void aw_read_fel_status(libusb_device_handle *usb)
 {
     char buf[8];
-    aw_usb_read(usb, &buf, sizeof(buf));
+    aw_usb_read(usb, &buf, sizeof(buf), NULL);
 }
 
 void aw_fel_get_version(libusb_device_handle *usb, struct aw_fel_version *buf)
 {
     aw_send_fel_request(usb, AW_FEL_VERSION, 0, 0);
-    aw_usb_read(usb, buf, sizeof(*buf));
+    aw_usb_read(usb, buf, sizeof(*buf), NULL);
     aw_read_fel_status(usb);
 
     buf->soc_id = (le32toh(buf->soc_id) >> 8) & 0xFFFF;
@@ -249,7 +277,11 @@ void aw_fel_print_version(libusb_device_handle *usb)
 void aw_fel_read(libusb_device_handle *usb, uint32_t offset, void *buf, size_t len)
 {
     aw_send_fel_request(usb, AW_FEL_1_READ, offset, len);
-    aw_usb_read(usb, buf, len);
+    aw_usb_read(usb, buf, len, progress ? progress_bar : NULL);
+    if (progress) {
+        fprintf(stderr,"\n");
+    }
+
     aw_read_fel_status(usb);
 }
 
@@ -264,7 +296,10 @@ void aw_fel_write(libusb_device_handle *usb, void *buf, uint32_t offset, size_t
         exit(1);
     }
     aw_send_fel_request(usb, AW_FEL_1_WRITE, offset, len);
-    aw_usb_write(usb, buf, len);
+    aw_usb_write(usb, buf, len, progress ? progress_bar : NULL);
+    if (progress) {
+        fprintf(stderr,"\n");
+    }
     aw_read_fel_status(usb);
 }
 
@@ -1064,6 +1099,7 @@ int main(int argc, char **argv)
     if (argc <= 1) {
         printf("Usage: %s [options] command arguments... [command...]\n"
             "    -v, --verbose            Verbose logging\n"
+            "    -p, --progress            Show progress bar when transferring large files\n"
             "\n"
             "    spl file            Load and execute U-Boot SPL\n"
             "        If file additionally contains a main U-Boot binary\n"
@@ -1114,16 +1150,18 @@ int main(int argc, char **argv)
         exit(1);
     }
 
-    if (argc > 1 && (strcmp(argv[1], "--verbose") == 0 ||
-             strcmp(argv[1], "-v") == 0)) {
-        verbose = 1;
-        argc -= 1;
-        argv += 1;
-    }
-
     while (argc > 1 ) {
         int skip = 1;
-        if (strncmp(argv[1], "hex", 3) == 0 && argc > 3) {
+
+        if (argc > 1 && (strcmp(argv[1], "--verbose") == 0 ||
+                 strcmp(argv[1], "-v") == 0)) {
+            verbose = 1;
+            skip = 1;
+        } else if (argc > 1 && (strcmp(argv[1], "--progress") == 0 ||
+                 strcmp(argv[1], "-p") == 0)) {
+            progress = 1;
+            skip = 1;
+        } else if (strncmp(argv[1], "hex", 3) == 0 && argc > 3) {
             aw_fel_hexdump(handle, strtoul(argv[2], NULL, 0), strtoul(argv[3], NULL, 0));
             skip = 3;
         } else if (strncmp(argv[1], "dump", 4) == 0 && argc > 3) {
--
2.1.4

[Bernhard Nortmann]

Hi Alex!

Am Freitag, 23. Oktober 2015 22:22:24 UTC+2 schrieb kapla...@gmail.com:
> Hi,
>
> I added an option --progress (or -p for short) to display a progress bar during large transfers.
>
> Hope it is useful.
> Alex
>
> ---
> fel.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++------------------
> 1 file changed, 59 insertions(+), 21 deletions(-)
>
> diff --git a/fel.c b/fel.c
> index e9f6450..27d30de 100644
> --- a/fel.c
> +++ b/fel.c
> @@ -64,6 +64,7 @@ static int AW_USB_FEL_BULK_EP_OUT;
> static int AW_USB_FEL_BULK_EP_IN;
> static int timeout = 60000;
> static int verbose = 0; /* Makes the 'fel' tool more talkative if non-zero */
> +static int progress = 0; /* Makes the 'fel' tool show a progress bar when transferring large files */

Instead of introducing another global here, we could strive to make it
local to main() instead. The potential drawback is that it might require
passing the actual value to the functions involved as an additional
parameter.

I can also imagine not having it as a "flag" type, but instead use the
progress callback (function) pointer directly, defaulting it to NULL.
Specifying "-p" (or "--progress") would then set it to progress_bar().

I'd prefer to substitute "static int WIDTH = 60;" for that "w", making
it more clear we're dealing with a 'constant' value that's somewhat
user-configurable. The check for "progress" here is redundant - after
all, if it's not enabled, the function isn't supposed to be called in
the first place.

The typecasts are understandable, but a bit awkward. Maybe do
float r = 1.0f * sent / total;
and
fprintf(stderr,"\r%3.0f%% [", r * 100.0f);
instead?

Is it necessary to print this to stderr instead of stdout? (e.g. pr_info
uses the latter) Are you simply avoiding having to call fflush() that way?

> +void usb_bulk_send(libusb_device_handle *usb, int ep, const void *data, int length, progress_cb_t progress_cb)
> {
> - int rc, sent;
> + int rc, sent, total=length, len;
> while (length > 0) {
> - int len = length < AW_USB_MAX_BULK_SEND ? length : AW_USB_MAX_BULK_SEND;
> + len = length < AW_USB_MAX_BULK_SEND ? length : AW_USB_MAX_BULK_SEND;
> rc = libusb_bulk_transfer(usb, ep, (void *)data, len, &sent, timeout);
> if (rc != 0) {
> fprintf(stderr, "libusb usb_bulk_send error %d\n", rc);
> @@ -91,6 +115,10 @@ void usb_bulk_send(libusb_device_handle *usb, int ep, const void *data, int leng
> }
> length -= sent;
> data += sent;
> +
> + if (progress_cb) {
> + progress_cb(total, total-length, len);
> + }
> }
> }

"len" was taken from the original source. If we're reworking that, why
not use "chunk" instead? I'm inclined to find "len" vs. "length"
slightly confusing.

If "progress" (or maybe rename it to "progress_cb") was a function
pointer here, as suggested further up. this would simply be:
aw_usb_read(usb, buf, len, progress_cb);

Also, I don't like the "\n" being output here. I'd think that to be a
task of the progress_bar() function - it's simple enough to detect
completion by testing for (sent == total) at the end.

> aw_read_fel_status(usb);
> }
>
> @@ -264,7 +296,10 @@ void aw_fel_write(libusb_device_handle *usb, void *buf, uint32_t offset, size_t
> exit(1);
> }
> aw_send_fel_request(usb, AW_FEL_1_WRITE, offset, len);
> - aw_usb_write(usb, buf, len);
> + aw_usb_write(usb, buf, len, progress ? progress_bar : NULL);
> + if (progress) {
> + fprintf(stderr,"\n");
> + }

same as before

As you've noticed, the introduction of multiple "flag"-type command line
arguments requires a change in their handling. The approach above
is reasonable, as we can expect (or just tell) users to specify "-v" or
"-p" at the beginning of parameters.

Note that inside the loop "(argc > 1)" and "skip = 1;" are redundant and
should be removed.

> aw_fel_hexdump(handle, strtoul(argv[2], NULL, 0), strtoul(argv[3], NULL, 0));
> skip = 3;
> } else if (strncmp(argv[1], "dump", 4) == 0 && argc > 3) {
> --
> 2.1.4

Regards, NiteHawk

[Bernhard Nortmann]

I've been discussing this briefly with libv on IRC, and he was in favour of
a boolean "progress" flag and keeping it global (which simplifies things).

After a bit of experimenting with it, the default maximum bulk transfer
size of 4 MiB causes rather long intervals inbetween updates of the
progress bar. It's probably better to lower the "chunk" size to smaller,
more useful values when progress display is desired.

So I decided to quickly throw together a revised commit/patch that
incorporates the suggested changes. It's attached to this message.

Regards, B. Nortmann

[Alexander Kaplan]

Hi Bernhard,

thanks for picking this up.

Your changes look good to me.

Cheers

Alex

[Bernhard Nortmann]

I've transformed this into a pull request:
https://github.com/linux-sunxi/sunxi-tools/pull/35

Let me know if you'd prefer a set of patches sent to the mailing list
(@Siarhei?).

Regards, B. Nortmann

[Siarhei Siamashka]

On Fri, 30 Oct 2015 13:44:24 +0100
Bernhard Nortmann <bernhard...@web.de> wrote:

> I've transformed this into a pull request:
> https://github.com/linux-sunxi/sunxi-tools/pull/35

Hello,

If I understand the problem correctly, it is all about slow USB
transfer speed. We have two possible ways to deal with it:
1. Improve the performance to finish the job faster
2. Entertain the user while the transfer is in progress

Improving the performance of transferring huge initramfs images
to the device can be done by using some other USB protocol in
U-Boot (fastboot or DFU) instead of the inherently less than
perfect FEL implementation in the BROM. Out of these two, DFU
is reasonably simple and easy to use:
http://lists.denx.de/pipermail/u-boot/2015-October/231589.html

Please note that U-Boot is not quite ready yet and needs some
work because not all of the sunxi devices have USB OTG enabled
in defconfigs and U-Boot still can't select the host/device
role based on the state of the ID pin. For smooth integration
and ease of use, the sunxi-fel tool would probably have to
cannibalize the dfu-util code (which is rather small and has a
compatible license) and automatically handle the transition
from the FEL protocol to DFU. The U-Boot SPL header would also
need to have some flag, which indicates that it supports DFU.

Some tuning is possible for the FEL transfers too. This had
been taken care of by the older "fel: Faster USB transfers via
'fel write' to DRAM" commit:
https://github.com/linux-sunxi/sunxi-tools/commit/e4b3da2b17ee9d7c
Not all of the SoC variants can benefit from it yet, but this
can be fixed.

The transfer speed using different methods is listed in the
SoC support status table in the linux-sunxi wiki:
https://linux-sunxi.org/FEL/USBBoot#SoC_support_status


Now regarding the "entertainment" part. Adding a progress bar is
reasonably simple. The most challenging aspect is the 'sunxi-fel'
command line interface evolution, so that it remains easy and
intuitive to use and also does not break compatibility with older
'sunxi-fel' versions unnecessarily in the future. Let's consider
the following current command line usage example:

sunxi-fel uboot u-boot-sunxi-with-spl.bin \
write 0x42000000 uImage \
write 0x43000000 sun7i-a20-cubietruck.dtb \
write 0x43100000 boot.scr \
write 0x43300000 rootfs.cpio.lzma.uboot

This boots the whole system with a single 'sunxi-fel' tool invocation.

How many progress bars do we want to see in this case? Do we want
to have a progress bar for each of the files? Do we want a common
progress bar for all of them? Do we want to selectively enable
progress bars only for some of the files (for example, the initramfs
image 'rootfs.cpio.lzma.uboot' can be huge, but 'boot.scr' and the
dtb files are usually tiny).

I see that you seem to be proposing a solution, which allows placing
the '-p' option at any part of the command line. So that for having
the progress bar only for the initramfs file, we can use it as:

sunxi-fel uboot u-boot-sunxi-with-spl.bin \
write 0x42000000 uImage \
write 0x43000000 sun7i-a20-cubietruck.dtb \
write 0x43100000 boot.scr \
-p write 0x43300000 rootfs.cpio.lzma.uboot

This looks rather clever/smartass. The down side is that it looks a
little bit unorthodox. The command line tools usually have the options
prefixed by '-' only in the beginning of the command line. Additionally,
we might want to have another option to toggle the progress bar off in
the middle of the command line too.

But maybe as an alternative, we could just substitute "-p write" with
a new command "writep"? The exact name is yet to be decided, it could
be either short or a rather verbose. Which would do the write with
the progress bar enabled for this particular transfer. We can even
introduce another command to transfer multiple files with a common
progress bar. For example, something like this:

sunxi-fel uboot u-boot-sunxi-with-spl.bin \
write-many-with-progress 4 \
0x42000000 uImage \
0x43000000 sun7i-a20-cubietruck.dtb \
0x43100000 boot.scr \
0x43300000 rootfs.cpio.lzma.uboot

This would transfer 4 files with a common shared progress bar. Now
regarding the progress bar itself. It would be also very nice to
see the current transfer speed and remaining time. The progress bar
of 'wget' or similar tools could be used as an inspiration.
Alternatively, it may be a good idea to support integration with
tools like 'dialog' for implementing nice loking console user
interfaces:
http://swoolley.org/man.cgi/dialog

An example of implementing progress bar with 'dialog':

for i in $(seq 0 10 100) ; do sleep 1; echo $i | dialog --gauge \
"Please wait" 10 70 0; done

If we want to support 'dialog', then we might introduce one more
command "write-many-with-dialog-progress", which would just print
the current percent of transferred data to stdout for piping
into 'dialog'.

The syntax of "write-many-with-progress" might look a bit cumbersome
when typing short commands in the command line. So we might want to
introduce a syntax sugar :-) The '-p' option in the beginning
of the command line might be handy. It could be interpreted as a
global option, which would join back-to-back "write" commands and
display a common progress bar for all of them.



Regarding the implementation details. IMHO the right place to integrate
all of this would be in the part of code, which currently benchmarks
the transfer speed:
https://github.com/linux-sunxi/sunxi-tools/blob/9bf1de0ad822/fel.c#L1144

In order to get progress updates, we would need to use something like
a new "aw_fel_write_with_progress_callback()" function instead of
"aw_fel_write()". The progress callback would get an opaque pointer
parameter (a pointer to a struct with all the progress bar state data)
and the size of the just transferred data chunk.

It had been already discussed before that the "aw_fel_write()" is a
kind of privileged operation, which is heavily used as a part of
implementing more complex commands (such as "spl"). We don't want
to hook up tons of ballast there, even the following check should
be better done outside of "aw_fel_write()" and also handle the
"fill" and "clear" commands too:
https://github.com/linux-sunxi/sunxi-tools/blob/9bf1de0ad822/fel.c#L258

We can introduce a separate non-privileged write operation for
serving commands from the command line, with an optional
progress callback argument. The callback functions could have
different implementations for the regular progress bar and
dialog-compatible progress bar.


Please let me know what you think about this. The point is that
the main concern is about *what* we want to implement, and much
less about *how* we do it. If we add some weird command line options
now, they can become a liability in the future and become a source
of confusion for the users.

> Let me know if you'd prefer a set of patches sent to the mailing list
> (@Siarhei?).

The mailing list is currently used because it allows the patches to
be noticed and reviewed by a large crowd of mailing list subscribers.

Yes, it is possible to stop spamming the mailing list and move entirely
to github with the pull requests based workflow. How many people would
be willing to subscribe to github notifications for the sunxi-tools
project on github? Also such workflow works best when having a real
maintainer, who is responsible for reviewing such pull requests in
a timely manner. Instead of random people who may or may not be in
the mood to do it :-)

Bernhard, would you perhaps want to become the maintainer of
sunxi-tools?

--
Best regards,
Siarhei Siamashka

[Alex Kaplan]

Hi,

of course the reason to entertain the user is the slow transfer speed.
Improving the speed is another discussion though.
A simple progress bar makes sense independent of the transfer speed.

Displaying the data rate is nice but involves some time measurements.
That's why I like the idea to have an option for piping progress
information to pv or dialog.
Calculating the overall progress for multiple file transfers would
require to determine the size of all input files at the beginning, but
should be doable.

Are data rate display and calculation of the overall progress for
multiple files a requirement? Or could those possibly be added in a
later step?

To avoid future obligations regarding command line parameters one could
also define environment variable to enable the display of progress
information.

[Bernhard Nortmann]

Hi Siarhei!

Am 11.11.2015 um 01:16 schrieb Siarhei Siamashka:
> [...]

> Hello,
>
> If I understand the problem correctly, it is all about slow USB
> transfer speed. We have two possible ways to deal with it:
> 1. Improve the performance to finish the job faster
> 2. Entertain the user while the transfer is in progress

I'm with Alexander here - having some kind of progress display is
independent
of the actual transfer speed. Now matter how fast you get going in the end,
there'll probably always be some files (especially initrd) that are "huge"
enough to cause some noticeable transfer delays. For the most part, the
display
may be about "entertaining" the user while he waits, but it also provides
useful visual feedback that the transfer is actually still in progress.

> [...]

>
> Now regarding the "entertainment" part. Adding a progress bar is
> reasonably simple. The most challenging aspect is the 'sunxi-fel'
> command line interface evolution, so that it remains easy and
> intuitive to use and also does not break compatibility with older
> 'sunxi-fel' versions unnecessarily in the future. Let's consider
> the following current command line usage example:
>
> sunxi-fel uboot u-boot-sunxi-with-spl.bin \
> write 0x42000000 uImage \
> write 0x43000000 sun7i-a20-cubietruck.dtb \
> write 0x43100000 boot.scr \
> write 0x43300000 rootfs.cpio.lzma.uboot
>
> This boots the whole system with a single 'sunxi-fel' tool invocation.
>
> How many progress bars do we want to see in this case? Do we want
> to have a progress bar for each of the files? Do we want a common
> progress bar for all of them? Do we want to selectively enable
> progress bars only for some of the files (for example, the initramfs
> image 'rootfs.cpio.lzma.uboot' can be huge, but 'boot.scr' and the
> dtb files are usually tiny).

If I understood Alex right, this is where the "large"/huge definition comes
into play. His proposal makes use of the chunk size to suppress the progress
display for transfers that are reasonably small, i.e. complete in a single
iteration of the usb_bulk_send() loop. So we can mitigate this problem a bit
by carefully selecting an appropriate chunk size. In your example, the 512K
value I've introduced would ignore the transfers for U-Boot itself, DTB and
script file - so you'd probably end up with two progress bars: one for
uImage,
and another for the rootfs.

> I see that you seem to be proposing a solution, which allows placing
> the '-p' option at any part of the command line. So that for having
> the progress bar only for the initramfs file, we can use it as:
>
> sunxi-fel uboot u-boot-sunxi-with-spl.bin \
> write 0x42000000 uImage \
> write 0x43000000 sun7i-a20-cubietruck.dtb \
> write 0x43100000 boot.scr \
> -p write 0x43300000 rootfs.cpio.lzma.uboot
>
> This looks rather clever/smartass. The down side is that it looks a
> little bit unorthodox. The command line tools usually have the options
> prefixed by '-' only in the beginning of the command line. Additionally,
> we might want to have another option to toggle the progress bar off in
> the middle of the command line too.

Yes, this would be an implicit benefit of allowing these options anywhere on
the command line. Handling the "boolean" options this way may be a little
unusual, but it makes sense. I didn't even think of being able to turn the
progress display off again - that's a pretty neat suggestion, too.

It's possible to do that. However, currently I fail to see any real benefit
that this approach would offer over a (more intuitive?) "--progress" /
"--no-progress" solution. (Apart from being able to postpone the decision
on how to handle multiple command line options.)

Support for the 'dialog' utility may be interesting, yes. However, we'd have
to be careful about other program output mixing in, possibly confusing
'dialog' - especially with "--verbose"?

> Regarding the implementation details. IMHO the right place to integrate
> all of this would be in the part of code, which currently benchmarks
> the transfer speed:
> https://github.com/linux-sunxi/sunxi-tools/blob/9bf1de0ad822/fel.c#L1144
>
> In order to get progress updates, we would need to use something like
> a new "aw_fel_write_with_progress_callback()" function instead of
> "aw_fel_write()". The progress callback would get an opaque pointer
> parameter (a pointer to a struct with all the progress bar state data)
> and the size of the just transferred data chunk.

I'm afraid that won't fly - at least not without more fundamental changes.
The basic problem here is that any kind of progress callback (either direct
or via a function pointer) would have to take place at the "chunk" transfer
level. Currently the only place to achieve that is the corresponding loop in
usb_bulk_send(). Any potential aw_fel_write_with_progress_callback() wishing
more fine-grained control would still have to trickle down the progress
callback (or flag) information down the call chain (-> aw_write_usb
-> usb_bulk_send), as long as we'd want to avoid code duplication or using
a global variable for this purpose. I think what you're suggesting would
ultimately lead to a design of "aw_fel_write(..., progress_callback)"
vs. "aw_fel_write(..., NULL)", depending on whether (optional) progress
display is desired or unwanted.

> It had been already discussed before that the "aw_fel_write()" is a
> kind of privileged operation, which is heavily used as a part of
> implementing more complex commands (such as "spl"). We don't want
> to hook up tons of ballast there, even the following check should
> be better done outside of "aw_fel_write()" and also handle the
> "fill" and "clear" commands too:
> https://github.com/linux-sunxi/sunxi-tools/blob/9bf1de0ad822/fel.c#L258
>
> We can introduce a separate non-privileged write operation for
> serving commands from the command line, with an optional
> progress callback argument. The callback functions could have
> different implementations for the regular progress bar and
> dialog-compatible progress bar.

I understand your point here. It's probably a sane approach to keep the
"privileged" aw_fel_write() down to a minimum, and have an intermediate
("aw_write_buffer()"?) that handles additional checks and features, where
higher-level invocations like the spl/fill stuff would make use of this
function instead of aw_fel_write().

Still, this doesn't solve the fundamental problem of having to invoke the
callback at the usb_bulk_send() level, nor the fact that this low-level
routine gets used for lots of "privileged" operations. The only solution
I can
currently think of is either enforcing a NULL callback from the caller, or
to make sure that all of the smaller transfers involved in control
operations
ignore the callback (the "single chunk" logic mentioned earlier). It may be
best to combine both.

> Please let me know what you think about this. The point is that
> the main concern is about *what* we want to implement, and much
> less about *how* we do it. If we add some weird command line options
> now, they can become a liability in the future and become a source
> of confusion for the users.

With the current "boolean" type options (verbosity and progress display) on
the table, I don't see any immediate disadvantage of converting them to
"positional" command line arguments and being able to switch them on/off
multiple times. Except for potentially unwanted, "excess" output they're
also
not causing any harm when users always specify them at the beginning.
Maybe their "proper" usage might require explicit documentation.

Also, at the moment I perceive no extra value in trying to preserve / parse
all options at the start of the command line. The same goes for introducing
additional, specialized variants (with progress display) of the "write"
command instead of having an option for it. Even a global, "all or nothing"
progress flag would have little negative impact (in terms of overhead or
excess output), as long as it properly ignores "small" transfers.

>> Let me know if you'd prefer a set of patches sent to the mailing list
>> (@Siarhei?).
> The mailing list is currently used because it allows the patches to
> be noticed and reviewed by a large crowd of mailing list subscribers.
>
> Yes, it is possible to stop spamming the mailing list and move entirely
> to github with the pull requests based workflow. How many people would
> be willing to subscribe to github notifications for the sunxi-tools
> project on github? Also such workflow works best when having a real
> maintainer, who is responsible for reviewing such pull requests in
> a timely manner. Instead of random people who may or may not be in
> the mood to do it :-)

I see. I just noticed that it has occasionally been done "the github way"
in the past, and was wondering if it might things easier for you or the
(apparently missing) maintainer. But I'll stick with the mailing list then.
I'll probably rework the patches from my pull request over the next days,
and submit a new version to the ML after that.

> Bernhard, would you perhaps want to become the maintainer of
> sunxi-tools?

Hmm... Remember it was you in the first place who talked me into picking up
the FEL <-> U-Boot interoperability stuff? :D

Actually I don't follow the mailing list very closely, and - apart from what
little I've picked up from the 'fel' utility - I'm not very familiar
with the
inner workings of sunxi-tools. They cover a lot of areas (USB, JTAG, NAND)
where I don't feel much at home; usually I grab/tweak whatever I need
and move
on. Even figuring that it's probably more about mere administrative
tasks, my
limited time would likely make me a poor maintainer.

Regards, B. Nortmann