Added individual led brightness option to led strip #73

plattysoft · 2017-09-18T19:15:04Z

Added a new utility method that can receive an integer array of brightness.

It is backwards compatible, if individual brightness is set, the maximum brightness is returned for getBrighness()

getting up to date code

jdkoren

I have two main concerns:

Should global brightness and individual brightness be completely separated, and the driver uses whichever approach was last elected?
Issues around the length of the individual brightness values array.

jdkoren · 2017-09-18T19:37:07Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

@@ -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++) {


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

jdkoren · 2017-09-18T19:38:04Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

 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


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?).

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.

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.

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.

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

jdkoren · 2017-09-18T19:42:14Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

@@ -67,7 +67,16 @@
 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;
+ private int mLedBrightness[] = new int[] {


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.

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

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.

jdkoren · 2017-09-18T19:44:04Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

 */
 public int getBrightness() {
- return mLedBrightness;
+ return mLedBrightnessGlobal;


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.)

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.

jdkoren · 2017-09-18T20:36:00Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

@@ -67,7 +67,16 @@
 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;


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

googlebot · 2017-09-19T09:41:44Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

plattysoft · 2017-09-19T09:47:56Z

updated mail preferences

googlebot · 2017-09-19T09:47:58Z

CLAs look good, thanks!

blundell · 2017-09-19T11:57:01Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

@@ -67,7 +67,8 @@
 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[];


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;

good point, thanks!

blundell · 2017-09-19T11:57:49Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

@@ -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;


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

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.

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)

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

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

blundell · 2017-09-19T12:59:41Z

apa102/src/main/java/com/google/android/things/contrib/driver/apa102/Apa102.java

+ return mLedBrightnessGlobal;
+ }
+
+ public int[] getIndividualBrightness() {
 return mLedBrightness;


this can now return null, you could use @Nullable

good point, thanks!

plattysoft · 2017-09-25T18:59:09Z

Hey @jdkoren I did the changes you requested almost a week ago, please comment if you require more changes, approve this PR or discard it.

proppy · 2017-09-28T02:56:02Z

Did we consider (ab)using the android.graphics.Color alpha channel as a brightness indicator?

plattysoft · 2017-09-28T12:28:20Z

@proppy the original idea was to keep backwards compatibility. The simplest way to keep the backwards compatibility was to add a brightness array, but the idea of using the alpha channel is very interesting.

jdkoren

Can we add unit tests for setIndividualBrightness (the values are being returned by getIndividualBrightness) and for the new ways write can behave?

proppy · 2017-10-06T15:48:07Z

@plattysoft @jdkoren I don't think it would break backward compatibility since currently the alpha channel is ignored (this would only affect people who used the driver with Color integer that had a brightness component).

jdkoren · 2017-10-06T16:10:47Z

@proppy I actually like the idea, it would remove the need for both setBrightness and setIndividualBrightness, and there would be no concerns about mismatched array lengths. I think the backwards compatibility issue is this:

Suppose my code calls write({ 0x00ff0000, 0xffff0000 });. Both LEDs would light up red, with the same brightness. Let's presume I'm using the default brightness of MAX_BRIGHTNESS >> 1.
Using the alpha channel for brightness, this call now behaves differently than before in 2 ways: The two LEDs are no longer the same, and neither would be at the default brightness. (The first would be off, the second would be on at full brightness.)

It's worth exploring, but we would need to communicate this change to developers and also make sure our samples are updated accordingly.

Fleker · 2017-10-06T18:14:52Z

If we move to using the alpha value of Color, should we deprecate setBrightness? That would help communicate the definitive way to change the brightness.

We'd keep the method for now but explain why it's being deprecated.

jdkoren · 2017-10-06T18:18:24Z

@Fleker Yes, we would deprecate it.

plattysoft · 2017-10-06T19:08:26Z

So, should I move on to just add the tests, or should we discard this approach and move towards the alpha channel? I'm happy to do it either way, just looking for a confirmation @jdkoren

mangini · 2017-12-19T23:21:15Z

I like using alpha for individual brightness. To maintain backwards compatibility, instead of deprecating setBrightness, I suggest the following changes:

if globalBrightness is set (0-255), individual colors' alpha are ignored and globalBrightness is used for all LED's instead
if globalBrightness is UNSET (e.g., -1), individual colors' alpha are used
if setBrightness is called with a UNSET value (-1), it is unset

So, the following changes would be required:

describe this behavior in javadoc
rename mLedBrighness and ledBrightness to mGlobalBrightness and globalBrightness
create a private GLOBAL_BRIGHTNESS_UNSET=-1 constant
create a unsetBrightness() public method (ideally should be called unsetGlobalBrightness, but it's better to keep parity with the existing setter/getter methods)
change write behavior to use color<<24 for alpha if mGlobalBrightness is UNSET

Assuming Color transparency is used, these specific change requests are moot - although the new code will still need tests.

mangini

Adding my comment as a review now:

I like using alpha for individual brightness. To maintain backwards compatibility, instead of deprecating setBrightness, I suggest the following changes:

if globalBrightness is set (0-255), individual colors' alpha are ignored and globalBrightness is used for all LED's instead
if globalBrightness is UNSET (e.g., -1), individual colors' alpha are used
if setBrightness is called with a UNSET value (-1), it is unset

So, the following changes would be required:

describe this behavior in javadoc
rename mLedBrighness and ledBrightness to mGlobalBrightness and globalBrightness
create a private GLOBAL_BRIGHTNESS_UNSET=-1 constant
create a unsetBrightness() public method (ideally should be called unsetGlobalBrightness, but it's better to keep parity with the existing setter/getter methods)
change write behavior to use color<<24 for alpha if mGlobalBrightness is UNSET

plattysoft added 3 commits September 18, 2017 19:28

Merge pull request #1 from androidthings/master

eb5a03d

getting up to date code

added invidual brightness setting per led

9066f46

added invidual brightness setting per led

903b118

jdkoren requested changes Sep 18, 2017

View reviewed changes

separating the individual and global brightness concepts

1b58fec

updating method name in tests

35b243c

blundell reviewed Sep 19, 2017

View reviewed changes

Raul Portales and others added 2 commits September 19, 2017 15:18

fixed code style

0dc3695

added check of array sizes

96fc125

jdkoren previously requested changes Oct 6, 2017

View reviewed changes

mangini self-requested a review December 19, 2017 23:25

mangini suggested changes Dec 19, 2017

View reviewed changes

Added individual led brightness option to led strip #73

Are you sure you want to change the base?

Added individual led brightness option to led strip #73

Conversation

plattysoft commented Sep 18, 2017

jdkoren left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdkoren Sep 18, 2017 • edited Loading

Choose a reason for hiding this comment

googlebot commented Sep 19, 2017

plattysoft commented Sep 19, 2017

googlebot commented Sep 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

plattysoft commented Sep 25, 2017

proppy commented Sep 28, 2017 • edited Loading

plattysoft commented Sep 28, 2017 • edited Loading

jdkoren left a comment

Choose a reason for hiding this comment

proppy commented Oct 6, 2017

jdkoren commented Oct 6, 2017

Fleker commented Oct 6, 2017 • edited Loading

jdkoren commented Oct 6, 2017

plattysoft commented Oct 6, 2017

mangini commented Dec 19, 2017

mangini left a comment

Choose a reason for hiding this comment

jdkoren Sep 18, 2017 •

edited

Loading

proppy commented Sep 28, 2017 •

edited

Loading

plattysoft commented Sep 28, 2017 •

edited

Loading

Fleker commented Oct 6, 2017 •

edited

Loading