eclipse-archived / smarthome

Eclipse SmartHome™ project
https://www.eclipse.org/smarthome/
Eclipse Public License 2.0
866 stars 782 forks source link

Automation: ability to negate a condition #4101

Open ghys opened 7 years ago

ghys commented 7 years ago

Hi,

Would you consider a new flag in a condition's config in a rule, telling it should NOT be satisfied for the rule to run?

Thanks!


For context - My use case is, I have hacked together a GUI to make rules from flowcharts as an experiment: https://github.com/ghys/flowsbuilder

and allowed the ability to specify an "else" branch for conditions like this: image

Basically when encountering this during the traversal of the flow:

condition A -(then)-> do B
           \-(else)-> do C

it will do this:

  1. Create a new rule {CurrentRuleUid}_{nodeID} with condition A and continue traversing down the "then" path
  2. Create another rule {CurrentRuleUid}_{nodeID}neg with condition NOT(A) and continue traversing down the "else" path
  3. In the current rule {CurrentRuleUid], append a core.RunRuleAction configuring it to run both rules {CurrentRuleUid}_{nodeID} and {CurrentRuleUid}_{nodeID}neg with the considerConditions flag, so that only one will actually be executed.

To perform the "NOT(A)" part, since there is was no other way (afaik), I have temporarily hardcoded the "negate" functions for the built-in conditions in the app, and I'm not proud of it ;) https://github.com/ghys/flowsbuilder/blob/5897739e532344a29e6506ebb85e05c3805c2fd4/web/app/services/modulestypes-ext.service.js#L43

ghys commented 7 years ago

Possibly related to #3224 (even though it's about triggers, not conditions)

danchom commented 7 years ago

I'm not sure if this config property is needed. Actually if you write a condition which is satisfied on positive value you can write a negative condition too. For example if the door == open is positive condition, the door != open is its negative one. We have core.GenericCompareCondition which supports equal and not equal operation.

Isn't be possible to implement your use case in the following way: Rule1: triggers -> (aways_mode = on) -> execute: Say "intrusion detected", play_barking.mp3, report_intrusion
Rule2: triggers -> (aways_mode != on && DayOfWeekCondition(SAT,SUN)) -> execute: start weekend playlist. Rule3: triggers -> (aways_mode != on && DayOfWeekCondition(MON,TUE,WED,THU, FRI)) -> execute: Say "welcome home", run after work routine .

There was an idea to implement a logical expression over the conditions, the problem was how to define the logic expression. The proposal of rule configuration property containing LDAP expression over the configuration properties was too complex. Also it is not needed anymore because you can implement any logic expression by the script condition.

ghys commented 7 years ago

Sure, that's what I meant by "hardcoded the negate functions for the built-in conditions": my client knows how to compute the negation of the well-known conditions timer.DayOfWeekCondition, core.ItemStateCondition, script.ScriptCondition but if another module type without a well-known negation were added, it wouldn't know how to do it (I'm currently throwing an exception to the user telling it's impossible to have an "else" branch in that case).

My suggestion was to allow an optional flag (at the condition or the rule level actually) so the engine would invert the result of the condition's isSatisfied call, to basically make a "unless (condition)" predicate instead of "but only if (condition)", in Paper UI's terms - if it makes sense, of course.

https://github.com/eclipse/smarthome/blob/a0ebb170eb5831902fc20b841e1f070d065747fb/bundles/automation/org.eclipse.smarthome.automation.core/src/main/java/org/eclipse/smarthome/automation/core/internal/RuleEngine.java#L960-L987

Or is it possible to negate an arbitrary condition with the GenericCompareCondition?

danchom commented 7 years ago

Actually the GenericCompareCondition is intended to compare values, not to negate the result. And yes you are right in some case it is hard to create a negate condition. For example if we have a timer which must not be satisfied on day 15 of mount it is not suitable to create 30 other rules. It can be implemented in few ways: 1) condition property - it changes current JSON api it is not breakable change but it is limited only to the negate operation 2) configuration property of moduleType - It will be implemented only for some module types 3) RuleEngine can support a few new composite type i.e. ORConditionComposite, NOTConditionComposite, ParallelActionComposite. In this type it will be possible to create arbitrary condition expression. For example: if a rule has conditions: ORConditionComposite (condition1, condition2), NOTConditionComposite(condition3) it will be satisfied when the expression ((condition1 || condition2) & !condition3) returns true. @kaikreuzer, @maggu2810 , @sjka what do you think?

maggu2810 commented 7 years ago

An optional flag to allow the negate of the condition result could be a simple option to satisfy @ghys requirements. It does not break an API and I don't see a problem to support it.

danchom commented 7 years ago

Yes it is the simplest solution which satisfied @ghys requirement, but isn't be useful to have such composite module types handled by the RuleEngine itself? In that way we can create any logic expression on the conditions and it won't be needed to duplicate the triggers and actions in the second rule when we need an OR operation between conditions.

adimova commented 7 years ago

I'm not sure where this "optional flag" can be:

  1. in rule configuration (which does not exist at the moment)
  2. in the condition configuration, which is described in the concrete module type
  3. as a field in Condition, which changes current JSON schema and changes the Condition class

For me only Dancho's option 3 is appropriate

ghys commented 7 years ago

Is there a way to configure children of composite types at runtime i.e. when building a rule? If I understand correctly (correct me if I'm wrong) they are used to define new module types, but how can I configure a composite condition with children each having their own configuration? Could I do something like this in a rule?

...
      "conditions": [
        {
          "inputs": {},
          "label": "negate the child condition",
          "description": "",
          "configuration": {
          },
          "id": 2,
          "children": [
            {
              "inputs": {},
              "negated": true,
              "id": "2",
              "label": "an item has a given state",
              "description": "Compares the item state with the given value",
              "configuration": {
                "properties": {
                  "itemName": "Light_GF_Corridor_Wardrobe",
                  "operator": "\u003d",
                  "state": "ON"
                }
              },
              "type": "core.ItemStateCondition"
            }
          ],
          "type": "core.NegateCompositeCondition"
        }
      ],
...
ghys commented 7 years ago

Btw: I implemented

  1. condition property - it changes current JSON api it is not breakable change but it is limited only to the negate operation

see my commit above (https://github.com/ghys/smarthome/commit/8b59c85693000485efb30d9d96b99034f64dd6b6), but I won't submit a PR since the approach is still being discussed. I can discard it.

kaikreuzer commented 7 years ago

I am actually not sure about the whole idea. The conditions in a rule are not meant as a mechanism for the control flow, but merely as an option to suppress the execution of certain rules (where the reason for the suppression is NOT necessarily a part of the use case per se). Stuff like "only if I want automatic rules to execute" or "if there are no guests at home", etc.

The flow you showed above has multiple if/else statements, so imho this cannot be mapped on the existing rule structure anyhow, even with such a new negate mechanism. From a user perspective, I would very much want to see one flow = one rule in the Paper UI in the end; having an external tool that allows me to model one use case as one flow, I would not want this to pollute my system by creating 10 ESH rules for the single flow in the end.

So wouldn't it much more straight forward to only work with composite actions? Or possibly even by creating a single Javascript action for a flow, which has all the control logic in scripted form? Yes, we might have to think about how making pre-configured modules (such as the say-action) available to scripts, but I would think that we will need to solve this anyhow sooner or later.

ghys commented 7 years ago

@kaikreuzer Point taken. To be honest I wasn't too sure about the approach either for similar reasons, thinking I was abusing the rule system a little bit, but figured it was worth a try, see how it went. The "run rules" module type certainly allows interesting scenarios even with the current API.

One week later, I feel fine with those "technical" rules in my system, since 1) they're put under a prefix (flow1:rulexyz) to emphasize they're part of a cohesive whole (could also tag them), 2) are clearly marked "generated by a tool, don't edit or run directly"... I even contemplated hiding them from the view (there is a "visibility" flag after all) as they might be considered as mere building blocks managed under the hood by an external tool; like when you design a GUI window with a graphical editor and it generates the lower-level code you're not supposed to touch. There's also a button in the GUI to "unpublish" them i.e. remove them all at once if necessary.

That being said, since the flows are indeed basically equivalent to imperative programming, replacing them with real code in a script rule would be cleaner, yes. If we're able in the future to run modules on the fly in scripts, like if (checkCondition('core.ItemStateCondition', config, context)) or runAction('media.SayAction', config, context) I can definitely see that happening.