search

joopert / nad_receiver

python api to connect to NAD receivers
MIT License
10 stars 18 forks source link

Catch BrokenPipeError and try to reconnect #59

Closed sputnik73 closed 1 year ago

sputnik73 commented 1 year ago

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 https://github.com/home-assistant/core/issues/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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

sputnik73 commented 1 year ago

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