<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Sun, Nov 2, 2014 at 1:13 PM, 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">John,<br>
<br>
Nice work. This is already a huge step in the right direction, but we're not there yet.<br>
<br>
There is a problem in the way you store the cochran_data_t structure in the device handle. Most of this info only used by the dump/foreach function (or one of its helpers), so it doesn't really belong inside the handle. </blockquote><div><br><br></div><div>The structure members under "// Config items" are used to allow for different Cochran models and different feature configurations for a given model. The fields are set during cochran_set_device_config() so that I know how to handle reading the logbook and the sample data. So while I'm detecting the model I thought I might as well set the variables needed in foreach so I have one section of code to look at when adding a new model.<br><br></div><div>Some of the variables I need to keep between calls to foreach and this seemed to be a good place.<br><br></div><div>I know I'm keeping id*, config* and misc* around after they are needed. I could drop them sooner but they come in handy for the dump function.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This results in a number of problems. For example you allocate several pieces of data (id, config, etc) in those functions and store them in the handle. But if someone calls one of those functions a second time, you allocate them again and overwrite the original pointers. So you'll end up with a memory leak. You can free the old data first, but I think you're better off using local data structures because there is no reason why any of this data needs to persist between the high-level calls.<br>
<br></blockquote><div><br></div><div>I see, calling _open and _foreach a second time could leak memory. <br><br></div><div>Fixed.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Overall, I find the logic rather hard to follow because it's spread over all your cochran_read_* functions. These functions do not only download some data, but also sets some fields, which are then used in one of the other functions. I think it would be easier if the processing was a bit more separated from the downloading. I have the feeling that there's something wrong, but I can't really tell what or where. Maybe it's just that I don't understand the format very well. Do you have some data files available?<br></blockquote><div><br></div><div>There are two reasons I split the read up into several functions. The cochran_read_id() function reads the blocks needed to detect the DC model so it knows how to perform the other reads. I do this in the cochran_common_device_open() because I wanted to give immediate feedback to the application if the DC wasn't supported.<br><br>The other reads could be in one function but it would be a large function. There is some handle data settings in the read functions. <br><br></div><div>cochran_read_id(): determine model type and features<br></div><div>cochran_read_config(): determine the head pointer to the sample buffer, I see now that I don't use this.<br></div><div>cochran_read_misc(): read only to mimic Analyst<br>cochran_read_logbook(): Get number of logged dives(from config1), calculate logbook_size<br></div><div>cochran_read_parms(): Doesn't read anything, calculate parameters needed for reading samples.<br></div><div>cochran_read_samples(): Reads sample data.<br><br></div><div>There really isn't much happening inside the read calls that isn't needed there. The read_config() has a few lines that can actually be removed. The read_logbook has another few lines that calculate the size, and read_samples has two lines that calculate size. Since all the read_*() functions are called by read_all() it's trivial to move them there.<br><br></div><div>Done. Calculations are moved to the read_all function.<br></div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Note that several of those data pieces have a fixed size, so why even allocate them? Just use a byte array with the right size. Some like:<br>
<br>
struct {<br>
...<br>
unsigned char id0[67];<br>
unsigned char id1[67];<br>
unsigned char config1[512];<br>
unsigned char config2[512];<br>
...<br>
};<br>
<br>
Now you no longer have to worry about freeing them. Instead of config1, config2, etc you could even use an array here. That will make your code even more generic. Instead of repeating the same code, you can use a loop.<br>
<br></blockquote><div><br></div><div>I used mallocs to save memory. I can make them arrays. Done.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
BTW, I'm not sure what's in these config pages, but based on the name, I assume it contains settings. So this is something applications may want to access directly. I suspect there the protocol also support changing those settings. Have a look at the hw_ostc_device_eeprom_{read,<u></u>write}. (This is certainly not a must have feature right now, but it's something you may want to keep in mind already.)<br>
<br></blockquote><div><br></div><div>Yes, that is device configuration as well as current calculated tissue loads. I didn't look at the OSTC code so I didn't see any evidence of setting configs on the DCs. I intend to write code to do just that for the Cochran and I wasn't sure where to do that since it looked like libdivecomputer doesn't expose an API for it. There are some settings, like date/time that aren't configurable on the DC. And I would like to be able to set configuration from Linux or even a tablet.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
You have the same problem with the progress field. Again, I think it makes more sense to make this a local variable, and pass it as a parameter to the internal read function. Note that you're also reporting progress for each individual step, rather than the entire download.<br>
<br></blockquote><div> </div><div>I don't know the size of the entire download when I start needing a progress bar. You'll note I don't have one for cochran_read_id(), read_config() or read_misc(). These are small enough I figured one wasn't needed. I don't know the size of the sample data until I read the logbook. So it seemed the best approach was to have two.<br><br></div><div>The other backends I looked at read the from the DC in blocks of data and were able to manage the progress within the function. The cochran does very big reads at very high data rates (800Kb/s). So I can't do the progress at the cochran_common_device_read() function because it issues one command to read a huge block of data. So I did it in the cochran_packet() function. Since that is exposed I couldn't add a parameter without breaking the API. I could add a separate packet function that accepts a progress but this seems to be a better approach.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Those high baudrates are indeed a bit odd. Do they even work? You don't check the return value of the serial_configure() function. So if the underlying driver (or OS) doesn't support it, unexpected results may happen. I implemented the support for non-standard baudrates long time ago, and unfortunately it's not guaranteed to work everywhere. Initially it was needed for the iconhd backend, in the end it turned out that this baudrate was a bug in the mares application. The iconhd uses some microchip with CDC-ACM, where the baudrate doesn't matter at all. So every value works equally well.<span class=""><br></span></blockquote><div><br></div><div>They are little twitchy but I get good reads most of the time. I've tried different rates and it does require a non-standard rate. At least I'm not aware of a standard rate that high.<br><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
On 28-10-14 01:18, John Van Ostrand wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Removed dependency on packed structure.<br>
</blockquote>
<br></span>
Nice. I noticed that you introduced CMD_* and EMC_* constants to keep the code readable. An alternative I have used in some backends, is to have a layout structure containing those offsets. And then you define one such structure for each model, and store a pointer to the correct table in the device/parser handle. This allows you to write code like this:<br>
<br>
value = array_uint16_le (log + layout->xxx);<br>
<br>
You can see this for example in the hw_ostc_parser.c file. The nice thing is that you can re-use the same code for all models. Adding a new model is just a matter of adding a new structure.<br>
<br>
I'm not saying you can or must use this technique in your backend, but it's maybe something to consider.<br></blockquote><div><br></div><div>That's a good idea.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Removed extraneous close/opens.<br>
</blockquote>
<br>
I'm glad those are not necessary after all.<span class=""><br>
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Formated lines to 78 chars.<br>
</blockquote>
<br></span>
This wasn't needed. I have no strict limits on the line length. Of course you should try to keep the line length reasonable, but if you have a longer one once in a while, I won't complain. Artificially limiting the lines to 78 chars makes the code only less readable. So please don't do that.<br>
<br>
For example some of those long lines are because you have assignments in if conditions:<span class=""><br>
<br>
if ((rc = cochran_common_serial_open(<u></u>device, context))<br>
!= DC_STATUS_SUCCESS)<br></span>
return rc;<br>
<br>
replace that with:<br>
<br>
rc = cochran_common_serial_open(<u></u>device, context);<span class=""><br>
if (rc != DC_STATUS_SUCCESS)<br>
return rc;<br>
<br></span>
Much more readable and shorter too!<span class=""><br></span></blockquote><div><br></div><div>Fixed.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Merged EMC and Commander backends to common.<br>
</blockquote>
<br></span>
Great. Just a few issues remaining here.<br>
<br>
You named your backend DC_FAMILY_COCHRAN and the corresponding functions cochran_common_{device,parser}<u></u>_*. Please name them after one specific model. For example DC_FAMILY_COCHRAN_COMMANDER and cochran_commander_{device,<u></u>parser}_*, etc. That's because in the future, there might be more than one cochran backend. The backend name is purely internal (except for the family name), so it doesn't really matter that the commander backend also supports the emc.<br>
<br>
For the public header files, make one cochran_commander.h file containing the open and create functions (e.g. your current cochran.h file), and then a new cochran.h file that just includes this file. See the existing files for examples.<span class=""><br></span></blockquote><div><br></div><div>Done.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Change nanosleep() calls to serial_sleep().<br>
</blockquote>
<br></span>
We're getting closer, but the windows build is still broken:<br>
<br>
CC cochran_common_parser.lo<br>
In file included from ../../source/src/cochran_<u></u>common_parser.c:33:<br>
../../source/src/cochran_<u></u>common.h:73: error: expected specifier-qualifier-list before 'time_t'<br>
../../source/src/cochran_<u></u>common_parser.c: In function 'cochran_common_parser_get_<u></u>datetime':<br>
../../source/src/cochran_<u></u>common_parser.c:113: error: 'cochran_data_t' has no member named 'date_format'<br>
../../source/src/cochran_<u></u>common_parser.c: In function 'cochran_common_parser_get_<u></u>field':<br>
../../source/src/cochran_<u></u>common_parser.c:147: warning: control reaches end of non-void function<br>
../../source/src/cochran_<u></u>common_parser.c: In function 'cochran_common_parser_<u></u>samples_foreach':<br>
../../source/src/cochran_<u></u>common_parser.c:166: warning: control reaches end of non-void function<br>
../../source/src/cochran_<u></u>common_parser.c: In function 'cochran_common_handle_event':<br>
../../source/src/cochran_<u></u>common_parser.c:175: warning: unused variable 'data'<br>
make[1]: *** [cochran_common_parser.lo] Error 1<br>
<br>
I think you're missing an #include <time.h> somewhere for the use of time_t in the cochran_data_t structure. But you shouldn't need that because libdivecomputer has its own time functions and structures. Use those instead.<span class=""><br></span></blockquote><div><br></div><div>I think I've fixed that.<br> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Change cochran_common_dump to use dc_buffer properly.<br>
</blockquote>
<br></span>
I still need to look at the details of your new dump implementation. One thing I did notice is that you forgot to remove the cochran_data_dump from the public header :-)<br></blockquote><div><br></div><div>Done.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
That's it for now.<span class="HOEnZb"><font color="#888888"><br>
<br>
Jef<br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><div dir="ltr"><div>John Van Ostrand<br></div><div>At large on sabbatical<br></div><br></div></div>
</div></div>