-
Notifications
You must be signed in to change notification settings - Fork 786
added script support for MQTT actions #6660
Conversation
@davidgraeff Took a while as it was more tricky than I thought - but with this change, you can now call the MQTT action from DSL rules like this:
Would be nice if you could briefly test this and provide feedback - as discussed, we should get that into the openHAB 2.4 release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect, I will test this tonight. Could you add your example
val actions = getActions("mqtt","mqtt:systemBroker:embedded-mqtt-broker")
actions.publishMQTT("mytopic","myvalue")
to the MQTT.generic readme as well?
@@ -31,11 +31,11 @@ | |||
* | |||
* @author David Graeff - Initial contribution | |||
*/ | |||
@ActionScope(name = "binding.mqtt") | |||
@Component(immediate = false, service = { AnnotatedActionThingHandlerService.class }) | |||
@ThingActionsScope(name = "mqtt") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you adapt the readme.md file as well? Because that is the only reference documentation at the moment.
@davidgraeff Done! |
@@ -60,6 +67,7 @@ | |||
* @author Simon Merschjohann - refactored to be an ScriptExtensionProvider | |||
* | |||
*/ | |||
@Component(immediate = true) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need immediate here, do we?
I see it has been immediate before, but as you changed from XML to annotations I think it is a good time to question it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it might not be needed, but as this is a last-minute change that I'd like to get into the openHAB 2.4 release, I prefer to keep it the same as before to reduce risk. Happy to remove it in a week!
@@ -114,7 +114,7 @@ class RulesJvmModelInferrer extends ScriptJvmModelInferrer { | |||
logger.warn("Duplicate field: '{}'. Ignoring '{}'.", name, type.class.name) | |||
} | |||
] | |||
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change that only adds tailing whitespaces can be removed from the commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
removed
@davidgraeff Did you have a chance to test? |
I'm testing now, hold on a moment please |
Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Kai Kreuzer <kai@openhab.org>
Signed-off-by: Kai Kreuzer <kai@openhab.org>
42c5656
to
ac671c0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functional testing performed.
- It works with the rule DSL under different conditions with two different brokers.
I get:
11:24:56.602 ERROR o.e.s.a.m.c.h.AnnotationActionHandler[:90] - Could not call method 'void org.eclipse.smarthome.binding.mqtt.action.MQTTActions.publishMQTT(java.lang.String,java.lang.String)' from module type 'mqtt.publishMQTT'.
java.lang.IllegalAccessException: Class org.eclipse.smarthome.automation.module.core.handler.AnnotationActionHandler can not access a member of class org.eclipse.smarthome.binding.mqtt.action.MQTTActions with modifiers ""
at sun.reflect.Reflection.ensureMemberAccess(Reflection.java:102)
at java.lang.reflect.AccessibleObject.slowCheckMemberAccess(AccessibleObject.java:296)
at java.lang.reflect.AccessibleObject.checkAccess(AccessibleObject.java:288)
at java.lang.reflect.Method.invoke(Method.java:491)
at org.eclipse.smarthome.automation.module.core.handler.AnnotationActionHandler.execute(AnnotationActionHandler.java:88)
at org.eclipse.smarthome.automation.core.internal.RuleEngineImpl.executeActions(RuleEngineImpl.java:1199)
at org.eclipse.smarthome.automation.core.internal.RuleEngineImpl.runNow(RuleEngineImpl.java:1047)
at org.eclipse.smarthome.automation.core.internal.RuleEngineImpl.runNow(RuleEngineImpl.java:1063)
at org.eclipse.smarthome.automation.rest.internal.RuleResource.runNow(RuleResource.java:288)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
When I add a (useless) Paper UI automation rule to fire the MQTT publish action.
This PR is not about making GUI rules work, so this is not in scope. Actually, I think we should remove the registration as an action in the new rule engine as it isn't usable anyhow, would you agree? |
Oh I just used Paper UI because I have no clue yet how to write automation engine jsons, the result should be similar (except that the mqtt publish is a no-op because no topic is given). So the error is still relevant, isn't it? I think you don't need to hide it, because the entire module is still marked as experimental. Other UIs (like HaBot) can also create automation rules. |
Signed-off-by: Kai Kreuzer <kai@openhab.org>
@davidgraeff With my last commit, I have fixed the |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/mqtt-binding-version-2-4/53811/205 |
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/openhab-2-4-has-been-released/60077/38 |
Signed-off-by: Kai Kreuzer kai@openhab.org