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 ?