On 22-08-14 11:58, Venkatesh Shukla wrote:
Libdivecomputer - As considerable number of the divecomputers have ftdi chipsets
in them, the first step was support of libdivecomputer for ftdi chipsets on
android. As tty interface is not available for android, FTDI chipset specific
script have been developed that uses libftdi for communication. This libftdi in
turn uses libusb for USB communication.
serial_ftdi.c - This script is almost complete. Anton has found a crucial bug in
this script because of which a deadlock condition is met during serial_read
operations on android. The deadlock does not appear always and has now been
overcome by timing out in this condition. This needs further examination. I have
not been able to find a reason for this yet.
I finally had some time for a detailed review of your ftdi code. I'm sorry it
took so long.
Let's start with some general comments. First of all, as explained in a previous
email, proper integration of your work will require some design changes on the
libdivecomputer. The modifications you made to the dive computer backends are
fine for prototyping, but are not acceptable for the official libdivecomputer
code. But that's not something you can do about. Another thing your code needs
is a whitespace cleanup. Libdivecomputer code uses only tabs for indentation,
not spaces.
The serial_ftdi.c code will also require some work:
1. Use hex values for constants (e.g. MODEM_DCD). Binary constants are not
portable.
2. Rename the ftdi_ctx member in serial_t to just "handle" or "ftdi". There is
already a context member, which is only confusing.
3. Do we need to call ftdi_init after ftdi_new?
4. Do we need to call ftdi_usb_close before ftdi_free?
5. Call ftdi_setflowctrl just once, like you already do for the other
attributes.
6. I think the input/output arguments to ftdi_{read,write}_data_set_chunksize
functions should be swapped. But I'm not sure this function does what it should
do. Read the description of the Windows SetupComm function. Anyway, this is not
really critical because this function isn't used anywhere.
7. The half-duplex emulation does not change the device in half-duplex mode.
It's basically a hack for two wire interfaces. It tries to wait just long enough
after each write, such that the next read doesn't start before the data is
transmitted. See commit b6d24e72e263dc6a374a87df98a0321d390e42f0 for more details.
8. Clip the latency values to 1 and 255 instead of returning an error. This is a
best effort interface only. A value of zero basically means as low as possible.
See the linux implementation.
9. Get rid of the static variable "backoff" in serial_read. Global variables are
not thread-safe and thus not acceptable. If the value needs to persist between
calls, the serial_t struct is the place where it belongs.
10. Where is the timeout handling in serial_read? This is a critical bug.
Libdivecomputer relies on correct timeouts to avoid blocking forever. Some dive
computer protocols have also very strict timing requirements. At first sight,
the libftdi doesn't support timeouts very well, so that might be a serious problem.
11. The serial_send_break function isn't used, but the serial_set_break is. So
ideally this should be implemented too. I think you can use
ftdi_set_line_property2 for this purpose.
12. For the serial_get_received function you access the internals of the ftdi
context. This is usually bad practice. Maybe there is an alternative solution
here? I'm not even sure the current implementation will work, because there
might be data waiting in the hardware buffer too. Note that this function is
required by several dive computer backends, for polling or determine the optimal
packet size. For polling, the exact number of bytes isn't relevant, because we
only need to know whether data is available or not. For the packet size, this is
less critical and always returning zero would be acceptable, because there is
always a fallback to the default size.
For the build systems changes, I also have a few comments and questions. In the
configure.ac and Makefile.am you check and conditionally compile for Android.
But the ftdi backend may also be useful on non-Android systems. I think that in
the long term we'll want to support multiple serial backends, and then you
should conditionally compile on the presence of libftdi, not Android. For now a
with --enable-ftdi option might be the interim solution? Then you can also build
the ftdi backend for non-Android platforms.
On my ubuntu system I don't have a libftdi1 package, only libftdi (notice the
absence of the 1 at the end). This is something that may need some autodetection.
Jef
_______________________________________________
subsurface mailing list
[email protected]
http://lists.hohndel.org/cgi-bin/mailman/listinfo/subsurface
