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 Enphase Envoy component #15081

Merged
merged 19 commits into from Aug 2, 2018

Conversation

Projects
None yet
7 participants
@jesserizzo
Copy link
Contributor

commented Jun 21, 2018

Description:

Add component to get energy consumption and solar panel energy production from an Enphase Envoy

Related issue (if applicable): fixes #

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

Example entry for configuration.yaml (if applicable):

# Example configuration.yaml entry
sensor:
  - platform: enphase_envoy
    ip: LOCAL_IP_OF_ENVOY
    name: NAME_TO_DISPLAY_IN_FRONTEND
    monitored_conditions:
    - production
    - daily_production
    - 7_days_production
    - lifetime_production
    - consumption
    - daily_consumption
    - 7_days_consumption
    - lifetime_consumption

Checklist:

  • The code change is tested and works locally.
  • Local tests pass with tox. Your PR cannot be merged unless tests pass

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

If the code communicates with devices, web services, or third-party tools:

  • New dependencies have been added to the REQUIREMENTS variable (example).
  • New dependencies are only imported inside functions that use them (example).
  • New or updated 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:

  • Tests have been added to verify that the new code works.
@homeassistant

This comment has been minimized.

Copy link

commented Jun 21, 2018

Hi @jesserizzo,

It seems you haven't yet signed a CLA. Please do so here.

Once you do that we will be able to review and accept this pull request.

Thanks!

"""Initialize the sensor."""


self._url = "http://{}/production.json".format(ip_address)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

too many blank lines (2)




class Envoy(Entity):

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

too many blank lines (3)

#out of the config, or that the given sensor is in the monitored conditions list
for i in range(8):
if monitored_conditions == {} or SENSOR_TYPES[i] in monitored_conditions:
add_devices([Envoy(ip_address, DEFAULT_NAMES[i], SENSOR_TYPES[i])], True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

line too long (85 > 79 characters)

#Iterate through the list of sensors, adding it if either monitored conditions has been left
#out of the config, or that the given sensor is in the monitored conditions list
for i in range(8):
if monitored_conditions == {} or SENSOR_TYPES[i] in monitored_conditions:

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

line too long (81 > 79 characters)

monitored_conditions = config.get(CONF_MONITORED_CONDITIONS, {})

#Iterate through the list of sensors, adding it if either monitored conditions has been left
#out of the config, or that the given sensor is in the monitored conditions list

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

block comment should start with '# '
line too long (84 > 79 characters)


DEFAULT_NAMES = ["Envoy Current Energy Production", "Envoy Today's Energy Production", "Envoy Last Seven Days Energy Production",
"Envoy Lifetime Energy Production", "Envoy Current Energy Consumption", "Envoy Today's Energy Consumption",
"Envoy Last Seven Days Energy Consumption", "Envoy Lifetime Energy Consumption"]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent
line too long (80 > 79 characters)

CONF_MONITORED_CONDITIONS = 'monitored_conditions'

DEFAULT_NAMES = ["Envoy Current Energy Production", "Envoy Today's Energy Production", "Envoy Last Seven Days Energy Production",
"Envoy Lifetime Energy Production", "Envoy Current Energy Consumption", "Envoy Today's Energy Consumption",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent
line too long (107 > 79 characters)

CONF_IP_ADDRESS = 'ip'
CONF_MONITORED_CONDITIONS = 'monitored_conditions'

DEFAULT_NAMES = ["Envoy Current Energy Production", "Envoy Today's Energy Production", "Envoy Last Seven Days Energy Production",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

line too long (129 > 79 characters)

from homeassistant.helpers.entity import Entity
from homeassistant.components.sensor import PLATFORM_SCHEMA
import homeassistant.helpers.config_validation as cv
from homeassistant.const import CONF_NAME

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

'homeassistant.const.CONF_NAME' imported but unused

@@ -0,0 +1,126 @@
"""
Support for monitoring energy usage and solar panel energy production using the Enphase Envoy.

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

line too long (94 > 79 characters)

@ghost ghost added the in progress label Jun 21, 2018

@homeassistant homeassistant added cla-signed and removed cla-needed labels Jun 21, 2018

add_devices([Envoy(ip_address, DEFAULT_NAMES[i], \
SENSOR_TYPES[i])], True)

class Envoy(Entity):

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

expected 2 blank lines, found 1

if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:
add_devices([Envoy(ip_address, DEFAULT_NAMES[i], \
SENSOR_TYPES[i])], True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent

for i in range(8):
if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:
add_devices([Envoy(ip_address, DEFAULT_NAMES[i], \

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

the backslash is redundant between brackets

#is in the monitored conditions list
for i in range(8):
if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line missing indentation or outdented


#Iterate through the list of sensors, adding it if either monitored
#conditions has been left out of the config, or that the given sensor
#is in the monitored conditions list

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

block comment should start with '# '

ip_address = config.get(CONF_IP_ADDRESS)
monitored_conditions = config.get(CONF_MONITORED_CONDITIONS, {})

#Iterate through the list of sensors, adding it if either monitored

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

block comment should start with '# '


SENSOR_TYPES = ["production", "daily_production", "7_days_production",
"lifetime_production", "consumption", "daily_consumption",
"7_days_consumption", "lifetime_consumption"]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent

"Envoy Lifetime Energy Consumption"]

SENSOR_TYPES = ["production", "daily_production", "7_days_production",
"lifetime_production", "consumption", "daily_consumption",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent

DEFAULT_NAMES = ["Envoy Current Energy Production",
"Envoy Today's Energy Production",
"Envoy Last Seven Days Energy Production",
"Envoy Lifetime Energy Production",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent


DEFAULT_NAMES = ["Envoy Current Energy Production",
"Envoy Today's Energy Production",
"Envoy Last Seven Days Energy Production",

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent

if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:
add_devices([Envoy(ip_address, DEFAULT_NAMES[i],
SENSOR_TYPES[i])], True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line under-indented for visual indent

# is in the monitored conditions list
for i in range(8):
if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

continuation line with same indent as next logical line

from homeassistant.helpers.entity import Entity
from homeassistant.components.sensor import PLATFORM_SCHEMA
import homeassistant.helpers.config_validation as cv
#from homeassistant.const import CONF_NAME

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 21, 2018

block comment should start with '# '

jesserizzo added some commits Jun 21, 2018

import requests

try:
response = requests.get(self._url, timeout=5)

This comment has been minimized.

Copy link
@exxamalte

exxamalte Jun 22, 2018

Contributor

Why not use the RestData class (components/sensor/rest.py) here? May save a bit of code duplication.
Or maybe even use one instance of RestData and share across all sensors?

@exxamalte

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2018

I have an Envoy-S myself and so far have simply been using rest sensors to fetch the data and display it in HA. In my experience, having HA to retrieve the JSON from the Envoy device 8 times in a very short time sometimes led to one or two requests to fail. Hence I am suggesting to use RestData to fetch the data and share this object across all sensors.

@exxamalte

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2018

I'd also like to suggest to add some unit tests. The JSON format used by the Envoy is not a public API, and having a few current sample JSON responses to test your code against would help just in case Enphase decides to change the format; you would then just update the test JSON and modify the code until the tests work again.

if type == 'production' or type == 'consumption':
self._unit_of_measurement = 'watts'
else:
self._unit_of_measurement = "watt hours"

This comment has been minimized.

Copy link
@exxamalte

exxamalte Jun 22, 2018

Contributor

I would prefer W and Wh here. Looking at homeassistant/const.py it seems to be the consensus for other units of measurement to use the abbreviated version. Maybe these two could be added there?

This comment has been minimized.

Copy link
@jesserizzo

jesserizzo Jun 22, 2018

Author Contributor

A couple of really good points, thank you. I'm not really sure how to do unit tests. Is there somewhere you can suggest to learn more about that?

This comment has been minimized.

Copy link
@exxamalte

exxamalte Jun 23, 2018

Contributor

Maybe the easiest way is by looking at the existing unit tests. Most unit tests focus on two areas - setting up the component, and then testing the various functionalities, transformation, error handling, etc. You can load a static JSON file with load_fixture.
The developer documentation tells you how to run tests in your environment.

elif self._type == "daily_consumption":
self._state = int(response_parsed["consumption"][0]["whToday"])
elif self._type == "7_days_consumption":
self._state = int(response_parsed["consumption"][0]["whLastSevenDays"])

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

line too long (83 > 79 characters)

elif self._type == "daily_production":
self._state = int(response_parsed["production"][1]["whToday"])
elif self._type == "7_days_production":
self._state = int(response_parsed["production"][1]["whLastSevenDays"])

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

line too long (82 > 79 characters)

self._name = name


if sensor_type == 'production' or sensor_type == 'consumption':

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

too many blank lines (2)

import json
import voluptuous as vol

from homeassistant.components.sensor.rest import RestData

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

'homeassistant.components.sensor.rest.RestData' imported but unused

self._state = int(response_parsed["consumption"][0]["whToday"])
elif self._type == "7_days_consumption":
self._state = \
int(response_parsed["consumption"][0]["whLastSevenDays"])

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

continuation line missing indentation or outdented

self._state = int(response_parsed["production"][1]["whToday"])
elif self._type == "7_days_production":
self._state = \
int(response_parsed["production"][1]["whLastSevenDays"])

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

continuation line missing indentation or outdented

if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:
add_devices([Envoy(ip_address, DEFAULT_NAMES[i],
SENSOR_TYPES[i])], True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

continuation line under-indented for visual indent

if monitored_conditions == {} or \
SENSOR_TYPES[i] in monitored_conditions:
add_devices([Envoy(ip_address, DEFAULT_NAMES[i],
SENSOR_TYPES[i])], True)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jun 29, 2018

continuation line over-indented for visual indent

@frenck frenck added the docs-missing label Jun 30, 2018

@jesserizzo jesserizzo referenced this pull request Jun 30, 2018

Merged

Add documentation for new Enphase Envoy platform #5638

2 of 2 tasks complete
@frenck

This comment has been minimized.

Copy link
Member

commented Jun 30, 2018

Docs PR spotted. Removed docs-missing label.

@MartinHjelmare
Copy link
Member

left a comment

The requests and parsing should be broken out into standalone library published on pypi.

https://developers.home-assistant.io/docs/en/creating_platform_code_review.html#6-communication-with-devices-services

@MartinHjelmare
Copy link
Member

left a comment

Nice! See further comments below.

For more details about this platform, please refer to the documentation at
https://home-assistant.io/components/sensor.enphase_envoy/
"""

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Please remove this blank line.

"""

import logging
import voluptuous as vol

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Please add a blank line between standard library and 3rd party imports and 3rd party and homeassistant imports.

REQUIREMENTS = ['envoy_reader==0.1']
_LOGGER = logging.getLogger(__name__)

CONF_IP_ADDRESS = 'ip'

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Please import and use CONF_IP_ADDRESS from const.py.

_LOGGER = logging.getLogger(__name__)

CONF_IP_ADDRESS = 'ip'
CONF_MONITORED_CONDITIONS = 'monitored_conditions'

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Import and use CONF_MONITORED_CONDITIONS from const.py.


PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_IP_ADDRESS): cv.string,
vol.Optional(CONF_MONITORED_CONDITIONS): vol.All(cv.ensure_list)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Make sure the selected conditions are in the SENSOR_TYPES list and default to the whole list.

vol.Optional(CONF_MONITORED_CONDITIONS, default=SENSOR_TYPES): vol.All(
    cv.ensure_list, [vol.In(SENSOR_TYPES)])
# Iterate through the list of sensors, adding it if either monitored
# conditions has been left out of the config, or that the given sensor
# is in the monitored conditions list
for i in range(8):

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Just iterate monitored_conditions.

"Envoy Last Seven Days Energy Consumption",
"Envoy Lifetime Energy Consumption"]

SENSOR_TYPES = [

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

It's cleaner to make this a list of lists or tuples, where the items in the list or tuple are the sensor type and the corresponding sensor name. Then we don't have to keep track of that the order in the list of types matches the order of the list of names. See eg:
https://github.com/home-assistant/home-assistant/blob/dev/homeassistant/components/sensor/airvisual.py#L43-L47

# conditions has been left out of the config, or that the given sensor
# is in the monitored conditions list
for i in range(8):
if monitored_conditions == {} or \

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Validation should already be done in the config schema, so we don't need to check that here.

"""Initialize the sensor."""
self._ip_address = ip_address
self._name = name
if sensor_type == 'production' or sensor_type == 'consumption':

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Move the unit of measurement type to the nested list of sensor types. If no unit should be used, put None in the list.

@@ -1451,3 +1454,5 @@ zigpy-xbee==0.1.1

# homeassistant.components.zha
zigpy==0.1.0

# homeassistant.components.sensor.enphase_envoy

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Jul 30, 2018

Member

Run script/gen_requirements.py to update the requirements file correctly.

@home-assistant home-assistant deleted a comment from houndci-bot Jul 30, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Jul 30, 2018

@home-assistant home-assistant deleted a comment from houndci-bot Jul 30, 2018

@jesserizzo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

Ok @MartinHjelmare I did most of the changes you suggested. Thanks for being so detailed. The only thing I did differently is I made the sensors a dictionary with value a tuple of the default name and unit of measurement.

SENSORS = {
    "production": ("Envoy Current Energy Production", 'W'),  
    "daily_production": ("Envoy Today's Energy Production", "Wh"),  
    "7_days_production": ("Envoy Last Seven Days Energy Production", "Wh"),  
    "lifetime_production": ("Envoy Lifetime Energy Production", "Wh"),  
    "consumption": ("Envoy Current Energy Consumption", "W"),  
    "daily_consumption": ("Envoy Today's Energy Consumption", "Wh"),  
    "7_days_consumption": ("Envoy Last Seven Days Energy Consumption", "Wh"),  
    "lifetime_consumption": ("Envoy Lifetime Energy Consumption", "Wh")  
    }

I think this accomplishes what you wanted so we aren't trying to keep multiple lists synced up.

Also, I can't get the gen_requirements_all.py to run. I'm getting

No such file or directory: '../homeassistant/package_constraints.txt'

Any suggestions? Thanks.

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 2, 2018

from homeassistant.components.sensor import PLATFORM_SCHEMA
import homeassistant.helpers.config_validation as cv
from homeassistant.const import (CONF_IP_ADDRESS)
from homeassistant.const import (CONF_MONITORED_CONDITIONS)

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Aug 2, 2018

Member

Group imports from the same module. I use isort to do this automatically for new modules. Most advanced editors have plugins that can do this.

from homeassistant.const import (
    CONF_IP_ADDRESS, CONF_MONITORED_CONDITIONS)
ip_address = config[CONF_IP_ADDRESS]
monitored_conditions = config[CONF_MONITORED_CONDITIONS]

# Iterate through the list of sensors

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Aug 2, 2018

Member

Indent the comment with the code that the comment is aimed at.

https://home-assistant.io/components/sensor.enphase_envoy/
"""
import logging
import voluptuous as vol

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Aug 2, 2018

Member

There should be a blank line between logging and voluptuous.

jesserizzo added some commits Aug 2, 2018

@jesserizzo

This comment has been minimized.

Copy link
Contributor Author

commented Aug 2, 2018

@MartinHjelmare Did you run script/setup first?

Yep, that was it. Thanks.

@MartinHjelmare
Copy link
Member

left a comment

Great!

@MartinHjelmare MartinHjelmare merged commit affd4e7 into home-assistant:dev Aug 2, 2018

4 of 5 checks passed

coverage/coveralls Coverage decreased (-1.2%) to 93.823%
Details
Hound No violations found. Woof!
WIP ready for review
Details
cla-bot Everyone involved has signed the CLA
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ghost ghost removed the in progress label Aug 2, 2018

@machielw

This comment has been minimized.

Copy link

commented Aug 5, 2018

Error with envoy which does not measure the Consumption (Envoy s standard). I Left the last 4 lines away from the sensor configuration.

Log Details (ERROR)
Sun Aug 05 2018 17:12:29 GMT+0200 (CEST)

Invalid config for [sensor.enphase_envoy]: required key not provided @ data['ip_address']. Got None. (See ?, line ?). Please check the docs at https://home-assistant.io/components/sensor.enphase_envoy

@MartinHjelmare

This comment has been minimized.

Copy link
Member

commented Aug 5, 2018

Please open an issue.

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Aug 5, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.