[WIP] Fix thing creation for updated XML #1027

afuechsel · 2019-09-10T07:52:57Z

This is a new approach to solve the problem that I was trying to solve in #628 that in turn was based on another PR of @J-N-K (#527).

It works as long as there are no user-defined channels, therefore it's still work in progress. I'd like to ask for our comments about this approach.

ThingManager#doInitializeHandler calls BaseThingHandler#setChannelsFromThingTypeDefinition to only create channels, that are actually defined in the XML. This allows updated XML descriptions for already existing things to be taken into account.

Signed-off-by: André Füchsel [email protected]

…sFromThingTypeDefinition` to only create channels, that are actually defined in the XML. This allows updated XML descriptions for already existing things to be taken into account. Signed-off-by: André Füchsel <[email protected]>

cweitkamp · 2019-09-14T07:10:17Z

Just FTR: Possible solution for eclipse-archived/smarthome#2555.

cweitkamp

Thanks for working on this feature. Very much appreciated.

cweitkamp · 2019-09-14T07:15:11Z

...nhab.core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/BaseThingHandler.java

+                configDescriptionRegistry);
+
+        ThingBuilder thingBuilder = editThing();
+        thingBuilder.withChannels(channels);


IIRC this method clears the list of all existing channels. We have to filter the list of new channels to add only non existing ones. On the other hand we have to deal with removal of channels and changes in channel types too.

Be aware there currently is an issue regarding the withoutChannels for the BridgeBuilder - it returns a ThingBuilder instance - see https://github.com/openhab/openhab-core/pull/908/files#diff-44fdf6ebca4daa065ec6088297fb2859R82.

We have to filter the list of new channels to add only non existing ones

Why ? (What if there are also channels removed from the XML ?)

Indeed this method clears all channels on the thing and replaces them with the one declared in the XML, but once the handler is initialized again - all other channels (added dynamically) will be re-added (it will behave just like if the thing is paired for a very first time)

Yes, exactly but please keep in mind that channels can have a user specific configuration. You will throw away any of them when you re-add all the channels. We should try to keep them.

and there are channel that can be added by user interaction -- so manually per request. That information will be lost, too.

Yes - I know - this is already mentioned in the description in the PR.

Few questions on that topic:
How are actually those channels used ?
How are the stated updated if the handler doesn't know about them.
How commands sent to those channels handled ?

Signed-off-by: André Füchsel <[email protected]>

afuechsel · 2019-09-26T09:27:10Z

Hmm, not sure why the build failed. Just pushed an update to my branch...

cweitkamp · 2019-09-26T09:33:27Z

I think it failed because there are merge conflicts due to my recent builder refactoring (see #908).

afuechsel · 2019-09-26T11:35:47Z

I think it failed because there are merge conflicts due to my recent builder refactoring (see #908).

Maybe, but after a rebase I am not sure how to fix my code, because BridgeBuilder now cannot access the thing. I studied your PR about changing builders but I do not completely understand, why this change was necessary and what exactly is the improvement.

cweitkamp · 2019-09-26T14:43:39Z

A builder should build independent instances of an entity each time you call its build() method. This was not the case before my refactoring (e.g. if you built a thing once and called withLabel(...) a second time on the builder it should not change the label of the previous created thing).

The solution for your approach is to add an internal list of things to the BridgeBuilder. You can move the logic to add things to the bridge into the build() method. Is that a possible solution for you?

cweitkamp

@afuechsel Do you have an idea how to continue?

I did some tests with your implementation and it looks very good. I did the rebase like this: afuechsel@a5f55bb.

cweitkamp · 2019-10-08T09:27:44Z

...core.thing/src/main/java/org/eclipse/smarthome/core/thing/binding/builder/BridgeBuilder.java

@@ -101,4 +102,13 @@ public BridgeBuilder withLocation(@Nullable String location) {
        return (BridgeBuilder) super.withLocation(location);
    }

+    public BridgeBuilder withThings(@Nullable List<Thing> things) {
+        BridgeImpl bridge = ((BridgeImpl) thing);


Some additional stuff:

validateThingUID(thing) - makes sure the thing belongs to the bridge

ensureUniqueThing(thing) - ensures the thing will be added only once

Bear with me - I am short of time. I think, the only issue left open is the removal of channels during editThing, that are not defined in XML and therefore must not be removed. I have no clear idea yet, how to solve this.

It might help to add a field to the Channel class for tracking the origin. Then it can be removed if it was created from the ThingType definition.

There are also some other corner cases:

The new ThingType defines a channel that was already created by the user. In that case the user channel and configuration might get lost because it is replaced with that defined in the ThingType.

An existing channel is renamed. If the channel has configuration parameters its values will be lost when the old channel is removed.

I'm fine with skipping those corner cases for now because I think they won't occur often and the changes in this PR would do more good than harm. :-)

It might help to add a field to the Channel class for tracking the origin.

Yes, that is true. But I am afraid it is not possible until we found a solution for #731.

btw: I prepared a new rebase for these changes including some unit tests. master...cweitkamp:pr-1027

openhab-bot · 2020-01-16T11:49:36Z

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

https://community.openhab.org/t/openhab-unstable-after-upgrade-to-2-5/87861/65

openhab-bot · 2020-02-06T16:18:00Z

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

https://community.openhab.org/t/system-started-triggered-every-minute-with-2-5-0-stable/89970/19

5iver · 2020-02-06T16:20:54Z

How are things coming along with this PR? Any chance this could be added to the OH 3.0 backlog? Is that the intent of this project... https://github.com/orgs/openhab/projects/3?

cweitkamp · 2020-02-07T15:11:24Z

Any chance this could be added to the OH 3.0 backlog?

Done. I absolutely vote for it.

cdjackson · 2020-02-07T15:27:16Z

We have to filter the list of new channels to add only non existing ones. On the other hand we have to deal with removal of channels and changes in channel types too.

Just a comment on this - we also need to handle changes to channels. Channels have properties and we need to manage changes in these as well.

At some point in the past I had proposed to add a version number to the thing definition so that it was explicit that the definition had changed rather than trying to detect the all changes which might be problematic.

openhab-bot · 2020-03-19T19:28:01Z

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

https://community.openhab.org/t/how-should-i-upgrade-existing-things-when-i-update-my-binding/94842/4

J-N-K · 2020-04-13T22:11:01Z

Another year done and still no solution to it. The longer it takes, the more I am convinced that my original idea with versioned thing-types and upgrade-logic within the binding is a better and more viable solution than this general approach.

afuechsel · 2020-04-14T06:04:37Z

@J-N-K Unfortunately it does not. I tried several times with your original solution and I was not able to get it to work completely. I was not able to get all tests green nor did the newly created tests work out well. In the runtime it only worked sometimes. This was due to the dynamics of the thing handler creation and their conflict with the ready marker triggers. Also the introduction of a queue of all not YET resolved thing types did not work out well. I have a 95% solution still as a branch.

Therefore we decided to go with the now shown solution here and it worked out pretty well (with some modifications not yet part of this PR). In our solution that is still based on the frozen old ESH code base we now have a working implementation of it.

I also tried to address the openHAB issue with user-created channels, something we don´t have in our solution. The easiest way was the introduction of a flag in the channels that indicate whether or not they have been created by the XMLs.

Although I am not actively contributing to openHAB I was hoping I could eventually finish this PR but it seems I am lacking the time to. May be I could find some time to at least push the latest changes from our code base which works for us.. The problem now is that openHAB evolved over the time but we are still based on plain ESH which is slightly different. Sorry for that.

J-N-K · 2020-04-14T06:15:15Z

@afuechsel my original idea was implemented in the OneWire binding in ESH, before it got merged and removed during review. It simply had a property thingTypeVersion for each thing-type, a number that has to be increased by the developer each time changes to the thing type are done. The thing handlers check the version number during initialization and upgrade the thing when necessary. Since the developer knows what changed, he can make the correct decision which channels need to be added, changed or removed and what needs to be kept.

afuechsel · 2020-04-14T08:48:17Z

@J-N-K Feel free to revert the changes here and go with your idea. Maybe I made a mistake since the update with your implementation never worked for me. As I said, I have unfortunately not enough time to deal deeper with openHAB.

openhab-bot · 2020-06-01T10:32:24Z

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

https://community.openhab.org/t/solved-openweathermap-binding-no-difference-between-condition-id-and-icon-id/99829/9

J-N-K · 2020-12-13T08:18:50Z

IMO removing this from OH3 backlog is a big step backwards. It's now more than four years that we have to delete and re-create things after each update that adds new features.

kaikreuzer · 2020-12-13T09:30:21Z

@J-N-K What do you mean? It is no step backward, it is just no step forward.

J-N-K · 2020-12-13T10:16:18Z

It‘s a step backward to remove it from the backlog. Previously this was accepted as an issue to be solved for OH3.

kaikreuzer · 2020-12-13T11:31:43Z

Keeping it in the backlog won't fix it, though. If you can come up with a PR and a fix for it in time for 3.0, I'll happily put it back.

J-N-K · 2020-12-13T11:45:05Z

(moved comment to #1924)

Signed-off-by: Ben Clark <[email protected]>

wborn · 2021-11-19T21:16:59Z

Hello @afuechsel, do you think you will find time to continue working on this PR?

afuechsel · 2021-11-20T09:03:52Z

Unfortunately I no more time to work for openHAB. Feel free to close this PR or someone else could continue with it. It’s rather old now.

wborn · 2021-11-20T09:58:35Z

Thanks for letting us know @afuechsel! 👍

I've archived your branch in my own repo, so someone can pick this up in the future as it certainly looked like this was the most promising approach so far.

cweitkamp added the work in progress A PR that is not yet ready to be merged label Sep 14, 2019

cweitkamp reviewed Sep 14, 2019

View reviewed changes

preserving channel configuration

3a95765

Signed-off-by: André Füchsel <[email protected]>

cweitkamp reviewed Oct 8, 2019

View reviewed changes

This was referenced Nov 2, 2019

RFXCom lighting1 commandString channel openhab/openhab-addons#6285

Closed

[lifx] Add support for LIFX Tile and Tile Effects openhab/openhab-addons#6180

Merged

cweitkamp added the enhancement An enhancement or new feature of the Core label Dec 7, 2019

cweitkamp mentioned this pull request Oct 13, 2020

[icalendar] EventFilter for existing calendars openhab/openhab-addons#8583

Merged

wborn mentioned this pull request Dec 13, 2020

Update existing Things for definition changes #1924

Closed

wborn linked an issue Dec 13, 2020 that may be closed by this pull request

Update existing Things for definition changes #1924

Closed

Rosi2143 pushed a commit to Rosi2143/openhab-core that referenced this pull request Dec 26, 2020

Change testing repository to Artifactory (openhab#1027)

7f73c89

Signed-off-by: Ben Clark <[email protected]>

afuechsel requested a review from a team as a code owner December 28, 2020 08:22

Base automatically changed from master to main January 18, 2021 20:04

wborn added the stale As soon as a PR is marked stale, it can be removed 6 months later. label Nov 19, 2021

wborn closed this Nov 20, 2021

[WIP] Fix thing creation for updated XML #1027

[WIP] Fix thing creation for updated XML #1027

Conversation

afuechsel commented Sep 10, 2019

cweitkamp commented Sep 14, 2019

cweitkamp left a comment

Choose a reason for hiding this comment

cweitkamp Sep 14, 2019

Choose a reason for hiding this comment

ivivanov-bg Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

cweitkamp Sep 16, 2019

Choose a reason for hiding this comment

maggu2810 Sep 16, 2019

Choose a reason for hiding this comment

ivivanov-bg Sep 16, 2019 • edited Loading

Choose a reason for hiding this comment

afuechsel commented Sep 26, 2019

cweitkamp commented Sep 26, 2019

afuechsel commented Sep 26, 2019

cweitkamp commented Sep 26, 2019

cweitkamp left a comment

Choose a reason for hiding this comment

cweitkamp Oct 8, 2019

Choose a reason for hiding this comment

afuechsel Oct 8, 2019

Choose a reason for hiding this comment

wborn Oct 23, 2019 • edited Loading

Choose a reason for hiding this comment

cweitkamp Nov 12, 2019

Choose a reason for hiding this comment

openhab-bot commented Jan 16, 2020

openhab-bot commented Feb 6, 2020

5iver commented Feb 6, 2020

cweitkamp commented Feb 7, 2020

cdjackson commented Feb 7, 2020

openhab-bot commented Mar 19, 2020

J-N-K commented Apr 13, 2020

afuechsel commented Apr 14, 2020 • edited Loading

J-N-K commented Apr 14, 2020 • edited Loading

afuechsel commented Apr 14, 2020

openhab-bot commented Jun 1, 2020

J-N-K commented Dec 13, 2020

kaikreuzer commented Dec 13, 2020

J-N-K commented Dec 13, 2020

kaikreuzer commented Dec 13, 2020

J-N-K commented Dec 13, 2020 • edited Loading

wborn commented Nov 19, 2021

afuechsel commented Nov 20, 2021

wborn commented Nov 20, 2021 • edited Loading

ivivanov-bg Sep 16, 2019 •

edited

Loading

ivivanov-bg Sep 16, 2019 •

edited

Loading

wborn Oct 23, 2019 •

edited

Loading

afuechsel commented Apr 14, 2020 •

edited

Loading

J-N-K commented Apr 14, 2020 •

edited

Loading

J-N-K commented Dec 13, 2020 •

edited

Loading

wborn commented Nov 20, 2021 •

edited

Loading