From c93314026c316a4aa4f4df8002c642882360e7a4 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Fri, 31 Jan 2014 15:10:52 +0100 Subject: [PATCH 1/5] Use the same standard baudrate for all models. The Mares Icon HD use a processor with integrated USB circuit, which presents itself to the host system as a CDC-ACM device. Since there is no external usb-serial chip involved, the baudrate and other serial line parameters are irrelevant. By choosing the same settings as the later models, which do use an usb-serial chip, we can completely avoid the problem of unresponsive devices due to using the wrong baudrate. --- src/mares_iconhd.c | 14 ++------------ 1 file changed, 2 insertions(+), 12 deletions(-) diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index 5e09ea3..52305c1 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -37,12 +37,6 @@ rc == -1 ? DC_STATUS_IO : DC_STATUS_TIMEOUT \ ) -#if defined(_WIN32) || defined(__APPLE__) -#define BAUDRATE 256000 -#else -#define BAUDRATE 230400 -#endif - #define MATRIX 0x0F #define ICONHD 0x14 #define ICONHDNET 0x15 @@ -290,12 +284,8 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * return DC_STATUS_IO; } - // Set the serial communication protocol (256000 8N1). - if (model == NEMOWIDE2 || model == MATRIX || model == PUCKPRO || model == PUCK2) { - rc = serial_configure (device->port, 115200, 8, SERIAL_PARITY_EVEN, 1, SERIAL_FLOWCONTROL_NONE); - } else { - rc = serial_configure (device->port, BAUDRATE, 8, SERIAL_PARITY_NONE, 1, SERIAL_FLOWCONTROL_NONE); - } + // Set the serial communication protocol (115200 8E1). + rc = serial_configure (device->port, 115200, 8, SERIAL_PARITY_EVEN, 1, SERIAL_FLOWCONTROL_NONE); if (rc == -1) { ERROR (context, "Failed to set the terminal attributes."); serial_close (device->port); From c718abbe06280c33624a611dd9bf727df79c9525 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 3 Feb 2014 13:44:37 +0100 Subject: [PATCH 2/5] Use the same packet size for all models. The recommended packet size is 256 bytes, which matches the maximal amount of data that can be read back from the internal memory chip. Larger requests are split by the firmware into multiple blocks of maximum 256 bytes. Note that initially, we already used 256 byte packets for the newer models, but it seems this was accidentally changed to only 64 bytes in commit ad0e187c0c932121efa50763afc02771633cbd61. The main benefit of this change is a simplification of the code, because now there is no longer a difference between the Icon HD and Matrix variant of the protocol. --- src/mares_iconhd.c | 116 +++++++++++++++------------------------------ 1 file changed, 39 insertions(+), 77 deletions(-) diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index 52305c1..18c1fab 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -47,6 +47,8 @@ #define ACK 0xAA #define EOF 0xEA +#define SZ_PACKET 256 + typedef struct mares_iconhd_layout_t { unsigned int memsize; unsigned int rb_profile_begin; @@ -59,7 +61,6 @@ typedef struct mares_iconhd_device_t { const mares_iconhd_layout_t *layout; unsigned char fingerprint[10]; unsigned char version[140]; - unsigned int packetsize; } mares_iconhd_device_t; static dc_status_t mares_iconhd_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -115,8 +116,7 @@ mares_iconhd_get_model (mares_iconhd_device_t *device, unsigned int model) static dc_status_t mares_iconhd_transfer (mares_iconhd_device_t *device, const unsigned char command[], unsigned int csize, - unsigned char answer[], unsigned int asize, - dc_event_progress_t *progress) + unsigned char answer[], unsigned int asize) { dc_device_t *abstract = (dc_device_t *) device; @@ -155,34 +155,11 @@ mares_iconhd_transfer (mares_iconhd_device_t *device, } } - unsigned int nbytes = 0; - while (nbytes < asize) { - // Set the minimum packet size. - unsigned int len = 1024; - - // Increase the packet size if more data is immediately available. - int available = serial_get_received (device->port); - if (available > len) - len = available; - - // Limit the packet size to the total size. - if (nbytes + len > asize) - len = asize - nbytes; - - // Read the packet. - n = serial_read (device->port, answer + nbytes, len); - if (n != len) { - ERROR (abstract->context, "Failed to receive the answer."); - return EXITCODE (n); - } - - // Update and emit a progress event. - if (progress) { - progress->current += len; - device_event_emit (abstract, DC_EVENT_PROGRESS, progress); - } - - nbytes += len; + // Read the packet. + n = serial_read (device->port, answer, asize); + if (n != asize) { + ERROR (abstract->context, "Failed to receive the answer."); + return EXITCODE (n); } // Receive the trailer byte. @@ -207,44 +184,10 @@ static dc_status_t mares_iconhd_version (mares_iconhd_device_t *device, unsigned char data[], unsigned int size) { unsigned char command[] = {0xC2, 0x67}; - return mares_iconhd_transfer (device, command, sizeof (command), data, size, NULL); + return mares_iconhd_transfer (device, command, sizeof (command), data, size); } -static dc_status_t -mares_iconhd_read (mares_iconhd_device_t *device, unsigned int address, unsigned char data[], unsigned int size, dc_event_progress_t *progress) -{ - dc_status_t rc = DC_STATUS_SUCCESS; - - unsigned int nbytes = 0; - while (nbytes < size) { - // Calculate the packet size. - unsigned int len = size - nbytes; - if (device->packetsize && len > device->packetsize) - len = device->packetsize; - - // Read the packet. - unsigned char command[] = {0xE7, 0x42, - (address ) & 0xFF, - (address >> 8) & 0xFF, - (address >> 16) & 0xFF, - (address >> 24) & 0xFF, - (len ) & 0xFF, - (len >> 8) & 0xFF, - (len >> 16) & 0xFF, - (len >> 24) & 0xFF}; - rc = mares_iconhd_transfer (device, command, sizeof (command), data, len, progress); - if (rc != DC_STATUS_SUCCESS) - return rc; - - nbytes += len; - address += len; - data += len; - } - - return rc; -} - dc_status_t mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char *name, unsigned int model) { @@ -267,13 +210,10 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * memset (device->version, 0, sizeof (device->version)); if (model == NEMOWIDE2 || model == MATRIX || model == PUCKPRO || model == PUCK2) { device->layout = &mares_matrix_layout; - device->packetsize = 64; } else if (model == ICONHDNET) { device->layout = &mares_iconhdnet_layout; - device->packetsize = 0; } else { device->layout = &mares_iconhd_layout; - device->packetsize = 0; } // Open the device. @@ -366,9 +306,36 @@ mares_iconhd_device_set_fingerprint (dc_device_t *abstract, const unsigned char static dc_status_t mares_iconhd_device_read (dc_device_t *abstract, unsigned int address, unsigned char data[], unsigned int size) { + dc_status_t rc = DC_STATUS_SUCCESS; mares_iconhd_device_t *device = (mares_iconhd_device_t *) abstract; - return mares_iconhd_read (device, address, data, size, NULL); + unsigned int nbytes = 0; + while (nbytes < size) { + // Calculate the packet size. + unsigned int len = size - nbytes; + if (len > SZ_PACKET) + len = SZ_PACKET; + + // Read the packet. + unsigned char command[] = {0xE7, 0x42, + (address ) & 0xFF, + (address >> 8) & 0xFF, + (address >> 16) & 0xFF, + (address >> 24) & 0xFF, + (len ) & 0xFF, + (len >> 8) & 0xFF, + (len >> 16) & 0xFF, + (len >> 24) & 0xFF}; + rc = mares_iconhd_transfer (device, command, sizeof (command), data, len); + if (rc != DC_STATUS_SUCCESS) + return rc; + + nbytes += len; + address += len; + data += len; + } + + return rc; } @@ -384,19 +351,14 @@ mares_iconhd_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) return DC_STATUS_NOMEMORY; } - // Enable progress notifications. - dc_event_progress_t progress = EVENT_PROGRESS_INITIALIZER; - progress.maximum = device->layout->memsize; - device_event_emit (abstract, DC_EVENT_PROGRESS, &progress); - // Emit a vendor event. dc_event_vendor_t vendor; vendor.data = device->version; vendor.size = sizeof (device->version); device_event_emit (abstract, DC_EVENT_VENDOR, &vendor); - return mares_iconhd_read (device, 0, dc_buffer_get_data (buffer), - dc_buffer_get_size (buffer), &progress); + return device_dump_read (abstract, dc_buffer_get_data (buffer), + dc_buffer_get_size (buffer), SZ_PACKET); } From 2049bdb836d446822666de287701c3e1021feaa6 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 3 Feb 2014 20:55:23 +0100 Subject: [PATCH 3/5] Remove an unnecessary function. --- src/mares_iconhd.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index 18c1fab..bb06656 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -180,14 +180,6 @@ mares_iconhd_transfer (mares_iconhd_device_t *device, } -static dc_status_t -mares_iconhd_version (mares_iconhd_device_t *device, unsigned char data[], unsigned int size) -{ - unsigned char command[] = {0xC2, 0x67}; - return mares_iconhd_transfer (device, command, sizeof (command), data, size); -} - - dc_status_t mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char *name, unsigned int model) { @@ -255,7 +247,9 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * serial_flush (device->port, SERIAL_QUEUE_BOTH); // Send the version command. - dc_status_t status = mares_iconhd_version (device, device->version, sizeof (device->version)); + unsigned char command[] = {0xC2, 0x67}; + dc_status_t status = mares_iconhd_transfer (device, command, sizeof (command), + device->version, sizeof (device->version)); if (status != DC_STATUS_SUCCESS) { serial_close (device->port); free (device); From 991d0180d8d94e89d256312be129098c2b57e984 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Tue, 4 Feb 2014 10:38:50 +0100 Subject: [PATCH 4/5] Autodetect the model using the version packet. The model number stored in the main memory isn't always the most reliable source of information, because there are devices where the model number has not been filled in properly. Instead, we check the product name in the version packet against the list with valid names, and return the corresponding model number. As an additional advantage, we no longer depend on the model number supplied by the application for selecting the correct memory layout. Nevertheless, the model parameter is kept for backwards compatibility. --- src/mares_iconhd.c | 66 ++++++++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 17 deletions(-) diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index bb06656..bd4867c 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -30,6 +30,8 @@ #include "serial.h" #include "array.h" +#define C_ARRAY_SIZE(array) (sizeof (array) / sizeof *(array)) + #define ISINSTANCE(device) dc_device_isinstance((device), &mares_iconhd_device_vtable) #define EXITCODE(rc) \ @@ -55,12 +57,18 @@ typedef struct mares_iconhd_layout_t { unsigned int rb_profile_end; } mares_iconhd_layout_t; +typedef struct mares_iconhd_model_t { + unsigned char name[16 + 1]; + unsigned int id; +} mares_iconhd_model_t; + typedef struct mares_iconhd_device_t { dc_device_t base; serial_t *port; const mares_iconhd_layout_t *layout; unsigned char fingerprint[10]; unsigned char version[140]; + unsigned int model; } mares_iconhd_device_t; static dc_status_t mares_iconhd_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -98,16 +106,25 @@ static const mares_iconhd_layout_t mares_matrix_layout = { }; static unsigned int -mares_iconhd_get_model (mares_iconhd_device_t *device, unsigned int model) +mares_iconhd_get_model (mares_iconhd_device_t *device) { - dc_context_t *context = (device ? ((dc_device_t *) device)->context : NULL); + const mares_iconhd_model_t models[] = { + {"Matrix", MATRIX}, + {"Icon HD", ICONHD}, + {"Icon AIR", ICONHDNET}, + {"Puck Pro", PUCKPRO}, + {"Nemo Wide 2", NEMOWIDE2}, + {"Puck 2", PUCK2}, + }; - // Try to correct an invalid model code using the version packet. - if (model == 0xFF) { - WARNING (context, "Invalid model code detected!"); - const unsigned char iconhdnet[] = "Icon AIR"; - if (device && memcmp (device->version + 0x46, iconhdnet, sizeof (iconhdnet) - 1) == 0) - model = ICONHDNET; + // Check the product name in the version packet against the list + // with valid names, and return the corresponding model number. + unsigned int model = 0; + for (unsigned int i = 0; i < C_ARRAY_SIZE(models); ++i) { + if (memcmp (device->version + 0x46, models[i].name, sizeof (models[i].name) - 1) == 0) { + model = models[i].id; + break; + } } return model; @@ -198,15 +215,10 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * // Set the default values. device->port = NULL; + device->layout = NULL; memset (device->fingerprint, 0, sizeof (device->fingerprint)); memset (device->version, 0, sizeof (device->version)); - if (model == NEMOWIDE2 || model == MATRIX || model == PUCKPRO || model == PUCK2) { - device->layout = &mares_matrix_layout; - } else if (model == ICONHDNET) { - device->layout = &mares_iconhdnet_layout; - } else { - device->layout = &mares_iconhd_layout; - } + device->model = 0; // Open the device. int rc = serial_open (&device->port, context, name); @@ -256,6 +268,26 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * return status; } + // Autodetect the model using the version packet. + device->model = mares_iconhd_get_model (device); + + // Load the correct memory layout. + switch (device->model) { + case MATRIX: + case PUCKPRO: + case NEMOWIDE2: + case PUCK2: + device->layout = &mares_matrix_layout; + break; + case ICONHDNET: + device->layout = &mares_iconhdnet_layout; + break; + case ICONHD: + default: + device->layout = &mares_iconhd_layout; + break; + } + *out = (dc_device_t *) device; return DC_STATUS_SUCCESS; @@ -374,7 +406,7 @@ mares_iconhd_device_foreach (dc_device_t *abstract, dc_dive_callback_t callback, // Emit a device info event. unsigned char *data = dc_buffer_get_data (buffer); dc_event_devinfo_t devinfo; - devinfo.model = mares_iconhd_get_model (device, data[0]); + devinfo.model = device->model; devinfo.firmware = 0; devinfo.serial = array_uint32_le (data + 0x0C); device_event_emit (abstract, DC_EVENT_DEVINFO, &devinfo); @@ -403,7 +435,7 @@ mares_iconhd_extract_dives (dc_device_t *abstract, const unsigned char data[], u return DC_STATUS_DATAFORMAT; // Get the model code. - unsigned int model = mares_iconhd_get_model (device, data[0]); + unsigned int model = device ? device->model : data[0]; // Get the corresponding dive header size. unsigned int header = 0x5C; From 9394773cd25b58ae066c0b367f67a01c632ef273 Mon Sep 17 00:00:00 2001 From: Jef Driesen Date: Mon, 17 Feb 2014 10:27:26 +0100 Subject: [PATCH 5/5] Increase the packet size again for the Icon HD. The switch from downloading the entire data with a single large packet to multiple smaller 256 byte packets, resulted in a considerable performance regression. In one particular case, the difference was a factor 6.7 slower! I performed a small tests (using an Icon HD Net Ready) with 256, 1024 and 4096 byte packets, and the total time was respectively 21.0, 11.3 and 6.5 seconds. For a single large packet, the total time is only 5.9 seconds. Thus the difference with a 4096 byte packet is negligible. --- src/mares_iconhd.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/mares_iconhd.c b/src/mares_iconhd.c index bd4867c..e3009a4 100644 --- a/src/mares_iconhd.c +++ b/src/mares_iconhd.c @@ -49,8 +49,6 @@ #define ACK 0xAA #define EOF 0xEA -#define SZ_PACKET 256 - typedef struct mares_iconhd_layout_t { unsigned int memsize; unsigned int rb_profile_begin; @@ -69,6 +67,7 @@ typedef struct mares_iconhd_device_t { unsigned char fingerprint[10]; unsigned char version[140]; unsigned int model; + unsigned int packetsize; } mares_iconhd_device_t; static dc_status_t mares_iconhd_device_set_fingerprint (dc_device_t *abstract, const unsigned char data[], unsigned int size); @@ -219,6 +218,7 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * memset (device->fingerprint, 0, sizeof (device->fingerprint)); memset (device->version, 0, sizeof (device->version)); device->model = 0; + device->packetsize = 0; // Open the device. int rc = serial_open (&device->port, context, name); @@ -278,13 +278,16 @@ mares_iconhd_device_open (dc_device_t **out, dc_context_t *context, const char * case NEMOWIDE2: case PUCK2: device->layout = &mares_matrix_layout; + device->packetsize = 256; break; case ICONHDNET: device->layout = &mares_iconhdnet_layout; + device->packetsize = 4096; break; case ICONHD: default: device->layout = &mares_iconhd_layout; + device->packetsize = 4096; break; } @@ -339,8 +342,8 @@ mares_iconhd_device_read (dc_device_t *abstract, unsigned int address, unsigned while (nbytes < size) { // Calculate the packet size. unsigned int len = size - nbytes; - if (len > SZ_PACKET) - len = SZ_PACKET; + if (len > device->packetsize) + len = device->packetsize; // Read the packet. unsigned char command[] = {0xE7, 0x42, @@ -384,7 +387,7 @@ mares_iconhd_device_dump (dc_device_t *abstract, dc_buffer_t *buffer) device_event_emit (abstract, DC_EVENT_VENDOR, &vendor); return device_dump_read (abstract, dc_buffer_get_data (buffer), - dc_buffer_get_size (buffer), SZ_PACKET); + dc_buffer_get_size (buffer), device->packetsize); }