There has long been a code fragment floating around these forums, and it’s broken in a somewhat unobvious (to many) way. While I’ve noticed this in the past when the code fragment has been reposted, I’ve never really chimed in to point out the error (that little fellow on my shoulder whispering “stay out of it, rigpapa”), but in light of other recent circumstances, I now realize that that choice was an error.
So, here is my treatise on what I will call “the allow
meme” that has become ubiquitous in people’s scene Lua code here in the community.
Background… we’re talking about variants of this particular code fragment, intended to be used in scene Lua as a way to unconditionally turn off the scene actions under certain conditions. That is, the intent is that setting (explicitly or by formula) allow = false
will have the scene actions not execute regardless of the result of an accompanying test condition. Here’s typical example code using temperature, although time of day is also commonly seen:
-- Run scene actions only when temp between 68 and 72 and virtual switch is ON (never runs when switch OFF).
local tCurrent = luup.variable_get( "urn:upnp-org:serviceId:TemperatureSensor1", "CurrentTemperature", someTempSensor )
local tLow = 68
local tHigh = 72
-- allow: scene runs when true, false blocks it [yeah, no, it doesn't. Keep reading.]
local allow = luup.variable_get( "urn:upnp-org:serviceId:SwitchPower1", "Status", someVirtualSwitch ) == "1"
return (((tCurrent >= tLow) and (tCurrent <= tHigh)) == allow)
Seems legit. When allow
is true, in this case when someVirtualSwitch is on, then this Lua fragment will return true to the scene when the current temperature reading is between 68 and 72, and therefore the scene will run its actions. If the temperature is not in the range, it returns false, causing the scene to abort without running its actions.
The problem comes when allow
is false (in this case, when the switch is off). The intent is that when allow
is false, the return
statement always returns false and therefore the scene actions never run. It’s supposed to be a master switch for the scene actions.
That’s the intent. But that’s not how it works. This is not a master switch. It is, in fact, an inverter. That is, when allow
is false, it will cause the code to return false when the temperature is in the range, and true when outside the range–the inverse of its behavior when allow
is true. It does not return false always. And the result of this is that the scene actions will still run, they’ll just run when you really don’t expect them to.
Why is this?
Let’s simplify the range condition part, which is ((tCurrent >= tLow) and (tCurrent <= tHigh))
down to the two possible result values it can have: true and false. It’s always true when the current temp is between the low and high inclusive, and always false otherwise.
Given that, then when allow
is true–when the temperature is in range–the final test boils down to true == true
, because the temp is in range (first true) and allow is true (second true). Therefore, the result of the expression is true. If the temperature is outside the range, the final test is false == true
, which is, of course, false. So that’s fine.
Here’s where it goes wrong.
When allow
is false, and the temperature is in range, the final test is true == false
, which is false. But, when the temperature is outside the range, that final test becomes false == false
which is true, and that true will be returned to the scene, which then happily runs the actions, even though allow
is false.
Consider how potentially dangerous that could even be: the code will let the scene actions run when allow
is false (the expectation that no scene actions run ever) and when the temperature is outside the range that would normally cause the scene actions to run. If it’s controlling a heater, for example, it could turn it on and leave it on long enough to start a fire, when the user’s expectation is that the code is not controlling the heater at all.
There are several ways to properly structure this code to get the desired result. One is simply:
if not allow then
return false
end
return tCurrent >= tLow and tCurrent <= tHigh
The first block can also be written as one line if not allow then return false end
, if you’re into the whole brevity thing. Either way, the separate, explicit allow
test, done first, will cause the code to take the conditional branch and return false whenever and always if allow
is false. If allow
is true, then the returned value is the result of the temperature range test expression, whatever that might be.
The other way, if you prefer to write the return expression on one line, is something like this:
return allow ~= false and (tCurrent >= tLow and tCurrent <= tHigh) or false
If we follow from left to right, this abbreviated/boolean-ated code says "if allow is not false (i.e. true) then return the result of the temperature range test expression, otherwise return false). It’s a bit more confusing for relative newcomers to look at, but it’s succinct and it will work as intended. But if clarity is your preference, I would use the first example.
Like most memes, the “allow meme” needs to die, or at least, we need to understand how it’s wrong, and ignore it accordingly in favor of better options.