Read and expose accelerometer values from cros-ec-accel trigger.

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
new file mode 100644
index 0000000000000000000000000000000000000000..3129750b2da345594e5a0b0291428e4c4988ef75
--- /dev/null
+++ b/chromeos/accelerometer/accelerometer_reader.cc
@@ -0,0 +1,207 @@
+// 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 "base/bind.h"
+#include "base/file_util.h"
+#include "base/location.h"
+#include "base/message_loop/message_loop.h"
+#include "base/strings/safe_sprintf.h"
+#include "base/strings/string_number_conversions.h"
+#include "base/strings/string_util.h"
+#include "base/task_runner_util.h"
+#include "base/threading/sequenced_worker_pool.h"
+#include "content/public/browser/browser_thread.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/");
+
+// Files containing the scales of the accelerometers.

[Daniel Erat, 2014/03/26 22:26:14]

nit: mention that these are files within kAccelerometerIioBasePath?

(maybe also s/Path/BaseName/ in the names to make it more clear that they aren't
full paths?)

[flackr, 2014/03/27 15:21:07]

Done.

+const base::FilePath::CharType kAccelerometerBaseScalePath[] =
+    FILE_PATH_LITERAL("in_accel_base_scale");
+const base::FilePath::CharType kAccelerometerLidScalePath[] =
+    FILE_PATH_LITERAL("in_accel_lid_scale");
+
+// 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 maximum length string needed to store the accelerometer scan index path.
+// i.e. kAccelerometerScanIndexPath with the longest of kAccelerometerNames and
+// kAccelerometerAxes filled in. This cannot be computed as it then fails to
+// find a template match to SafeSPrintf in AccelerometerReader::Initialize for
+// the array sized to the computed value.
+const size_t kMaxAccelerometerScanIndexPathLength = 37;
+
+// The names of the accelerometers and axes in the order we want to read them.
+const char kAccelerometerNames[][5] = {"base", "lid"};
+const char kAccelerometerAxes[][2] = {"x", "y", "z"};
+const size_t kTriggerDataLength =
+    2 * arraysize(kAccelerometerNames) * arraysize(kAccelerometerAxes);
+
+// The length required to read uint values from configuration files.
+const unsigned int kMaxAsciiUintLength = 21;

[Daniel Erat, 2014/03/26 22:26:14]

nit: s/unsigned int/size_t/

[flackr, 2014/03/27 15:21:07]

Done.

+
+// The time to wait between reading the accelerometer.
+const unsigned int kDelayBetweenReadsMilliseconds = 100;

[Daniel Erat, 2014/03/26 22:26:14]

nit: just use int here, maybe also s/Milliseconds/Ms/ (at least, i think that
this abbreviation is commonly used)

[flackr, 2014/03/27 15:21:07]

Done.

+
+// Reads |path| to the unsigned int pointed to by |value|. Returns true on
+// success or false on failure.
+bool ReadFileToUint(const base::FilePath& path, unsigned int* value) {
+  std::string s;
+  DCHECK(value);
+  if (!base::ReadFileToString(path,
+          &s, kMaxAsciiUintLength)) {

[Daniel Erat, 2014/03/26 22:26:14]

nit: can you unwrap this line?

[flackr, 2014/03/27 15:21:07]

Done.

+    LOG(ERROR) << "Failed to read " << path.value();
+    return false;
+  }
+  base::TrimString(s, "\r\n", &s);

[Daniel Erat, 2014/03/26 22:26:14]

nit: base::TrimWhitespace(s, base::TRIM_ALL, &s)?

[flackr, 2014/03/27 15:21:07]

Done.

+  if (!base::StringToUint(s, value)) {
+    LOG(ERROR) << "Failed to parse \"" << s << "\" from " << path.value();
+    return false;
+  }
+  return true;
+}
+
+}  // namespace
+
+using content::BrowserThread;
+
+AccelerometerReader::AccelerometerReader(AccelerometerDelegate* delegate)
+      : delegate_(delegate),
+        has_accelerometer_(false),
+        weak_factory_(this) {
+  // Asynchronously detect and initialize the accelerometer to avoid delaying
+  // startup.
+  base::PostTaskAndReplyWithResult(
+      BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(

[flackr, 2014/03/26 19:06:48]

Is there a better way to post a non-blocking task for the AccelerometerReader
from the UI thread, we of course don't want it to slow down the UI but putting
it on an IO thread would mean that it would jank while doing heavy operations
right?

[Daniel Erat, 2014/03/26 22:26:14]

i think that the blocking pool is the right place for this. in general, i
believe that it's usually safe to assume that there will be an available thread
there.

+          base::SequencedWorkerPool::SKIP_ON_SHUTDOWN).get(),

[Daniel Erat, 2014/03/26 22:26:14]

if you don't need the SKIP_ON_SHUTDOWN semantics (Initialize() doesn't seem like
it's likely to still be running when chrome shuts down, or like it'll take long
to run, right?), you can just use BrowserThread::PostBlockingPoolTaskAndReply()
here.

[flackr, 2014/03/27 15:21:07]

Done.

+      FROM_HERE,
+      base::Bind(&AccelerometerReader::Initialize,
+                 base::Unretained(this)),

[Daniel Erat, 2014/03/26 22:26:14]

pass weak_ptrs instead to make sure that these tasks won't run after the object
has been deleted

[flackr, 2014/03/27 15:21:07]

Had to avoid return values because
weak_ptrs_can_only_bind_to_methods_without_return_values. Done.

[Daniel Erat, 2014/03/27 15:37:51]

ah, that's unfortunate.

+      base::Bind(&AccelerometerReader::OnInitialized,
+                 base::Unretained(this)));
+}
+
+AccelerometerReader::~AccelerometerReader() {
+}
+
+bool AccelerometerReader::Initialize() {

[Daniel Erat, 2014/03/26 22:26:14]

nit: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[flackr, 2014/03/27 15:21:07]

Done.

+  // 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)) {

[Daniel Erat, 2014/03/26 22:26:14]

just to check: this is expected for devices that don't have accelerometers,
right?

[flackr, 2014/03/27 15:21:07]

Yes.

+    return false;
+  }
+
+  if (!base::PathExists(base::FilePath(kAccelerometerTriggerPath))) {
+    LOG(ERROR) << "Accelerometer trigger does not exist.";

[Daniel Erat, 2014/03/26 22:26:14]

nit: include kAccelerometerTriggerPath in the error message

[flackr, 2014/03/27 15:21:07]

Done.

+    return false;
+  }
+
+  base::FilePath iio_path(base::FilePath(kAccelerometerIioBasePath).Append(
+      device));
+  // Read accelerometer scales
+  if (!ReadFileToUint(iio_path.Append(kAccelerometerBaseScalePath),
+                      &accelerometer_base_scale_)) {
+    return false;

[Daniel Erat, 2014/03/26 22:26:14]

nit: log an error and include kAccelerometerBaseScalePath in the message

[flackr, 2014/03/27 15:21:07]

For here and following uses, errors are already logged (with the path) in
ReadFileToUint for both failure cases, is this okay?

[Daniel Erat, 2014/03/27 15:37:51]

sure, sounds fine.

+  }
+  if (!ReadFileToUint(iio_path.Append(kAccelerometerLidScalePath),
+                      &accelerometer_lid_scale_)) {
+    return false;

[Daniel Erat, 2014/03/26 22:26:14]

nit: log an error and include kAccelerometerLidScalePath in the message

[flackr, 2014/03/27 15:21:07]

See comment above.

+  }
+
+  // Read indices of each accelerometer axis from each accelerometer from
+  // /sys/bus/iio/devices/iio:deviceX/scan_elements/in_accel_{x,y,z}_%s_index
+  for (size_t i = 0; i < arraysize(kAccelerometerNames); ++i) {
+    for (size_t j = 0; j < arraysize(kAccelerometerAxes); ++j) {
+      char accelerometer_index_path[kMaxAccelerometerScanIndexPathLength];
+      unsigned int index = 0;
+      base::strings::SafeSPrintf(accelerometer_index_path,

[Daniel Erat, 2014/03/26 22:26:14]

can you just use an std::string in conjunction with base::StringPrintf() instead
of a fixed-size char buffer here?

[flackr, 2014/03/27 15:21:07]

Done.

+          kAccelerometerScanIndexPath, kAccelerometerAxes[j],
+          kAccelerometerNames[i]);
+      if (!ReadFileToUint(iio_path.Append(accelerometer_index_path),
+                          &index)) {
+        return false;

[Daniel Erat, 2014/03/26 22:26:14]

nit: log an error and include the path name in the message

[flackr, 2014/03/27 15:21:07]

See comment above.

+      }
+      accelerometer_index_.push_back(index);

[Daniel Erat, 2014/03/26 22:26:14]

do bounds-checking here to ensure that a buggy/malicious accelerometer can't
trick you into an out-of-bounds read later in ReadAccelerometer()

[flackr, 2014/03/27 15:21:07]

Done.

+    }
+  }
+  return true;
+}
+
+void AccelerometerReader::OnInitialized(bool success) {

[Daniel Erat, 2014/03/26 22:26:14]

nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[flackr, 2014/03/27 15:21:07]

Done.

+  has_accelerometer_ = success;
+  if (has_accelerometer_)
+    TriggerRead();
+}
+
+void AccelerometerReader::TriggerRead() {

[Daniel Erat, 2014/03/26 22:26:14]

nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[flackr, 2014/03/27 15:21:07]

Done.

+  base::PostTaskAndReplyWithResult(
+      BrowserThread::GetBlockingPool()->GetTaskRunnerWithShutdownBehavior(
+          base::SequencedWorkerPool::SKIP_ON_SHUTDOWN).get(),
+      FROM_HERE,
+      base::Bind(&AccelerometerReader::ReadAccelerometer,
+                 base::Unretained(this)),
+      base::Bind(&AccelerometerReader::OnDataRead,
+                 base::Unretained(this)));

[Daniel Erat, 2014/03/26 22:26:14]

same comments here as above

[flackr, 2014/03/27 15:21:07]

Done.

+}
+
+bool AccelerometerReader::ReadAccelerometer() {

[Daniel Erat, 2014/03/26 22:26:14]

nit: DCHECK(BrowserThread::GetBlockingPool()->RunsTasksOnCurrentThread());

[flackr, 2014/03/27 15:21:07]

Done.

+  // Initiate the trigger to read accelerometers simultaneously
+  int bytesWritten = base::WriteFile(

[Daniel Erat, 2014/03/26 22:26:14]

s/bytesWritten/bytes_written/

[flackr, 2014/03/27 15:21:07]

Done.

+      base::FilePath(kAccelerometerTriggerPath), "1\n", 2);
+  if (bytesWritten < 2) {
+    LOG(ERROR) << "Accelerometer trigger failure: " << bytesWritten;

[Daniel Erat, 2014/03/26 22:26:14]

nit: PLOG(ERROR)

[flackr, 2014/03/27 15:21:07]

Done.

+    return false;
+  }
+
+  // Read resulting sample from /dev/cros-ec-accel.
+  char data[kTriggerDataLength];
+  int bytesRead = base::ReadFile(base::FilePath(kAccelerometerDevicePath),

[Daniel Erat, 2014/03/26 22:26:14]

s/bytesRead/bytes_read/

[flackr, 2014/03/27 15:21:07]

Done.

+                                 data, kTriggerDataLength);

[Daniel Erat, 2014/03/26 22:26:14]

just out of curiosity: have you timed this? how long can it block?

[flackr, 2014/03/27 15:21:07]

I haven't timed it yet, but I can do this. I believe it to be quite fast, given
the initialization, reading the first value, and screen rotation can finish
before you can see the login screen (i.e. it looks as though the login screen
was always correctly oriented).

+  if (bytesRead < static_cast<int>(kTriggerDataLength)) {
+    LOG(ERROR) << "Read " << bytesRead << " bytes, expected "

[Daniel Erat, 2014/03/26 22:26:14]

nit: s/bytes/byte(s)/

[flackr, 2014/03/27 15:21:07]

Done.

+               << kTriggerDataLength << " bytes from accelerometer";
+    return false;
+  }
+
+  base_accelerometer_.set_x(*((int16*)data + accelerometer_index_[0]));

[Daniel Erat, 2014/03/26 22:26:14]

nit: use reinterpret_cast<int16*> instead of c-style casts

[flackr, 2014/03/27 15:21:07]

Done.

+  base_accelerometer_.set_y(*((int16*)data + accelerometer_index_[1]));
+  base_accelerometer_.set_z(*((int16*)data + accelerometer_index_[2]));
+  base_accelerometer_.Scale(1.0f / accelerometer_base_scale_);
+
+  lid_accelerometer_.set_x(*((int16*)data + accelerometer_index_[3]));
+  lid_accelerometer_.set_y(*((int16*)data + accelerometer_index_[4]));
+  lid_accelerometer_.set_z(*((int16*)data + accelerometer_index_[5]));
+  lid_accelerometer_.Scale(1.0f / accelerometer_lid_scale_);

[Daniel Erat, 2014/03/26 22:26:14]

i'm a bit concerned that a future change could add the ability for outside
classes to read |base_accelerometer_| and |lid_accelerometer_| directly from
this class on the UI thread, in which case it'd be possible to read these while
they're being updated on the blocking pool. an alternative approach would be
passing the newly-read values back from the blocking pool to the ui thread. i'm
not sure if you can do this without packing them into a new struct, though.

[flackr, 2014/03/27 15:21:07]

I added a comment to this effect (in the header), and warning others not to read
these values directly, and I'd be happy to look into this in a future CL.

+  return true;
+}
+
+void AccelerometerReader::OnDataRead(bool success) {

[Daniel Erat, 2014/03/26 22:26:14]

nit: DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));

[flackr, 2014/03/27 15:21:07]

Done.

+  // If successful, notify the AccelerometerDelegate.
+  if (success)
+    delegate_->OnAccelerometerRead(base_accelerometer_, lid_accelerometer_);
+
+  // Trigger another read after the current sampling delay.
+  base::MessageLoop::current()->PostDelayedTask(
+      FROM_HERE,
+      base::Bind(&AccelerometerReader::TriggerRead,
+                 weak_factory_.GetWeakPtr()),
+      base::TimeDelta::FromMilliseconds(kDelayBetweenReadsMilliseconds));
+}
+
+}  // namespace chromeos