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 5 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
32 changes: 28 additions & 4 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,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[];
Copy link
Contributor

Choose a reason for hiding this comment

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

best practice dictates to put the array initialiser on the field type, so you can easily see the difference between a single int and an int array.

private int[] mLedBrightness;

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, thanks!


// Direction of the led strip;
private Direction mDirection;
Expand Down Expand Up @@ -162,13 +163,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;
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.

}

public int[] getIndividualBrightness() {
return mLedBrightness;
Copy link
Contributor

Choose a reason for hiding this comment

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

this can now return null, you could use @Nullable

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, thanks!

}

Expand Down Expand Up @@ -212,9 +233,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
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.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