[U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar

[Marek Vasut]

On Thursday, September 18, 2014 at 10:11:49 AM, Bo Shen wrote:
> Hi Marek,
> 
> On 09/18/2014 10:32 AM, Marek Vasut wrote:
> > On Thursday, September 18, 2014 at 03:34:18 AM, Bo Shen wrote:
> >> Hi Marek,
> >> 
> >> On 09/17/2014 07:16 PM, Marek Vasut wrote:
> >>> On Wednesday, September 17, 2014 at 12:28:57 PM, Bo Shen wrote:
> >>>> Hi Marek,
> >>>> 
> >>>> On 09/17/2014 06:10 PM, Marek Vasut wrote:
> >>>>> On Wednesday, September 17, 2014 at 09:43:56 AM, Bo Shen wrote:
> >>>>> 
> >>>>> +CC Lukasz, this is his turf.
> >>>>> 
> >>>>>> When download is ongoing, if the actual size of one transfer is
> >>>>>> not the same as BTYES_PER_DOT, which will cause the dot won't
> >>>>>> print anymore. Then it will let the user thinking it is stuck,
> >>>>>> actually it is transfering without dot printed.
> >>>>>> 
> >>>>>> So, improve the method to show the progress bar (print dot).
> >>>>>> 
> >>>>>> Signed-off-by: Bo Shen <voice.shen at atmel.com>
> >>>>>> ---
> >>>>>> 
> >>>>>>     drivers/usb/gadget/f_fastboot.c | 7 +++++--
> >>>>>>     1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>>> 
> >>>>>> diff --git a/drivers/usb/gadget/f_fastboot.c
> >>>>>> b/drivers/usb/gadget/f_fastboot.c index 7a1acb9..2f13bf0 100644
> >>>>>> --- a/drivers/usb/gadget/f_fastboot.c
> >>>>>> +++ b/drivers/usb/gadget/f_fastboot.c
> >>>>>> @@ -51,6 +51,7 @@ static inline struct f_fastboot
> >>>>>> *func_to_fastboot(struct usb_function *f) static struct
> >>>>>> f_fastboot *fastboot_func;
> >>>>>> 
> >>>>>>     static unsigned int download_size;
> >>>>>>     static unsigned int download_bytes;
> >>>>>> 
> >>>>>> +static unsigned int num_of_dot;
> >>>>>> 
> >>>>>>     static struct usb_endpoint_descriptor fs_ep_in = {
> >>>>>>     
> >>>>>>     	.bLength            = USB_DT_ENDPOINT_SIZE,
> >>>>>> 
> >>>>>> @@ -414,9 +415,10 @@ static void rx_handler_dl_image(struct
> >>>>>> usb_ep *ep, struct usb_request *req) req->length = ep->maxpacket;
> >>>>>> 
> >>>>>>     	}
> >>>>>> 
> >>>>>> -	if (download_bytes && !(download_bytes % BYTES_PER_DOT)) {
> >>>>>> +	if (download_bytes && ((download_bytes / BYTES_PER_DOT) >
> >>>>>> num_of_dot)) { +		num_of_dot = download_bytes / 
BYTES_PER_DOT;
> >>>>>> 
> >>>>>>     		putc('.');
> >>>>>> 
> >>>>>> -		if (!(download_bytes % (74 * BYTES_PER_DOT)))
> >>>>>> +		if (!(num_of_dot % 74))
> >>>>>> 
> >>>>>>     			putc('\n');
> >>>>>>     	
> >>>>>>     	}
> >>>>>>     	req->actual = 0;
> >>>>>> 
> >>>>>> @@ -431,6 +433,7 @@ static void cb_download(struct usb_ep *ep,
> >>>>>> struct usb_request *req) strsep(&cmd, ":");
> >>>>>> 
> >>>>>>     	download_size = simple_strtoul(cmd, NULL, 16);
> >>>>>>     	download_bytes = 0;
> >>>>>> 
> >>>>>> +	num_of_dot = 0;
> >>>>> 
> >>>>> Make it a 'download_total' and log the total amount of bytes
> >>>>> transferred please, that way it can be re-used for other purposes
> >>>>> in the future ; for example for printing how much data were
> >>>>> already transferred ;-)
> >>>> 
> >>>> The download_bytes record the total amount of bytes transferred.
> >>>> And the download_bytes will print after finishing transfer.
> >>> 
> >>> So why can this not be used to indicate the total progress ? Because
> >>> the transfeer speed is variating too much ?
> >> 
> >> As I described in the commit message. If the transfer length is not
> >> exactly the same as the request length, then the old method
> >> 
> >>     "download_bytes % BYTES_PER_DOT"
> >> 
> >> won't be 0 anymore, so for the following transfer, it won't print dot
> >> anymore.
> > 
> > And can you not reset the "download_bytes" for each transfer ?
> 
> No, I don't reset the "download_bytes" for each transfer. It reset the
> "download_bytes" before transfer start.
> The "download_bytes" is increase in rx_handler_dl_image function.

OK

> > Maybe we're not even aligned on what "transfer" means, so we might want
> > to sync on this word first. What does "transfer" mean in this case?
> 
> Transfer I mean here is a usb request, which trying to transfer
> EP_BUFFER_SIZE at one time.
> In my test case, sometime it transfer less than EP_BUFFER_SIZE in a usb
> request. So, it cause dot won't print the dot, and seems stuck. However,
> it will finish transfer after some time.

I see now. This code is really weird.

What would happen if the following condition is met in the code for k>0 ?
(download_bytes == download_size) AND (download_bytes = k * BYTES_PER_DOT)

I think the original code would happily print a dot after printing this output:
printf("\ndownloading of %d bytes finished\n", download_bytes);

Do you agree ? If yes, then I believe this code should go into the else branch 
only.

Also, you can probably avoid the counting variable if you do something like:

if (download_bytes / CONST != (download_bytes + transfer_size) / CONST) {
	print(dot);
	if (download_bytes / (74 * CONST) != ((download_bytes + transfer_size) / 
(74 * CONST))
		print(\n);
}

Surely, the code can be simplified . You would also need to be careful about 
this assignment at the top of the function : download_bytes += transfer_size;

What do you think ?