tux-droid-user

[tuxdroid-user] usb_read_TuxDroid() correctness

[neimad <ror...@gm...>]

I have comments and questions about usb_read_TuxDroid():

  (1) It uses a for loop to read data from USB, and tampers with the
      loop index within the loop's body. This is generally not a good
      idea; a while loop would be preferable in such a case.

  (2) Where does the loop's upper bound (5) come from ? Does it have
      anything to do with TUX_RECV_LENGTH, which also happens to be
      5 (but soon to be fixed) ?

  (3) The documentation[1] of usb_interrupt_read() doesn't say much,
      but I guess it may read fewer bytes than requested, and even
      return 0 in case of a timeout. This means that the loop could
      have read data in several iterations and still reset the loop
      index to 0.

[1] http://libusb.sourceforge.net/doc/function.usbinterruptread.html

I don't get what this is supposed to do. If this code is indeed
correct, it should be rewritten and/or commented.

	Damien

Re: [tuxdroid-user] usb_read_TuxDroid() correctness

[David B. <da...@ja...>]

On Mon, 04 Jun 2007 22:29:30 +0200, neimad <ror...@gm...> wrote:

>
> I have comments and questions about usb_read_TuxDroid():

Looking into it, it seems to be an old function which is not used anymore.  
In the previous usb firmware, status were read one at a time (4 bytes) and  
usually 5 different status were generated each 100ms, that's where the  
loop and the upper bound come from. This didn't make much sense that's why  
I suggested to change the usb protocol to be able to request all status  
that are available in one shot.

I'm going to remove that function.

>   (3) The documentation[1] of usb_interrupt_read() doesn't say much,
>       but I guess it may read fewer bytes than requested, and even
>       return 0 in case of a timeout. This means that the loop could
>       have read data in several iterations and still reset the loop
>       index to 0.

The USB IC has a transfer queue and it's not possible to request more  
bytes than present in the queue. I guess it was a kind of synchronisation  
trick though I'm not sure it was useful.

Sorry for the trouble,
David