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-aroundsample_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 includingpost-dive events
profile_capacity_remaining -= sample_size;if (profile_capacity_remaining < 0) {// Save the last dive that is missingprofile data
data->invalid_profile_dive_num = i;}// Accumulate read size for progress barsample_read_size += read_size;}if (profile_capacity_remaining < 0) {// There is no profile for this divesample_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 bookrc = 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);
// Validateif (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, "Failedto read the sample data.");
return rc;Again a memory leak here! There are two similar ones a few lines below.
Fixed.
Jef