[sane-devel] MP730 driver broken since pixma-0.12.2 - proposed patch to sanei_usb.c
Nicolas Martin
nicolas0martin at gmail.com
Wed May 20 20:22:16 UTC 2009
Some progress here with the sanei_usb.c code failing on Pixma MP730 (and
probably some other Pixma), following some recent exchanges with Wade.
I looked at the sanei_usb.c file, especially the 2 functions:
sanei_usb_init()
sanei_usb_open()
These 2 functions should work closely together, but it turns out that:
- sanei_usb_init() performs a first analysis of which usb device
could be a scanner, then stores the candidate scanners and usb
interfaces they are hooked to.
This code and analysis looks correct to me, AFAIK.
- Then, sanei_usb_open() makes another kind of weaker analysis,
not taking into account sanei_usb_init() results, and looping
blindly through all usb configs and interfaces to seek for
endpoints.
This looks wrong to me (agree with you Allan), as it turns out
that it finally grabs wrong endpoints.
So we tested a small change in sanei_usb_open(), to loop first into
those interfaces selected previously in sanei_usb_init()
The result is correct, and this time, the good endpoints are selected
for the MP730 scanner.
This patch to sanei_usb.c follows.
Before going into a full rewrite of sanei_usb.c requiring efforts and
time, I propose to commit this patch.
Any trouble with that ?
As this file is common to many scanners supported by Sane, feedback is
essential if any discrepancies are noticed.
Nicolas
diff --git a/sanei/sanei_usb.c b/sanei/sanei_usb.c
index 79745f0..e706dd4 100644
--- a/sanei/sanei_usb.c
+++ b/sanei/sanei_usb.c
@@ -1236,6 +1236,9 @@ sanei_usb_open (SANE_String_Const devname,
SANE_Int * dn)
/* Loop through all of the alternate settings */
for (a = 0; a <
dev->config[c].interface[i].num_altsetting; a++)
{
+ /* For config[0], seek only interfaces found in
sanei_usb_init */
+ if (c == 0 && i != devices[devcount].interface_nr)
+ continue;
DBG (5, "sanei_usb_open: configuration nr: %d\n", c);
DBG (5, "sanei_usb_open: interface nr: %d\n", i);
@@ -1540,6 +1543,10 @@ sanei_usb_open (SANE_String_Const devname,
SANE_Int * dn)
/* Loop through all of the alternate settings */
for (a = 0; a < config->interface[i].num_altsetting; a++)
{
+ /* For config[0], seek only interfaces found in
sanei_usb_init */
+ if (c == 0 && i != devices[devcount].interface_nr)
+ continue;
+
const struct libusb_interface_descriptor *interface;
DBG (5, "sanei_usb_open: configuration nr: %d\n", c);
Le samedi 25 avril 2009 à 10:57 -0400, m. allan noah a écrit :
> Have to agree with Wade here. sanei_usb selects a config and
> interface, then loops thru all configs and interfaces, and pulls out
> endpoints. That's just broken, and it's not only affecting this
> machine. We'll re-write sanei_usb after the release.
> I'll start a new thread at that time.
>
> allan
>
> On Sat, Apr 25, 2009 at 8:14 AM, Wade Fitzpatrick
> <wade.fitzpatrick at gmail.com> wrote:
> > Nicolas
> >
> > My latest testing was on kernel 2.6.27-gentoo-r8. I don't see why a
> > different kernel version would change the interface/endpoint numbering
> > anyway - surely that is hardware specific to the MP730. I have the latest
> > version of everything: kernel 2.6.27-gentoo-r8, libusb-0.1.12-r5, sane
> > backends 1.0.20cvs. It's not a version problem, the MP730 has *never* worked
> > using libsane. It worked enough to do scans using Wittawat Yamwong's
> > ``scan'' utility until pixma-0.12.1 but never using libsane. We just never
> > got that far in testing it back then.
> >
> > Ubuntu users also complained about it but obviously never bothered to ask on
> > sane-devel.
> > http://ubuntuforums.org/showthread.php?t=592643&highlight=mp730
> > http://ubuntuforums.org/showthread.php?t=962535&highlight=mp730
> >
> > I didn't mean to suggest that the patch I have is ready for inclusion in
> > CVS, only that it should apply to current if somebody wants to use it for
> > testing. I'm well aware that fixing this properly will mean a significant
> > rewrite of sanei/sanei_usb.c and will probably touch most if not all backend
> > drivers if you solve it the way I would, which is to specify the requisite
> > interfaces and endpoints in the [backend]_config_t structure and pass that
> > into sanei_usb code.
> >
> > Let's take the remaining problems into another thread.
> >
> > Cheers,
> > Wade.
> >
> > 2009/4/25 Nicolas Martin <nicolas.martin at freesurf.fr>
> >>
> >> Hi Wade,
> >>
> >> Sorry if act sometimes like "guardians of the temple", but we need to be
> >> very accurate on the changes we make into the code, as there are lots of
> >> shared code in Sane backends (especially pixma) and libs.
> >>
> >> - So for points 1 and 2: we can close those.
> >>
> >> - Point 3: this is still to investigate, as we must be sure this
> >> endpoint issue is not brought by kernel 2.6.18-ck1 (BTW, do you know
> >> what does the -ck1 bring ?). This ck kernel is dated sept-2006 (on
> >> kernel.org), and after googling a little bit "2.6.18 usb endpoints" it
> >> looks like there are some issues around that...
> >> My suggestion would still be to test with a recent kernel, with which we
> >> are sure Sane and endpoints work fine (recent Ubuntu distros can help a
> >> lot for that) and check if we can reproduce your endpoints issue.
> >> My fear here is not to introduce into the Sane code, some turnarounds
> >> and tweaks only for specific kernels, buggy on usb.
> >>
> >> Concerning points 1 and 2 you raise, this is interesting:
> >>
> >> The pixma_mp730.c file has been left almost untouched for a while (no
> >> requests or testing ability so far), so it may be in a less "enhanced"
> >> state than other ones covering more recent Pixma models, that have been
> >> recently updated.
> >> Dennis Lou wrote a while back the pixma_imageclass.c file, based on the
> >> pixma_mp730.c file, and which covers several Canon ImageClass models in
> >> the pixma backend. The code has been nicely brushed up, and is now in a
> >> better shape than mp730's one.
> >> So for the 2 issues you raise, I'll take a look and compare the
> >> pixma_mp730.c code with pixma_imageclass.c and pixma_mp150.c , probably
> >> will help a lot to solve those issues, thanks to your cooperation for
> >> testing and feedback.
> >>
> >> Nicolas
> >>
> >>
> >> Le vendredi 24 avril 2009 à 01:19 +1000, Wade Fitzpatrick a écrit :
> >> > Hi Nicolas
> >> >
> >> > Thanks for reviewing my patches. I have to agree with you... too many
> >> > late nights... must be coding in my sleep...
> >> > 1. I had the PIXMA_CAP_EVENTS flag removed at some stage as
> >> > that's what got the driver working again between pixma-0.12.1
> >> > and pixma-0.12.2. It doesn't seem to make any difference now
> >> > and I can't explain why so just ignore it. I don't think the
> >> > resolution and maximum scan area parameters are correct but
> >> > that's an issue for another day.
> >> > 2. I don't remember adding that line but I guess I must have. It
> >> > may have been an accidental 'p' in vim. Ignore this one too.
> >> > 3. I checked out a fresh new cvs today and applied just the lines
> >> > you pasted below to sanei/sanei_usb.c - it works just fine
> >> > with that change alone. I ran through about 30 successful
> >> > tests too: using ADF, button-controlled, ADF
> >> > +button-controlled, Gray, Color and at various resolutions and
> >> > geometries on and off the platen glass, pressing Cancel during
> >> > a scan, pressing Ctrl-C during a scan, jamming the paper in
> >> > the ADF, batch scans etc.
> >> > The only problems I can find now are:
> >> > 1. Scanning at 1200dpi Grayscale. Sometimes it hangs depending on
> >> > the x and y values - the larger the values, the sooner it
> >> > fails. If it does succeed, the data is always bogus.
> >> > 2. The return code after a successful scan is always ECANCELED
> >> > which causes a problem for batch mode as it will only scan one
> >> > page. It doesn't seem to matter for single scans.
> >> > Does this hold true for other models too?
> >> > Does batch mode succeed on other models with ADF?
> >> >
> >> > I can't believe I've spent so much time on such a trivial change :(
> >> >
> >> > Regards,
> >> > Wade.
> >> > 2009/4/23 Nicolas <nicolas0martin at gmail.com>
> >> > Hi Wade,
> >> >
> >> > Good news that you succeeded in having your MP730 work after
> >> > 2,5 years !
> >> >
> >> > I did carefully inspect the small changes you have done to the
> >> > pixma backend, but there are some points I don't understand
> >> > clearly, or maybe I've missed sthg, but you can surely give us
> >> > some more info here:
> >> >
> >> > 1/ File backend/pixma_mp730.c
> >> >
> >> > I don't understand the change you've brought here individually
> >> > for each device, the line that you replaced for MP730:
> >> >
> >> > - DEVICE ("Canon MultiPASS MP730", "MP730", MP730_PID, 1200,
> >> > 637, 868, PIXMA_CAP_ADF),
> >> > + DEVICE ("Canon MultiPASS MP730", "MP730", MP730_PID, 1200,
> >> > 637, 868, PIXMA_CAP_ADF|PIXMA_CAP_EVENTS),
> >> >
> >> > has exactly the same effect than the original one in the
> >> > device definition:
> >> > - PIXMA_CAP_GRAY|PIXMA_CAP_EVENTS|cap \
> >> > + PIXMA_CAP_GRAY|cap /* capabilities */
> >> > \
> >> >
> >> > Here, you move the PIXMA_CAP_EVENTS flag from the global
> >> > device definition to the individual definition of each device.
> >> >
> >> > - For MP730, this looks to have the same effect, or where is
> >> > the difference ?
> >> > - But this changes the definition for all other devices here,
> >> > and I would not like to bring here any regression to other
> >> > devices.
> >> >
> >> > So: could you give the motivation for this modification here ?
> >> >
> >> > 2/ file pixma_io_sanei.c
> >> >
> >> > The only diff I see here adds a PDBG debug statement:
> >> > + PDBG (pixma_dbg (3, "sanei_usb_read_bulk returned error
> >> > code: %i\n", error));
> >> >
> >> > The other line is a commented line:
> >> >
> >> > + /*error = pixma_map_status_errno (pixma_get_be16
> >> > (buf)); */
> >> >
> >> > And I don't understand the comment you added:
> >> > "The call to |pixma_map_status_errno() in |
> >> > backend/pixma_io_sane.c:pixma_read()
> >> > is in the wrong place as |pixma_read() may be re-entrant so as
> >> > to read a partial buffer."
> >> >
> >> > - There are _no_ calls to |pixma_map_status_errno() in current
> >> > cvs ||pixma_read()
> >> > The one here is added as part of you modifications, but is
> >> > commented out.
> >> > - This part of the pixma code is common to all pixma devices
> >> > - The modifications you added here do not bring any functional
> >> > change.
> >> >
> >> > So one again, what are those changes for ?
> >> >
> >> > 3/ File sanei_usb.c
> >> >
> >> > This is the only file where modifications appear to have a
> >> > functional impact:
> >> > |+ /* don't try to read non-scanner device
> >> > classes */
> >> > + if (interface->bInterfaceClass == 0x07) {
> >> > + DBG (3, "sanei_usb_open: skipping Printer
> >> > interface\n");
> >> > + continue;
> >> > + }
> >> > + if (interface->bInterfaceClass == 0x08) {
> >> > + DBG (3, "sanei_usb_open: skipping Mass
> >> > Storage interface\n");
> >> > + continue;
> >> > + }
> >> > + if (interface->bInterfaceClass == 0xff && i
> >> > == 3) {
> >> > + DBG (3, "sanei_usb_open: skipping second
> >> > Vendor Specific interface\n");
> >> > + continue;
> >> > + }
> >> >
> >> > As Allan pointed out, this may need some deeper analysis to
> >> > the sanei_usb code, but modifications in this file are very
> >> > sensitive, as they impact all usb devices
> >> > supported by Sane, not only pixma.
> >> > As for the 1.0.20 release, I would not recommend to add this
> >> > change now, as it probably needs deeper testing and
> >> > understanding.
> >> >
> >> > In other words, to step further, Wade:
> >> > Could you give a try with the current Sane CVS files and patch
> >> > only file sanei_usb.c, see if it is still ok?
> >> > If not, this means there's something I did not catch, so we'll
> >> > need to talk again to finalize the changes, if any, required
> >> > for MP730.
> >> >
> >> > Nicolas
> >>
> >> >
> >>
> >>
> >
> >
>
>
>
More information about the sane-devel
mailing list