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
------------------------------------------------------------------------------
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
