-
Notifications
You must be signed in to change notification settings - Fork 18
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
Catch BrokenPipeError and try to reconnect #59
Conversation
As I wrote in the HA ticket, I do not see this issue my self For a patch to be valid, the code to be altered should be at about line 120, which is "library exception wrapper code for the library due to HA NAD integration limitations" time.sleep without a VALID reason will not be approved (good 2 have is not valid, then patch the integration) In general the telnet code is a hack due to issues in the HA NAD integration. ONE day I will address this, but probably not in 2023 |
Thank you for your feedback, and I am glad you did provide the telnet hack. As suggested the exceptions are now handled in the wrapper. It turned out it's sufficient just to close the connection on BrokenPipeError, ConnectionResetError exceptions, too. This solves the reconnection problem in my HA. |
Nice ! Except for the ",B" 'problem' this LGTM BTW, always a good idea, in python, to run pep8 and pylint on "ones changes" @joopert This patch looks good to me, but I cannot merge (no merge rights) @sputnik73 FYI, more steps to follow |
Thanks @andreas-amlabs @sputnik73 thanks for the improvement, I'll merge it, I cannot give you a date yet when I'll create a new release. |
Great, thank you for the quick review/merge and the helpful hints. |
Catch BrokenPipeError and try to reconnect (joopert#59)
I am using this python API with Home Assistant and it works fine with my NAD T758v3i.
However every time the telnet connection is lost (e.g. if the receiver was temporarily powered off by a external power plug), the API does not try to reconnect but instead permanently reports BrokenPipeError.
A detailed bug report is available at home-assistant/core#92303
I could solve the connection problem with this quick change. However I do not have experience with python and this api, so If you think this code is not useful that is fine to me of course.