[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.
John Van Ostrand
john at vanostrand.com
Fri Jun 2 09:26:16 PDT 2017
On Fri, Jun 2, 2017 at 5:18 AM, Jef Driesen <jef at libdivecomputer.org> wrote:
> John,
>
> 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.
>
>
> I wouldn't mind a more detailed explanation either.
>
> Some more comments below.
>
> On 2017-06-01 01:24, John Van Ostrand wrote:
>
>> +typedef enum cochran_profile_size_t {
>> + PROFILE_FULL_SIZE,
>> + PROFILE_READ_SIZE,
>> +} cochran_profile_size_t;
>>
>
> Just a matter of style, but I prefer to use a common prefix, like this:
>
> typedef enum cochran_profile_size_t {
> PROFILE_SIZE_FULL,
> PROFILE_SIZE_READ,
> } cochran_profile_size_t;
Done and done.
>
>
> +static int
>> +cochran_commander_profile_size(cochran_commander_device_t *device,
>> cochran_data_t *data, int dive_num, cochran_profile_size_t type)
>>
>
> Can this function ever return a negative value? Since it's a size, I guess
> the answer is no. In that case use an unsigned int. Makes it easier to read
> the code because you immediately know the value can never be negative!
>
> (Obviously the code where the return value is used needs to be adjusted as
> well.)
Also done.
>
>
> + const unsigned char *log_entry = data->logbook + dive_num *
>> device->layout->rb_logbook_entry_size;
>> + unsigned int sample_start_address = -1;
>>
>
> If you want to initialize the value to 0xFFFFFFFF, use that and not -1.
Done.
>
>
> + // Calculate the size of the profile only
>> + int sample_size = sample_end_address - sample_start_address;
>> +
>> + if (sample_size < 0)
>> + // Adjust for ring buffer wrap-around
>> + sample_size += device->layout->rb_profile_end -
>> device->layout->rb_profile_begin;
>> +
>> + return sample_size;
>>
>
> You can easily avoid the signed integer here. Even better is to use the
> ringbuffer_distance function:
>
> ringbuffer_distance(sample_start_address, sample_end_address, 0,
> device->layout->rb_profile_begin, device->layout->rb_profile_end)
>
> One of the advantages is that it contains asserts, which forces you to
> validate the pointers before using them. That's a good thing!
Cool. I'll look for other places to use ringbuffer functions.
>
> +/*
>> + * Do several things. Find the log that matches the fingerprint,
>> + * calculate the total read size for progress indicator,
>> + * Determine the most recent dive without profile data.
>> + */
>> +
>> +static int
>> cochran_commander_find_fingerprint(cochran_commander_device_t
>> *device, cochran_data_t *data)
>>
>
> Same comment about signed/unsigned return value here.
Done.
>
>
> + // Determine if sample exists
>> + if (profile_capacity_remaining > 0) {
>> + // Subtract this dive's profile size including
>> post-dive events
>> + profile_capacity_remaining -= sample_size;
>> + if (profile_capacity_remaining < 0) {
>> + // Save the last dive that is missing
>> profile data
>> + data->invalid_profile_dive_num = i;
>> + }
>> + // Accumulate read size for progress bar
>> + sample_read_size += read_size;
>> + }
>> +
>> + if (profile_capacity_remaining < 0) {
>> + // There is no profile for this dive
>> + sample_size = 0;
>> + }
>>
>
> This if statement can be removed. It doesn't have any effect. You set a
> local variable, which goes out of scope immediately after.
Done.
>
>
> @@ -772,9 +791,64 @@ cochran_commander_device_foreach (dc_device_t
>> *abstract, dc_dive_callback_t call
>> cochran_data_t data;
>> data.logbook = NULL;
>> data.sample = NULL;
>>
>
> The data.sample field is no longer used for anything. Remove it!
Good catch.
>
>
> + // Allocate space for log book.
>> + data.logbook = (unsigned char *) malloc(data.logbook_size);
>> + if (data.logbook == NULL) {
>> + ERROR (abstract->context, "Failed to allocate memory.");
>> + return DC_STATUS_NOMEMORY;
>> + }
>> +
>> + // Request log book
>> + rc = cochran_commander_read(device, &progress, 0, data.logbook,
>> data.logbook_size);
>> + if (rc != DC_STATUS_SUCCESS)
>> + return rc;
>>
>
> There is a memory leak here, because you are not freeing data.logbook!
Fixed.
>
>
> unsigned int dive_count = 0;
>> if (data.dive_count < device->layout->rb_logbook_entry_count)
>> dive_count = data.dive_count;
>> @@ -808,44 +879,11 @@ cochran_commander_device_foreach (dc_device_t
>> *abstract, dc_dive_callback_t call
>> unsigned int sample_start_address = array_uint32_le
>> (log_entry +
>> device->layout->pt_profile_begin);
>> unsigned int sample_end_address = array_uint32_le
>> (log_entry +
>> device->layout->pt_profile_end);
>>
>> - // 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.
>
>
> + 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.
>
>
> + 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.
>
> Jef
>
--
John Van Ostrand
At large on sabbatical
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://libdivecomputer.org/pipermail/devel/attachments/20170602/e7a6738b/attachment.html>
More information about the devel
mailing list