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 <string>
 
 #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.
+// Legacy file name within the device in kAccelerometerIioBasePath containing
+// the scale of the accelerometers.
 const base::FilePath::CharType kScaleFileName[] = "in_accel_scale";

[flackr, 2015/08/19 21:43:25]

Can we just remove this if the scale location has changed? This was added to
accommodate the name change in kernel 3.18 but if it has changed again it seems
this won't ever be the filesystem format.

[jonross, 2015/08/20 20:34:13]

Done.

 
 // 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 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.
@@ skipping 20 matching lines @@
 // 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) << "JR Failed to parse \"" << s << "\" from " << path.value();

[flackr, 2015/08/19 21:43:25]

Remove JR, also as per my comment on 110 this should probably specify that it's
expecting an int now that there are multiple types.

[jonross, 2015/08/20 20:34:13]

Done.

     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) << "Accelerometer Failed to parse \"" << s << "\" from "

[flackr, 2015/08/19 21:43:25]

Add the type being parsed so that it's obvious from the log what it's trying to
parse as. Also, the LOG should show the filename generating the error so it's
probably redundant to say "Accelerometer".

[jonross, 2015/08/20 20:34:13]

Done.

+               << 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 30 matching lines @@
     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];
+
+    // If true the accelerometer devices must be read from distinct files.
+    bool read_device_separately;
+
+    // Length of reading per accelerometer
+    size_t device_length[ACCELEROMETER_SOURCE_COUNT];
+
+    // The path to append to |kAccelerometerDevicePath| where the accelerometer
+    // device resides.
+    base::FilePath device_path[ACCELEROMETER_SOURCE_COUNT];
   };
 
   ~AccelerometerFileReader() {}
 
   // 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.
@@ skipping 23 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);
+  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) << "Accelerometer device file invalid.\n";

[flackr, 2015/08/19 21:43:25]

Output name and change log message to describe exactly what failed so that you
can check the file in the filesystem on seeing this error. e.g. "Failed to read
symbolic link " << kAccelerometerDevicePath << "/" << name

[jonross, 2015/08/20 20:34:13]

Done.

+      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;
+    if (base::ReadFileToString(
+            base::FilePath(iio_path).Append(kAccelerometerLocationFileName),
+            &location)) {
+      configuration_.read_device_separately = true;
+      base::TrimWhitespaceASCII(location, base::TRIM_ALL, &location);
+      int config_index = -1;
+      for (size_t j = 0; j < arraysize(kAccelerometerNames); ++j) {

[flackr, 2015/08/19 21:43:25]

It would probably be more clear / shorter to use std::find.

+        if (location == kAccelerometerNames[j]) {
+          config_index = j;
+          break;
+        }
       }
+
+      if (config_index == -1) {
+        LOG(ERROR) << "JR invalid location.\n";

[flackr, 2015/08/19 21:43:25]

Name the location and the file it was read from so we can debug this.

[jonross, 2015/08/20 20:34:13]

Done.

+        return;
+      }
+
+      configuration_.device_path[config_index] = name.BaseName();
+      configuration_.has[config_index] = true;
+
+      double scale;
+      if (!ReadFileToDouble(iio_path.Append(kAccelerometerScaleFileName),
+                            &scale)) {
+        LOG(ERROR) << "Accelerometer the scale file cannot be parsed\n";

[flackr, 2015/08/19 21:43:24]

ReadFileToDouble logs if it can't read or parse the file, is this extra log line
necessary?

[jonross, 2015/08/20 20:34:13]

Nope, missed this when refactoring readtodouble out. removed.

+        return;
+      }
+
+      for (size_t j = 0; j < arraysize(kAccelerometerAxes); ++j) {
+        std::string accelerometer_index_path = base::StringPrintf(
+            "scan_elements/in_accel_%s_index", kAccelerometerAxes[j]);

[flackr, 2015/08/19 21:43:25]

Use a constant.

[jonross, 2015/08/20 20:34:13]

Done.

+        if (!ReadFileToInt(iio_path.Append(accelerometer_index_path.c_str()),
+                           &(configuration_.index[config_index][j]))) {
+          configuration_.has[config_index] = false;
+          break;
+        }
+        configuration_.scale[config_index][j] = scale;

[flackr, 2015/08/19 21:43:25]

The scale file now contains a double and scales to G's?

[jonross, 2015/08/20 20:34:13]

Correct, this was also changed in the new kernel. Also one scale per
accelerometer, instead of per axis.

+      }
+      configuration_.device_length[config_index] = kDataSize * 3;

[flackr, 2015/08/19 21:43:25]

This looks constant.

[jonross, 2015/08/20 20:34:13]

Done.

+      if (configuration_.has[config_index])
+        configuration_.count++;
+    } else {
+      // Old configuration code
+      LOG(ERROR) << "JR no location file\n";

[flackr, 2015/08/19 21:43:25]

I'd suggest splitting this off into functions for reading new and old config if
it's not reasonable for the two configurations to share a code path.

[jonross, 2015/08/19 22:22:24]

Yeah I had at first wondered if I could keep them merged. Too far diverged.

Will refactor out.

     }
-    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;

[flackr, 2015/08/19 21:43:25]

Don't we still need to fix the axes' directions?

[jonross, 2015/08/19 22:22:24]

The new values being reported are quite incorrect. Laptop mode thinkgs it's 90
degree portrait tablet.

I need to talk with them to see what is actually expected.

   }
 
   // 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)) {
         LOG(ERROR) << "Field index for " << kAccelerometerNames[i] << " "
                    << kAccelerometerAxes[j] << " axis out of bounds.";
         return;
       }
     }
   }
-  configuration_.length = kDataSize * 3 * configuration_.count;
+
+  // Legacy length
+  // 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),
@@ skipping 15 matching lines @@
     AccelerometerReader::Observer* observer) {
   observers_->RemoveObserver(observer);
 }
 
 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);

[flackr, 2015/08/19 21:43:24]

We don't need multiple triggers?

[jonross, 2015/08/19 22:22:24]

Discussion implied only one was needed. I can add logging to confirm.

[jonross, 2015/08/20 20:34:13]

Confirmed with on device testing. One trigger.

   if (bytes_written < 2) {
-    PLOG(ERROR) << "Accelerometer trigger failure: " << bytes_written;
+    LOG(ERROR) << "JR 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;
+  int bytes_read;
+  update_ = new AccelerometerUpdate();
+  if (!configuration_.read_device_separately) {
+    bytes_read = base::ReadFile(base::FilePath(kAccelerometerDevicePath),
+                                reading, configuration_.length);
+    if (bytes_read < static_cast<int>(configuration_.length)) {
+      LOG(ERROR) << "Accelerometer Read " << bytes_read << " byte(s), expected "
+                 << configuration_.length << " bytes from accelerometer";
+      return;
+    }
+    // TODO(jonross): Legacy parsing
+  } else {
+    for (int i = 0; i < ACCELEROMETER_SOURCE_COUNT; ++i) {
+      if (!configuration_.has[i])
+        continue;
+      bytes_read = base::ReadFile(base::FilePath(kAccelerometerDevicePath)
+                                      .Append(configuration_.device_path[i]),
+                                  reading, configuration_.device_length[i]);
+      if (bytes_read < static_cast<int>(configuration_.device_length[i])) {
+        LOG(ERROR) << "Acceleromter Read " << bytes_read
+                   << " byte(s), expected " << configuration_.device_length[i]
+                   << " bytes from accelerometer";
+        continue;
+      }
+
+      int16* values = reinterpret_cast<int16*>(reading);
+      update_->Set(
+          static_cast<AccelerometerSource>(i),
+          values[configuration_.index[i][0]] * configuration_.scale[i][0],
+          values[configuration_.index[i][1]] * configuration_.scale[i][1],
+          values[configuration_.index[i][2]] * configuration_.scale[i][2]);
+    }
   }
-
-  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(
-        static_cast<AccelerometerSource>(i),
-        values[configuration_.index[i][0]] * configuration_.scale[i][0],
-        values[configuration_.index[i][1]] * configuration_.scale[i][1],
-        values[configuration_.index[i][2]] * configuration_.scale[i][2]);
-  }
-
   observers_->Notify(FROM_HERE,
                      &AccelerometerReader::Observer::OnAccelerometerUpdated,
                      update_);
 }
 
-AccelerometerFileReader::ConfigurationData::ConfigurationData() : count(0) {
+AccelerometerFileReader::ConfigurationData::ConfigurationData()
+    : count(0), read_device_separately(false) {
   for (int i = 0; i < ACCELEROMETER_SOURCE_COUNT; ++i) {
     has[i] = false;
     for (int j = 0; j < 3; ++j) {
       scale[i][j] = 0;
       index[i][j] = -1;
     }
   }
 }
 
 AccelerometerFileReader::ConfigurationData::~ConfigurationData() {
@@ skipping 24 matching lines @@
 }
 
 AccelerometerReader::AccelerometerReader()
     : accelerometer_file_reader_(new AccelerometerFileReader()) {
 }
 
 AccelerometerReader::~AccelerometerReader() {
 }
 
 }  // namespace chromeos