<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Oct 26, 2014 at 3:08 AM, Jef Driesen <span dir="ltr"><<a href="mailto:jef@libdivecomputer.org" target="_blank">jef@libdivecomputer.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">On 22-10-14 17:33, John Van Ostrand wrote:<br>
</span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
On Wed, Oct 22, 2014 at 3:23 AM, Jef Driesen <<a href="mailto:jef@libdivecomputer.org" target="_blank">jef@libdivecomputer.org</a>><br>
wrote:<br>
<br>
</span><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
On 2014-10-07 18:09, John Van Ostrand wrote:<br></span><span class="">
Now, there are a couple of issues that needs to be addressed before this<br>
can be included in libdivecomputer:<br>
<br>
You used a few Linux specific calls such as nanosleep() in your code. This<br>
obviously breaks the windows builds. The rule of thumb is that the dive<br>
computer backends should contain no platform specific code at all. All<br>
platform specific code should be moved to the platform specific modules.<br>
For example we already have a serial_sleep() function.<br>
</span></blockquote><span class="">
<br>
Those I should be able to change to serial_sleep.<br>
</span></blockquote>
<br>
Yeah, that's pretty straightforward.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
A related issue is that you used packed structures, which are a gcc<br>
extension. Although, I personally use mingw (gcc) for my Windows builds,<br>
the msvc compiler is supported too. Casting the raw data to a structure for<br>
easier parsing is non-portable anyway (e.g. little vs big endian).<br>
Therefore, in libdivecomputer we always de-serialize the data in a portable<br>
way using the array_uint{16,24,32}_{le,be} functions. During development<br>
the structures are indeed very convenient (I occasionally do that as well),<br>
but for the final version they should be replaced with something more<br>
portable.<br>
</blockquote>
<br>
Those packed structures are the dive logs and I realize they have a lot<br>
more information than libdivecomputer needs but I wanted to save that<br>
research because it's been useful in debugging and decoding other model's<br>
logs and it may become useful in the future. You'll see that they are all<br>
char or char arrays that I either de-serialize in-line (sometimes for good<br>
reason) or use the array_uint* functions. I'd prefer to keep a structure in<br>
place rather than directly access select bytes. Would a union between a<br>
char array and the structure be a portable way of packing it? Would that be<br>
an option?<br>
</blockquote>
<br></span>
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.<br>
<br>
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.<span class=""><br></span></blockquote><div><br></div><div>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)).<br><br></div><div>I've made changes to remove the structure. It's in the new patch.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Your dc_device_dump implementation uses a custom cochran_data_dump<br>
structure instead of the dc_buffer_t structure. You either need to get rid<br>
of this custom structure, or not implement this function. Right now if<br>
anyone calls this function with dc_buffer_t structure, bad things will<br>
happen. I guess this is also the reason why you added your own example<br>
application instead of the generic example application. If really<br>
necessary, you can implement and expose backend specific functions to<br>
support features outside the generic api.<br>
<br>
</blockquote>
<br>
I should be able to refactor that function to use dc_buffer properly. You<br>
mentioned that the dump function was more for debugging or bypassing<br>
libdivecomputer's foreach functions so I was liberal with it. I still want<br>
to export all the data so how about I use dc_buffer directly, put pointers<br>
to the data sections in first and stack the data in after, one big blob.<br>
Maybe use a union to a structure to easily decode it.<br>
<br>
I wrote the cochran_download program because I wanted a way for a user to<br>
be able to download their data and send it to me easily. I assume there<br>
will be lots more work needed to support various DCs. The data produced can<br>
be used with a simulator I wrote.h<br>
</blockquote>
<br></span>
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.<br>
<br>
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).<div><div class="h5"><br></div></div></blockquote><div><br></div><div>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.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div class="h5">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
You implemented two backends. One for the EMC and one for the Commander.<br>
But the communication protocol is roughly identical, and you already handle<br>
the differences in the common code. That means you only need a single<br>
backend, which supports multiple models. This is very common in other<br>
backends too. Just name your backend after one of them (for example<br>
DC_FAMILY_COCHRAN_EMC).<br>
<br>
</blockquote>
<br>
I thought that being very specific about model was important in not leading<br>
users to believe their computer was supported. There are significant<br>
differences between models and possibly within a model that has different<br>
features enabled that would cause the communication or decoding to fail. I<br>
have access for four different models with 3 purchased recently which means<br>
I don't have any duplicates to compare, duplicate models with different<br>
features enable to compare, and I can't tell if things change within a<br>
model over time.<br>
<br>
A little background may help in this point and the next. I chatted with<br>
Mike Cochran who refused his company's help in decoding because he was<br>
concerned that it would leak hints about the decompression algorithm. He<br>
also warned that others have bricked their DC trying to communicate with it<br>
and even admitted their programmers have bricked DCs while developing their<br>
Windows Analyst dive log software. I chose to take that quite seriously.<br>
<br>
The code is extremely conservative in identifying DCs. Right now it uses a<br>
6 digit model string (e.g. AM2315) which I suspect not only varies between<br>
models but varies within a model based on what features are enabled (e.g.<br>
more memory, additional gases) and possible based on which microcontroller<br>
is used (perhaps it changes over the life of a model.)<br>
<br>
For example the following seem to be specific between models and depending<br>
on which features are enabled.<br>
     Data format (ie log, samples)<br>
     Baud rate<br>
     Start and End memory addresses (needed when the log "wraps" around.<br>
<br>
I've determined that byte 3 and 4 of that model string indicate the data<br>
format but I have no way of determining the baud rate or memory range.<br>
<br>
I'm also working on a third model except all I have is data.<br>
<br>
Does this change your view on this?<br>
</blockquote>
<br></div></div>
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!<br>
<br>
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.<br>
<br>
[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.]<br>
<br>
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.)<span class=""><br></span></blockquote><div><br></div><div>I've collapsed the two into one backend while keeping the two devices in the list. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="">
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
During the communication you close and re-open the serial port. Are you<br>
sure this is really necessary? What kind of interface does the Cochran use?<br>
A usb-serial chip (prolific, ftdi, etc)? Maybe you just need the right<br>
trick like toggling some serial lines to restart the communication? The<br>
reason why I'm asking is that in future versions, opening the serial port<br>
will likely be moved to the application side, and in that case re-opening<br>
the port will no longer be possible.<br>
<br>
</blockquote>
<br>
You may recall me posting about communication in the past. The cable uses<br>
an end-of-life FTDI chip to communicate with the three contacts on the DC<br>
(3-wire serial?) The cochran DCs seem tricky. They initially take commands<br>
at 9600 baud and they deliver results for small data transfers at the same<br>
rate, but for logbook and sample data they operate at what I suspect is<br>
their microcontroller's full speed, so I have unusual baud rates like<br>
825,000 baud.<br>
<br>
Their Analyst software always downloads the log data as a whole, something<br>
I suspect isn't necessary and sample data can be quite big (3 bytes every<br>
second) but I decided, based on Mr. Cochran's warning, that I should mimic<br>
their windows software closely. That means I open and close the connection.<br>
<br>
I could revisit that code and see if I can remove the close/open to see if<br>
it works but I'd still need to do the baud change and there is a need for<br>
several flushes to wake the DC up. The DC also logs computer connections<br>
and if those logs looked unusual users might find Cochran contesting<br>
warranty claims. I don't know that close/open is logged, I was being<br>
careful.<br>
</blockquote>
<br></span>
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.<br></blockquote><div><br></div><div>I was partly going on the description from the Cochran manual:<br><br>"Inter-Dive Events: Number of Initializations, Unit Activation,<br>Altitude Changes of 500 Feet, Temperature Changes of 10 degrees,<br>Low Batteries, Sensor Malfunction, Analyst® interface with Dive<br>Computer."<br><br></div><div>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.<br><br></div><div>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. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
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.<br>
<br></blockquote><div><br></div><div>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.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
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.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Will your plans to move the open() to the application side allow for baud<br>
rate changes that are needed by the Cochran?<br>
</blockquote>
<br></span>
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.<span class=""><font color="#888888"><br>
<br>
Jef<br>
</font></span></blockquote></div><br></div><div class="gmail_extra">The patch was sent to the list prior to this email. Let me know what you think.<br><br clear="all"></div><div class="gmail_extra"><br>-- <br><div dir="ltr"><div>John Van Ostrand<br></div><div>At large on sabbatical<br></div><br></div>
</div></div>