[sane-devel] MP730 driver broken since pixma-0.12.2
m. allan noah
kitno455 at gmail.com
Sat Apr 25 14:57:15 UTC 2009
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
>>
>> >
>>
>>
>
>
--
"The truth is an offense, but not a sin"
More information about the sane-devel
mailing list