Added a bit more support for the SenseHat. #101

Walter-Stroebel · 2018-05-04T19:12:57Z

Added support for the LPS25H on the SenseHat.

Now to find the I2C details.

googlebot · 2018-05-04T19:13:01Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here (e.g. I signed it!) and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

Walter-Stroebel · 2018-05-04T19:15:12Z

I signed it!

googlebot · 2018-05-04T19:15:16Z

CLAs look good, thanks!

Added usage for new BaroTemp sensor class.

proppy · 2018-05-04T22:41:29Z

Related #24 #86

Fleker

Thanks for submitting this pull request. I have a few comments about it, to better match our code style in other drivers.

Fleker · 2018-05-05T00:40:42Z

sensehat/README.md

@@ -54,6 +54,17 @@ display.draw(bitmap);
 // Close the display when done.
 display.close();
 ```
+```
+                try (BaroTemp bt = SenseHat.openBaroTemp()) {


I think you're indenting too much here.
Your snippet also lacks a comment of what it does.

Fixed. And thanks for the awesome code review!

Fleker · 2018-05-05T00:42:24Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+ * See also: https://www.pololu.com/file/download/LPS25H.pdf?file_id=0J761</p>
+ * <p>Source code referenced: https://github.com/tkurbad/mipSIE/blob/master/python/AltIMU-10v5/i2c.py</p>
+ */
+public class BaroTemp implements AutoCloseable {


Generally our class names closely reflect the actual name of the part, this being the LPS25H. Then the comments in the class would describe that it is a temperature/barometric pressure sensor.

I followed the style of the original code for the LedMatrix. I'm willing to follow this suggestion in a later phase but atm time constraints prohibit me to do so now.

Fleker · 2018-05-05T00:43:49Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+    // The next two registers need special soldering
+    private final int LPS_RPDS_L = 0x39;//  Pressure offset for differential pressure computing, low byte
+    private final int LPS_RPDS_H = 0x3A; //  Differential offset, high byte
+    private int milliBarAdjust = 0;


Nit, code style for fields are m<Name>

Fleker · 2018-05-05T00:44:04Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+     * The sensor seems to have an offset to the actual pressure. You can find your local "real" pressure quite easily on the web. Get the measured value from the
+     * sensor and compute the difference. The value obtained can be passed to this method to "calibrate" your board's sensor.
+     *
+     * @param hPa difference to actual air pressure


Can you mention the units?

Fleker · 2018-05-05T00:44:29Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+    }
+
+    /**
+     * The sensor seems to have an offset to the actual pressure. You can find your local "real" pressure quite easily on the web. Get the measured value from the


Code style is that the length of each line be no more than 100 chars. Can you review your line lengths?

Fleker · 2018-05-05T00:47:11Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/SenseHat.java

-    public static final String BUS_DISPLAY = "I2C1";
+    public static final int I2C_DISPLAY_ADDRESS =  0x46;
+    public static final int I2C_LPS25H_ADDRESS =  0x46;
+    public static final String BUS_NAME = "I2C1";
    public static final int DISPLAY_WIDTH = LedMatrix.WIDTH;
    public static final int DISPLAY_HEIGHT = LedMatrix.HEIGHT;

    public static LedMatrix openDisplay() throws IOException {


Can you add Javadoc for this method as well?

Fleker · 2018-05-05T00:47:32Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+    }
+
+    /**
+     * Fetch raw value, see the data sheet. Note that this call waits for data to be available.


Can you link to the datasheet in this comment?

There is a global reference to the datasheet for the entire class.

Fleker · 2018-05-05T00:47:37Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+    }
+
+    /**
+     * Fetch raw value, see the data sheet. Note that this call waits for data to be available.


Can you link to the datasheet in this comment?

Fleker · 2018-05-05T00:48:48Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+import java.io.IOException;
+
+/**
+ * This class allows access to the LPS25H on the SenseHat.


Generally with our sensors we have two classes. One implements the raw protocol, Lps25h, and the other plugs into the sensor framework, ie. Lps25hSensorDriver.

One example is in our BMX280

I followed the style of the original code for the LedMatrix. I'm willing to follow this suggestion in a later phase but atm time constraints prohibit me to do so now.

Fleker · 2018-05-05T00:49:39Z

...c/androidTest/java/com/google/android/things/contrib/driver/sensehat/SenseHatDeviceTest.java

-        display.draw(Color.MAGENTA);
-        // Close the display when done.
-        display.close();
+        try (LedMatrix display = SenseHat.openDisplay()) {


Should we be doing try here? If it doesn't work, I think we'd want the test to fail.

It will, there is no catch(). I changed this to use the autoclose feature which was implemented but never used / demonstrated by the original author.

So what happens if SenseHat.openDisplay() fails? Does the test still pass because it's within a try block?

MacroYau · 2018-05-05T04:48:12Z

Hey @proppy, what is the current plan for the Sense HAT drivers?

Is there anything I can help further #24 (and possibly #86 which fills the gap of my PR)?

proppy · 2018-05-05T04:50:24Z

@MacroYau Yep, sorry I never got to send an update update to your PR about the SenseHat, I'd love to make progress on this and I'm happy to help with the review.

Walter-Stroebel · 2018-05-05T05:08:42Z

@MacroYau @proppy
I'm sorry, I was not aware of #24
Unfortunately time constraints prohibit me from merging the two related PR's at the moment.
Feel free to either decline my PR, merge it with #24 or merge #24 with my PR, at your convenience.

Fleker

Hey I'm taking another look at this PR.

One thing that I don't see is an update to the gradle.properties file or CHANGELOG.md which should be updated.

Fleker · 2018-07-03T18:18:07Z

sensehat/README.md

@@ -4,7 +4,7 @@ Sense Hat driver for Android Things
 This driver provides easy access to the peripherals available on the Raspberry Pi [Sense Hat][product]:
 - 8x8 LED matrix
 - TODO: 5 buttons joystick
- TODO: Sensors
+- TODO: Sensors other then LPS25H


I'd rephrase this list:

8x8 LED matrix

LPS25H
...

TODO: Other sensors

Fleker · 2018-07-03T18:19:44Z

sensehat/README.md

@@ -54,6 +54,19 @@ display.draw(bitmap);
 // Close the display when done.
 display.close();
 ```
+```


You can add Java after the backticks to add code highlighting to the code block. Also, you won't need to add additional indenting.

// just a snippet that verifies the device is working // your actual use would of course be much more awesome try (BaroTemp bt = SenseHat.openBaroTemp()) { bt.setBarometerOffset(-6.2); Log.i(TAG, "PressureRaw: " + bt.getBarometerRaw()); Log.i(TAG, "TemperatureRaw: " + bt.getTemperatureRaw()); Log.i(TAG, "Pressure: " + bt.getBarometer()); Log.i(TAG, "Temperature: " + bt.getTemperature()); } catch (Exception e) { Log.e(TAG, "Oops", e); }

Fleker · 2018-07-03T18:20:02Z

sensehat/README.md

@@ -54,6 +54,19 @@ display.draw(bitmap);
 // Close the display when done.
 display.close();
 ```
+```
+    // just a snippet that verifies the device is working


I'm not sure if we need the comments, as the snippet is fairly descriptive.

Fleker · 2018-07-03T18:20:59Z

...c/androidTest/java/com/google/android/things/contrib/driver/sensehat/SenseHatDeviceTest.java

-        display.draw(Color.MAGENTA);
-        // Close the display when done.
-        display.close();
+        try (LedMatrix display = SenseHat.openDisplay()) {


So what happens if SenseHat.openDisplay() fails? Does the test still pass because it's within a try block?

Fleker · 2018-07-03T18:21:30Z

...c/androidTest/java/com/google/android/things/contrib/driver/sensehat/SenseHatDeviceTest.java

+    @Test
+    public void senseHat_BaroTemp() throws IOException {
+        try (BaroTemp baroTemp = SenseHat.openBaroTemp()) {
+            Log.i(BaroTemp.class.getName(),"SenseHat Barotemp raw pressure: "+baroTemp.getBarometerRaw());


Is there any way we can add assertions to this? Can we verify that the values are non-zero?

Fleker · 2018-07-03T18:27:07Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+     * @param hPa difference to actual air pressure in hectoPascal (hPa) or millibar.
+     * @throws IOException from I2cDevice
+     */
+    public void setBarometerOffset(double hPa) throws IOException {


I don't think this method does any I2C writing, so it should not need to throw IOException.

Fleker · 2018-07-03T18:28:50Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+        // wait for data available
+        while (0 == (mDevice.readRegByte(LPS_STATUS_REG) & 2)) {
+            try {
+                Thread.sleep(1);


I'm not sure if we should sleep a thread in trying to read a value. What happens if we remove it? Will the data just not be refreshed?

Fleker · 2018-07-03T18:29:54Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/BaroTemp.java

+     * @return The raw sensor value, adjusted by the given offset (if any).
+     * @throws IOException from I2cDevice
+     */
+    public int getBarometerRaw() throws IOException {


In general, do we need to make these raw readings public? It doesn't seem like they'd be very useful to have.

Fleker · 2018-07-03T18:30:41Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/SenseHat.java

@@ -23,12 +23,47 @@
 */
 @SuppressWarnings({"unused", "WeakerAccess"})
 public class SenseHat {
-    public static final int I2C_ADDRESS =  0x46;
-    public static final String BUS_DISPLAY = "I2C1";
+    public static final int I2C_DISPLAY_ADDRESS =  0x46;


We probably should not rename existing public constants as that will break the API for developers.

Fleker · 2018-07-03T18:31:04Z

sensehat/src/main/java/com/google/android/things/contrib/driver/sensehat/SenseHat.java

+     * @return The autoclosing LedMatrix object.
+     * @throws IOException If caused by the hardware.
+     */
+    public static LedMatrix openDisplay(final String busName) throws IOException {


Do these need to be final?

walter added 4 commits May 1, 2018 23:13

This should be the first step...

30185d2

Now to find the I2C details.

Changed the names of the constants to be less "selfish".

281a22a

Should work but can't test.

e7c8c23

Implemented LPS25H (tested and works).

7b94d10

Update README.md

3eb7beb

Added usage for new BaroTemp sensor class.

Fleker reviewed May 5, 2018

View reviewed changes

Fixes based on Fleker's review, with thanks!

1c64936

walter added 2 commits May 5, 2018 06:58

Fixes based on Fleker's review, with thanks!

00a6d54

Merge branch 'master' of github.com:Walter-Stroebel/contrib-drivers

140be97

Merge remote-tracking branch 'upstream/master'

2a716a9

Fleker requested changes Jul 3, 2018

View reviewed changes

Added a bit more support for the SenseHat. #101

Are you sure you want to change the base?

Added a bit more support for the SenseHat. #101

Conversation

Walter-Stroebel commented May 4, 2018

googlebot commented May 4, 2018

What to do if you already signed the CLA

Individual signers

Corporate signers

Walter-Stroebel commented May 4, 2018

googlebot commented May 4, 2018

proppy commented May 4, 2018 • edited Loading

Fleker 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

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

MacroYau commented May 5, 2018

proppy commented May 5, 2018

Walter-Stroebel commented May 5, 2018

Fleker 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

proppy commented May 4, 2018 •

edited

Loading