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

Create a new socket each time 'connect' is called #12

Merged
merged 5 commits into from
Feb 17, 2024
Merged

Conversation

akrabi
Copy link
Contributor

@akrabi akrabi commented Jan 5, 2024

Each call to connect will create a new socket (closing the old one) and use it to connect.
The purpose of this change is to allow recovery from a broken socket which is not possible without it (unless you create a new instance of the entire class).

Some additional changes:

  • Add missing locking for all_on_off and toggle_mute operations
  • Convert try/finally statements in favor of 'with' statements for locks
  • Prefix local method with double underscore, this helps see which methods are part of the supported library API.
  • Correct some spelling mistakes in logs and comments

@laf
Copy link
Owner

laf commented Jan 7, 2024

I don't use this anymore so I can't test it.

@acambitsis @altersis Any thoughts from either of you?

@akrabi
Copy link
Contributor Author

akrabi commented Jan 7, 2024

Thanks for responding @laf .
As for testing, I've been using this modified version on my Home Assistant installation for several days and so far it looks good.
I am planning to follow-up on this PR with an update to the HA integration as well.

@acambitsis
Copy link
Collaborator

I'm also not using this anymore so I also can't test.

@altersis
Copy link
Collaborator

altersis commented Jan 7, 2024

Hi Gents, happy new year! I'm still using this integration, and very happy that @akrabi had a chance to enhance it. I cannot test it today, but will certainly do it tomorrow and report back the results. Cheers!

@akrabi
Copy link
Contributor Author

akrabi commented Jan 9, 2024

Hey @altersis , have you had a chance to test these changes?

@akrabi
Copy link
Contributor Author

akrabi commented Jan 17, 2024

Hey, this change has been running on my HA installation for the past two weeks with no issues.
@acambitsis, do you still want to validate this on your setup?

@acambitsis
Copy link
Collaborator

@akrabi no need from my side. I have moved off russound a while back now. It was @altersis who has the ability to test, and can make a call if further testing is needed.

@akrabi
Copy link
Contributor Author

akrabi commented Jan 22, 2024

@altersis ?

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.

4 participants