Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add an AbstractStorageBasedTypeProvider #3407

Merged
merged 3 commits into from
May 7, 2023
Merged

Add an AbstractStorageBasedTypeProvider #3407

merged 3 commits into from
May 7, 2023

Conversation

J-N-K
Copy link
Member

@J-N-K J-N-K commented Feb 26, 2023 •
edited
Loading

Related to #3394

This adds a combined type provider for use in bindings that create ChannelTypes, ThingTypes or ChannelGroupTypes dynamically. It provides methods for adding/removing these types and persists them, so they are available on next start-up. Together with #3397 this should allow a smooth transition for all bindings.

There are two reference implementations for use in bindings: openhab/openhab-addons#14501 and openhab/openhab-addons#14507

@J-N-K J-N-K added the enhancement An enhancement or new feature of the Core label Feb 26, 2023
@J-N-K J-N-K requested a review from a team as a code owner February 26, 2023 11:57
@lolodomo
Copy link
Contributor

lolodomo commented Feb 27, 2023 •
edited
Loading

You are pointing twice the same PR. Please fix one of them to point 14501.

@splatch
Copy link
Contributor

splatch commented Mar 2, 2023

One note about naming - its not only a dynamic provider, its storage based. The dynamism part is far less specific than the way how it retains its sate.

@digitaldan
Copy link
Contributor

digitaldan commented Mar 3, 2023 •
edited
Loading

This is a list of openhab-addons that use ChannelTypeBuilder, i am guesing these are the likely addons that need updating?

/~https://github.com/search?q=repo%3Aopenhab%2Fopenhab-addons+ChannelTypeBuilder&type=code&p=2

@J-N-K J-N-K changed the title Add an AbstractDynamicTypeProvider Add an AbstractStorageBasedTypeProvider Mar 4, 2023
@J-N-K
Copy link
Member Author

J-N-K commented Mar 4, 2023

TR-064 is not affected, it dynamically creates the types, but does so when the binding starts up. Only bindings that create the thing/channel-type when the thing initializes are affected. But there is no need to rush, with #3397 in place it only causes a delay on startup but does not prevent the thing from initializing.

J-N-K added 2 commits March 4, 2023 12:38
@J-N-K
Add an AbstractDynamicTypeProvider
fb6a7bd 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K
address review comment
93c603c 
Signed-off-by: Jan N. Klug <github@klug.nrw>
@J-N-K J-N-K force-pushed the persistentthingtype branch from cac5a9e to 93c603c Compare March 4, 2023 12:02
@digitaldan
Copy link
Contributor

But there is no need to rush, with #3397 in place it only causes a delay on startup but does not prevent the thing from initializing.

Thanks for the workaround, and fixes!

@wborn wborn linked an issue Mar 11, 2023 that may be closed by this pull request
Binding in "NOT_YET_READY" state after recent core changes #3394
Closed
@J-N-K J-N-K mentioned this pull request Mar 22, 2023
Closed
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/openhab-4-0-snapshot-discussion/142322/293

@ccutrer
Copy link
Contributor

ccutrer commented Apr 18, 2023

Is this PR waiting for something? I'm not sure when I'll have time (my discretionary coding time is much less than it used to be), but I'd like to get started on using this for mqtt.homie and mqtt.homeassistant at some point. I suppose this PR doesn't have to be merged before I do that, it would just be a little bit easier.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 18, 2023

It's waiting for review. From the code it's complete.

@J-N-K J-N-K requested review from a team and removed request for a team April 18, 2023 14:35
@ccutrer
Copy link
Contributor

ccutrer commented Apr 18, 2023

Happy to look, though I'm not a core maintainer. I looked through this PR, as well as the two linked reference implementations.

@lolodomo
Copy link
Contributor

lolodomo commented Apr 30, 2023 •
edited
Loading

I can see that the locale parameter of the "get" methods are ignored in your implementation.
Was it handled in the existing abstract thing type / channel type providers ?

@openhab/core-maintainers : as several add-ons will require an update, please review & merge this PR not late (not just before the official release) to let each binding maintainer the time to adjust the bindings they maintained.

@lolodomo
Copy link
Contributor

I can see that the locale parameter of the "get" methods are ignored in your implementation.
Was it handled in the existing abstract thing type / channel type providers ?

It is at least handled in class DefaultSystemChannelTypeProvider.

@lolodomo
Copy link
Contributor

I can see that the locale parameter of the "get" methods are ignored in your implementation.
Was it handled in the existing abstract thing type / channel type providers ?

It is at least handled in class DefaultSystemChannelTypeProvider.

Maybe I am wrong and for dynamic types, it is rather the responsibility of each binding to add/provide already localized types.

@J-N-K
Copy link
Member Author

J-N-K commented Apr 30, 2023

If we look at channel labels and description for managed things: they are localized during creation and then the label/description is stored/restored from the database. The same thing applies here: The localization should take place during creation of the channel-type/thing-type and is then stored.

@lolodomo
Copy link
Contributor

lolodomo commented May 2, 2023

The localization should take place during creation of the channel-type/thing-type and is then stored.

I think you're right. So locale can be ignored, no problem.

kaikreuzer
kaikreuzer approved these changes May 7, 2023
View reviewed changes
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, thanks.

...s/src/main/java/org/openhab/core/thing/binding/AbstractStorageBasedTypeProviderOSGiTest.java Outdated Show resolved Hide resolved
@kaikreuzer
Update itests/org.openhab.core.thing.tests/src/main/java/org/openhab/…
2ed766a 
…core/thing/binding/AbstractStorageBasedTypeProviderOSGiTest.java

Signed-off-by: Kai Kreuzer <kai@openhab.org>
@kaikreuzer kaikreuzer merged commit 8e1a2cf into openhab:main May 7, 2023
@kaikreuzer kaikreuzer added this to the 4.0 milestone May 7, 2023
@J-N-K J-N-K deleted the persistentthingtype branch May 7, 2023 19:56
@lolodomo lolodomo mentioned this pull request May 8, 2023
Open
30 tasks
@lolodomo
Copy link
Contributor

lolodomo commented May 9, 2023

@J-N-K : as mentioned in openhab/openhab-addons@14954, I encounter deserialization errors when using this new class:

00:04:16.548 [ERROR] [ore.storage.json.internal.JsonStorage] - Couldn't deserialize value 'org.openhab.core.storage.json.internal.StorageEntry@35e456f3'. Root cause is: Interfaces can't be instantiated! Register an InstanceCreator or a TypeAdapter for this type. Interface name: org.openhab.core.types.StateDescriptionFragment

@J-N-K
Copy link
Member Author

J-N-K commented May 9, 2023

Do you think you can provide a PR for that?

@lolodomo
Copy link
Contributor

lolodomo commented May 9, 2023

Do you think you can provide a PR for that?

Unfortunately not, I should admit all this is a little obscure for me.

splatch pushed a commit to ConnectorIO/copybara-hab-core that referenced this pull request Jul 12, 2023
@J-N-K @splatch
Add an AbstractStorageBasedTypeProvider (openhab#3407)
e565152 
* Add an AbstractDynamicTypeProvider

Signed-off-by: Jan N. Klug <github@klug.nrw>
GitOrigin-RevId: 8e1a2cf
@openhab-bot
Copy link
Collaborator

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

https://community.openhab.org/t/incorporating-matter/127907/772

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kaikreuzer kaikreuzer kaikreuzer approved these changes

@ccutrer ccutrer ccutrer approved these changes

Assignees
No one assigned
Labels
enhancement An enhancement or new feature of the Core
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.

Binding in "NOT_YET_READY" state after recent core changes
7 participants
@J-N-K @lolodomo @splatch @digitaldan @openhab-bot @ccutrer @kaikreuzer