From: Aurelien J. <au...@gn...> - 2014-02-04 22:57:50
|
Hello, Here is a bunch of patches, mosty SCPI (and Rigol) related. It mainly adds a scan API to the SCPI layer, and add a libusb based USBTMC implementation. Comments welcome. [PATCH 01/11] rigol-ds: properly deal with dev_close() getting called [PATCH 02/11] rigol-ds: properly end WAIT_TRIGGER event handling [PATCH 03/11] rigol-ds: apply :KEY:LOCK DISABLE only to DS1K [PATCH 04/11] add helper macro to read 8 bits integer, matching the [PATCH 05/11] endian neutral helper macro to write 8/16/32 bits [PATCH 06/11] scpi: add a struct drv_context parameter to [PATCH 07/11] scpi: add a generic scan API and implement it in usbtmc [PATCH 08/11] rigol-ds: use the new scpi scan API [PATCH 09/11] hameg-hmo: use the new scpi scan API [PATCH 10/11] scpi: add a libusb based implementation of usbtmc [PATCH 11/11] usb: remove unused sr_usb_find_usbtmc() |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:57:58
|
--- hardware/rigol-ds/api.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/hardware/rigol-ds/api.c b/hardware/rigol-ds/api.c index f61d58c..ae14f06 100644 --- a/hardware/rigol-ds/api.c +++ b/hardware/rigol-ds/api.c @@ -482,8 +482,8 @@ static int dev_close(struct sr_dev_inst *sdi) struct sr_scpi_dev_inst *scpi; struct dev_context *devc; - if (sdi->status != SR_ST_INACTIVE) - return SR_OK; + if (sdi->status != SR_ST_ACTIVE) + return SR_ERR_DEV_CLOSED; scpi = sdi->conn; devc = sdi->priv; -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:05
|
--- hardware/rigol-ds/protocol.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/rigol-ds/protocol.c b/hardware/rigol-ds/protocol.c index 3e6a5be..d1c568c 100644 --- a/hardware/rigol-ds/protocol.c +++ b/hardware/rigol-ds/protocol.c @@ -505,7 +505,7 @@ SR_PRIV int rigol_ds_receive(int fd, int revents, void *cb_data) return TRUE; if (rigol_ds_channel_start(sdi) != SR_OK) return TRUE; - break; + return TRUE; case WAIT_BLOCK: if (rigol_ds_block_wait(sdi) != SR_OK) return TRUE; -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:12
|
--- hardware/rigol-ds/api.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/hardware/rigol-ds/api.c b/hardware/rigol-ds/api.c index ae14f06..706391f 100644 --- a/hardware/rigol-ds/api.c +++ b/hardware/rigol-ds/api.c @@ -488,7 +488,7 @@ static int dev_close(struct sr_dev_inst *sdi) scpi = sdi->conn; devc = sdi->priv; - if (devc->model->series->protocol >= PROTOCOL_V2) + if (devc->model->series->protocol == PROTOCOL_V2) rigol_ds_config_set(sdi, ":KEY:LOCK DISABLE"); if (scpi) { -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:19
|
--- libsigrok-internal.h | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/libsigrok-internal.h b/libsigrok-internal.h index aa8283f..05d3f2a 100644 --- a/libsigrok-internal.h +++ b/libsigrok-internal.h @@ -51,6 +51,13 @@ #endif /** + * Read a 8 bits integer out of memory. + * @param x a pointer to the input memory + * @return the corresponding integer + */ +#define R8(x) ((unsigned)((const uint8_t*)(x))[0]) + +/** * Read a 16 bits big endian integer out of memory. * @param x a pointer to the input memory * @return the corresponding integer -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:27
|
--- libsigrok-internal.h | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/libsigrok-internal.h b/libsigrok-internal.h index 05d3f2a..6e665c5 100644 --- a/libsigrok-internal.h +++ b/libsigrok-internal.h @@ -93,6 +93,49 @@ ((unsigned)((const uint8_t*)(x))[1] << 8) | \ (unsigned)((const uint8_t*)(x))[0]) +/** + * Write a 8 bits integer to memory. + * @param p a pointer to the output memory + * @param x the input integer + */ +#define W8(p, x) do { ((uint8_t*)(p))[0] = (uint8_t) (x); } while(0) + +/** + * Write a 16 bits integer to memory stored as big endian. + * @param p a pointer to the output memory + * @param x the input integer + */ +#define WB16(p, x) do { ((uint8_t*)(p))[1] = (uint8_t) (x); \ + ((uint8_t*)(p))[0] = (uint8_t)((x)>>8); } while(0) + +/** + * Write a 16 bits integer to memory stored as little endian. + * @param p a pointer to the output memory + * @param x the input integer + */ +#define WL16(p, x) do { ((uint8_t*)(p))[0] = (uint8_t) (x); \ + ((uint8_t*)(p))[1] = (uint8_t)((x)>>8); } while(0) + +/** + * Write a 32 bits integer to memory stored as big endian. + * @param p a pointer to the output memory + * @param x the input integer + */ +#define WB32(p, x) do { ((uint8_t*)(p))[3] = (uint8_t) (x); \ + ((uint8_t*)(p))[2] = (uint8_t)((x)>>8); \ + ((uint8_t*)(p))[1] = (uint8_t)((x)>>16); \ + ((uint8_t*)(p))[0] = (uint8_t)((x)>>24); } while(0) + +/** + * Write a 32 bits integer to memory stored as little endian. + * @param p a pointer to the output memory + * @param x the input integer + */ +#define WL32(p, x) do { ((uint8_t*)(p))[0] = (uint8_t) (x); \ + ((uint8_t*)(p))[1] = (uint8_t)((x)>>8); \ + ((uint8_t*)(p))[2] = (uint8_t)((x)>>16); \ + ((uint8_t*)(p))[3] = (uint8_t)((x)>>24); } while(0) + /* Portability fixes for FreeBSD. */ #ifdef __FreeBSD__ #define LIBUSB_CLASS_APPLICATION 0xfe -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:34
|
--- hardware/common/scpi.c | 6 +++--- hardware/common/scpi_serial.c | 5 +++-- hardware/common/scpi_tcp.c | 5 +++-- hardware/common/scpi_usbtmc.c | 5 +++-- hardware/common/scpi_vxi.c | 5 +++-- hardware/hameg-hmo/api.c | 2 +- hardware/rigol-ds/api.c | 2 +- libsigrok-internal.h | 8 ++++---- 8 files changed, 21 insertions(+), 17 deletions(-) diff --git a/hardware/common/scpi.c b/hardware/common/scpi.c index fc61855..2307a8b 100644 --- a/hardware/common/scpi.c +++ b/hardware/common/scpi.c @@ -87,8 +87,8 @@ static const struct sr_scpi_dev_inst *scpi_devs[] = { #endif }; -SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(const char *resource, - const char *serialcomm) +SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(struct drv_context *drvc, + const char *resource, const char *serialcomm) { struct sr_scpi_dev_inst *scpi = NULL; const struct sr_scpi_dev_inst *scpi_dev; @@ -103,7 +103,7 @@ SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(const char *resource, *scpi = *scpi_dev; scpi->priv = g_malloc0(scpi->priv_size); params = g_strsplit(resource, "/", 0); - if (scpi->dev_inst_new(scpi->priv, resource, + if (scpi->dev_inst_new(scpi->priv, drvc, resource, params, serialcomm) != SR_OK) { sr_scpi_free(scpi); scpi = NULL; diff --git a/hardware/common/scpi_serial.c b/hardware/common/scpi_serial.c index cf962af..6e73c89 100644 --- a/hardware/common/scpi_serial.c +++ b/hardware/common/scpi_serial.c @@ -35,11 +35,12 @@ struct scpi_serial { size_t read; }; -static int scpi_serial_dev_inst_new(void *priv, const char *resource, - char **params, const char *serialcomm) +static int scpi_serial_dev_inst_new(void *priv, struct drv_context *drvc, + const char *resource, char **params, const char *serialcomm) { struct scpi_serial *sscpi = priv; + (void)drvc; (void)params; if (!(sscpi->serial = sr_serial_dev_inst_new(resource, serialcomm))) diff --git a/hardware/common/scpi_tcp.c b/hardware/common/scpi_tcp.c index 796ea9a..21235f3 100644 --- a/hardware/common/scpi_tcp.c +++ b/hardware/common/scpi_tcp.c @@ -51,11 +51,12 @@ struct scpi_tcp { int response_bytes_read; }; -static int scpi_tcp_dev_inst_new(void *priv, const char *resource, - char **params, const char *serialcomm) +static int scpi_tcp_dev_inst_new(void *priv, struct drv_context *drvc, + const char *resource, char **params, const char *serialcomm) { struct scpi_tcp *tcp = priv; + (void)drvc; (void)resource; (void)serialcomm; diff --git a/hardware/common/scpi_usbtmc.c b/hardware/common/scpi_usbtmc.c index 75fcf3a..6cd7964 100644 --- a/hardware/common/scpi_usbtmc.c +++ b/hardware/common/scpi_usbtmc.c @@ -37,11 +37,12 @@ struct usbtmc_scpi { int response_bytes_read; }; -static int scpi_usbtmc_dev_inst_new(void *priv, const char *resource, - char **params, const char *serialcomm) +static int scpi_usbtmc_dev_inst_new(void *priv, struct drv_context *drvc, + const char *resource, char **params, const char *serialcomm) { struct usbtmc_scpi *uscpi = priv; + (void)drvc; (void)params; (void)serialcomm; diff --git a/hardware/common/scpi_vxi.c b/hardware/common/scpi_vxi.c index 0377d52..4bdb6ab 100644 --- a/hardware/common/scpi_vxi.c +++ b/hardware/common/scpi_vxi.c @@ -37,11 +37,12 @@ struct scpi_vxi { unsigned int read_complete; }; -static int scpi_vxi_dev_inst_new(void *priv, const char *resource, - char **params, const char *serialcomm) +static int scpi_vxi_dev_inst_new(void *priv, struct drv_context *drvc, + const char *resource, char **params, const char *serialcomm) { struct scpi_vxi *vxi = priv; + (void)drvc; (void)resource; (void)serialcomm; diff --git a/hardware/hameg-hmo/api.c b/hardware/hameg-hmo/api.c index b63ee51..5abd8c5 100644 --- a/hardware/hameg-hmo/api.c +++ b/hardware/hameg-hmo/api.c @@ -220,7 +220,7 @@ static struct sr_dev_inst *hmo_probe_serial_device(const char *serial_device, scpi = NULL; hw_info = NULL; - if (!(scpi = scpi_dev_inst_new(serial_device, serial_options))) + if (!(scpi = scpi_dev_inst_new(di->priv, serial_device, serial_options))) goto fail; sr_info("Probing %s.", serial_device); diff --git a/hardware/rigol-ds/api.c b/hardware/rigol-ds/api.c index 706391f..0f319cb 100644 --- a/hardware/rigol-ds/api.c +++ b/hardware/rigol-ds/api.c @@ -269,7 +269,7 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev *devices = NULL; - if (!(scpi = scpi_dev_inst_new(resource, serialcomm))) + if (!(scpi = scpi_dev_inst_new(di->priv, resource, serialcomm))) return SR_ERR; if (sr_scpi_open(scpi) != SR_OK) { diff --git a/libsigrok-internal.h b/libsigrok-internal.h index 6e665c5..951d91d 100644 --- a/libsigrok-internal.h +++ b/libsigrok-internal.h @@ -438,8 +438,8 @@ struct sr_scpi_dev_inst { const char *name; const char *prefix; int priv_size; - int (*dev_inst_new)(void *priv, const char *resource, char **params, - const char *serialcomm); + int (*dev_inst_new)(void *priv, struct drv_context *drvc, + const char *resource, char **params, const char *serialcomm); int (*open)(void *priv); int (*source_add)(void *priv, int events, int timeout, sr_receive_data_callback_t cb, void *cb_data); @@ -453,8 +453,8 @@ struct sr_scpi_dev_inst { void *priv; }; -SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(const char *resource, - const char *serialcomm); +SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(struct drv_context *drvc, + const char *resource, const char *serialcomm); SR_PRIV int sr_scpi_open(struct sr_scpi_dev_inst *scpi); SR_PRIV int sr_scpi_source_add(struct sr_scpi_dev_inst *scpi, int events, int timeout, sr_receive_data_callback_t cb, void *cb_data); -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:42
|
note that sr_usb_find_serial() is copied from the hameg-hmo driver --- hardware/common/scpi.c | 68 +++++++++++++++++ hardware/common/scpi_serial.c | 167 ++++++++++++++++++++++++++++++++++++++++++ hardware/common/scpi_usbtmc.c | 24 ++++++ libsigrok-internal.h | 3 + 4 files changed, 262 insertions(+) diff --git a/hardware/common/scpi.c b/hardware/common/scpi.c index 2307a8b..21ff178 100644 --- a/hardware/common/scpi.c +++ b/hardware/common/scpi.c @@ -87,6 +87,74 @@ static const struct sr_scpi_dev_inst *scpi_devs[] = { #endif }; +static GSList *sr_scpi_scan_resource(struct drv_context *drvc, + const char *resource, const char *serialcomm, + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)) +{ + struct sr_scpi_dev_inst *scpi; + struct sr_dev_inst *sdi; + + if (!(scpi = scpi_dev_inst_new(drvc, resource, serialcomm))) + return NULL; + + if (sr_scpi_open(scpi) != SR_OK) { + sr_info("Couldn't open SCPI device."); + sr_scpi_free(scpi); + return NULL; + }; + + if ((sdi = probe_device(scpi))) + return g_slist_append(NULL, sdi); //FIXME list_new !!! + + sr_scpi_close(scpi); + sr_scpi_free(scpi); + return NULL; +} + +SR_PRIV GSList *sr_scpi_scan(struct drv_context *drvc, GSList *options, + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)) +{ + GSList *resources, *l, *d, *devices = NULL; + const char *resource = NULL; + const char *serialcomm = NULL; + unsigned i; + + for (l = options; l; l = l->next) { + struct sr_config *src = l->data; + switch (src->key) { + case SR_CONF_CONN: + resource = g_variant_get_string(src->data, NULL); + break; + case SR_CONF_SERIALCOMM: + serialcomm = g_variant_get_string(src->data, NULL); + break; + } + } + + for (i = 0; i < ARRAY_SIZE(scpi_devs); i++) { + if ((resource && strcmp(resource, scpi_devs[i]->prefix)) + || !scpi_devs[i]->scan) + continue; + resources = scpi_devs[i]->scan(drvc); + for (l = resources; l; l = l->next) + if ((d = sr_scpi_scan_resource(drvc, l->data, serialcomm, + probe_device))) + devices = g_slist_concat(devices, d); + g_slist_free_full(resources, g_free); + } + + if (!devices && resource) + devices = sr_scpi_scan_resource(drvc, resource, serialcomm, + probe_device); + + /* Tack a copy of the newly found devices onto the driver list. */ + if (devices) + drvc->instances = g_slist_concat(drvc->instances, + g_slist_copy(devices)); + + return devices; +} + SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(struct drv_context *drvc, const char *resource, const char *serialcomm) { diff --git a/hardware/common/scpi_serial.c b/hardware/common/scpi_serial.c index 6e73c89..9ab9e16 100644 --- a/hardware/common/scpi_serial.c +++ b/hardware/common/scpi_serial.c @@ -22,6 +22,8 @@ #include "libsigrok-internal.h" #include <glib.h> +#include <glib/gstdio.h> +#include <stdlib.h> #include <string.h> #define LOG_PREFIX "scpi_serial" @@ -35,6 +37,170 @@ struct scpi_serial { size_t read; }; +static struct { + uint16_t vendor_id; + uint16_t product_id; +} scpi_serial_usb_ids[] = { + { 0x0403, 0xed72 }, /* Hameg HO720 */ + { 0x0403, 0xed73 }, /* Hameg HO730 */ +}; + +/** + * Find USB serial devices via the USB vendor ID and product ID. + * + * @param vendor_id Vendor ID of the USB device. + * @param product_id Product ID of the USB device. + * + * @return A GSList of strings containing the path of the serial device or + * NULL if no serial device is found. The returned list must be freed + * by the caller. + */ +static GSList *sr_usb_find_serial(uint16_t vendor_id, uint16_t product_id) +{ +#ifdef __linux__ + const gchar *usb_dev; + const char device_tree[] = "/sys/bus/usb/devices/"; + GDir *devices_dir, *device_dir; + GSList *l = NULL; + GSList *tty_devs; + GSList *matched_paths; + FILE *fd; + char tmp[5]; + gchar *vendor_path, *product_path, *path_copy; + gchar *prefix, *subdir_path, *device_path, *tty_path; + unsigned long read_vendor_id, read_product_id; + const char *file; + + l = NULL; + tty_devs = NULL; + matched_paths = NULL; + + if (!(devices_dir = g_dir_open(device_tree, 0, NULL))) + return NULL; + + /* + * Find potential candidates using the vendor ID and product ID + * and store them in matched_paths. + */ + while ((usb_dev = g_dir_read_name(devices_dir))) { + vendor_path = g_strconcat(device_tree, + usb_dev, "/idVendor", NULL); + product_path = g_strconcat(device_tree, + usb_dev, "/idProduct", NULL); + + if (!g_file_test(vendor_path, G_FILE_TEST_EXISTS) || + !g_file_test(product_path, G_FILE_TEST_EXISTS)) + goto skip_device; + + if ((fd = g_fopen(vendor_path, "r")) == NULL) + goto skip_device; + + if (fgets(tmp, sizeof(tmp), fd) == NULL) { + fclose(fd); + goto skip_device; + } + read_vendor_id = strtoul(tmp, NULL, 16); + + fclose(fd); + + if ((fd = g_fopen(product_path, "r")) == NULL) + goto skip_device; + + if (fgets(tmp, sizeof(tmp), fd) == NULL) { + fclose(fd); + goto skip_device; + } + read_product_id = strtoul(tmp, NULL, 16); + + fclose(fd); + + if (vendor_id == read_vendor_id && + product_id == read_product_id) { + path_copy = g_strdup(usb_dev); + matched_paths = g_slist_prepend(matched_paths, + path_copy); + } + +skip_device: + g_free(vendor_path); + g_free(product_path); + } + g_dir_close(devices_dir); + + /* For every matched device try to find a ttyUSBX subfolder. */ + for (l = matched_paths; l; l = l->next) { + subdir_path = NULL; + + device_path = g_strconcat(device_tree, l->data, NULL); + + if (!(device_dir = g_dir_open(device_path, 0, NULL))) { + g_free(device_path); + continue; + } + + prefix = g_strconcat(l->data, ":", NULL); + + while ((file = g_dir_read_name(device_dir))) { + if (g_str_has_prefix(file, prefix)) { + subdir_path = g_strconcat(device_path, + "/", file, NULL); + break; + } + } + g_dir_close(device_dir); + + g_free(prefix); + g_free(device_path); + + if (subdir_path) { + if (!(device_dir = g_dir_open(subdir_path, 0, NULL))) { + g_free(subdir_path); + continue; + } + g_free(subdir_path); + + while ((file = g_dir_read_name(device_dir))) { + if (g_str_has_prefix(file, "ttyUSB")) { + tty_path = g_strconcat("/dev/", + file, NULL); + sr_dbg("Found USB device %04x:%04x attached to %s.", + vendor_id, product_id, tty_path); + tty_devs = g_slist_prepend(tty_devs, + tty_path); + break; + } + } + g_dir_close(device_dir); + } + } + g_slist_free_full(matched_paths, g_free); + + return tty_devs; +#else + (void)vendor_id; + (void)product_id; + + return NULL; +#endif +} + +static GSList *scpi_serial_scan(struct drv_context *drvc) +{ + GSList *l, *resources = NULL; + unsigned i; + + (void)drvc; + + for (i = 0; i < ARRAY_SIZE(scpi_serial_usb_ids); i++) { + if ((l = sr_usb_find_serial(scpi_serial_usb_ids[i].vendor_id, + scpi_serial_usb_ids[i].product_id)) == NULL) + continue; + resources = g_slist_concat(resources, l); + } + + return resources; +} + static int scpi_serial_dev_inst_new(void *priv, struct drv_context *drvc, const char *resource, char **params, const char *serialcomm) { @@ -190,6 +356,7 @@ SR_PRIV const struct sr_scpi_dev_inst scpi_serial_dev = { .name = "serial", .prefix = "", .priv_size = sizeof(struct scpi_serial), + .scan = scpi_serial_scan, .dev_inst_new = scpi_serial_dev_inst_new, .open = scpi_serial_open, .source_add = scpi_serial_source_add, diff --git a/hardware/common/scpi_usbtmc.c b/hardware/common/scpi_usbtmc.c index 6cd7964..1ffc34a 100644 --- a/hardware/common/scpi_usbtmc.c +++ b/hardware/common/scpi_usbtmc.c @@ -37,6 +37,29 @@ struct usbtmc_scpi { int response_bytes_read; }; +static GSList *scpi_usbtmc_scan(struct drv_context *drvc) +{ + GSList *resources = NULL; + GDir *dir; + const char *dev_name; + char *resource; + + (void)drvc; + + if (!(dir = g_dir_open("/sys/class/usbmisc/", 0, NULL))) + if (!(dir = g_dir_open("/sys/class/usb/", 0, NULL))) + return NULL; + while ((dev_name = g_dir_read_name(dir))) { + if (strncmp(dev_name, "usbtmc", 6)) + continue; + resource = g_strconcat("/dev/", dev_name, NULL); + resources = g_slist_append(resources, resource); + } + g_dir_close(dir); + + return resources; +} + static int scpi_usbtmc_dev_inst_new(void *priv, struct drv_context *drvc, const char *resource, char **params, const char *serialcomm) { @@ -190,6 +213,7 @@ SR_PRIV const struct sr_scpi_dev_inst scpi_usbtmc_dev = { .name = "USBTMC", .prefix = "/dev/usbtmc", .priv_size = sizeof(struct usbtmc_scpi), + .scan = scpi_usbtmc_scan, .dev_inst_new = scpi_usbtmc_dev_inst_new, .open = scpi_usbtmc_open, .source_add = scpi_usbtmc_source_add, diff --git a/libsigrok-internal.h b/libsigrok-internal.h index 951d91d..c84403a 100644 --- a/libsigrok-internal.h +++ b/libsigrok-internal.h @@ -438,6 +438,7 @@ struct sr_scpi_dev_inst { const char *name; const char *prefix; int priv_size; + GSList *(*scan)(struct drv_context *drvc); int (*dev_inst_new)(void *priv, struct drv_context *drvc, const char *resource, char **params, const char *serialcomm); int (*open)(void *priv); @@ -453,6 +454,8 @@ struct sr_scpi_dev_inst { void *priv; }; +SR_PRIV GSList *sr_scpi_scan(struct drv_context *drvc, GSList *options, + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)); SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(struct drv_context *drvc, const char *resource, const char *serialcomm); SR_PRIV int sr_scpi_open(struct sr_scpi_dev_inst *scpi); -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:49
|
--- hardware/rigol-ds/api.c | 91 ++++++------------------------------------------- 1 file changed, 11 insertions(+), 80 deletions(-) diff --git a/hardware/rigol-ds/api.c b/hardware/rigol-ds/api.c index 0f319cb..1c0d9c4 100644 --- a/hardware/rigol-ds/api.c +++ b/hardware/rigol-ds/api.c @@ -255,11 +255,10 @@ static int init(struct sr_context *sr_ctx) return std_init(sr_ctx, di, LOG_PREFIX); } -static int probe_port(const char *resource, const char *serialcomm, GSList **devices) +static struct sr_dev_inst *probe_device(struct sr_scpi_dev_inst *scpi) { struct dev_context *devc; struct sr_dev_inst *sdi; - struct sr_scpi_dev_inst *scpi; struct sr_scpi_hw_info *hw_info; struct sr_probe *probe; long n[3]; @@ -267,22 +266,11 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev const struct rigol_ds_model *model = NULL; gchar *channel_name, **version; - *devices = NULL; - - if (!(scpi = scpi_dev_inst_new(di->priv, resource, serialcomm))) - return SR_ERR; - - if (sr_scpi_open(scpi) != SR_OK) { - sr_info("Couldn't open SCPI device."); - sr_scpi_free(scpi); - return SR_ERR; - }; - if (sr_scpi_get_hw_id(scpi, &hw_info) != SR_OK) { sr_info("Couldn't get IDN response."); sr_scpi_close(scpi); sr_scpi_free(scpi); - return SR_ERR; + return NULL; } for (i = 0; i < ARRAY_SIZE(supported_models); i++) { @@ -301,7 +289,7 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev sr_scpi_hw_info_free(hw_info); sr_scpi_close(scpi); sr_scpi_free(scpi); - return SR_ERR_NA; + return NULL; } sr_scpi_close(scpi); @@ -312,7 +300,7 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev sdi->inst_type = SR_INST_SCPI; if (!(devc = g_try_malloc0(sizeof(struct dev_context)))) - return SR_ERR_MALLOC; + return NULL; devc->limit_frames = 0; devc->model = model; @@ -346,7 +334,7 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev for (i = 0; i < model->analog_channels; i++) { if (!(channel_name = g_strdup_printf("CH%d", i + 1))) - return SR_ERR_MALLOC; + return NULL; probe = sr_probe_new(i, SR_PROBE_ANALOG, TRUE, channel_name); sdi->probes = g_slist_append(sdi->probes, probe); devc->analog_groups[i].name = channel_name; @@ -358,11 +346,11 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev if (devc->model->has_digital) { for (i = 0; i < 16; i++) { if (!(channel_name = g_strdup_printf("D%d", i))) - return SR_ERR_MALLOC; + return NULL; probe = sr_probe_new(i, SR_PROBE_LOGIC, TRUE, channel_name); g_free(channel_name); if (!probe) - return SR_ERR_MALLOC; + return NULL; sdi->probes = g_slist_append(sdi->probes, probe); devc->digital_group.probes = g_slist_append( devc->digital_group.probes, probe); @@ -384,77 +372,20 @@ static int probe_port(const char *resource, const char *serialcomm, GSList **dev devc->vdivs = &vdivs[i]; if (!(devc->buffer = g_try_malloc(ACQ_BUFFER_SIZE))) - return SR_ERR_MALLOC; + return NULL; if (!(devc->data = g_try_malloc(ACQ_BUFFER_SIZE * sizeof(float)))) - return SR_ERR_MALLOC; + return NULL; devc->data_source = DATA_SOURCE_LIVE; sdi->priv = devc; - *devices = g_slist_append(NULL, sdi); - - return SR_OK; + return sdi; } static GSList *scan(GSList *options) { - struct drv_context *drvc; - struct sr_config *src; - GSList *l, *devices; - GDir *dir; - int ret; - const gchar *dev_name; - gchar *port = NULL; - gchar *serialcomm = NULL; - - drvc = di->priv; - - for (l = options; l; l = l->next) { - src = l->data; - switch (src->key) { - case SR_CONF_CONN: - port = (char *)g_variant_get_string(src->data, NULL); - break; - case SR_CONF_SERIALCOMM: - serialcomm = (char *)g_variant_get_string(src->data, NULL); - break; - } - } - - devices = NULL; - if (port) { - if (probe_port(port, serialcomm, &devices) == SR_ERR_MALLOC) { - g_free(port); - if (serialcomm) - g_free(serialcomm); - return NULL; - } - } else { - if (!(dir = g_dir_open("/sys/class/usbmisc/", 0, NULL))) - if (!(dir = g_dir_open("/sys/class/usb/", 0, NULL))) - return NULL; - while ((dev_name = g_dir_read_name(dir))) { - if (strncmp(dev_name, "usbtmc", 6)) - continue; - port = g_strconcat("/dev/", dev_name, NULL); - ret = probe_port(port, serialcomm, &devices); - g_free(port); - if (serialcomm) - g_free(serialcomm); - if (ret == SR_ERR_MALLOC) { - g_dir_close(dir); - return NULL; - } - } - g_dir_close(dir); - } - - /* Tack a copy of the newly found devices onto the driver list. */ - l = g_slist_copy(devices); - drvc->instances = g_slist_concat(drvc->instances, l); - - return devices; + return sr_scpi_scan(di->priv, options, probe_device); } static GSList *dev_list(void) -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:58:57
|
--- hardware/hameg-hmo/api.c | 206 +---------------------------------------------- 1 file changed, 2 insertions(+), 204 deletions(-) diff --git a/hardware/hameg-hmo/api.c b/hardware/hameg-hmo/api.c index 5abd8c5..241e86b 100644 --- a/hardware/hameg-hmo/api.c +++ b/hardware/hameg-hmo/api.c @@ -18,7 +18,6 @@ */ #include <stdlib.h> -#include <glib/gstdio.h> #include "protocol.h" #define SERIALCOMM "115200/8n1/flow=1" @@ -35,16 +34,6 @@ static const int32_t hwopts[] = { SR_CONF_SERIALCOMM, }; -struct usb_id_info { - uint16_t vendor_id; - uint16_t product_id; -}; - -static struct usb_id_info ho_models[] = { - { 0x0403, 0xed72 }, /* HO720 */ - { 0x0403, 0xed73 }, /* HO730 */ -}; - enum { PG_INVALID = -1, PG_NONE, @@ -57,145 +46,6 @@ static int init(struct sr_context *sr_ctx) return std_init(sr_ctx, di, LOG_PREFIX); } -/** - * Find USB serial devices via the USB vendor ID and product ID. - * - * @param vendor_id Vendor ID of the USB device. - * @param product_id Product ID of the USB device. - * - * @return A GSList of strings containing the path of the serial device or - * NULL if no serial device is found. The returned list must be freed - * by the caller. - */ -static GSList *auto_find_usb(uint16_t vendor_id, uint16_t product_id) -{ -#ifdef __linux__ - const gchar *usb_dev; - const char device_tree[] = "/sys/bus/usb/devices/"; - GDir *devices_dir, *device_dir; - GSList *l = NULL; - GSList *tty_devs; - GSList *matched_paths; - FILE *fd; - char tmp[5]; - gchar *vendor_path, *product_path, *path_copy; - gchar *prefix, *subdir_path, *device_path, *tty_path; - unsigned long read_vendor_id, read_product_id; - const char *file; - - l = NULL; - tty_devs = NULL; - matched_paths = NULL; - - if (!(devices_dir = g_dir_open(device_tree, 0, NULL))) - return NULL; - - /* - * Find potential candidates using the vendor ID and product ID - * and store them in matched_paths. - */ - while ((usb_dev = g_dir_read_name(devices_dir))) { - vendor_path = g_strconcat(device_tree, - usb_dev, "/idVendor", NULL); - product_path = g_strconcat(device_tree, - usb_dev, "/idProduct", NULL); - - if (!g_file_test(vendor_path, G_FILE_TEST_EXISTS) || - !g_file_test(product_path, G_FILE_TEST_EXISTS)) - goto skip_device; - - if ((fd = g_fopen(vendor_path, "r")) == NULL) - goto skip_device; - - if (fgets(tmp, sizeof(tmp), fd) == NULL) { - fclose(fd); - goto skip_device; - } - read_vendor_id = strtoul(tmp, NULL, 16); - - fclose(fd); - - if ((fd = g_fopen(product_path, "r")) == NULL) - goto skip_device; - - if (fgets(tmp, sizeof(tmp), fd) == NULL) { - fclose(fd); - goto skip_device; - } - read_product_id = strtoul(tmp, NULL, 16); - - fclose(fd); - - if (vendor_id == read_vendor_id && - product_id == read_product_id) { - path_copy = g_strdup(usb_dev); - matched_paths = g_slist_prepend(matched_paths, - path_copy); - } - -skip_device: - g_free(vendor_path); - g_free(product_path); - } - g_dir_close(devices_dir); - - /* For every matched device try to find a ttyUSBX subfolder. */ - for (l = matched_paths; l; l = l->next) { - subdir_path = NULL; - - device_path = g_strconcat(device_tree, l->data, NULL); - - if (!(device_dir = g_dir_open(device_path, 0, NULL))) { - g_free(device_path); - continue; - } - - prefix = g_strconcat(l->data, ":", NULL); - - while ((file = g_dir_read_name(device_dir))) { - if (g_str_has_prefix(file, prefix)) { - subdir_path = g_strconcat(device_path, - "/", file, NULL); - break; - } - } - g_dir_close(device_dir); - - g_free(prefix); - g_free(device_path); - - if (subdir_path) { - if (!(device_dir = g_dir_open(subdir_path, 0, NULL))) { - g_free(subdir_path); - continue; - } - g_free(subdir_path); - - while ((file = g_dir_read_name(device_dir))) { - if (g_str_has_prefix(file, "ttyUSB")) { - tty_path = g_strconcat("/dev/", - file, NULL); - sr_dbg("Found USB device %04x:%04x attached to %s.", - vendor_id, product_id, tty_path); - tty_devs = g_slist_prepend(tty_devs, - tty_path); - break; - } - } - g_dir_close(device_dir); - } - } - g_slist_free_full(matched_paths, g_free); - - return tty_devs; -#else - (void)vendor_id; - (void)product_id; - - return NULL; -#endif -} - static int check_manufacturer(const char *manufacturer) { unsigned int i; @@ -207,26 +57,16 @@ static int check_manufacturer(const char *manufacturer) return SR_ERR; } -static struct sr_dev_inst *hmo_probe_serial_device(const char *serial_device, - const char *serial_options) +static struct sr_dev_inst *hmo_probe_serial_device(struct sr_scpi_dev_inst *scpi) { struct sr_dev_inst *sdi; struct dev_context *devc; struct sr_scpi_hw_info *hw_info; - struct sr_scpi_dev_inst *scpi; sdi = NULL; devc = NULL; - scpi = NULL; hw_info = NULL; - if (!(scpi = scpi_dev_inst_new(di->priv, serial_device, serial_options))) - goto fail; - - sr_info("Probing %s.", serial_device); - if (sr_scpi_open(scpi) != SR_OK) - goto fail; - if (sr_scpi_get_hw_id(scpi, &hw_info) != SR_OK) { sr_info("Couldn't get IDN response."); goto fail; @@ -275,49 +115,7 @@ fail: static GSList *scan(GSList *options) { - GSList *devices; - struct drv_context *drvc; - struct sr_dev_inst *sdi; - const char *serial_device, *serial_options; - GSList *l, *tty_devs; - unsigned int i; - - serial_device = NULL; - serial_options = SERIALCOMM; - sdi = NULL; - devices = NULL; - drvc = di->priv; - drvc->instances = NULL; - - if (sr_serial_extract_options(options, &serial_device, - &serial_options) == SR_OK) { - sdi = hmo_probe_serial_device(serial_device, serial_options); - if (sdi != NULL) { - devices = g_slist_append(devices, sdi); - drvc->instances = g_slist_append(drvc->instances, sdi); - } - } else { - tty_devs = NULL; - - for (i = 0; i < ARRAY_SIZE(ho_models); i++) { - if ((l = auto_find_usb(ho_models[i].vendor_id, - ho_models[i].product_id)) == NULL) - continue; - tty_devs = g_slist_concat(tty_devs, l); - } - - for (l = tty_devs; l; l = l->next) { - sdi = hmo_probe_serial_device(l->data, serial_options); - if (sdi != NULL) { - devices = g_slist_append(devices, sdi); - drvc->instances = g_slist_append(drvc->instances, sdi); - } - } - - g_slist_free_full(tty_devs, g_free); - } - - return devices; + return sr_scpi_scan(di->priv, options, hmo_probe_serial_device); } static GSList *dev_list(void) -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:59:05
|
--- hardware/common/Makefile.am | 2 +- hardware/common/scpi.c | 4 + hardware/common/scpi_usbtmc_libusb.c | 532 +++++++++++++++++++++++++++++++++++ 3 files changed, 537 insertions(+), 1 deletion(-) create mode 100644 hardware/common/scpi_usbtmc_libusb.c diff --git a/hardware/common/Makefile.am b/hardware/common/Makefile.am index cdffe47..7e32ded 100644 --- a/hardware/common/Makefile.am +++ b/hardware/common/Makefile.am @@ -33,7 +33,7 @@ libsigrok_hw_common_la_SOURCES += serial.c scpi_serial.c endif if NEED_USB -libsigrok_hw_common_la_SOURCES += ezusb.c usb.c +libsigrok_hw_common_la_SOURCES += ezusb.c usb.c scpi_usbtmc_libusb.c endif if NEED_VISA diff --git a/hardware/common/scpi.c b/hardware/common/scpi.c index 21ff178..c09a062 100644 --- a/hardware/common/scpi.c +++ b/hardware/common/scpi.c @@ -69,6 +69,7 @@ SR_PRIV extern const struct sr_scpi_dev_inst scpi_serial_dev; SR_PRIV extern const struct sr_scpi_dev_inst scpi_tcp_raw_dev; SR_PRIV extern const struct sr_scpi_dev_inst scpi_tcp_rigol_dev; SR_PRIV extern const struct sr_scpi_dev_inst scpi_usbtmc_dev; +SR_PRIV extern const struct sr_scpi_dev_inst scpi_usbtmc_libusb_dev; SR_PRIV extern const struct sr_scpi_dev_inst scpi_vxi_dev; SR_PRIV extern const struct sr_scpi_dev_inst scpi_visa_dev; @@ -76,6 +77,9 @@ static const struct sr_scpi_dev_inst *scpi_devs[] = { &scpi_tcp_raw_dev, &scpi_tcp_rigol_dev, &scpi_usbtmc_dev, +#ifdef HAVE_LIBUSB_1_0 + &scpi_usbtmc_libusb_dev, +#endif #if HAVE_RPC &scpi_vxi_dev, #endif diff --git a/hardware/common/scpi_usbtmc_libusb.c b/hardware/common/scpi_usbtmc_libusb.c new file mode 100644 index 0000000..711cf9b --- /dev/null +++ b/hardware/common/scpi_usbtmc_libusb.c @@ -0,0 +1,532 @@ +/* + * This file is part of the libsigrok project. + * + * Copyright (C) 2014 Aurelien Jacobs <au...@gn...> + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see <http://www.gnu.org/licenses/>. + */ + +#include <string.h> +#include "libsigrok.h" +#include "libsigrok-internal.h" + +#define LOG_PREFIX "scpi_usbtmc" + +#define MAX_TRANSFER_LENGTH 2048 +#define TRANSFER_TIMEOUT 1000 + +struct scpi_usbtmc_libusb { + struct sr_context *ctx; + struct sr_usb_dev_inst *usb; + uint8_t interface; + uint8_t bulk_in_ep; + uint8_t bulk_out_ep; + uint8_t interrupt_ep; + uint8_t usbtmc_int_cap; + uint8_t usbtmc_dev_cap; + uint8_t usb488_dev_cap; + uint8_t bTag; + uint8_t bulkin_attributes; + uint8_t buffer[MAX_TRANSFER_LENGTH]; + int response_length; + int response_bytes_read; + int remaining_length; +}; + +/* Some USBTMC-specific enums, as defined in the USBTMC standard. */ +#define SUBCLASS_USBTMC 0x03 +#define USBTMC_USB488 0x01 + +enum { + /* USBTMC control requests */ + INITIATE_ABORT_BULK_OUT = 1, + CHECK_ABORT_BULK_OUT_STATUS = 2, + INITIATE_ABORT_BULK_IN = 3, + CHECK_ABORT_BULK_IN_STATUS = 4, + INITIATE_CLEAR = 5, + CHECK_CLEAR_STATUS = 6, + GET_CAPABILITIES = 7, + INDICATOR_PULSE = 64, + + /* USB488 control requests */ + READ_STATUS_BYTE = 128, + REN_CONTROL = 160, + GO_TO_LOCAL = 161, + LOCAL_LOCKOUT = 162, +}; + +/* USBTMC capabilities */ +#define USBTMC_INT_CAP_LISTEN_ONLY 0x01 +#define USBTMC_INT_CAP_TALK_ONLY 0x02 +#define USBTMC_INT_CAP_INDICATOR 0x04 + +#define USBTMC_DEV_CAP_TERMCHAR 0x01 + +#define USB488_DEV_CAP_DT1 0x01 +#define USB488_DEV_CAP_RL1 0x02 +#define USB488_DEV_CAP_SR1 0x04 +#define USB488_DEV_CAP_SCPI 0x08 + +/* Bulk messages constants */ +#define USBTMC_BULK_HEADER_SIZE 12 + +/* Bulk MsgID values */ +#define DEV_DEP_MSG_OUT 1 +#define REQUEST_DEV_DEP_MSG_IN 2 +#define DEV_DEP_MSG_IN 2 + +/* bmTransferAttributes */ +#define EOM 0x01 +#define TERM_CHAR_ENABLED 0x02 + + +static GSList *scpi_usbtmc_libusb_scan(struct drv_context *drvc) +{ + struct libusb_device **devlist; + struct libusb_device_descriptor des; + struct libusb_config_descriptor *confdes; + const struct libusb_interface_descriptor *intfdes; + GSList *resources = NULL; + int confidx, intfidx, ret, i; + char *res; + + libusb_get_device_list(drvc->sr_ctx->libusb_ctx, &devlist); + for (i = 0; devlist[i]; i++) { + if ((ret = libusb_get_device_descriptor(devlist[i], &des))) { + sr_err("Failed to get device descriptor: %s.", + libusb_error_name(ret)); + continue; + } + + for (confidx = 0; confidx < des.bNumConfigurations; confidx++) { + if (libusb_get_config_descriptor(devlist[i], confidx, &confdes) != 0) { + sr_err("Failed to get configuration descriptor."); + break; + } + for (intfidx = 0; intfidx < confdes->bNumInterfaces; intfidx++) { + intfdes = confdes->interface[intfidx].altsetting; + if (intfdes->bInterfaceClass != LIBUSB_CLASS_APPLICATION || + intfdes->bInterfaceSubClass != SUBCLASS_USBTMC || + intfdes->bInterfaceProtocol != USBTMC_USB488) + continue; + sr_dbg("Found USBTMC device (VID:PID = %04x:%04x, " + "bus.address = %d.%d).", des.idVendor, des.idProduct, + libusb_get_bus_number(devlist[i]), + libusb_get_device_address(devlist[i])); + res = g_strdup_printf("usbtmc/%d.%d", + libusb_get_bus_number(devlist[i]), + libusb_get_device_address(devlist[i])); + resources = g_slist_append(resources, res); + } + libusb_free_config_descriptor(confdes); + } + } + libusb_free_device_list(devlist, 1); + + sr_dbg("Found %d device(s).", g_slist_length(resources)); + + return resources; +} + +static int scpi_usbtmc_libusb_dev_inst_new(void *priv, struct drv_context *drvc, + const char *resource, char **params, const char *serialcomm) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + GSList *devices; + + (void)resource; + (void)serialcomm; + + if (!params || !params[1]) { + sr_err("Invalid parameters."); + return SR_ERR; + } + + uscpi->ctx = drvc->sr_ctx; + devices = sr_usb_find(uscpi->ctx->libusb_ctx, params[1]); + if (g_slist_length(devices) != 1) { + sr_err("Failed to find USB device '%s'.", params[1]); + g_slist_free_full(devices, (GDestroyNotify)sr_usb_dev_inst_free); + return SR_ERR; + } + uscpi->usb = devices->data; + g_slist_free(devices); + + return SR_OK; +} + +static int scpi_usbtmc_libusb_open(void *priv) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + struct sr_usb_dev_inst *usb = uscpi->usb; + struct libusb_device *dev; + struct libusb_device_descriptor des; + struct libusb_config_descriptor *confdes; + const struct libusb_interface_descriptor *intfdes; + const struct libusb_endpoint_descriptor *ep; + int confidx, intfidx, epidx, config = 0; + uint8_t capabilities[24]; + int ret, found = 0; + + if (usb->devhdl) + return SR_OK; + + if (sr_usb_open(uscpi->ctx->libusb_ctx, usb) != SR_OK) + return SR_ERR; + + dev = libusb_get_device(usb->devhdl); + if ((ret = libusb_get_device_descriptor(dev, &des))) { + sr_err("Failed to get device descriptor: %s.", + libusb_error_name(ret)); + return SR_ERR; + } + + for (confidx = 0; confidx < des.bNumConfigurations; confidx++) { + if (libusb_get_config_descriptor(dev, confidx, &confdes) != 0) { + sr_err("Failed to get configuration descriptor."); + continue; + } + for (intfidx = 0; intfidx < confdes->bNumInterfaces; intfidx++) { + intfdes = confdes->interface[intfidx].altsetting; + if (intfdes->bInterfaceClass != LIBUSB_CLASS_APPLICATION || + intfdes->bInterfaceSubClass != SUBCLASS_USBTMC || + intfdes->bInterfaceProtocol != USBTMC_USB488) + continue; + uscpi->interface = intfdes->bInterfaceNumber; + sr_dbg("Interface %d", uscpi->interface); + config = confdes->bConfigurationValue; + sr_dbg("Configuration %d", config); + for (epidx = 0; epidx < intfdes->bNumEndpoints; epidx++) { + ep = &intfdes->endpoint[epidx]; + if (ep->bmAttributes == LIBUSB_TRANSFER_TYPE_BULK && + !(ep->bEndpointAddress & (LIBUSB_ENDPOINT_DIR_MASK))) { + uscpi->bulk_out_ep = ep->bEndpointAddress; + sr_dbg("Bulk OUT EP %d", uscpi->bulk_out_ep); + } + if (ep->bmAttributes == LIBUSB_TRANSFER_TYPE_BULK && + ep->bEndpointAddress & (LIBUSB_ENDPOINT_DIR_MASK)) { + uscpi->bulk_in_ep = ep->bEndpointAddress; + sr_dbg("Bulk IN EP %d", uscpi->bulk_in_ep); + } + if (ep->bmAttributes == LIBUSB_TRANSFER_TYPE_INTERRUPT && + ep->bEndpointAddress & (LIBUSB_ENDPOINT_DIR_MASK)) { + uscpi->interrupt_ep = ep->bEndpointAddress; + sr_dbg("Interrupt EP %d", uscpi->interrupt_ep); + } + } + found = 1; + } + libusb_free_config_descriptor(confdes); + if (found) + break; + } + + if (!found) { + sr_err("Failed to find USBTMC interface"); + return SR_ERR; + } + + if (libusb_kernel_driver_active(usb->devhdl, uscpi->interface) == 1) + if ((ret = libusb_detach_kernel_driver(usb->devhdl, + uscpi->interface)) < 0) { + sr_err("failed to detach kernel driver: %s", + libusb_error_name(ret)); + return SR_ERR; + } + + if ((ret = libusb_set_configuration(usb->devhdl, config))) { + sr_err("Failed to set configuration: %s.", libusb_error_name(ret)); + return SR_ERR; + } + + if ((ret = libusb_claim_interface(usb->devhdl, uscpi->interface))) { + sr_err("Failed to claim interface: %s.", libusb_error_name(ret)); + return SR_ERR; + } + + libusb_clear_halt(usb->devhdl, uscpi->bulk_in_ep); + libusb_clear_halt(usb->devhdl, uscpi->bulk_out_ep); + libusb_clear_halt(usb->devhdl, uscpi->interrupt_ep); + + /* get capabilities */ + ret = libusb_control_transfer(usb->devhdl, + LIBUSB_ENDPOINT_IN | + LIBUSB_REQUEST_TYPE_CLASS | + LIBUSB_RECIPIENT_INTERFACE, + GET_CAPABILITIES, 0, + uscpi->interface, + capabilities, sizeof(capabilities), + TRANSFER_TIMEOUT); + if (ret == sizeof(capabilities)) { + uscpi->usbtmc_int_cap = capabilities[ 4]; + uscpi->usbtmc_dev_cap = capabilities[ 5]; + uscpi->usb488_dev_cap = capabilities[15]; + } + sr_dbg("device capabilities: %s%s%s%s%s, %s, %s", + uscpi->usb488_dev_cap & USB488_DEV_CAP_SCPI ? "SCPI, " : "", + uscpi->usbtmc_dev_cap & USBTMC_DEV_CAP_TERMCHAR ? "TermChar, ": "", + uscpi->usbtmc_int_cap & USBTMC_INT_CAP_LISTEN_ONLY? "L3, " : + uscpi->usbtmc_int_cap & USBTMC_INT_CAP_TALK_ONLY ? "" : "L4, ", + uscpi->usbtmc_int_cap & USBTMC_INT_CAP_TALK_ONLY ? "T5, " : + uscpi->usbtmc_int_cap & USBTMC_INT_CAP_LISTEN_ONLY? "" : "T6, ", + uscpi->usb488_dev_cap & USB488_DEV_CAP_SR1 ? "SR1" : "SR0", + uscpi->usb488_dev_cap & USB488_DEV_CAP_RL1 ? "RL1" : "RL0", + uscpi->usb488_dev_cap & USB488_DEV_CAP_DT1 ? "DT1" : "DT0"); + + return SR_OK; +} + +static int scpi_usbtmc_libusb_source_add(void *priv, int events, int timeout, + sr_receive_data_callback_t cb, void *cb_data) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + (void)events; + return usb_source_add(uscpi->ctx, timeout, cb, cb_data); +} + +static int scpi_usbtmc_libusb_source_remove(void *priv) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + return usb_source_remove(uscpi->ctx); +} + +static void usbtmc_bulk_out_header_write(void *header, uint8_t MsgID, + uint8_t bTag, + uint32_t TransferSize, + uint8_t bmTransferAttributes, + char TermChar) +{ + W8(header+ 0, MsgID); + W8(header+ 1, bTag); + W8(header+ 2, ~bTag); + W8(header+ 3, 0); + WL32(header+ 4, TransferSize); + W8(header+ 8, bmTransferAttributes); + W8(header+ 9, TermChar); + WL16(header+10, 0); +} + +static int usbtmc_bulk_in_header_read(void *header, uint8_t MsgID, + unsigned char bTag, + int32_t *TransferSize, + uint8_t *bmTransferAttributes) +{ + if (R8(header+0) != MsgID || + R8(header+1) != bTag || + R8(header+2) != (unsigned char)~bTag) + return SR_ERR; + if (TransferSize) + *TransferSize = RL32(header+4); + if (bmTransferAttributes) + *bmTransferAttributes = R8(header+8); + return SR_OK; +} + +static int scpi_usbtmc_bulkout(struct scpi_usbtmc_libusb *uscpi, + uint8_t msg_id, const void *data, int32_t size, + uint8_t transfer_attributes) +{ + struct sr_usb_dev_inst *usb = uscpi->usb; + int padded_size, ret, transferred; + + if (data && size+USBTMC_BULK_HEADER_SIZE+3 > (int)sizeof(uscpi->buffer)) { + sr_err("USBTMC bulk out transfer too big"); + return SR_ERR; + } + + uscpi->bTag++; + uscpi->bTag += !uscpi->bTag; /* bTag == 0 is invalid so avoid it */ + + usbtmc_bulk_out_header_write(uscpi->buffer, msg_id, uscpi->bTag, + size, transfer_attributes, 0); + if (data) + memcpy(uscpi->buffer+USBTMC_BULK_HEADER_SIZE, data, size); + else + size = 0; + size += USBTMC_BULK_HEADER_SIZE; + padded_size = (size + 3) & ~0x3; + memset(uscpi->buffer+size, 0, padded_size - size); + + ret = libusb_bulk_transfer(usb->devhdl, uscpi->bulk_out_ep, + uscpi->buffer, padded_size, &transferred, + TRANSFER_TIMEOUT); + if (ret) { + sr_err("USBTMC bulk out transfer error: %d", ret); + return SR_ERR; + } + + if (transferred < padded_size) { + sr_dbg("USBTMC bulkout partial transfer (%d/%d bytes)", + transferred, padded_size); + return SR_ERR; + } + + return transferred - USBTMC_BULK_HEADER_SIZE; +} + +static int scpi_usbtmc_bulkin_start(struct scpi_usbtmc_libusb *uscpi, + uint8_t msg_id, void *data, int32_t size, + uint8_t *transfer_attributes) +{ + struct sr_usb_dev_inst *usb = uscpi->usb; + int ret, transferred, message_size; + + ret = libusb_bulk_transfer(usb->devhdl, uscpi->bulk_in_ep, data, size, + &transferred, TRANSFER_TIMEOUT); + if (ret) { + sr_err("USBTMC bulk in transfer error: %d", ret); + return SR_ERR; + } + + if (usbtmc_bulk_in_header_read(data, msg_id, uscpi->bTag, &message_size, + transfer_attributes) != SR_OK) { + sr_err("USBTMC invalid bulk in header"); + return SR_ERR; + } + + message_size += USBTMC_BULK_HEADER_SIZE; + uscpi->response_length = MIN(transferred, message_size); + uscpi->response_bytes_read = USBTMC_BULK_HEADER_SIZE; + uscpi->remaining_length = message_size - uscpi->response_length; + + return transferred - USBTMC_BULK_HEADER_SIZE; +} + +static int scpi_usbtmc_bulkin_continue(struct scpi_usbtmc_libusb *uscpi, + void *data, int size) +{ + struct sr_usb_dev_inst *usb = uscpi->usb; + int ret, transferred; + + ret = libusb_bulk_transfer(usb->devhdl, uscpi->bulk_in_ep, data, size, + &transferred, TRANSFER_TIMEOUT); + if (ret) { + sr_err("USBTMC bulk in transfer error: %d", ret); + return SR_ERR; + } + + uscpi->response_length = MIN(transferred, uscpi->remaining_length); + uscpi->response_bytes_read = 0; + uscpi->remaining_length -= uscpi->response_length; + + return transferred; +} + +static int scpi_usbtmc_libusb_send(void *priv, const char *command) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + + if (scpi_usbtmc_bulkout(uscpi, DEV_DEP_MSG_OUT, + command, strlen(command), EOM) <= 0) + return SR_ERR; + + sr_spew("Successfully sent SCPI command: '%s'.", command); + + return SR_OK; +} + +static int scpi_usbtmc_libusb_read_begin(void *priv) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + + uscpi->remaining_length = 0; + + if (scpi_usbtmc_bulkout(uscpi, REQUEST_DEV_DEP_MSG_IN, + NULL, INT32_MAX, 0) < 0) + return SR_ERR; + if (scpi_usbtmc_bulkin_start(uscpi, DEV_DEP_MSG_IN, + uscpi->buffer, sizeof(uscpi->buffer), + &uscpi->bulkin_attributes) < 0) + return SR_ERR; + + return SR_OK; +} + +static int scpi_usbtmc_libusb_read_data(void *priv, char *buf, int maxlen) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + int read_length; + + if (uscpi->response_bytes_read >= uscpi->response_length) { + if (uscpi->remaining_length > 0) { + if (scpi_usbtmc_bulkin_continue(uscpi, uscpi->buffer, + sizeof(uscpi->buffer)) <= 0) + return SR_ERR; + } else { + if (uscpi->bulkin_attributes & EOM) + return SR_ERR; + if (scpi_usbtmc_libusb_read_begin(uscpi) < 0) + return SR_ERR; + } + } + + read_length = MIN(uscpi->response_length - uscpi->response_bytes_read, maxlen); + + memcpy(buf, uscpi->buffer + uscpi->response_bytes_read, read_length); + + uscpi->response_bytes_read += read_length; + + return read_length; +} + +static int scpi_usbtmc_libusb_read_complete(void *priv) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + return uscpi->response_bytes_read >= uscpi->response_length && + uscpi->remaining_length <= 0 && + uscpi->bulkin_attributes & EOM; +} + +static int scpi_usbtmc_libusb_close(void *priv) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + struct sr_usb_dev_inst *usb = uscpi->usb; + + if (!usb->devhdl) + return SR_ERR; + + libusb_clear_halt(usb->devhdl, uscpi->bulk_in_ep); + libusb_clear_halt(usb->devhdl, uscpi->bulk_out_ep); + libusb_clear_halt(usb->devhdl, uscpi->interrupt_ep); + libusb_release_interface(usb->devhdl, uscpi->interface); + libusb_attach_kernel_driver(usb->devhdl, uscpi->interface); + libusb_close(usb->devhdl); + usb->devhdl = NULL; + + return SR_OK; +} + +static void scpi_usbtmc_libusb_free(void *priv) +{ + struct scpi_usbtmc_libusb *uscpi = priv; + sr_usb_dev_inst_free(uscpi->usb); +} + + +SR_PRIV const struct sr_scpi_dev_inst scpi_usbtmc_libusb_dev = { + .name = "USBTMC", + .prefix = "usbtmc", + .priv_size = sizeof(struct scpi_usbtmc_libusb), + .scan = scpi_usbtmc_libusb_scan, + .dev_inst_new = scpi_usbtmc_libusb_dev_inst_new, + .open = scpi_usbtmc_libusb_open, + .source_add = scpi_usbtmc_libusb_source_add, + .source_remove = scpi_usbtmc_libusb_source_remove, + .send = scpi_usbtmc_libusb_send, + .read_begin = scpi_usbtmc_libusb_read_begin, + .read_data = scpi_usbtmc_libusb_read_data, + .read_complete = scpi_usbtmc_libusb_read_complete, + .close = scpi_usbtmc_libusb_close, + .free = scpi_usbtmc_libusb_free, +}; -- 1.9.rc1 |
From: Aurelien J. <au...@gn...> - 2014-02-04 22:59:12
|
--- hardware/common/usb.c | 61 --------------------------------------------------- libsigrok-internal.h | 1 - 2 files changed, 62 deletions(-) diff --git a/hardware/common/usb.c b/hardware/common/usb.c index 8d4224d..a4845c9 100644 --- a/hardware/common/usb.c +++ b/hardware/common/usb.c @@ -29,10 +29,6 @@ #define CONN_USB_VIDPID "^([0-9a-z]{4})\\.([0-9a-z]{4})$" #define CONN_USB_BUSADDR "^(\\d+)\\.(\\d+)$" -/* Some USBTMC-specific enums, as defined in the USBTMC standard. */ -#define SUBCLASS_USBTMC 0x03 -#define USBTMC_USB488 0x01 - #define LOG_PREFIX "usb" /** @@ -135,63 +131,6 @@ SR_PRIV GSList *sr_usb_find(libusb_context *usb_ctx, const char *conn) return devices; } -/** - * Find USB devices supporting the USBTMC class - * - * @param usb_ctx libusb context to use while scanning. - * - * @return A GSList of struct sr_usb_dev_inst, with bus and address fields - * indicating devices with USBTMC support. - */ -SR_PRIV GSList *sr_usb_find_usbtmc(libusb_context *usb_ctx) -{ - struct sr_usb_dev_inst *usb; - struct libusb_device **devlist; - struct libusb_device_descriptor des; - struct libusb_config_descriptor *confdes; - const struct libusb_interface_descriptor *intfdes; - GSList *devices; - int confidx, intfidx, ret, i; - - devices = NULL; - libusb_get_device_list(usb_ctx, &devlist); - for (i = 0; devlist[i]; i++) { - if ((ret = libusb_get_device_descriptor(devlist[i], &des))) { - sr_err("Failed to get device descriptor: %s.", - libusb_error_name(ret)); - continue; - } - - for (confidx = 0; confidx < des.bNumConfigurations; confidx++) { - if (libusb_get_config_descriptor(devlist[i], confidx, &confdes) != 0) { - sr_err("Failed to get configuration descriptor."); - break; - } - for (intfidx = 0; intfidx < confdes->bNumInterfaces; intfidx++) { - intfdes = confdes->interface[intfidx].altsetting; - if (intfdes->bInterfaceClass != LIBUSB_CLASS_APPLICATION - || intfdes->bInterfaceSubClass != SUBCLASS_USBTMC - || intfdes->bInterfaceProtocol != USBTMC_USB488) - continue; - sr_dbg("Found USBTMC device (VID:PID = %04x:%04x, bus.address = " - "%d.%d).", des.idVendor, des.idProduct, - libusb_get_bus_number(devlist[i]), - libusb_get_device_address(devlist[i])); - - usb = sr_usb_dev_inst_new(libusb_get_bus_number(devlist[i]), - libusb_get_device_address(devlist[i]), NULL); - devices = g_slist_append(devices, usb); - } - libusb_free_config_descriptor(confdes); - } - } - libusb_free_device_list(devlist, 1); - - sr_dbg("Found %d device(s).", g_slist_length(devices)); - - return devices; -} - SR_PRIV int sr_usb_open(libusb_context *usb_ctx, struct sr_usb_dev_inst *usb) { struct libusb_device **devlist; diff --git a/libsigrok-internal.h b/libsigrok-internal.h index c84403a..e7a2ceb 100644 --- a/libsigrok-internal.h +++ b/libsigrok-internal.h @@ -244,7 +244,6 @@ SR_PRIV void sr_dev_inst_free(struct sr_dev_inst *sdi); /* USB-specific instances */ SR_PRIV struct sr_usb_dev_inst *sr_usb_dev_inst_new(uint8_t bus, uint8_t address, struct libusb_device_handle *hdl); -SR_PRIV GSList *sr_usb_find_usbtmc(libusb_context *usb_ctx); SR_PRIV void sr_usb_dev_inst_free(struct sr_usb_dev_inst *usb); #endif -- 1.9.rc1 |
From: Damir J. <po...@po...> - 2014-02-05 13:28:46
|
Please add a prefix to the commit message. Uwe used the "libsigrok-internal.h:" prefix for commits that introduce changes to this file. Same goes for the other commits without such a prefix. |
From: Aurelien J. <au...@gn...> - 2014-02-05 23:21:31
|
On Wed, Feb 05, 2014 at 02:28:37PM +0100, Damir Jelić wrote: > Please add a prefix to the commit message. I generally try to add a prefix to each commit message... > Uwe used the "libsigrok-internal.h:" > prefix for commits that introduce changes to this file. but this "libsigrok-internal.h:" prefix looks a bit ugly and use too much line space for my taste. But anyway, OK, I added this prefix. Aurel |
From: Damir J. <po...@po...> - 2014-02-05 13:29:21
|
On Tue, Feb 04, 2014 at 11:45:22PM +0100, Aurelien Jacobs wrote: > note that sr_usb_find_serial() is copied from the hameg-hmo driver > --- > hardware/common/scpi.c | 68 +++++++++++++++++ > hardware/common/scpi_serial.c | 167 ++++++++++++++++++++++++++++++++++++++++++ > hardware/common/scpi_usbtmc.c | 24 ++++++ > libsigrok-internal.h | 3 + > 4 files changed, 262 insertions(+) > > diff --git a/hardware/common/scpi.c b/hardware/common/scpi.c > index 2307a8b..21ff178 100644 > --- a/hardware/common/scpi.c > +++ b/hardware/common/scpi.c > @@ -87,6 +87,74 @@ static const struct sr_scpi_dev_inst *scpi_devs[] = { > #endif > }; > > +static GSList *sr_scpi_scan_resource(struct drv_context *drvc, > + const char *resource, const char *serialcomm, > + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)) > +{ > + struct sr_scpi_dev_inst *scpi; > + struct sr_dev_inst *sdi; > + > + if (!(scpi = scpi_dev_inst_new(drvc, resource, serialcomm))) > + return NULL; > + > + if (sr_scpi_open(scpi) != SR_OK) { > + sr_info("Couldn't open SCPI device."); > + sr_scpi_free(scpi); > + return NULL; > + }; > + > + if ((sdi = probe_device(scpi))) > + return g_slist_append(NULL, sdi); //FIXME list_new !!! This comment should be more verbose, also please don't use // for comments, use /* */ instead. > + > + sr_scpi_close(scpi); > + sr_scpi_free(scpi); > + return NULL; > +} > + > +SR_PRIV GSList *sr_scpi_scan(struct drv_context *drvc, GSList *options, > + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)) > +{ > + GSList *resources, *l, *d, *devices = NULL; > + const char *resource = NULL; > + const char *serialcomm = NULL; > + unsigned i; > + > + for (l = options; l; l = l->next) { > + struct sr_config *src = l->data; > + switch (src->key) { > + case SR_CONF_CONN: > + resource = g_variant_get_string(src->data, NULL); > + break; > + case SR_CONF_SERIALCOMM: > + serialcomm = g_variant_get_string(src->data, NULL); > + break; > + } > + } You can use sr_serial_extract_options() for this and you forgot to handle the default SERIALCOMM value that drivers provide. This way serial devices need to provide the SERIALCOMM parameter always which kind of defeats the purpose of the auto scan. Note: This actually doesn't break the auto scan for my Hameg HMO if it's connected via USB but I consider this to be an accident. > + > + for (i = 0; i < ARRAY_SIZE(scpi_devs); i++) { > + if ((resource && strcmp(resource, scpi_devs[i]->prefix)) > + || !scpi_devs[i]->scan) > + continue; > + resources = scpi_devs[i]->scan(drvc); > + for (l = resources; l; l = l->next) > + if ((d = sr_scpi_scan_resource(drvc, l->data, serialcomm, > + probe_device))) > + devices = g_slist_concat(devices, d); > + g_slist_free_full(resources, g_free); > + } > + > + if (!devices && resource) > + devices = sr_scpi_scan_resource(drvc, resource, serialcomm, > + probe_device); > + > + /* Tack a copy of the newly found devices onto the driver list. */ > + if (devices) > + drvc->instances = g_slist_concat(drvc->instances, > + g_slist_copy(devices)); > + > + return devices; > +} > + > SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(struct drv_context *drvc, > const char *resource, const char *serialcomm) > { > diff --git a/hardware/common/scpi_serial.c b/hardware/common/scpi_serial.c > index 6e73c89..9ab9e16 100644 > --- a/hardware/common/scpi_serial.c > +++ b/hardware/common/scpi_serial.c > @@ -22,6 +22,8 @@ > #include "libsigrok-internal.h" > > #include <glib.h> > +#include <glib/gstdio.h> > +#include <stdlib.h> > #include <string.h> > > #define LOG_PREFIX "scpi_serial" > @@ -35,6 +37,170 @@ struct scpi_serial { > size_t read; > }; > > +static struct { > + uint16_t vendor_id; > + uint16_t product_id; > +} scpi_serial_usb_ids[] = { > + { 0x0403, 0xed72 }, /* Hameg HO720 */ > + { 0x0403, 0xed73 }, /* Hameg HO730 */ > +}; > + > +/** > + * Find USB serial devices via the USB vendor ID and product ID. > + * > + * @param vendor_id Vendor ID of the USB device. > + * @param product_id Product ID of the USB device. > + * > + * @return A GSList of strings containing the path of the serial device or > + * NULL if no serial device is found. The returned list must be freed > + * by the caller. > + */ > +static GSList *sr_usb_find_serial(uint16_t vendor_id, uint16_t product_id) > +{ > +#ifdef __linux__ > + const gchar *usb_dev; > + const char device_tree[] = "/sys/bus/usb/devices/"; > + GDir *devices_dir, *device_dir; > + GSList *l = NULL; > + GSList *tty_devs; > + GSList *matched_paths; > + FILE *fd; > + char tmp[5]; > + gchar *vendor_path, *product_path, *path_copy; > + gchar *prefix, *subdir_path, *device_path, *tty_path; > + unsigned long read_vendor_id, read_product_id; > + const char *file; > + > + l = NULL; > + tty_devs = NULL; > + matched_paths = NULL; > + > + if (!(devices_dir = g_dir_open(device_tree, 0, NULL))) > + return NULL; > + > + /* > + * Find potential candidates using the vendor ID and product ID > + * and store them in matched_paths. > + */ > + while ((usb_dev = g_dir_read_name(devices_dir))) { > + vendor_path = g_strconcat(device_tree, > + usb_dev, "/idVendor", NULL); > + product_path = g_strconcat(device_tree, > + usb_dev, "/idProduct", NULL); > + > + if (!g_file_test(vendor_path, G_FILE_TEST_EXISTS) || > + !g_file_test(product_path, G_FILE_TEST_EXISTS)) > + goto skip_device; > + > + if ((fd = g_fopen(vendor_path, "r")) == NULL) > + goto skip_device; > + > + if (fgets(tmp, sizeof(tmp), fd) == NULL) { > + fclose(fd); > + goto skip_device; > + } > + read_vendor_id = strtoul(tmp, NULL, 16); > + > + fclose(fd); > + > + if ((fd = g_fopen(product_path, "r")) == NULL) > + goto skip_device; > + > + if (fgets(tmp, sizeof(tmp), fd) == NULL) { > + fclose(fd); > + goto skip_device; > + } > + read_product_id = strtoul(tmp, NULL, 16); > + > + fclose(fd); > + > + if (vendor_id == read_vendor_id && > + product_id == read_product_id) { > + path_copy = g_strdup(usb_dev); > + matched_paths = g_slist_prepend(matched_paths, > + path_copy); > + } > + > +skip_device: > + g_free(vendor_path); > + g_free(product_path); > + } > + g_dir_close(devices_dir); > + > + /* For every matched device try to find a ttyUSBX subfolder. */ > + for (l = matched_paths; l; l = l->next) { > + subdir_path = NULL; > + > + device_path = g_strconcat(device_tree, l->data, NULL); > + > + if (!(device_dir = g_dir_open(device_path, 0, NULL))) { > + g_free(device_path); > + continue; > + } > + > + prefix = g_strconcat(l->data, ":", NULL); > + > + while ((file = g_dir_read_name(device_dir))) { > + if (g_str_has_prefix(file, prefix)) { > + subdir_path = g_strconcat(device_path, > + "/", file, NULL); > + break; > + } > + } > + g_dir_close(device_dir); > + > + g_free(prefix); > + g_free(device_path); > + > + if (subdir_path) { > + if (!(device_dir = g_dir_open(subdir_path, 0, NULL))) { > + g_free(subdir_path); > + continue; > + } > + g_free(subdir_path); > + > + while ((file = g_dir_read_name(device_dir))) { > + if (g_str_has_prefix(file, "ttyUSB")) { > + tty_path = g_strconcat("/dev/", > + file, NULL); > + sr_dbg("Found USB device %04x:%04x attached to %s.", > + vendor_id, product_id, tty_path); > + tty_devs = g_slist_prepend(tty_devs, > + tty_path); > + break; > + } > + } > + g_dir_close(device_dir); > + } > + } > + g_slist_free_full(matched_paths, g_free); > + > + return tty_devs; > +#else > + (void)vendor_id; > + (void)product_id; > + > + return NULL; > +#endif > +} I don't think that this function should go into scpi_serial. There is nothing SCPI specific about it. I think it would make sense to put it in serial.c and enable auto scan functionality for all serial-USB devices. I know that there are talks about a "device tree" and a vast overhaul of how scanning works but it seems unlikely to me that this will be finished before the next release. The devs will probably have a vastly different opinion so I don't request any change here. > + > +static GSList *scpi_serial_scan(struct drv_context *drvc) > +{ > + GSList *l, *resources = NULL; > + unsigned i; > + > + (void)drvc; Why do we need drvc here? Do we expect it to be used in the near future? > + > + for (i = 0; i < ARRAY_SIZE(scpi_serial_usb_ids); i++) { > + if ((l = sr_usb_find_serial(scpi_serial_usb_ids[i].vendor_id, > + scpi_serial_usb_ids[i].product_id)) == NULL) > + continue; > + resources = g_slist_concat(resources, l); > + } > + > + return resources; > +} > + > static int scpi_serial_dev_inst_new(void *priv, struct drv_context *drvc, > const char *resource, char **params, const char *serialcomm) > { > @@ -190,6 +356,7 @@ SR_PRIV const struct sr_scpi_dev_inst scpi_serial_dev = { > .name = "serial", > .prefix = "", > .priv_size = sizeof(struct scpi_serial), > + .scan = scpi_serial_scan, > .dev_inst_new = scpi_serial_dev_inst_new, > .open = scpi_serial_open, > .source_add = scpi_serial_source_add, > diff --git a/hardware/common/scpi_usbtmc.c b/hardware/common/scpi_usbtmc.c > index 6cd7964..1ffc34a 100644 > --- a/hardware/common/scpi_usbtmc.c > +++ b/hardware/common/scpi_usbtmc.c > @@ -37,6 +37,29 @@ struct usbtmc_scpi { > int response_bytes_read; > }; > > +static GSList *scpi_usbtmc_scan(struct drv_context *drvc) > +{ > + GSList *resources = NULL; > + GDir *dir; > + const char *dev_name; > + char *resource; > + > + (void)drvc; Same as for serial_scan(). > + > + if (!(dir = g_dir_open("/sys/class/usbmisc/", 0, NULL))) > + if (!(dir = g_dir_open("/sys/class/usb/", 0, NULL))) > + return NULL; > + while ((dev_name = g_dir_read_name(dir))) { > + if (strncmp(dev_name, "usbtmc", 6)) > + continue; > + resource = g_strconcat("/dev/", dev_name, NULL); > + resources = g_slist_append(resources, resource); > + } > + g_dir_close(dir); > + > + return resources; > +} This function is not portable, you should guard it with an ifdef just like sr_usb_find_serial(), also do we know if usbtmc devices use SCPI exclusively? Do you think it would make sense to put this into usb.c? > + > static int scpi_usbtmc_dev_inst_new(void *priv, struct drv_context *drvc, > const char *resource, char **params, const char *serialcomm) > { > @@ -190,6 +213,7 @@ SR_PRIV const struct sr_scpi_dev_inst scpi_usbtmc_dev = { > .name = "USBTMC", > .prefix = "/dev/usbtmc", > .priv_size = sizeof(struct usbtmc_scpi), > + .scan = scpi_usbtmc_scan, > .dev_inst_new = scpi_usbtmc_dev_inst_new, > .open = scpi_usbtmc_open, > .source_add = scpi_usbtmc_source_add, > diff --git a/libsigrok-internal.h b/libsigrok-internal.h > index 951d91d..c84403a 100644 > --- a/libsigrok-internal.h > +++ b/libsigrok-internal.h > @@ -438,6 +438,7 @@ struct sr_scpi_dev_inst { > const char *name; > const char *prefix; > int priv_size; > + GSList *(*scan)(struct drv_context *drvc); > int (*dev_inst_new)(void *priv, struct drv_context *drvc, > const char *resource, char **params, const char *serialcomm); > int (*open)(void *priv); > @@ -453,6 +454,8 @@ struct sr_scpi_dev_inst { > void *priv; > }; > > +SR_PRIV GSList *sr_scpi_scan(struct drv_context *drvc, GSList *options, > + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)); > SR_PRIV struct sr_scpi_dev_inst *scpi_dev_inst_new(struct drv_context *drvc, > const char *resource, const char *serialcomm); > SR_PRIV int sr_scpi_open(struct sr_scpi_dev_inst *scpi); > -- > 1.9.rc1 |
From: Damir J. <po...@po...> - 2014-02-05 13:29:36
|
On Tue, Feb 04, 2014 at 11:45:24PM +0100, Aurelien Jacobs wrote: > --- > hardware/hameg-hmo/api.c | 206 +---------------------------------------------- > 1 file changed, 2 insertions(+), 204 deletions(-) > > diff --git a/hardware/hameg-hmo/api.c b/hardware/hameg-hmo/api.c > index 5abd8c5..241e86b 100644 > --- a/hardware/hameg-hmo/api.c > +++ b/hardware/hameg-hmo/api.c > @@ -18,7 +18,6 @@ > */ > > #include <stdlib.h> > -#include <glib/gstdio.h> > #include "protocol.h" > > #define SERIALCOMM "115200/8n1/flow=1" Here is the default SERIALCOMM parameter that I mentioned already. > @@ -35,16 +34,6 @@ static const int32_t hwopts[] = { > SR_CONF_SERIALCOMM, > }; > > -struct usb_id_info { > - uint16_t vendor_id; > - uint16_t product_id; > -}; > - > -static struct usb_id_info ho_models[] = { > - { 0x0403, 0xed72 }, /* HO720 */ > - { 0x0403, 0xed73 }, /* HO730 */ > -}; > - > enum { > PG_INVALID = -1, > PG_NONE, > @@ -57,145 +46,6 @@ static int init(struct sr_context *sr_ctx) > return std_init(sr_ctx, di, LOG_PREFIX); > } > > -/** > - * Find USB serial devices via the USB vendor ID and product ID. > - * > - * @param vendor_id Vendor ID of the USB device. > - * @param product_id Product ID of the USB device. > - * > - * @return A GSList of strings containing the path of the serial device or > - * NULL if no serial device is found. The returned list must be freed > - * by the caller. > - */ > -static GSList *auto_find_usb(uint16_t vendor_id, uint16_t product_id) > -{ > -#ifdef __linux__ > - const gchar *usb_dev; > - const char device_tree[] = "/sys/bus/usb/devices/"; > - GDir *devices_dir, *device_dir; > - GSList *l = NULL; > - GSList *tty_devs; > - GSList *matched_paths; > - FILE *fd; > - char tmp[5]; > - gchar *vendor_path, *product_path, *path_copy; > - gchar *prefix, *subdir_path, *device_path, *tty_path; > - unsigned long read_vendor_id, read_product_id; > - const char *file; > - > - l = NULL; > - tty_devs = NULL; > - matched_paths = NULL; > - > - if (!(devices_dir = g_dir_open(device_tree, 0, NULL))) > - return NULL; > - > - /* > - * Find potential candidates using the vendor ID and product ID > - * and store them in matched_paths. > - */ > - while ((usb_dev = g_dir_read_name(devices_dir))) { > - vendor_path = g_strconcat(device_tree, > - usb_dev, "/idVendor", NULL); > - product_path = g_strconcat(device_tree, > - usb_dev, "/idProduct", NULL); > - > - if (!g_file_test(vendor_path, G_FILE_TEST_EXISTS) || > - !g_file_test(product_path, G_FILE_TEST_EXISTS)) > - goto skip_device; > - > - if ((fd = g_fopen(vendor_path, "r")) == NULL) > - goto skip_device; > - > - if (fgets(tmp, sizeof(tmp), fd) == NULL) { > - fclose(fd); > - goto skip_device; > - } > - read_vendor_id = strtoul(tmp, NULL, 16); > - > - fclose(fd); > - > - if ((fd = g_fopen(product_path, "r")) == NULL) > - goto skip_device; > - > - if (fgets(tmp, sizeof(tmp), fd) == NULL) { > - fclose(fd); > - goto skip_device; > - } > - read_product_id = strtoul(tmp, NULL, 16); > - > - fclose(fd); > - > - if (vendor_id == read_vendor_id && > - product_id == read_product_id) { > - path_copy = g_strdup(usb_dev); > - matched_paths = g_slist_prepend(matched_paths, > - path_copy); > - } > - > -skip_device: > - g_free(vendor_path); > - g_free(product_path); > - } > - g_dir_close(devices_dir); > - > - /* For every matched device try to find a ttyUSBX subfolder. */ > - for (l = matched_paths; l; l = l->next) { > - subdir_path = NULL; > - > - device_path = g_strconcat(device_tree, l->data, NULL); > - > - if (!(device_dir = g_dir_open(device_path, 0, NULL))) { > - g_free(device_path); > - continue; > - } > - > - prefix = g_strconcat(l->data, ":", NULL); > - > - while ((file = g_dir_read_name(device_dir))) { > - if (g_str_has_prefix(file, prefix)) { > - subdir_path = g_strconcat(device_path, > - "/", file, NULL); > - break; > - } > - } > - g_dir_close(device_dir); > - > - g_free(prefix); > - g_free(device_path); > - > - if (subdir_path) { > - if (!(device_dir = g_dir_open(subdir_path, 0, NULL))) { > - g_free(subdir_path); > - continue; > - } > - g_free(subdir_path); > - > - while ((file = g_dir_read_name(device_dir))) { > - if (g_str_has_prefix(file, "ttyUSB")) { > - tty_path = g_strconcat("/dev/", > - file, NULL); > - sr_dbg("Found USB device %04x:%04x attached to %s.", > - vendor_id, product_id, tty_path); > - tty_devs = g_slist_prepend(tty_devs, > - tty_path); > - break; > - } > - } > - g_dir_close(device_dir); > - } > - } > - g_slist_free_full(matched_paths, g_free); > - > - return tty_devs; > -#else > - (void)vendor_id; > - (void)product_id; > - > - return NULL; > -#endif > -} > - > static int check_manufacturer(const char *manufacturer) > { > unsigned int i; > @@ -207,26 +57,16 @@ static int check_manufacturer(const char *manufacturer) > return SR_ERR; > } > > -static struct sr_dev_inst *hmo_probe_serial_device(const char *serial_device, > - const char *serial_options) > +static struct sr_dev_inst *hmo_probe_serial_device(struct sr_scpi_dev_inst *scpi) > { > struct sr_dev_inst *sdi; > struct dev_context *devc; > struct sr_scpi_hw_info *hw_info; > - struct sr_scpi_dev_inst *scpi; > > sdi = NULL; > devc = NULL; > - scpi = NULL; > hw_info = NULL; > > - if (!(scpi = scpi_dev_inst_new(di->priv, serial_device, serial_options))) > - goto fail; > - > - sr_info("Probing %s.", serial_device); > - if (sr_scpi_open(scpi) != SR_OK) > - goto fail; > - > if (sr_scpi_get_hw_id(scpi, &hw_info) != SR_OK) { > sr_info("Couldn't get IDN response."); > goto fail; > @@ -275,49 +115,7 @@ fail: > > static GSList *scan(GSList *options) > { > - GSList *devices; > - struct drv_context *drvc; > - struct sr_dev_inst *sdi; > - const char *serial_device, *serial_options; > - GSList *l, *tty_devs; > - unsigned int i; > - > - serial_device = NULL; > - serial_options = SERIALCOMM; Here it would be used and only overwritten if the user specifies the options. > - sdi = NULL; > - devices = NULL; > - drvc = di->priv; > - drvc->instances = NULL; > - > - if (sr_serial_extract_options(options, &serial_device, > - &serial_options) == SR_OK) { > - sdi = hmo_probe_serial_device(serial_device, serial_options); > - if (sdi != NULL) { > - devices = g_slist_append(devices, sdi); > - drvc->instances = g_slist_append(drvc->instances, sdi); > - } > - } else { > - tty_devs = NULL; > - > - for (i = 0; i < ARRAY_SIZE(ho_models); i++) { > - if ((l = auto_find_usb(ho_models[i].vendor_id, > - ho_models[i].product_id)) == NULL) > - continue; > - tty_devs = g_slist_concat(tty_devs, l); > - } > - > - for (l = tty_devs; l; l = l->next) { > - sdi = hmo_probe_serial_device(l->data, serial_options); > - if (sdi != NULL) { > - devices = g_slist_append(devices, sdi); > - drvc->instances = g_slist_append(drvc->instances, sdi); > - } > - } > - > - g_slist_free_full(tty_devs, g_free); > - } > - > - return devices; > + return sr_scpi_scan(di->priv, options, hmo_probe_serial_device); > } > How to fix this? You could pass the default options to scpi_scan() or you could pair them with the USB VID/PID. I don't know which option should be preferred, if any, maybe someone has a better idea. Despite of this small issue I think this is really great work, it moves us closer to having common driver/scpi boiler plate separated from the drivers. > static GSList *dev_list(void) > -- > 1.9.rc1 |
From: Damir J. <po...@po...> - 2014-02-05 13:29:51
|
Could you explain a little bit further in the commit message why this is needed, recently there was an librevisa scpi backend added, doesn't the librevisa backend offer everything an libusb backend would offer? I currently have no way to test this so I didn't do a full review of this either. Hopefully someone else can step in. |
From: Aurelien J. <au...@gn...> - 2014-02-06 00:47:19
|
On Wed, Feb 05, 2014 at 02:29:43PM +0100, Damir Jelić wrote: > Could you explain a little bit further in the commit message why > this is needed, recently there was an librevisa scpi backend added, > doesn't the librevisa backend offer everything an libusb backend > would offer? librevisa offers even more... It offers a dependency on a one developper C++ lib which has no release yet, and which is probably not packaged by any linux distro or BSD flavor... I'm not saying librevisa is useless, or that we shouldn't support it. It just think it is really worth having a portable usbtmc implementation without additional dependency. > I currently have no way to test this so I didn't do a full review of > this either. Hopefully someone else can step in. OK. Aurel |
From: Aurelien J. <au...@gn...> - 2014-02-05 23:53:46
|
On Wed, Feb 05, 2014 at 02:29:12PM +0100, Damir Jelić wrote: > On Tue, Feb 04, 2014 at 11:45:22PM +0100, Aurelien Jacobs wrote: > > note that sr_usb_find_serial() is copied from the hameg-hmo driver > > --- > > hardware/common/scpi.c | 68 +++++++++++++++++ > > hardware/common/scpi_serial.c | 167 ++++++++++++++++++++++++++++++++++++++++++ > > hardware/common/scpi_usbtmc.c | 24 ++++++ > > libsigrok-internal.h | 3 + > > 4 files changed, 262 insertions(+) > > > > diff --git a/hardware/common/scpi.c b/hardware/common/scpi.c > > index 2307a8b..21ff178 100644 > > --- a/hardware/common/scpi.c > > +++ b/hardware/common/scpi.c > > @@ -87,6 +87,74 @@ static const struct sr_scpi_dev_inst *scpi_devs[] = { > > #endif > > }; > > > > +static GSList *sr_scpi_scan_resource(struct drv_context *drvc, > > + const char *resource, const char *serialcomm, > > + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)) > > +{ > > + struct sr_scpi_dev_inst *scpi; > > + struct sr_dev_inst *sdi; > > + > > + if (!(scpi = scpi_dev_inst_new(drvc, resource, serialcomm))) > > + return NULL; > > + > > + if (sr_scpi_open(scpi) != SR_OK) { > > + sr_info("Couldn't open SCPI device."); > > + sr_scpi_free(scpi); > > + return NULL; > > + }; > > + > > + if ((sdi = probe_device(scpi))) > > + return g_slist_append(NULL, sdi); //FIXME list_new !!! > > This comment should be more verbose, also please don't use // for comments, > use /* */ instead. Oooops ! I only use // for personnal comments to myself, that's why it is really not verbose enough, but it shouldn't have left my computer. I just forgot about it. Thanks for catching it. Removed. It was supposed to remind me to check if there was no better function to initialize a new list with one element. I've now checked and I don't think there is anything better. > > +SR_PRIV GSList *sr_scpi_scan(struct drv_context *drvc, GSList *options, > > + struct sr_dev_inst *(*probe_device)(struct sr_scpi_dev_inst *scpi)) > > +{ > > + GSList *resources, *l, *d, *devices = NULL; > > + const char *resource = NULL; > > + const char *serialcomm = NULL; > > + unsigned i; > > + > > + for (l = options; l; l = l->next) { > > + struct sr_config *src = l->data; > > + switch (src->key) { > > + case SR_CONF_CONN: > > + resource = g_variant_get_string(src->data, NULL); > > + break; > > + case SR_CONF_SERIALCOMM: > > + serialcomm = g_variant_get_string(src->data, NULL); > > + break; > > + } > > + } > > You can use sr_serial_extract_options() for this Yes, and no... This is not specific to serial. It will also get conn string for other kind of resources (USBTMC, VXI...). The sr_serial_extract_options() would generate some debug output such as "Parsed serial device" which would be wrong in this case. In fact the sr_serial_extract_options() should be moved and converted to a more generic sr_conn_extract_options() or something like this. Feel free to send a patch ;-) > and you forgot to handle the default SERIALCOMM value that drivers > provide. This way serial devices need to provide the SERIALCOMM > parameter always which kind of defeats the purpose of the auto scan. Ouch ! This one is more serious, and I totally overlooked it... > Note: This actually doesn't break the auto scan for my Hameg HMO if it's > connected via USB but I consider this to be an accident. Good for you, but this indeed doesn't make it acceptable anyway. > > diff --git a/hardware/common/scpi_serial.c b/hardware/common/scpi_serial.c > > index 6e73c89..9ab9e16 100644 > > --- a/hardware/common/scpi_serial.c > > +++ b/hardware/common/scpi_serial.c > > @@ -22,6 +22,8 @@ > > #include "libsigrok-internal.h" > > > > [...] > > > > +/** > > + * Find USB serial devices via the USB vendor ID and product ID. > > + * > > + * @param vendor_id Vendor ID of the USB device. > > + * @param product_id Product ID of the USB device. > > + * > > + * @return A GSList of strings containing the path of the serial device or > > + * NULL if no serial device is found. The returned list must be freed > > + * by the caller. > > + */ > > +static GSList *sr_usb_find_serial(uint16_t vendor_id, uint16_t product_id) > > +{ > > +#ifdef __linux__ > > + const gchar *usb_dev; > > + const char device_tree[] = "/sys/bus/usb/devices/"; > > + GDir *devices_dir, *device_dir; > > [...] > > + g_slist_free_full(matched_paths, g_free); > > + > > + return tty_devs; > > +#else > > + (void)vendor_id; > > + (void)product_id; > > + > > + return NULL; > > +#endif > > +} > > I don't think that this function should go into scpi_serial. There is > nothing SCPI specific about it. Totally agree. I thought about this while putting this function here, but I wasn't sure where else could I put it. > I think it would make sense to put it in serial.c Sounds like a decent proposition. Adopted. > and enable auto scan functionality for all serial-USB devices. I agree. But that's another task and I'll probably leave it for someone else. > > +static GSList *scpi_serial_scan(struct drv_context *drvc) > > +{ > > + GSList *l, *resources = NULL; > > + unsigned i; > > + > > + (void)drvc; > > Why do we need drvc here? Do we expect it to be used in the near future? It is used in my usbtmc_libusb implementation. More generally, it is needed for example to access the global libusb_ctx. > > diff --git a/hardware/common/scpi_usbtmc.c b/hardware/common/scpi_usbtmc.c > > index 6cd7964..1ffc34a 100644 > > --- a/hardware/common/scpi_usbtmc.c > > +++ b/hardware/common/scpi_usbtmc.c > > @@ -37,6 +37,29 @@ struct usbtmc_scpi { > > int response_bytes_read; > > }; > > > > +static GSList *scpi_usbtmc_scan(struct drv_context *drvc) > > +{ > > + GSList *resources = NULL; > > + GDir *dir; > > + const char *dev_name; > > + char *resource; > > + > > + (void)drvc; > > + > > + if (!(dir = g_dir_open("/sys/class/usbmisc/", 0, NULL))) > > + if (!(dir = g_dir_open("/sys/class/usb/", 0, NULL))) > > + return NULL; > > + while ((dev_name = g_dir_read_name(dir))) { > > + if (strncmp(dev_name, "usbtmc", 6)) > > + continue; > > + resource = g_strconcat("/dev/", dev_name, NULL); > > + resources = g_slist_append(resources, resource); > > + } > > + g_dir_close(dir); > > + > > + return resources; > > +} > > This function is not portable, Really ? It only try to open two different directories, and if they do not exist, it will just return cleanly. This should work on any OS that do not have those 2 directories. > you should guard it with an ifdef Actually this whole file won't do anything useful on non-linux OS. So better not clutter it with ifdef and just exclude it from compilation on anything non-linux. > also do we know if usbtmc devices use SCPI exclusively? Do you think > it would make sense to put this into usb.c? I bet some usbtmc devices won't be really SCPI compliant (no *IDN?, or even there own non-SCPI based commands), but I guess they will all be sufficiently close to SCPI that we can use our SCPI api with them anyway. After all, scpi_send() and scpi_read() just map to sending and receiving bytes following the USBTMC protocol. It doesn't assume anything regarding the data that is transfered. So I don't think it make sense to put this into usb.c. Aurel |
From: Aurelien J. <au...@gn...> - 2014-02-06 00:02:53
|
On Wed, Feb 05, 2014 at 02:29:27PM +0100, Damir Jelić wrote: > On Tue, Feb 04, 2014 at 11:45:24PM +0100, Aurelien Jacobs wrote: > > --- > > hardware/hameg-hmo/api.c | 206 +---------------------------------------------- > > 1 file changed, 2 insertions(+), 204 deletions(-) > > > > diff --git a/hardware/hameg-hmo/api.c b/hardware/hameg-hmo/api.c > > index 5abd8c5..241e86b 100644 > > --- a/hardware/hameg-hmo/api.c > > +++ b/hardware/hameg-hmo/api.c > > @@ -18,7 +18,6 @@ > > */ > > > > #include <stdlib.h> > > -#include <glib/gstdio.h> > > #include "protocol.h" > > > > #define SERIALCOMM "115200/8n1/flow=1" > > Here is the default SERIALCOMM parameter that I mentioned already. > > [...] > > How to fix this? You could pass the default options to scpi_scan() or > you could pair them with the USB VID/PID. Pairing them with the USB VID/PID sounds like the best option to me. This way, if someday we manage to write a generic scpi-scope driver, it won't need to know if it is talking to a rigol through usbtmc or to a hameg through a HO730 serial interface. The serialcomm is associated with the serial interface itself, not with the scope driver. So I implemented this. Fixed patchset is available here: https://github.com/aurelj/libsigrok/commits/scpi If you want me to post it on the mailing list again, just tell me. > Despite of this small issue I think this is really great work Thanks. > it moves us closer to having common driver/scpi boiler plate > separated from the drivers. That was the goal :-) Aurel |
From: Damir J. <po...@po...> - 2014-02-06 12:25:47
|
On Thu, Feb 06, 2014 at 01:02:45AM +0100, Aurelien Jacobs wrote: > On Wed, Feb 05, 2014 at 02:29:27PM +0100, Damir Jelić wrote: > > On Tue, Feb 04, 2014 at 11:45:24PM +0100, Aurelien Jacobs wrote: > > > --- > > > hardware/hameg-hmo/api.c | 206 +---------------------------------------------- > > > 1 file changed, 2 insertions(+), 204 deletions(-) > > > > > > diff --git a/hardware/hameg-hmo/api.c b/hardware/hameg-hmo/api.c > > > index 5abd8c5..241e86b 100644 > > > --- a/hardware/hameg-hmo/api.c > > > +++ b/hardware/hameg-hmo/api.c > > > @@ -18,7 +18,6 @@ > > > */ > > > > > > #include <stdlib.h> > > > -#include <glib/gstdio.h> > > > #include "protocol.h" > > > > > > #define SERIALCOMM "115200/8n1/flow=1" > > > > Here is the default SERIALCOMM parameter that I mentioned already. > > > > [...] > > > > How to fix this? You could pass the default options to scpi_scan() or > > you could pair them with the USB VID/PID. > > Pairing them with the USB VID/PID sounds like the best option to me. > This way, if someday we manage to write a generic scpi-scope driver, it > won't need to know if it is talking to a rigol through usbtmc or to a > hameg through a HO730 serial interface. The serialcomm is associated > with the serial interface itself, not with the scope driver. > > So I implemented this. > > Fixed patchset is available here: > https://github.com/aurelj/libsigrok/commits/scpi > If you want me to post it on the mailing list again, just tell me. > You don't need to post it again for me since it's unlikely that I will have further comments, but I generally think that it is a good idea to post patches on the mailing list since it catches a broader audience than posting a link to a github repo on IRC. For example if I didn't pay attention on IRC the moment you posted the link I would have missed this patch set and I wouldn't review it (I'm not requesting you to post it just explaining why I always post on the mailing list and why I consider this to be good practice). |
From: Damir J. <po...@po...> - 2014-02-06 12:25:59
|
On Thu, Feb 06, 2014 at 12:53:37AM +0100, Aurelien Jacobs wrote: > On Wed, Feb 05, 2014 at 02:29:12PM +0100, Damir Jelić wrote: > > On Tue, Feb 04, 2014 at 11:45:22PM +0100, Aurelien Jacobs wrote: > > > [...] > > > +/** > > > + * Find USB serial devices via the USB vendor ID and product ID. > > > + * > > > + * @param vendor_id Vendor ID of the USB device. > > > + * @param product_id Product ID of the USB device. > > > + * > > > + * @return A GSList of strings containing the path of the serial device or > > > + * NULL if no serial device is found. The returned list must be freed > > > + * by the caller. > > > + */ > > > +static GSList *sr_usb_find_serial(uint16_t vendor_id, uint16_t product_id) > > > +{ > > > +#ifdef __linux__ > > > + const gchar *usb_dev; > > > + const char device_tree[] = "/sys/bus/usb/devices/"; > > > + GDir *devices_dir, *device_dir; > > > [...] > > > + g_slist_free_full(matched_paths, g_free); > > > + > > > + return tty_devs; > > > +#else > > > + (void)vendor_id; > > > + (void)product_id; > > > + > > > + return NULL; > > > +#endif > > > +} > > > > I don't think that this function should go into scpi_serial. There is > > nothing SCPI specific about it. > > Totally agree. I thought about this while putting this function here, > but I wasn't sure where else could I put it. > > > I think it would make sense to put it in serial.c > > Sounds like a decent proposition. Adopted. > > > and enable auto scan functionality for all serial-USB devices. > > I agree. But that's another task and I'll probably leave it for > someone else. I didn't actually request you to do it, I just wanted to explain why it would be useful somewhere else. > > > > +static GSList *scpi_serial_scan(struct drv_context *drvc) > > > +{ > > > + GSList *l, *resources = NULL; > > > + unsigned i; > > > + > > > + (void)drvc; > > > > Why do we need drvc here? Do we expect it to be used in the near future? > > It is used in my usbtmc_libusb implementation. More generally, it is > needed for example to access the global libusb_ctx. > > > > diff --git a/hardware/common/scpi_usbtmc.c b/hardware/common/scpi_usbtmc.c > > > index 6cd7964..1ffc34a 100644 > > > --- a/hardware/common/scpi_usbtmc.c > > > +++ b/hardware/common/scpi_usbtmc.c > > > @@ -37,6 +37,29 @@ struct usbtmc_scpi { > > > int response_bytes_read; > > > }; > > > > > > +static GSList *scpi_usbtmc_scan(struct drv_context *drvc) > > > +{ > > > + GSList *resources = NULL; > > > + GDir *dir; > > > + const char *dev_name; > > > + char *resource; > > > + > > > + (void)drvc; > > > + > > > + if (!(dir = g_dir_open("/sys/class/usbmisc/", 0, NULL))) > > > + if (!(dir = g_dir_open("/sys/class/usb/", 0, NULL))) > > > + return NULL; > > > + while ((dev_name = g_dir_read_name(dir))) { > > > + if (strncmp(dev_name, "usbtmc", 6)) > > > + continue; > > > + resource = g_strconcat("/dev/", dev_name, NULL); > > > + resources = g_slist_append(resources, resource); > > > + } > > > + g_dir_close(dir); > > > + > > > + return resources; > > > +} > > > > This function is not portable, > > Really ? It only try to open two different directories, and if they do > not exist, it will just return cleanly. This should work on any OS that > do not have those 2 directories. > When I say it isn't portable I don't mean that it will break on other platforms, I mean that it won't do what it's supposed to do on other platforms. sr_usb_find_serial() wouldn't break either but it has a ifdef guard so it is clear that it isn't implemented for other platforms. > > you should guard it with an ifdef > > Actually this whole file won't do anything useful on non-linux OS. So > better not clutter it with ifdef and just exclude it from compilation on > anything non-linux. > I disagree, we should only guard the parts that need to be rewritten (under the assumption that we can do this without any breakage) so it's clear for other people what parts need to be implemented if they want this functionality on their platform. Maybe it would be useful to print a warning that this functionality is missing so users would be aware of this and they would possibly be motivated to implement it (but maybe this is just wishful thinking). > > also do we know if usbtmc devices use SCPI exclusively? Do you think > > it would make sense to put this into usb.c? > > I bet some usbtmc devices won't be really SCPI compliant (no *IDN?, or > even there own non-SCPI based commands), but I guess they will all be > sufficiently close to SCPI that we can use our SCPI api with them > anyway. After all, scpi_send() and scpi_read() just map to sending and > receiving bytes following the USBTMC protocol. It doesn't assume > anything regarding the data that is transfered. > So I don't think it make sense to put this into usb.c. Ok. Thanks, Damir. |
From: Aurelien J. <au...@gn...> - 2014-02-07 22:14:19
|
On Thu, Feb 06, 2014 at 01:25:50PM +0100, Damir Jelić wrote: > On Thu, Feb 06, 2014 at 12:53:37AM +0100, Aurelien Jacobs wrote: > > On Wed, Feb 05, 2014 at 02:29:12PM +0100, Damir Jelić wrote: > > > On Tue, Feb 04, 2014 at 11:45:22PM +0100, Aurelien Jacobs wrote: > > > > > [...] > > > > > diff --git a/hardware/common/scpi_usbtmc.c b/hardware/common/scpi_usbtmc.c > > > > index 6cd7964..1ffc34a 100644 > > > > --- a/hardware/common/scpi_usbtmc.c > > > > +++ b/hardware/common/scpi_usbtmc.c > > > > @@ -37,6 +37,29 @@ struct usbtmc_scpi { > > > > int response_bytes_read; > > > > }; > > > > > > > > +static GSList *scpi_usbtmc_scan(struct drv_context *drvc) > > > > +{ > > > > + GSList *resources = NULL; > > > > + GDir *dir; > > > > + const char *dev_name; > > > > + char *resource; > > > > + > > > > + (void)drvc; > > > > + > > > > + if (!(dir = g_dir_open("/sys/class/usbmisc/", 0, NULL))) > > > > + if (!(dir = g_dir_open("/sys/class/usb/", 0, NULL))) > > > > + return NULL; > > > > + while ((dev_name = g_dir_read_name(dir))) { > > > > + if (strncmp(dev_name, "usbtmc", 6)) > > > > + continue; > > > > + resource = g_strconcat("/dev/", dev_name, NULL); > > > > + resources = g_slist_append(resources, resource); > > > > + } > > > > + g_dir_close(dir); > > > > + > > > > + return resources; > > > > +} > > > > > > This function is not portable, > > > > Really ? It only try to open two different directories, and if they do > > not exist, it will just return cleanly. This should work on any OS that > > do not have those 2 directories. > > > > When I say it isn't portable I don't mean that it will break on other > platforms, I mean that it won't do what it's supposed to do on other > platforms. sr_usb_find_serial() wouldn't break either but it has a ifdef > guard so it is clear that it isn't implemented for other platforms. > > > > you should guard it with an ifdef > > > > Actually this whole file won't do anything useful on non-linux OS. So > > better not clutter it with ifdef and just exclude it from compilation on > > anything non-linux. > > > > I disagree, we should only guard the parts that need to be rewritten > (under the assumption that we can do this without any breakage) so it's > clear for other people what parts need to be implemented if they want > this functionality on their platform. The point is that this whole file is about supporting the linux only /dev/usbtmc* device and kernel driver. AFAIK, no other OS has anything looking like a usbtmc kernel driver exposed through a character device. So in partice, this whole file is linux only and it is not possible to port it on other platform. That's why I think it is pointless (and harmful) to cripple this file with some platform specific #ifdef. And that's also one of the reason why I wrote a libusb based usbtmc implementation: to have a portable usbtmc implementation. Aurel |
From: Damir J. <po...@po...> - 2014-02-07 22:26:34
|
On Fri, Feb 07, 2014 at 11:14:09PM +0100, Aurelien Jacobs wrote: > > The point is that this whole file is about supporting the linux only > /dev/usbtmc* device and kernel driver. AFAIK, no other OS has anything > looking like a usbtmc kernel driver exposed through a character device. > So in partice, this whole file is linux only and it is not possible > to port it on other platform. > That's why I think it is pointless (and harmful) to cripple this file > with some platform specific #ifdef. > And that's also one of the reason why I wrote a libusb based usbtmc > implementation: to have a portable usbtmc implementation. > Oh right. I totally forgot that this whole usbtmc implementation will never be supported on other platforms. Sorry for the noise. Thanks, Damir. |