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.

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

[WebThing] Initial contribution #9555

Merged
merged 16 commits into from
Apr 11, 2021
Merged

[WebThing] Initial contribution #9555

merged 16 commits into from
Apr 11, 2021

Conversation

grro
Copy link
Contributor

@grro grro commented Dec 28, 2020

Signed-off-by: Gregor Roth gregor.roth@web.de

This merge request includes a WebThing binding for openHAB. It replaces the earlier PR #9185 which has been closed due to underlying git problems.

The WebThing binding supports an interface to remote devices implementing the Web Thing API. The Web Thing API is specified by the W3C Consortium (draft). The specification describes how a device or thing can be discovered and linked based on web technologies.
Essentially, a Web Thing API is a RESTful interface including WebSockets support to provide real-time notifications of events as soon as they happen.

Please refer to the readme.md for more detailed information

@andrewfg
Copy link
Contributor

@grro this is very interesting. It occurs to me that this binding is really only half the story; it is something that imports WoT things into OH. The other half of the story would be something that exports OH Things as WoT things. Probably such an WoT exporter would need to be implemented as part of the OH core rather than as an OH add-on (since for example it would need to access the Thing registry). It might for example be an extension of the REST server. So I wonder if your WoT import binding needs any extensions or adaptions to its architecture to prepare the way for a future WoT export add-on? Do you have any thoughts about this?

@grro
Copy link
Contributor Author

grro commented Dec 28, 2020

@grro this is very interesting. It occurs to me that this binding is really only half the story; it is something that imports WoT things into OH. The other half of the story would be something that exports OH Things as WoT things. Probably such an WoT exporter would need to be implemented as part of the OH core rather than as an OH add-on (since for example it would need to access the Thing registry). It might for example be an extension of the REST server. So I wonder if your WoT import binding needs any extensions or adaptions to its architecture to prepare the way for a future WoT export add-on? Do you have any thoughts about this?

Hi,

this PR supports consuming WebThings (as a client) only. My main motivation behind this was to reach a better integration of my pi-based hardware solutions that provides the WebThing API. Providing a WebThing binding could help makers to make their solutions more interoperable by implementing a lightweight standard, for instance, such as in my case.

Providing openHAB artifacts via the WebThings API would also be nice. However, my impression is, that the need for consuming support is higher. Exposing WebThings may be a second step behind this PR. I would prefer to implement features step by step in an iterative, incremental way.

Essentially, this binding implements (1) a client-side interface similar to https://www.w3.org/TR/wot-scripting-api/#the-consumedthing-interface, (2) the WebThing discovery, (3) the (semantic) mapping of WebThing properties <-> openHAB channels as well as (4) the integration into the openHAB infrastructure. At least (3) the (semantic) mapping of WebThing properties <-> openHAB channels should be reusable by providing the server-side support. I have tried to keep the implementation design as clean (-> separation of concerns, encapsulation dependencies, etc) as possible which may help

@andrewfg
Copy link
Contributor

^
Very interesting. Thank you.

@Hilbrand Hilbrand added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 29, 2020
Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

@grro
Copy link
Contributor Author

grro commented Dec 31, 2020

There are some checkstyle warnings left. You could take a look at target/code-analysis/report.html.

The warning are NullAnnotationsCheck warnings.

I understood that this check does not make sense for classes such as WebThingConfiguration. Additionally, the NullAnnotationsCheck for test classes seems to be a slight exaggeration in my opinion. What do you think?
I have also tried to suppress the checkstyle rules for these cases without success

@fwolter
Copy link
Member

fwolter commented Dec 31, 2020

The null annotations can be omitted for DTOs. The configuration should be annotated as the values can be null if the user forgets to specify a parameter in textual configuration.

The annotations are also missing for PropertyAccessException, WebSocketConnectionImpl and UnknownPropertyException.

@grro
Copy link
Contributor Author

grro commented Dec 31, 2020

The null annotations can be omitted for DTOs. The configuration should be annotated as the values can be null if the user forgets to specify a parameter in textual configuration.

The annotations are also missing for PropertyAccessException, WebSocketConnectionImpl and UnknownPropertyException.

checkstyle warnings are fixed now

Copy link
Member

@fwolter fwolter left a comment

Choose a reason for hiding this comment

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

LGTM

Since this is a new binding, another maintainer needs to review your code before it can be merged.

@fwolter fwolter added the cre Coordinated Review Effort label Jan 5, 2021
@grro
Copy link
Contributor Author

grro commented Jan 5, 2021

LGTM

Since this is a new binding, another maintainer needs to review your code before it can be merged.

Thank you

@grro
Copy link
Contributor Author

grro commented Jan 11, 2021

LGTM
Since this is a new binding, another maintainer needs to review your code before it can be merged.

Thank you

Do I have to do anything regarding the second reviewer?

@fwolter
Copy link
Member

fwolter commented Jan 12, 2021

You don't need to as the "cre" tag signals a pending second review, but you can additionally request a review from openhab/add-ons-maintainers.

@grro
Copy link
Contributor Author

grro commented Jan 14, 2021

You don't need to as the "cre" tag signals a pending second review, but you can additionally request a review from openhab/add-ons-maintainers.

I have provided a bugfix commit. It turned out that there are webthing descriptions that include several "alternate" links.

Furthermore, I noticed that my local build reports "Header line doesn't match pattern ^ * Copyright (c) 2010-2020 Contributors to the openHAB project$" errors. These errors have not appeared in my local build of the previous commit (9 days ago).

@fwolter
Copy link
Member

fwolter commented Jan 14, 2021

Your changes look good. To fix the build, you need to rebase your code to get the latest checkstyle rules.

Here are the commands for rebasing your branch:

If not already done, add the upstream openHAB addon repo as a remote to your local repo and fetch it:

git remote add upstream https://github.com/openhab/openhab-addons.git
git fetch upstream

Then, you can rebase your PR's branch onto main:

git rebase upstream/main

Finally force-push the rebased branch to this PR's branch:

git push origin [your branch name of this PR] --force-with-lease

grro and others added 11 commits January 14, 2021 11:51
Signed-off-by: Gregor Roth <gregor.roth@web.de>
…/binding/webthing/internal/WebThingHandler.java

Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
…/binding/webthing/internal/discovery/WebthingDiscoveryService.java

Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
…/binding/webthing/internal/discovery/WebthingDiscoveryService.java

Co-authored-by: Fabian Wolter <github@fabian-wolter.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: Gregor Roth <gregor.roth@web.de>
…. E.g. there may non-WebSocket "alternate" links such as links of mediaType text/html

Signed-off-by: Gregor Roth <gregor.roth@web.de>
@fwolter
Copy link
Member

fwolter commented Feb 27, 2021

will there be a second review? is there no interest in contributing this?

You could request a review from @openhab/add-ons-maintainers.

@grro
Copy link
Contributor Author

grro commented Mar 2, 2021

will there be a second review? is there no interest in contributing this?

You could request a review from @openhab/add-ons-maintainers.

Unfortunately, the link returns 404.

@bobadair
Copy link
Member

bobadair commented Mar 2, 2021

Unfortunately, the link returns 404.

That's weird. If you click on "reviewers" in the upper right corner and start typing "openhab" you should see openhab/add-ons-maintainers come up. You shouldn't really need to do that, though. It just takes a while for another review sometimes. It looks like there are a lot of new bindings in the queue right now, which is good news for openHAB but a lot of work for the maintainers!

@moonthug
Copy link

moonthug commented Mar 9, 2021

Will this PR be reviewed anytime soon? Interested in this functionality in OpenHAB

Signed-off-by: Fabian Wolter <github@fabian-wolter.de>
@fwolter
Copy link
Member

fwolter commented Apr 11, 2021

Due to lack of review capacity, the rules for new bindings were weakened. New bindings need only the approval of one reviewer instead of two from now on.

@fwolter fwolter merged commit d9ed461 into openhab:main Apr 11, 2021
@fwolter fwolter changed the title [WebThing] Initial contribution (replaces former PR#9185) [WebThing] Initial contribution Apr 11, 2021
@fwolter fwolter removed the cre Coordinated Review Effort label Apr 11, 2021
@fwolter fwolter added this to the 3.1 milestone Apr 11, 2021
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
Signed-off-by: Gregor Roth <gregor.roth@web.de>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
Signed-off-by: Gregor Roth <gregor.roth@web.de>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: Gregor Roth <gregor.roth@web.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new binding If someone has started to work on a binding. For a new binding PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants