Usb backend refactor step 2: Separate Usb Device Handle and Usb Device (deprecate)

chrome/browser/usb/usb_device.h

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #ifndef CHROME_BROWSER_USB_USB_DEVICE_H_
 #define CHROME_BROWSER_USB_USB_DEVICE_H_
 
 #include <map>
 #include <vector>
 
 #include "base/memory/ref_counted.h"
 #include "base/synchronization/lock.h"
 #include "chrome/browser/usb/usb_interface.h"
 #include "net/base/completion_callback.h"
 #include "net/base/io_buffer.h"
+#include "third_party/libusb/src/libusb/libusb.h"
 
 struct libusb_device;
 struct libusb_device_handle;
 struct libusb_iso_packet_descriptor;
 struct libusb_transfer;
 
 typedef libusb_device* PlatformUsbDevice;

[pfeldman, 2013/07/21 08:48:23]

All these typedefs were here to encapsulate dependency from libusb.h. You are
now including it in public header, so call sites are effectively getting this
dependency. Is that intentional? Would you like to hide libusb behind this API
long term or do you allow clients to use both APIs?

[Bei Zhang, 2013/07/21 21:31:52]

I noticed earlier that this will make your code hard to compile due to the
conflict of winsock.h and winsock2.h. If you are using sockets, you cannot
include libusb.h any more in in 1.0.16. The introduction of libusb.h is for the
LIBUSB_CALL below. I will use API_CALL in base/port.h instead.

On 2013/07/21 08:48:23, pfeldman wrote:

 typedef libusb_device_handle* PlatformUsbDeviceHandle;
 typedef libusb_iso_packet_descriptor* PlatformUsbIsoPacketDescriptor;
 typedef libusb_transfer* PlatformUsbTransferHandle;
 
 class UsbService;
 
 namespace net {
 class IOBuffer;
 }  // namespace net
 
 enum UsbTransferStatus {
   USB_TRANSFER_COMPLETED = 0,
   USB_TRANSFER_ERROR,
   USB_TRANSFER_TIMEOUT,
   USB_TRANSFER_CANCELLED,
   USB_TRANSFER_STALLED,
   USB_TRANSFER_DISCONNECT,
   USB_TRANSFER_OVERFLOW,
   USB_TRANSFER_LENGTH_SHORT,
 };
 
-typedef base::Callback<void(UsbTransferStatus, scoped_refptr<net::IOBuffer>,
+typedef base::Callback<void(
+    UsbTransferStatus,
+    scoped_refptr<net::IOBuffer>,
     size_t)> UsbTransferCallback;
-typedef base::Callback<void(bool)> UsbInterfaceCallback;
+typedef base::Callback<void(bool success)> UsbInterfaceCallback;
+typedef base::Callback<void(bool success)> UsbResetDeviceCallback;
 
 // A UsbDevice wraps the platform's underlying representation of what a USB
 // device actually is, and provides accessors for performing many of the
 // standard USB operations.
-class UsbDevice : public base::RefCounted<UsbDevice> {
+class UsbDevice : public base::RefCountedThreadSafe<UsbDevice> {
  public:
   enum TransferRequestType { STANDARD, CLASS, VENDOR, RESERVED };
   enum TransferRecipient { DEVICE, INTERFACE, ENDPOINT, OTHER };
 
-  // Usually you will not want to directly create a UsbDevice, favoring to let
-  // the UsbService take care of the logistics of getting a platform device
-  // handle and handling events for it.
-  UsbDevice(UsbService* service, PlatformUsbDeviceHandle handle);
-
-  PlatformUsbDeviceHandle handle() { return handle_; }
+  int device() const { return device_; }
+  PlatformUsbDeviceHandle handle() const { return handle_; }
 
   // Close the USB device and release the underlying platform device. |callback|
   // is invoked after the device has been closed.
   virtual void Close(const base::Callback<void()>& callback);
 
+  // FILE thread:

[pfeldman, 2013/07/21 08:48:23]

This requires a more comprehensive documentation. I.e. which API is used on
which thread any why.

If there is a need for half of this class to function on one thread and another
half on the other, it suggests that there are two different entities operating
on different threads, potentially having different lifetimes. Like I'd expect
ClaimInterface to return UsbTransfer* (raw pointer with ownership, re-using
net::Socket model) that would have Control, Bulk, etc. transfer methods. Client
needs to claim interface prior to using endpoints and such separation would make
both: workflow and threading perspectives clear.

To be more consistent with chrome threading model,
- UsbService and UsbDevice would operate on UI thread
- UsbTransfer would operate on single IO thread.

or

- UsbService would operate on UI thread
- UsbDevice and UsbTransfer would operate on single IO thread

[Bei Zhang, 2013/07/21 21:31:52]

The reason I allow those Async IO to be called on any thread is that it has way
more arguments than we can support in base::Callback. Basically both UsbDevice
and UsbService are not thread safe and can only be used on the FILE thread.

Introducing a UsbTransfer is a good idea and might be essential for exposing an
API to cancel a current async transfer. The down side is that it will make the
life cycle management of usb transfers even harder.

On 2013/07/21 08:48:23, pfeldman wrote:

   virtual void ListInterfaces(UsbConfigDescriptor* config,
                               const UsbInterfaceCallback& callback);
-
   virtual void ClaimInterface(const int interface_number,
                               const UsbInterfaceCallback& callback);
-
   virtual void ReleaseInterface(const int interface_number,
                                 const UsbInterfaceCallback& callback);
-
   virtual void SetInterfaceAlternateSetting(
       const int interface_number,
       const int alternate_setting,
       const UsbInterfaceCallback& callback);
 
+  // Async IO. Can be called on any thread.
   virtual void ControlTransfer(const UsbEndpointDirection direction,
                                const TransferRequestType request_type,
                                const TransferRecipient recipient,
                                const uint8 request,
                                const uint16 value,
                                const uint16 index,
                                net::IOBuffer* buffer,
                                const size_t length,
                                const unsigned int timeout,
                                const UsbTransferCallback& callback);
-
   virtual void BulkTransfer(const UsbEndpointDirection direction,
                             const uint8 endpoint,
                             net::IOBuffer* buffer,
                             const size_t length,
                             const unsigned int timeout,
                             const UsbTransferCallback& callback);
-
   virtual void InterruptTransfer(const UsbEndpointDirection direction,
                                  const uint8 endpoint,
                                  net::IOBuffer* buffer,
                                  const size_t length,
                                  const unsigned int timeout,
                                  const UsbTransferCallback& callback);
-
   virtual void IsochronousTransfer(const UsbEndpointDirection direction,
                                    const uint8 endpoint,
                                    net::IOBuffer* buffer,
                                    const size_t length,
                                    const unsigned int packets,
                                    const unsigned int packet_length,
                                    const unsigned int timeout,
                                    const UsbTransferCallback& callback);
 
-  virtual void ResetDevice(const base::Callback<void(bool)>& callback);
-
-  // Normal code should not call this function. It is called by the platform's
-  // callback mechanism in such a way that it cannot be made private. Invokes
-  // the callbacks associated with a given transfer, and removes it from the
-  // in-flight transfer set.
-  void TransferComplete(PlatformUsbTransferHandle transfer);
+  // FILE thread:
+  virtual void ResetDevice(const UsbResetDeviceCallback& callback);
 
  protected:
   // This constructor variant is for use in testing only.
   UsbDevice();
 
-  friend class base::RefCounted<UsbDevice>;
+  friend class base::RefCountedThreadSafe<UsbDevice>;
   virtual ~UsbDevice();
 
  private:
-  struct Transfer {
-    Transfer();
-    ~Transfer();
+  // UsbDeviceHandle should only be created from UsbDevice class.
+  UsbDevice(UsbService* service, int device, PlatformUsbDeviceHandle handle);
 
-    UsbTransferType transfer_type;
-    scoped_refptr<net::IOBuffer> buffer;
-    size_t length;
-    UsbTransferCallback callback;
-  };
+  friend class UsbDeviceStub;
 
-  // Checks that the device has not yet been closed.
-  void CheckDevice();
+  static void HandleTransferCompletionFileThread(
+      PlatformUsbTransferHandle transfer);
+
+  static void LIBUSB_CALL HandleTransferCompletion(
+      PlatformUsbTransferHandle transfer);
+
+  void TransferComplete(PlatformUsbTransferHandle transfer);
+
+  void InternalClose();
 
   // Submits a transfer and starts tracking it. Retains the buffer and copies
   // the completion callback until the transfer finishes, whereupon it invokes
   // the callback then releases the buffer.
   void SubmitTransfer(PlatformUsbTransferHandle handle,
                       UsbTransferType transfer_type,
                       net::IOBuffer* buffer,
                       const size_t length,
                       const UsbTransferCallback& callback);
 
   // The UsbService isn't referenced here to prevent a dependency cycle between
   // the service and the devices. Since a service owns every device, and is
   // responsible for its destruction, there is no case where a UsbDevice can
   // have outlived its originating UsbService.
   UsbService* const service_;
+  const int device_;
   PlatformUsbDeviceHandle handle_;
+  base::ThreadChecker thread_checker_;
 
   // transfers_ tracks all in-flight transfers associated with this device,
   // allowing the device to retain the buffer and callback associated with a
-  // transfer until such time that it completes. It is protected by lock_.
-  base::Lock lock_;
-  std::map<PlatformUsbTransferHandle, Transfer> transfers_;
+  // transfer until such time that it completes.
+  struct TransferInfo;
+  std::map<PlatformUsbTransferHandle, TransferInfo> transfers_;
 
   DISALLOW_COPY_AND_ASSIGN(UsbDevice);
 };
 
 #endif  // CHROME_BROWSER_USB_USB_DEVICE_H_