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

[Bo Shen]

Hi Marek,

On 09/18/2014 05:59 PM, Marek Vasut wrote:
> 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 am not fully understand what you plan to present here.

> 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.

Yes, I agree. This may happen, if the (download_bytes % BTYES_PER_DOT) 
equals to 0.

> 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 ?
>

I think this piece of code is better, which won't introduce new variable.

If no other comments, I will modify the code like this and send the v2 
patch.

Best Regards,
Bo Shen