[sane-devel] MP730 driver broken since pixma-0.12.2

[Nicolas Martin]

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

>