-
Notifications
You must be signed in to change notification settings - Fork 782
Conversation
* @throws IllegalArgumentException if the UID or the item type is null or empty, | ||
* or the the meta information is null | ||
*/ | ||
public ChannelType(ChannelTypeUID uid, boolean advanced, String itemType, String triggerType, String label, |
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 would say this new constructor should NOT have itemType and state params, so that it is the one used for creating instances of trigger channels.
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.
ChannelType has two constructors: The first one for use without triggerType, the second for use withTriggerType (itemType is then null). I can't add a constructor only for triggerType, as itemType and triggerType are both String
and then it would clash with the itemType
constructor.
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 see, ok.
Thanks @phxql Looks in general good to me as a starting point - and it isn't that much code change after all. What seems to be still completely missing is the adaption of the Channel class itself, right? To fully see the implications, we would need the triggers to be considered there as well as in the REST API and the thing file definitions. |
What do you mean with thing file definitions? The thing-types.xml? |
Ah, the DSL. I'll start with the adaption of the channel class and the REST interface, as i know that already. |
I updated the PR:
|
Open issues:
|
Having had a quick glance, the approach still looks good so far. However, I'm missing a change in Thing.xtext and GenericThingProvider so that it becomes possible to define the channels in the Thing DSL. |
Proposal: Thing channel with item type String
That's the syntax right now. Thing channel with item type String
Thats an alternative syntax for the same usecase. Thing channel with trigger type String
Thats the syntax for adding a trigger channel. |
Wrote a gist containing the changes: https://gist.github.com/phxql/e75b287ab8601d43d63d553175e6b877 |
@@ -92,6 +92,7 @@ public void receive(Event event) { | |||
if (callback != null) { | |||
logger.trace("Received Event: Source: " + event.getSource() + " Topic: " + event.getTopic() + " Type: " | |||
+ event.getType() + " Payload: " + event.getPayload()); | |||
// TODO MKA: Why is the topic checked against the source and not event.getSource()? |
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.
simply because for some event types the event source is not the item (or even not set...)
@maggu2810, @kaikreuzer could you please have a look? |
@SJKA Sure, it is already placed somewhere in my PriorityQueue 😉 |
@@ -93,7 +93,7 @@ public void receive(Event event) { | |||
if (callback != null) { | |||
logger.trace("Received Event: Source: " + event.getSource() + " Topic: " + event.getTopic() + " Type: " | |||
+ event.getType() + " Payload: " + event.getPayload()); | |||
|
|||
// Check the topic for the source as the event source is not set for the most events. |
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 is pretty misleading - the event.source is something completely different than the "source" configuration property in this handler (which means "object that this event refers to", but not "sender of the event").
Added system triggers. |
@@ -0,0 +1,21 @@ | |||
package org.eclipse.smarthome.core.thing; |
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.
license header missing
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.
Done.
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.
Thanks :-)
Signed-off-by: Moritz Kammerer <moritz.kammerer@qaware.de>
Here as well, bit challenged in time... will take a look shortly |
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.
Haven't tested the code, but code review done with some minor comments.
* | ||
* @param uid the unique identifier which identifies this Channel type within | ||
* the overall system (must neither be null, nor empty) | ||
* |
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.
Why do you add an empty line after every @param
tag?
Please remove 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.
Fixed.
@@ -29,112 +31,121 @@ import org.slf4j.LoggerFactory | |||
*/ | |||
class RulesJvmModelInferrer extends ScriptJvmModelInferrer { | |||
|
|||
static private final Logger logger = LoggerFactory.getLogger(RulesJvmModelInferrer) |
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.
Don't know anything about Xtend / Xtext, but does we need to use a static variable here?
But you should use the common modifier order: private static final (or drop static).
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.
Fixed.
Signed-off-by: Moritz Kammerer <moritz.kammerer@qaware.de>
Signed-off-by: Moritz Kammerer <moritz.kammerer@qaware.de>
Signed-off-by: Moritz Kammerer <moritz.kammerer@qaware.de>
Thanks for the review @maggu2810. |
I just noticed that this PR has >1000 lines, so we anyhow cannot merge immediately - I have created CQ 12006 for it. |
CQ has been approved! |
I am looking at this for the HarmonyHub binding now. I can see their use in rules, but how does a thing handler respond to a trigger channel? To give a little background, The HarmonyHub has a thing type called "device" , think of this as your TV or Stereo. Each "device" has a channel called "buttonPress", think of this as pressing a button on your remote control. This channels sends the name of the button to press as a string, so "Power On" or "ENTER" for example. This value are dynamically created for the channel as State Options. So for example: DeviceThing -> ButtonPressChannel -> ButtonName {PowerOn, PowerOff, Enter, 1, 2, 3, 4 ...} I can see how I could use trigger events to send the button names, but how would the ThingHandler get these events. Also how would a user define a trigger channel in say a sitemap? Should I rather be looking on how to compliment my current button channel just for rules or is this meant as a replacement for this type of "Send Only" channel. |
It seems I misinterpreted the "buttonPress" channel in the HarmonyHub binding when I suggested you to look at the trigger channels. Trigger channels are ONLY meant for events coming from the remote device, i.e. you get hold of an external button press on some remote control. |
This PR implements a PoC for triggers.
Sample XML snippet:
In the handler:
In the rule engine DSL: