Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Some USB printers do not like set_configuration and set_interface. #3965

Closed
michaelrsweet opened this Issue Oct 21, 2011 · 5 comments

Comments

Projects
None yet
1 participant
Collaborator

michaelrsweet commented Oct 21, 2011

Version: 1.5-current
CUPS.org User: henry128

See http://blog.stuffedcow.net/2011/10/samsung-linux-cups-usb-printing/

Some printers (e.g., Samsung ML-2010, Samsung ML-1740) don't tolerate being sent a SET_CONFIGURATION or SET_INTERFACE request after the printer has already been configured. Sending these extra requests are legal under the USB spec, but it causes my printer to often ignore subsequent print jobs after the first one. These problems did not occur using usblp.

The libusb backend sends SET_CONFIGURATION and SET_INTERFACE requests before every print job. Under usblp, these are not (verified by capturing a USB trace).

This patch first makes the libusb backend first query whether sending SET_CONFIGURATION and SET_INTERFACE are necessary before sending them, bringing the behaviour closer to usblp. It also fixes the problems with my Samsung printers.

With this patch, the only remaining difference between usblp and libusb is that a GET_CONFIGURATION request is sent before each print job, whereas usblp assumes without verification that the printer is already using the correct configuration. The GET_CONFIGURATION request was added by this patch because I wasn't comfortable with blindly assuming the currently-selected configuration is correct.

This problem is probably related to STR #3964, and may also fix that issue, without needing to do an intrusive USB reset.

Collaborator

michaelrsweet commented Nov 11, 2011

CUPS.org User: till.kamppeter

I have applied this patch and the patch of STR #3978 to the package in Ubuntu Oneiric now and asked the people suffering the following Ubuntu bugs for testing:

https://bugs.launchpad.net/ubuntu/+source/cups/+bug/887094
usb printer backend crashes

https://bugs.launchpad.net/ubuntu/+source/cups/+bug/872483
laser printer only prints first job correct

https://bugs.launchpad.net/ubuntu/+source/cups/+bug/883169
Brother MFC-8840D usb printer not detected

https://bugs.launchpad.net/ubuntu/+source/cups/+bug/793244
After suspend/hibernate usb printer backend repeatedly crashes

https://bugs.launchpad.net/ubuntu/+source/cups/+bug/564917
Ubuntu 10.04 does not recognize local printer connected via
USB/parallel adapter cable

For the first three I got answers and they are definitely fixed by the two patches. On the other two I am still waiting for an answer but they are most probably also fixed.

Together with the USB fix on the Linux kernel (STR #3884) it looks like that all known problems of libusb-based USB printing (after deprecation of the "usblp" kernel module) are solved together with these two patches on the libusb-based "usb" CUPS backend. So I highly recommend the inclusion of the two patches in the upstream repository of CUPS, for both CUPS 1.5.x and 1.6.x.

Collaborator

michaelrsweet commented Dec 14, 2011

CUPS.org User: mike

This should be applied to CUPS 1.5.

Collaborator

michaelrsweet commented Jan 27, 2012

CUPS.org User: mike

Fixed in Subversion repository.

Collaborator

michaelrsweet commented Jan 27, 2012 edited

"cups-1.6-r10087-usb-skip-interface-altsetting.patch":

Index: usb-libusb.c
===================================================================
--- usb-libusb.c    (revision 10087)
+++ usb-libusb.c    (working copy)
@@ -631,6 +631,7 @@
             int           verbose) /* I - Update connecting-to-device state? */
 {
   int  number;             /* Configuration/interface/altset numbers */
+  char current_bConfiguration;


  /*
@@ -647,27 +648,40 @@
   if ((printer->handle = usb_open(printer->device)) == NULL)
     return (-1);

- /*
-  * Then set the desired configuration...
-  */

   if (verbose)
     fputs("STATE: +connecting-to-device\n", stderr);

+ /*
+  * Set the desired configuration, but only if it needs changing. Some
+  * printers (e.g., Samsung) don't like usb_set_configuration. It will succeed,
+  * but the following print job is sometimes silently lost by the printer.
+  */
+  if (usb_control_msg(printer->handle,
+                USB_TYPE_STANDARD | USB_ENDPOINT_IN | USB_RECIP_DEVICE,
+                8, /* GET_CONFIGURATION */
+                0, 0, &current_bConfiguration, 1, 5000) != 1)
+  {
+    current_bConfiguration = 0;   /* Failed. Assume not configured */
+  }
+    
   number = printer->device->config[printer->conf].bConfigurationValue;
-
-  if (usb_set_configuration(printer->handle, number) < 0)
+  if (number != current_bConfiguration)
   {
-   /*
-    * If the set fails, chances are that the printer only supports a
-    * single configuration.  Technically these printers don't conform to
-    * the USB printer specification, but otherwise they'll work...
-    */

-    if (errno != EBUSY)
-      fprintf(stderr, "DEBUG: Failed to set configuration %d for %04x:%04x\n",
-              number, printer->device->descriptor.idVendor,
-         printer->device->descriptor.idProduct);
+    if (usb_set_configuration(printer->handle, number) < 0)
+    {
+     /*
+      * If the set fails, chances are that the printer only supports a
+      * single configuration.  Technically these printers don't conform to
+      * the USB printer specification, but otherwise they'll work...
+      */
+
+      if (errno != EBUSY)
+        fprintf(stderr, "DEBUG: Failed to set configuration %d for %04x:%04x\n",
+                number, printer->device->descriptor.idVendor,
+           printer->device->descriptor.idProduct);
+    }
   }

  /*
@@ -700,20 +714,24 @@
 #endif /* 0 */

  /*
-  * Set alternate setting...
+  * Set alternate setting, but only if there is more than one option.
+  * Some printers (e.g., Samsung) don't like usb_set_altinterface.
   */
-
-  number = printer->device->config[printer->conf].interface[printer->iface].
-               altsetting[printer->altset].bAlternateSetting;
-  while (usb_set_altinterface(printer->handle, number) < 0)
+  if (printer->device->config[printer->conf].interface[printer->iface].
+          num_altsetting > 1)
   {
-    if (errno != EBUSY)
-      fprintf(stderr,
-              "DEBUG: Failed to set alternate interface %d for %04x:%04x: %s\n",
-              number, printer->device->descriptor.idVendor,
-         printer->device->descriptor.idProduct, strerror(errno));
+    number = printer->device->config[printer->conf].interface[printer->iface].
+                 altsetting[printer->altset].bAlternateSetting;
+    while (usb_set_altinterface(printer->handle, number) < 0)
+    {
+      if (errno != EBUSY)
+        fprintf(stderr,
+                "DEBUG: Failed to set alternate interface %d for %04x:%04x: %s\n",
+                number, printer->device->descriptor.idVendor,
+           printer->device->descriptor.idProduct, strerror(errno));

-    goto error;
+      goto error;
+    }
   }

   if (verbose)
Collaborator

michaelrsweet commented Jan 27, 2012 edited

"str3965+3978.patch":

Index: backend/usb-libusb.c
===================================================================
--- backend/usb-libusb.c    (revision 10195)
+++ backend/usb-libusb.c    (working copy)
@@ -3,7 +3,7 @@
  *
  *   Libusb interface code for CUPS.
  *
- *   Copyright 2007-2011 by Apple Inc.
+ *   Copyright 2007-2012 by Apple Inc.
  *
  *   These coded instructions, statements, and computer programs are the
  *   property of Apple Inc. and are protected by Federal copyright
@@ -423,22 +423,34 @@
   * bytes.  The 1284 spec says the length is stored MSB first...
   */

-  length = (((unsigned)buffer[0] & 255) << 8) +
+  length = (((unsigned)buffer[0] & 255) << 8) |
       ((unsigned)buffer[1] & 255);

  /*
-  * Check to see if the length is larger than our buffer; first
-  * assume that the vendor incorrectly implemented the 1284 spec,
-  * and then limit the length to the size of our buffer...
+  * Check to see if the length is larger than our buffer or less than 14 bytes
+  * (the minimum valid device ID is "MFG:x;MDL:y;" with 2 bytes for the length).
+  *
+  * If the length is out-of-range, assume that the vendor incorrectly
+  * implemented the 1284 spec and re-read the length as LSB first,..
   */

-  if (length > bufsize)
-    length = (((unsigned)buffer[1] & 255) << 8) +
+  if (length > bufsize || length < 14)
+    length = (((unsigned)buffer[1] & 255) << 8) |
         ((unsigned)buffer[0] & 255);

   if (length > bufsize)
     length = bufsize;

+  if (length < 14)
+  {
+   /*
+    * Invalid device ID, clear it!
+    */
+
+    *buffer = '\0';
+    return (-1);
+  }
+
   length -= 2;

  /*
@@ -631,6 +643,7 @@
             int           verbose) /* I - Update connecting-to-device state? */
 {
   int  number;             /* Configuration/interface/altset numbers */
+  char current;            /* Current configuration */


  /*
@@ -647,27 +660,37 @@
   if ((printer->handle = usb_open(printer->device)) == NULL)
     return (-1);

+  if (verbose)
+    fputs("STATE: +connecting-to-device\n", stderr);
+
  /*
-  * Then set the desired configuration...
+  * Set the desired configuration, but only if it needs changing. Some
+  * printers (e.g., Samsung) don't like usb_set_configuration. It will succeed,
+  * but the following print job is sometimes silently lost by the printer.
   */

-  if (verbose)
-    fputs("STATE: +connecting-to-device\n", stderr);
+  if (usb_control_msg(printer->handle,
+                      USB_TYPE_STANDARD | USB_ENDPOINT_IN | USB_RECIP_DEVICE,
+                      8, /* GET_CONFIGURATION */
+                      0, 0, &current, 1, 5000) != 1)
+    current = 0;           /* Assume not configured */

   number = printer->device->config[printer->conf].bConfigurationValue;
-
-  if (usb_set_configuration(printer->handle, number) < 0)
+  if (number != current)
   {
-   /*
-    * If the set fails, chances are that the printer only supports a
-    * single configuration.  Technically these printers don't conform to
-    * the USB printer specification, but otherwise they'll work...
-    */
+    if (usb_set_configuration(printer->handle, number) < 0)
+    {
+     /*
+      * If the set fails, chances are that the printer only supports a
+      * single configuration.  Technically these printers don't conform to
+      * the USB printer specification, but otherwise they'll work...
+      */

-    if (errno != EBUSY)
-      fprintf(stderr, "DEBUG: Failed to set configuration %d for %04x:%04x\n",
-              number, printer->device->descriptor.idVendor,
-         printer->device->descriptor.idProduct);
+      if (errno != EBUSY)
+        fprintf(stderr, "DEBUG: Failed to set configuration %d for %04x:%04x\n",
+                number, printer->device->descriptor.idVendor,
+           printer->device->descriptor.idProduct);
+    }
   }

  /*
@@ -679,41 +702,35 @@
   while (usb_claim_interface(printer->handle, number) < 0)
   {
     if (errno != EBUSY)
-      fprintf(stderr, "DEBUG: Failed to claim interface %d for %04x:%04x: %s\n",
+      fprintf(stderr,
+              "DEBUG: Failed to claim interface %d for %04x:%04x: %s\n",
               number, printer->device->descriptor.idVendor,
          printer->device->descriptor.idProduct, strerror(errno));

     goto error;
   }

-#if 0 /* STR #3801: Claiming interface 0 causes problems with some printers */
-  if (number != 0)
-    while (usb_claim_interface(printer->handle, 0) < 0)
+ /*
+  * Set alternate setting, but only if there is more than one option.  Some
+  * printers (e.g., Samsung) don't like usb_set_altinterface.
+  */
+
+  if (printer->device->config[printer->conf].interface[printer->iface].
+          num_altsetting > 1)
+  {
+    number = printer->device->config[printer->conf].interface[printer->iface].
+                 altsetting[printer->altset].bAlternateSetting;
+
+    while (usb_set_altinterface(printer->handle, number) < 0)
     {
       if (errno != EBUSY)
-   fprintf(stderr, "DEBUG: Failed to claim interface 0 for %04x:%04x: %s\n",
-       printer->device->descriptor.idVendor,
-       printer->device->descriptor.idProduct, strerror(errno));
+        fprintf(stderr,
+                "DEBUG: Failed to set alternate interface %d for %04x:%04x: "
+                "%s\n", number, printer->device->descriptor.idVendor,
+           printer->device->descriptor.idProduct, strerror(errno));

       goto error;
     }
-#endif /* 0 */
-
- /*
-  * Set alternate setting...
-  */
-
-  number = printer->device->config[printer->conf].interface[printer->iface].
-               altsetting[printer->altset].bAlternateSetting;
-  while (usb_set_altinterface(printer->handle, number) < 0)
-  {
-    if (errno != EBUSY)
-      fprintf(stderr,
-              "DEBUG: Failed to set alternate interface %d for %04x:%04x: %s\n",
-              number, printer->device->descriptor.idVendor,
-         printer->device->descriptor.idProduct, strerror(errno));
-
-    goto error;
   }

   if (verbose)

@michaelrsweet michaelrsweet added this to the Stable milestone Mar 17, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment