Scubapro Galileo 2 (G2)

[Jef Driesen]

On 2017-06-09 07:28, Linus Torvalds wrote:
> diff --git a/examples/common.c b/examples/common.c
> index 3c4561b..5ebdd77 100644
> --- a/examples/common.c
> +++ b/examples/common.c
> @@ -54,6 +54,7 @@ static const backend_table_t g_backends[] = {
>  	{"smart",       DC_FAMILY_UWATEC_SMART,        0x10},
> +	{"g2",          DC_FAMILY_UWATEC_SMART,        0x10},
>  	{"meridian",    DC_FAMILY_UWATEC_MERIDIAN,     0x20},

I think you meant DC_FAMILY_SCUBAPRO_G2 here.

> diff --git a/include/libdivecomputer/common.h 
> b/include/libdivecomputer/common.h
> index 67398d1..fdaaa1b 100644
> --- a/include/libdivecomputer/common.h
> +++ b/include/libdivecomputer/common.h
> @@ -59,6 +59,8 @@ typedef enum dc_family_t {
>  	DC_FAMILY_UWATEC_MEMOMOUSE,
>  	DC_FAMILY_UWATEC_SMART,
>  	DC_FAMILY_UWATEC_MERIDIAN,
> +	/* We'll enumerate the Scubapro G2 under Uwatec */
> +	DC_FAMILY_SCUBAPRO_G2,

For consistency with the other uwatec/scubapro backends, replace 
DC_FAMILY_SCUBAPRO_G2 with DC_FAMILY_UWATEC_G2. Same remark for the 
filenames. For end-users the fact that the internal name is uwatec 
instead of scubapro doesn't really matter, because they will only see 
the name "Scubapro G2".

BTW, we now have 3 backends (smart, meridian and g2) using the exact 
same communication protocol, but with a different transport (irda, 
usb-serial and usbhid). I wonder if we can merge this back into a single 
backend some day? Because once the I/O refactoring lands, the difference 
between the transports will disappear and those backends will even look 
more similar.

The main difference is the transfer function. For the smart it's a plain 
IrDA read/write, for the meridian there is some additional framing 
added, and for the G2 there is the USB HID packet stuff.

> diff --git a/src/parser.c b/src/parser.c
> index ebbc6a6..fea1454 100644
> --- a/src/parser.c
> +++ b/src/parser.c
> @@ -96,6 +96,9 @@ dc_parser_new_internal (dc_parser_t **out, 
> dc_context_t *context, dc_family_t fa
>  	case DC_FAMILY_UWATEC_MEMOMOUSE:
>  		rc = uwatec_memomouse_parser_create (&parser, context, devtime, 
> systime);
>  		break;
> +	case DC_FAMILY_SCUBAPRO_G2:
> +		rc = uwatec_smart_parser_create (&parser, context, 0x11, devtime, 
> systime);
> +		break;
>  	case DC_FAMILY_UWATEC_SMART:
>  	case DC_FAMILY_UWATEC_MERIDIAN:
>  		rc = uwatec_smart_parser_create (&parser, context, model, devtime, 
> systime);

Is there a reason why you have hardcoded the model number to 0x11? Does 
the data contain a different model number? Because in that case we 
should update the parser to support the new model number.

> +typedef struct scubapro_g2_device_t {
> +	...
> +	unsigned int address;
> +	...
> +} scubapro_g2_device_t;

The address member is a left-over from the irda code and can be removed.

> +#define PACKET_SIZE 64
> +static int receive_data(scubapro_g2_device_t *g2, unsigned char 
> *buffer, int size)
> +{
> +	while (size) {
> +		unsigned char buf[PACKET_SIZE];
> +		size_t transferred = 0;
> +		dc_status_t rc;
> +		int len;
> +
> +		rc = dc_usbhid_read(g2->usbhid, buf, PACKET_SIZE, &transferred);
> +		if (rc != DC_STATUS_SUCCESS) {
> +			ERROR(g2->base.context, "read interrupt transfer failed");
> +			return -1;
> +		}
> +		if (transferred != PACKET_SIZE) {
> +			ERROR(g2->base.context, "incomplete read interrupt transfer (got 
> %zu, expected %d)", transferred, PACKET_SIZE);
> +			return -1;
> +		}
> +		len = buf[0];
> +		if (len >= PACKET_SIZE) {
> +			ERROR(g2->base.context, "read interrupt transfer returns impossible 
> packet size (%d)", len);
> +			return -1;
> +		}
> +		HEXDUMP (g2->base.context, DC_LOGLEVEL_DEBUG, "rcv", buf+1, len);
> +		if (len > size) {
> +			ERROR(g2->base.context, "receive result buffer too small - 
> truncating");
> +			len = size;
> +		}
> +		memcpy(buffer, buf+1, len);
> +		size -= len;
> +		buffer += len;
> +	}
> +	return 0;
> +}

There are a two issues with this code:

The error code from dc_usbhid_read is dropped and replaced with a 
generic -1, which is translated again to DC_STATUS_IO in the caller. It 
would be much better to pass the actual error code. The difference 
between for example TIMEOUT, IO and PROTOCOL errors is valuable info. 
(Maybe not for the end-user, but certainly for the developer debugging 
the problem.) Those other -1 should be translated into 
DC_STATUS_PROTOCOL.

Truncating the data if the buffer is too small is a bad idea because 
you'll return corrupt data. Just bail out with error DC_STATUS_PROTOCOL. 
If this ever happens, then it's most likely caused by a bug in the code 
or a change in the communication protocol. In both cases the code should 
be fixed.

> +dc_status_t
> +scubapro_g2_device_open(dc_device_t **out, dc_context_t *context)
> +{
> +	dc_status_t status = DC_STATUS_SUCCESS;
> +	scubapro_g2_device_t *device = NULL;
> +
> +	...
> +
> +	// Open the irda socket.
> +	status = dc_usbhid_open(&device->usbhid, context, 0x2e6c, 0x3201);
> +	if (status != DC_STATUS_SUCCESS) {
> +		ERROR (context, "Failed to open USB device");
> +		goto error_free;
> +	}
> +
> +	*out = (dc_device_t*) device;
> +
> +	return DC_STATUS_SUCCESS;
> +
> +error_free:
> +	dc_device_deallocate ((dc_device_t *) device);
> +	return status;
> +}

Is the handshaking no longer necessary for the G2? I've always wondered 
why it's there for the smart and meridian because it doesn't really 
return any useful info. It's certainly possible that it can be omitted, 
but I never really tried that. So I'm just curious why you left it out.

> +static dc_status_t
> +scubapro_g2_device_dump (dc_device_t *abstract, dc_buffer_t *buffer)
> +{
> +	scubapro_g2_device_t *device = (scubapro_g2_device_t*) abstract;
> +	dc_status_t rc = DC_STATUS_SUCCESS;
> +
> +	...
> +
> +	if (receive_data(device, data, length)) {
> +		ERROR (abstract->context, "Received an unexpected size.");
> +		return DC_STATUS_IO;
> +	}
> +
> +	...
> +
> +	return DC_STATUS_SUCCESS;
> +}

This will need some improvements to report more fine-grained progress 
events.

The rest looks good to me. Will you send an updated patch, or do I make 
the changes myself?

Jef