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 5 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
13 changes: 12 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,17 @@ 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);
}

try (BaroTemp bt = SenseHat.openBaroTemp()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed. And thanks for the awesome code review!

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,170 @@
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 milliBarAdjust = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, code style for fields are m<Name>

Copy link
Author

Choose a reason for hiding this comment

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

Ah, ok.


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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

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

Choose a reason for hiding this comment

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

Can you mention the units?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

* @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.milliBarAdjust = (int) Math.round(hPa * 4096);
}

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

Choose a reason for hiding this comment

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

Can you link to the datasheet in this comment?

Copy link
Author

Choose a reason for hiding this comment

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

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

*
* @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) + milliBarAdjust;
}

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

Choose a reason for hiding this comment

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

Can you link to the datasheet in this comment?

*
* @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 (milli bar). Note that this call waits for data to be available.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe it's just millibar, without a space.

Copy link
Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Contributor

Choose a reason for hiding this comment

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

How long do we expect to wait? Does it make sense to make this asynchronous?

Copy link
Author

Choose a reason for hiding this comment

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

Fixed in description. Usually the wait will be just the overhead for fetching the status register as it makes little sense for an application to poll the device more than 12.5 times a second so data should always be available. The JavaDoc now explains this.

*
* @return The current air pressure, 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!
*
* @return The temperature as reported by the sensor.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you specify the units here as well?

Copy link
Author

Choose a reason for hiding this comment

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

Done

* @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,17 @@
*/
@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 = 0x46;
Copy link
Contributor

Choose a reason for hiding this comment

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

They're the same address?

Copy link
Author

Choose a reason for hiding this comment

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

Good catch! Fixed!

public static final String BUS_NAME = "I2C1";
Copy link
Contributor

Choose a reason for hiding this comment

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

We should make this bus name a parameter that can be provided, for a variety of devices with different bus names.

Copy link
Author

Choose a reason for hiding this comment

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

Yes we should! However, this is from the original code and I daresay a common issue through the other libraries as well. Maybe there should be a global device data object to address this?
Meanwhile I've provided overloaded factory methods.

public static final int DISPLAY_WIDTH = LedMatrix.WIDTH;
public static final int DISPLAY_HEIGHT = LedMatrix.HEIGHT;

public static LedMatrix openDisplay() 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.

Can you add Javadoc for this method as well?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

return new LedMatrix(BUS_DISPLAY);
return new LedMatrix(BUS_NAME);
}

public static BaroTemp openBaroTemp() 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.

Can you add JavaDoc for this method?

Copy link
Author

Choose a reason for hiding this comment

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

Done.

return new BaroTemp(BUS_NAME);
}
}