February 2016 - devel - libdivecomputer.org

[PATCH 1/2] Cochran: Addressed simpler code review issues
by John Van Ostrand 27 Feb '16

27 Feb '16

Mostly style, best practice issues and a few bugs. --- src/cochran_cmdr_parser.c | 135 +++++++++++++++---------- src/cochran_commander.c | 18 ++-- src/cochran_commander_parser.c | 79 +++++++++------ src/cochran_commander_parser.h | 104 ++++++++----------- src/cochran_emc_parser.c | 222 +++++++++++++++-------------------------- 5 files changed, 259 insertions(+), 299 deletions(-) diff --git a/src/cochran_cmdr_parser.c b/src/cochran_cmdr_parser.c index 4e2915b..20d4fd2 100644 --- a/src/cochran_cmdr_parser.c +++ b/src/cochran_cmdr_parser.c @@ -40,7 +40,8 @@ cochran_cmdr_parser_get_field (dc_parser_t *abstract, dc_field_type_t type, unsigned int flags, void *value) { const unsigned char *data = abstract->data; - const unsigned char *log = data + COCHRAN_MODEL_SIZE; + const unsigned char *log_entry = data + COCHRAN_MODEL_SIZE; + unsigned int minutes = 0, qfeet = 0; dc_gasmix_t *gasmix = (dc_gasmix_t *) value; dc_salinity_t *water = (dc_salinity_t *) value; @@ -48,60 +49,72 @@ cochran_cmdr_parser_get_field (dc_parser_t *abstract, dc_field_type_t type, if (value) { switch (type) { case DC_FIELD_TEMPERATURE_SURFACE: - *((unsigned int*) value) = ((float) log[CMD_START_TEMP] - 32) / 1.8; + *((unsigned int*) value) = (log_entry[CMD_START_TEMP] - 32.0) / 1.8; break; case DC_FIELD_TEMPERATURE_MINIMUM: - if (array_uint16_le(log + CMD_MIN_TEMP) == 0xFFFF) - *((double *) value) = 0; + if (log_entry[CMD_MIN_TEMP] == 0xFF) + return DC_STATUS_UNSUPPORTED; else - *((unsigned int*) value) = ((float) log[CMD_MIN_TEMP] / 2 + *((unsigned int*) value) = (log_entry[CMD_MIN_TEMP] / 2.0 + 20 - 32) / 1.8; break; case DC_FIELD_TEMPERATURE_MAXIMUM: - if (array_uint16_le(log + CMD_MAX_TEMP) == 0xFFFF) - *((double *) value) = 0; + if (log_entry[CMD_MAX_TEMP] == 0xFF) + return DC_STATUS_UNSUPPORTED; else - *((unsigned int*) value) = ((float) log[CMD_MAX_TEMP] / 2 + *((unsigned int*) value) = (log_entry[CMD_MAX_TEMP] / 2.0 + 20 - 32) / 1.8; break; case DC_FIELD_DIVETIME: - if (array_uint16_le(log + CMD_BT) == 0xFFFF) - *((double *) value) = 0; + minutes = array_uint16_le(log_entry + CMD_BT); + + if (minutes == 0xFFFF) + return DC_STATUS_UNSUPPORTED; else - *((unsigned int *) value) = array_uint16_le (log + CMD_BT) * 60; + *((unsigned int *) value) = minutes * 60; break; case DC_FIELD_MAXDEPTH: - if (array_uint16_le(log + CMD_MAX_DEPTH) == 0xFFFF) - *((double *) value) = 0; + qfeet = array_uint16_le(log_entry + CMD_MAX_DEPTH); + if (qfeet == 0xFFFF) + return DC_STATUS_UNSUPPORTED; else - *((double *) value) = (float) array_uint16_le (log - + CMD_MAX_DEPTH) / 4 * FEET; + *((double *) value) = qfeet / 4.0 * FEET; break; case DC_FIELD_AVGDEPTH: - if (array_uint16_le(log + CMD_AVG_DEPTH) == 0xFFFF) - *((double *) value) = 0; + qfeet = array_uint16_le(log_entry + CMD_AVG_DEPTH); + if (qfeet == 0xFFFF) + return DC_STATUS_UNSUPPORTED; else - *((double *) value) = (float) array_uint16_le (log - + CMD_AVG_DEPTH) / 4 * FEET; + *((double *) value) = qfeet / 4.0 * FEET; break; case DC_FIELD_GASMIX_COUNT: *((unsigned int *) value) = 2; break; case DC_FIELD_GASMIX: - gasmix->oxygen = (double) array_uint16_le (log - + CMD_O2_PERCENT + 2 * flags) / 256 / 100; + // Gas percentages are decimal and encoded as + // highbyte = integer portion + // lowbyte = decimal portion, divide by 256 to get decimal value + gasmix->oxygen = array_uint16_le (log_entry + + CMD_O2_PERCENT + 2 * flags) / 256.0 / 100; gasmix->helium = 0; gasmix->nitrogen = 1.0 - gasmix->oxygen - gasmix->helium; break; case DC_FIELD_SALINITY: - // 0 = low conductivity, 1 = high, maybe there's a 2? - water->type = ( (log[CMD_WATER_CONDUCTIVITY] & 0x3) == 0 + // 0x00 = low conductivity, 0x10 = high, maybe there's a 0x01 and 0x11? + // Assume Cochran's conductivity ranges from 0 to 3 + // 0 is fresh water, anything else is sea water + // for density assume + // 0 = 1000kg/m³, 2 = 1025kg/m³ + // and other values are linear + water->type = ( (log_entry[CMD_WATER_CONDUCTIVITY] & 0x3) == 0 ? DC_WATER_FRESH : DC_WATER_SALT ); - water->density = 1000 + 12.5 * (log[CMD_WATER_CONDUCTIVITY] & 0x3); + water->density = 1000 + 12.5 * (log_entry[CMD_WATER_CONDUCTIVITY] & 0x3); break; case DC_FIELD_ATMOSPHERIC: + // Cochran measures air pressure and stores it as altitude. + // Convert altitude (measured in 1/4 kilofeet) back to pressure. *(double *) value = ATM / BAR * pow(1 - 0.0000225577 - * (double) log[CMD_ALTITUDE] * 250 * FEET, 5.25588); + * log_entry[CMD_ALTITUDE] * 250.0 * FEET, 5.25588); break; default: return DC_STATUS_UNSUPPORTED; @@ -116,31 +129,34 @@ dc_status_t cochran_cmdr_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t callback, void *userdata) { - const unsigned char *log = abstract->data + COCHRAN_MODEL_SIZE; - const unsigned char *samples = log + COCHRAN_CMDR_LOG_SIZE; + const unsigned char *log_entry = abstract->data + COCHRAN_MODEL_SIZE; + const unsigned char *samples = log_entry + COCHRAN_CMDR_LOG_SIZE; unsigned int size = abstract->size - COCHRAN_MODEL_SIZE - COCHRAN_CMDR_LOG_SIZE; - const unsigned char *s; - dc_sample_value_t sample = {0}, empty_sample = {0}; - unsigned int time = 0, last_sample_time; + dc_sample_value_t sample = {0}; + unsigned int time = 0, last_sample_time = 0; unsigned int offset = 0; double temperature; - double depth; + int depth_qfeet; double ascent_rate; unsigned char corrupt_dive = 0; - int cmdr_event_bytes[15][2] = { {0x00, 17}, {0x01, 21}, {0x02, 18}, - {0x03, 17}, {0x06, 19}, {0x07, 19}, - {0x08, 19}, {0x09, 19}, {0x0a, 19}, - {0x0b, 21}, {0x0c, 19}, {0x0d, 19}, - {0x0e, 19}, {0x10, 21}, - { -1, 1} }; - - if (array_uint32_le(log + COCHRAN_CMDR_LOG_SIZE / 2) == 0xFFFFFFFF) { + struct event_size cmdr_event_bytes[15] = { {0x00, 17}, {0x01, 21}, {0x02, 18}, + {0x03, 17}, {0x06, 19}, {0x07, 19}, + {0x08, 19}, {0x09, 19}, {0x0a, 19}, + {0x0b, 21}, {0x0c, 19}, {0x0d, 19}, + {0x0e, 19}, {0x10, 21}, + { -1, 1} }; + + // In rare circumstances Cochran computers won't record the end-of-dive + // log entry block. When the end-sample pointer is 0xFFFFFFFF it's corrupt. + // That means we don't really know where the dive samples end and we don't + // know what the dive summary values are (i.e. max depth, min temp) + if (array_uint32_le(log_entry + COCHRAN_CMDR_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; - WARNING(abstract->context, "Incomplete dive on %02d/%02d/%02d at %02d:%02d:%02d, trying to parse samples", log[CMD_YEAR], log[CMD_MON], log[CMD_DAY], log[CMD_HOUR], log[CMD_MIN], log[CMD_SEC]); + WARNING(abstract->context, "Incomplete dive on %02d/%02d/%02d at %02d:%02d:%02d, trying to parse samples", log_entry[CMD_YEAR], log_entry[CMD_MON], log_entry[CMD_DAY], log_entry[CMD_HOUR], log_entry[CMD_MIN], log_entry[CMD_SEC]); // Eliminate inter-dive events size = cochran_backparse(abstract, samples, size, &cmdr_event_bytes); @@ -150,19 +166,19 @@ cochran_cmdr_parser_samples_foreach (dc_parser_t *abstract, // and temp ever other second. // Prime values from the dive log section - depth = array_uint16_le (log + CMD_START_DEPTH) / 4; + depth_qfeet = array_uint16_le (log_entry + CMD_START_DEPTH); last_sample_time = sample.time = time; if (callback) callback (DC_SAMPLE_TIME, sample, userdata); - sample.depth = depth * FEET; + sample.depth = (double) depth_qfeet * FEET; if (callback) callback (DC_SAMPLE_DEPTH, sample, userdata); - sample.temperature = (float) *(log + CMD_START_TEMP) - 32 / 1.8; + sample.temperature = (log_entry[CMD_START_TEMP] - 32.0) / 1.8; if (callback) callback (DC_SAMPLE_TEMPERATURE, sample, userdata); while (offset < size) { - sample = empty_sample; + const unsigned char *s = NULL; sample.time = time; if (callback && last_sample_time != sample.time) { @@ -175,10 +191,15 @@ cochran_cmdr_parser_samples_foreach (dc_parser_t *abstract, // If corrupt_dive end before offset if (corrupt_dive) { - if ( s[0] == 0x10 - || s[0] == 0xFF - || s[0] == 0xA8) { - break; + // When we aren't sure where the sample data ends we can + // look for events that shouldn't be in the sample data. + // 0xFF is unwritten memory + // 0xA8 indicates start of post-dive interval + // 0xE3 (switch to FO2 mode) and 0xF3 (switch to blend 1) occur + // at dive start so when we see them after the first second we + // found the beginning of the next dive. + if (s[0] == 0xFF || s[0] == 0xA8) { + break; } if (time > 1 && (s[0] == 0xE3 || s[0] == 0xF3)) break; @@ -187,25 +208,33 @@ cochran_cmdr_parser_samples_foreach (dc_parser_t *abstract, // Check for event if (s[0] & 0x80) { offset += cochran_commander_handle_event(abstract, callback, - userdata, s[0], offset, time); + userdata, s[0], time); continue; } // Depth is logged as change in feet, bit 0x40 means negative depth - depth += (float) (s[0] & 0x3F) / 4 * (s[0] & 0x40 ? -1 : 1); - sample.depth = depth * FEET; + if (s[0] & 0x40) + depth_qfeet -= (s[0] & 0x3f); + else + depth_qfeet += (s[0] & 0x3f); + + sample.depth = depth_qfeet / 4.0 * FEET; if (callback) callback (DC_SAMPLE_DEPTH, sample, userdata); // Ascent rate is logged in the 0th sample, temp in the 1st, repeat. if (time % 2 == 0) { // Ascent rate - ascent_rate = (float) (s[1] & 0x7f) / 4 * (s[1] & 0x80 ? 1 : -1); + if (s[1] & 0x80) + ascent_rate = (s[1] & 0x7f) / 4.0; + else + ascent_rate = - (s[1] & 0x7f) / 4.0; + sample.ascent_rate = ascent_rate * FEET; if (callback) callback (DC_SAMPLE_ASCENT_RATE, sample, userdata); } else { // Temperature logged in half degrees F above 20 - temperature = (float) s[1] / 2 + 20; + temperature = s[1] / 2.0 + 20; sample.temperature = (temperature - 32.0) / 1.8; if (callback) callback (DC_SAMPLE_TEMPERATURE, sample, userdata); diff --git a/src/cochran_commander.c b/src/cochran_commander.c index ae2dc45..d11df92 100644 --- a/src/cochran_commander.c +++ b/src/cochran_commander.c @@ -855,7 +855,7 @@ cochran_commander_device_dump (dc_device_t *abstract, dc_buffer_t *data) static unsigned int cochran_guess_sample_end_address(cochran_layout_t *layout, cochran_data_t *data, unsigned int log_num) { - const unsigned char *log = data->logbook + layout->rb_log_size * log_num; + const unsigned char *log_entry = data->logbook + layout->rb_log_size * log_num; if (log_num == data->dive_count) // Return next usable address from config0 page @@ -863,7 +863,7 @@ cochran_guess_sample_end_address(cochran_layout_t *layout, cochran_data_t *data, + layout->rb_profile_end); // Next log's start address - return array_uint32_le(log + layout->rb_log_size + 6); + return array_uint32_le(log_entry + layout->rb_log_size + 6); } @@ -878,7 +878,7 @@ cochran_commander_device_foreach (dc_device_t *abstract, unsigned int sample_start_address, sample_end_address; dc_status_t rc; - unsigned char *log, *fingerprint, *sample, *dive; + unsigned char *log_entry, *fingerprint, *sample, *dive; int sample_size, dive_size; rc = cochran_commander_device_read_all (abstract); @@ -889,10 +889,10 @@ cochran_commander_device_foreach (dc_device_t *abstract, // Loop through each dive int i; for (i = data->dive_count - 1; i > data->fp_dive_num; i--) { - log = data->logbook + i * layout->rb_log_size; + log_entry = data->logbook + i * layout->rb_log_size; - sample_start_address = array_uint32_le (log + 6); - sample_end_address = array_uint32_le (log + layout->rb_log_size / 2); + sample_start_address = array_uint32_le (log_entry + 6); + sample_end_address = array_uint32_le (log_entry + layout->rb_log_size / 2); if (sample_end_address == 0xFFFFFFFF) // Corrupt dive, guess the end address @@ -901,7 +901,7 @@ cochran_commander_device_foreach (dc_device_t *abstract, sample = data->sample + sample_start_address - data->sample_data_offset; // Determine size of sample - unsigned int dive_num =array_uint16_le(log + layout->pt_log_dive_number); + unsigned int dive_num =array_uint16_le(log_entry + layout->pt_log_dive_number); if (dive_num >= data->profile_tail) sample_size = sample_end_address - sample_start_address; else @@ -912,7 +912,7 @@ cochran_commander_device_foreach (dc_device_t *abstract, sample_size += layout->rb_profile_end - layout->rb_profile_begin; - fingerprint = log + layout->pt_log_dive_number; + fingerprint = log_entry + layout->pt_log_dive_number; // Build dive blob dive_size = COCHRAN_MODEL_SIZE + layout->rb_log_size + sample_size; @@ -921,7 +921,7 @@ cochran_commander_device_foreach (dc_device_t *abstract, return DC_STATUS_NOMEMORY; memcpy(dive, data->id + 0x3B, 8); // model string - memcpy(dive + COCHRAN_MODEL_SIZE, log, layout->rb_log_size); // log + memcpy(dive + COCHRAN_MODEL_SIZE, log_entry, layout->rb_log_size); // log // Copy profile data if (sample_size) { diff --git a/src/cochran_commander_parser.c b/src/cochran_commander_parser.c index e2b854c..a6380e4 100644 --- a/src/cochran_commander_parser.c +++ b/src/cochran_commander_parser.c @@ -43,9 +43,6 @@ static dc_status_t cochran_commander_parser_get_field (dc_parser_t *abstract, static dc_status_t cochran_commander_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t callback, void *userdata); -int cochran_commander_handle_event (dc_parser_t *abstract, - dc_sample_callback_t callback, void *userdata, unsigned char code, - unsigned int offset, unsigned int time); typedef struct cochran_commander_parser_t cochran_commander_parser_t; @@ -81,6 +78,8 @@ cochran_commander_parser_create (dc_parser_t **out, dc_context_t *context) return DC_STATUS_NOMEMORY; } + parser->layout = NULL; + *out = (dc_parser_t *) parser; return DC_STATUS_SUCCESS; @@ -92,12 +91,14 @@ cochran_commander_parser_set_data (dc_parser_t *abstract, const unsigned char *data, unsigned int size) { cochran_commander_parser_t *parser = (cochran_commander_parser_t *) abstract; - abstract->data = data; - abstract->size = size; // Determine cochran data format // abstract->data is prefixed by the model string - parser->layout = cochran_commander_get_layout(data); + // FIXME: Instead of prefixing the model string, pass the model + // number to the cochran_commander_parser_create() function. That's + // how other backends do this. + if (size >= COCHRAN_MODEL_SIZE) + parser->layout = cochran_commander_get_layout(data); return DC_STATUS_SUCCESS; } @@ -110,22 +111,24 @@ cochran_commander_parser_get_datetime (dc_parser_t *abstract, { cochran_commander_parser_t *parser = (cochran_commander_parser_t *) abstract; const unsigned char *data = abstract->data; - const unsigned char *log = data + COCHRAN_MODEL_SIZE; + const unsigned char *log_entry = data + COCHRAN_MODEL_SIZE; + + // FIXME: See my comment regarding a layout structure below. if (parser->layout->date_format == DATE_SMHDMY) { - datetime->second = log[0]; - datetime->minute = log[1]; - datetime->hour = log[2]; - datetime->day = log[3]; - datetime->month = log[4]; - datetime->year = log[5] + (log[5] > 91 ? 1900 : 2000); + datetime->second = log_entry[0]; + datetime->minute = log_entry[1]; + datetime->hour = log_entry[2]; + datetime->day = log_entry[3]; + datetime->month = log_entry[4]; + datetime->year = log_entry[5] + (log_entry[5] > 91 ? 1900 : 2000); } else { - datetime->second = log[1]; - datetime->minute = log[0]; - datetime->hour = log[3]; - datetime->day = log[2]; - datetime->month = log[5]; - datetime->year = log[4] + (log[5] > 91 ? 1900 : 2000); + datetime->second = log_entry[1]; + datetime->minute = log_entry[0]; + datetime->hour = log_entry[3]; + datetime->day = log_entry[2]; + datetime->month = log_entry[5]; + datetime->year = log_entry[4] + (log_entry[4] > 91 ? 1900 : 2000); } return DC_STATUS_SUCCESS; @@ -137,14 +140,20 @@ cochran_commander_parser_get_field (dc_parser_t *abstract, dc_field_type_t type, { cochran_commander_parser_t *parser = (cochran_commander_parser_t *) abstract; + // FIXME: These two get_field functions appear very similar. I think + // you can easily merge them if you introduce a layout structure + // where you have the offset of each field. Check the uwatec parser + // with the uwatec_smart_header_info_t struct for an example. You + // already have the EMC_xxx and CMD_xxx macros. + + // FIXME: I suggest using the same model numbers as in descriptor.c + // and just list all models explictly with extra case statements. switch (parser->layout->model & 0xFF0000) { case COCHRAN_MODEL_COMMANDER_FAMILY: return cochran_cmdr_parser_get_field(abstract, type, flags, value); - break; case COCHRAN_MODEL_EMC_FAMILY: return cochran_emc_parser_get_field(abstract, type, flags, value); - break; } return DC_STATUS_UNSUPPORTED; @@ -156,6 +165,10 @@ cochran_commander_parser_samples_foreach (dc_parser_t *abstract, { cochran_commander_parser_t *parser = (cochran_commander_parser_t *) abstract; + // FIXME: Same remarks here. At first sight these two functions look + // again very similar. I wonder if we can merge them and deal with + // the difference by means of the info in a layout structure? + switch (parser->layout->model & 0xFF0000) { case COCHRAN_MODEL_COMMANDER_FAMILY: @@ -187,7 +200,7 @@ cochran_commander_get_event_info(const unsigned char code, int cochran_commander_handle_event (dc_parser_t *abstract, dc_sample_callback_t callback, void *userdata, unsigned char code, - unsigned int offset, unsigned int time) + unsigned int time) { dc_sample_value_t sample = {0}; cochran_events_t event; @@ -220,18 +233,15 @@ cochran_commander_handle_event (dc_parser_t *abstract, break; default: // Don't send known events of type NONE - if (! event.type == SAMPLE_EVENT_NONE) { + if (event.type != SAMPLE_EVENT_NONE) { sample.event.type = event.type; sample.event.flags = event.flag; if (callback) callback (DC_SAMPLE_EVENT, sample, userdata); } } } else { - // Unknown event, send it so we know we missed something - sample.event.type = SAMPLE_EVENT_NONE; - sample.event.flags = SAMPLE_FLAGS_NONE; - sample.event.value = code; - if (callback) callback (DC_SAMPLE_EVENT, sample, userdata); + // Unknown event, send warning so we know we missed something + WARNING(abstract->context, "Unkown event 0x%02x", code); } return event.data_bytes; @@ -243,13 +253,16 @@ cochran_commander_handle_event (dc_parser_t *abstract, * Used to find the end of a dive that has an incomplete dive-end * block. It parses backwards past inter-dive events. */ - -int cochran_backparse(dc_parser_t *abstract, const char *samples, int size, int (*event_bytes)[15][2]) { +int cochran_backparse(dc_parser_t *abstract, unsigned const char *samples, int size, struct event_size (*event_bytes)[15]) { int result = size, best_result = size; - for (int x = 0; (*event_bytes)[x][0] != -1; x++) { - int ptr = size - (*event_bytes)[x][1]; - if (ptr > 0 && samples[ptr] == (*event_bytes)[x][0]) { + for (int x = 0; (*event_bytes)[x].code != -1; x++) { + int ptr = size - (*event_bytes)[x].size; + if (ptr > 0 && samples[ptr] == (*event_bytes)[x].code) { + // Recurse to find the largest match. Because we are parsing backwards + // and the events vary in size we can't be sure the byte that matches + // the event code is an event code or data from inside a longer or shorter + // event. result = cochran_backparse(abstract, samples, ptr, event_bytes); } diff --git a/src/cochran_commander_parser.h b/src/cochran_commander_parser.h index 730edf7..0a125e5 100644 --- a/src/cochran_commander_parser.h +++ b/src/cochran_commander_parser.h @@ -22,70 +22,52 @@ typedef struct cochran_events_t { unsigned char code; unsigned char data_bytes; - const char *name; parser_sample_event_t type; parser_sample_flags_t flag; } cochran_events_t; +struct event_size { + int code; + int size; +}; + +// FIXME: Mark this variable as const! +// FIXME: This does not belong in a header file! Now you end up with +// multiple instances, one in each source file. +// FIXME: I actually prefer to see the emc and cmdr files merged into +// the commander file anyway. If you read my comments there, I think we +// can share most of the code there, so using separate files becomes +// pointless. And then the previous remark is no longer an issue. static cochran_events_t cochran_events[] = { - { 0xA8, 1, "Entered PDI mode", - SAMPLE_EVENT_SURFACE, SAMPLE_FLAGS_BEGIN }, - { 0xA9, 1, "Exited PDI mode", - SAMPLE_EVENT_SURFACE, SAMPLE_FLAGS_END }, - { 0xAB, 5, "Ceiling decrease", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xAD, 5, "Ceiling increase", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xBD, 1, "Switched to nomal PO2 setting", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xC0, 1, "Switched to FO2 21% mode", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xC1, 1, "Ascent rate greater than limit", - SAMPLE_EVENT_ASCENT, SAMPLE_FLAGS_BEGIN }, - { 0xC2, 1, "Low battery warning", - SAMPLE_EVENT_BATTERY, SAMPLE_FLAGS_NONE }, - { 0xC3, 1, "CNS Oxygen toxicity warning", - SAMPLE_EVENT_OLF, SAMPLE_FLAGS_NONE }, - { 0xC4, 1, "Depth exceeds user set point", - SAMPLE_EVENT_MAXDEPTH, SAMPLE_FLAGS_NONE }, - { 0xC5, 1, "Entered decompression mode", - SAMPLE_EVENT_DEEPSTOP, SAMPLE_FLAGS_BEGIN }, - { 0xC8, 1, "PO2 too high", - SAMPLE_EVENT_FLOOR, SAMPLE_FLAGS_BEGIN }, - { 0xCC, 1, "Low Cylinder 1 pressure", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_BEGIN }, - { 0xCE, 1, "Non-decompression warning", - SAMPLE_EVENT_RBT, SAMPLE_FLAGS_BEGIN }, - { 0xCD, 1, "Switched to deco blend", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xD0, 1, "Breathing rate alarm", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_BEGIN }, - { 0xD3, 1, "Low gas 1 flow rate", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xD6, 1, "Depth is less than ceiling", - SAMPLE_EVENT_CEILING, SAMPLE_FLAGS_BEGIN }, - { 0xD8, 1, "End decompression mode", - SAMPLE_EVENT_DEEPSTOP, SAMPLE_FLAGS_END }, - { 0xE1, 1, "End ascent rate warning", - SAMPLE_EVENT_ASCENT, SAMPLE_FLAGS_END }, - { 0xE2, 1, "Low SBAT battery warning", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xE3, 1, "Switched to FO2 mode", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xE5, 1, "Switched to PO2 mode", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xEE, 1, "End non-decompresison warning", - SAMPLE_EVENT_RBT, SAMPLE_FLAGS_END }, - { 0xEF, 1, "Switch to blend 2", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xF0, 1, "Breathing rate alarm", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_END }, - { 0xF3, 1, "Switch to blend 1", - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, - { 0xF6, 1, "End Depth is less than ceiling", - SAMPLE_EVENT_CEILING, SAMPLE_FLAGS_END }, - { 0x00, 1, NULL, - SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE } + { 0xA8, 1, SAMPLE_EVENT_SURFACE, SAMPLE_FLAGS_BEGIN }, // Entered PDI mode + { 0xA9, 1, SAMPLE_EVENT_SURFACE, SAMPLE_FLAGS_END }, // Exited PDI mode + { 0xAB, 5, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Ceiling decrease + { 0xAD, 5, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Ceiling increase + { 0xBD, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switched to nomal PO2 setting + { 0xC0, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switched to FO2 21% mode + { 0xC1, 1, SAMPLE_EVENT_ASCENT, SAMPLE_FLAGS_BEGIN }, // Ascent rate greater than limit + { 0xC2, 1, SAMPLE_EVENT_BATTERY, SAMPLE_FLAGS_NONE }, // Low battery warning + { 0xC3, 1, SAMPLE_EVENT_OLF, SAMPLE_FLAGS_NONE }, // CNS Oxygen toxicity warning + { 0xC4, 1, SAMPLE_EVENT_MAXDEPTH, SAMPLE_FLAGS_NONE }, // Depth exceeds user set point + { 0xC5, 1, SAMPLE_EVENT_DEEPSTOP, SAMPLE_FLAGS_BEGIN }, // Entered decompression mode + { 0xC8, 1, SAMPLE_EVENT_FLOOR, SAMPLE_FLAGS_BEGIN }, // PO2 too high + { 0xCC, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_BEGIN }, // Low Cylinder 1 pressure + { 0xCE, 1, SAMPLE_EVENT_RBT, SAMPLE_FLAGS_BEGIN }, // Non-decompression warning + { 0xCD, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switched to deco blend + { 0xD0, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_BEGIN }, // Breathing rate alarm + { 0xD3, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Low gas 1 flow rate + { 0xD6, 1, SAMPLE_EVENT_CEILING, SAMPLE_FLAGS_BEGIN }, // Depth is less than ceiling + { 0xD8, 1, SAMPLE_EVENT_DEEPSTOP, SAMPLE_FLAGS_END }, // End decompression mode + { 0xE1, 1, SAMPLE_EVENT_ASCENT, SAMPLE_FLAGS_END }, // End ascent rate warning + { 0xE2, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Low SBAT battery warning + { 0xE3, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switched to FO2 mode + { 0xE5, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switched to PO2 mode + { 0xEE, 1, SAMPLE_EVENT_RBT, SAMPLE_FLAGS_END }, // End non-decompresison warning + { 0xEF, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switch to blend 2 + { 0xF0, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_END }, // Breathing rate alarm + { 0xF3, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE }, // Switch to blend 1 + { 0xF6, 1, SAMPLE_EVENT_CEILING, SAMPLE_FLAGS_END }, // End Depth is less than ceiling + { 0x00, 1, SAMPLE_EVENT_NONE, SAMPLE_FLAGS_NONE } }; @@ -101,8 +83,8 @@ void cochran_commander_get_event_info(const unsigned char code, cochran_events_t *event); int cochran_commander_handle_event (dc_parser_t *abstract, dc_sample_callback_t callback, void *userdata, unsigned char code, - unsigned int offset, unsigned int time); -int cochran_backparse(dc_parser_t *abstract, const char *samples, int size, int (*event_bytes)[15][2]); + unsigned int time); +int cochran_backparse(dc_parser_t *abstract, unsigned const char *samples, int size, struct event_size (*event_bytes)[15]); // EMC FAMILY diff --git a/src/cochran_emc_parser.c b/src/cochran_emc_parser.c index c4a4d56..aae484f 100644 --- a/src/cochran_emc_parser.c +++ b/src/cochran_emc_parser.c @@ -35,103 +35,101 @@ #include "cochran_commander_parser.h" -struct dive_stats { - unsigned int dive_time; - float max_depth; - float avg_depth; - float min_temp; - float max_temp; -}; - // Size of inter-dive events -int emc_event_bytes[15][2] = { {0x00, 19}, {0x01, 23}, {0x02, 20}, +struct event_size emc_event_bytes[15] = { {0x00, 19}, {0x01, 23}, {0x02, 20}, {0x03, 19}, {0x06, 21}, {0x07, 21}, {0x0a, 21}, {0x0b, 21}, {0x0f, 19}, {0x10, 21}, { -1, 1} }; -static void cochran_emc_parse_dive_stats (dc_parser_t *abstract, - struct dive_stats *stats); - dc_status_t cochran_emc_parser_get_field (dc_parser_t *abstract, dc_field_type_t type, unsigned int flags, void *value) { - const unsigned char *log = abstract->data + COCHRAN_MODEL_SIZE; + const unsigned char *log_entry = abstract->data + COCHRAN_MODEL_SIZE; unsigned char corrupt_dive = 0; - if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) + // FIXME: Is there a reason why you have this global check instead + // of checking the actual value for 0xFFFF, like you do in the + // commander function? + if (array_uint32_le(log_entry + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) corrupt_dive = 1; dc_gasmix_t *gasmix = (dc_gasmix_t *) value; dc_salinity_t *water = (dc_salinity_t *) value; - struct dive_stats stats; - if (value) { + + // Parse data to recreate dive summary information + sample_statistics_t parsed_stats = {0}; + if (corrupt_dive) + cochran_emc_parser_samples_foreach(abstract, sample_statistics_cb, (void *) &parsed_stats); + switch (type) { case DC_FIELD_TEMPERATURE_SURFACE: - *((unsigned int*) value) = ((float) log[EMC_START_TEMP] - 32) / 1.8; + *((unsigned int*) value) = (log_entry[EMC_START_TEMP] - 32.0) / 1.8; break; case DC_FIELD_TEMPERATURE_MINIMUM: if (corrupt_dive) { - cochran_emc_parse_dive_stats(abstract, &stats); - *((unsigned int*) value) = stats.min_temp; + return DC_STATUS_UNSUPPORTED; } else - *((unsigned int*) value) = ((float) log[EMC_MIN_TEMP] / 2 + *((unsigned int*) value) = (log_entry[EMC_MIN_TEMP] / 2.0 + 20 - 32) / 1.8; break; case DC_FIELD_TEMPERATURE_MAXIMUM: if (corrupt_dive) { - cochran_emc_parse_dive_stats(abstract, &stats); - *((unsigned int*) value) = stats.max_temp; + return DC_STATUS_UNSUPPORTED; } else - *((unsigned int*) value) = ((float) log[EMC_MAX_TEMP] / 2 + *((unsigned int*) value) = (log_entry[EMC_MAX_TEMP] / 2.0 + 20 - 32) / 1.8; break; case DC_FIELD_DIVETIME: if (corrupt_dive) { - cochran_emc_parse_dive_stats(abstract, &stats); - *((unsigned int*) value) = stats.dive_time; + *((unsigned int*) value) = parsed_stats.divetime; } else - *((unsigned int *) value) = array_uint16_le (log + EMC_BT) * 60; + *((unsigned int *) value) = array_uint16_le (log_entry + EMC_BT) * 60; break; case DC_FIELD_MAXDEPTH: if (corrupt_dive) { - cochran_emc_parse_dive_stats(abstract, &stats); - *((unsigned int*) value) = stats.max_depth; + *((unsigned int*) value) = parsed_stats.maxdepth; } else - *((double *) value) = array_uint16_le (log + EMC_MAX_DEPTH) / 4 + *((double *) value) = array_uint16_le (log_entry + EMC_MAX_DEPTH) / 4.0 * FEET; break; case DC_FIELD_AVGDEPTH: if (corrupt_dive) { - cochran_emc_parse_dive_stats(abstract, &stats); - *((unsigned int*) value) = stats.avg_depth; + return DC_STATUS_UNSUPPORTED; } else - *((double *) value) = array_uint16_le (log + EMC_AVG_DEPTH) / 4 + *((double *) value) = array_uint16_le (log_entry + EMC_AVG_DEPTH) / 4.0 * FEET; break; case DC_FIELD_GASMIX_COUNT: *((unsigned int *) value) = 10; break; case DC_FIELD_GASMIX: - gasmix->oxygen = (double) array_uint16_le (log - + EMC_O2_PERCENT + 2 * flags) / 256 / 100; - gasmix->helium = (double) array_uint16_le (log - + EMC_HE_PERCENT + 2 * flags) / 256 / 100; + // Gas percentages are decimal and encoded as + // highbyte = integer value + // lowbyte = decimal portion, divide by 256 to get decimal value + gasmix->oxygen = array_uint16_le (log_entry + + EMC_O2_PERCENT + 2 * flags) / 256.0 / 100; + gasmix->helium = array_uint16_le (log_entry + + EMC_HE_PERCENT + 2 * flags) / 256.0 / 100; gasmix->nitrogen = 1.0 - gasmix->oxygen - gasmix->helium; break; case DC_FIELD_SALINITY: - // 0 = low conductivity, 2 = high, maybe there's a 1? - water->type = ( (log[EMC_WATER_CONDUCTIVITY] & 0x3) == 0 + // 0x00 = low conductivity, 0x10 = high, maybe there's a 0x01 and 0x11? + water->type = ( (log_entry[EMC_WATER_CONDUCTIVITY] & 0x3) == 0 ? DC_WATER_FRESH : DC_WATER_SALT ); - water->density = 1000 + 12.5 * (log[EMC_WATER_CONDUCTIVITY] & 0x3); + // Assume Cochran's conductivity ranges from 0 to 3 + // so 0 = 1000kg/m³, 2 = 1025kg/m³ + water->density = 1000 + 12.5 * (log_entry[EMC_WATER_CONDUCTIVITY] & 0x3); break; case DC_FIELD_ATMOSPHERIC: + // Cochran measures atm pressure and stores it as altitude. + // Convert altitude (measured in 1/4 kilofeet) back to pressure. *(double *) value = ATM / BAR * pow(1 - 0.0000225577 - * (double) log[EMC_ALTITUDE] * 250 * FEET, 5.25588); + * log_entry[EMC_ALTITUDE] * 250.0 * FEET, 5.25588); break; default: return DC_STATUS_UNSUPPORTED; @@ -141,33 +139,37 @@ cochran_emc_parser_get_field (dc_parser_t *abstract, dc_field_type_t type, return DC_STATUS_SUCCESS; } - +// TODO: Not reviewed in detail yet dc_status_t cochran_emc_parser_samples_foreach (dc_parser_t *abstract, dc_sample_callback_t callback, void *userdata) { const unsigned char *data = abstract->data; - const unsigned char *log = data + COCHRAN_MODEL_SIZE; - const unsigned char *samples = log + COCHRAN_EMC_LOG_SIZE; + const unsigned char *log_entry = data + COCHRAN_MODEL_SIZE; + const unsigned char *samples = log_entry + COCHRAN_EMC_LOG_SIZE; unsigned int size = abstract->size - COCHRAN_MODEL_SIZE - COCHRAN_EMC_LOG_SIZE; - const unsigned char *s; dc_sample_value_t sample = {0}, empty_sample = {0}; - unsigned int time = 0, last_sample_time; + unsigned int time = 0, last_sample_time = 0; unsigned int offset = 0; + int depth_qfeet = 0; double temperature; - double depth; + double start_depth = 0; double ascent_rate; unsigned char deco_obligation = 0; unsigned int deco_ceiling = 0; - unsigned int deco_time; + unsigned int deco_time = 0; unsigned char corrupt_dive = 0; - if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) { + // In rare circumstances Cochran computers won't record the end-of-dive + // log entry block. When the end-sample pointer is 0xFFFFFFFF it's corrupt. + // That means we don't really know where the dive samples end and we don't + // know what the dive summary values are (i.e. max depth, min temp) + if (array_uint32_le(log_entry + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; - WARNING(abstract->context, "Incomplete dive on %02d/%02d/%02d at %02d:%02d:%02d, trying to parse samples.", log[EMC_YEAR], log[EMC_MON], log[EMC_DAY], log[EMC_HOUR], log[EMC_MIN], log[EMC_SEC]); + WARNING(abstract->context, "Incomplete dive on %02d/%02d/%02d at %02d:%02d:%02d, trying to parse samples.", log_entry[EMC_YEAR], log_entry[EMC_MON], log_entry[EMC_DAY], log_entry[EMC_HOUR], log_entry[EMC_MIN], log_entry[EMC_SEC]); // Eliminate inter-dive events size = cochran_backparse(abstract, samples, size, &emc_event_bytes); @@ -181,18 +183,20 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, */ // Prime with values from the dive log section - depth = array_uint16_le (log + EMC_START_DEPTH) / 256; + start_depth = array_uint16_le (log_entry + EMC_START_DEPTH) / 256; last_sample_time = sample.time = time; if (callback) callback (DC_SAMPLE_TIME, sample, userdata); - sample.depth = depth * FEET; + sample.depth = start_depth * FEET; if (callback) callback (DC_SAMPLE_DEPTH, sample, userdata); - sample.temperature = (log[EMC_START_TEMP] - 32) / 1.8; + sample.temperature = (log_entry[EMC_START_TEMP] - 32) / 1.8; if (callback) callback (DC_SAMPLE_TEMPERATURE, sample, userdata); while (offset < size) { + const unsigned char *s = NULL; + sample = empty_sample; sample.time = time; @@ -206,9 +210,14 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, // If corrupt_dive end before offset if (corrupt_dive) { - if ( s[0] == 0x10 - || s[0] == 0xFF - || s[0] == 0xA8) { + // When we aren't sure where the sample data ends we can + // look for events that shouldn't be in the sample data. + // 0xFF is unwritten memory + // 0xA8 indicates start of post-dive interval + // 0xE3 (switch to FO2 mode) and 0xF3 (switch to blend 1) occur + // at dive start so when we see them after the first second we + // found the beginning of the next dive. + if (s[0] == 0xFF || s[0] == 0xA8) { break; } if (time > 1 && (s[0] == 0xE3 || s[0] == 0xF3)) @@ -218,7 +227,7 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, // Check for event if (s[0] & 0x80) { offset += cochran_commander_handle_event(abstract, callback, - userdata, s[0], offset, time); + userdata, s[0], time); switch (s[0]) { case 0xC5: // Deco obligation begins @@ -231,7 +240,7 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, deco_ceiling += 10; // feet sample.deco.type = DC_DECO_DECOSTOP; - sample.deco.time = (array_uint16_le(s + 3) + 1) * 60; + sample.deco.time = (array_uint16_le(s + 3) + 1) * 60; sample.deco.depth = deco_ceiling * FEET; if (callback) callback(DC_SAMPLE_DECO, sample, userdata); break; @@ -262,25 +271,33 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, // Depth is logged as change in feet, bit 0x40 means negative depth - depth += (float) (s[0] & 0x3F) / 4 * (s[0] & 0x40 ? -1 : 1); - sample.depth = depth * FEET; + if (s[0] & 0x40) + depth_qfeet -= (s[0] & 0x3f); + else + depth_qfeet += (s[0] & 0x3f); + + sample.depth = (start_depth + depth_qfeet / 4.0) * FEET; if (callback) callback (DC_SAMPLE_DEPTH, sample, userdata); // Ascent rate is logged in the 0th sample, temp in the 1st, repeat. if (time % 2 == 0) { // Ascent rate - ascent_rate = (float) (s[1] & 0x7f) / 4 * (s[1] & 0x80 ? 1 : -1); + if (s[1] & 0x80) + ascent_rate = (s[1] & 0x7f) / 4.0; + else + ascent_rate = - (s[1] & 0x7f) / 4.0; + sample.ascent_rate = ascent_rate * FEET; if (callback) callback (DC_SAMPLE_ASCENT_RATE, sample, userdata); } else { // Temperature logged in half degrees F above 20 - temperature = (float) s[1] / 2 + 20; + temperature = s[1] / 2.0 + 20; sample.temperature = (temperature - 32.0) / 1.8; if (callback) callback (DC_SAMPLE_TEMPERATURE, sample, userdata); } - + // Some samples are split over two seconds // Skip over event bytes to find the next sample const unsigned char *n = s + COCHRAN_EMC_SAMPLE_SIZE; @@ -326,84 +343,3 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, return DC_STATUS_SUCCESS; } - -void cochran_emc_parse_dive_stats (dc_parser_t *abstract, - struct dive_stats *stats) -{ - const unsigned char *log = abstract->data + COCHRAN_MODEL_SIZE; - const unsigned char *samples = log + COCHRAN_EMC_LOG_SIZE; - unsigned int size = abstract->size - COCHRAN_MODEL_SIZE - - COCHRAN_EMC_LOG_SIZE; - const unsigned char *s; - - unsigned int offset = 0; - float depth_m = 0, depth, temp; - unsigned int time = 0; - unsigned char corrupt_dive = 0; - - if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) { - corrupt_dive = 1; - - // Eliminate inter-dive events - size = cochran_backparse(abstract, samples, size, &emc_event_bytes); - } - - // Prime with values from the dive log section - depth = array_uint16_le (log + EMC_START_DEPTH) / 256; - - stats->max_depth = depth; - stats->min_temp = 999; - stats->avg_depth = 0; - stats->max_temp = 0; - - while (offset < size) { - s = samples + offset; - - if (corrupt_dive) { - if (s[0] == 0x10 - || s[0] == 0xFF - || s[0] == 0xA8) { - // End corrupted dive - break; - } - if (time > 1 && (s[0] == 0xE3 || s[0] == 0xF3)) - break; - } - - // Check for event - if (s[0] & 0x80) { - cochran_events_t event; - cochran_commander_get_event_info(s[0], &event); - - // Advance some bytes - if (event.code) - offset += event.data_bytes; - else - offset ++; - - continue; - } - - // Depth is logged as change in feet, bit 0x40 means negative depth - depth += (float) (s[0] & 0x3F) / 4 * (s[0] & 0x40 ? -1 : 1); - depth_m = depth * FEET; - - if (depth_m > stats->max_depth) stats->max_depth = depth_m; - - stats->avg_depth = (stats->avg_depth * (time - 1) + depth_m) / time; - - if (time % 2 != 0) { - // Temperature logged in half degrees F above 20 - temp = (float) s[1] / 2 + 20; - temp = (temp - 32.0) / 1.8; - - if (temp < stats->min_temp) stats->min_temp = temp; - if (temp > stats->max_temp) stats->max_temp = temp; - } - - time ++; - offset += COCHRAN_EMC_SAMPLE_SIZE; - } - - stats->dive_time = time - 1; -} -- 2.4.3

1 1

Re: Cochran Changes
by Jef Driesen 06 Feb '16

06 Feb '16

On 2016-01-29 10:55, john(a)vanostrand.com wrote: > On Wed, Jan 27, 2016 at 10:21 AM, Jef Driesen > <jef(a)libdivecomputer.org> wrote: >> The dc_device_foreach() function must return the dives in reverse >> chronological order. This is a requirement for the download only new >> dives feature. When dives are returned in reverse chronological >> order, an application (or libdivecomputer itself) can simply stop >> downloading dives as soon as a previously downloaded dive is >> recognized. Very simple and efficient. > > This is my loop in dc_device_foreach. > > int i; > for (i = data->dive_count - 1; i > data->fp_dive_num; i--) { Sorry, I missed that. >> Thus if we need to return the dives in reverse chronological order, >> it makes sense to process (and also download) the data in this >> order. Otherwise you'll end up with a rather inefficient >> implementation. >> >> I have to read through your documentation again, but processing the >> dives in reverse order might also make the recovery of corrupt dives >> easier. If the tail of a dive is missing, then it can run at most >> until the start of the next dive. And due the reverse order we >> already have that one. >> >>> 3. Corrupt dive handling. In some cases (like a low battery >>> especially in >>> cold water) the computer resets during a dive. This results in a >>> "start-dive" block written but no valid "end-dive" block written. >>> We know >>> information from the start of a dive (like date/time, gasses, >>> profile start >>> pointer, etc.) but we don't know information accumulated during or >>> at the >>> end of a dive (like end-profile pointer, max depth, min temp, >>> etc.) I've >>> taken to guessing the end of a dive by starting with the next >>> dive's >>> pre-dive-profile-pointer and backing up until we think we have the >>> previous >>> dive's end. We haven't resolved our differences on this. It seems >>> to down >>> to the question: Do we present a partial or broken profile in the >>> interest >>> of giving the diver something or do we give nothing in the >>> interest of >>> being accurate? >> >> That's a difficult question. In general, I prefer to be very strict >> and simply fail on unexpected data. Usually this is the correct >> thing to do, because such unexpected data often turns out to be an >> wrong assumption in the code. So being strict helps finding bugs. >> But sometimes the data is really wrong (due to a firmware bug, >> running out of battery during the dive, etc), and if it happens >> frequently, then a workaround might indeed be necessary. >> >> It also depends on where the data "corruption" is located. If the >> information needed to move from one dive to another is good, but we >> are unsure about the contents of the dive, then we can return the >> bogus dive to the application and let the parser deal with it. You >> might get incorrect data for that particular dive, or even a failure >> to parse the dive. This would ensure that we can still download the >> other dives. But if the primary structure is damaged and we can no >> longer safely move to the next dive, then I think we should fail. > > I've been working on code to retrieve corrupt dives and it's becoming > unwieldy. In working backwards from a future dive's profile I have to > remove inter-dive events which vary in length. it's possible that two > events might match the data and the code would have to decide which > event to use. The code also needs to do range checking, like ensuring > it doesn't exceed the malloc'ed memory and things like ring-buffer > wrapping. I should also remove any surface time (the time the DC still > stores samples in case you re-descend.) I think the problem of recovering corrupt samples becomes a lot easier if we can defer it to the parser. At the device layer we simply use the begin/end/pre-dive pointers. If the tail of the dive is missing (e.g. end pointer equal to 0xFFFFFFFF), we assume it runs to the start of the next dive. And then it's up to the parser to deal with this. In the parser we no longer have to worry about ringbuffer wrapping, only the end of the buffer. That should makes things already easier. > There are ways to make this better but I think I'm going to have to > think about it for a bit. I'm considering pulling that code. Any ideas > on what I should do with the code so it isn't lost. An #ifdef maybe? > > I've done some work and I may have a decent algorithm to backtrack > over the inter-dive events to get to good data. It goes as far as > removing good data in an effort to eliminate giving bad data. > > To refresh the issue: it has never been finding the beginning of data. > The issue has been finding the end. ‎We do have the start of the > next dive or, if no next dive, we have the ring buffer head pointer. > What happens if we simply use those pointers is that interdive data, > the data recorded between dives like power on events, is interpreted > as profile data. The profile then looks erratic at the end. So the > trick is to backtrack to find the real end of good data. Ideally wed > be able to do that accurately but if we have to remove good data to be > sure it would result in only loosing a few seconds of dive samples. > > Now that I've thought about the problem more and identified many of > the issues I have a standalone test program that's doing a good job of > backtracking to get to good data. I've used recursion to simplify the > code and derive at an optimal solution. In the dozen examples I'm > using it's working on 11 of them well enough to remove all non profile > data. The 12th leaves several seconds of non profile data. Don't we have the pre-dive pointer to indicate where the interdive data starts? Thus if the tail of a dive is missing, then the end is not really at the start of the next dive, but at the start of the interdive data. So the risk of trying to interpret interdive data as sample data disappears. Or am I missing something here? Jef

2 2

[PATCH 1/2] Cochran: Added warning when we process an incomplete dive.
by John Van Ostrand 06 Feb '16

06 Feb '16

--- src/cochran_cmdr_parser.c | 2 ++ src/cochran_emc_parser.c | 2 ++ 2 files changed, 4 insertions(+) diff --git a/src/cochran_cmdr_parser.c b/src/cochran_cmdr_parser.c index a7db1cd..4e2915b 100644 --- a/src/cochran_cmdr_parser.c +++ b/src/cochran_cmdr_parser.c @@ -140,6 +140,8 @@ cochran_cmdr_parser_samples_foreach (dc_parser_t *abstract, if (array_uint32_le(log + COCHRAN_CMDR_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; + WARNING(abstract->context, "Incomplete dive on %02d/%02d/%02d at %02d:%02d:%02d, trying to parse samples", log[CMD_YEAR], log[CMD_MON], log[CMD_DAY], log[CMD_HOUR], log[CMD_MIN], log[CMD_SEC]); + // Eliminate inter-dive events size = cochran_backparse(abstract, samples, size, &cmdr_event_bytes); } diff --git a/src/cochran_emc_parser.c b/src/cochran_emc_parser.c index 322f3d4..c891a2a 100644 --- a/src/cochran_emc_parser.c +++ b/src/cochran_emc_parser.c @@ -167,6 +167,8 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; + WARNING(abstract->context, "Incomplete dive on %02d/%02d/%02d at %02d:%02d:%02d, trying to parse samples\n", log[EMC_YEAR], log[EMC_MON], log[EMC_DAY], log[EMC_HOUR], log[EMC_MIN], log[EMC_SEC]); + // Eliminate inter-dive events size = cochran_backparse(abstract, samples, size, &emc_event_bytes); } -- 2.4.3

1 1

Cochran Simulator, tools and data
by John Van Ostrand 06 Feb '16

06 Feb '16

Jef, You may be interested in these files if you want to work with the Cochran code. simcochran.c is a simulator that now uses ptys so I can avoid having to use some serial port emulator. It's as easy as running: simcochran -e emc emc.tar.bz and cmdr.tar.bz are data files for use with simcochran and list. These include some incomplete dives. list.c is a program to list the dives from a memory file. This is handy for finding corrupt dives. list -c cmdr/memory -- John Van Ostrand At large on sabbatical

1 0

[PATCH] Cochran: Added back-parser to eliminate inter-dive events on incomplete dives.
by John Van Ostrand 06 Feb '16

06 Feb '16

Incomplete dives may have profile data but since we don't have an end-pointer, we have to use the next dive's pointer or ringbuffer head point in config to get close to the end of the dive. This often includes inter-dive events. We can eliminate those by parsing backwards. --- src/cochran_cmdr_parser.c | 14 ++++++++++++-- src/cochran_commander.c | 9 +++++++-- src/cochran_commander_parser.c | 26 +++++++++++++++++++++++++- src/cochran_commander_parser.h | 2 ++ src/cochran_emc_parser.c | 19 ++++++++++++++++--- 5 files changed, 62 insertions(+), 8 deletions(-) diff --git a/src/cochran_cmdr_parser.c b/src/cochran_cmdr_parser.c index 73b344f..a7db1cd 100644 --- a/src/cochran_cmdr_parser.c +++ b/src/cochran_cmdr_parser.c @@ -130,10 +130,20 @@ cochran_cmdr_parser_samples_foreach (dc_parser_t *abstract, double depth; double ascent_rate; unsigned char corrupt_dive = 0; - - if (array_uint32_le(log + COCHRAN_CMDR_LOG_SIZE / 2) == 0xFFFFFFFF) + int cmdr_event_bytes[15][2] = { {0x00, 17}, {0x01, 21}, {0x02, 18}, + {0x03, 17}, {0x06, 19}, {0x07, 19}, + {0x08, 19}, {0x09, 19}, {0x0a, 19}, + {0x0b, 21}, {0x0c, 19}, {0x0d, 19}, + {0x0e, 19}, {0x10, 21}, + { -1, 1} }; + + if (array_uint32_le(log + COCHRAN_CMDR_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; + // Eliminate inter-dive events + size = cochran_backparse(abstract, samples, size, &cmdr_event_bytes); + } + // Cochran samples depth every second and varies between ascent rate // and temp ever other second. diff --git a/src/cochran_commander.c b/src/cochran_commander.c index a8b2f4a..ae2dc45 100644 --- a/src/cochran_commander.c +++ b/src/cochran_commander.c @@ -195,8 +195,13 @@ cochran_packet (cochran_device_t *device, dc_event_progress_t *progress, rc = serial_configure(device->port, layout->high_baud_rate, 8, SERIAL_PARITY_NONE, 2, SERIAL_FLOWCONTROL_NONE); if (rc == -1) { - ERROR (abstract->context, "Failed to set the high baud rate."); - return DC_STATUS_IO; + // Assume we are talking to a pty, a simulator + rc = serial_configure(device->port, 115200, 8, SERIAL_PARITY_NONE, + 2, SERIAL_FLOWCONTROL_NONE); + if (rc == -1) { + ERROR (abstract->context, "Failed to set the high baud rate."); + return DC_STATUS_IO; + } } } diff --git a/src/cochran_commander_parser.c b/src/cochran_commander_parser.c index 955dba0..e2b854c 100644 --- a/src/cochran_commander_parser.c +++ b/src/cochran_commander_parser.c @@ -100,7 +100,7 @@ cochran_commander_parser_set_data (dc_parser_t *abstract, parser->layout = cochran_commander_get_layout(data); return DC_STATUS_SUCCESS; -} +} // There are two date formats used by Cochran @@ -236,3 +236,27 @@ cochran_commander_handle_event (dc_parser_t *abstract, return event.data_bytes; } + +/* +* cochran_backparse +* +* Used to find the end of a dive that has an incomplete dive-end +* block. It parses backwards past inter-dive events. +*/ + +int cochran_backparse(dc_parser_t *abstract, const char *samples, int size, int (*event_bytes)[15][2]) { + int result = size, best_result = size; + + for (int x = 0; (*event_bytes)[x][0] != -1; x++) { + int ptr = size - (*event_bytes)[x][1]; + if (ptr > 0 && samples[ptr] == (*event_bytes)[x][0]) { + result = cochran_backparse(abstract, samples, ptr, event_bytes); + } + + if (result < best_result) { + best_result = result; + } + } + + return best_result; +} diff --git a/src/cochran_commander_parser.h b/src/cochran_commander_parser.h index 4e9fc01..730edf7 100644 --- a/src/cochran_commander_parser.h +++ b/src/cochran_commander_parser.h @@ -102,6 +102,8 @@ void cochran_commander_get_event_info(const unsigned char code, int cochran_commander_handle_event (dc_parser_t *abstract, dc_sample_callback_t callback, void *userdata, unsigned char code, unsigned int offset, unsigned int time); +int cochran_backparse(dc_parser_t *abstract, const char *samples, int size, int (*event_bytes)[15][2]); + // EMC FAMILY dc_status_t cochran_emc_parser_get_field (dc_parser_t *abstract, diff --git a/src/cochran_emc_parser.c b/src/cochran_emc_parser.c index 884f8aa..322f3d4 100644 --- a/src/cochran_emc_parser.c +++ b/src/cochran_emc_parser.c @@ -43,6 +43,12 @@ struct dive_stats { float max_temp; }; +// Size of inter-dive events +int emc_event_bytes[15][2] = { {0x00, 19}, {0x01, 23}, {0x02, 20}, + {0x03, 19}, {0x06, 21}, {0x07, 21}, + {0x0a, 21}, {0x0b, 21}, {0x0f, 19}, + {0x10, 21}, + { -1, 1} }; static void cochran_emc_parse_dive_stats (dc_parser_t *abstract, struct dive_stats *stats); @@ -158,10 +164,13 @@ cochran_emc_parser_samples_foreach (dc_parser_t *abstract, unsigned int deco_time; unsigned char corrupt_dive = 0; - - if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) + if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; + // Eliminate inter-dive events + size = cochran_backparse(abstract, samples, size, &emc_event_bytes); + } + /* * Cochran samples depth every second and varies between ascent rate * and temp ever other second. @@ -330,9 +339,13 @@ void cochran_emc_parse_dive_stats (dc_parser_t *abstract, unsigned int time = 0; unsigned char corrupt_dive = 0; - if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) + if (array_uint32_le(log + COCHRAN_EMC_LOG_SIZE / 2) == 0xFFFFFFFF) { corrupt_dive = 1; + // Eliminate inter-dive events + size = cochran_backparse(abstract, samples, size, &emc_event_bytes); + } + // Prime with values from the dive log section depth = array_uint16_le (log + EMC_START_DEPTH) / 256; -- 2.4.3

1 0

Missing Patch
by John Van Ostrand 04 Feb '16

04 Feb '16

You may have notice there isn't a [PATCH 2/3]. That's because I messed up my branch and got a blank patch when I created a new branch. I think I may have deleted the main branch when I was experimenting with creating and removing branches. -- John Van Ostrand At large on sabbatical

1 0

[PATCH 1/3] Cochran Commander: get_field would return bad values (xFF) when dive was incomplete.
by John Van Ostrand 04 Feb '16

04 Feb '16

The commander parser wasn't detecting when dives were incomplete (i.e. dive end block would be set to all 0xFF). So it returned large max depths, temps and other fields. --- src/cochran_cmdr_parser.c | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/src/cochran_cmdr_parser.c b/src/cochran_cmdr_parser.c index b4e8fb7..73b344f 100644 --- a/src/cochran_cmdr_parser.c +++ b/src/cochran_cmdr_parser.c @@ -49,22 +49,40 @@ cochran_cmdr_parser_get_field (dc_parser_t *abstract, dc_field_type_t type, switch (type) { case DC_FIELD_TEMPERATURE_SURFACE: *((unsigned int*) value) = ((float) log[CMD_START_TEMP] - 32) / 1.8; + break; case DC_FIELD_TEMPERATURE_MINIMUM: - *((unsigned int*) value) = ((float) log[CMD_MIN_TEMP] / 2 - + 20 - 32) / 1.8; + if (array_uint16_le(log + CMD_MIN_TEMP) == 0xFFFF) + *((double *) value) = 0; + else + *((unsigned int*) value) = ((float) log[CMD_MIN_TEMP] / 2 + + 20 - 32) / 1.8; + break; case DC_FIELD_TEMPERATURE_MAXIMUM: - *((unsigned int*) value) = ((float) log[CMD_MAX_TEMP] / 2 - + 20 - 32) / 1.8; + if (array_uint16_le(log + CMD_MAX_TEMP) == 0xFFFF) + *((double *) value) = 0; + else + *((unsigned int*) value) = ((float) log[CMD_MAX_TEMP] / 2 + + 20 - 32) / 1.8; + break; case DC_FIELD_DIVETIME: - *((unsigned int *) value) = array_uint16_le (log + CMD_BT) * 60; + if (array_uint16_le(log + CMD_BT) == 0xFFFF) + *((double *) value) = 0; + else + *((unsigned int *) value) = array_uint16_le (log + CMD_BT) * 60; break; case DC_FIELD_MAXDEPTH: - *((double *) value) = (float) array_uint16_le (log + CMD_MAX_DEPTH) - / 4 * FEET; + if (array_uint16_le(log + CMD_MAX_DEPTH) == 0xFFFF) + *((double *) value) = 0; + else + *((double *) value) = (float) array_uint16_le (log + + CMD_MAX_DEPTH) / 4 * FEET; break; case DC_FIELD_AVGDEPTH: - *((double *) value) = (float) array_uint16_le (log + CMD_AVG_DEPTH) - / 4 * FEET; + if (array_uint16_le(log + CMD_AVG_DEPTH) == 0xFFFF) + *((double *) value) = 0; + else + *((double *) value) = (float) array_uint16_le (log + + CMD_AVG_DEPTH) / 4 * FEET; break; case DC_FIELD_GASMIX_COUNT: *((unsigned int *) value) = 2; -- 2.4.3

1 1