Update AccelerometerReader to support separate iio devices

chromeos/accelerometer/accelerometer_reader.cc

Index: chromeos/accelerometer/accelerometer_reader.cc
diff --git a/chromeos/accelerometer/accelerometer_reader.cc b/chromeos/accelerometer/accelerometer_reader.cc
index 04122ca09b90f18974d914382fba2a87a5721ce9..790e950892a4ca8433ea077acffd991d1dee5c8c 100644
--- a/chromeos/accelerometer/accelerometer_reader.cc
+++ b/chromeos/accelerometer/accelerometer_reader.cc
@@ -7,6 +7,7 @@
 #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"
@@ -32,8 +33,8 @@ const base::FilePath::CharType kAccelerometerDevicePath[] =
 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
@@ -42,6 +43,14 @@ const base::FilePath::CharType kScaleFileName[] = "in_accel_scale";
 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[] =
@@ -82,7 +91,24 @@ bool ReadFileToInt(const base::FilePath& path, int* value) {
   }
   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;
@@ -136,6 +162,16 @@ class AccelerometerFileReader
 
     // 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() {}
@@ -179,8 +215,10 @@ void AccelerometerFileReader::Initialize(
   // 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;
   }
 
@@ -190,59 +228,64 @@ void AccelerometerFileReader::Initialize(
     return;
   }
 
-  base::FilePath iio_path(base::FilePath(kAccelerometerIioBasePath).Append(
-      device));
+  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;
+    }
 
-  // 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;
-  }
+    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;
+        }
+      }
 
-  // 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 (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;
       }
-      if (scale_divisor == 0) {
-        LOG(ERROR) << "Accelerometer " << accelerometer_scale_path
-                   << "has scale of 0 and will not be used.";
-        continue;
+
+      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;
       }
-    }
 
-    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;
+      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.
@@ -259,7 +302,9 @@ void AccelerometerFileReader::Initialize(
       }
     }
   }
-  configuration_.length = kDataSize * 3 * configuration_.count;
+
+  // Legacy length
+  // configuration_.length = kDataSize * 3 * configuration_.count;
   initialization_successful_ = true;
   Read();
 }
@@ -297,39 +342,51 @@ void AccelerometerFileReader::ReadFileAndNotify() {
   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;
+      }
 
-  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]);
+      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) {