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

MagicLight/Flux WiFi Color LED Light Component #2534

Merged
merged 3 commits into from
Jul 20, 2016
Merged

MagicLight/Flux WiFi Color LED Light Component #2534

merged 3 commits into from
Jul 20, 2016

Conversation

Danielhiversen
Copy link
Member

@Danielhiversen Danielhiversen commented Jul 16, 2016 •
edited
Loading

Description:

https://community.home-assistant.io/t/magiclight-flux-wifi-color-led-light-component/2271

Many thanks to mplawner and @tchellomello for helping me with testing

Pull request in home-assistant.io with documentation (if applicable): home-assistant/home-assistant.io#649

Example entry for configuration.yaml (if applicable):

light:                                                                                                                                                                                                            
   platform: flux_led                                                                                                                                                                                               
   automatic_add: True                                                                                                                                                                                              
   devices:                                                                                                                                                                                                         
     192.168.1.5:                                                                                                                                                                                                   
       name: test                                                                                                                                                                                                   
     192.168.1.3:                                                                                                                                                                                                   
       name: test2

Checklist:

If user exposed functionality or configuration variables are added/changed:

If code communicates with devices, web services, or a:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New dependencies have been added to requirements_all.txt by running script/gen_requirements_all.py.
  • New files were added to .coveragerc.

If the code does not interact with devices:

  • Local tests with tox run successfully. Your PR cannot be merged unless tests pass
  • Tests have been added to verify that the new code works.

balloobbot
balloobbot reviewed Jul 17, 2016
View reviewed changes
homeassistant/components/light/flux_led.py
self._bulb = flux_led.WifiLedBulb(ipaddr)
except socket.error:
self.is_valid = False
_LOGGER.error("Failed to connect to bulb %s, %s",
Copy link

Choose a reason for hiding this comment

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

farcy v1.1

  • 62: W291 trailing whitespace

@fabaff
Copy link
Member

fabaff commented Jul 17, 2016

Automatic detection of my WiFi LED CONTROLLER works. On/off and interfering with the MagicHome app works too.

fabaff
fabaff reviewed Jul 17, 2016
View reviewed changes
homeassistant/components/light/flux_led.py
from homeassistant.components.light import Light
import homeassistant.helpers.config_validation as cv

REQUIREMENTS = ['https://github.com/Danielhiversen/flux_led/archive/0.3.zip']
Copy link
Member

Choose a reason for hiding this comment

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

The traceback I got:

16-07-17 15:49:56 INFO (MainThread) [homeassistant.loader] Loaded light.flux_led from homeassistant.components.light.flux_led
Traceback (most recent call last):
  File "/home/fab/Documents/repos/home-assistant/homeassistant/util/package.py", line 46, in check_package_exists
    req = pkg_resources.Requirement.parse(package)
  [...]
  File "/home/fab/Documents/repos/home-assistant/homeassistant/util/package.py", line 49, in check_package_exists
    req = pkg_resources.Requirement.parse(urlparse(package).fragment)
  File "/home/fab/Documents/repos/home-assistant/lib64/python3.4/site-packages/pkg_resources/__init__.py", line 3047, in parse
    req, = parse_requirements(s)
ValueError: need more than 0 values to unpack

I guess that it could be fixed by changing the REQUIREMENTS (untested) -> REQUIREMENTS = ['https://github.com/Danielhiversen/flux_led/archive/0.3.zip#flux_led==0.3']

@Danielhiversen
Copy link
Member Author

Thanks for your feedback.
The mistake with the library is now fixed, tested and seems to work.

@Danielhiversen
Copy link
Member Author

@balloob Could you have a look at this?

balloob
balloob reviewed Jul 18, 2016
View reviewed changes
homeassistant/components/light/flux_led.py

PLATFORM_SCHEMA = vol.Schema({
vol.Required("platform"): DOMAIN,
vol.Optional('devices', default={}): vol.All(dict, _valid_lights),
Copy link
Member

Choose a reason for hiding this comment

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

Instead of vol.All(dict, _valid_lights) you can use:

{cv.string: DEVICE_SCHEMA}

Copy link
Member

Choose a reason for hiding this comment

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

Also, should this be required?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should not be required.
You do not have to specify any devices/lights.

@Danielhiversen
Copy link
Member Author

I think this is ready for a new review.

@balloob
Copy link
Member

balloob commented Jul 20, 2016

Looks good! 🐬

@balloob balloob merged commit a6e95db into dev Jul 20, 2016
@balloob balloob deleted the flux_light branch July 20, 2016 02:32
@home-assistant home-assistant locked and limited conversation to collaborators Mar 17, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Danielhiversen @fabaff @balloob @balloobbot