* [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar [not found] <ABFEDD1A8D2AAF42A54E3C074D49DC729DB2AD9B@penmbx01> @ 2014-09-18 8:11 ` Bo Shen 2014-09-18 9:59 ` Marek Vasut 0 siblings, 1 reply; 4+ messages in thread From: Bo Shen @ 2014-09-18 8:11 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar 2014-09-18 8:11 ` [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar Bo Shen @ 2014-09-18 9:59 ` Marek Vasut 2014-09-19 3:27 ` Bo Shen 0 siblings, 1 reply; 4+ messages in thread From: Marek Vasut @ 2014-09-18 9:59 UTC (permalink / raw) To: u-boot 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 ? ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar 2014-09-18 9:59 ` Marek Vasut @ 2014-09-19 3:27 ` Bo Shen 2014-09-19 4:24 ` Marek Vasut 0 siblings, 1 reply; 4+ messages in thread From: Bo Shen @ 2014-09-19 3:27 UTC (permalink / raw) To: u-boot 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar 2014-09-19 3:27 ` Bo Shen @ 2014-09-19 4:24 ` Marek Vasut 0 siblings, 0 replies; 4+ messages in thread From: Marek Vasut @ 2014-09-19 4:24 UTC (permalink / raw) To: u-boot 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! ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2014-09-19 4:24 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <ABFEDD1A8D2AAF42A54E3C074D49DC729DB2AD9B@penmbx01>
2014-09-18 8:11 ` [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar Bo Shen
2014-09-18 9:59 ` Marek Vasut
2014-09-19 3:27 ` Bo Shen
2014-09-19 4:24 ` Marek Vasut
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.