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

Sensor component for Fronius inverters, storages, and smart meters #11446

Closed
wants to merge 23 commits into from

Conversation

@gbeine
Copy link

commented Jan 4, 2018

Description:

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

Example entry for configuration.yaml (if applicable):

- platform: fronius
  host: 172.31.106.10
  device: 1
  type: inverter

- platform: fronius
  host: 172.31.106.11
  type: power_flow

## optional: create shortcut to attributes of the device
- platform: template
  sensors:
    electricity_inverter1_power_netto:
      unit_of_measurement: 'W'
      value_template: >-
        {% if states.sensor.fronius_inverter_1723110610_1.attributes.power_ac is defined %}
          {{ states.sensor.fronius_inverter_1723110610_1.attributes.power_ac | float | round(2) }}
        {% else %}
          0
        {% endif %}
    electricity_autonomy:  
      unit_of_measurement: '%'
      value_template: >-
        {% if states.sensor.fronius_power_flow_1723110611.attributes.relative_autonomy is defined %}
          {{ states.sensor.fronius_power_flow_1723110611.attributes.relative_autonomy | float | round(2) }}
        {% else %}
          0
        {% endif %}

Checklist:

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

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

  • [WiP] 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.
gbeine added 5 commits Jan 2, 2018

@gbeine gbeine requested a review from andrey-git as a code owner Jan 4, 2018

@homeassistant

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2018

Hi @gbeine,

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!

attributes = {}
for key in values:
if 'value' in values[key]:
attributes[key] = values[key]['value'] if values[key]['value'] else 0

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

line too long (85 > 79 characters)


_LOGGER.debug(values)

if values:

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

trailing whitespace

_LOGGER.debug("Update {}".format(self.name))

values = {}

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

blank line contains whitespace

def async_update(self):
"""Retrieve latest state."""
_LOGGER.debug("Update {}".format(self.name))

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

blank line contains whitespace

device = DEFAULT_DEVICE

sensor = FroniusSensor(fronius, name, config[CONF_TYPE], config[CONF_SCOPE], device)

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

blank line contains whitespace

vol.Optional(CONF_DEVICE): cv.positive_int,
})

@asyncio.coroutine

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

expected 2 blank lines, found 1

PLATFORM_SCHEMA = PLATFORM_SCHEMA.extend({
vol.Required(CONF_HOST): cv.string,
vol.Required(CONF_TYPE): vol.In(SENSOR_TYPES),
vol.Optional(CONF_SCOPE, default=DEFAULT_SCOPE): vol.All(cv.ensure_list, [vol.In(SCOPE_TYPES)]),

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

line too long (100 > 79 characters)

DEFAULT_DEVICE = None

SENSOR_TYPES = [ TYPE_INVERTER, TYPE_STORAGE, TYPE_METER, TYPE_POWER_FLOW ]
SCOPE_TYPES = [ SCOPE_DEVICE, SCOPE_SYSTEM ]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

whitespace after '['
whitespace before ']'

DEFAULT_SCOPE = SCOPE_DEVICE
DEFAULT_DEVICE = None

SENSOR_TYPES = [ TYPE_INVERTER, TYPE_STORAGE, TYPE_METER, TYPE_POWER_FLOW ]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

whitespace after '['
whitespace before ']'

from homeassistant.helpers.aiohttp_client import async_get_clientsession
from homeassistant.helpers.entity import Entity

REQUIREMENTS = [ 'pyfronius==0.2' ]

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Jan 4, 2018

whitespace after '['
whitespace before ']'

@homeassistant homeassistant added cla-signed and removed cla-needed labels Jan 4, 2018

@nielstron

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2018

That's great (already tried to publish a fronius component myself). Could you ads support for the "monitored conditions" settings (like in WeatherUnderground)?

@gbeine gbeine changed the title Fronius Sensor component for Fronius inverters, storages, and smart meters Jan 4, 2018

@gbeine gbeine referenced this pull request Jan 4, 2018
2 of 2 tasks complete
@fabaff
Copy link
Member

left a comment

Also, requirements_all.txt contains unrelated changes.

@@ -0,0 +1,157 @@
'''

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

This is a docstring, use """.

from homeassistant.const import (CONF_HOST, CONF_SCAN_INTERVAL)
from homeassistant.helpers.event import async_track_time_interval
from homeassistant.helpers.aiohttp_client import async_get_clientsession
from homeassistant.helpers.entity import Entity

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Please keep the imports ordered. Use isort if you don't want to do it manually.

_LOGGER = logging.getLogger(__name__)

CONF_TYPE = 'type'
CONF_DEVICE = 'device'

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

CONF_DEVICE is already defined in const.py.

async_track_time_interval(hass, async_fronius, interval)


class FroniusSensor(Entity):

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Docstring for the class is missing.

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 18, 2018

Author

Hm... I've added a docstring as in other components, but this comment still persists...


@property
def state(self):
"""Return the ???."""

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Needs an update.

@asyncio.coroutine
def async_setup_platform(hass, config, async_add_devices, discovery_info=None):
"""Set up of Fronius platform."""
_LOGGER.debug("Running setup")

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

That's already logged.

try:
yield from sensor.async_update()
except:
_LOGGER.error("yield failed")

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Need an update. The current message is not useful for an user.

try:
values = yield from self._update()
except ServerDisconnectedError:
_LOGGER.error("ServerDisconnectedError")

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Log message should be useful for the user.

except ServerDisconnectedError:
_LOGGER.error("ServerDisconnectedError")
except TimeoutError:
_LOGGER.error("TimeoutError")

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Dito

if values:
self._state = values['status']['Code']
self._attributes = self._get_attributes(values)
self.async_update_ha_state()

This comment has been minimized.

Copy link
@fabaff

fabaff Feb 2, 2018

Member

Is that really needed?

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 3, 2018

Author

Which part of these lines do you mean?
All? Or just the update?

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

This is a coroutine. Just calling it like this does nothing so it can be safely removed.

"""Update all the fronius sensors."""
try:
yield from sensor.async_update()
except:

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Bare excepts are not allowed. Specify the exceptions that you expect.

return self.data.current_power_flow()

def _get_attributes(self, values):
"""Map the attributes and ensure proper values."""

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 18, 2018

trailing whitespace

try:
values = yield from self._update()
except ServerDisconnectedError:
_LOGGER.error("Sensor data cannot be updated because of connection error.")

This comment has been minimized.

Copy link
@houndci-bot

houndci-bot Feb 18, 2018

line too long (87 > 79 characters)

gbeine added 2 commits Feb 18, 2018
@gbeine

This comment has been minimized.

Copy link
Author

commented Feb 18, 2018

@fabaff @balloob thanks a lot for your comments.
I've updated the sensor component according to your notes - and I hope I've done it properly regarding to the standards.

https://home-assistant.io/components/sensor.fronius/
"""
import asyncio
from concurrent.futures._base import TimeoutError

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

import this from asyncio

import homeassistant.helpers.config_validation as cv
from homeassistant.helpers.entity import Entity
from homeassistant.helpers.event import async_track_time_interval
import voluptuous as vol

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

please put 3rd party imports above home assistant imports

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 19, 2018

Author

Hm... this order has been generated from isort.
Somewhere else @fabaff asked me to use isort for the imports.
What's the right way?

This comment has been minimized.

Copy link
@fabaff

fabaff Apr 28, 2018

Member

This is not sorted by isort. It would look like this

...
import logging

from aiohttp.client_exceptions import ServerDisconnectedError
import voluptuous as vol

from homeassistant.components.sensor import PLATFORM_SCHEMA
from homeassistant.const import CONF_HOST, CONF_SCAN_INTERVAL
...
@asyncio.coroutine
def async_setup_platform(hass, config, async_add_devices, discovery_info=None):
"""Set up of Fronius platform."""
_LOGGER.debug(config)

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Let's remove this debug as it contains the config.

fronius = pyfronius.Fronius(session, config[CONF_HOST])

name = "fronius_{}_{}".format(config[CONF_TYPE], config[CONF_HOST])
if CONF_DEVICEID in config.keys():

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

No need for keys()

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 19, 2018

Author

I'll try it, but AFAIR without keys() something different happened.

except Exception:
_LOGGER.error("Update of sensor data failed.")

interval = config.get(CONF_SCAN_INTERVAL) or timedelta(seconds=10)

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Don't do this. The Entity Platform takes care of this.

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 19, 2018

Author

Ok, is this described somewhere?
How can I access the interval?

This comment has been minimized.

Copy link
@nielstron

nielstron Feb 19, 2018

Contributor

Why would you need to access the interval time? The Entity superclass already handles calling the update method on a regular basis (based on time interval).

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 19, 2018

Author

How does the Entity superclass know about the interval that is configured?
Where does this magic happen?
Can I leave the configuration parameter as it is or is there a pattern that I need to follow?

This comment has been minimized.

Copy link
@balloob
_LOGGER.error("Update of sensor data failed.")

interval = config.get(CONF_SCAN_INTERVAL) or timedelta(seconds=10)
async_track_time_interval(hass, async_fronius, interval)

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Remove this, entity platform will do this if should_poll returns True.

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 19, 2018

Author

I'll search for this. Did this change soon?
I've got these two lines from another sensor.

"""Get the values for the current state."""
if self._type == TYPE_INVERTER:
if self._scope == SCOPE_SYSTEM:
return self.data.current_system_inverter_data()

This comment has been minimized.

Copy link
@balloob

balloob Feb 18, 2018

Member

Is this doing I/O ?

This comment has been minimized.

Copy link
@gbeine

gbeine Feb 19, 2018

Author

This triggers an HTTP request, so it is I/O via network.

This comment has been minimized.

Copy link
@balloob

balloob Feb 22, 2018

Member

This function is a coroutine. You are not allowed to do I/O inside a coroutine.

This comment has been minimized.

Copy link
@balloob

balloob Feb 22, 2018

Member

Remove asyncio.coroutine and name your update method update instead of async_update

This comment has been minimized.

Copy link
@MartinHjelmare

MartinHjelmare Feb 26, 2018

Member

This call returns a coroutine object according to the source in pyfronius.

@nielstron

This comment has been minimized.

Copy link
Contributor

commented Feb 19, 2018

Why do you recommend such a strange configuration?

- platform: template
  sensors:
    electricity_inverter1_power_netto:
      unit_of_measurement: 'W'
      value_template: >-
        {% if states.sensor.fronius_inverter_1723110610_1.attributes.power_ac is defined %}
          {{ states.sensor.fronius_inverter_1723110610_1.attributes.power_ac | float | round(2) }}
        {% else %}
          0
        {% endif %}
    electricity_autonomy:  
      unit_of_measurement: '%'
      value_template: >-
        {% if states.sensor.fronius_power_flow_1723110611.attributes.relative_autonomy is defined %}
          {{ states.sensor.fronius_power_flow_1723110611.attributes.relative_autonomy | float | round(2) }}
        {% else %}
          0
        {% endif %}

If the pyfronius module does not return 0 instead of undefined, please open an issue over at https://github.com/nielstron/pyfronius

@gbeine

This comment has been minimized.

Copy link
Author

commented Feb 19, 2018

@nielstron I recommended this because I wrote this part of pyfronius.
It is a difference if a device returns undefined or 0.

One thing in addition: The component and pyfronius ensures that there is always a valid state of the fronius device in HA. As there is a number of attributes documented in the Fronius API that may or may not be returned on a request, it is always possible that a value is undefined.
The template is just a shortcut for this, the template sensor is not necessary to use the component and see the data.

@fabaff

This comment has been minimized.

Copy link
Member

commented Apr 28, 2018

This PR is gone stale. Please re-open it if you want to go on.

@fabaff fabaff closed this Apr 28, 2018

@home-assistant home-assistant locked and limited conversation to collaborators Sep 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.