[Sensor] Initial implementation of the Generic Sensor API #353

jimmy-huang · 2016-11-02T20:54:22Z

Initial implementation for the W3C Generic Sensor API and add
support for the Accelerometer and Gyroscope sensors using
the BMI160 inertia sensor built-in on the Arduino 101,
the current implementation is polling from the ARC and channeling
the data back to X86.

Also adding two sample JS code testing both APIs.

Signed-off-by: Jimmy Huang [email protected]

kenchris · 2016-11-03T07:40:25Z

arc/src/main.c

@@ -322,6 +324,7 @@ static void handle_glcd(struct zjs_ipm_message* msg)
            glcd = device_get_binding(GROVE_LCD_NAME);

            if (!glcd) {
+                PRINT("failed to initialize Grove LCD\n");


Seems unrelated

Yes, I forgot to add it to the glcd when I am implementing sensor, so I am thinking to include it as part of the patch, but I'll remove it and throw in another patch.

kenchris · 2016-11-03T07:51:37Z

src/zjs_sensor.c

+
+    if (old_state == SENSOR_STATE_ACTIVATING &&
+        state == SENSOR_STATE_ACTIVATED) {
+        func = zjs_get_property(obj, "onactivate");


Great that you are actively following the spec, this was just added two days ago :-)

w3c/sensors@a85830b

kenchris · 2016-11-03T07:52:46Z

src/zjs_sensor.c

+
+static struct nano_sem sensor_sem;
+
+enum sensor_state {


I believe these might become internals: Check

w3c/sensors#104 (comment)

(that part is not fixed in spec yet, ie the checkbox isnt checked yet)

We discussed these changes back in the W3C TPAC meeting in Lisbon and they are slowly making it into the spec

We might do away with "states" (internal slots, so not exposed to developers) , but right now its not in the spec yet.

kenchris · 2016-11-03T07:55:17Z

src/zjs_sensor.c

+            // For now, frequency is ignored,  we just report new event
+            // as soon as we detect a value change.
+            if (option_freq <= 0) {
+                PRINT("zjs_sensor_create: invalid frequency, default to 50hz\n");


kenchris · 2016-11-03T07:55:57Z

src/zjs_sensor.c

+            bool option_gravity;
+            if (zjs_obj_get_boolean(options, "includeGravity", &option_gravity) &&
+                option_gravity) {
+                // TODO: find out if BMI160 can be configured to include gravity


I guess the math to add it manually wouldn't be too hard

that's one possible way to do it

kenchris · 2016-11-03T07:59:10Z

src/zjs_ipm.h

+#endif
+
+#ifdef BUILD_MODULE_SENSOR
+        struct sensor_data {


For external sensors (ie DHT22 etc) we would probably need something like "controller" here so that I can do

new Hygrometer( { controller: "dht22" } ) which would hook up to the right Zephyr driver

Yeah, I was just thinking the same, since I need to add the ambient light sensor later, and if sensor is external, then I might need to pass in like the I/O pin it's connected to as well.

kenchris · 2016-11-03T07:59:57Z

scripts/analyze.sh

@@ -104,6 +104,11 @@ if [ $? -eq 0 ] && [[ $MODULE != *"BUILD_MODULE_BUFFER"* ]]; then
    >&2 echo Using module: Buffer
    MODULES+=" -DBUILD_MODULE_BUFFER"
 fi
+sensor=$(grep -E AccelerometerSensor\|Gyroscope  $SCRIPT)


I thought the Sensor part was being removed... I will ask Riju to look at this PR

Yes, we have removed the "sensor" part. https://w3c.github.io/accelerometer/#accelerometer

Ok, I'll update accordingly, I was wondering that too since it's odd that the one has, but one does not.

kenchris · 2016-11-03T08:00:19Z

samples/Accelerometer.js

+// Test code to use the AccelerometerSensor (subclass of Generic Sensor) API
+// to communicate with the BMI160 inertia sensor on the Arduino 101
+// and obtaining information about acceleration applied to the X, Y and Z axis
+console.log("Acclerometer test...");


spelling error

kenchris · 2016-11-03T08:02:15Z

samples/Gyroscope.js

+// Test code to use the Gyroscope (subclass of Generic Sensor) API
+// to communicate with the BMI160 inertia sensor on the Arduino 101
+// and monitor the rate of rotation around the the X, Y and Z axis
+print("Gyroscope test...");


Riju, do you have tests from Blink that we can reuse?

We have the AmbientLightSensor tests here -> https://cs.chromium.org/chromium/src/third_party/WebKit/LayoutTests/sensor/ambient-light-sensor.html?q=ambient&sq=package:chromium&l=1

Gyro and Accelerometer test cases will be upstreamed along with their implementation soon.

riju · 2016-11-03T11:31:59Z

samples/Gyroscope.js

+
+sensor.onchange = function(event) {
+    print("rotation (rad/s): " +
+          " x=" + event.reading.rotationRateX +


rotationRateX is now x, changed recently
https://w3c.github.io/gyroscope/#gyroscopereading

I'll update accordingly.

riju · 2016-11-03T12:02:54Z

src/zjs_sensor.c

+    jerry_value_t z_val = jerry_create_number(z);
+    jerry_value_t reading_obj = jerry_create_object();
+    if (channel == SENSOR_CHAN_ACCEL_ANY) {
+        zjs_set_property(reading_obj, "accelerationX", x_val);


accelerationX -> x, changed recently.
https://w3c.github.io/accelerometer/#accelerometerreading

riju · 2016-11-03T12:03:08Z

src/zjs_sensor.c

+        zjs_set_property(reading_obj, "accelerationY", y_val);
+        zjs_set_property(reading_obj, "accelerationZ", z_val);
+    } else if (channel == SENSOR_CHAN_GYRO_ANY) {
+        zjs_set_property(reading_obj, "rotationRateX", x_val);


riju · 2016-11-03T12:04:56Z

samples/Accelerometer.js

+
+sensor.onchange = function(event) {
+    console.log("acceleration (m/s^2): " +
+          " x=" + event.reading.accelerationX +


accelerationX -> x , changed recently
https://w3c.github.io/accelerometer/#accelerometerreading

riju · 2016-11-03T12:17:06Z

samples/Gyroscope.js

+};
+
+sensor.onerrorchange = function(error) {
+    console.log("error: " + error);


nit : sensor.onerror = event => console.log(event.error.name, event.error.message);

This is a limitation of the jerryscript engine, it doesn't support this syntax, it's ES6 right? It only supports ES5 for now.

good to know that. iirc, its better to use event, instead of error object. not sure, right now.

sensor.onerror = function(event) { console.log(event.error.name, event.error.message);

riju · 2016-11-03T13:02:57Z

samples/Accelerometer.js

+console.log("Acclerometer test...");
+
+
+var sensor = new AccelerometerSensor({


AccelerometerSensor -> Accelerometer
Just a heads up, very soon, we will split the Accelerometer interface to 2 interfaces, one for Accelerometer and another for LinearAccelerometer. In that case includeGravity will not be needed. You may want to change this when it is reflected in the spec.

Ok, I'll keep that in mind, thanks for the heads up.

riju · 2016-11-03T13:08:27Z

samples/Accelerometer.js

+    console.log("state: " + event);
+};
+
+sensor.onerrorchange = function(error) {


sensor.onerror = event => console.log(event.error.name, event.error.message);

See my comment above.

riju · 2016-11-03T13:34:13Z

src/zjs_ipm.h

+#define TYPE_SENSOR_START                                  0x0031
+#define TYPE_SENSOR_STOP                                   0x0032
+#define TYPE_SENSOR_EVENT_STATE_CHANGE                     0x0033
+#define TYPE_SENSOR_EVENT_VALUE_CHANGE                     0x0034


nit : TYPE_SENSOR_EVENT_READING_CHANGE ? we have this notion of SensorReading.

that's fine with me

riju · 2016-11-03T13:43:43Z

src/zjs_ipm.h

+        struct sensor_data {
+            enum sensor_channel channel;
+            uint32_t frequency;
+            double value[3];


Would you prefer it to split this struct into 2 ? Like we have for SensorReading (frequency&timestamp) as base struct, and say concreteSensor's reading (which contains the actual value) like https://w3c.github.io/gyroscope/#gyroscopereading.
Also no timestamp ? like performance.now() equivalent .

When you implement the next sensor, say AmbientLightSensor/Proximity , there is only 1 double for "value".
If you implement Uncalibrated Magnetometer or Uncalibrated Gyro, then double value[6].

ok yeah, this is the internal implementation, and only specific to Arduino 101 where we have to channel the data, so we can fix it up however we want, but that's a good idea.

@riju I updated patch to use a union to hold the reading values depending on the sensor type

riju · 2016-11-03T13:52:49Z

src/zjs_sensor.c

+    int32_t id;
+    enum sensor_channel channel;
+    enum sensor_state state;
+    double sensor_value[3];


Fine for now, but not generic enough. See 294d36b#r86344326

grgustaf

I've gone through 2/3, I'll give you this much so far. :)

grgustaf · 2016-11-04T21:32:02Z

arc/prj.conf.base

+CONFIG_BMI160_NAME="bmi160"
+CONFIG_BMI160_SPI_PORT_NAME="SPI_1"
+CONFIG_BMI160_SLAVE=1
+CONFIG_BMI160_SPI_BUS_FREQ=88


@jimmy-huang, @jprestwo: This reminds me, we should do something like analyze.sh for arc too. However, maybe we should have two things we can do:

build arc generically with all features enabled, so you don't have to keep updating, and

build arc optimally for a particular application

We could have a make arc-generic as opposed to make arc perhaps. Or maybe make arc continues to be generic, but make all does the optimized arc version, that's kinda cool.

If ARC is completely unused by an app, we could even disable ARC entirely. I wonder if that saves power?

(Not requesting this in this patch, but you could look at it for a follow-on.)

grgustaf · 2016-11-08T19:09:29Z

arc/src/main.c

@@ -388,6 +388,250 @@ static void handle_glcd(struct zjs_ipm_message* msg)
    ipm_send_reply(msg);
 }

+#ifndef DEBUG_BUILD


I think you meant #ifdef? This is used only in DEBUG_BUILD below AFAICT. Bad Jimmy, didn't build with DEBUG! :)

grgustaf · 2016-11-08T19:10:08Z

arc/src/main.c

+    zjs_ipm_send(MSG_ID_SENSOR, &msg);
+}
+
+#define ABS(x) ((x) > 0) ? (x) : -(x)


>= perhaps?

ok, i'll change it

grgustaf · 2016-11-08T19:16:50Z

arc/src/main.c

+            result = (double)val2 * (-0.000001);
+        } else {
+            result = val1 + (double)val2 * (-0.000001);
+        }


What happens in this block if val1 > 0 and val2 < 0, doesn't it do the wrong thing?

It kind of seems like this logic could be simplified to be more obviously correct. I'm still not sure what it's doing but will wait for your response to this before digging deeper. :)

if val1 > 0, then it falls into the first case, and if val1 > 0, then val2 can't be < 0 as the sign bit is in val1. But anyway, this is taken from the the zephyr sample how they checked.

grgustaf · 2016-11-08T19:18:36Z

arc/src/main.c

+    dval[2] = convert_sensor_value(&val[2]);
+
+    if (dval[0] == 0 && dval[1] == 0 && dval[2] == 0) {
+        // FIXME: BUG? why sometimes it reports 0, 0, 0 on all axis


axis -> axes. Did you report that to Zephyr folks? Maybe open a JIRA?

I think this will be fixed with the trigger, that's why I wasn't looking into this.

grgustaf · 2016-11-08T19:22:59Z

arc/src/main.c

+
+    if (ABS(dval[0] - gyro_last_value[0]) > 0 ||
+        ABS(dval[1] - gyro_last_value[1]) > 0 ||
+        ABS(dval[2] - gyro_last_value[2]) > 0) {


If your threshold for difference is 0, then isn't this the same as dval[0] != gyro_last_value[0]?

yeah, i was going to add a threshold here intead of 0, but I don't really know what value to put in for now, so i left it as 0. I'll update it.

grgustaf · 2016-11-08T19:53:13Z

src/zjs_ipm.h

+#endif
+#ifdef BUILD_MODULE_SENSOR
+#define MSG_ID_SENSOR                                      0x04
+#endif


I think you can go ahead and define all of these all the time, without the ifdefs. And actually I think elsewhere you use them without being in an ifdef so that would break the build anyway. Maybe review your usage of MSG_ID_* everywhere to make sure it makes sense.

ok, i'll revert them.

grgustaf · 2016-11-08T19:55:10Z

src/zjs_ipm.h

 #define TYPE_AIO_OPEN                                      0x0000
 #define TYPE_AIO_PIN_READ                                  0x0001
 #define TYPE_AIO_PIN_ABORT                                 0x0002
 #define TYPE_AIO_PIN_CLOSE                                 0x0003
 #define TYPE_AIO_PIN_SUBSCRIBE                             0x0004
 #define TYPE_AIO_PIN_UNSUBSCRIBE                           0x0005
 #define TYPE_AIO_PIN_EVENT_VALUE_CHANGE                    0x0006
+#endif


Defining macros like this that are never used (if the corresponding module isn't built) doesn't really hurt anything, so I doubt we need these #ifdefs?

grgustaf · 2016-11-08T19:56:21Z

src/zjs_ipm.h

@@ -69,13 +96,14 @@ typedef struct zjs_ipm_message {
    uint32_t error_code;

    union {
-        // AIO
+#ifdef BUILD_MODULE_AIO


Even defining structs that don't get used shouldn't really hurt anything. A few microseconds faster built time but more clutter in the code.

grgustaf · 2016-11-08T19:57:47Z

src/zjs_sensor.c

+    SENSOR_STATE_IDLE,
+    SENSOR_STATE_ACTIVATING,
+    SENSOR_STATE_ACTIVATED,
+    SENSOR_STATE_ERRORED,


Is this non-word Errored something you're inheriting from elsewhere? :)

grgustaf · 2016-11-08T20:08:13Z

Trying out Accelerometer.js, when my board is sitting flat it reports something like:
acceleration (m/s^2): x=-0.92958 y=0.25773799999999997 z=11.353628

We would expect x=0, y=0, z=9.81; so y is understandable but x and z seem surprisingly off I think?

grgustaf · 2016-11-08T20:09:04Z

It would be very nice if the output of the sample could be printed with a fixed width, so the y and z values line up as they're spewing by, then you could watch them go up and down without having to pause the stream.

grgustaf · 2016-11-08T21:26:47Z

I can confirm the calibration made the flat on desk case more accurate. If I tip it up on short end or long end it shows x=-8.3 though or y=-8.3, where i'd expect -9.81. But flipped the opposite way where gravity is positive they are around 9.8. Not sure if it can get better, especially without a dynamic user involved calibration?

grgustaf · 2016-11-08T20:58:41Z

src/zjs_sensor.c

+} sensor_handle_t;
+
+static sensor_handle_t *accel_handle = NULL;
+static sensor_handle_t *gyro_handle = NULL;


Maybe plurals would be better here to remind us these are lists?

grgustaf · 2016-11-08T21:06:17Z

src/zjs_sensor.c

+static void zjs_sensor_signal_callbacks(sensor_handle_t *handle,
+                                        union sensor_reading reading)
+{
+    if (!handle)


This isn't needed because your loop will be skipped if it's NULL.

grgustaf · 2016-11-08T23:55:51Z

Jimmy and I found that the auto calibration and error handling commits (at least) segfault if you try to run AIO.js. If you run the BTGrove demo, it does the same once you start to connect.

riju · 2016-11-09T13:00:48Z

samples/Accelerometer.js

+    frequency: 50
+});
+
+sensor.onchange = function(event) {


@jimmy-huang : Some changes were made for the Sensor Reading
Now

accleromterSensor.onchange = function () { console.log("acceleration (m/s^2): " + " x=" + accleromterSensor.reading.x + " y=" + accleromterSensor.reading.y + " z=" + accleromterSensor.reading.z); }

just a heads up, you can make this changes in subsequent PR also, upto you.

riju · 2016-11-09T13:01:12Z

samples/Accelerometer.js

+                " z=" + event.reading.z);
+};
+
+sensor.onstatechange = function(event) {


onstatechange is also removed

riju · 2016-11-09T13:02:04Z

samples/Gyroscope.js

+
+sensor.onchange = function(event) {
+    console.log("rotation (rad/s): " +
+                " x=" + event.reading.x +


Please see the comment above for accelerometer

jimmy-huang · 2016-11-11T23:21:32Z

this needs to be rebased once #415 is merged

Initial implementation for the W3C Generic Sensor API and add support for the Accelerometer and Gyroscope sensors using the BMI160 inertia sensor built-in on the Arduino 101, the current implementation is polling from the ARC and channeling the data back to X86. Also adding two sample JS code testing both APIs. Signed-off-by: Jimmy Huang <[email protected]>

Signed-off-by: Jimmy Huang <[email protected]>

Also fixed a bug where we still report change events after error is reported Signed-off-by: Jimmy Huang <[email protected]>

Signed-off-by: Jimmy Huang <[email protected]>

jimmy-huang · 2016-11-12T00:26:09Z

@grgustaf, updated pr and rebased, please review

kenchris reviewed Nov 3, 2016

View reviewed changes

riju reviewed Nov 3, 2016

View reviewed changes

haoxli mentioned this pull request Nov 7, 2016

W3C Sensors API #372

Closed

jimmy-huang force-pushed the sensor-api branch 3 times, most recently from e200030 to 968b8ac Compare November 7, 2016 23:04

grgustaf suggested changes Nov 8, 2016

View reviewed changes

jimmy-huang force-pushed the sensor-api branch from 845ee18 to a603fdd Compare November 8, 2016 22:53

jimmy-huang force-pushed the sensor-api branch from 9815f4c to 16d55f4 Compare November 9, 2016 01:06

riju reviewed Nov 9, 2016

View reviewed changes

samples/Accelerometer.js

" z=" + event.reading.z);

};

sensor.onstatechange = function(event) {

Copy link

riju Nov 9, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

onstatechange is also removed

riju reviewed Nov 9, 2016

View reviewed changes

jimmy-huang added needs research needs rebase labels Nov 9, 2016

jimmy-huang force-pushed the sensor-api branch from 476fa80 to 817e2d1 Compare November 11, 2016 23:20

jimmy-huang removed the needs research label Nov 11, 2016

jimmy-huang added 9 commits November 11, 2016 16:20

Fixed typos and updating API according to latest spec changes

36efdc8

Signed-off-by: Jimmy Huang <[email protected]>

Use a union to hold readings instead of array

7c3f41a

Signed-off-by: Jimmy Huang <[email protected]>

Fixed error handling based on feedback

e252b6d

Also fixed a bug where we still report change events after error is reported Signed-off-by: Jimmy Huang <[email protected]>

Added auto calibration

501ab09

Signed-off-by: Jimmy Huang <[email protected]>

Updating PR based on Geoff's comments

67f7297

Signed-off-by: Jimmy Huang <[email protected]>

Rebased against master with ring buffer patch

abf1009

Signed-off-by: Jimmy Huang <[email protected]>

Fixed build fail on travis

ff8154b

Signed-off-by: Jimmy Huang <[email protected]>

Rebase against master

5b7ef1a

Signed-off-by: Jimmy Huang <[email protected]>

jimmy-huang force-pushed the sensor-api branch from 817e2d1 to 5b7ef1a Compare November 12, 2016 00:21

jimmy-huang removed the needs rebase label Nov 12, 2016

grgustaf approved these changes Nov 14, 2016

View reviewed changes

grgustaf merged commit f9791d9 into intel:master Nov 14, 2016

		console.log("Acclerometer test...");


		var sensor = new AccelerometerSensor({

[Sensor] Initial implementation of the Generic Sensor API #353

[Sensor] Initial implementation of the Generic Sensor API #353

Conversation

jimmy-huang commented Nov 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riju Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riju Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

riju Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgustaf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgustaf commented Nov 8, 2016

grgustaf commented Nov 8, 2016

grgustaf commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grgustaf commented Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimmy-huang commented Nov 11, 2016

jimmy-huang commented Nov 12, 2016

riju Nov 3, 2016 •

edited

Loading

riju Nov 3, 2016 •

edited

Loading

riju Nov 3, 2016 •

edited

Loading

grgustaf commented Nov 8, 2016 •

edited

Loading