[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]

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/

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;

> +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.)

> +	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.

> +	// 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!

> +/*
> + * 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.

> +		// 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.

> @@ -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!

> +	// 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!

>  	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!

> +				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.

> +				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.

Jef