Introduction of Profiles #4108

sjsf · 2017-08-22T14:11:57Z

This PR now reworks the factored CommunicationManager (#4067) and makes its behavior modifiable by "profiles". Such profiles are basically exchangeable parts which the communication manager delegates its actual duties to. I also outlined their purpose in the profiles.md file below to become part of the documentation.

The idea is to make the used profile configurable on ItemChannelLinks. In this first step the profile selection is more or less static though. Although I already anticipated a "ProfileFactory" interface (mainly for testing purposes here), this PR is still missing the configurability by purpose. As this will involve touching many files and the Thing-DSL, I would like to handle this in a separate follow-up PR.

Using profiles also opens up another interesting use-case: I noticed that so far no binding implements the ThingHandler.handleUpdate(...) method. Technically spoken some very few do, but they don't do anything there. The main idea of this method apparently was to allow devices to run as "slaves" of other devices, i.e. react on item updates only. A typical use-case for this would be a light switch with an indicator light which should follow the state of the the physical lamp without the risk of running out-of-sync.
Therefore I already added a "slave" profile which does exactly that: it listens to item updates, transforms them to commands (if possible) and forwards them to the ThingHandler.handleCommand(...) directly. At the same time it cuts off all other communication coming out of the thing handle. With this it would be easily possible to run any device now as a slave without the need to fix/implement this in all of the existing bindings.

At the same time I deprecated the ThingHandler.handleUpdate(...) method to prevent any new binding to implement it. I think it is fair to allow for a grace period before we drop this method from the interface.

The profiles here (especially the TriggerProfiles) are required for the second part of #1043 a.k.a. #2226. I only created a RawButtonTriggerProfile here as an example and proof-of-concept. But there are more required, of course. Especially for the system channels they make much sense! Nevertheless, I would like to keep the work of implementing these out this PR as well.

davidgraeff · 2017-08-22T15:46:56Z

Hm, I find it very hard to follow the ESH documentation in some parts and the newly added profiles.md is one of those sections that would highly confuse me. I think this is mainly because there are no diagrams that provide an overview with whom and how profiles are interacting with other components and there are no example usages.

sjsf · 2017-08-22T16:27:42Z

I agree, the docu is not yet complete. Examples can be added only once it actually can be configured. As this part is yet missing, there is not much to put there right now. I will add some pictures for the concrete profiles once all this here somehow got agreed. I hope it will get clearer then. For the time being I just thought it's important to write it down anywhere at all.

For all the other confusing parts: Fully agreed. Any help is highly appreciated!

kaikreuzer

Looks pretty good, just some very minor remarks.

kaikreuzer · 2017-09-01T14:21:03Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java


    private final Logger logger = LoggerFactory.getLogger(CommunicationManager.class);
+    private final Set<String> subscribedEventTypes = ImmutableSet.of(ItemStateEvent.TYPE, ItemCommandEvent.TYPE,


Could you do it without Guava?

kaikreuzer · 2017-09-01T16:01:02Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/DefaultSlaveProfile.java

+ * @author Simon Kaufmann - initial contribution and API.
+ *
+ */
+public class DefaultSlaveProfile extends DefaultMasterProfile {


Why does it extend the master profile? Shouldn't it rather implements StateProfile as well?

I just was trying to prevent code duplication by delegating slave's onUpdate(...) to master's onCommand(...). But I see this is confusing. I will change that.

kaikreuzer · 2017-09-01T16:03:14Z

...g/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/DefaultMasterProfile.java

+ * This is the default profile for stateful channels.
+ *
+ * It forwards commands to the {@link ThingHandler}. In the other direction it posts events to the event bus
+ * for state updates and commands which are initiated by the handler.


"...and commands which are initiated by the handler" - was that really a use case for a "master" device? Do you have an example?

Really good point - I just mindlessly took that from the old API. Now thinking about it, I don't have a good example.

No binding in ESH is using it.

In openhab2-addons there are only two bindings using it: lutron and squeezebox. I'm not familiar with both yet, so it's hard to judge whether this in fact is correct or not. On a first glance it looks like it might not be needed.

PS: Just do double-check I read you correctly: This is not only about the JavaDoc, but you are suggesting to make master's "postCommand" a no-op. Correct?

kaikreuzer · 2017-09-01T16:04:30Z

...rc/main/java/org/eclipse/smarthome/core/thing/internal/profiles/RawButtonTriggerProfile.java

+import org.eclipse.smarthome.core.thing.link.ItemChannelLink;
+import org.eclipse.smarthome.core.thing.profiles.TriggerProfile;
+
+public class RawButtonTriggerProfile implements TriggerProfile {


JavaDoc missing

kaikreuzer · 2017-09-01T16:17:00Z

...ng/src/main/java/org/eclipse/smarthome/core/thing/internal/profiles/DefaultSlaveProfile.java

+ * <p>
+ * This allows devices to be operated as "slaves" of another one directly, without the need to write any rules.
+ * <p>
+ * No events are send for state updates or commands initiated by the ThingHandler. They are assumed to be passive only.


typo: are sent

Sorry if I forgot the details again: Taking the KNX wall switch as an example. Would this have to be modelled as two channels? One with master-profile only sending commands and one with slave-profile only receiving updates? What spoke against allowing slave-channels to send commands (to their "master" within ESH)?

What spoke against allowing slave-channels to send commands (to their "master" within ESH)?

Nothing really, IIRC.

If a slave issues a command towards ESH, it will have to live with the fact that the state bounces back as an update via the master (which then gets turned into a command again by the slave profile, this time flowing from ESH towards the binding). But that shouldn't matter much - it rather makes sense because it gets the real value that the master reported back as an update, which is not necessarily identical to the command that the slave initially sent towards ESH.

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

...between the framework and the binding (i.e. ThingHandler). By now, the profiles are still statically set. Selecting a different profile for a link will be done in a next step. Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

...and adding a slave profile which could be used instead in order to configure channels to react on item state updates and therefore running as "slave" of another one. Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kaikreuzer · 2017-09-05T15:11:55Z

There are 3 test failures:

  ThingManagerOSGiTest.ThingManager handles post command correctly:804->OSGiTest.waitForAssert:-1->OSGiTest.waitForAssert:191->OSGiTest.waitForAssert:220 
Expected: not null
     but: was null
  DefaultSlaveProfileTest.testPostCommand:117 
No interactions wanted here:
-> at org.eclipse.smarthome.core.thing.internal.profiles.DefaultSlaveProfileTest.testPostCommand(DefaultSlaveProfileTest.java:117)
But found this interaction:
-> at org.eclipse.smarthome.core.thing.internal.profiles.DefaultSlaveProfile.postCommand(DefaultSlaveProfile.java:92)
Actually, above is the only interaction with this mock.
  DefaultMasterProfileTest.testPostCommand:121 
Wanted but not invoked:
eventPublisher.post(<Capturing argument>);
-> at org.eclipse.smarthome.core.thing.internal.profiles.DefaultMasterProfileTest.testPostCommand(DefaultMasterProfileTest.java:121)
Actually, there were zero interactions with this mock.

sjsf · 2017-09-05T15:20:37Z

My bad, I neglected to update the tests...
Done.

kaikreuzer · 2017-09-05T19:24:27Z

Still one failing test :-(

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

maggu2810

It has been my first reading of the code, so I added some comments. All the nullness stuff for private method is just because I realized it while reading. If we only care about on public methods ignore it.

maggu2810 · 2017-09-06T09:11:18Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

+            return new NoOpProfile();
+        }
+
+        Channel channel = thing.getChannel(link.getLinkedUID().getId());


So, the parameter link must not be null.
ItemChannelLink link => @NonNull ItemChannelLink link

maggu2810 · 2017-09-06T09:12:38Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

-                            "Cannot delegate command '{}' for item '{}' to handler for channel '{}', "
-                                    + "because no thing with the UID '{}' could be found.",
-                            command, itemName, channelUID, channelUID.getThingUID());
+    private Profile getProfile(ItemChannelLink link, Item item, Thing thing) {


We could mark the return type with @NonNull

maggu2810 · 2017-09-06T09:13:59Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

+        for (ProfileFactory profileFactory : profileFactories.keySet()) {
+            Profile profile = profileFactory.createProfile(link, item, channel);
+            if (profile != null) {
+                profileFactories.get(profileFactory).add(link.getLinkedUID());


If you need access to the profileFactory, wouldn't it make sense to use entrySet() instead of keySet()?

maggu2810 · 2017-09-06T09:14:21Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

+        return null;
+    }
+
+    private Profile createDefaultProfile(Channel channel) {


@NonNull Channel

maggu2810 · 2017-09-06T09:14:50Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

+        return null;
+    }
+
+    private void receiveCommand(ItemCommandEvent commandEvent) {


@NonNull ItemCommandEvent

maggu2810 · 2017-09-06T09:17:14Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

+        });
+    }
+
+    private void receiveTrigger(ChannelTriggeredEvent channelTriggeredEvent) {


@NonNull ChannelTriggeredEvent

maggu2810 · 2017-09-06T09:18:00Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

-            eventPublisher
-                    .post(ItemEventFactory.createStateEvent(item.getName(), acceptedState, channelUID.toString()));
-        }
+        final Thing thing = getThing(channelUID.getThingUID());


@NonNull ChannelUID channelUID

maggu2810 · 2017-09-06T09:18:20Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/internal/CommunicationManager.java

-        for (String item : items) {
-            eventPublisher.post(ItemEventFactory.createCommandEvent(item, command, channelUID.toString()));
-        }
+        final Thing thing = getThing(channelUID.getThingUID());


@nonnull ChannelUID channelUID

maggu2810 · 2017-09-06T09:23:03Z

...rc/main/java/org/eclipse/smarthome/core/thing/internal/profiles/RawButtonTriggerProfile.java

+ * This profile allows a channel of the "system:rawbutton" type to be bound to an item.
+ *
+ * It reads the triggered events and uses the item's current state to calculate the new state once it detects that the
+ * button was pressed.


So, it is a toggle profile. Correct?

maggu2810 · 2017-09-06T09:26:48Z

docs/documentation/concepts/profiles.md

+
+
+
+


Why do you add all that empty lines?

maggu2810 · 2017-09-06T09:28:43Z

Damn, merged while reviewing 😉

kaikreuzer · 2017-09-06T09:31:48Z

Sorry, I had put my approval 18 hours ago and the PR is 15 days old - so I was hoping you didn't have any further comments.

But never mind - I am sure @SJKA will happily address everything in a follow-up PR :-)

maggu2810 · 2017-09-06T09:36:52Z

I had put my approval 18 hours ago and the PR is 15 days old

I am slow 😉

sjsf · 2017-09-06T13:17:54Z

Sure, no problem

...as a follow-up of eclipse-archived#4108 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

...as a follow-up of #4108 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

openhab-bot · 2018-12-01T10:10:58Z

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

https://community.openhab.org/t/what-is-the-difference-between-system-default-and-system-follow/53076/5

sjsf changed the title ~~Profiles static~~ Introduction of Profiles Aug 22, 2017

This was referenced Aug 24, 2017

Add support for properties on ItemChannelLinks #583

Closed

Allow bindings to deactivate auto-update for channels #595

Closed

kaikreuzer suggested changes Sep 1, 2017

View reviewed changes

kaikreuzer added the Core label Sep 1, 2017

Simon Kaufmann added 5 commits September 4, 2017 12:47

fix thing.test launch config

186f4e3

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

Profiles determine the communication flow

14a2eee

...between the framework and the binding (i.e. ThingHandler). By now, the profiles are still statically set. Selecting a different profile for a link will be done in a next step. Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

tests for the communication manager

ed20b67

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

deprecating ThingHandler.handleUpdate()

0e3f153

...and adding a slave profile which could be used instead in order to configure channels to react on item state updates and therefore running as "slave" of another one. Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

seeding profile docu

3e9601c

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf force-pushed the profiles_static branch from 41e8898 to a2d3488 Compare September 4, 2017 14:38

kaikreuzer approved these changes Sep 5, 2017

View reviewed changes

sjsf force-pushed the profiles_static branch from a2d3488 to c32139f Compare September 5, 2017 15:19

incorporated review feedback

e5cffbb

Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf force-pushed the profiles_static branch from c32139f to e5cffbb Compare September 6, 2017 07:07

kaikreuzer merged commit 74880d0 into eclipse-archived:master Sep 6, 2017

maggu2810 reviewed Sep 6, 2017

View reviewed changes

sjsf deleted the profiles_static branch September 6, 2017 13:17

sjsf pushed a commit to sjsf/smarthome that referenced this pull request Sep 6, 2017

improvements on profiles

062691e

...as a follow-up of eclipse-archived#4108 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf pushed a commit to sjsf/smarthome that referenced this pull request Sep 6, 2017

improvements on profiles

b086061

...as a follow-up of eclipse-archived#4108 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf mentioned this pull request Sep 6, 2017

improvements on profiles #4204

Merged

sjsf pushed a commit to sjsf/smarthome that referenced this pull request Sep 7, 2017

improvements on profiles

b89e1fe

...as a follow-up of eclipse-archived#4108 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

kaikreuzer pushed a commit that referenced this pull request Sep 11, 2017

improvements on profiles (#4204)

1173494

...as a follow-up of #4108 Signed-off-by: Simon Kaufmann <simon.kfm@googlemail.com>

sjsf mentioned this pull request Sep 15, 2017

Trigger channels #4272

Closed

sjsf mentioned this pull request Sep 25, 2017

Locking of channels #2240

Open

lewie mentioned this pull request Oct 4, 2017

initial contribution of the KNX Basic binding openhab/openhab-addons#2323

Merged

philomatic mentioned this pull request Nov 2, 2017

CommunicationManager & DefaultMasterProfile : linking items through channels not supported by default #4464

Closed

This was referenced Nov 9, 2017

The future of handleUpdate(...) #4525

Open

System trigger profiles #4542

Closed

kaikreuzer added this to the 0.9.0 milestone Nov 30, 2017

kgoderis mentioned this pull request Dec 2, 2017

AutoBridge Binding - Enable a mechanism to control how Command and St… #2881

Closed

fruggy83 mentioned this pull request Apr 11, 2018

Sender Id and options for spoofing fruggy83/openocean#6

Open

This was referenced Sep 23, 2020

Remove deprecated handleUpdate from ThingHandler openhab/openhab-core#1669

Merged

Remove deprecated handleUpdate from ThingHandler openhab/openhab-addons#8558

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduction of Profiles #4108

Introduction of Profiles #4108

sjsf commented Aug 22, 2017

davidgraeff commented Aug 22, 2017 •

edited

Loading

sjsf commented Aug 22, 2017

kaikreuzer left a comment

kaikreuzer Sep 1, 2017

kaikreuzer Sep 1, 2017

sjsf Sep 4, 2017

kaikreuzer Sep 1, 2017

sjsf Sep 4, 2017

sjsf Sep 4, 2017

kaikreuzer Sep 4, 2017

kaikreuzer Sep 1, 2017

kaikreuzer Sep 1, 2017

kaikreuzer Sep 1, 2017

sjsf Sep 4, 2017

kaikreuzer commented Sep 5, 2017

sjsf commented Sep 5, 2017

kaikreuzer commented Sep 5, 2017

maggu2810 left a comment

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 Sep 6, 2017

maggu2810 commented Sep 6, 2017

kaikreuzer commented Sep 6, 2017

maggu2810 commented Sep 6, 2017

sjsf commented Sep 6, 2017

openhab-bot commented Dec 1, 2018


		private final Logger logger = LoggerFactory.getLogger(CommunicationManager.class);
		private final Set<String> subscribedEventTypes = ImmutableSet.of(ItemStateEvent.TYPE, ItemCommandEvent.TYPE,

Introduction of Profiles #4108

Introduction of Profiles #4108

Conversation

sjsf commented Aug 22, 2017

davidgraeff commented Aug 22, 2017 • edited Loading

sjsf commented Aug 22, 2017

kaikreuzer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaikreuzer commented Sep 5, 2017

sjsf commented Sep 5, 2017

kaikreuzer commented Sep 5, 2017

maggu2810 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maggu2810 commented Sep 6, 2017

kaikreuzer commented Sep 6, 2017

maggu2810 commented Sep 6, 2017

sjsf commented Sep 6, 2017

openhab-bot commented Dec 1, 2018

davidgraeff commented Aug 22, 2017 •

edited

Loading