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

[kgoderis]

@sjka @kaikreuzer DefaultMasterProfile does not support items configuration like for example:

DateTime Time "Time [%1$tT]" { channel="ntp:ntp:myhometown:dateTime", channel="knx:generic:ip1:1_1_12:9_1_1"}

Well, it does, but in comparison to for example openHAB 1, it is no longer possible to have a binding, in casu NTP, update the Time Item, and automatically have the runtime post an update for that Item so that the same State can be handled by the other binding, in casu KNX

e.g. I presume it was by design that DefaultMasterProfile contains

    @Override
    public void onUpdate(ItemChannelLink link, Thing thing, State state) {
        // no-op
    }

Maybe I overlooked something, but shouldn't a simple Profile, similar to RawButtonToggleProfile, be provided in the core to enable these kinds of item configurations? Or, from a user perspective, should these kind of Item configuration be handled through Rules?

[kgoderis]

Ok, The above example/usage can be solved by assigning a DefaultSlaveProfile to the given knx channel, but on a more general note, referring to https://github.com/eclipse/smarthome/pull/2881, which is superseded by Profiles concept, there might be a benefit to do away with the Autoupdate binding, and rather re-implement it through Profiles, possibly extending it with the inter-binding and intra-binding bridging concepts of https://github.com/eclipse/smarthome/pull/2881

That being said, I am wondering if we could/should enable access to the ItemChannelLinkRegistry for these and other more advanced use-cases?

[sjsf]

The above example/usage can be solved by assigning a DefaultSlaveProfile to the given knx channel

That's exactly its purpose!

there might be a benefit to do away with the Autoupdate binding, and rather re-implement it through Profiles

Yes, that's the plan to make it a configuration of the default master profile. The tricky part will be to keep backward compatibility for openHAB's compatibility layer.

possibly extending it with the inter-binding and intra-binding bridging concepts

I don't think this should be in scope here. You brought this up in the context of the KNX binding. This however is something that could be "implemented" in KNX side by assigning common GAs and therefore rather should be done there. I'd really like to avoid KNX concepts sneaking into ESH.

[kgoderis]

@sjka In any case, the Profiles PR breaks the "bridging-via-Items" concept that was introduced by OH1, and that people do use in in their OH2 setups as well (as such nothing to do with the Autoupdate binding, that's an as you point out another issue to deal with). It surprises me that no-one complained about this up to now. The short-term workaround is to work with Rules and additional Items to copy over values.

[sjsf]

No, it doesn't. It just makes it explicit.

Hardly any ESH/OH2 binding implemented the handleUpdate() at all. And those very few (which you can count on one hand) didn't do there what you would habe expected. Mainly I think because nobody really got the idea of that method. So that "bridging" magic that you are referring to never worked anyway. That's why nobody complained I guess.

For sure, you can still bind two channels to the same item using the masterprofile, so they will get the same commands. And now you can newly make a channel simply follow the state of another one using the slave profile, without the need to fix all (!) of the bindings that we currently have.

[philomatic]

I'm sorry to say this (that late):

Personally I have the strong feeling that the profiles (or specific: the DefaultSlaveProfile in combination with the ESH concepts) is inconsistent and somehow.... just "wrong", at least for me. Let me explain this (or just jump to tl;dr :) )

No, it doesn't. It just makes it explicit.

I feel the opposite is the case. At least the current state confuses me a lot.

With #4108, the usage of handleUpdate(...) is somewhat broken:

• It is never called for state updates anymore.
• Therefore it's deprecated (usage and overriding), but still in the docs: https://github.com/eclipse/smarthome/blob/master/docs/documentation/development/bindings/thing-handler.md

So bindings which use handleUpdate(...) will fail with runtime errors, as it's still declared in the interface. Maintainers won't notice this otherwise as with new bugs. Following your logic ("And those very few..."), the method should be dropped at least (as it only affects very few people), so people have a chance (by build failure) to notice this directly.

The newly introduced DefaultSlaveProfile reacts on onUpdate(...) and forwards/converts this to handleCommand(...) in the ThingHandler. But why happens this in onUpdate(...)? What's the purpose of onCommand(...) then? Compare the declarations and the first locs:

public void onUpdate(ItemChannelLink link, Thing thing, State state) {
        if (!(state instanceof Command)) {

...

public void onCommand(ItemChannelLink link, Thing thing, Command command) {

I may be too stupid ( 😉 ) but this looks odd to me like a false-friend, even in the meaning of slaves.

How can the use case / feature

a thing handler of a binding wants to get (non-command) state updates for a linked item,

which worked as documented before, now be fulfilled? Saying that "almost/probably no-one used this feature before, so let's strike out it" does not convince me. If developers used this feature in a wrong way, it's up to these programmers and reviewers to address this. But not by changing/deleting a (for me a clear documented) api method.

Writing a new profile (a StateUpdateProfile) for this feature would be fine for me, but it won't work without handleUpdate(...).

Don't get me wrong, the profiles introduce a new interesting way of configuration (and I'm glad you implemented this feature) but I'm unhappy with the current state. It's an inconsistent API for me, as handleCommand(...) (and others) are still there and handleUpdate(...) is not.

Hardly any ESH/OH2 binding implemented the handleUpdate() at all. And those very few (which you can count on one hand) didn't do there what you would habe expected

You're right, but you can say that only about those bindings in the current master. And yes, most of them do not use handleUpdate(...) as it's supposed to. (though mihome does so, lutron in a way complicated).

However, there are many bindings which you can't see:

• in private only use
• in current development
• closed source

Yes, ESH is open-source and as it's not v1.0 released yet, API changes are perfectly fine for me. However, it should be also developer friendly and as it's an EPL licensed framework, also suitable for closed-source extensions (?). So imo, (breaking) API changes should be noticeable in the code (if applicable) and therefore in build failures, but not appear later on silently in the runtime?

As for me, I can't track all PRs, Issues and discussions on ESH/OH2 due to time reasons, so I did not follow the changes introduced with https://github.com/eclipse/smarthome/pull/4108, https://github.com/eclipse/smarthome/pull/4204 and https://github.com/eclipse/smarthome/pull/4374.

As our builds (both, for closed-source and a new open-source binding) went fine, I didn't expect such a change and was very surprised about the current state. By now, I just have stumbled across a bug in our system which was introduced by these changes. (It would have been detected by a complete system/integration test, so shame on me for that one 😉)

I agree with @kgoderis and I'm also surprised that no-one complained about this.

tl;dr The feature

a thing handler of a binding wants to get (non-command) state updates for a linked item,

should be brought back somehow, most ideal in combination with the new profiles.

Sorry for the wall of text and best regards

[kgoderis]

Hardly any ESH/OH2 binding implemented the handleUpdate() at all. And those very few (which you can count on one hand) didn't do there what you would habe expected. Mainly I think because nobody really got the idea of that method. So that "bridging" magic that you are referring to never worked anyway. That's why nobody complained I guess.

@sjka Mhh... going back in time, and that is certainly 2011-2012 or so, this was actually a kind of 'feature' of OH1. Anyways, I use it quite a lot in my setup, albeit I admit it is very often with the KNX binding, since KNX is the primary technology I have here at home. The initial KNX binding in OH1 did implement the handleUpdate() if I am not mistaken, but indeed very few others do.

That being said, I do agree that making it explicit is far more better, and that doing this via Profiles is the best way forward and offers the greatest flexibility for custom configurations.

I also tend to agree with @philomatic in the sense that we should be a bit more consistent. I think that part of the confusion in the early days stems from the fact that State and Command are quite similar, and often interchangeable since a lot of Types are inheriting from both State and Command. Therefore most binding developer dealt with both State and Commands in the handleCommand(), or at least tried to cram everything in there, or limit the information they send/receive from remote devices to those Types that both inherited from State and Command, and never bothered with handleUpdate() because of that

The alternative is that we take away the distinction between Command and State Types altogether, and that Command and Update get a different type of meaning. e.g. it would not be to differentiate between what kind of information is to be sent/was received by a Binding, but rather to differentiate between the sources the Type originated from. e.g. if the runtime sends a value to an Item, we call it a Command, and it passed along to the bindings over the Channels. If it is an Item that changes because the state of a connected Channel changes (through a Binding), then we call it an Update. At least, that's how I interpret the code in your Profiles contribution

Or alternatively, we do away with the distinction all together, and everything becomes a Command

[philomatic]

Therefore most binding developer dealt with both State and Commands in the handleCommand(), or at least tried to cram everything in there, or limit the information they send/receive from remote devices to those Types that both inherited from State and Command, and never bothered with handleUpdate() because of that

Which I honestly can't understand. Correct me if I'm wrong, but as I remember ESH worked the following way:

• ThingHandler.handleCommand(...) was only called in case of an incoming command, not for a state update.
• ThingHandler.handleUpdate(...) worked the other way round, called on incoming state "only" events, not commands (there was a bug once dealing with echoes from updateState(...), but I could be wrong).

So I don't get how someone could successfully deal with commands and states in one of these two methods only without calling each other. Yes, I was confused by this in the first way too, but the framework "pushes" you to implement the right method.

The distinction between command and state update is important imo and should not be thrown away or get mixed. It makes a difference if you just get noticed about e.g. a sensor reading or if you are called to execute an action (command). And I think there are / will be bindings which could use both information ways for state updates (from the event bus and their own implementation). Especially when taking io/transport bindings like MQTT into consideration.

Using the static class hierarchy in combination with the api methods is my preferred way for this, but I agree that this can be confusing. Maybe using dynamic runtime concepts (like an isCommand() flag, asking for the origin of an update (@kgoderis) or even s.th. like a decorator pattern) are less confusing and more developer friendly.

Profiles however are a great way to steer and fine adjust the information flow between framework and bindings (that's basically what they do in my understanding), allowing new concepts like slave bindings. But this should not degrading the API methods in ThingHandler or even drop support for some of them. These are two different independent things for me. Actually, I even think the "default" profile should behave like the (old) API methods indicate, but that's an detail and truly arguable.

ESH takes the claim to be an universal (?) framework for smart home applications. Therefore we should not only focus on the

Hardly any ESH/OH2 binding implemented the handleUpdate() at all.

thing, but take other appliances into consideration. The Eclipse devs won't drop a feature in the rich client platform just because the IDE doesn't use it at all, right? 😉

[kgoderis]

Therefore most binding developer dealt with both State and Commands in the handleCommand(), or at least tried to cram everything in there, or limit the information they send/receive from remote devices to those Types that both inherited from State and Command, and never bothered with handleUpdate() because of that Which I honestly can't understand. Correct me if I'm wrong, but as I remember ESH worked the following way:

ThingHandler.handleCommand(...) was only called in case of an incoming command, not for a state update. ThingHandler.handleUpdate(...) worked the other way round, called on incoming state "only" events, not commands (there was a bug once dealing with echoes from updateState(...), but I could be wrong).

yep, you are right but this different "interpretation" of handleCommand() and handleUpdate() stems from the pre-ESH-era, and not all bindings were reworked in the same way to follow the new heuristics.

@sjka pointed out that the Profiles included are examples, and that others need to follow, so I suggest to close here and await the inclusion of other Profiles that do deal with the handleUpdate() situation

[sjsf]

Although it's not directly related to the original topic, I would like to mention for the record here that with PR #4516 the default profile indeed behaves again like ESH used to before introducing the profiles. I still would like to understand better the use-case for the handleUpdate() in bindings but will open another issue for it. But thanks for the constructive feedback anyway!

[philomatic]

No problem, thanks for accepting it.

Looking forward to discuss (possible) use-cases for handleUpdate(...)

[kaikreuzer]

Looking forward to discuss (possible) use-cases for handleUpdate(...)

We are looking forward to your input on #4525 to see what use cases there actually are :-)