Hi,
I made a patch to openhab1-master that allows to handle regexp in item names in rules and also introduces some syntax extension to catch events to items in groups (instead of catching it on the group level).
I’d like you to review that: https://github.com/openhab/openhab/pull/3102
If the changes I made to openhab1 are fine, I would be eager to migrate them to openhab2…
dero
I am a bit ambivalent about that change request. While the new features make sense and will probably help many users to simplify their rules, you will be aware that we consider the openHAB 1 core to be frozen and only critical fixes should be done to it. When scanning your PR, it furthermore seems to me that there are more code changes in it than necessary for these new features - it looks like a refactoring of central parts of the rule engine, which could be a risky thing to do (and hard to be migrated to Eclipse SmartHome, because the code has already further evolved there).
For Eclipse SmartHome (and therefore openHAB 2), there is currently a whole new rule engine being implemented; enhancing the “old” means that there is yet another feature that needs to be implemented in a compatible way, so again more efforts and risk involved here.
For these reasons, I am not sure how we should proceed here. @teichsta, what’s your opinion?
Kai,
While looking over the code I stumbled over a few serious bugs and some interface design flaws. It wasn’t that easy to enhance it, so I rewrote it in compact functional style (no instanceof anymore). (I am Scala guy…)
If you could point me to the corresponding sources in oh2, I would be happy to get my hands on it…
Dero
I was just looking into the discussion of deprecation the xtext-based rule engine in favor over json structures.
From a programmatic perspective, I fully understand that structured json rules make a lot of sense if they are to be generated using a graphical rule editor.
However, completely removing or just letting bleed dry the xtext-based rule engine would be a big mistake IMHO. Allowing people to script out rules instead of just clicking them together in a UI is much more powerful. I see both approaches equally important, and would strongly advocate for the xtext-based rule engine!
Having said this, I am fine to port my changes to smarthome…
Kai, you also said that my changes are risky. I guess you mean that they are not working or might break things. Alas, I couldn’t find any unit-tests to test them out, but I was testing them using the demo site. But, IHMO, I think, not having unit-tests is the risky part here… (Also, there were bugs in the old code, e.g. reacting to changed rules files was buggy since it passed in the NEW model for removing rules).
dero
You probably misunderstood the plan here: The new rule engine will replace the current RuleEngineImpl and RuleTriggerManager (etc.) classes, i.e. the implementation that orchestrates the execution. My plan is to still have scripting available, simply by offering according modules for the new engine. This could be JSR223-scripting (e.g. JS), but also Xbase scripts. I’d like to even parse the existing rule files and feed them into the new rule engine. This is exactly the part why I am cautious to add new features in the syntax; it would automatically mean more effort for this migration.
Exactly, I meant they can potentially break things and we would not notice because there is no unit test coverage available.
If there are obvious bugs (and you could potentially even produce them through a unit test), then fixing them in 1.x should be done without a doubt. What I would like to ask you for is to split such changes into different PRs:
Kai,
I am currently in the process of migrating my house from FHEM to openhab. The things I added are the main features I missed in oh which FHEM has. FHEM is flexible like hell. I’ll mean this literally: The whole trigger concept is just tying Perl code to regexp event matching. It makes it flexible, but very very error prone and slow.
I drive a full fledged home-build alarm system (motion, door sensors, sirens), wind sensors, fire detectors, heat pump, window blinds, anti-theft presence simulator, etc. So many rules…
What I am also thinking of, why not using a Scala based DSL for the whole config. Scala’s language is so flexible, it’s easy to “abuse” it as a DSL for items, rules, and site maps. Plus: You get a error checked, typesafe, and compiled config.
I don’t know what to do right now. The changes work for me and I can just start to migrate things. To be egoistic, I would rather just lay back and propose that oh2 should allow this kind or some similar syntax extensions and wait…
Dero
Dero, I am not dismissing your feature, I am only trying to assess the impact on merging it into Eclipse SmartHome.
My main request is that you split the PR into dedicated parts - if there was a PR that only dealt with the changes absolutely required for the new features, it would be much easier to review. You mention that regexp processing in FHEM make it slow - this was actually also a reason why I didn’t initially introduce this in the triggers. Why do you think that this feature does not cause any performance issues for openHAB as well?
Note that I am only doing this discussion with respect to Eclipse SmartHome. The openHAB 1.x PRs are processed, reviewed and merged by @teichsta, so I would want to wait for his opinion, too.
In my code, whenever an item is seen the very first time in some event, all triggers are checked whether they respond to that item. The results are cached in an Item -> Trigger map. So, the next time no regexp matching happens, but just a cache lookup. When a rule model is changed, the cache is just purged…
Dero
Sounds like a way to avoid performance impacts. I think it should be feasible to encapsulate this logic into a trigger module of the new Eclipse rule engine, so I don’t want to block this feature, but still waiting for @teichsta’s feedback.
Meanwhile, feel free to extract the “severe bug fixes” into a new, separate PR, so that this can be processed asap. Could you please check, if these bugs also still exist in https://github.com/eclipse/smarthome/tree/master/bundles/model/org.eclipse.smarthome.model.rule.runtime/src/org/eclipse/smarthome/model/rule/runtime/internal/engine?
Cheers,
Kai
Hi Dero,
i had a quick look into your PR and would like to thank you a lot for your efforts.
However, as Kai i am quite reluctant to add new features to the core for the sake of keeping the migration efforts as small as possible. Would it be an option for your to have a look into the JSR223 Rule Engine? Since this one is right now considered as “may change in the future” everybody using it will be aware of possible changes in the future.
Having the fixes as separate PR would be very great though!
Best, Thomas E.-E.