[PATCH] Cochran: Changed profile download to be incremental. It will result in a 30 minute download for full computers but it significantly reduces the time to download partial dives.

[Jef Driesen]

On 2017-06-02 18:26, John Van Ostrand wrote:
> On Fri, Jun 2, 2017 at 5:18 AM, Jef Driesen <jef at libdivecomputer.org> 
> wrote:
>> Can you reformat your commit message to follow the standard git 
>> format?
>> You can find a very nice description here:
>> 
>> https://chris.beams.io/posts/git-commit/
> 
> It's been a while since I used git and it became clear after doing a 
> couple
> commits that I forgot how commit messages appear.

No problem at all. It takes some learning.

Anyway, with git you can very easily modify commits afterwards (commit 
message and/or content) to submit an updated patch.

git rebase --interactive
git commit --amend

>>> -               // Validate
>>> -               if (sample_start_address < 
>>> device->layout->rb_profile_begin
>>> ||
>>> -                       sample_start_address >
>>> device->layout->rb_profile_end ||
>>> -                       sample_end_address <
>>> device->layout->rb_profile_begin ||
>>> -                       (sample_end_address >
>>> device->layout->rb_profile_end &&
>>> -                       sample_end_address != 0xFFFFFFFF)) {
>>> -                       continue;
>>> -               }
>>> 
>> 
>> Why did you remove the validation? Now you are using data that is
>> potentially bogus. That's a bad idea!
> 
> The cochran_commander_profile_size() function returns sample_size of 0 
> if
> these values are bad. If the sample_size is 0 we don't use the data.
> Although I notice that in one case it will alter sample_end_address, so 
> I
> added that to the foreach function too.

You are right. I didn't notice that, and that's exactly why I still 
don't like this construct. I prefer to do the validation right away. 
That way, the data is always safe to use. Now, you'll run into problems 
if there is a code path where the function isn't called.

BTW, I usually also prefer to report errors instead of silently ignoring 
them. It's might be a bit less convenient for the end-user that runs 
into the problem, but bug fixing becomes easier. Because that makes the 
difference between a bugreport saying "something looks wrong" versus "it 
fails with error X (at line Y in file Z)".

>> +                               do {
>>> +                                       rc = cochran_commander_read
>>> (device, &progress,
>>> sample_start_address, dive + device->layout->rb_logbook_entry_size,
>>> sample_size);
>>> +                               } while (rc != DC_STATUS_SUCCESS &&
>>> tries++ < 3);
>>> 
>> 
>> What's the reason for the retrying here? You don't do it elsewhere 
>> (for
>> example when reading the logbook, or from the 
>> cochran_commander_device_{read,dump}
>> functions), so I wonder what's different here.
> 
> Reading from the Cochran at the high baud rates isn't reliable. Very 
> large
> reads (multiple MB) result in errors about 1/3rd of the time. This is
> reason why I decided to read each dive profile individually. Small 
> reads
> are more reliable but when reading hundreds of times it's likely a read
> error will occur.

In that case you should integrate the retrying in the 
cochran_commander_read() function itself (or some extra helper 
function). Then you no longer have to duplicate the retrying code 
everywhere the function is being called.

>> +                               if (rc != DC_STATUS_SUCCESS) {
>>> +                                       ERROR (abstract->context, 
>>> "Failed
>>> to read the sample data.");
>>> +                                       return rc;
>>> 
>> 
>> Again a memory leak here! There are two similar ones a few lines 
>> below.
> 
> Fixed.

There is still memory leak left. The "dive" pointer isn't freed when you 
jump to the error handler.

Jef