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>
 }
 
 #include <setupapi.h>
 #include <winioctl.h>
 
+#include <algorithm>
+
 namespace device {
 
+namespace {
+
+struct CollectionHasReportId {
+  bool operator()(const HidCollectionInfo& info) const {
+    return info.report_ids.size() > 0;
+  }
+};
+
+bool DeviceHasReportId(const HidDeviceInfo& device_info) {

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

Very nice.

[jracle (use Gerrit), 2014/06/07 12:56:21]

Thx. Wish I could use lambdas and C++11 though.
Sorry for my baby-knowledge of C++03, I'm so used to lambdas in other languages
that I have hard time reminding me of good std:c++ patterns!!

On 2014/06/06 20:04:43, Ken Rockot wrote:

+  return std::find_if(device_info.collections.begin(),
+                      device_info.collections.end(),
+                      CollectionHasReportId()) != device_info.collections.end();
+}
+
+}  // namespace
+
 struct PendingHidTransfer : public base::RefCounted<PendingHidTransfer>,
                             public base::win::ObjectWatcher::Delegate,
                             public base::MessageLoop::DestructionObserver {
   PendingHidTransfer(scoped_refptr<HidConnectionWin> connection,
                      scoped_refptr<net::IOBufferWithSize> target_buffer,
                      scoped_refptr<net::IOBufferWithSize> receive_buffer,
                      HidConnection::IOCallback callback);
 
   void TakeResultFromWindowsAPI(BOOL result);
 
@@ skipping 87 matching lines @@
 }
 
 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) {
+  if (device_info().max_input_report_size == 0) {
     // The device does not support input reports.
     callback.Run(false, 0);
     return;
   }
 
   // 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 receive_buffer_size = device_info().max_input_report_size;
   int expected_buffer_size = receive_buffer_size;
-  if (!device_info().has_report_id)
+  if (!DeviceHasReportId(device_info()))
     expected_buffer_size -= 1;

[Ken Rockot(use gerrit already), 2014/06/06 20:04:43]

It's this adjust-by-1 logic which exists throughout the Windows code that I have
big concerns about.

It would be really awesome if you could do some validation of this (testing with
a device which uses report IDs and with one that doesn't).

But if you'd rather defer on that, as long as the logic in this change matches
the logic which existed before, I'm OK with leaving it as-is.

[jracle (use Gerrit), 2014/06/07 12:56:21]

I totally follow you on that point, and will validate as you propose. I'm very
uncomfortable with that black-magic (though this would mean removing my great
DeviceHasReportId matcher ;).

Not kidding, I'll try to fix it in that CL (cause, even if I submit today - I
know I'm supposed to be off, just here for 2hs, I'll have probably to submit
other patch sets next week regarding protected reports filtering).

On 2014/06/06 20:04:43, Ken Rockot wrote:

[jracle (use Gerrit), 2014/06/10 14:07:40]

Hi Ken,
 
  today I've ben given a special firmware to try out above logic, and it appears
that it is correct! Windows pads with a leading 0x00 reports without ID.

Yet, I'm also grouping logic for filtering out protected usages now, and hence
will commit tomorrow or the day after.

Cheers

On 2014/06/07 12:56:21, jracle wrote:

 
   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) {
+  if (device_info().max_output_report_size == 0) {
     // The device does not support output 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));
   transfers_.insert(transfer);
   transfer->TakeResultFromWindowsAPI(
       WriteFile(file_.Get(),
                 output_buffer->data(),
                 static_cast<DWORD>(output_buffer->size()),
                 NULL,
                 transfer->GetOverlapped()));
 }
 
 void HidConnectionWin::GetFeatureReport(
     uint8_t report_id,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().feature_report_size == 0) {
+  if (device_info().max_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 receive_buffer_size = device_info().max_feature_report_size;
   int expected_buffer_size = receive_buffer_size;
-  if (!device_info().has_report_id)
+  if (!DeviceHasReportId(device_info()))
     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);
 
@@ skipping 11 matching lines @@
                       static_cast<DWORD>(receive_buffer->size()),
                       NULL,
                       transfer->GetOverlapped()));
 }
 
 void HidConnectionWin::SendFeatureReport(
     uint8_t report_id,
     scoped_refptr<net::IOBufferWithSize> buffer,
     const IOCallback& callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
-  if (device_info().feature_report_size == 0) {
+  if (device_info().max_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;
@@ skipping 38 matching lines @@
   }
 }
 
 void HidConnectionWin::OnTransferCanceled(
     scoped_refptr<PendingHidTransfer> transfer) {
   transfers_.erase(transfer);
   transfer->callback_.Run(false, 0);
 }
 
 }  // namespace device