Skip to content
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
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
package com.google.android.things.contrib.driver.apa102;

import android.graphics.Color;
import android.support.annotation.Nullable;
import android.support.annotation.VisibleForTesting;

import com.google.android.things.pio.PeripheralManagerService;
Expand Down Expand Up @@ -67,7 +68,8 @@ public enum Direction {
private Mode mLedMode;

// RGB LED strip settings that have sensible defaults.
private int mLedBrightness = MAX_BRIGHTNESS >> 1; // default to half
private int mLedBrightnessGlobal = MAX_BRIGHTNESS >> 1; // Default to half
private int[] mLedBrightness;

// Direction of the led strip;
private Direction mDirection;
Expand Down Expand Up @@ -162,13 +164,33 @@ public void setBrightness(int ledBrightness) {
throw new IllegalArgumentException("Brightness needs to be between 0 and "
+ MAX_BRIGHTNESS);
}
mLedBrightness = ledBrightness;
mLedBrightnessGlobal = ledBrightness;
mLedBrightness = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

why do you make it null? (issues later that you have to null check) Can you just make an empty array / clear it?

Copy link
Author

Choose a reason for hiding this comment

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

I make it null to invalidate it, so when the user calls write later, the global value is used instead. This is to allow switching from individual brightness to global brightness.
I am open to do it in any other way, I just thought this one was the simpler one.

Copy link
Contributor

Choose a reason for hiding this comment

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

you could fill the array with the global brightness instead of relying on null to say that there is no individual brightness (Connascence of Values)

Copy link
Author

Choose a reason for hiding this comment

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

but the original request for change suggested by jdkoren was to keep them separated.

Copy link
Contributor

Choose a reason for hiding this comment

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

It's not my repo, so it was just a suggestion 👍 I'd go with the mod's advice ;-)

}

/**
* Get the current brightness level
* Sets the brightness for all LEDs in the strip.
* @param ledBrightness The brightness of the LED strip, between 0 and {@link #MAX_BRIGHTNESS}.
*/
public void setIndividualBrightness(int[] ledBrightness) {
mLedBrightness = new int[ledBrightness.length];
for (int i=0; i<ledBrightness.length; i++) {
if (ledBrightness[i] < 0 || ledBrightness[i] > MAX_BRIGHTNESS) {
throw new IllegalArgumentException("Brightness needs to be between 0 and "
+ MAX_BRIGHTNESS);
}
mLedBrightness[i] = ledBrightness[i];
}
}

/**
* Get the current brightness
*/
public int getBrightness() {
return mLedBrightnessGlobal;
}

public @Nullable int[] getIndividualBrightness() {
return mLedBrightness;
}

Expand Down Expand Up @@ -196,6 +218,9 @@ public void write(int[] colors) throws IOException {
if (mDevice == null) {
throw new IllegalStateException("SPI device not open");
}
if (mLedBrightness != null && colors.length != mLedBrightness.length) {
throw new IllegalStateException("colors and brightness arrays must be of the same length");
}

final int size = APA_START_FRAME_PACKET_LENGTH
+ APA_COLOR_PACKET_LENGTH * colors.length
Expand All @@ -212,9 +237,12 @@ public void write(int[] colors) throws IOException {
pos += APA_START_FRAME_PACKET_LENGTH;

// Compute the packets to send.
byte brightness = (byte) (0xE0 | mLedBrightness); // Less brightness possible
final Direction currentDirection = mDirection; // Avoids reading changes of mDirection during loop
byte brightness = (byte) (0xE0 | mLedBrightnessGlobal); // Default initialization
for (int i = 0; i < colors.length; i++) {
if (mLedBrightness != null) {
brightness = (byte) (0xE0 | mLedBrightness[i]); // Less brightness possible
}
int di = currentDirection == Direction.NORMAL ? i : colors.length - i - 1;
copyApaColorData(brightness, colors[di], mLedMode, mLedData, pos);
pos += APA_COLOR_PACKET_LENGTH;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,20 @@ public void setBrightness_throwsIfTooLarge() throws IOException {
leds.setBrightness(Apa102.MAX_BRIGHTNESS + 1);
}

@Test
public void setBrightnessArray_throwsIfTooSmall() throws IOException {
Apa102 leds = new Apa102(mSpiDevice, Apa102.Mode.BGR, Apa102.Direction.NORMAL);
mExpectedException.expect(IllegalArgumentException.class);
leds.setIndividualBrightness(new int[] {-1});
}

@Test
public void setBrightnessArray_throwsIfTooLarge() throws IOException {
Apa102 leds = new Apa102(mSpiDevice, Apa102.Mode.BGR, Apa102.Direction.NORMAL);
mExpectedException.expect(IllegalArgumentException.class);
leds.setIndividualBrightness(new int[] {Apa102.MAX_BRIGHTNESS + 1});
}

@Test
public void setDirection() throws IOException {
Apa102 leds = new Apa102(mSpiDevice, Apa102.Mode.BGR, Apa102.Direction.NORMAL);
Expand Down