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

Fix linky sensor login error #17110

Merged
merged 8 commits into from
Oct 20, 2018
Merged

Fix linky sensor login error #17110

merged 8 commits into from
Oct 20, 2018

Conversation

Debaru
Copy link
Contributor

@Debaru Debaru commented Oct 3, 2018

Description:

Fix linky sensor login error .

Related issue (if applicable): fixes #17077

Checklist:

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

@homeassistant
Copy link
Contributor

Hi @Debaru,

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!

from pylinky.client import PyLinkyError
from pylinky.client import LinkyClient, PyLinkyError

client = LinkyClient(self._username, self._password)
Copy link
Contributor

Choose a reason for hiding this comment

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

You probably need to close the session after fetch_data().
See also pyLinky main.

    finally:
        client.close_session()

homeassistant/components/sensor/linky.py Outdated Show resolved Hide resolved
hobbe and others added 2 commits October 4, 2018 09:56
Adding following enhancements:
* Make sure the platform loads correctly by making the first API request in setup_platform.
* Close the session after each API call.
* Use timeout parameter everywhere.
Make platform fail-safe
from pylinky.client import PyLinkyError
from pylinky.client import LinkyClient, PyLinkyError

client = LinkyClient(self._username, self._password, None, self._timeout)

Choose a reason for hiding this comment

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

line too long (81 > 79 characters)

@MartinHjelmare
Copy link
Member

Please add to the PR description, why this change solves the issue.

@Debaru
Copy link
Contributor Author

Debaru commented Oct 5, 2018

Sorry.

  • commit 164d82e fix @fabaff request. @hobbe add timeout parameter and check sensor works in setup_platform.
  • commit 36676e0 fix Fix Hound CI error: line too long.

Is it okay ? First PR here =)

from pylinky.client import PyLinkyError
from pylinky.client import LinkyClient, PyLinkyError

client = LinkyClient(self._username, self._password, None,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to create a new client on every update?

Copy link
Contributor Author

@Debaru Debaru Oct 5, 2018

Choose a reason for hiding this comment

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

First solution use one client, created in setup_platform, but it's seems to not work (Login error).
With a new client on every update, that resolve the issue.

Copy link
Member

@MartinHjelmare MartinHjelmare Oct 5, 2018

Choose a reason for hiding this comment

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

It would be good to know why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree with you.
My solution was a hotfix what work for me and closing error login issue, it's why i share it. In wait I've found a better solution, do you want I cancel my PR ?

Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't necessarily close this. Let's hear what other reviewers think. If this PR fixes the problem, that's a first step.

Copy link
Contributor

Choose a reason for hiding this comment

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

I can confirm that this fixes the original issue.

I have tried to re-use the client, but the original error occurs again.
It seems that the session cannot be re-used. Then this might be an issue in the pyLinky library.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After some tests, I agree with @hobbe : pyLinky library session cannot be re-used.
So, for now, we need to re-create an client at each update.

Copy link
Member

Choose a reason for hiding this comment

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

I suggest that you open an issue for pyLinky to get it fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've open a PR on pyLinly library to improve client (Pirionfr/pyLinky#5).

@mbo18
Copy link
Contributor

mbo18 commented Oct 9, 2018

Can this fix be pushed to 0.80 as this component is not working since its release in 0.79?
Thank you

@hobbe
Copy link
Contributor

hobbe commented Oct 9, 2018

@mbo18, see this temporary workaround.

@@ -75,11 +75,12 @@ def unit_of_measurement(self):
@Throttle(SCAN_INTERVAL)
def update(self):
"""Fetch new state data for the sensor."""
from pylinky.client import PyLinkyError
from pylinky.client import LinkyClient, PyLinkyError

Choose a reason for hiding this comment

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

'pylinky.client.LinkyClient' imported but unused

@Debaru
Copy link
Contributor Author

Debaru commented Oct 20, 2018

Now, we are using last pyLink version (0.1.8) which allow to reuse client session.
Available if you have some questions.

Copy link
Member

@MartinHjelmare MartinHjelmare left a comment

Choose a reason for hiding this comment

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

Great!

@MartinHjelmare MartinHjelmare merged commit e980d1b into home-assistant:dev Oct 20, 2018
@ghost ghost removed the in progress label Oct 20, 2018
@balloob balloob mentioned this pull request Oct 26, 2018
@CygaLV
Copy link

CygaLV commented Jan 27, 2019

Hi !
It seems that the problem reappeared !
It worked fine for me until recently (1 or 2 weeks I guess).
And now it fails saying my password and user are wrong.
PyLinky version I have is 0.1.8. It is the latest right ?
I'm using it on a Synology NAS DS216j.

I modified the python source file to expose the user and names, and I can guarantee they are valid.

Trying to trace the execution, it seems that it fails finding the cookie iPlanetDirectoryPro. I checked with the debug options of my web browser, and locally I've got this cookie...
I've also checked the SunQueryParamsString, and it seems that it is good.

One thing though, now, when connecting on the Enedis webpage, it doesn't bring you to your user space, but it rather do several redirections (during these redirections the cookie seems to be not accessible) and brings to you to the website welcome page (iPlanetDirectoryPro) is then available.

Am I the only one with this issue ?

@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 Jan 27, 2019
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.

Linky sensor cannot log in to Enedis
9 participants