REST API: Extend support for TOGGLE command

[pfink]

TOGGLE command works now with Player + Group items as well as with any other item that accepts toggleable commands

Signed-off-by: Patrick Fink mail@pfink.de

[pfink]

Build error does not seem to be related to this PR...

[maggu2810]

Your change itself looks good to me.

But I am wonder why we support a magic "toggle" command by the REST interface only / at all.

[pfink]

@maggu2810: Then please open a separate issue and/or forum thread for this question and I'm happy to contribute to this discussion. But what I really don't like and what I have seen now several times is that because of small PRs people get suddenly aware of a certain topic and open fundamental discussions or demand things from the contributor that were not there before. And suddenly, the PR is blocked and - in the worst case - idles for a very long time.

Accepting a small improvement on an existing concept does not mean that you have to agree with this concept and it does also not mean that this concept cannot be revised anymore in the future. So calling a concept into question, as reasonable as it might be, should not be a reason to block small improvements on it.

[maggu2810]

Let's first add this comment: The following does not need to fit to your changes.

If a code is changed by a PR because a contributor wants to improve a situation it runs into and someone thinks that

• the code shouldn't be part of the repo at all
• the code should be cleaned up another way
• ...
  it is IMHO a good reason to start a discussion and perhaps block a PR.

Why should it be a good thing to merge a PR just to drop that change a day later because we agreed in a newly created issue that we don't like to support that code anymore?

[pfink]

Why should it be a good thing to merge a PR just to drop that change a day later because we agreed in a newly created issue that we don't like to support that code anymore?

Because for that time period until it is dropped, we have a better code than without the PR. And be honest: In most cases, PRs are not blocked for just one day, but for weeks, months or even years. Even if we agree tomorrow that refactoring is necessary, it'll not be done tomorrow. And until it's done, I see absolutely no reason why not to live with the improved existing code meanwhile.

[htreu]

The toggle command is part of the item REST interface from the very beginning of ESH history. There is a good chance that clients out there are using it. I´m with @pfink that improving this part makes sense and that further discussions are better off in a separate issue.
@maggu2810´s arguments hold true for any PR though. In this particular case I would give it a go and merge.

[kaikreuzer]

The toggle command is part of the item REST interface

This is not correct. The interface (namly the swagger annotations in that case) say:
"valid item command (e.g. ON, OFF, UP, DOWN, REFRESH)"

"toogle" is NOT a valid item command.
So we are talking here about a hidden internal feature that is not officially supported. Afair, this hack was introduced when the Android app added NFC support and somehow required a way to toggle as it was to cumbersome to first request the item state.

because of small PRs people get suddenly aware of a certain topic

No, people get aware that internal hacks that should have been removed are being used by others. Therefore, we are not talking about an improvement of a feature, but about manifesting a hack and making it a public API. I do not see any good argument, why this should be done without any discussion.

[pfink]

... as it was to cumbersome to first request the item state.

Exactly, that's the use case. Many small & simple apps do not offer a feature for implementing such logic, they just allow to do a single REST request. So toggle is not something evil, it makes life easier for users of the REST API.

[kaikreuzer]

Do you have an example for such an app / use case (apart from the one I mentioned)?

Just as some background: When defining the item types back in 2010 I deliberately didn't add a TOGGLE command besides ON and OFF for switch items as my intention was that it is better for external apps to know what it is doing - by sending an ON or OFF, the intended outcome is clear, but this isn't the case for a TOGGLE, so this can be a rather dangerous operation - and also one which actually cannot executed, if the current item state is unknown.

[pfink]

I think many mobile apps for smart home gadgets offer the possibility to make a single REST request. E.g. I remember that the Flic app offered such a possibility, or also Tasker which can be used for many automations based on mobile phone activity (Android). Currently, many Bluetooth based smart home gadgets are entering the market which mostly offer Android/iOS apps supporting either IFTTT or single REST requests or Tasker integration. Probably not all of them will get a binding, so the REST API would be an easy integration possibility without having to use the cloud. Of course, TOGGLE is architecturally not perfect, but I think most people just want to have something easy. If it does not work perfect, they have still the possibility to use ON and OFF / UP and DOWN / PLAY and PAUSE instead.

[ThomDietrich]

@kaikreuzer

Do you have an example for such an app / use case

I have such a use case and came to this issue because if it. I would also like to see a TOGGLE (REST/rule) command. You might have a point, that some applications should know what they are doing exactly, however other applications don't/can't or shouldn't.

My use case: (On my PC I've got a multimedia keyboard with a lot of useless keys for e.g. next song. I've developed a small AutoHotKey script to control openHAB with those.)
When I press the "next song" key I want to toggle my lights. No further logic needed or beneficial for the use case. Could be a simple oneliner. Instead I have to first send an HTTP request, evaluate the response, and construct a corresponding command. Took me quite some time to get right inside the horrible scripting options of AutoHotKey, which I didn't use before.

Btw. I very much agree with @pfink here and also saw quite a few issues with simple ideas buried by big discussions, never to be implemented. Just to mention one example: #4583

[htreu]

In the meanwhile I had the chance to understand and test the concept of profiles a little better: The serial button binding does exactly what we try to achieve here by sending a trigger event which is turned into a toggled command for the linked item.
So now I agree with @kaikreuzer that the current implementation is merely a hack. Maybe we can introduce the trigger concept into the REST API and make use of the new infrastructure.
@sjk: any thoughts on that?

[htreu]

Btw: the big discussions are the most powerful tool we have to build ESH as a solid and consistent platform. In case discussions get stuck its either no one cares or its not as easy as it seems. In case of #4583 its the latter.

[maggu2810]

Off-Topic:

and also saw quite a few issues with simple ideas buried by big discussions

I understand that it is annoying if you wait for something and it ends up into a long discussion.
But what is your suggestion?
Should we merge every PR without any think about how it fits into the general concept or should we state "no, we don't like that" and stop posting any further comments?

I'm annoyed by long discussions sometimes, but at the end of the day it's "always" better.

---

Using a toggle profile sounds (IMHO) like a good idea.

[pfink]

@htreu @maggu2810: The point was not that discussions are something bad and should be repressed. I fully agree with @htreu that these discussions will lead to a solid and consistent platform and it's necessary to do them. The point of criticism was that currently, while a discussion is running, PRs are blocked that improve a really bad situation so that we have at least just a bad situation.

In this case, it did already lead to the situation that we have meanwhile another ESH release which contains a very inconsistent functionality. If we went for my initial suggestion, we would have now a more consistent ESH release and it would not have prevented anybody from discussing and implementing a better solution.

[sjsf]

Maybe we can introduce the trigger concept into the REST API and make use of the new infrastructure.
@sjk: any thoughts on that?

Yes - for the sake of simplicity I created #4897 to see if that would work. Having such a profile is the least problem we have. The bigger question would be how events can get into the system. But I don't want to hijack this PR with this question, therefore feel free to tear mine apart - maybe we all get happily back to this quick solution in the end 😉

[sjsf]

So, just to report back here: My little side-experiment pretty quickly turned out to be a dead-end. Therefore we're back here, left with a couple of options:

1. Drop this PR, leave it as is, never talk about it again
2. Accept this little PR, let it stay an "unofficial" easter-egg-like feature on a use-at-your-own-risk basis, being not much worse than before as the actual harm was done with the initial contribution
3. Make the toggle-"command" become officially supported in the REST API (or even as a proper Command) with documentation and all the full glory
4. Drop it entirely from the code and tidy up the legacy

Frankly spoken from reading the above statements it looks like the odds are on option 4. This however leaves room for an extension on e.g. the Eclipse Marketplace which adds a REST endpoint for exactly this feature - with all its nice simplicity but also its caveats, of course. Wdyt?

[htreu]

@SJKA, nice round up :-)
Its a pity that toggle/trigger profiles do not work out from the REST endpoint side. I vote for option 4 and the creative power the missing API will create to solve this issue.

[ThomDietrich]

I vote for 2 if not even 3. There are real use cases for this feature (see above) and no impacts on other functions. If your device or logic can't TOGGLE well just don't use it...
The only real counter-argument was the one by @kaikreuzer:

by sending an ON or OFF, the intended outcome is clear, but this isn't the case for a TOGGLE, so this can be a rather dangerous operation - and also one which actually cannot executed, if the current item state is unknown.

which I can not agree with. How can toggle be especially "dangerous"? That depends on the device and use case (see). doomsday_machine.sendCommand(ON) is also dangerous.
As for the second part: if the state is unknown TOGGLE will not switch (OR will start at OFF or similar). That is just a matter of definition.

[sjsf]

How can toggle be especially "dangerous"?

It's particular dangerous in all scenarios where you don't have visual control.

Imagine that in the best intention to switch off the bedroom lights from when leaving the house, you accidentally switch them on because there were switched off after you left the room and you didn't know -> the new-born baby wakes up -> your wife gets pretty upset -> and voila, there is the dangerous situation 😄

If you habe multiple devices connected to the same item group, "toggling" only is fun as long as all devices always receive their latest commands.

Also, think about race conditions: Two OFFs at about the same time go unnoticed. Two "toggles" are not.

So, seriously, it all boils down to "toggle" lacking idempotency and therefore not being a good candidate for being used as commands in (home) automation.

[ThomDietrich]

@SJKA I tried to clearly distinguish between having and wanting to use TOGGLE. In your example one would of course send a clear OFF command. Every user with a small sense of logic knows that. In other examples TOGGLE might be a valid and useful option. My keyboard shortcut example up above is such an example.

I am totally on your side regarding possible misuse of the TOGGLE command but why would we not at least offer the option as a possibility riddles me.

Please correct me if I'm wrong but I don't see race conditions on application automation layer that are not user-fault. Once again, if the TOGGLE command is not a practical option the user is at liberty to use the definitive commands.

[maggu2810]

I vote for option 4, but I also think that adding a new REST endpoint implementation to the marketplace would make sense. So, the one that want to use a toggle REST endpoint feature could install that support from the marketplace and the official framework does not contain any undocumented easter egg (and no its not about adding the documentation 😉 ).

IIRC @kaikreuzer told us recently that the marketplace only supports bindings and rule templates. So would it make sense to add more types of extensions, too?

[pfink]

It'll not surprise you that I'm with @ThomDietrich and would vote for option 3. What will may surprise you in fact I'm always a big advocate of coherent APIs. A very important thing about programming APIs is that you don't let use cases disappear if you don't include them into your APIs. Don't handling them within the API means that they will be handled outside the API which will mostly lead to a more inconsistent implementation landscape in the end.

The only real argument apart from "I don't like the feature" yet is that it's unclear what to do with items that have an UNDEF state. The point is, I don't see a reason why this decision is better made on client-side. Being honest, most users/developers will just don't care about this in their implementation and the behavior will be just random depending on the order of if statements used. Vice-versa, when a developer wants or needs to care about it, the availability of TOGGLE will not prevent him to do so. In conclusion, not offering the TOGGLE command will IMHO not lead to better client-side toggle implementations as implied by some posts here.

"It could be dangerous, imagine that" is no argument for me as everything can be dangerous if used wrong. Rules are a much more dangerous feature from this perspective. "Imagine you have to implement 20 rules for every item where you want to use TOGGLE to bypass the lack of this feature, and you make copy&paste errors..." - THIS will drive your wife crazy ;-)

[maggu2810]

The only real argument apart from "I don't like the feature" yet is that it's unclear what to do with items that have an UNDEF state.

What about #4638 (comment)

[pfink]

@maggu2810: What is the argument here? That we should not support this feature because it is currently not supported? Sounds like a vicious circle for me.

[maggu2810]

This is not correct. The interface (namly the swagger annotations in that case) say:
"valid item command (e.g. ON, OFF, UP, DOWN, REFRESH)"

"toogle" is NOT a valid item command.

The API refers to ESH command types (see the class hierarchy of the Java command interface). There is no ToggleType that implements the Type interface.
This REST API endpoints is not about string commands that could be useful but about the command types that exists in the whole framework.

[maggu2810]

To make it clear, I veto against a toggle command that exists on the REST interface only and as long as it is not an official command (and we are refer to the send a supported item command REST endpoint at all).

The support for an official toggle command type that will be supported on all interfaces (Java, Rules, REST) is something we could still think about and if there is a (nom dead end) PR we could discuss the related changes.

[pfink]

This REST API endpoints is not about string commands that could be useful but about the command types that exists in the whole framework.

IMO, REST APIs are always intended as an easy-to-use interface for a broad range of use cases, but not as a perfect 1:1 interface to an underlying framework. For the latter purpose, more complex web service architectures make much more sense. Example: You currently cannot define data types at all, it's always String. E.g. if you pass a Number to a Group item with no data type via REST, it's currently always recognized as a String command, not as a Number.

[kaikreuzer]

I agree that a toggle command would probably be the cleanest solution, but it also completely breaks compatibility, because all 150+ bindings out there would be expected to implement this command and I doubt that we would manage to introduce it in a clean way without having to touch all those. I doubt that this little (and yes, risky) feature would be worth the effort - and I am not even convinced that it is a good solution.

Looking at it, I feel it is actually a very similar discussion as #4748: We talk about a piece of logic (reading state -> inverting it -> sending command), which is not at all specific to a certain binding, but can be applied to any of them. Thus, this logic should imho not be coded in the bindings (which would be the case if we introduced a toggle command). I'd therefore rather see this positioned near the rule engine, which is our means to deal with "pieces of logic".

[pfink]

I fully agree that it makes completely no sense to introduce code duplication to 150 bindings ;-)

[kaikreuzer]

To make it clear, I veto against a toggle command that exists on the REST interface only and as long as it is not an official command (and we are refer to the send a supported item command REST endpoint at all).

I think with this statement, we can close the PR as it is in any case. As ESH is EOL, everybody is free to revive the discussion over at https://github.com/openhab/openhab-core.