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

Provide a trigger for thing Online/Offline status in rule. #3001

Merged
merged 14 commits into from Apr 8, 2017

Conversation

Projects
None yet
6 participants
@kceiw
Copy link
Contributor

commented Feb 12, 2017

This is to fix the issue in #1654
With this change, the user can specify the triggers such as
"Thing received update" or "Thing changed" in a rule.
Please note that is a string instead of a ID. This is done so because thingUID contains the character ":". And Online is defined as Thing.getStatus() == ThingStatus.Online. Otherwise, it's offline. I've tested it with two simple rules that just output a log for "received update" and "changed" triggers.
This change is not try to expose thing status to script. That should be done separately.

kceiw added some commits Jan 9, 2017

RunEngineImple subscribes to ThingStatusInfoChangedEvent.
We're going to expose thing status to rule. This is the first step
toward the goal. The rule engine needs to subscribe to the event.

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Add a thing syntax in rule.
The syntax is "Thing <thingUid> changed [from <oldState>] [to <newState>]"

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Expose ThingRegistry in rule.
Similar to how ItemRegistry is exposedin RulesJvmModelInferrer, ThingRegistry
is exposed in RulesJvmModelInferrer.xtend

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Refactor and add RulesThingRefresher.
RulesThingsRefresher acts like RulesItemRefresher. It listens to
things changes (added/removed) and schedules to reload the rules.

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Infer whether an event trigger is ThingStateChangedEventTrigger.
Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
RuleEngine listens to ThingStatusChangedEvent and execut the rule.
Convert the thing status from online to Online which is used in the rule.
Other thing status are converted to Offline.
The rule engine also supports the rule with trigger thing state change.

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Support to handle thing state update in rule engine.
Add the syntax 'Thing' <thingUid> 'received update' in the syntax file. This
is similar to item status update.
Handle the thing status event and execute thing update rule in rule engine.

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Fix issues in thing update/change rules.
The rule is changed to take thingUid as a string instead of an ID. And we
don't use the type Thing internally to pass the information in the rule. This
is done so because when the thing is removed, it can occur before the rule is
checked and executed. When it occurs, we cannot get the Thing from thingUid.
Fix an issue in storing thing related rules and retrieve them.

Bug: #1654
Signed-off-by: Maoliang Huang <hellomao@outlook.com>
@kceiw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 13, 2017

After I spent a few hours fixing the failing test (https://travis-ci.org/eclipse/smarthome/builds/200974384), I have to ask for help.
The failing test is

Running org.eclipse.smarthome.model.script.tests.scriptengine.ScriptEngineOSGiTest
Tests run: 1, Failures: 1, Errors: 0, Skipped: 0, Time elapsed: 0.744 sec <<< FAILURE! - in org.eclipse.smarthome.model.script.tests.scriptengine.ScriptEngineOSGiTest

testInterpreter(org.eclipse.smarthome.model.script.tests.scriptengine.ScriptEngineOSGiTest) Time elapsed: 0.465 sec <<< FAILURE!
java.lang.AssertionError: null

at org.junit.Assert.fail(Assert.java:86)
at org.junit.Assert.assertTrue(Assert.java:41)
at org.junit.Assert.assertNotNull(Assert.java:621)
at org.junit.Assert.assertNotNull(Assert.java:631)
at org.junit.Assert$assertNotNull$0.callStatic(Unknown Source)
at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallStatic(CallSiteArray.java:53)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:157)
at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callStatic(AbstractCallSite.java:165)

In the test, it verifies the ScriptServiceUtil is not null but it is

scriptServiceUtil = getService(ScriptServiceUtil)
assertNotNull(scriptServiceUtil)

The only change in ScriptServiceUtil is

<reference bind="setThingRegistry" cardinality="1..1" interface="org.eclipse.smarthome.core.thing.ThingRegistry" name="ThingRegistry" policy="static" unbind="unsetThingRegistry"/>

My guess is that ThingRegistry isn't created some how. I tried to debug like

def ThingRegistry thingRegistry = getService(ThingRegistry)

or

def ThingHandlerFactory thingHandlerFactory = getService(ThingHandlerFactory) // ThingRegistry needs a ThingHandlerFactory

or

def ThingHandler thinHandler = getService(ThingHandler) // ThingHandlerFactory needs a ThingHandler

All of them return zero. I am not quite familiar internal OSGI works. During my debugging, what I can tell is that those classes (ThingRegistry, ThingHandlerFactory, ThingHandler) are found (somewhere LoadClass is called and it is successful). But an instance isn't created (don't have this source code and don't understand what's going on). Thus it returns null.

Can anybody please shed a light on it?

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Feb 17, 2017

I'm no expert on this myself, but I assume that you are simply missing the bundle org.eclipse.smarthome.core.thing during your test executions, since this provides the ThingRegistry as a service. Note that you can add additional required dependencies here, which are then available during the test execution. Check also https://www.eclipse.org/smarthome/documentation/development/testing.html#groovy.

Fix a test org.eclipse.smarthome.model.script.tests.
- Need to add the bundle org.eclipse.smarthome.thing to the plugin in pom.xml.

Signed-off-by: Maoliang Huang <hellomao@outlook.com>
@kceiw

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2017

Thanks @kaikreuzer . That works. I added it to pom.xml and updated the pull request.

@fishpark

This comment has been minimized.

Copy link

commented Feb 25, 2017

Cool, seems that you are making progress. When will this feature be available in the snapshot relaease?

@kceiw

This comment has been minimized.

Copy link
Contributor Author

commented Mar 2, 2017

I don't see any reviews yet and the maintainers don't approve it yet. I think that is the process to complete the pull request. Until then, I don't think it'll be in snapshot release soon.

@fishpark

This comment has been minimized.

Copy link

commented Mar 2, 2017

Thanks for the info @kceiw. I can hardly wait to use this feature :-)

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Mar 3, 2017

I don't see any reviews yet and the maintainers don't approve it yet. I think that is the process to complete the pull request. Until then, I don't think it'll be in snapshot release soon.

You are right 😉
Please be patient.

@kaikreuzer
Copy link
Member

left a comment

Sorry, I also didn't yet find the time for a thorough review and test.
I think @kceiw did a tremendous job and really dived deep into the whole Xtext inner workings - definitely no easy job, congrats!
I just did a cursory review and have some initial remarks - the main one being a misconception about Item State vs. Thing Status.

import org.eclipse.smarthome.core.types.PrimitiveType;
import org.eclipse.smarthome.core.types.State;

public enum OnlineOfflineType implements PrimitiveType, State, Convertible {

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Mar 3, 2017

Member

ONLINE/OFFLINE must not be introduced as a new state - it is a "status" which only applies for Things, not for Items.

This comment has been minimized.

Copy link
@kceiw

kceiw Mar 3, 2017

Author Contributor

Please see the comment in Rules.xtext about ValidState and ThingStatusInfo

@@ -28,6 +28,7 @@ Import-Package: org.eclipse.smarthome.core.common.registry,
org.quartz.spi,
org.quartz.utils,
org.slf4j
Require-Bundle: org.eclipse.smarthome.model.rule
Require-Bundle: org.eclipse.smarthome.model.rule,
org.eclipse.smarthome.core

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Mar 3, 2017

Member

please use Import-Package instead

@@ -72,6 +74,13 @@ SystemOnShutdownTrigger:
'System' 'shuts down'
;

ThingStateUpdateEventTrigger:
'Thing' thing=STRING 'received update' (state=ValidState)?

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Mar 3, 2017

Member

The update is not a "ValidState", but ThingStatus (or even rather a ThingStatusInfo, check out the classes - it would need to be decided what syntax to use here in the DSL for it).

This comment has been minimized.

Copy link
@kceiw

kceiw Mar 3, 2017

Author Contributor

How about exposing ThingStatusInfo to the rule and scripts.

How about this new syntax?
'Thing' thing=STRING 'received update' (state=ThingStatusInfo)?
ThingStatusInfo = STRING // or something that make sense and can be converted from ThingStatusInfo

This comment has been minimized.

Copy link
@kaikreuzer

kaikreuzer Mar 15, 2017

Member

Instead of allowing strings (and requiring hyphens around them), I think we should better introduce dedicated keywords - it is a well defined list, so this should not be hard.
Not sure if we would also have to make the ThingStatusDetail available here, which would complicate stuff - I'd therefore suggest to leave it out of scope and rather only make the ThingStatusInfo available within the "then" block, so that more detailed constraints can be implemented in there.

This comment has been minimized.

Copy link
@kceiw

kceiw Mar 20, 2017

Author Contributor

That sounds good to me. So we agree on the syntax like "Thing thing=STRING received update (state=ThingStatusInfo)" And "ThingStatusInfo=... (the well defined list)"

kceiw added some commits Apr 1, 2017

Change thing changed event syntax.
- Use a pre-defined list as the thing status. the list is the same as
  ThingStatus.

Bug: #1654
Signed-off-by: kceiw <hellomao@outlook.com>
Expose ThingStatusInfo via an action.
- The thing uid contains ':' which cannot be used as an identifier. So we cannot
  use something like "ThingType:Thing" in the script and interpreset it as a
  thing. So we need to create an action to accept the thing uid and return its
  status.
- This is to allow the user to create more restrains using ThingStatusInfo in
  the script.

Bug: #1654
Signed-off-by: kceiw <hellomao@outlook.com>
remove org.eclipse.smarthome.core from required package.
Bug: #1654
Signed-off-by: kceiw <hellomao@outlook.com>

@kceiw kceiw force-pushed the kceiw:ThingStateRule branch from e573b8b to 2a56168 Apr 1, 2017

@kceiw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2017

Hi @kaikreuzer
I create a list of thing status in the Rules.xtext. So it won't messed with item state. I use the string format of ThingStatus.

At the same time, I create a new action (ThingAction) for script. The user can use this action to get ThingStatusInfo in 'then' block. I cannot use dot notation like "item.state" because a thing uid contains ':' which make it an invalid identifier in the script. So I create the action to accept the thing uid and return ThingStatusInfo.

Can you look at that again and give me more feedback?

@kceiw kceiw force-pushed the kceiw:ThingStateRule branch from 2a56168 to 5eeb0e2 Apr 1, 2017

Add missing activate functions in ThingActionService.
Bug: #1654
Signed-off-by: kceiw <hellomao@outlook.com>
@kceiw

This comment has been minimized.

Copy link
Contributor Author

commented Apr 8, 2017

I tested with a rule. Verify that the rule is executed when the updated/changed events are fired. And getThingStatusInfo() returns the right ThingStatusInfo in 'then' script.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Apr 8, 2017

Many thanks @kceiw, this sounds good and at a quick glance, the code looks very clean as well - incredible, how you dived into Xtext/Xbase/actions etc. 👍
I didn't find yet the time to test it myself, but I trust you that all works as expected - so many thanks again and sorry for my always too slow feedback!

@kaikreuzer kaikreuzer merged commit 0c51457 into eclipse:master Apr 8, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
ip-validation
Details

@kaikreuzer kaikreuzer changed the title Provide a trigger for thing Online/Offline state in rule. Provide a trigger for thing Online/Offline status in rule. Apr 8, 2017

result = Iterables.concat(thingUpdateEventTriggeredRules.values());
break;
case THINGCHANGE:
result = Iterables.concat(thingChangedEventTriggeredRules.values());

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Apr 10, 2017

Contributor

I assume a break is missing here!

This comment has been minimized.

Copy link
@kceiw

kceiw Apr 15, 2017

Author Contributor

Many thanks. I'll create a new PR.

case THINGUPDATE:
for (Set<Rule> rules : thingUpdateEventTriggeredRules.values()) {
rules.remove(rule);
}

This comment has been minimized.

Copy link
@maggu2810

maggu2810 Apr 10, 2017

Contributor

missing break

This comment has been minimized.

Copy link
@kceiw

kceiw Apr 15, 2017

Author Contributor

I'll create a new PR.

@maggu2810

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2017

@kceiw As this PR has already been merged, can you create a new one that adds a break / return in every case branch?

@kceiw kceiw referenced this pull request Apr 15, 2017

Merged

Address PR feedback #3253

@jn-se jn-se referenced this pull request Jun 12, 2017

Closed

Offline Hue lights #1776

@yo8192

This comment has been minimized.

Copy link

commented Jun 17, 2017

Hi,

This looks like what I need, thank you for your work on this.
Any chance you could point me to some documentation on how to use these new features (what is the syntax), or alternatively provide a few examples to get me started?

Thank you,
Thib.

@kceiw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2017

@yo8192 Thanks.
The documentation in this project doesn't seem to touch this area. I'm not quite sure where to put it. Please let me know if you know it.
At the same time, let me answer your question below.
The syntax for the trigger can be found in the file https://github.com/eclipse/smarthome/pull/3001/files#diff-8033c42c5df568f5b7a61e26b48c1d52
It'll look similar to the one for item. Instead of using item and item name, please use thing and thing uid.
If you want to receive update, you write it
Thing <thing uid> received update
If you want to know whenever there is a change
Thing <thing uid> changed

You can find from Paper UI. It's a string separate by ':'
Of course, you can put some optional states after 'received update' or 'changed as you do to item. The valid state for a thing can be found from the above file.

In your rule or script, you can get the thing state by using getThingStatusInfo(thingUid) (see https://github.com/kceiw/smarthome/blob/25b04e91c423901ba492c77888a3a4517a8765bd/bundles/model/org.eclipse.smarthome.model.script/src/org/eclipse/smarthome/model/script/actions/ThingAction.java). This function returns a ThingStatusInfo (https://github.com/eclipse/smarthome/blob/3ae5d7b1b284f2bde6199bfe8dad950fd38cae39/bundles/core/org.eclipse.smarthome.core.thing/src/main/java/org/eclipse/smarthome/core/thing/ThingStatusInfo.java) and you can get the state by calling getStatus on the ThingStatusInfo you get.
Note that if you get null for the ThingStatusInfo, usually that means the thing is removed.

@kaikreuzer

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

@kceiw You are right that the ESH documentation does not cover such features at all.
But it would be cool if you could do a contribution to the openHAB rule documentation as it would fit in there perfectly!

@openhab-bot

This comment has been minimized.

Copy link
Contributor

commented Sep 5, 2017

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/how-to-restart-binding-with-in-rules/29843/8

kceiw added a commit to kceiw/openhab-docs that referenced this pull request Sep 26, 2017

Add document about getting thing status in scripts and rule.
The change of the code is at eclipse/smarthome#3001
After this commit, the end user can find the documentation about how to react
to thing status change in the rule.

Bug: openhab#517
Signed-off-by: Maoliang Huang <hellomao@outlook.com>

Incorporate more pull request feedback.

ThomDietrich added a commit to openhab/openhab-docs that referenced this pull request Sep 26, 2017

[Rules] Add chapters on thing status in actions and rules (#463)
The change of the code is at eclipse/smarthome#3001
After this commit, the end user can find the documentation about how to react
to thing status change in the rule.

Bug: #517

Incorporate more pull request feedback.

Signed-off-by: Maoliang Huang <hellomao@outlook.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.