[sane-devel] xerox_mfp usb malfunction

[Warren Turkal]

2011/3/9 ABC <abc at telekom.ru>

> |   for (tr = available_transports; tr->ttype; tr++) {
> |
> |    DBG (4, "%s: %s: tr->ttype=%s\n", __FUNCTION__, devname, tr->ttype);
> |
> |    if (!strncmp (devname, tr->ttype, strlen(tr->ttype)) ||
> |        (!strncmp (devname, "libusb", strlen("libusb")) &&
> |         !strncmp (tr->ttype, "usb", strlen("usb"))))
> |      break;
>
> That is more hacky to me, becasue 1) you using/duplicating internal
> representation of device name "usb" from available_transports struct which
> somewhere else in the code; 2) you using/duping internal representation of
> "libusb" which is very very far away somewhere else in deepness of sanei
> (or libusb?) code. If we using what sanei gives us then we should not
> assume
> what library it is using for usb access and it's device names. But for
> "tcp" - that is what we invented to handle intentionally. So we only
> need work out special case of "tcp" and keep rest logic unchanged.
>

I couldn't agree more. That's why I didn't want my patch to be committed to
the code base.

 What i think should actually happen is that sanei should have it's own
method for dealing with usb (and other busses) in whatever form it takes
(e.g. /dev/scanner dev file or libusb or scsi device or whatever). Sanei
should call any callbacks into the backend with a generic form for the bus
type instead of exposing it's innards. Assuming that a string is the best
form for that generic form, something like "usb:vendor_id:product_id" is
probably reasonable.

Even more useful might be a struct with a type field (maybe a string or
integer) and a union field that allows a pointer to a struct for each of the
different busses that is supported by SANE. For example, I think it would be
similar to the following:
enum device_type {
  USB_DEVICE_TYPE,
  SCSI_DEVICE_TYPE,
  PARALLEL_DEVICE_TYPE,
};

struct device {
  enum device_type device_type;
  union info {
    struct usb_device_info* usb;
    struct scsi_device_info* scsi;
    struct parallel_device_info* parallel;
  };
};

struct usb_device_info {
  int16_t vendor_id;
  int16_t product_id;
  ...other fields...
};

Then the attach callback signature could be:
static SANE_Status
attach(struct device)

One could even take it a step further and have an attach_funcs struct that
we could register handlers for different busses:
stuct attach_funcs {
  SANE_Status (*usb_attach)(struct usb_device_info*);
  SANE_Status (*scsi_attach)(struct scsi_device_info*);
  SANE_Status (*parallel_attach)(struct parallel_device_info*);
};

const struct attach_funcs DEFAULT_ATTACH_FUNCS = {
  usb_attach = NULL;
  scsi_attach = NULL;
  parallel_attach = NULL;
};

Then one could just do something like the following for a USB only backend:
struct attach_funcs xerox_mfp_attach_funcs = DEFAULT_ATTACH_FUNCS;
xerox_mfp_attach_funcs.usb_attach = xerox_mfp_usb_attach;

Then a pointer to that struct can be passed instead of a single function
pointer that takes a string. The sanei code could know to call the
appropriate bus function if it's not NULL. The primary point of this design
is that the sanei code could decide which bus to use and call the
appropriate callback. I think that could actually improve debugability.

I'm sure I could take this further, but I am going to stop now. :) However,
I would like to say that I think using ideas like this across all backends
could make the backend code much easier to read since it has the potential
to normalize some of the code used in the backends.


> And 3) original intent of tr->ttype name is not fulfilled a) it was
> wrong becasue it assumes usb device names which it should' not,
> b) we still need to compare to "libusb" as for "usb".
>

I agree.

I just think that there is a lot room for improvement in the backend code in
general.

I know my original patch was a hack. It was an attempt to point out the real
problem. It seems to have worked. :)


> Thanks for your valuable help. :)


No problem. Thanks for discussing the solution. This is really what make
free software great in my opinion. I really only do this because I want to
see SANE and other projects improve and become more useful to users.

wt
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/sane-devel/attachments/20110309/12a22bc1/attachment-0001.htm>