chrome.hid: enrich model with report IDs

device/hid/hid_connection_win.cc

 // Copyright (c) 2014 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.
 
 #include "device/hid/hid_connection_win.h"
 
 #include <cstring>
 
 #include "base/files/file.h"
 #include "base/message_loop/message_loop.h"
-#include "base/stl_util.h"
-#include "base/threading/thread_restrictions.h"
 #include "base/win/object_watcher.h"
-#include "base/win/scoped_handle.h"
-#include "device/hid/hid_service.h"
-#include "device/hid/hid_service_win.h"
 
 #define INITGUID
 
 #include <windows.h>
 #include <hidclass.h>
 
 extern "C" {
 #include <hidsdi.h>
 }
 
@@ skipping 69 matching lines @@
   Release();
 }
 
 void PendingHidTransfer::WillDestroyCurrentMessageLoop() {
   watcher_.StopWatching();
   connection_->OnTransferCanceled(this);
 }
 
 HidConnectionWin::HidConnectionWin(const HidDeviceInfo& device_info)
     : HidConnection(device_info) {
-  DCHECK(thread_checker_.CalledOnValidThread());
   file_.Set(CreateFileA(device_info.device_id.c_str(),
                         GENERIC_WRITE | GENERIC_READ,
                         FILE_SHARE_READ | FILE_SHARE_WRITE,
                         NULL,
                         OPEN_EXISTING,
                         FILE_FLAG_OVERLAPPED,
                         NULL));
 
   if (!file_.IsValid() &&
       GetLastError() == base::File::FILE_ERROR_ACCESS_DENIED) {
     file_.Set(CreateFileA(device_info.device_id.c_str(),
                           GENERIC_READ,
                           FILE_SHARE_READ,
                           NULL,
                           OPEN_EXISTING,
                           FILE_FLAG_OVERLAPPED,
                           NULL));
   }
 }
 
-bool HidConnectionWin::available() const {
-  return file_.IsValid();
-}
-
 HidConnectionWin::~HidConnectionWin() {
-  DCHECK(thread_checker_.CalledOnValidThread());
   CancelIo(file_.Get());
 }
 
-void HidConnectionWin::Read(scoped_refptr<net::IOBufferWithSize> buffer,
-                            const HidConnection::IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().input_report_size == 0) {
-    // The device does not support input reports.
-    callback.Run(false, 0);
-    return;
-  }
+void HidConnectionWin::PlatformRead(scoped_refptr<net::IOBufferWithSize> buffer,
+                                    const HidConnection::IOCallback& callback) {
+  scoped_refptr<net::IOBufferWithSize> receive_buffer =
+      new net::IOBufferWithSize(device_info().max_input_report_size);
 
-  // This fairly awkward logic is correct: If Windows does not expect a device
-  // to supply a report ID in its input reports, it requires the buffer to be
-  // 1 byte larger than what the device actually sends.
-  int receive_buffer_size = device_info().input_report_size;
-  int expected_buffer_size = receive_buffer_size;
-  if (!device_info().has_report_id)
-    expected_buffer_size -= 1;
-
-  if (buffer->size() < expected_buffer_size) {
-    callback.Run(false, 0);
-    return;
-  }
-
-  scoped_refptr<net::IOBufferWithSize> receive_buffer(buffer);
-  if (receive_buffer_size != expected_buffer_size)
-    receive_buffer = new net::IOBufferWithSize(receive_buffer_size);
   scoped_refptr<PendingHidTransfer> transfer(
       new PendingHidTransfer(this, buffer, receive_buffer, callback));
   transfers_.insert(transfer);
   transfer->TakeResultFromWindowsAPI(
       ReadFile(file_.Get(),
                receive_buffer->data(),
                static_cast<DWORD>(receive_buffer->size()),
                NULL,
                transfer->GetOverlapped()));
 }
 
-void HidConnectionWin::Write(uint8_t report_id,
-                             scoped_refptr<net::IOBufferWithSize> buffer,
-                             const HidConnection::IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().output_report_size == 0) {
-    // The device does not support output reports.
-    callback.Run(false, 0);
-    return;
-  }
-
+void HidConnectionWin::PlatformWrite(
+    uint8_t report_id,
+    scoped_refptr<net::IOBufferWithSize> buffer,
+    const HidConnection::IOCallback& callback) {
   // The Windows API always wants either a report ID (if supported) or
   // zero at the front of every output report.
   scoped_refptr<net::IOBufferWithSize> output_buffer(buffer);
   output_buffer = new net::IOBufferWithSize(buffer->size() + 1);
   output_buffer->data()[0] = report_id;
   memcpy(output_buffer->data() + 1, buffer->data(), buffer->size());
 
   scoped_refptr<PendingHidTransfer> transfer(
       new PendingHidTransfer(this, output_buffer, NULL, callback));
   transfers_.insert(transfer);
   transfer->TakeResultFromWindowsAPI(
       WriteFile(file_.Get(),
                 output_buffer->data(),
                 static_cast<DWORD>(output_buffer->size()),
                 NULL,
                 transfer->GetOverlapped()));
 }
 
-void HidConnectionWin::GetFeatureReport(
+void HidConnectionWin::PlatformGetFeatureReport(
     uint8_t report_id,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().feature_report_size == 0) {
-    // The device does not support feature reports.
-    callback.Run(false, 0);
-    return;
-  }
-
-  int receive_buffer_size = device_info().feature_report_size;
-  int expected_buffer_size = receive_buffer_size;
-  if (!device_info().has_report_id)
-    expected_buffer_size -= 1;
-  if (buffer->size() < expected_buffer_size) {
-    callback.Run(false, 0);
-    return;
-  }
-
-  scoped_refptr<net::IOBufferWithSize> receive_buffer(buffer);
-  if (receive_buffer_size != expected_buffer_size)
-    receive_buffer = new net::IOBufferWithSize(receive_buffer_size);
-
+  scoped_refptr<net::IOBufferWithSize> receive_buffer =
+      new net::IOBufferWithSize(device_info().max_feature_report_size);
   // The first byte of the destination buffer is the report ID being requested.
   receive_buffer->data()[0] = report_id;
+
   scoped_refptr<PendingHidTransfer> transfer(
       new PendingHidTransfer(this, buffer, receive_buffer, callback));
   transfers_.insert(transfer);
   transfer->TakeResultFromWindowsAPI(
       DeviceIoControl(file_.Get(),
                       IOCTL_HID_GET_FEATURE,
                       NULL,
                       0,
                       receive_buffer->data(),
                       static_cast<DWORD>(receive_buffer->size()),
                       NULL,
                       transfer->GetOverlapped()));
 }
 
-void HidConnectionWin::SendFeatureReport(
+void HidConnectionWin::PlatformSendFeatureReport(
     uint8_t report_id,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().feature_report_size == 0) {
-    // The device does not support feature reports.
-    callback.Run(false, 0);
-    return;
-  }
-
   // The Windows API always wants either a report ID (if supported) or
   // zero at the front of every output report.
   scoped_refptr<net::IOBufferWithSize> output_buffer(buffer);
   output_buffer = new net::IOBufferWithSize(buffer->size() + 1);
   output_buffer->data()[0] = report_id;
   memcpy(output_buffer->data() + 1, buffer->data(), buffer->size());
 
   scoped_refptr<PendingHidTransfer> transfer(
       new PendingHidTransfer(this, output_buffer, NULL, callback));
   transfer->TakeResultFromWindowsAPI(
       DeviceIoControl(file_.Get(),
                       IOCTL_HID_SET_FEATURE,
                       output_buffer->data(),
                       static_cast<DWORD>(output_buffer->size()),
                       NULL,
                       0,
                       NULL,
                       transfer->GetOverlapped()));
 }
 
 void HidConnectionWin::OnTransferFinished(
     scoped_refptr<PendingHidTransfer> transfer) {
+  transfers_.erase(transfer);
+
   DWORD bytes_transferred;
-  transfers_.erase(transfer);
   if (GetOverlappedResult(
           file_, transfer->GetOverlapped(), &bytes_transferred, FALSE)) {
-    if (bytes_transferred == 0)
+    if (bytes_transferred == 0) {
       transfer->callback_.Run(true, 0);
-    // If this is an input transfer and the receive buffer is not the same as
-    // the target buffer, we need to copy the receive buffer into the target
-    // buffer, discarding the first byte. This is because the target buffer's
-    // owner is not expecting a report ID but Windows will always provide one.
-    if (transfer->receive_buffer_ &&
-        transfer->receive_buffer_ != transfer->target_buffer_) {
-      // Move one byte forward.
-      --bytes_transferred;
-      memcpy(transfer->target_buffer_->data(),
-             transfer->receive_buffer_->data() + 1,
-             bytes_transferred);
+      return;
     }
-    transfer->callback_.Run(true, bytes_transferred);
+
+    if (transfer->receive_buffer_) {
+      // If owner HID top-level collection does not have report ID, we need to
+      // copy the receive buffer into the target buffer, discarding the first
+      // byte. This is because the target buffer's owner is not expecting a
+      // report ID but Windows will always provide one.
+      if (!has_report_id()) {
+        // Assert first byte is 0x00
+        DCHECK(transfer->receive_buffer_->data()[0] ==

[Ken Rockot(use gerrit already), 2014/07/01 14:55:23]

An assert here is pretty aggressive since we can't control what real hardware
might do. How about just testing and if there's an unexpected value, VLOG(1) it:

VLOG(1) << "Unexpected report ID in HID report."

and feel just discard the report (without completing a read).

[jracle (use Gerrit), 2014/07/01 15:32:38]

Ok, Should I pass report ID as an argument?

VLOG(..) << "Unexpected report ID in HID report." << report_id

what is the correct syntax plz?

On 2014/07/01 14:55:23, Ken Rockot wrote:

[Ken Rockot(use gerrit already), 2014/07/01 15:37:14]

Doesn't much matter IMO. But yes if you want to include the ID in the log
message, that's the right syntax. Would use different punctuation though, like:

VLOG(1) << "Unexpected report ID in HID report: " << report_id;

[jracle (use Gerrit), 2014/07/01 15:42:56]

OK thanks

On 2014/07/01 15:37:14, Ken Rockot wrote:

+               HidConnection::kNullReportId);
+
+        // Move one byte forward.
+        --bytes_transferred;
+        memcpy(transfer->target_buffer_->data(),
+               transfer->receive_buffer_->data() + 1,
+               bytes_transferred);
+      } else {
+        memcpy(transfer->target_buffer_->data(),
+               transfer->receive_buffer_->data(),
+               bytes_transferred);
+      }
+    }
+
+    CompleteRead(transfer->target_buffer_, transfer->callback_);

[Ken Rockot(use gerrit already), 2014/07/01 14:55:23]

Boo. So the way this is structured (not having separate receive and callback
queues like Linux and Mac have) makes it really awkward to behave like I'd hope.
You can't really push the transfer request back into the front of the queue,
because any previously issued ReadFile calls will still execute in-order, and
each ReadFile is tied directly to a callback.

This is where it would be nice to have what you were working on before, which is
to unify the queuing across platform implementations.

However I still think it's too much to cover for this CL, so let's accept that
API consumers on Windows will have to deal with the occasional empty input
report until this is fixed, as long as that can happen before 38 branches.

[jracle (use Gerrit), 2014/07/01 15:32:38]

Indeed..

Do I have to handle CompleteRead return result? No I guess in that case,
correct?

On 2014/07/01 14:55:23, Ken Rockot wrote:

[Ken Rockot(use gerrit already), 2014/07/01 15:37:14]

Nope, not much you can do with it yet.

[jracle (use Gerrit), 2014/07/01 15:42:56]

got it.

On 2014/07/01 15:37:14, Ken Rockot wrote:

   } else {
     transfer->callback_.Run(false, 0);
   }
 }
 
 void HidConnectionWin::OnTransferCanceled(
     scoped_refptr<PendingHidTransfer> transfer) {
   transfers_.erase(transfer);
   transfer->callback_.Run(false, 0);
 }
 
 }  // namespace device