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

[John Van Ostrand]

On Sun, Oct 26, 2014 at 3:08 AM, Jef Driesen <jef at libdivecomputer.org>
wrote:

> 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.
>

My patch does use array_uint. There are a few cases where I do it manually
because two bytes of a uint16 are separated by bytes. I figured that a
union might have been a portable way to pack the structure without using
the _atribute__((packed)).

I've made changes to remove the structure. It's in the new patch.


>  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).
>
>
I've done something like that. The buffer contains a pointer array in the
first 50 bytes to the various data sections that follow.  It's in the new
patch.


>  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.)
>

I've collapsed the two into one backend while keeping the two devices in
the list.

>
>  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.
>

I was partly going on the description from the Cochran manual:

"Inter-Dive Events: Number of Initializations, Unit Activation,
Altitude Changes of 500 Feet, Temperature Changes of 10 degrees,
Low Batteries, Sensor Malfunction, Analyst® interface with Dive
Computer."

I did some testing and it seems that data downloads aren't logged. I've
seen logs after using Analyst but it may be when configuration changes are
made to the DC.

I also did some testing and was able to verify that a close/open wasn't
needed between big data downloads. I still need to do some serial
re-configuration though. I've removed the extraneous closes.

>
> 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.
>
>
I would have thought the same thing. I've worked with serial configuration
(and only a little serial programming) for two decades so I understand the
protocol from a practical perspective. I can't explain why flushes are
needed but they are, perhaps it has something to do with firmware on the
FTDI side. it's not merely make-work wait time. The DC, when woken up after
prompted by several flushes, starts issuing what I call a heartbeat byte.
The DC only recognizes commands after it emits one of these bytes. So my
first attempt (as well as others) was to simply wait for this byte, and it
never came. I've been able to wake up the DC by sending a byte but I'm wary
to rely on that since it might become part of the next command if the DC
just happens to be in the mood. The Analyst software uses flushes to wake
up the DC.

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
>

The patch was sent to the list prior to this email. Let me know what you
think.


-- 
John Van Ostrand
At large on sabbatical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141027/5b3d6c50/attachment-0001.html>