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

Sonos binding: add new channel clearqueue #3740

Merged
merged 1 commit into from Jul 3, 2017

Conversation

Projects
None yet
4 participants
@lolodomo
Copy link
Contributor

commented Jun 25, 2017

Fix #2894

Signed-off-by: Laurent Garnier lg.hc@free.fr

Sonos binding: add new channel clearqueue
Fix #2894

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
@maggu2810
Copy link
Contributor

left a comment

If the "clear queue" channel is using a switch, shouldn't the binding update the state to off again if the command on has been received and the queue has been cleared?

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2017

I kept the same philosophy as for the other switch channels in the binding like for example the stop channel, we don't care the switch state.
But I agree that could be something to change for all channels of this kind.

@sjka

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

At least it is consistent then 😄 Makes sense imho.

Still I would like to understand whether we are missing something conceptually in the framework, or this binding is modeled in a creative way.

Do channels like this indicate a need for "write-only" channels which only accept commands but do not have a state (similar as trigger channels, just that the "event" is going the other way)? How and if to introduce such a channel kind is of course by far beyond this PR and should not be discussed here - I only want to understand this here and I would be fine with it how it is implemented currently.

Or is it rather that such a channel actually should be the representation of the device being in "cleanup" state for even a very short-lived moment? Then I'd agree it's weird that it remains in the ON state and should switch off again (consistently, i.e. for all these channels, best in a separate PR again).

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2017

Do channels like this indicate a need for "write-only" channels which only accept commands but do not have a state (similar as trigger channels, just that the "event" is going the other way)?

Isn't this already supported by the framework.
Configure the autoupdate to not be used for that channel, don't set a state in the binding itself and handle the command only. Sure, the state will be NULL all the time, but isn't that an indication that the state is not available at the moment?

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2017

I confirm it is a write only command channel without state representing device state.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 27, 2017

In the Sonos binding you have at least 10 channels of this kind. Sometimes you have another channel that reflects the state. For example, the control channel (player) is indirectly updated when using the stop channel as command.
There is no channel in the binding representing the state of the device playing queue.

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jun 28, 2017

So what is the status for this PR ?

Maybe a new bug should be declared if you think the Sonos binding have unexpected handling of channels ?

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Jun 28, 2017

@sjka What do you think about.
Isn't a channel that does never receive a state update already similar to a "write-only" channel?
Should we create a separate PR to use that approach (we need to stop the auto update for that channels) for some Sonos channels?
Can we continue with that PR as it uses the same mechanism already used by other channels of the binding (I don't know, but IIRC @lolodomo stated that above)

@sjka

This comment has been minimized.

Copy link
Contributor

commented Jun 29, 2017

As I understand:

I only want to understand this here and I would be fine with it how it is implemented currently.

And this holds true. No need to change anything here, I really did not intend to block this PR with it. And I'm not even sure whether we need such a dedicated write-only channel kind. Need to make up my mind. And of course, we can discuss it elsewhere.

This PR LGTM!

@sjka

sjka approved these changes Jun 29, 2017

@lolodomo

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2017

Can this PR be merged please ? I am waiting for it to implement #3741 that will require changes in the channels file too.

@sjka sjka merged commit d3a24bf into eclipse:master Jul 3, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details
@sjka

This comment has been minimized.

Copy link
Contributor

commented Jul 3, 2017

Oh sorry, yes, sure.
Thanks!

@lolodomo lolodomo deleted the lolodomo:clearqueue branch Jul 3, 2017

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Jul 4, 2017

And I'm not even sure whether we need such a dedicated write-only channel kind. Need to make up my mind. And of course, we can discuss it elsewhere.

Just as some random thoughts, before I forget to write them down:

  • I agree that this PR is in line with how it is done in the rest of the Sonos binding, so all fine.
  • In general, I think it is the wrong approach: We should rather have a single channel that accepts such kinds of commands (as the split into separate channels should usually be done by checking, which "states" are handled independently of each other - so as we do not have any states here, there is no need for a channel for each of them).
  • Currently, many developers model it as switches, because it is the easiest way to make the functionality available through UIs - this is a sign that we are lacking features regarding UI integration of other type than switched.
  • We should have something like "commandOptions", similar to the already existing "stateOptions". This meta-data could be provided to UIs so that they could render buttons for such features (similar as the "mapping" functionality of Switch widgets in sitemaps).
  • Administrative features (so nothing that the real end user wants to use on a daily basis) might also rather be modelled as a device management feature, once this is available.

Just my 2 cents for the discussion that might or might not take place "elsewhere" ;-)

@sjka sjka referenced this pull request Sep 15, 2017

Closed

Trigger channels #4272

@kaikreuzer kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.