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

Log out of Sagemcom client after every query #30

Merged
merged 6 commits into from Feb 6, 2022

Conversation

rhpijnacker
Copy link
Contributor

Since the F5359 device (KPN Box v12) only allows one client connected at a time, we need to logout after every query.

@rhpijnacker
Copy link
Contributor Author

Please note that this needs #18 to work properly.
Also, it needs the both iMicknl/python-sagemcom-api#107 and iMicknl/python-sagemcom-api#108 for the full workflow.

The current status: I'm seeing Devices and Entities get created.

@iMicknl
Copy link
Owner

iMicknl commented Dec 8, 2021

The current status: I'm seeing Devices and Entities get created.

Awesome! I wonder if we need to logout after every query, or just relogin() when the exception is thrown. Wouldn't that make it even easier? (and allows other routers to keep the session alive. My router for example can keep the session open, reducing the amount of logins and resources required).

@rhpijnacker
Copy link
Contributor Author

I wonder if we need to logout after every query

The way it is set up currently is that it is trying to log-in at least twice, which is causing the max-sessions exceptions.
Also, we would need to be sure that in case of a restart of HA, or some kind of error occurs the session is closed, otherwise the max-session exception occurs, starting a period of 5 minutes where the device is inaccessible.

Keeping the session open (in HA) will mean that it becomes impossible to log in to the device manually, which I find a big downside.

All in all, I find that logging out after doing the query keeps it very local in code and allows me to manually log in to the device (breaking HA for a short while, which is acceptable and automatically recovers once I manually log out).

or just relogin() when the exception is thrown

I'm not exactly sure what exception you are referring to (once we see the max-session error logging in again isn't possible).
Which scenario do you have in mind where this would work?

@rhpijnacker
Copy link
Contributor Author

@iMicknl Is there anything you would like me to update before moving this PR forward?

Copy link
Owner

@iMicknl iMicknl left a comment

Choose a reason for hiding this comment

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

Thanks! And sorry for the late merge.

I am not a huge fan of logging out after every query and I think that this perhaps should be an option (or auto detected based on the type / model / exceptions). But let's merge it to unblock you and the other user, and we can always see if other users face issues.

@iMicknl iMicknl merged commit 716cc22 into iMicknl:master Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants