-
Notifications
You must be signed in to change notification settings - Fork 147
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
fixes #43 #49
Closed
Closed
fixes #43 #49
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
5adb615
move led blinking on Audio callback to another thread and limit the n…
plattysoft 6ddab48
move led blinking on Audio callback to another thread and limit the n…
plattysoft 9752d96
put back default volume to 100
plattysoft 71d83cc
addressing PR comments
plattysoft 78b76c5
addressing PR comments
plattysoft 4196ed1
addressing second set of comments
plattysoft 709227d
updated Log to error
plattysoft File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
68 changes: 68 additions & 0 deletions
68
app/src/main/java/com/example/androidthings/assistant/LedBlinkThread.java
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
package com.example.androidthings.assistant; | ||
|
||
import android.util.Log; | ||
|
||
import com.google.android.things.pio.Gpio; | ||
|
||
import java.io.IOException; | ||
import java.util.Random; | ||
|
||
public class LedBlinkThread extends Thread { | ||
private static final String TAG = LedBlinkThread.class.getSimpleName(); | ||
private static final long BLINK_INTERVAL = 200; | ||
|
||
private final Gpio mLed; | ||
private final Random mRandom; | ||
private boolean mBlinking = false; | ||
private boolean mClose = false; | ||
|
||
public LedBlinkThread(Gpio led) { | ||
mLed = led; | ||
mRandom = new Random(); | ||
} | ||
|
||
public void blink() { | ||
if (mClose) { | ||
return; | ||
} | ||
mBlinking = true; | ||
synchronized (this) { | ||
notify(); | ||
} | ||
} | ||
|
||
public void close() { | ||
mClose = true; | ||
mBlinking = false; | ||
synchronized (this) { | ||
notify(); | ||
} | ||
} | ||
|
||
@Override | ||
public void run() { | ||
do { | ||
synchronized (this) { | ||
try { | ||
wait(); | ||
} catch (InterruptedException e) { | ||
e.printStackTrace(); | ||
} | ||
} | ||
while (mBlinking) { | ||
mBlinking = false; | ||
try { | ||
mLed.setValue(false); | ||
sleep(BLINK_INTERVAL); | ||
mLed.setValue(true); | ||
sleep(BLINK_INTERVAL); | ||
} catch (IOException e) { | ||
Log.e(TAG, "Error accessing the LED", e); | ||
} catch (InterruptedException e) { | ||
Log.e(TAG, "Error while sleeping", e); | ||
} | ||
} | ||
} | ||
while (!mClose); | ||
} | ||
} |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we turn off the LED in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the LED is configured as ACTIVE_LOW, true is actually off (I said that as a note on the description) I find it confusing, but I did not want to change the original behaviour.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
ACTIVE_LOW
/ACTIVE_HIGH
are set so that setting the value to true/false does what is expected. In my case, the LEDs areACTIVE_LOW
, but that's where the value should be changed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my case led.setValue(false) does turn it on, I'm not sure if that is a configuration problem, a hardware difference or something that changed on the OS since this project was originally made
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually the blinking is following this pattern, first on, then off, so at the end of each blink, it stays off. Let me know if you want me to revert all of them and change the active mode.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would revert the changes so that
true
=>on
andfalse
=>off
.In my experience, the LED is
ACTIVE_LOW
, and I don't know if I want to change that if that's how the VoiceHat really is, but we should probably change it there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful if someone else could verify the LED state on different AIY voice kits and Android Things versions. Until then, I'm not inclined to change anything.
IMHO opening the LED with the correct mode should be a responsibility of the voiceHAT driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, after one more test / reading of the code, onButtonEvent the LED is off when pressed and turns on when released (pressed == false)
The code is mLed.setValue(pressed)
And that is code I have not touched.
Unless it does not work as I described for you, then we clearly have some inconsistency on hardware / operating system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mLed.setValue(pressed);
makes sense using the true/on setup.I do agree that the LED should be set up through the VoiceHat. Until that change is made and published to the driver, we should refrain from making this workaround.