[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.
--- src/cochran_commander.c | 366 ++++++++++++++++++++++++++++-------------------- 1 file changed, 212 insertions(+), 154 deletions(-) diff --git a/src/cochran_commander.c b/src/cochran_commander.c index 83dd14a..4c15541 100644 --- a/src/cochran_commander.c +++ b/src/cochran_commander.c @@ -41,6 +41,11 @@ typedef enum cochran_endian_t { ENDIAN_BE, } cochran_endian_t; +typedef enum cochran_profile_size_t { + PROFILE_FULL_SIZE, + PROFILE_READ_SIZE, +} cochran_profile_size_t; + typedef struct cochran_commander_model_t { unsigned char id[2 + 1]; unsigned int model; @@ -53,6 +58,7 @@ typedef struct cochran_data_t { unsigned short int dive_count; int fp_dive_num; + int invalid_profile_dive_num; unsigned int logbook_size; @@ -430,23 +436,133 @@ cochran_commander_read (cochran_commander_device_t *device, dc_event_progress_t } -static void +/* + * For corrupt dives the end-of-samples pointer is 0xFFFFFFFF + * search for a reasonable size, e.g. using next dive start sample + * or end-of-samples to limit searching for recoverable samples + */ +static unsigned int +cochran_commander_guess_sample_end_address(cochran_commander_device_t *device, cochran_data_t *data, unsigned int log_num) +{ + const unsigned char *log_entry = data->logbook + device->layout->rb_logbook_entry_size * log_num; + + if (log_num == data->dive_count) + // Return next usable address from config page + return array_uint32_le(data->config + device->layout->rb_profile_end); + + // Next log's start address + return array_uint32_le(log_entry + device->layout->rb_logbook_entry_size + device->layout->pt_profile_begin); +} + + +static int +cochran_commander_profile_size(cochran_commander_device_t *device, cochran_data_t *data, int dive_num, cochran_profile_size_t type) +{ + + const unsigned char *log_entry = data->logbook + dive_num * device->layout->rb_logbook_entry_size; + unsigned int sample_start_address = -1; + unsigned int sample_end_address = array_uint32_le (log_entry + device->layout->pt_profile_end); + + if (type == PROFILE_FULL_SIZE) { + // actual size, includes pre-dive events + sample_start_address = array_uint32_le (log_entry + device->layout->pt_profile_pre); + } else if (type == PROFILE_READ_SIZE) { + // read size, only include dive profile + sample_start_address = array_uint32_le (log_entry + device->layout->pt_profile_begin); + } + + // Validate addresses + 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)) { + return 0; + } + + if (sample_end_address == 0xFFFFFFFF) + // Corrupt dive, guess the end address + sample_end_address = cochran_commander_guess_sample_end_address(device, data, dive_num); + + // 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; +} + + +/* + * 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) { - // Skip to fingerprint to reduce time + // We track profile ringbuffer usage to determine which dives have profile data + int profile_capacity_remaining = device->layout->rb_profile_end - device->layout->rb_profile_begin; + + int dive_count = -1; + + // Start at end of log if (data->dive_count < device->layout->rb_logbook_entry_count) - data->fp_dive_num = data->dive_count; + dive_count = data->dive_count; else - data->fp_dive_num = device->layout->rb_logbook_entry_count; - data->fp_dive_num--; + dive_count = device->layout->rb_logbook_entry_count; + dive_count--; + + int sample_read_size = 0; + data->invalid_profile_dive_num = -1; + + // Remove the pre-dive events that occur after the last dive + int rb_head_ptr = (array_uint32_le(data->config + device->layout->cf_last_interdive) & 0xfffff000) + 0x2000; + int last_dive_end_address = array_uint32_le(data->logbook + dive_count * device->layout->rb_logbook_entry_size + device->layout->pt_profile_end); + if (rb_head_ptr > last_dive_end_address) + profile_capacity_remaining -= rb_head_ptr - last_dive_end_address; + + // Loop through dives to find FP, Accumulate profile data size, + // and find the last dive with invalid profile + for (int i = dive_count; i > 0; i--) { + unsigned char *log_entry = data->logbook + i * device->layout->rb_logbook_entry_size; + + // We're done if we find the fingerprint + if (!memcmp(device->fingerprint, log_entry, sizeof(device->fingerprint))) { + data->fp_dive_num = i; + break; + } - while (data->fp_dive_num >= 0 && memcmp(device->fingerprint, - data->logbook + data->fp_dive_num * device->layout->rb_logbook_entry_size, - sizeof(device->fingerprint))) - data->fp_dive_num--; + int sample_size = cochran_commander_profile_size(device, data, i, PROFILE_FULL_SIZE); + int read_size = cochran_commander_profile_size(device, data, i, PROFILE_READ_SIZE); + + // 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; + } + + } + + return sample_read_size; } + static void cochran_commander_get_sample_parms(cochran_commander_device_t *device, cochran_data_t *data) { @@ -509,103 +625,6 @@ cochran_commander_get_sample_parms(cochran_commander_device_t *device, cochran_d } -/* - * For corrupt dives the end-of-samples pointer is 0xFFFFFFFF - * search for a reasonable size, e.g. using next dive start sample - * or end-of-samples to limit searching for recoverable samples - */ -static unsigned int -cochran_commander_guess_sample_end_address(cochran_commander_device_t *device, cochran_data_t *data, unsigned int log_num) -{ - const unsigned char *log_entry = data->logbook + device->layout->rb_logbook_entry_size * log_num; - - if (log_num == data->dive_count) - // Return next usable address from config page - return array_uint32_le(data->config + device->layout->rb_profile_end); - - // Next log's start address - return array_uint32_le(log_entry + device->layout->rb_logbook_entry_size + device->layout->pt_profile_begin); -} - - -static dc_status_t -cochran_commander_read_all (cochran_commander_device_t *device, cochran_data_t *data) -{ - dc_device_t *abstract = (dc_device_t *) device; - dc_status_t rc = DC_STATUS_SUCCESS; - - // Calculate max data sizes - unsigned int max_config = sizeof(data->config); - unsigned int max_logbook = device->layout->rb_logbook_end - device->layout->rb_logbook_begin; - unsigned int max_sample = device->layout->rb_profile_end - device->layout->rb_profile_begin; - - dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; - progress.maximum = max_config + max_logbook + max_sample; - device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); - - // Emit ID block - dc_event_vendor_t vendor; - vendor.data = device->id; - vendor.size = sizeof (device->id); - device_event_emit (abstract, DC_EVENT_VENDOR, &vendor); - - // Read config - rc = cochran_commander_read_config(device, &progress, data->config, sizeof(data->config)); - if (rc != DC_STATUS_SUCCESS) - return rc; - - // Determine size of dive list to read. - if (device->layout->endian == ENDIAN_LE) - data->dive_count = array_uint16_le (data->config + device->layout->cf_dive_count); - else - data->dive_count = array_uint16_be (data->config + device->layout->cf_dive_count); - - if (data->dive_count > device->layout->rb_logbook_entry_count) { - data->logbook_size = device->layout->rb_logbook_entry_count * device->layout->rb_logbook_entry_size; - } else { - data->logbook_size = data->dive_count * device->layout->rb_logbook_entry_size; - } - - progress.maximum -= max_logbook - data->logbook_size; - device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); - - // 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; - - // Determine sample memory to read - cochran_commander_find_fingerprint(device, data); - cochran_commander_get_sample_parms(device, data); - - progress.maximum -= max_sample - data->sample_size; - device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); - - if (data->sample_size > 0) { - data->sample = (unsigned char *) malloc(data->sample_size); - if (data->sample == NULL) { - ERROR (abstract->context, "Failed to allocate memory."); - return DC_STATUS_NOMEMORY; - } - - // Read the sample data - rc = cochran_commander_read (device, &progress, data->sample_data_offset, data->sample, data->sample_size); - if (rc != DC_STATUS_SUCCESS) { - ERROR (abstract->context, "Failed to read the sample data."); - return rc; - } - } - - return DC_STATUS_SUCCESS; -} - dc_status_t cochran_commander_device_open (dc_device_t **out, dc_context_t *context, const char *name) { @@ -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; - status = cochran_commander_read_all (device, &data); - if (status != DC_STATUS_SUCCESS) - goto error; + + // Calculate max data sizes + unsigned int max_config = sizeof(data.config); + unsigned int max_logbook = device->layout->rb_logbook_end - device->layout->rb_logbook_begin; + unsigned int max_sample = device->layout->rb_profile_end - device->layout->rb_profile_begin; + + // setup progress indication + dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; + progress.maximum = max_config + max_logbook + max_sample; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + // Emit ID block + dc_event_vendor_t vendor; + vendor.data = device->id; + vendor.size = sizeof (device->id); + device_event_emit (abstract, DC_EVENT_VENDOR, &vendor); + + // Read config + dc_status_t rc = DC_STATUS_SUCCESS; + rc = cochran_commander_read_config(device, &progress, data.config, sizeof(data.config)); + if (rc != DC_STATUS_SUCCESS) + return rc; + + // Determine size of dive list to read. + if (device->layout->endian == ENDIAN_LE) + data.dive_count = array_uint16_le (data.config + device->layout->cf_dive_count); + else + data.dive_count = array_uint16_be (data.config + device->layout->cf_dive_count); + + if (data.dive_count > device->layout->rb_logbook_entry_count) { + data.logbook_size = device->layout->rb_logbook_entry_count * device->layout->rb_logbook_entry_size; + } else { + data.logbook_size = data.dive_count * device->layout->rb_logbook_entry_size; + } + + // Update progress indicator with new maximum + progress.maximum -= max_logbook - data.logbook_size; + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + // 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; + + // Locate fingerprint, recent dive with invalid profile and calc read size + int profile_read_size = cochran_commander_find_fingerprint(device, &data); + // Update progress indicator with new maximum + progress.maximum -= (max_sample - profile_read_size); + device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); + + cochran_commander_get_sample_parms(device, &data); // Emit a device info event. dc_event_devinfo_t devinfo; @@ -792,9 +866,6 @@ cochran_commander_device_foreach (dc_device_t *abstract, dc_dive_callback_t call goto error; } - // We track profile ringbuffer usage to determine which dives have profile data - int profile_capacity_remaining = device->layout->rb_profile_end - device->layout->rb_profile_begin; - 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; - } - - if (sample_end_address == 0xFFFFFFFF) - // Corrupt dive, guess the end address - sample_end_address = cochran_commander_guess_sample_end_address(device, &data, i); - - // Determine if sample exists - if (profile_capacity_remaining > 0) { - // Subtract this dive's profile size including post-dive events - profile_capacity_remaining -= (last_start_address - sample_start_address); - // Adjust for a dive that wraps the buffer - if (sample_start_address > last_start_address) - profile_capacity_remaining -= device->layout->rb_profile_end - device->layout->rb_profile_begin; - } - last_start_address = sample_start_address; - - unsigned char *sample = NULL; int sample_size = 0; - if (profile_capacity_remaining < 0) { - // There is no profile for this dive - sample = NULL; - sample_size = 0; - } else { - // Calculate the size of the profile only - sample = data.sample + sample_start_address - data.sample_data_offset; - 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; - } + + // Determine if profile exists + if (i > data.invalid_profile_dive_num) + sample_size = cochran_commander_profile_size(device, &data, i, PROFILE_READ_SIZE); // Build dive blob unsigned int dive_size = device->layout->rb_logbook_entry_size + sample_size; @@ -857,17 +895,37 @@ cochran_commander_device_foreach (dc_device_t *abstract, dc_dive_callback_t call memcpy(dive, log_entry, device->layout->rb_logbook_entry_size); // log - // Copy profile data + int tries = 0; + // Read profile data if (sample_size) { if (sample_start_address <= sample_end_address) { - memcpy(dive + device->layout->rb_logbook_entry_size, sample, sample_size); + 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); + if (rc != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to read the sample data."); + return rc; + } } else { // It wrapped the buffer, copy two sections unsigned int size = device->layout->rb_profile_end - sample_start_address; - memcpy(dive + device->layout->rb_logbook_entry_size, sample, size); - memcpy(dive + device->layout->rb_logbook_entry_size + size, - data.sample, sample_end_address - device->layout->rb_profile_begin); + tries = 0; + do { + rc = cochran_commander_read (device, &progress, sample_start_address, dive + device->layout->rb_logbook_entry_size, size); + } while (rc != DC_STATUS_SUCCESS && tries++ < 3); + if (rc != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to read the sample data."); + return rc; + } + tries = 0; + do { + rc = cochran_commander_read (device, &progress, device->layout->rb_profile_begin, dive + device->layout->rb_logbook_entry_size + size, sample_end_address - device->layout->rb_profile_begin); + } while (rc != DC_STATUS_SUCCESS && tries++ < 3); + if (rc != DC_STATUS_SUCCESS) { + ERROR (abstract->context, "Failed to read the sample data."); + return rc; + } } } -- 2.4.11
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
On Fri, Jun 2, 2017 at 5:18 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
John,
Can you reformat your commit message to follow the standard git format? You can find a very nice description here:
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
On 2017-06-02 18:26, John Van Ostrand wrote:
On Fri, Jun 2, 2017 at 5:18 AM, Jef Driesen <jef@libdivecomputer.org> wrote:
Can you reformat your commit message to follow the standard git format? You can find a very nice description here:
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
participants (2)
-
Jef Driesen -
John Van Ostrand