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