Subsurface android downloader - Project Status

[Jef Driesen]

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