Clean up HID backend and API.

device/hid/hid_connection_mac.cc

Index: device/hid/hid_connection_mac.cc
diff --git a/device/hid/hid_connection_mac.cc b/device/hid/hid_connection_mac.cc
index bce711336798cfa033d2430aea7ed382cd3a285d..b47cfadb7cfaaa8f8beda5ce6e474b796996f869 100644
--- a/device/hid/hid_connection_mac.cc
+++ b/device/hid/hid_connection_mac.cc
@@ -5,26 +5,26 @@
 #include "device/hid/hid_connection_mac.h"
 
 #include "base/bind.h"
-#include "base/callback.h"
 #include "base/mac/foundation_util.h"
 #include "base/threading/thread_restrictions.h"
-#include "base/tuple.h"
-#include "device/hid/hid_service.h"
+#include "device/hid/hid_connection_mac.h"
 #include "device/hid/hid_service_mac.h"
 #include "net/base/io_buffer.h"
 
-#include <CoreFoundation/CoreFoundation.h>
-#include <IOKit/hid/IOHIDManager.h>
-
 namespace device {
 
+PendingHidReport::PendingHidReport() : size(0) {}
+
+PendingHidReport::~PendingHidReport() {}
+
+PendingHidRead::PendingHidRead() : size(0) {}
+
+PendingHidRead::~PendingHidRead() {}
+
 HidConnectionMac::HidConnectionMac(HidServiceMac* service,
                                    HidDeviceInfo device_info,
                                    IOHIDDeviceRef device)
-    : HidConnection(device_info),
-      service_(service),
-      device_(device),
-      disconnected_(false) {
+    : HidConnection(device_info), service_(service), device_(device) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   message_loop_ = base::MessageLoopProxy::current();
@@ -39,32 +39,35 @@ HidConnectionMac::HidConnectionMac(HidServiceMac* service,
       this);
   IOHIDDeviceOpen(device_, kIOHIDOptionsTypeNone);
 }
+
 HidConnectionMac::~HidConnectionMac() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
-  while (read_queue_.size()) {
-    read_queue_.front().c.Run(false, 0);
-    read_queue_.pop();
+  while (!pending_reads_.empty()) {
+    pending_reads_.front().callback.Run(false, 0);
+    pending_reads_.pop();
   }
 
   IOHIDDeviceClose(device_, kIOHIDOptionsTypeNone);
 }
 
-void HidConnectionMac::InputReportCallback(void * context,
+void HidConnectionMac::InputReportCallback(void* context,
                                            IOReturn result,
-                                           void * sender,
+                                           void* sender,
                                            IOHIDReportType type,
-                                           uint32_t reportID,
-                                           uint8_t * report,
+                                           uint32_t report_id,
+                                           uint8_t* report_bytes,
                                            CFIndex reportLength) {
-  HidConnectionMac* connection = reinterpret_cast<HidConnectionMac*>(context);
-  size_t length = reportLength + (reportID != 0);
+  HidConnectionMac* connection = static_cast<HidConnectionMac*>(context);
+
+  // A non-zero report ID requires an extra byte.

[Mark Mentovai, 2014/02/19 22:48:15]

What’s special about 0 that you don’t need to put it into the buffer? Isn’t
there an ambiguity problem—whatever ultimately consumes the data won’t know
whether the first byte is the report_id or report_bytes.

[Ken Rockot(use gerrit already), 2014/02/21 02:15:36]

When working with a specific raw device, the developer must already know whether
or not the device supports Report IDs. If it does, the first byte of every input
report is a non-zero Report ID. If it doesn't, the first byte of every input
report is something else (but it is never a zero Report ID).

On other OSes, this is a non-issue; the contents of an input report are passed
along into user space as-is for consumers to interpret. In the case of OS X,
what happens is this:

1. If a device doesn't support Report IDs, report_id is 0 here and the input
report contents (report_bytes) are verbatim what the device sent.

2. If a device DOES support Report IDs, the first byte of the input report is
graciously stripped off by the system and passed in through the report_id
argument.

Injecting the Report ID (if present) back into the buffer provides behavior that
is consistent with other platforms and hopefully consistent with the
expectations of any developer working with raw HID devices.

+  size_t length = reportLength + (report_id != 0 ? 1 : 0);
   scoped_refptr<net::IOBuffer> buffer(new net::IOBuffer(length));
-  if (reportID) {
-    buffer->data()[0] = reportID;
-    memcpy(buffer->data() + 1, report, reportLength);
+  if (report_id) {
+    buffer->data()[0] = report_id;

[Mark Mentovai, 2014/02/19 22:48:15]

report_id is uint32_t. This will truncate it. Is data loss acceptable?

If we’re not using numbered reports at all, maybe you should get rid of all of
the non-0 stuff and simply DCHECK that the report_id is 0.

[Ken Rockot(use gerrit already), 2014/02/21 02:15:36]

The protocol only uses a single byte for a Report ID, so this is just an oddity
of IOKit's API. Truncation is fine.

+    memcpy(buffer->data() + 1, report_bytes, reportLength);
   } else {
-    memcpy(buffer->data(), report, reportLength);
+    memcpy(buffer->data(), report_bytes, reportLength);
   }
   connection->message_loop_->PostTask(
       FROM_HERE,
@@ -77,28 +80,28 @@ void HidConnectionMac::InputReportCallback(void * context,
 
 void HidConnectionMac::ProcessReadQueue() {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  while(read_queue_.size() && input_reports_.size()) {
-    PendingRead read = read_queue_.front();
-    read_queue_.pop();
-    PendingReport report = input_reports_.front();
-
-    if (read.b < report.second) {
-      read.c.Run(false, report.second);
+  while (pending_reads_.size() && pending_reports_.size()) {
+    PendingHidRead read = pending_reads_.front();
+    pending_reads_.pop();
+    PendingHidReport report = pending_reports_.front();
+    if (read.size < report.size) {
+      read.callback.Run(false, report.size);
     } else {
-      memcpy(read.a->data(), report.first->data(), report.second);
-      input_reports_.pop();
-      read.c.Run(true, report.second);
+      memcpy(read.buffer->data(), report.buffer->data(), report.size);
+      pending_reports_.pop();
+      read.callback.Run(true, report.size);
     }
   }
 }
 
 void HidConnectionMac::ProcessInputReport(IOHIDReportType type,
-                                          scoped_refptr<net::IOBuffer> report,
+                                          scoped_refptr<net::IOBuffer> buffer,
                                           CFIndex reportLength) {
   DCHECK(thread_checker_.CalledOnValidThread());
-
-  input_reports_.push(std::make_pair(report, reportLength));
+  PendingHidReport report;
+  report.buffer = buffer;
+  report.size = reportLength;
+  pending_reports_.push(report);
   ProcessReadQueue();
 }
 
@@ -107,24 +110,21 @@ void HidConnectionMac::WriteReport(IOHIDReportType type,
                                    size_t size,
                                    const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (disconnected_ || !device_) {
+  if (!device_) {
     callback.Run(false, 0);
     return;
   }
   const unsigned char* data_to_send =
       reinterpret_cast<const unsigned char*>(buffer->data());
   size_t length_to_send = size;
-  if (data_to_send[0] == 0x0) {
-      /* Not using numbered Reports.
-       Don't send the report number. */
+  uint32_t report_id = static_cast<uint32_t>(buffer->data()[0]);

[Mark Mentovai, 2014/02/19 22:48:15]

Similar questions about interpretation ambiguity and data type width apply to
the write case.

What informed this design? Other platforms? Something else? This logic smells
fishy.

[Ken Rockot(use gerrit already), 2014/02/21 02:15:36]

There does not appear to be a great way of determining whether or not a device
supports Report IDs, which is unfortunate given that IOKit always requires
report ID to be given separately from buffer contents (which ultimately must
result in the driver injecting that ID as the first byte in the buffer anyway).

The working solution we went with for this problem was to require users of
HidConnection to always include a Report ID in their output reports, using zero
when sending to a device which does not use Report IDs.

This is what the Windows HID API expects as well - it always expects the first
byte of an output report to be a report ID, and if that ID is zero, it gets
stripped off before the report data is sent to the device.

I considered mimicking IOKit's API instead of Windows' - that is, make Write
take an explicit Report ID argument (which will be injected if non-zero), and
make Read's callback get an explicit Report ID argument which is automatically
stripped from an input report. Unfortunately this introduces an ambiguity that
can't be resolved by the Mac impl: we don't know if we should pluck a Report ID
off a report coming from the device.

So we instead leave it up to the developer to properly interpret the raw data
from the device, and require the developer to unambiguously prefix all output
reports with a Report-ID-or-zero byte.

+  if (report_id == 0) {
+    // If the report ID is 0, don't actually send a leading report number.
       ++data_to_send;
       --length_to_send;
   }
-  IOReturn res = IOHIDDeviceSetReport(device_.get(),
-                                      type,
-                                      buffer->data()[0], /* Report ID*/
-                                      data_to_send,
-                                      length_to_send);
+  IOReturn res = IOHIDDeviceSetReport(
+      device_.get(), type, report_id, data_to_send, length_to_send);
   if (res != kIOReturnSuccess) {
     callback.Run(false, 0);
   } else {
@@ -136,11 +136,15 @@ void HidConnectionMac::Read(scoped_refptr<net::IOBuffer> buffer,
                             size_t size,
                             const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (disconnected_ || !device_) {
+  if (!device_) {
     callback.Run(false, 0);
     return;
   }
-  read_queue_.push(MakeTuple(buffer, size, callback));
+  PendingHidRead read;
+  read.buffer = buffer;
+  read.size = size;
+  read.callback = callback;
+  pending_reads_.push(read);
   ProcessReadQueue();
 }
 
@@ -162,7 +166,7 @@ void HidConnectionMac::GetFeatureReport(scoped_refptr<net::IOBuffer> buffer,
                                         size_t size,
                                         const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (disconnected_ || !device_ || device_info_.feature_report_size == 0) {
+  if (!device_ || device_info_.feature_report_size == 0) {
     callback.Run(false, 0);
     return;
   }
@@ -173,14 +177,15 @@ void HidConnectionMac::GetFeatureReport(scoped_refptr<net::IOBuffer> buffer,
     return;
   }
 
-  CFIndex len = device_info_.feature_report_size;
-  IOReturn res = IOHIDDeviceGetReport(device_,
-                                      kIOHIDReportTypeFeature,
-                                      0,
-                                      (uint8_t*) buffer->data(),
-                                      &len);
-  if (res == kIOReturnSuccess)
-    callback.Run(true, len);
+  uint8_t* feature_report_buffer = reinterpret_cast<uint8_t*>(buffer->data());
+  CFIndex feature_report_size = device_info_.feature_report_size;
+  IOReturn result = IOHIDDeviceGetReport(device_,
+                                         kIOHIDReportTypeFeature,
+                                         0,
+                                         feature_report_buffer,
+                                         &feature_report_size);
+  if (result == kIOReturnSuccess)
+    callback.Run(true, feature_report_size);
   else
     callback.Run(false, 0);
 }