[PATCH] Cochran: Added support for Pre-21000 s/n Commander dive computers

[Jef Driesen]

On 2017-06-03 01:51, John Van Ostrand wrote:
> This adds support for older Cochran Commander dive computers,
> specifically Commanders with serial numbers prior to 21000.
> 
> This also renames "Commander" model to "Commander II" and
> adds "Commander I" to refer to pre-21000 models.
> 
> This commit also fixes problems with dive computers where the
> log ringbuffer has wrapped.

Don't mix two unrelated changes. It only makes it more difficult to see 
what change is what. Use two separate patches instead.

> +unsigned int
> +array_uint32_word_be (const unsigned char data[])
> +{
> +	return data[1] + (data[0] << 8) + (data[3] << 16) + (data[2] << 24);
> +}

That's a weird encoding! I wonder who thought that would be a good idea 
:-)

> @@ -205,10 +241,12 @@ static unsigned int
>  cochran_commander_get_model (cochran_commander_device_t *device)
>  {
>  	const cochran_commander_model_t models[] = {
> -		{"\x11""2", COCHRAN_MODEL_COMMANDER_AIR_NITROX},
> -		{"73",      COCHRAN_MODEL_EMC_14},
> -		{"A3",      COCHRAN_MODEL_EMC_16},
> -		{"23",      COCHRAN_MODEL_EMC_20},
> +		{"\x11""21", COCHRAN_MODEL_COMMANDER_PRE21000},
> +		{"\x11""22", COCHRAN_MODEL_COMMANDER_AIR_NITROX},
> +		{"730",      COCHRAN_MODEL_EMC_14},
> +		{"A30",      COCHRAN_MODEL_EMC_16},
> +		{"230",      COCHRAN_MODEL_EMC_20},
> +		{"231",      COCHRAN_MODEL_EMC_20},
>  	};

How can this work? Originally you had this:

                {"AM\x11""2212\x02", COCHRAN_MODEL_COMMANDER_AIR_NITROX},
                {"AM7303\x8b\x43",   COCHRAN_MODEL_EMC_14},
                {"AMA315\xC3\xC5",   COCHRAN_MODEL_EMC_16},
                {"AM2315\xA3\x71",   COCHRAN_MODEL_EMC_20},

Thus for the EMC 16 that would be "A31" and not "A30". Does that mean it 
has always been wrong?

> +	unsigned int head_dive = 0, tail_dive = 0;
> +
> +	if (data->dive_count <= device->layout->rb_logbook_entry_count) {
> +		head_dive = data->dive_count;
> +		tail_dive = 0;
> +	} else {
> +		// Log wrapped
> +		tail_dive = data->dive_count % 
> device->layout->rb_logbook_entry_count;
> +		head_dive = tail_dive;
> +	}
> +
>  	// 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 int i = head_dive;
> +	do {
> +		i = ringbuffer_decrement(i, 1, 0, 
> device->layout->rb_logbook_entry_count);
> +
>  		unsigned char *log_entry = data->logbook + i *
> device->layout->rb_logbook_entry_size;
> 
>  		...
>  		}
> -	}
> +	} while (i != tail_dive);

I find this construct more complex than necessary. With a do-while loop 
the important part, the condition, is hidden at the very end. You also 
need to be careful to handle the dive_count=0 case (which you do earlier 
in the code). In other backends I use this loop to get the same result:

for (unsigned int i = 0; i < dive_count; ++i) {
     unsigned int idx = (layout->rb_logbook_entry_count + head_dive - (i 
+ 1)) % layout->rb_logbook_entry_count;
     ...
}

> +// Cochran time stamps start at Jan 1, 1992
> +#define COCHRAN_TIME_SHIFT 694242000

This time shift is typically named the epoch.

> +typedef enum cochran_date_encoding_t {
> +	DATE_ENCODING_ELEMENTAL,
> +	DATE_ENCODING_SECONDS_SINCE,
> +} cochran_date_encoding_t;

This two representations are typically named broken-down time 
(dc_datetime_t, struct tm) and seconds since the epoch (dc_ticks_t, 
time_t). So I suggest to rename to DATETIME and TICKS. ELEMENTAL sounds 
a bit weird to me.

>  typedef struct cochran_parser_layout_t {
>  	cochran_sample_format_t format;
>  	unsigned int headersize;
>  	unsigned int samplesize;
> +	cochran_date_encoding_t date_encoding;
>  	unsigned int second, minute, hour, day, month, year;
> +	unsigned int timestamp;
>  	unsigned int pt_profile_begin;
>  	unsigned int water_conductivity;
>  	unsigned int pt_profile_pre;

You now have several fields for the same thing. Depending on the 
encoding you either use timestamp or second/minute/etc. And that second 
one comes in two different variants (1, 0, 3, 2, 5, 4 and 0, 1, 2, 3, 4, 
5). Maybe this can be dealt with three encodings:

DATETIME_ENCODING_MSDHYM
DATETIME_ENCODING_SMHDMY
DATETIME_ENCODING_TICKS

and just one "datetime" offset.

> +		if (layout->date_encoding == DATE_ENCODING_SECONDS_SINCE) {
> +			time_t ts;
> +			struct tm *t;
> +			ts = array_uint32_le(data + layout->timestamp) + 
> COCHRAN_TIME_SHIFT;
> +			t = localtime(&ts);
> +			datetime->second = t->tm_sec;
> +			datetime->minute = t->tm_min;
> +			datetime->hour = t->tm_hour;
> +			datetime->day = t->tm_mday;
> +			datetime->month = t->tm_mon + 1;
> +			datetime->year = 1900 + t->tm_year;

Use the dc_datetime_localtime function instead of localtime directly! It 
will use the thread-safe localtime_r function (if available) and take 
care of the conversion to dc_datetime_t automatically.

> diff --git a/src/descriptor.c b/src/descriptor.c
> index b59ea45..76157da 100644
> --- a/src/descriptor.c
> +++ b/src/descriptor.c
> @@ -292,10 +292,11 @@ static const dc_descriptor_t g_descriptors[] = {
>  	{"DiveSystem", "iX3M Deep",     DC_FAMILY_DIVESYSTEM_IDIVE, 0x23},
>  	{"DiveSystem", "iX3M Tec",      DC_FAMILY_DIVESYSTEM_IDIVE, 0x24},
>  	{"DiveSystem", "iX3M Reb",      DC_FAMILY_DIVESYSTEM_IDIVE, 0x25},
> -	{"Cochran", "Commander",	DC_FAMILY_COCHRAN_COMMANDER, 0},
> -	{"Cochran", "EMC-14",		DC_FAMILY_COCHRAN_COMMANDER, 1},
> -	{"Cochran", "EMC-16",		DC_FAMILY_COCHRAN_COMMANDER, 2},
> -	{"Cochran", "EMC-20H",		DC_FAMILY_COCHRAN_COMMANDER, 3},
> +	{"Cochran", "Commander I",	DC_FAMILY_COCHRAN_COMMANDER, 0},
> +	{"Cochran", "Commander II",	DC_FAMILY_COCHRAN_COMMANDER, 1},
> +	{"Cochran", "EMC-14",		DC_FAMILY_COCHRAN_COMMANDER, 2},
> +	{"Cochran", "EMC-16",		DC_FAMILY_COCHRAN_COMMANDER, 3},
> +	{"Cochran", "EMC-20H",		DC_FAMILY_COCHRAN_COMMANDER, 4},

The model numbers are supposed to be stable and never change (except for 
fixing bugs). Otherwise you won't be able to parse dives that have been 
downloaded with a previous libdivecomputer version. So the general rule 
is that if you want to add a new model, it will need to use the next 
available number instead of shifting the existing ones. (I think there 
are very few Cochran users out there, so the chance that someone is 
affected is probably small, so I could make a small exception.)

I assume there is no way to detect the dataformat based on the dive data 
itself?

Jef