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.

Sign up for GitHub

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

Jump to bottom

Added individual led brightness option to led strip #73

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Added individual led brightness option to led strip #73

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb5a03d
Merge pull request #1 from androidthings/master
plattysoft Sep 18, 2017
9066f46
added invidual brightness setting per led
plattysoft Sep 18, 2017
903b118
added invidual brightness setting per led
plattysoft Sep 18, 2017
1b58fec
separating the individual and global brightness concepts
Sep 19, 2017
35b243c
updating method name in tests
Sep 19, 2017
0dc3695
fixed code style
Sep 19, 2017
96fc125
added check of array sizes
plattysoft Sep 19, 2017
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
40 changes: 35 additions & 5 deletions apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,16 @@ 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/2;
Copy link
Member

@jdkoren jdkoren Sep 18, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

nit: This was changed to division, while the ones below are still using shift. Perhaps revert this line?

private int mLedBrightness[] = new int[] {
Copy link
Member

Choose a reason for hiding this comment

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

This array has 7 elements, is it because the RainbowHAT has 7 LEDs? I don't think it's good to assume that users of this driver are also using the RainbowHAT or that they have a certain number of LEDs connected.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1
For example, the ReSpeaker 2-Mic Hat has Apa102 LEDs but only three of them.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. I was thinking only of the one on the Rainbow HAT. I'll change it to behave in the same way as the values.

MAX_BRIGHTNESS >> 1,
MAX_BRIGHTNESS >> 1,
MAX_BRIGHTNESS >> 1,
MAX_BRIGHTNESS >> 1,
MAX_BRIGHTNESS >> 1,
MAX_BRIGHTNESS >> 1,
MAX_BRIGHTNESS >> 1
}; // default to half

// Direction of the led strip;
private Direction mDirection;
Expand Down Expand Up @@ -162,14 +171,35 @@ public void setBrightness(int ledBrightness) {
throw new IllegalArgumentException("Brightness needs to be between 0 and "
+ MAX_BRIGHTNESS);
}
mLedBrightness = ledBrightness;
mLedBrightnessGlobal = ledBrightness;
for (int i=0; i<mLedBrightness.length; i++) {
Copy link
Member

Choose a reason for hiding this comment

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

I would say use Arrays.fill here, but see the comment about keeping global brightness and individual brightness separate.

mLedBrightness[i] = mLedBrightnessGlobal;
}
}

/**
* 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 setBrightness(int[] ledBrightness) {
mLedBrightnessGlobal = 0;
for (int i=0; i<mLedBrightness.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];
if (mLedBrightnessGlobal < ledBrightness[i]) {
mLedBrightnessGlobal = ledBrightness[i];
}
}
}

/**
* Get the current brightness level
* Get the current brightness maximum level
*/
public int getBrightness() {
return mLedBrightness;
return mLedBrightnessGlobal;
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that we can only provide one value here. To me it makes better sense to add a int getBrightness(int index) (or a int[] getBrightnessArray()), which then makes me think we should not try to have global brightness and individual brightness be synchronized at all: either the user uses global brightness, or the user uses individual brightness. (Assuming that's the way forward, we could use the global brightness by default.)

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 can return an array of int, then we can be consistent about returning the array of brightness as it is or null if only global brightness is set. I do think that keeping them in sync makes sense as long as the logic is clear. Also, I agree. I do not expect a developer to mix them on the same project.
Will do some changes later today.

}

/**
Expand Down Expand Up @@ -212,9 +242,9 @@ 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
for (int i = 0; i < colors.length; i++) {
byte brightness = (byte) (0xE0 | mLedBrightness[i]); // Less brightness possible
Copy link
Member

Choose a reason for hiding this comment

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

This line will throw index out of bounds exception if colors.length is larger than mBrightness.length. We would need to decide how to handle this (fallback to some value? throw an exception?).

Copy link
Author

Choose a reason for hiding this comment

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

in this case I think an Index out of bounds exception is fine (after the changes). The developer is passing two arrays, one for brightness and one for colors, both should be of the same length.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure throwing an exception is the best option. This method previously could never throw that exception, so ideally we wouldn't change that behavior. (Even if we do decide to throw, we should be the ones throwing it so that we can provide an appropriate message.)

One alternative, for example, would be to repeat the final brightness value.

Copy link
Author

Choose a reason for hiding this comment

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

I'm more in favor of returning an exception of the length of the arrays is different, so it does not behave unexpectedly, will implement that later.

Copy link
Author

Choose a reason for hiding this comment

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

so, @jdkoren I think this is good now, right?

int di = currentDirection == Direction.NORMAL ? i : colors.length - i - 1;
copyApaColorData(brightness, colors[di], mLedMode, mLedData, pos);
pos += APA_COLOR_PACKET_LENGTH;
Expand Down
14 changes: 14 additions & 0 deletions apa102/src/test/java/com/google/android/things/contrib/driver/apa102/Apa102Test.java
View file
Open in desktop
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.setBrightness(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.setBrightness(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