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 linky sensor #16468

Merged
merged 4 commits into from
Sep 24, 2018
Merged

Add linky sensor #16468

merged 4 commits into from
Sep 24, 2018

Conversation

tiste
Copy link
Contributor

@tiste tiste commented Sep 6, 2018

Description:

It adds a component (sensor) showing the last day consumption of your home from the Linky electric meter.

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

Example entry for configuration.yaml (if applicable):

sensor:
  - platform: linky
    username: !secret linky_username
    password: !secret linky_password

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.

Copy link
Member

@fabaff fabaff left a comment

Choose a reason for hiding this comment

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

Would be nice to use and update release of pylinky when
Pirionfr/pyLinky#3 was addressed.

.coveragerc Outdated
@@ -180,6 +180,8 @@ omit =
homeassistant/components/lametric.py
homeassistant/components/*/lametric.py

homeassistant/components/*/linky.py
Copy link
Member

Choose a reason for hiding this comment

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

Please move it to the others sensors below.

password = config.get(CONF_PASSWORD)

from pylinky.client import LinkyClient
client = LinkyClient(username, password)
Copy link
Member

Choose a reason for hiding this comment

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

Please check if the credentials are valid. If not, make the platform setup fail with a log entry. Otherwise will the users end-up with a platform which is not working in their instance.

Copy link
Contributor Author

@tiste tiste Sep 7, 2018

Choose a reason for hiding this comment

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

Actually, the constructor is doing nothing except storing credentials.
fetch_data() is the only method that login the user and load its data, that's the reason why it's inside the try/catch block.

Copy link
Member

Choose a reason for hiding this comment

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

You should make the first call in setup_platform in this case to ensure that the credentials are valid.


_LOGGER.debug(json.dumps(self._client.get_data(), indent=2))

if self._client.get_data():
Copy link
Member

Choose a reason for hiding this comment

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

Handle this in the try-except block. Return as early as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

get_data() is a getter, not an API call (fetch_data() is). Are you sure to want to put it into the try/catch block?

Copy link
Member

Choose a reason for hiding this comment

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

If fetch_data() fails then it doesn't make sense to go on with update().

REQUIREMENTS = ['pylinky==0.1.5']
_LOGGER = logging.getLogger(__name__)

DOMAIN = 'linky'
Copy link
Member

Choose a reason for hiding this comment

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

Remove this. This platform belongs to the sensor domain.

})


def setup_platform(hass, config, add_devices, discovery_info=None):
Copy link
Member

Choose a reason for hiding this comment

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

Rename add_devices to add_entities.


def setup_platform(hass, config, add_devices, discovery_info=None):
"""Configure the platform and add the Linky sensor."""
username = config.get(CONF_USERNAME)
Copy link
Member

Choose a reason for hiding this comment

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

Don't use dict.get use dict[key] for required config keys.

"""Fetch new state data for the sensor."""
try:
self._client.fetch_data()
except BaseException as exp:
Copy link
Member

Choose a reason for hiding this comment

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

Please only except the exception we expect.

# get the last past day data
self._state = self._client.get_data()['daily'][-2]['conso']
else:
self._state = 0
Copy link
Member

Choose a reason for hiding this comment

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

Unknown state should be represented with None.

timeout = config[CONF_TIMEOUT]

from pylinky.client import LinkyClient
client = LinkyClient(username, password, None, timeout)
Copy link
Member

Choose a reason for hiding this comment

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

If the credentials are wrong make the platform setup fail.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did you mean "valid" data?
Otherwise, there is no public login function. Everything is done in the fetch_data function.

Copy link
Member

Choose a reason for hiding this comment

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

You need to make the first request to the API during the platform setup and go for the PyLinkyError.

except BaseException as exp:
_LOGGER.error(exp)

_LOGGER.debug(json.dumps(self._client.get_data(), indent=2))
Copy link
Member

Choose a reason for hiding this comment

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

Again, return if you can't retrieve the data. The debug message will be empty, thus it will not be useful.

except PyLinkyError as exp:
_LOGGER.error(exp)
return
except BaseException as exp:
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Don't know if your read a bit about pylinky package, but most of the errors are not handled within PyLinkyError error. So it won't be handled.

Copy link
Member

Choose a reason for hiding this comment

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

We should only catch specific expected errors, and not use broad except. If there is a specific error that we expect we can add an except block for that.

except PyLinkyError as exp:
_LOGGER.error(exp)
return
except BaseException as exp:
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

@fabaff fabaff merged commit 4fd2f77 into home-assistant:dev Sep 24, 2018
@ghost ghost removed the in progress label Sep 24, 2018
@balloob balloob mentioned this pull request Sep 28, 2018
@totoroyamada
Copy link

Hi, I get login errors although my username and password are ok (i can connect to the Enedis site). Is there any special operation I have to do ? I tried to write my username and pwd without quotes, between quotes, between simple quotes, I even tried several combinations (username between quotes, pwd without etc...) but it doesn't change anything. Am I missing something ?
Thanks !

@MartinHjelmare
Copy link
Member

Please open an issue if you suspect a bug. If you need help please use our help channels:
https://home-assistant.io/help/#communication-channels

Merged PRs should not be used for support or bug reports. Thanks!

@home-assistant home-assistant locked as resolved and limited conversation to collaborators Oct 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants