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.

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

Add Philips Hue action type #33

Merged
merged 1 commit into from May 24, 2017
Merged

Add Philips Hue action type #33

merged 1 commit into from May 24, 2017

Conversation

@bazwilliams
Copy link
Contributor

@bazwilliams bazwilliams commented May 12, 2017

This adds Philips hue support using the phue library.

It would be nice to configure the hue bridge in the global configuration file, but I wasn't able to find a nice way to pass this into the actions.py class. As such I've forced the user to set it for each actor request.

Example use where the hostname for my Hue bridge is philips-hue.

actor.add_keyword(_('change to purple'), ChangeLightColour(say, "philips-hue", "Lounge Lamp", "FFFFbe"))

@googlebot
Copy link

@googlebot googlebot commented May 12, 2017

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.
@googlebot googlebot added the cla: no label May 12, 2017
@bazwilliams
Copy link
Contributor Author

@bazwilliams bazwilliams commented May 12, 2017

I signed it!

@googlebot
Copy link

@googlebot googlebot commented May 12, 2017

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels May 12, 2017
light.xy = converter.hex_to_xy(self.hex_colour)
self.say(_("Ok"))
bridge = self.find_bridge()
if bridge is not None:

This comment has been minimized.

@ensonic

ensonic May 15, 2017
Collaborator

if bridge:

@ensonic
Copy link
Collaborator

@ensonic ensonic commented May 15, 2017

Thanks. Please squash the changes and make sure travis is happy (follow Details link).

@bazwilliams
Copy link
Contributor Author

@bazwilliams bazwilliams commented May 15, 2017

Squashed and fixed travis. Also use if bridge: as suggested.

@drigz
Copy link
Member

@drigz drigz commented May 18, 2017

Thanks for the PR, @bazwilliams! If you can use rgbxy from PyPI, it's just some style changes required.

@bazwilliams
Copy link
Contributor Author

@bazwilliams bazwilliams commented May 18, 2017

@drigz What style changes do I need to make?

I tried the rgbxy lib from PyPI as I'd rather install the dependency, but it failed to install. I'll have another look tonight to see what I can do.

@drigz
Copy link
Member

@drigz drigz commented May 18, 2017

You should see the comments on the GitHub pull request.

Also, now #48 is merged, you can add phue and rgbxy to requirements.txt. If rgbxy is broken, maybe you could get in touch with the author? If they're not responding, you'll have to fork their project. Unfortunately, we can't bring the code into our repo.

@bazwilliams
Copy link
Contributor Author

@bazwilliams bazwilliams commented May 18, 2017

Thanks for clarifying and will get the requirements.txt file updated, worst case I'll fork publish a new pypi lib for rgbxy.

Library for RGB / CIE1931 "x, y" coversion.
Based on Philips implementation guidance:
http://www.developers.meethue.com/documentation/color-conversions-rgb-xy
Copyright (c) 2016 Benjamin Knight / MIT License.

This comment has been minimized.

@drigz

drigz May 18, 2017
Member

We can't add this file to our repo, as the copyright/license aren't compatible.

Can you use rgbxy from PyPI?

@@ -20,6 +20,10 @@

import actionbase

import phue
from rgb_xy import Converter
converter = Converter()

This comment has been minimized.

@drigz

drigz May 18, 2017
Member

Move converter to within ChangeLightColor instead of creating a global variable.

@@ -194,6 +198,43 @@ def run(self, voice_command):
self.say(to_repeat)


# Example: Change Philips Light Colour

This comment has been minimized.

@drigz

drigz May 18, 2017
Member

Please change to color, here and in the rest of the file. Google coding style is to use the US spellings (any other British spellings in the codebase are probably from me in a moment of weakness).

@@ -20,6 +20,10 @@

import actionbase

import phue

This comment has been minimized.

@drigz

drigz May 18, 2017
Member

The ordering of imports should be:

  1. Standard library
  2. Dependencies
  3. Within the project

So phue and rgbxy need to be a separate section between subprocess and actionbase.


class ChangeLightColour(object):

"""Change a philips hue bulb colour."""

This comment has been minimized.

@drigz

drigz May 18, 2017
Member

Uppercase Philips Hue.

@drigz
Copy link
Member

@drigz drigz commented May 18, 2017

FYI, I missed clicking Submit on the review, but I have now.

@@ -32,6 +32,7 @@ sudo pip3 install --upgrade pip virtualenv

cd "${scripts_dir}/.."
virtualenv --system-site-packages -p python3 env

This comment has been minimized.

@drigz

drigz May 19, 2017
Member

Please remove the empty line here.

@codecov-io
Copy link

@codecov-io codecov-io commented May 21, 2017

Codecov Report

Merging #33 into master will decrease coverage by 69.53%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #33       +/-   ##
==========================================
- Coverage    79.2%   9.67%   -69.54%     
==========================================
  Files           5       3        -2     
  Lines         202    1705     +1503     
  Branches        0     293      +293     
==========================================
+ Hits          160     165        +5     
- Misses         42    1530     +1488     
- Partials        0      10       +10
Impacted Files Coverage Δ
src/action.py 65.21% <100%> (+8.07%) ⬆️
tests/test_actor_base.py
tests/test_speak_time.py
tests/test_speak_shell_command_output.py
src/triggers/__init__.py 4.21% <0%> (ø)

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 28a0228...70b8735. Read the comment docs.

@bazwilliams
Copy link
Contributor Author

@bazwilliams bazwilliams commented May 22, 2017

I added a test to improve the coverage, but it hasn't been recognised by codecov. Any idea why not?

.coveragerc Outdated
*/mock/mock.py
*/mock/__init__.py
*/pbr/*.py
*/six.py

This comment has been minimized.

@ensonic

ensonic May 23, 2017
Collaborator

FYI: here is the generic coveragerc that codecov suggests: https://gist.github.com/codecov-io/bf15bde2c7db1a011b6e
I am not seeing any coverage data for your branch on:
https://codecov.io/gh/google/aiyprojects-raspbian/commits
I think you need to fixs fix the pep8 warnings, see
https://travis-ci.org/google/aiyprojects-raspbian/builds/234963910?utm_source=github_status&utm_medium=notification

@bazwilliams
Copy link
Contributor Author

@bazwilliams bazwilliams commented May 23, 2017

@ensonic you were right! The coverage never ran due to that pep8 warning that was added by mistake. Fixing that and including the suggested .coverage file shows the coverage I was expecting!

Thanks for helping out! I've rebased the PR.

@ensonic ensonic merged commit 9571936 into google:master May 24, 2017
4 checks passed
4 checks passed
cla/google All necessary CLAs are signed
codecov/patch 100% of diff hit (target 79.2%)
Details
codecov/project Absolute coverage decreased by -69.53% but relative coverage increased by +20.79% compared to 28a0228
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ensonic
Copy link
Collaborator

@ensonic ensonic commented May 24, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.