[PATCH] Added support for Cochran EMC and Commander Air import

[Jef Driesen]

On 22-10-14 17:33, John Van Ostrand wrote:
> On Wed, Oct 22, 2014 at 3:23 AM, Jef Driesen <jef at libdivecomputer.org>
> wrote:
>
>> On 2014-10-07 18:09, John Van Ostrand wrote:
>> Now, there are a couple of issues that needs to be addressed before this
>> can be included in libdivecomputer:
>>
>> You used a few Linux specific calls such as nanosleep() in your code. This
>> obviously breaks the windows builds. The rule of thumb is that the dive
>> computer backends should contain no platform specific code at all. All
>> platform specific code should be moved to the platform specific modules.
>> For example we already have a serial_sleep() function.
>
> Those I should be able to change to serial_sleep.

Yeah, that's pretty straightforward.

>> A related issue is that you used packed structures, which are a gcc
>> extension. Although, I personally use mingw (gcc) for my Windows builds,
>> the msvc compiler is supported too. Casting the raw data to a structure for
>> easier parsing is non-portable anyway (e.g. little vs big endian).
>> Therefore, in libdivecomputer we always de-serialize the data in a portable
>> way using the array_uint{16,24,32}_{le,be} functions. During development
>> the structures are indeed very convenient (I occasionally do that as well),
>> but for the final version they should be replaced with something more
>> portable.
>
> Those packed structures are the dive logs and I realize they have a lot
> more information than libdivecomputer needs but I wanted to save that
> research because it's been useful in debugging and decoding other model's
> logs and it may become useful in the future. You'll see that they are all
> char or char arrays that I either de-serialize in-line (sometimes for good
> reason) or use the array_uint* functions. I'd prefer to keep a structure in
> place rather than directly access select bytes. Would a union between a
> char array and the structure be a portable way of packing it? Would that be
> an option?

A union doesn't change the fact that casting a byte array to a structure is not 
strictly portable (alignment, endianness, etc). In practice it will most likely 
just work because you only used unsigned char's, but since we can't guarantee 
that, I prefer not to rely on that. The _attribute__((packed)) is a gcc 
extension, which is not supported by the msvc compiler, so you can't use that.

For anything longer than an 8 bit value, you'll need the array_uint helper 
functions anyway, so the only benefit of using the structure is that you don't 
need to calculate the byte offset manually.

>> Your dc_device_dump implementation uses a custom cochran_data_dump
>> structure instead of the dc_buffer_t structure. You either need to get rid
>> of this custom structure, or not implement this function. Right now if
>> anyone calls this function with dc_buffer_t structure, bad things will
>> happen. I guess this is also the reason why you added your own example
>> application instead of the generic example application. If really
>> necessary, you can implement and expose backend specific functions to
>> support features outside the generic api.
>>
>
> I should be able to refactor that function to use dc_buffer properly. You
> mentioned that the dump function was more for debugging or bypassing
> libdivecomputer's foreach functions so I was liberal with it. I still want
> to export all the data so how about I use dc_buffer directly, put pointers
> to the data sections in first and stack the data in after, one big blob.
> Maybe use a union to a structure to easily decode it.
>
> I wrote the cochran_download program because I wanted a way for a user to
> be able to download their data and send it to me easily. I assume there
> will be lots more work needed to support various DCs. The data produced can
> be used with a simulator I wrote.h

The dump function is indeed mainly for debugging purposes. If your 
implementation follows the expected pattern (e.g. returning a proper 
dc_buffer_t) you wouldn't need a custom application.

If you pack all the pieces in one blob, that's fine. Based on your code most 
pieces have a fixed size, so you can just append them. For the variable length 
ones, you'll need a small header with at least the size. Just make sure you 
serialize the data in a portable way (for example 32bit big endian integers for 
those lengths).

>> You implemented two backends. One for the EMC and one for the Commander.
>> But the communication protocol is roughly identical, and you already handle
>> the differences in the common code. That means you only need a single
>> backend, which supports multiple models. This is very common in other
>> backends too. Just name your backend after one of them (for example
>> DC_FAMILY_COCHRAN_EMC).
>>
>
> I thought that being very specific about model was important in not leading
> users to believe their computer was supported. There are significant
> differences between models and possibly within a model that has different
> features enabled that would cause the communication or decoding to fail. I
> have access for four different models with 3 purchased recently which means
> I don't have any duplicates to compare, duplicate models with different
> features enable to compare, and I can't tell if things change within a
> model over time.
>
> A little background may help in this point and the next. I chatted with
> Mike Cochran who refused his company's help in decoding because he was
> concerned that it would leak hints about the decompression algorithm. He
> also warned that others have bricked their DC trying to communicate with it
> and even admitted their programmers have bricked DCs while developing their
> Windows Analyst dive log software. I chose to take that quite seriously.
>
> The code is extremely conservative in identifying DCs. Right now it uses a
> 6 digit model string (e.g. AM2315) which I suspect not only varies between
> models but varies within a model based on what features are enabled (e.g.
> more memory, additional gases) and possible based on which microcontroller
> is used (perhaps it changes over the life of a model.)
>
> For example the following seem to be specific between models and depending
> on which features are enabled.
>      Data format (ie log, samples)
>      Baud rate
>      Start and End memory addresses (needed when the log "wraps" around.
>
> I've determined that byte 3 and 4 of that model string indicate the data
> format but I have no way of determining the baud rate or memory range.
>
> I'm also working on a third model except all I have is data.
>
> Does this change your view on this?

I certainly understand your concern about being careful. That's something I have 
been doing for the past years too. As far as I know libdivecomputer has never 
bricked any dive computer, and I certainly would like to keep it that way!

But that's not a reason not to use a single backend. One backend basically means 
one communication protocol. And that doesn't seems to be the case here. You have 
several different models that share the same communication protocol. As you 
explained above, you can identify the actual model from the data, so I see no 
valid reason to split this into different backends. If you are concerned that 
your code won't work correctly on models you haven't tested yet, then it's 
perfectly acceptable, to fail with DC_STATUS_UNSUPPORTED once you detect a model 
you don't know how to handle.

[That's pretty much the same situation as with the oceanics. If we encounter an 
unknown model, we fallback to the default model. In most cases that's just 
wrong, and downloading or parsing will fail. Libdivecomputer needs to be updated 
to learn the correct settings for each new model. The only difference here is 
that we don't fail but fallback to a default, because it doesn't harm the 
device, and usually allows us to get at least some data of the device.]

Also notice how the dc_device_open() function receives the device descriptor, 
which contains the model number. So if necessary, you know the requested model 
before starting the communication. Most backends ignore this model number, 
because they detect the model from the data. This autodetection is what makes it 
possible to select any model from the same family, and downloading will just 
work. If possible, I prefer doing autodetection, but if that's not possible or 
too risky, you can assume the model selected by the user. (If the user selects 
the wrong model, there is not much we can do about that.)

>> During the communication you close and re-open the serial port. Are you
>> sure this is really necessary? What kind of interface does the Cochran use?
>> A usb-serial chip (prolific, ftdi, etc)? Maybe you just need the right
>> trick like toggling some serial lines to restart the communication? The
>> reason why I'm asking is that in future versions, opening the serial port
>> will likely be moved to the application side, and in that case re-opening
>> the port will no longer be possible.
>>
>
> You may recall me posting about communication in the past. The cable uses
> an end-of-life FTDI chip to communicate with the three contacts on the DC
> (3-wire serial?) The cochran DCs seem tricky. They initially take commands
> at 9600 baud and they deliver results for small data transfers at the same
> rate, but for logbook and sample data they operate at what I suspect is
> their microcontroller's full speed, so I have unusual baud rates like
> 825,000 baud.
>
> Their Analyst software always downloads the log data as a whole, something
> I suspect isn't necessary and sample data can be quite big (3 bytes every
> second) but I decided, based on Mr. Cochran's warning, that I should mimic
> their windows software closely. That means I open and close the connection.
>
> I could revisit that code and see if I can remove the close/open to see if
> it works but I'd still need to do the baud change and there is a need for
> several flushes to wake the DC up. The DC also logs computer connections
> and if those logs looked unusual users might find Cochran contesting
> warranty claims. I don't know that close/open is logged, I was being
> careful.

I might be wrong on this, but I doubt the device can detect when an application 
opens the serial port. With true serial ports (and no handshaking) you can 
easily send data out, without anything connected to the port. And with 
usb-serial there is already a lot of communication going on before an 
application can even open the virtual serial port (usb device enumeration for 
example). Of course, once you're sending data, or changing serial lines the 
device will detect that.

In theory it makes absolutely no sense having to flush multiple times, or 
setting the same settings more than once. Most likely, the device just needs 
more time to finish whatever it was doing. So waiting a few (or more) 
milliseconds will work equally well. I have seen that open/close sequence before 
in other applications, and as I suspected it turned out to be unnecessary. I 
just needed the right amount of waiting time combined with the correct 
initialization sequence. I'm not saying that's the case here, but I suspect it is.

Note that opening/closing a serial port has a race condition. Between your open 
and close, some other application might open the port. Very unlikely in 
practice, but not impossible.

> Will your plans to move the open() to the application side allow for baud
> rate changes that are needed by the Cochran?

Only opening the serial port will move to the application side. Everything else, 
like setting the correct baudrate, will remain the responsibility of 
libdivecomputer. So switching baudrates won't be a problem.

Jef