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

Klf200 class with new frames, reorganisation of files #56

Merged
merged 59 commits into from Dec 21, 2020
Merged

Klf200 class with new frames, reorganisation of files #56

merged 59 commits into from Dec 21, 2020

Conversation

icommitdesnet
Copy link
Contributor

No description provided.

@Julius2342
Copy link
Owner

Julius2342 commented Nov 24, 2020

uh, this is a large one... :) Thank you very much!

The PR is marked as draft - tell me when the PR is in a state when it is worth having a deeper look.

Best

@icommitdesnet
Copy link
Contributor Author

Sorry - smaller chuncks in PR next time
Build check failes. Have to adapt Imports of test modules/classes too. Hope to get that right

Greets

@coveralls
Copy link

coveralls commented Nov 26, 2020

Coverage Status

Coverage increased (+0.01%) to 81.631% when pulling 804ccfa on icommitdesnet:master into f579c06 on Julius2342:master.

@@ -39,7 +39,9 @@ async def run(self, wait_for_completion=True):

def __str__(self):
"""Return object as readable string."""
return '<Scene name="{0}" ' 'id="{1}" />'.format(self.name, self.scene_id)
return '<{} name="{}" id="{}"/>'.format(
Copy link
Owner

Choose a reason for hiding this comment

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

What about using fancy python format strings here? (I would also be fine if we do this in a separate step)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hope, i get you right.
it was inconsistent with single or double quotes (see eg. pyvlx/api/frames/frame_set_node_name.py)
i added type(self).name is because i am lazy and dont want to type too much

seperate steps is maybe the main thing in this pr. too many things together

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, fully agree. It is a good improvement. I just thought of using:

return f'<{type(self).__name__} name="{self.name}" id="{self.scene_id}"/>'

But im not sure if this is an improvement...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nah. this is a mess. i should be even more strict
return '<{0} name="{1}" id="{2}"/>'.format( ...

pyvlx/pyvlx.py Outdated Show resolved Hide resolved
Copy link
Owner

@Julius2342 Julius2342 left a comment

Choose a reason for hiding this comment

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

Ugh, what a large PR! Thank you, all looks good to me.

I assume you tested all the changes locally (besides unit tests...)?

@Julius2342
Copy link
Owner

Another question, do you have any idea how to approach this "KLF 200 hangs" problem?

@icommitdesnet
Copy link
Contributor Author

icommitdesnet commented Dec 4, 2020

Ugh, what a large PR! Thank you, all looks good to me.

I assume you tested all the changes locally (besides unit tests...)?

those were easy to test with my klf200

@icommitdesnet
Copy link
Contributor Author

Another question, do you have any idea how to approach this "KLF 200 hangs" problem?

#30 ?

I currently use https://github.com/MiSchroe/ioBroker.klf200 which is restarted by default at 03:00 every day. maybe the same problems there?

@Julius2342
Copy link
Owner

Another question, do you have any idea how to approach this "KLF 200 hangs" problem?

#30 ?

I currently use https://github.com/MiSchroe/ioBroker.klf200 which is restarted by default at 03:00 every day. maybe the same problems there?

Yup. This seems like the same root cause. There is also a PR in Hass requesting this feature: https://github.com/home-assistant/core/pull/43198/files

@icommitdesnet
Copy link
Contributor Author

Another question, do you have any idea how to approach this "KLF 200 hangs" problem?

#30 ?
I currently use https://github.com/MiSchroe/ioBroker.klf200 which is restarted by default at 03:00 every day. maybe the same problems there?

Yup. This seems like the same root cause. There is also a PR in Hass requesting this feature: https://github.com/home-assistant/core/pull/43198/files

my first considerations:
i cannot reproduce these symptoms with tcp connection only. good.
according to other posts, it needs ungracefully finished ssl connections (dont know ssl connection states and those sessionids, thou)

but: its not the same situation as restart of HA, but i can even influcence an active connection from pyvlx or iobroker klf200.0 by killing just another connection:
a) kill gnutls-cli -V -s -p 51200 myklf may need some tries (<5)
b) wait for pyvlx heartbeat: <FrameGetStateRequest/> does not timeout and it is not possible to exit my main loop
c) try to connect with openssl: fails at ssl handshake. openssl seems to wait forever here.

$ openssl s_client -connect myklf:51200 -state
CONNECTED(00000003)
SSL_connect:before SSL initialization
SSL_connect:SSLv3/TLS write client hello

d) wait a bit and try openssl again -> works

@icommitdesnet
Copy link
Contributor Author

Another question, do you have any idea how to approach this "KLF 200 hangs" problem?

#30 ?
I currently use https://github.com/MiSchroe/ioBroker.klf200 which is restarted by default at 03:00 every day. maybe the same problems there?

Yup. This seems like the same root cause. There is also a PR in Hass requesting this feature: https://github.com/home-assistant/core/pull/43198/files

my first considerations:
i cannot reproduce these symptoms with tcp connection only. good.
according to other posts, it needs ungracefully finished ssl connections (dont know ssl connection states and those sessionids, thou)

but: its not the same situation as restart of HA, but i can even influcence an active connection from pyvlx or iobroker klf200.0 by killing just another connection:
a) kill gnutls-cli -V -s -p 51200 myklf may need some tries (<5)
b) wait for pyvlx heartbeat: <FrameGetStateRequest/> does not timeout and it is not possible to exit my main loop
c) try to connect with openssl: fails at ssl handshake. openssl seems to wait forever here.

$ openssl s_client -connect myklf:51200 -state
CONNECTED(00000003)
SSL_connect:before SSL initialization
SSL_connect:SSLv3/TLS write client hello

d) wait a bit and try openssl again -> works

this is exactly what

ConnectionAbortedError: SSL handshake is taking longer than 60.0 seconds: aborting the connection

says in #30

setting ssl handshake timeout and permanently retrying should do the job imho

Copy link
Collaborator

@pawlizio pawlizio left a comment

Choose a reason for hiding this comment

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

There are a lot of changes in this one PR.

leave_learn_state = LeaveLearnState(pyvlx=self.pyvlx)
await leave_learn_state.do_api_call()
if not leave_learn_state.success:
PYVLXLOG.warning("Unable to leave learn state")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a reason why some methods raise an exception and some only log a warning? I think this can be aligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aligned: raise pyvlx exceptions on api unsuccessful
fixed: LeaveLearnStateConfirmationStatus does not influcence Status
but arises question: should GW_ERROR_NTF be handled in any way?

pyvlx/pyvlx.py Outdated Show resolved Hide resolved
@Julius2342
Copy link
Owner

@icommitdesnet : Just tell me when you are ready, then I do a last round of reviews...

@icommitdesnet
Copy link
Contributor Author

@icommitdesnet : Just tell me when you are ready, then I do a last round of reviews...

I think this one is ok now.

@Julius2342 Julius2342 merged commit a1c4419 into Julius2342:master Dec 21, 2020
@Julius2342
Copy link
Owner

Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants