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

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

[Bo Shen]

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

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

Best Regards,
Bo Shen

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

[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@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

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

[Marek Vasut]

On Friday, September 19, 2014 at 05:27:05 AM, Bo Shen wrote:
> Hi Marek,
[...]
> >> 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.

Well OK then. Thanks!