Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a bit more support for the SenseHat. #101

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion sensehat/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd rephrase this list:

  • 8x8 LED matrix
  • LPS25H
    ...
  • TODO: Other sensors



NOTE: these drivers are not production-ready. They are offered as sample
Expand Down Expand Up @@ -54,6 +54,19 @@ display.draw(bitmap);
// Close the display when done.
display.close();
```
```
Copy link
Contributor

Choose a reason for hiding this comment

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

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);
}

// just a snippet that verifies the device is working
Copy link
Contributor

Choose a reason for hiding this comment

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

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

// 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);
}
```

License
-------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import android.support.test.InstrumentationRegistry;
import android.support.test.filters.SmallTest;
import android.support.test.runner.AndroidJUnit4;
import android.util.Log;

import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -38,34 +39,38 @@ public class SenseHatDeviceTest {
@Test
public void senseHat_DisplayColor() throws IOException {
// Color the LED matrix.
LedMatrix display = SenseHat.openDisplay();

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

display.draw(Color.MAGENTA);
}
}

@Test
public void senseHat_DisplayDrawable() throws IOException {
Context context = InstrumentationRegistry.getTargetContext();
// Display a drawable on the LED matrix.
LedMatrix display = SenseHat.openDisplay();
display.draw(context.getDrawable(android.R.drawable.ic_secure));
// Close the display when done.
display.close();
try (LedMatrix display = SenseHat.openDisplay()) {
display.draw(context.getDrawable(android.R.drawable.ic_secure));
}
}

@Test
public void senseHat_DisplayGradient() throws IOException {
// Display a gradient on the LED matrix.
LedMatrix display = SenseHat.openDisplay();
Bitmap bitmap = Bitmap.createBitmap(SenseHat.DISPLAY_WIDTH, SenseHat.DISPLAY_HEIGHT, Bitmap.Config.ARGB_8888);
Canvas canvas = new Canvas(bitmap);
Paint paint = new Paint();
paint.setShader(new RadialGradient(4, 4, 4, Color.RED, Color.BLUE, Shader.TileMode.CLAMP));
canvas.drawRect(0, 0, canvas.getWidth(), canvas.getHeight(), paint);
display.draw(bitmap);
// Close the display when done.
display.close();
try (LedMatrix display = SenseHat.openDisplay()) {
Bitmap bitmap = Bitmap.createBitmap(SenseHat.DISPLAY_WIDTH, SenseHat.DISPLAY_HEIGHT, Bitmap.Config.ARGB_8888);
Canvas canvas = new Canvas(bitmap);
Paint paint = new Paint();
paint.setShader(new RadialGradient(4, 4, 4, Color.RED, Color.BLUE, Shader.TileMode.CLAMP));
canvas.drawRect(0, 0, canvas.getWidth(), canvas.getHeight(), paint);
display.draw(bitmap);
}
}

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

Choose a reason for hiding this comment

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

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

Log.i(BaroTemp.class.getName(),"SenseHat Barotemp raw temperature: "+baroTemp.getTemperatureRaw());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,190 @@
package com.google.android.things.contrib.driver.sensehat;

import com.google.android.things.pio.I2cDevice;
import com.google.android.things.pio.PeripheralManager;

import java.io.IOException;

/**
* This class allows access to the LPS25H on the SenseHat.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

* <p>
* 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 {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

private I2cDevice mDevice;
// Register addresses
private final int LPS_REF_P_XL = 0x08; // Reference pressure, lowest byte
private final int LPS_REF_P_L = 0x09; // Reference pressure, low byte
private final int LPS_REF_P_H = 0x0A; // Reference pressure, high byte

private final int LPS_WHO_AM_I = 0x0F; // Returns 0xbd (read only)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure what this value is supposed to represent?


private final int LPS_RES_CONF = 0x10; // Set pressure and temperature resolution

private final int LPS_CTRL_REG1 = 0x20; // Set device power mode / ODR / BDU
private final int LPS_CTRL_REG2 = 0x21; // FIFO / I2C configuration
private final int LPS_CTRL_REG3 = 0x22; // Interrupt configuration
private final int LPS_CTRL_REG4 = 0x23; // Interrupt configuration

private final int LPS_INTERRUPT_CFG = 0x24;// Interrupt configuration
private final int LPS_INT_SOURCE = 0x25; // Interrupt source configuration

private final int LPS_STATUS_REG = 0x27; // Status (new pressure/temperature data available)

private final int LPS_PRESS_OUT_XL = 0x28; // Pressure output, loweste byte
private final int LPS_PRESS_OUT_L = 0x29; // Pressure output, low byte
private final int LPS_PRESS_OUT_H = 0x2A; // Pressure output, high byte

private final int LPS_TEMP_OUT_L = 0x2B; // Temperature output, low byte
private final int LPS_TEMP_OUT_H = 0x2C; // Temperature output, high byte

private final int LPS_FIFO_CTRL = 0x2E; // FIFO control / mode selection
private final int LPS_FIFO_STATUS = 0x2F; // FIFO status

private final int LPS_THS_P_L = 0x30; // Pressure interrupt threshold, low byte
private final int LPS_THS_P_H = 0x31; // Pressure interrupt threshold, high byte

// 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 mMillibarAdjust = 0;

/**
* Create a new barometric pressure and temperature sensor driver connected on the given I2C bus.
*
* @param bus I2C bus the sensor is connected to.
* @throws IOException when a lower level does
*/
public BaroTemp(String bus) throws IOException {
PeripheralManager pioService = PeripheralManager.getInstance();
mDevice = pioService.openI2cDevice(bus, SenseHat.I2C_LPS25H_ADDRESS);
// power down first
mDevice.writeRegByte(LPS_CTRL_REG1, (byte) 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we assume the device is in a powered-down state?

// power up, data rate 12.5Hz (10110000)
mDevice.writeRegByte(LPS_CTRL_REG1, (byte) 0xb0);
if (0xBD != (mDevice.readRegByte(LPS_WHO_AM_I) & 0xFF)) {
throw new IOException("This does not seem to be a LPS25H");
}
}

@Override
public void close() throws IOException {
if (mDevice != null) {
try {
// power down device
mDevice.writeRegByte(LPS_CTRL_REG1, (byte) 0);
} catch (Exception any) {
// we tried
}
try {
mDevice.close();
} finally {
mDevice = null;
}
}
}

private int readSigned24(int a0, int a1, int a2) throws IOException {
int ret = (mDevice.readRegByte(a0) & 0xFF);
ret += ((int) mDevice.readRegByte(a1) & 0xFF) << 8;
ret += ((int) mDevice.readRegByte(a2) & 0xFF) << 16;
if (ret < 8388608) return ret;
Copy link
Contributor

Choose a reason for hiding this comment

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

Google's style guide states that conditionals should be wrapped in curly braces, even if it's one line,

ie.

if (ret < 8388608) {
    return ret;
}

else return ret - 16777216;
}

private int readSigned16(int a0, int a1) throws IOException {
int ret = (mDevice.readRegByte(a0) & 0xFF);
ret += (mDevice.readRegByte(a1) & 0xFF) << 8;
if (ret < 32768) return ret;
else return ret - 65536;
}

/**
* 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. In the author's case the difference was 6.2 hPa which is quite significant. This
* error was confirmed with another (unrelated) sensor.
*
* @param hPa difference to actual air pressure in hectoPascal (hPa) or millibar.
* @throws IOException from I2cDevice
*/
public void setBarometerOffset(double hPa) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

this.mMillibarAdjust = (int) Math.round(hPa * 4096);
Copy link
Contributor

Choose a reason for hiding this comment

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

The use of this is not necessary as the m prefix to the variable name implies it is part of the class.

}

/**
* Fetch raw value, see the data sheet. <p>Note that this call waits for data to be available.
* From the data sheet the (selected) refresh rate is 12.5 Hz so the max wait could be
* 1000/12.5 = 80 milliseconds with an average of 40 milliseconds. Call from asynchronous code
* if this is an issue. If your code calls this method less frequently then 12.5 times per
* second there will be no wait.</p>
*
* @return The raw sensor value, adjusted by the given offset (if any).
* @throws IOException from I2cDevice
*/
public int getBarometerRaw() throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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?

} catch (InterruptedException e) {
throw new IOException(e);
}
}
return readSigned24(LPS_PRESS_OUT_XL, LPS_PRESS_OUT_L, LPS_PRESS_OUT_H) + mMillibarAdjust;
}

/**
* Fetch raw value, see the data sheet. <p>Note that this call waits for data to be available.
* From the data sheet the (selected) refresh rate is 12.5 Hz so the max wait could be
* 1000/12.5 = 80 milliseconds with an average of 40 milliseconds. Call from asynchronous code
* if this is an issue. If your code calls this method less frequently then 12.5 times per
* second there will be no wait.</p>
*
* @return The raw sensor value.
* @throws IOException from I2cDevice
*/
public int getTemperatureRaw() throws IOException {
// wait for data available
while (0 == (mDevice.readRegByte(LPS_STATUS_REG) & 1)) {
try {
Thread.sleep(1);
} catch (InterruptedException e) {
throw new IOException(e);
}
}
return readSigned16(LPS_TEMP_OUT_L, LPS_TEMP_OUT_H);
}

/**
* Fetch air pressure in hPa (millibar). <p>Note that this call waits for data to be available.
* From the data sheet the (selected) refresh rate is 12.5 Hz so the max wait could be
* 1000/12.5 = 80 milliseconds with an average of 40 milliseconds. Call from asynchronous code
* if this is an issue. If your code calls this method less frequently then 12.5 times per
* second there will be no wait.</p>
*
* @return The current air pressure in hPa(millibar), adjusted by the given offset (if any).
* @throws IOException from I2cDevice
*/
public double getBarometer() throws IOException {
return getBarometerRaw() / 4096.0;
}

/**
* Fetch the temperature in degrees Celcius. Note that the design of the SenseHat makes this
* more the temperature of the board then the actual (room) temperature!
* <p>Also note that this call waits for data to be available.
* From the data sheet the (selected) refresh rate is 12.5 Hz so the max wait could be
* 1000/12.5 = 80 milliseconds with an average of 40 milliseconds. Call from asynchronous code
* if this is an issue. If your code calls this method less frequently then 12.5 times per
* second there will be no wait.</p>
*
* @return The temperature as reported by the sensor in degrees Celcius.
* @throws IOException from I2cDevice
*/
public double getTemperature() throws IOException {
return 42.5 + getTemperatureRaw() / 480.0;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ public class LedMatrix implements AutoCloseable {
*/
public LedMatrix(String bus) throws IOException {
PeripheralManager pioService = PeripheralManager.getInstance();
mDevice = pioService.openI2cDevice(bus, SenseHat.I2C_ADDRESS);
mDevice = pioService.openI2cDevice(bus, SenseHat.I2C_DISPLAY_ADDRESS);
}

/* package */ LedMatrix(I2cDevice device) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

public static final int I2C_LPS25H_ADDRESS = 0x5C;
public static final String BUS_NAME = "I2C1";
public static final int DISPLAY_WIDTH = LedMatrix.WIDTH;
public static final int DISPLAY_HEIGHT = LedMatrix.HEIGHT;

/**
* Connect to the LedMatrix of the SenseHat on a Raspberry Pi 3.
* @return The autoclosing LedMatrix object.
* @throws IOException If caused by the hardware.
*/
public static LedMatrix openDisplay() throws IOException {
return new LedMatrix(BUS_DISPLAY);
return new LedMatrix(BUS_NAME);
}

/**
* Connect to the LPS25H barometer/temperature of the SenseHat sensor on a Raspberry Pi 3.
* @return The autoclosing BaroTemp object.
* @throws IOException If caused by the hardware.
*/
public static BaroTemp openBaroTemp() throws IOException {
return new BaroTemp(BUS_NAME);
}

/**
* Connect to the LedMatrix of the SenseHat.
* @param busName The name of the I2C bus device on a non-raspi3 board.
* @return The autoclosing LedMatrix object.
* @throws IOException If caused by the hardware.
*/
public static LedMatrix openDisplay(final String busName) throws IOException {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do these need to be final?

return new LedMatrix(busName);
}

/**
* Connect to the LPS25H barometer/temperature of the SenseHat sensor.
* @param busName The name of the I2C bus device on a non-raspi3 board.
* @return The autoclosing BaroTemp object.
* @throws IOException If caused by the hardware.
*/
public static BaroTemp openBaroTemp(final String busName) throws IOException {
return new BaroTemp(busName);
}
}