[PATCH] Revisions requested

[Jef Driesen]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-time.patch
Type: text/x-patch
Size: 3014 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141116/0710259e/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-libs.patch
Type: text/x-patch
Size: 670 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141116/0710259e/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-symbols.patch
Type: text/x-patch
Size: 916 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141116/0710259e/attachment-0002.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: clang.log
Type: text/x-log
Size: 2803 bytes
Desc: not available
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20141116/0710259e/attachment-0003.bin>