Add AIY common drivers. #90

enetor · 2017-06-27T00:36:55Z

This change includes two common drivers:

Button - A GPIO-based button driver that automatically debounces the
GPIO signal.
LED - A GPIO- and PWM-based LED driver that supports a set of simple
LED patterns.

Tested:
Manually tested on a RPi3 + VoiceHat.

This change includes two common drivers: 1) Button - A GPIO-based button driver that automatically debounces the GPIO signal. 2) LED - A GPIO- and PWM-based LED driver that supports a set of simple LED patterns. Tested: Manually tested on a RPi3 + VoiceHat.

codecov-io · 2017-06-27T00:53:47Z

Codecov Report

Merging #90 into master will increase coverage by 55.87%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           master      #90       +/-   ##
===========================================
+ Coverage    9.95%   65.83%   +55.87%     
===========================================
  Files           3        2        -1     
  Lines        1727      161     -1566     
  Branches      297       16      -281     
===========================================
- Hits          172      106       -66     
+ Misses       1543       52     -1491     
+ Partials       12        3        -9

Impacted Files	Coverage Δ
src/triggers/__init__.py

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4875227...7fcc47e. Read the comment docs.

drigz

Nice start!

Let's see what @ensonic thinks, as he knows more about the LED service.

drigz · 2017-06-27T08:32:25Z

src/aiy/common.py

+# limitations under the License.
+"""This library provides common drivers for the AIY projects.
+
+Drivers in this module requires GPIO.setmode(GPIO.BCM).


If you call this in the code (eg after importing or in __init__) then it is enforced in code. setmode can safely be called multiple times with the same mode.

drigz · 2017-06-27T08:41:45Z

src/aiy/common.py

+        return True
+
+
+class LED:


Do you have a plan for how this will be used? Currently we have a status-led service that animates even when the voice recognizer isn't running (eg during bootup). If we're going to keep that, maybe it would be better to have an API for the service here, instead of a second implementation.

+1 for avoiding two implementations.

My idea is to have the status-led service call this API instead. The benefit of having a standalone API for LEDs is potential users may utilize this API for their own app, not just a global LED service.

So no there won't be two implementations. And yes the status-led service will still be available.

Done. status-led now calls into aiy.common directly, eliminating the duplicate implementation of the LED driver.

ensonic · 2017-06-27T11:47:10Z

src/aiy/common.py

+  def start(self):
+    """Starts the LED driver."""
+    with self.lock:
+      if not self.running:


why would we need the lock here? If it is not running, there is no concurrency. If it is running we change a boolean which does not require a lock in python (GIL)

Correct me if I am wrong, but I think there could be contentions here. Changing the boolean is fine. But self.pwm.start(0) could be called after self.animator.start()

Two threads calling my_led.start()
(#1) self.running = True
(#2) self.running = True
self.pwm.start(0)
self.animator.start()
(#1) self.pwm.start(0)

ensonic · 2017-06-27T11:49:58Z

src/aiy/common.py

+        return
+      if state:
+        if not self._parse_state(state):
+          print('unsupported state: %d' % state)


Please use logging instead of printf or raise a ValueError.

ensonic

Thanks a few comments from my side as well.

The status-led service will now call into aiy.common.LED. Also raise a ValueError when an unexpected state is passed to aiy.common.LED.set_state. Tested: Tried src/main.py and observed expected LED animations.

enetor

PTAL thanks!

ensonic · 2017-06-28T08:18:46Z

src/led.py

 import RPi.GPIO as GPIO

-logger = logging.getLogger('led')
+logger = logging.getLogger("led")


I though we use single quotes for string literals in most of the places. Please keep those as they were.

This was changed by pyformat.

ensonic · 2017-06-28T08:20:26Z

src/led.py

+        "--gpio-pin",
+        default=25,
+        type=int,
+        help="GPIO pin for the LED (default: 25)")


This kind of reformatting is not helpful. I see that both calls used different wrapping styles (new line for each args or not), but here you just flip it.

I did not manually reformat the code, but pyformat did. At some point we have to automate this with some tool, preferably pyformat.

ensonic

Only a few nits left

ensonic

Thanks!

Add AIY common drivers.

6fb5f1c

This change includes two common drivers: 1) Button - A GPIO-based button driver that automatically debounces the GPIO signal. 2) LED - A GPIO- and PWM-based LED driver that supports a set of simple LED patterns. Tested: Manually tested on a RPi3 + VoiceHat.

googlebot added the cla: yes label Jun 27, 2017

enetor self-assigned this Jun 27, 2017

Reformat common.py to pass PEP8 style checks.

73ab0e4

enetor requested review from drigz and ensonic June 27, 2017 00:52

drigz reviewed Jun 27, 2017

View reviewed changes

ensonic reviewed Jun 27, 2017

View reviewed changes

ensonic suggested changes Jun 27, 2017

View reviewed changes

Call aiy.common.LED from led.py.

e4e7b7d

The status-led service will now call into aiy.common.LED. Also raise a ValueError when an unexpected state is passed to aiy.common.LED.set_state. Tested: Tried src/main.py and observed expected LED animations.

enetor commented Jun 27, 2017

View reviewed changes

ensonic reviewed Jun 28, 2017

View reviewed changes

ensonic suggested changes Jun 28, 2017

View reviewed changes

Revert some formatting introduced by pyformat.

7fcc47e

ensonic approved these changes Jun 29, 2017

View reviewed changes

enetor merged commit 5245e12 into master Jun 29, 2017

enetor deleted the aiy_common branch June 29, 2017 18:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add AIY common drivers. #90

Add AIY common drivers. #90

enetor commented Jun 27, 2017

codecov-io commented Jun 27, 2017 •

edited

Loading

drigz left a comment

drigz Jun 27, 2017

enetor Jun 27, 2017

drigz Jun 27, 2017

ensonic Jun 27, 2017

enetor Jun 27, 2017

enetor Jun 27, 2017

ensonic Jun 27, 2017

enetor Jun 27, 2017 •

edited

Loading

ensonic Jun 27, 2017

ensonic left a comment

enetor left a comment

ensonic Jun 28, 2017

enetor Jun 29, 2017

ensonic Jun 28, 2017

enetor Jun 29, 2017

ensonic left a comment

ensonic left a comment

Add AIY common drivers. #90

Add AIY common drivers. #90

Conversation

enetor commented Jun 27, 2017

codecov-io commented Jun 27, 2017 • edited Loading

Codecov Report

drigz 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

enetor Jun 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ensonic left a comment

Choose a reason for hiding this comment

enetor 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

ensonic left a comment

Choose a reason for hiding this comment

ensonic left a comment

Choose a reason for hiding this comment

codecov-io commented Jun 27, 2017 •

edited

Loading

enetor Jun 27, 2017 •

edited

Loading