Re: Scubapro Galileo 2 (G2)

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