John,
There is a problem with how you pass the data to the dive callback function. I didn't notice this the first time, but you pass a pointer to the cochran_data_t structure to the callback function. That's not how this is supposed to work. You need to extract the raw binary data of each dive, and then pass that to the callback function. What you do now is pass the data of all dives, with a couple of fields pointing to the dive of interest. That's wrong and needs to change.
The cochran_data_t alone is already a problem here, because it contains pointers. The dive data is supposed to be a simple byte array which can be serialized as-is. Remember, libdivecomputer has absolutely no control over how or when the application uses the data. For example the application can copy the dives, and only parse them after the download has finished. In that case the device handle will already have been closed when parsing the data. Thus anything in your structure pointing to data in that handle will have been freed, and unexpected things will happen. With a self contained blob of binary data that's not a problem. You can copy it, write it to disk, etc with no problems.
See some more comments inline.
On 03-11-14 02:58, John Van Ostrand wrote:
On Sun, Nov 2, 2014 at 1:13 PM, Jef Driesen jef@libdivecomputer.org wrote:
Nice work. This is already a huge step in the right direction, but we're not there yet.
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.
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.
Some of the variables I need to keep between calls to foreach and this seemed to be a good place.
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.
It's fine to store certain data in the device handle if it's needed for the device connection state. For example the id packets are read during the open, and you need them later in the other functions. That's a perfect argument to store them in the device handle.
But nearly everything else is state for either the dump or foreach function. After those functions returns there is no need to keep any of the data around. If those functions get called a second time, they should start with a completely fresh state. Thus the "global" device handle is the wrong place to store this data. The fact that there are memory leaks when someone calls those functions twice is a symptom of that. If the state were private to the function, then you wouldn't have to worry about left overs from a previous call.
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.
I see, calling _open and _foreach a second time could leak memory.
Fixed.
It's better now, but freeing the pointers is just fixing the symptoms, and not the underlying problem (see above).
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?
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.
That's fine. I didn't had any problem with that.
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.
cochran_read_id(): determine model type and features cochran_read_config(): determine the head pointer to the sample buffer, I see now that I don't use this. cochran_read_misc(): read only to mimic Analyst cochran_read_logbook(): Get number of logged dives(from config1), calculate logbook_size cochran_read_parms(): Doesn't read anything, calculate parameters needed for reading samples. cochran_read_samples(): Reads sample data.
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.
Done. Calculations are moved to the read_all function.
I always try to keep the code for downloading separate from the parsing/processing. That usually makes it easier to follow the logic. So I think your latest version is a move in the right direction.
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:
struct { ... unsigned char id0[67]; unsigned char id1[67]; unsigned char config1[512]; unsigned char config2[512]; ... };
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.
I used mallocs to save memory. I can make them arrays. Done.
If you're allocating them anyway, you're not really saving anything :-)
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,write}. (This is certainly not a must have feature right now, but it's something you may want to keep in mind already.)
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.
There is indeed no generic api for writing settings, because this is done very different for different models. This is not a high priority, and something we can always look at later, when we have the basic downloading done. But I just mentioned it in case you were planning to implement this too.
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.
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.
There are a few other backends that have the same problem where the total size isn't known from the start. What I did there, is to initialize the maximum field with the worst-case maximum (and in some cases that's UINT_MAX, see EVENT_PROGRESS_INITIALIZER). As soon as you have a better value available, you update the maximum value. There are backends where we never know the exact amount of data until the download is over. In that case we simply can't do better than assuming the worst case maximum.
The main problem with two independent progress is that the user will see a progressbar that goes from 0% to 100% twice. From the user point of view, that's just very counter intuitive, because you don't know when the download will be finished. And that's exactly the point of the progress bar in the first place.
With the worst case maximum approach, you do get one progress for the entire download. Initially the progress bar will stay near zero, but once the real maximum is known, you go nicely to 100%.
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.
Look for example at the hw_ostc3_transfer() function. The progress structure is passed as a parameter there. A single progress structure for the entire download operation is used there, and the hw_ostc3_transfer() function just updates for one specific part of the download.
You can find a similar situation in the suunto_vyper_device_read_dive() function. This is also a public function, but there is a suunto_vyper_read_dive() helper function that does the actual work and takes the progress parameter. So you don't ever see the progress parameter on the public function.
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.
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.
What I meant here, is that because you don't check the return value of the serial_configure() function, you don't know whether setting the custom baudrate actually succeeded. Support for custom baudrates depends heavily on the OS and driver. So if the call fails, you'll be using a different baudrate then the one you think you are using.
For example on linux, setting a custom baudrate requires to set the baudrate to 38400, and then some linux specific call to change the baudrate to the custom baudrate. But if that call fails, then the actual baudrate will be 38400, and not your custom one.
We're getting closer, but the windows build is still broken:
CC cochran_common_parser.lo In file included from ../../source/src/cochran_common_parser.c:33: ../../source/src/cochran_common.h:73: error: expected specifier-qualifier-list before 'time_t' ../../source/src/cochran_common_parser.c: In function 'cochran_common_parser_get_datetime': ../../source/src/cochran_common_parser.c:113: error: 'cochran_data_t' has no member named 'date_format' ../../source/src/cochran_common_parser.c: In function 'cochran_common_parser_get_field': ../../source/src/cochran_common_parser.c:147: warning: control reaches end of non-void function ../../source/src/cochran_common_parser.c: In function 'cochran_common_parser_samples_foreach': ../../source/src/cochran_common_parser.c:166: warning: control reaches end of non-void function ../../source/src/cochran_common_parser.c: In function 'cochran_common_handle_event': ../../source/src/cochran_common_parser.c:175: warning: unused variable 'data' make[1]: *** [cochran_common_parser.lo] Error 1
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.
I think I've fixed that.
It still doesn't compile for me. However, the only place where you use something from time.h, is to set the current_dive_start_time field which isn't used anywhere else. Just remove this, and everything starts to work.
I also need to link with -lm on my system. The symbols file also needs a small update after the renaming. See the attached patches.
The clang static analyzer also finds some potential issues. See the attached logfile.
Jef