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

Toggle functionality for "boolean" types #4381

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

magx2
Copy link
Contributor

@magx2 magx2 commented Sep 15, 2024

Hey,

Writing DSL functions I found out that it would be really nice to be able to toggle types to opposite, i.e. You got command ON from item x and you want to send command OFF (opposite command) to item y:

val oppositeState = if(x.state==ON) OFF else ON
y.sendCommand(oppositeState )

vs

y.sendCommand(x.state.toggle)

WDYT?

Signed-off-by: Martin Grześlowski <[email protected]>
@magx2 magx2 requested a review from a team as a code owner September 15, 2024 08:49
@spacemanspiff2007
Copy link
Contributor

How about a TOGGLE command which can be sent to the corresponding item type?

@magx2
Copy link
Contributor Author

magx2 commented Sep 15, 2024

Is there a toggle command?

@spacemanspiff2007
Copy link
Contributor

Currently not - I am suggesting it might make sense to implement one.

@magx2
Copy link
Contributor Author

magx2 commented Sep 16, 2024

Currently not - I am suggesting it might make sense to implement one.

No way. I already saw long discussions about it. If you want you can do it

#947
eclipse-archived/smarthome#4638
https://community.openhab.org/t/controlling-a-dimmer-item-by-physical-buttons-various-questons-to-achieve-clean-solution/131128/15

@andrewfg
Copy link
Contributor

andrewfg commented Sep 16, 2024

Umm. I have an issue with your word 'toggle' .. its dictionary definition is "to switch a feature on a computer on and off by pressing the same button or key" .. in other words the 'toggle' operation executes TWO operations e.g. ON=>OFF=>ON.

So I suggest some other word like 'invert' or 'reverse' or 'flip' ..

@magx2
Copy link
Contributor Author

magx2 commented Sep 16, 2024

@andrewfg

I think you misunderstand the Cambridge dictonary (or I did).

Look at examples of usage:

  • Use this key to toggle between the two typefaces -> Use toggle method to toggle between states
  • By toggling this key, you can switch the italics on and off. -> By toggling this method, you can switch the state on and off

Edit:

And definition from Google (of course without link to source...)

obraz

@mherwege
Copy link
Contributor

I believe toggle is a verb in this context. It implies an action to take. This is not the case in what you coded. There is no state change involved in the original item. It shouldn't be a verb. What about something like inversed?
To be perfectly honest, I don't see the value. While it may simplify the rules code a little bit, it can also confuse, as is also apparent from the discussion on the terminology. It is not an attribute of the the original state, but rather something you want to apply to another item's (or the same item's) state. So I actually think we should not do this and keep it explicit in the rule. But that is my opinion on it.

@rkoshak
Copy link

rkoshak commented Sep 16, 2024

Umm. I have an issue with your word 'toggle'

At least in American English at least, "toggle" is the correct term for something that switches the current state of a switch regardless of the state it's already in. That's the common usage for that term. I cannot speak for other English dialects.

I imagine if a way to implement a TOGGLE command in a way that doesn't require modifyinmg all the add-ons that implement Switch Channels it might be more acceptable than where the past discussions went. And most of the controversy in the ESH thread at least was around whether discussion about whether it was a good idea or not should be allowed or if the PR should just be merged without discussion.

I personally am not convinced by the arguments against a TOGGLE command. It should be up to the admin of each individual OH instance to decide whether TOGGLE is a dangerous command or not and configure their OH instance accordingly.

y.sendCommand(x.state.toggle)

You'd still need a test for NULL and UNDEF as that would obviously fail with an error. I'm not sure this approach buys us much. In fact

y.sendCommand(if(x.state == OFF) ON else OFF)

is preferable becuase if the state is NULL or UNDEF the switch will be commanded OFF instead of throwing an error.

@jimtng
Copy link
Contributor

jimtng commented Nov 26, 2024

JRuby helper library currently already offers a toggle method for SwitchItem. If the item's state is not ON (which includes OFF, NULL, and UNDEF), send an ON command. Otherwise, send an OFF command.

It also overrides the ! operator for OnOffType to invert the state, so !ON returns OFF and vice versa.

JSScripting has sendToggle() method somewhere too although I'm not familiar with its exact behaviour.

@spacemanspiff2007
Copy link
Contributor

spacemanspiff2007 commented Nov 26, 2024

Toggling on NULL and UNDEF is something I would not expect.
If a lamp is on, I start openHAB and then call toggle a ON command will be sent because the lamp state has not yet updated and is NULL -> the toggle command does the wrong thing.

The toggle scope is limited to ON -> OFF and OFF -> ON.
All other states should do nothing and/or throw an error.
That's how HABApp implements it.

That the suggestions above are wrong is imho the only reason to standardize the behavior:
Avoiding pitfalls in edge cases.

@jimtng
Copy link
Contributor

jimtng commented Nov 26, 2024

Toggling on NULL and UNDEF is something I would not expect.
If a lamp is on, I start openHAB and then call toggle a ON command will be sent because the lamp state has not yet updated and is NULL -> the toggle command does the wrong thing.

What's a practical example of when a script would issue a toggle command?

It is usually called by a momentary button press: press it to turn it on, and press it again to turn it off, and press it again to turn it back on. In this instance, physical light is already on, openhab just loaded, the item state is still NULL, so you press the button, it would send ON command, nothing happens (because it's already on). So user presses the button again, but this time because the item state would've already been initialised (either by item autoupdate, or by sending the command, the item would poll the current state), so the subsequent press turns it off.

Now if you instead throw an error, the item state will remain NULL, and the button is not "working" and you'll have to wait until the item state gets updated by the binding.

@spacemanspiff2007
Copy link
Contributor

I get why you would choose the pragmatic approach.
However you make a lot of assumptions and it's not guaranteed that these assumptions are always true.
But if I call a toggle method I expect it to toggle every time, not most of the time.
Contrary to commands like ON and OFF toggle is not stateless, so it by definition it requires that the current state is known.

@magx2
Copy link
Contributor Author

magx2 commented Nov 26, 2024

Contrary to commands like ON and OFF toggle is not stateless, so it by definition it requires that the current state is known.

To be fair in this use case ON and OFF are also not stateless cause you need to know actual state to send proper command:

val command = if(state === ON) OFF else ON 

This rule will suffer same problems as toggle method

@magx2
Copy link
Contributor Author

magx2 commented Nov 26, 2024

What's a practical example of when a script would issue a toggle command?

I have a physical button that only clicks - it does not stay in on state, just send and one time event with on and then goes off.

@jimtng
Copy link
Contributor

jimtng commented Nov 26, 2024

But if I call a toggle method I expect it to toggle every time, not most of the time.

"most of the time" implies that the command is unreliable: sometimes it works, sometimes it doesn't.

The implementation I described would only "not toggle the actual device" during the initial load and only if the current state is unknown. If the state is known prior to the call, it would behave the same way, with the same reliability of toggling every time too.

The only difference is that it handles an unexpected state in a pragmatic way, so that user need not code additional workarounds in most cases.

Should they insist on not toggling when the state is unknown, simply add a check for that

if MySwitch.state? # is the state known?
  MySwitch.toggle 
else
  logger.info "Sorry you cannot toggle MySwitch yet because its state is unknown."
end

As a result, when you press the button, nothing happens, the light stays ON (which is the same behaviour anyway). And even if you pressed the button 10 more times, as long as the device state remains unknown, no "toggling" would happen. That's far from "I expect it to toggle every time". So which is worse?

Either implementation still allows the user to achieve either behaviour, with or without some additional code. At the end of the day it's a design decision.

Getting back to this PR though: it would introduce another issue

MySwitch.command(MySwitch.state.inverse()) would fail if MySwitch.state is NULL / UNDEF because you haven't defined UnDefType.inverse(). PS I cal it inverse rather than toggle here.

@magx2
Copy link
Contributor Author

magx2 commented Nov 26, 2024

I just found that JS rules also has toggle option: https://github.com/openhab/openhab-js/blob/main/src/items/items.js#L341

@spacemanspiff2007
Copy link
Contributor

To be fair in this use case ON and OFF are also not stateless

The ON and OFF command is stateless since it will always turn on / turn off.
Toggle is not stateless because the new state depends on the current state.

"most of the time" implies that the command is unreliable: sometimes it works, sometimes it doesn't.

Control question:
If the device is on and the item state is NULL and I call the toggle method does the device output toggle?
Typically most of the time the state is not NULL so the toggle method works most of the time.


You always assume that the target state should be on.
What if I have e.g. a pump? Maybe it would make sense to turn it off?
Maybe I run software pwm for underfloor heading in rule - depending on the logic of NULL handling the output percentage will be inverted (e.g. suddenly 70% instead of 30%).

it's a design decision.

Correct

@magx2
Copy link
Contributor Author

magx2 commented Nov 26, 2024

To be fair in this use case ON and OFF are also not stateless

The ON and OFF command is stateless since it will always turn on / turn off. Toggle is not stateless because the new state depends on the current state.

You need to know current state to send ON/OFF, so (in this usecase) it's not stateless. You have quoted only half of my message, the other half explained what I've meant.

@rkoshak
Copy link

rkoshak commented Nov 26, 2024

Nothing suggested here eliminates the edge cases. They all still exist no matter the approach. The design decision should be to handle the most common edge cases without imposing extra work on the end user.

I think the use case where the TOGGLE command is expected to send a command to the device every time is the most common so I would choose it as the default (i.e. send ON command if the Item is UNDEF/NULL). If I press my garage door opener or a momentary button to toggle a light, if nothing happens, I'll press it again.

Wanting the TOGGLE command to do nothing or error if the Item is NULL/UNDEF is IMO the less common use case. However, just because it is less common doesn't mean that an approach should be chosen that precludes handling that use case. And I would even go so far as to say that handling it should be no more involved than the if statement oneliners we are trying to replace with this issue (i.e. setting up a proxy Item to intercept the TOGGLE command in a rule is significantly more involved than the if statements we have now).

Given that jRuby, JS Scripting, and HABApp all support toggle shows there is a demand for this capability (I don't think the JS Scripting capability is in the docs though). But it is worth ensuring we don't impose more work on a subset of users than they have now.

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.

6 participants