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

Catch BrokenPipeError and try to reconnect #59

Merged
merged 4 commits into from
Aug 6, 2023

Conversation

sputnik73
Copy link
Contributor

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.

@andreas-amlabs
Copy link
Contributor

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

@sputnik73
Copy link
Contributor Author

sputnik73 commented Aug 5, 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.

@andreas-amlabs
Copy link
Contributor

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
Set tag, upload to pip (joopert), make change in nad integration to use new label (sputnik73)

@joopert
Copy link
Owner

joopert commented Aug 6, 2023

Thanks @andreas-amlabs
It seems the checks need to be improved, as it notices the errors (flake8) but they just pass. I'll merge it now and will need to fix flake8 issues later. Not sure why it is implemented this way.

@sputnik73 thanks for the improvement, I'll merge it, I cannot give you a date yet when I'll create a new release.

@joopert joopert merged commit 7c7074b into joopert:master Aug 6, 2023
2 checks passed
@sputnik73
Copy link
Contributor Author

Great, thank you for the quick review/merge and the helpful hints.

rrooggiieerr added a commit to rrooggiieerr/nad_receiver that referenced this pull request Feb 20, 2024
Catch BrokenPipeError and try to reconnect (joopert#59)
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.

3 participants