[sensors] Android platform adaptation for Generic Sensor API

device/generic_sensor/android/java/src/org/chromium/device/sensors/GenericPlatformSensor.java

+// Copyright 2016 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.
+
+package org.chromium.device.sensors;
+
+import android.hardware.Sensor;
+import android.hardware.SensorEvent;
+import android.hardware.SensorEventListener;
+import android.hardware.SensorManager;
+import android.os.Build;
+import android.os.Handler;
+
+import org.chromium.mojom.device.mojom.ReportingMode;
+
+import java.nio.BufferOverflowException;
+import java.nio.ByteBuffer;
+
+import java.util.List;
+
+/**
+ * Implementation of PlatformSensor that uses Android Sensor Framework.
+ */
+public class GenericPlatformSensor extends PlatformSensor implements SensorEventListener {
+    private static final double MICROSECONDS_PER_SECOND = 1000000;
+    /**
+     * SENSOR_FREQUENCY_NORMAL corresponds to SensorManager.SENSOR_DELAY_NORMAL which is
+     * defined as 200000 microseconds.

[Ted C, 2016/09/01 00:12:36]

forgive me, but how is 5.0 related to 200000 microseconds?  There is no comment
in PlatformSensorConfiguration.

I'm suspecting this is the number of polls per second, but that isn't clear from
this comment.  Maybe this is obvious to those in the sensor world as well, so
maybe that level of knowledge is expected for users of this class.

[shalamov, 2016/09/06 12:36:42]

Done.

+     */
+    private static final double SENSOR_FREQUENCY_NORMAL = 5.0;
+    private final Sensor mSensor;

[Ted C, 2016/09/01 00:12:36]

I would add a space between the statics and the local variables.

[shalamov, 2016/09/06 12:36:42]

Done.

+    private final SensorManager mSensorManager;
+    private final Handler mHandler;
+    private final int mMinDelay;

[Ted C, 2016/09/01 00:12:36]

what are the units here?  add a Suffix (Ms, Sec, etc..)

[shalamov, 2016/09/06 12:36:43]

Done.

+    private final int mReadingCount;
+
+    protected GenericPlatformSensor(int sensorType, int readingCount, SensorManager sensorManager,

[Ted C, 2016/09/01 00:12:35]

javadoc w/ descriptions of these variables.

[shalamov, 2016/09/06 12:36:43]

Done.

+            Handler handler) throws PlatformSensorException {
+        mReadingCount = readingCount;
+        mSensorManager = sensorManager;
+        mHandler = handler;
+        List<Sensor> sensors = mSensorManager.getSensorList(sensorType);
+        if (sensors.isEmpty()) throw new PlatformSensorException();

[Ted C, 2016/09/01 00:12:35]

we have exposed very few exceptions in our code...I wonder if we really need
this and whether this could be done closer to a native style.

i.e.

public static GenericPlatformSensor(int sensorType, int readingCount, ...)
    List<Sensor> sensors = mSensorManager.getSensorList(sensorType);
    if (sensors.isEmpty()) return null;
    return new GenericPlatformSensor(...);
}

Or just throw an IllegalStateException or something.  I think a new exception
type is too much

[shalamov, 2016/09/06 12:36:42]

Done.

+        mSensor = sensors.get(0);
+        mMinDelay = mSensor.getMinDelay();
+    }
+
+    /**
+     * Fills shared buffer with sensor reading data, throws exception if number of received values
+     * is less than the number required for the PlatformSensor.
+     */
+    protected void fillSensorReadingData(SensorEvent event, ByteBuffer buffer)

[Ted C, 2016/09/01 00:12:36]

should this be private?

[shalamov, 2016/09/06 12:36:42]

Done.

+            throws PlatformSensorException {
+        if (event.values.length < mReadingCount) throw new PlatformSensorException();
+        for (int i = 0; i < mReadingCount; ++i) {
+            buffer.putDouble(event.values[i]);
+        }
+    }
+
+    @Override
+    protected int getReportingMode() {
+        if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.LOLLIPOP) {
+            return mSensor.getReportingMode() == Sensor.REPORTING_MODE_CONTINUOUS
+                    ? ReportingMode.CONTINUOUS
+                    : ReportingMode.ON_CHANGE;
+        }
+        return ReportingMode.CONTINUOUS;
+    }
+
+    @Override
+    protected PlatformSensorConfiguration getDefaultConfiguration() {
+        return new PlatformSensorConfiguration(SENSOR_FREQUENCY_NORMAL);
+    }
+
+    @Override
+    protected boolean startSensor(PlatformSensorConfiguration configuration) {
+        if (mNativePlatformSensorAndroid == 0 || mBuffer == null || mSensorReadingData == null) {

[Ted C, 2016/09/01 00:12:35]

how could this be called if mNativePlatformSensorAndroid == 0

Same thing for the buffer being null?  Is it valid to set up a sensor with those
being null?

Isn't this driven by native?  Seems like these are invalid states from the java
side and we shouldn't check them.

if anything, just add asserts:

assert mNativePlatformSensorAndroid != 0;
assert mBuffer != null;

But the buffer check should happen when the buffer is constructed/passed in java
rather than here.

[shalamov, 2016/09/06 12:36:42]

Done.

+            return false;
+        }
+
+        stopSensor();
+        return mSensorManager.registerListener(
+                this, mSensor, getSamplingPeriod(configuration), mHandler);
+    }
+
+    @Override
+    protected void stopSensor() {
+        mSensorManager.unregisterListener(this);

[Ted C, 2016/09/01 00:12:36]

Hmm...should we actually be calling this:
https://developer.android.com/reference/android/hardware/SensorManager.html#u...,
android.hardware.Sensor)

I guess it doesn't really matter since we only listen to one sensor and you can
not easily build a version that handles multiple sensor types.

[shalamov, 2016/09/06 12:36:42]

Done.

+    }
+
+    @Override
+    protected boolean checkSensorConfiguration(PlatformSensorConfiguration configuration) {
+        return mMinDelay <= getSamplingPeriod(configuration);
+    }
+
+    /**
+     * Converts frequency provided by the SensorConfiguration to sampling period in microseconds.
+     */
+    private int getSamplingPeriod(PlatformSensorConfiguration configuration) {
+        if (configuration.getFrequency() > 0) {

[Ted C, 2016/09/01 00:12:36]

is <= 0 a valid frequency?  Should we prevent that somewhere else?

[shalamov, 2016/09/06 12:36:42]

Checked in native side, cannot happen. Removed.

+            return (int) ((1 / configuration.getFrequency()) * MICROSECONDS_PER_SECOND);
+        } else {
+            return 0;
+        }
+    }
+
+    @Override
+    public void onAccuracyChanged(Sensor sensor, int accuracy) {}
+
+    @Override
+    public void onSensorChanged(SensorEvent event) {
+        try {
+            mSensorReadingData.mark();
+            // Timestamp in milliseconds
+            mSensorReadingData.putDouble(event.timestamp * 0.000001d);

[Ted C, 2016/09/01 00:12:36]

Make 0.000001d a constant too

[shalamov, 2016/09/06 12:36:42]

Done.

+            fillSensorReadingData(event, mSensorReadingData);
+
+            mSensorReadingData.reset();
+
+            mBuffer.mark();
+            mBuffer.put(mSensorReadingData);
+            mSensorReadingData.reset();
+            mBuffer.reset();
+
+            if (getReportingMode() == ReportingMode.ON_CHANGE) {
+                nativeNotifyPlatformSensorReadingChanged(mNativePlatformSensorAndroid);
+            }
+        } catch (BufferOverflowException | PlatformSensorException e) {
+            nativeNotifyPlatformSensorError(mNativePlatformSensorAndroid);
+        }
+    }
+}