libftdi Archives

Subject: RE: libftdi: Make ftdi_read_data() honor usb_read_timeout

From: "Michael Plante" <michael.plante@xxxxxxxxx>
To: <libftdi@xxxxxxxxxxxxxxxxxxxxxxx>
Date: Tue, 29 Mar 2011 08:56:57 -0400
Uwe Bonnes wrote:
>> But again, the behaviour is unexpected compared to all other read() calls
I know
>> of. How would you implement your requirement of reading variable length
>> response with low latency with a uc connected to a real serial port.
>> char buf[BIG];
>> int fd = open ("/dev/tty");
>> read (fd, buf, BIG);
>> wouldn't give you the result you expect libftdi() to work.
>> You would probably read single bytes until you have a sensible
>> response.

A serial port is character-oriented, and interrupts the PC after every
character.  USB is not.  I would not be able to do anything equivalent,
although I could do something vaguely similar by first select()ing, and then
by setting VMIN=1 and VTIME=inter-byte-timeout in termios.  So instead of
putting a timer on 62-byte chunks, there'd be one on 1-byte chunks.  But
that's unfortunately in deciseconds, so it's too coarse for me and I'd
probably not bother.  In contrast, ftdi_set_latency_timer() takes a value
from 1-255 milliseconds.

An analogy is to say you have a file on a slow block device (maybe a floppy
drive?) and call fread() with an overly-large buffer.  It will return at the
eof.  Now I'll grant there isn't a timeout, so perhaps it's not a great
analogy.  But just because USB is unique from other reads either of us can
think of, doesn't mean it's wrong.  This is, in any case, similar to the
behavior of libusb-0.1's and libusb-win32-0.1's read, which return if fewer
bytes than requested are returned (after accounting for splitting).


>> Working that way for your use case in libftdi already works, and with
>> the libftdi internal buffer getting filled with the first sucessfull
>> libusb_bulk_transfer() wouldn't produce system call overhead.

That's true.  It is an extra complication to have to make 2 or 3 calls
instead of one (the third would be a nonexistent accessor, so say 2), and
breaks existing code, but you're right that the overhead would be very
small.


>> So the same argument
>>    Michael> Ok, but that's not additional CPU overhead, it's additional
code complexity.
>> holds also for your use case...
>> That doesn't mean I understand your requirements, and libftdi should work
in a
>> sensible and _well documented_ way and in consense with as many users as
possible...

Fair enough.  Both the old and new behavior are allowed by the vague comment
at the top of the function.  But people likely figured out how it worked and
began using it that way, and this changes the existing behavior.  If the new
behavior were the original, then I would have had to do what was suggested
above:  read 1 byte, then read ftdi_context::readbuffer_remaining.  But the
code exists already, and so the new behavior, if it's truly beneficial,
should be given a new name.  It can certainly share a common static/private
function underneath to reduce duplication of lines of code.


>>     Michael> The FTDI device has the SI/WU pin.  How do you expect that
to
>>     Michael> function if you consider the current behavior "broken"?
>> The SI/WU pin functionality isn't impacted.

Could you elaborate?  What I expect is that libusb would send a
shorter-than-expected chunk to libftdi when that pin is strobed, and the new
libftdi would say "oh, it's still not enough.  read again."  So the patch
kills any expectation of immediate action by the application.  Where am I
misunderstanding?  (Incidentally, I am referring to the "SI" part, not the
"WU" part, of the name.)


>> What about
>> - returning when at least some data has been read
>> - or ftdi->usb_read_timeout has expired?
>>
>> At present we always return when the latency timer expires.

Are you sure?  Are we using the same terms?  The "latency timer" is internal
to the FTDI chip (ftdi_set_latency_timer()), and it essentially affects
whether a packet shorter than 64 bytes is returned.

Anyway, I would be ok with conditioning the old code on at least one byte
being present, rather than possibly returning zero bytes.  So "else if (ret
<= 2)" could be "else if (ret <= 2 && offset > 0)", if that's what you're
suggesting.  But I'm also okay with just splitting it into two different
calls.  I can't say for certain that change won't affect anything, but I
suspect it's considerably less likely to cause a problem.

Regards,
Michael


--
libftdi - see http://www.intra2net.com/en/developer/libftdi for details.
To unsubscribe send a mail to libftdi+unsubscribe@xxxxxxxxxxxxxxxxxxxxxxx   

Current Thread