On 22-10-14 17:33, John Van Ostrand wrote:

On Wed, Oct 22, 2014 at 3:23 AM, Jef Driesen <jef@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