Update AccelerometerReader to support separate iio devices

chromeos/accelerometer/accelerometer_reader.cc

 // Copyright 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 "chromeos/accelerometer/accelerometer_reader.h"
 
+#include <algorithm>
 #include <string>
+#include <vector>
 
 #include "base/bind.h"
+#include "base/files/file_enumerator.h"
 #include "base/files/file_util.h"
 #include "base/location.h"
 #include "base/memory/singleton.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/task_runner.h"
 #include "base/task_runner_util.h"
 #include "base/thread_task_runner_handle.h"
 #include "base/threading/platform_thread.h"
 #include "base/threading/sequenced_worker_pool.h"
 
 namespace chromeos {
 
 namespace {
 
 // Paths to access necessary data from the accelerometer device.
 const base::FilePath::CharType kAccelerometerTriggerPath[] =
     FILE_PATH_LITERAL("/sys/bus/iio/devices/trigger0/trigger_now");
 const base::FilePath::CharType kAccelerometerDevicePath[] =
     FILE_PATH_LITERAL("/dev/cros-ec-accel");
 const base::FilePath::CharType kAccelerometerIioBasePath[] =
     FILE_PATH_LITERAL("/sys/bus/iio/devices/");
 
-// File within the device in kAccelerometerIioBasePath containing the scale of
-// the accelerometers.
-const base::FilePath::CharType kScaleFileName[] = "in_accel_scale";
-
 // This is the per source scale file in use on kernels older than 3.18. We
 // should remove this when all devices having accelerometers are on kernel 3.18
 // or later or have been patched to use new format: http://crbug.com/510831
 const base::FilePath::CharType kSourceScaleNameFormatString[] =
     "in_accel_%s_scale";
 
+// File within kAccelerometerDevicePath/device* which denotes a single scale to
+// be used across all axes.
+const base::FilePath::CharType kAccelerometerScaleFileName[] = "scale";
+
+// File within kAccelerometerDevicePath/device* which denotes the
+// AccelerometerSource for the accelerometer.
+const base::FilePath::CharType kAccelerometerLocationFileName[] = "location";
+
 // The filename giving the path to read the scan index of each accelerometer
 // axis.
 const char kAccelerometerScanIndexPath[] =
     "scan_elements/in_accel_%s_%s_index";
 
+// The filename giving the path to read the scan index of each accelerometer
+// when they are separate device paths.
+const char kSeparateAccelerometerScanIndexPath[] =

[Daniel Erat, 2015/08/27 15:36:19]

marginal nit: i think i'd prefer if this constant and
kAccelerometerScanIndexPath had names ending with "FormatString"

[jonross, 2015/08/27 19:20:57]

Done.

+    "scan_elements/in_accel_%s_index";
+
 // The names of the accelerometers. Matches up with the enum AccelerometerSource
 // in chromeos/accelerometer/accelerometer_types.h.
 const char kAccelerometerNames[ACCELEROMETER_SOURCE_COUNT][5] = {"lid", "base"};
 
 // The axes on each accelerometer.
-const char kAccelerometerAxes[][2] = {"y", "x", "z"};
+const char kSeparateAccelerometerAxes[][2] = {"x", "y", "z"};
+const char kUnifiedAccelerometerAxes[][2] = {"y", "x", "z"};
 
 // The length required to read uint values from configuration files.
 const size_t kMaxAsciiUintLength = 21;
 
 // The size of individual values.
 const size_t kDataSize = 2;
 
+// The size of reading values for an entire accelerometer, when they are
+// separate device paths.
+const size_t kDataSizeForSeparateDevices = 6;
+
 // The mean acceleration due to gravity on Earth in m/s^2.
 const float kMeanGravity = 9.80665f;
 
 // The number of accelerometers.
 const int kNumberOfAccelerometers = 2;
 
 // The number of axes for which there are acceleration readings.
 const int kNumberOfAxes = 3;
 
 // The size of data in one reading of the accelerometers.
 const int kSizeOfReading = kDataSize * kNumberOfAccelerometers * kNumberOfAxes;
 
 // Reads |path| to the unsigned int pointed to by |value|. Returns true on
 // success or false on failure.
 bool ReadFileToInt(const base::FilePath& path, int* value) {
   std::string s;
   DCHECK(value);
   if (!base::ReadFileToString(path, &s, kMaxAsciiUintLength)) {
     return false;
   }
   base::TrimWhitespaceASCII(s, base::TRIM_ALL, &s);
   if (!base::StringToInt(s, value)) {
-    LOG(ERROR) << "Failed to parse \"" << s << "\" from " << path.value();
+    LOG(ERROR) << "Failed to parse int \"" << s << "\" from " << path.value();
     return false;
   }
   return true;
+}
+
+// Reads |path| to the double pointed to by |value|. Returns true on success or
+// false on failure.
+bool ReadFileToDouble(const base::FilePath& path, double* value) {
+  std::string s;
+  DCHECK(value);
+  if (!base::ReadFileToString(path, &s)) {
+    return false;
+  }
+  base::TrimWhitespaceASCII(s, base::TRIM_ALL, &s);
+  if (!base::StringToDouble(s, value)) {
+    LOG(ERROR) << "Failed to parse double \"" << s << "\" from "
+               << path.value();
+    return false;
+  }
+  return true;
 }
 
 }  // namespace
 
 const int AccelerometerReader::kDelayBetweenReadsMs = 100;
 
 // Work that runs on a base::TaskRunner. It determines the accelerometer
 // configuartion, and reads the data. Upon a successful read it will notify
 // all observers.
 class AccelerometerFileReader
@@ skipping 11 matching lines @@
   // read at the current sampling rate.
   void Read();
 
   // Add/Remove observers.
   void AddObserver(AccelerometerReader::Observer* observer);
   void RemoveObserver(AccelerometerReader::Observer* observer);
 
  private:
   friend class base::RefCountedThreadSafe<AccelerometerFileReader>;
 
+  // Represents necessary information in order to read an accelerometer device.
+  struct ReadingData {
+    // The full path to the accelerometer device to read.
+    base::FilePath path;
+
+    // The length of accelerometer updates to be read.
+    size_t length;
+
+    // All readings are placed in the same buffer, this is the offset to begin
+    // reading into for this accelerometer.
+    size_t offset;

[flackr, 2015/08/27 16:24:22]

Instead of length and offset, if we have std::vector<AccelerometerSource>
sources then the read code can process each file immediately, e.g.:

Then the read code would look like:
update_ = new AccelerometerUpdate();
for (auto reading_data : configuration_.reading_data) {
  ReadFile(reading_data.path, buffer, reading_data.size() * kReadingSize);
  for (AccelerometerSource source : reading_data.sources) {
    int16* values = reinterpret_cast<int16*>(buffer);
    update_->Set(...);
  }
}

The main advantage being we don't have to change the indices we've read from the
accelerometer config so it's closer to the real /sys filesystem and it's closer
to what we'll need when we have some sensors reading at separate rates.

[jonross, 2015/08/27 19:20:57]

Done.

+  };
+
   // Configuration structure for accelerometer device.
   struct ConfigurationData {
     ConfigurationData();
     ~ConfigurationData();
 
     // Number of accelerometers on device.
     size_t count;
 
-    // Length of accelerometer updates.
-    size_t length;
-
     // Which accelerometers are present on device.
     bool has[ACCELEROMETER_SOURCE_COUNT];
 
     // Scale of accelerometers (i.e. raw value * scale = m/s^2).
     float scale[ACCELEROMETER_SOURCE_COUNT][3];
 
     // Index of each accelerometer axis in data stream.
     int index[ACCELEROMETER_SOURCE_COUNT][3];
+
+    // The information for each accelerometer device to be read. In kernel 3.18
+    // there is one per ACCELEROMETER_SOURCE_COUNT, on 3.14 there is only one.
+    std::vector<ReadingData> reading_data;
   };
 
   ~AccelerometerFileReader() {}
 
+  // When accelerometers are presented as separate iio_devices this will perform
+  // the initialize for one of the devices, at the given |iio_path| and the
+  // symbolic link |name|. |location| is defined by AccelerometerSoure.
+  bool InitializeAccelerometer(const base::FilePath& iio_path,
+                               const base::FilePath& name,
+                               const std::string& location);
+
+  // TODO(jonross): Separate the initialization into separate files. Add a gyp
+  // rule to have them built for the appropriate kernels. (crbug.com/525658)
+  // When accelerometers are presented as a single iio_device this will perform
+  // the initialization for both of them.
+  bool InitializeLegacyAccelerometers(const base::FilePath& iio_path,
+                                      const base::FilePath& name);
+
   // Attempts to read the accelerometer data. Upon a success, converts the raw
   // reading to an AccelerometerUpdate and notifies observers.
   void ReadFileAndNotify();
 
   // True if Initialize completed successfully, and there is an accelerometer
   // file to read.
   bool initialization_successful_;
 
   // The accelerometer configuration.
   ConfigurationData configuration_;
@@ skipping 19 matching lines @@
 
 void AccelerometerFileReader::Initialize(
     scoped_refptr<base::SequencedTaskRunner> sequenced_task_runner) {
   DCHECK(
       base::SequencedWorkerPool::GetSequenceTokenForCurrentThread().IsValid());
   task_runner_ = sequenced_task_runner;
 
   // Check for accelerometer symlink which will be created by the udev rules
   // file on detecting the device.
   base::FilePath device;
-  if (!base::ReadSymbolicLink(base::FilePath(kAccelerometerDevicePath),
-                              &device)) {
+
+  if (base::IsDirectoryEmpty(base::FilePath(kAccelerometerDevicePath))) {
+    LOG(ERROR) << "Accelerometer device directory is empty at "
+               << kAccelerometerDevicePath;
     return;
   }
 
   if (!base::PathExists(base::FilePath(kAccelerometerTriggerPath))) {
     LOG(ERROR) << "Accelerometer trigger does not exist at"
                << kAccelerometerTriggerPath;
     return;
   }
 
-  base::FilePath iio_path(base::FilePath(kAccelerometerIioBasePath).Append(
-      device));
-
-  // Read the scale for all axes.
-  int scale_divisor = 0;
-  bool per_source_scale =
-      !ReadFileToInt(iio_path.Append(kScaleFileName), &scale_divisor);
-  if (!per_source_scale && scale_divisor == 0) {
-    LOG(ERROR) << "Accelerometer " << kScaleFileName
-               << "has scale of 0 and will not be used.";
-    return;
-  }
-
-  // Read configuration of each accelerometer axis from each accelerometer from
-  // /sys/bus/iio/devices/iio:deviceX/.
-  for (size_t i = 0; i < arraysize(kAccelerometerNames); ++i) {
-    if (per_source_scale) {
-      configuration_.has[i] = false;
-      // Read scale of accelerometer.
-      std::string accelerometer_scale_path = base::StringPrintf(
-          kSourceScaleNameFormatString, kAccelerometerNames[i]);
-      if (!ReadFileToInt(iio_path.Append(accelerometer_scale_path.c_str()),
-                         &scale_divisor)) {
-        continue;
-      }
-      if (scale_divisor == 0) {
-        LOG(ERROR) << "Accelerometer " << accelerometer_scale_path
-                   << "has scale of 0 and will not be used.";
-        continue;
-      }
+  base::FileEnumerator symlink_dir(base::FilePath(kAccelerometerDevicePath),
+                                   false, base::FileEnumerator::FILES);
+  bool separate_devices = false;
+  for (base::FilePath name = symlink_dir.Next(); !name.empty();
+       name = symlink_dir.Next()) {
+    base::FilePath iio_device;
+    if (!base::ReadSymbolicLink(name, &iio_device)) {
+      LOG(ERROR) << "Failed to read symbolic link " << kAccelerometerDevicePath
+                 << "/" << name.MaybeAsASCII() << "\n";
+      return;
     }
 
-    configuration_.has[i] = true;
-    for (size_t j = 0; j < arraysize(kAccelerometerAxes); ++j) {
-      configuration_.scale[i][j] = kMeanGravity / scale_divisor;
-      std::string accelerometer_index_path = base::StringPrintf(
-          kAccelerometerScanIndexPath, kAccelerometerAxes[j],
-          kAccelerometerNames[i]);
-      if (!ReadFileToInt(iio_path.Append(accelerometer_index_path.c_str()),
-                         &(configuration_.index[i][j]))) {
-        configuration_.has[i] = false;
-        break;
-      }
+    base::FilePath iio_path(base::FilePath(kAccelerometerIioBasePath)
+                                .Append(iio_device.BaseName()));
+    std::string location;
+    separate_devices = base::ReadFileToString(
+        base::FilePath(iio_path).Append(kAccelerometerLocationFileName),
+        &location);
+    if (separate_devices) {
+      base::TrimWhitespaceASCII(location, base::TRIM_ALL, &location);
+      if (!InitializeAccelerometer(iio_path, name, location))
+        return;
+    } else {
+      if (!InitializeLegacyAccelerometers(iio_path, name))
+        return;
     }
-    if (configuration_.has[i])
-      configuration_.count++;
-  }
-
-  // Adjust the directions of accelerometers to match the AccelerometerUpdate
-  // type specified in chromeos/accelerometer/accelerometer_types.h.
-  configuration_.scale[ACCELEROMETER_SOURCE_SCREEN][0] *= -1.0f;
-  for (int i = 0; i < 3; ++i) {
-    configuration_.scale[ACCELEROMETER_SOURCE_ATTACHED_KEYBOARD][i] *= -1.0f;
   }
 
   // Verify indices are within bounds.
   for (int i = 0; i < ACCELEROMETER_SOURCE_COUNT; ++i) {
     if (!configuration_.has[i])
       continue;
     for (int j = 0; j < 3; ++j) {
       if (configuration_.index[i][j] < 0 ||
           configuration_.index[i][j] >=
               3 * static_cast<int>(configuration_.count)) {
+        const char* axis = separate_devices ? kSeparateAccelerometerAxes[j]
+                                            : kUnifiedAccelerometerAxes[j];
         LOG(ERROR) << "Field index for " << kAccelerometerNames[i] << " "
-                   << kAccelerometerAxes[j] << " axis out of bounds.";
+                   << axis << " axis out of bounds.";
         return;
       }
     }
   }
-  configuration_.length = kDataSize * 3 * configuration_.count;
+
   initialization_successful_ = true;
   Read();
 }
 
 void AccelerometerFileReader::Read() {
   DCHECK(
       base::SequencedWorkerPool::GetSequenceTokenForCurrentThread().IsValid());
   ReadFileAndNotify();
   task_runner_->PostNonNestableDelayedTask(
       FROM_HERE, base::Bind(&AccelerometerFileReader::Read, this),
       base::TimeDelta::FromMilliseconds(
           AccelerometerReader::kDelayBetweenReadsMs));
 }
 
 void AccelerometerFileReader::AddObserver(
     AccelerometerReader::Observer* observer) {
   observers_->AddObserver(observer);
   if (initialization_successful_) {
     task_runner_->PostNonNestableTask(
         FROM_HERE,
         base::Bind(&AccelerometerFileReader::ReadFileAndNotify, this));
   }
 }
 
 void AccelerometerFileReader::RemoveObserver(
     AccelerometerReader::Observer* observer) {
   observers_->RemoveObserver(observer);
 }
 
+bool AccelerometerFileReader::InitializeAccelerometer(
+    const base::FilePath& iio_path,
+    const base::FilePath& name,
+    const std::string& location) {
+  int config_index = std::find(std::begin(kAccelerometerNames),

[Daniel Erat, 2015/08/27 15:36:19]

huh, i hadn't seen this before. just to double-check, is it allowed in chromium?

+                               std::end(kAccelerometerNames), location) -
+                     std::begin(kAccelerometerNames);

[Daniel Erat, 2015/08/27 15:36:19]

is it safe to subtract std::begin from std::last (if no value is found) like
this?

part of me feels like the simple loop-based approach (one line longer) is easier
to read, but i can be swayed if flackr@ disagrees. :-)

  int config_index = 0;
  for (; config_index < ARRAYSIZE(kAccelerometerNames); ++config_index) {
    if (location == kAccelerometerNames[config_index])
      break;
  }
  if (config_index >= ARRAYSIZE(kAccelerometerNames)) {
    LOG(ERROR) << ...

[flackr, 2015/08/27 16:24:22]

std::find returns the second argument (the end position, last) if it doesn't
find the item. This means if we fail to find the item config_index will actually
be ARRAYSIZE(kAccelerometerNames) (e.g. 2, not -1). That being said, this is
confusing to deduce.
I hadn't realized all the iterator conversions that using std::find with arrays
requires. I had imagined something like.

auto accel_iter = std::find(std::begin(kAccelerometerNames),
std::end(kAccelerometerNames), location);
if (accel_iter == std::end(kAccelerometerNames) {
  LOG(ERROR) << "..."
  return;
}
int config_index = accel_iter - std::begin(kAccelerometerNames);

But seeing it written out, the loop approach is probably simpler.

[jonross, 2015/08/27 19:20:57]

Yeah I thought std:find would have been cleaner than it was.

Switched to the for loop

+  if (config_index == -1) {
+    LOG(ERROR) << "Unrecognized location: " << location << " for device "
+               << name.MaybeAsASCII() << "\n";
+    return false;
+  }
+
+  double scale;
+  if (!ReadFileToDouble(iio_path.Append(kAccelerometerScaleFileName), &scale))
+    return false;
+
+  const int number_axes = arraysize(kSeparateAccelerometerAxes);

[Daniel Erat, 2015/08/27 15:36:19]

nit: kNumberAxes

[jonross, 2015/08/27 19:20:57]

Done.

+  for (size_t j = 0; j < number_axes; ++j) {

[Daniel Erat, 2015/08/27 15:36:19]

nit: s/j/i/

[jonross, 2015/08/27 19:20:57]

Done.

+    std::string accelerometer_index_path = base::StringPrintf(
+        kSeparateAccelerometerScanIndexPath, kSeparateAccelerometerAxes[j]);
+    if (!ReadFileToInt(iio_path.Append(accelerometer_index_path.c_str()),
+                       &(configuration_.index[config_index][j]))) {
+      LOG(ERROR) << "Index file " << accelerometer_index_path
+                 << " could not be parsed\n";
+      return false;
+    }
+    configuration_.index[config_index][j] += config_index * number_axes;
+    configuration_.scale[config_index][j] = scale;
+  }
+  configuration_.has[config_index] = true;
+  configuration_.count++;
+
+  ReadingData reading_data;
+  reading_data.path =
+      base::FilePath(kAccelerometerDevicePath).Append(name.BaseName());
+  reading_data.offset = config_index * kDataSizeForSeparateDevices;
+  reading_data.length = kDataSizeForSeparateDevices;
+
+  configuration_.reading_data.push_back(reading_data);
+
+  return true;
+}
+
+bool AccelerometerFileReader::InitializeLegacyAccelerometers(
+    const base::FilePath& iio_path,
+    const base::FilePath& name) {
+  // Read the scale for all axes.
+  int scale_divisor = 0;
+  // Read configuration of each accelerometer axis from each accelerometer from
+  // /sys/bus/iio/devices/iio:deviceX/.
+  for (size_t i = 0; i < arraysize(kAccelerometerNames); ++i) {
+    configuration_.has[i] = false;
+    // Read scale of accelerometer.
+    std::string accelerometer_scale_path = base::StringPrintf(
+        kSourceScaleNameFormatString, kAccelerometerNames[i]);
+    if (!ReadFileToInt(iio_path.Append(accelerometer_scale_path.c_str()),
+                       &scale_divisor)) {
+      continue;
+    }
+    if (scale_divisor == 0) {
+      LOG(ERROR) << "Accelerometer " << accelerometer_scale_path
+                 << "has scale of 0 and will not be used.";
+      continue;
+    }
+
+    configuration_.has[i] = true;
+    for (size_t j = 0; j < arraysize(kUnifiedAccelerometerAxes); ++j) {
+      configuration_.scale[i][j] = kMeanGravity / scale_divisor;
+      std::string accelerometer_index_path = base::StringPrintf(
+          kAccelerometerScanIndexPath, kUnifiedAccelerometerAxes[j],
+          kAccelerometerNames[i]);
+      if (!ReadFileToInt(iio_path.Append(accelerometer_index_path.c_str()),
+                         &(configuration_.index[i][j]))) {
+        configuration_.has[i] = false;
+        LOG(ERROR) << "Index file " << accelerometer_index_path
+                   << " could not be parsed\n";
+        return false;
+      }
+    }
+    if (configuration_.has[i])
+      configuration_.count++;
+  }
+
+  // Adjust the directions of accelerometers to match the AccelerometerUpdate
+  // type specified in chromeos/accelerometer/accelerometer_types.h.
+  configuration_.scale[ACCELEROMETER_SOURCE_SCREEN][1] *= -1.0f;
+  configuration_.scale[ACCELEROMETER_SOURCE_SCREEN][2] *= -1.0f;
+
+  ReadingData reading_data;
+  reading_data.path =
+      base::FilePath(kAccelerometerDevicePath).Append(name.BaseName());
+  reading_data.offset = 0;
+  reading_data.length = kDataSize * 3 * configuration_.count;
+
+  configuration_.reading_data.push_back(reading_data);
+  return true;
+}
+
 void AccelerometerFileReader::ReadFileAndNotify() {
   DCHECK(initialization_successful_);
-  char reading[kSizeOfReading];
 
   // Initiate the trigger to read accelerometers simultaneously
   int bytes_written = base::WriteFile(
       base::FilePath(kAccelerometerTriggerPath), "1\n", 2);
   if (bytes_written < 2) {
     PLOG(ERROR) << "Accelerometer trigger failure: " << bytes_written;
     return;
   }
 
   // Read resulting sample from /dev/cros-ec-accel.
-  int bytes_read = base::ReadFile(base::FilePath(kAccelerometerDevicePath),
-                                  reading, configuration_.length);
-  if (bytes_read < static_cast<int>(configuration_.length)) {
-    LOG(ERROR) << "Read " << bytes_read << " byte(s), expected "
-               << configuration_.length << " bytes from accelerometer";
-    return;
+  char reading[kSizeOfReading];
+  int bytes_read;

[Daniel Erat, 2015/08/27 15:36:19]

nit: scope this within the for loop

[jonross, 2015/08/27 19:20:57]

Done.

+  for (auto reading_data : configuration_.reading_data) {
+    bytes_read = base::ReadFile(
+        reading_data.path, reading + reading_data.offset, reading_data.length);
+    if (bytes_read < static_cast<int>(reading_data.length)) {
+      LOG(ERROR) << "Accelerometer Read " << bytes_read << " byte(s), expected "
+                 << reading_data.length << " bytes from accelerometer "
+                 << reading_data.path.MaybeAsASCII();
+      return;
+    }
   }
 
   update_ = new AccelerometerUpdate();
 
   for (int i = 0; i < ACCELEROMETER_SOURCE_COUNT; ++i) {
     if (!configuration_.has[i])
       continue;
 
     int16* values = reinterpret_cast<int16*>(reading);
     update_->Set(
@@ skipping 46 matching lines @@
 }
 
 AccelerometerReader::AccelerometerReader()
     : accelerometer_file_reader_(new AccelerometerFileReader()) {
 }
 
 AccelerometerReader::~AccelerometerReader() {
 }
 
 }  // namespace chromeos