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

Support loop_one mode #58

Closed
Ramblurr opened this issue Dec 21, 2022 · 9 comments · Fixed by #72
Closed

Support loop_one mode #58

Ramblurr opened this issue Dec 21, 2022 · 9 comments · Fixed by #72

Comments

@Ramblurr
Copy link

Ramblurr commented Dec 21, 2022

Roon's loop mode has three states: 'loop' | 'loop_one' | 'disabled'

source: https://roonlabs.github.io/node-roon-api/Zone.html

Currently this library only exposes loop and disabled

It would be great to have support for this in the API (and eventually HA).

@Ramblurr
Copy link
Author

I'm happy to write a PR for this.

What is this projects stance on breaking changes?

Should I break the function signature by changing the bool to a str type?

https://github.com/pavoni/pyroon/blob/master/roonapi/roonapi.py#L302

Or to maintain backwards compat should we support a boolean and string value?

@pavoni
Copy link
Owner

pavoni commented Dec 28, 2022

With only a couple of users - I don't think it's a big deal to change the API.

We should just make it clear in the release.

@pavoni
Copy link
Owner

pavoni commented Dec 31, 2022

As far as I can see the HA wrapper doesn't currently use Roon's repeat mode - and HA does support the 3 mode model.

So will be a good addition if you decide to do this.

@Ramblurr
Copy link
Author

Yea i'll write a patch in the new year!

@pavoni
Copy link
Owner

pavoni commented Feb 26, 2023

@doctorfree I'm tempted to make a breaking change to do this.

Will it break your current code?

Anything I can do to make it easier?

@doctorfree
Copy link
Contributor

@pavoni it would break the current RoonCommandLine support for repeat which I treat as a toggle. The changes I would have to make to support this change to the API are fairly simple and confined to a few lines of code. The only problem would be those users who upgrade the Python Roon API while running a version of RoonCommandLine prior to my changes to support this.

I'd say go ahead and add support for loop_one and I will update RoonCommandLine to support it as well.

@Ramblurr
Copy link
Author

Will it break your current code?
It would yes, but I can update my code easily enough when I update the pyroon dep. Thanks for asking.

Sorry I didn't get round yet to writing that PR, been busy with other things.

I'd say go ahead and add support for loop_one

+1

@pavoni
Copy link
Owner

pavoni commented Feb 27, 2023

@doctorfree if it helps I can flag you when I've merged the PR - but before I release the library.

@pavoni
Copy link
Owner

pavoni commented Feb 27, 2023

@doctorfree It was easy to make this backwards compatible - so shouldn't be an issue for you.

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 a pull request may close this issue.

3 participants