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
------------------------------------------------------------------------------
Managing the Performance of Cloud-Based Applications
Take advantage of what the Cloud has to offer - Avoid Common Pitfalls.
Read the Whitepaper.
http://pubads.g.doubleclick.net/gampad/clk?id=121051231&iu=/4140/ostg.clktrk
_______________________________________________
sigrok-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/sigrok-devel
