Re: [U-Boot] FW: [PATCH] usb: gadget: fastboot: improve download progress bar
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
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 ?
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
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!
participants (2)
-
Bo Shen -
Marek Vasut