[Worxlandroid] Initial contribution #16893
Conversation
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/worx-landroid-binding/95246/570 |
jlaur
added
the
new binding
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/worx-landroid-binding/95246/609 |
|
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/worx-landroid-binding/95246/619 |
There was a problem hiding this comment.
Thanks for contributing this binding. This is a partly review, i only looked at the documentation, thing structure and some metadata files. I'll continue my review when i find some more time.
I noticed an images folder, i guess that can be removed as they do not seem to be used anywhere.
...src/main/java/org/openhab/binding/worxlandroid/internal/discovery/MowerDiscoveryService.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/openhab/binding/worxlandroid/internal/discovery/MowerDiscoveryService.java
Outdated
Show resolved
Hide resolved
...worxlandroid/src/main/java/org/openhab/binding/worxlandroid/internal/api/WorxApiHandler.java
Outdated
Show resolved
Hide resolved
...worxlandroid/src/main/java/org/openhab/binding/worxlandroid/internal/api/WorxApiHandler.java
Outdated
Show resolved
Hide resolved
...c/main/java/org/openhab/binding/worxlandroid/internal/handler/WorxLandroidBridgeHandler.java
Show resolved
Hide resolved
...c/main/java/org/openhab/binding/worxlandroid/internal/handler/WorxLandroidBridgeHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/worxlandroid/internal/handler/WorxLandroidMowerHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/worxlandroid/internal/handler/WorxLandroidMowerHandler.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/openhab/binding/worxlandroid/internal/handler/WorxLandroidMowerHandler.java
Outdated
Show resolved
Hide resolved
...d/src/main/java/org/openhab/binding/worxlandroid/internal/handler/AWSClientThingHandler.java
Show resolved
Hide resolved
| <groupId>software.amazon.awssdk.iotdevicesdk</groupId> | ||
| <artifactId>aws-iot-device-sdk</artifactId> | ||
| <version>1.15.0</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>software.amazon.awssdk.crt</groupId> | ||
| <artifactId>aws-crt</artifactId> | ||
| <version>0.25.1</version> | ||
| </dependency> | ||
| </dependencies> |
There was a problem hiding this comment.
This binding performs some similar requests/response, so it should be possible to create them without these SDK's.
Could you remove these dependencies?
There was a problem hiding this comment.
Can you be more explicit ? I'm afraid I did not get the meaning of your comment.
IIRW I tried to upgrade dependencies but it had major impact on code structure, so prefered to leave as it was.
There was a problem hiding this comment.
I meant to say that the SDK should not be used. Instead the httpclient can be used to perform the same api request/responses.
You could look at the salus binding that also holds a very light weight base class doing much of the SDK work without needing the actual SDK.
Due to problems of the past with size, upgrades and other issues we try to keep these SDK’s out of the codebase.
There was a problem hiding this comment.
@lsiepel : This is for Mqtt client of AWS IoT, I remember I failed to switch to another MQTT client (because of the need of a websocket for the authentication IIRW that was not available with available one). I'll have another look at it to be sure.
There was a problem hiding this comment.
Have not looked in depth on how the MQTT client and websocket / authentication work together. I just know that we tent not to accept these SDK's. For the salus binding we had a similar issue with the software.amazon.awssdk.crt package. A fix was found by some lightweight classes that implemented the authentication.
Hope you figure out how to get rid of the SDK with these suggestions, otherwise please post the specific problems you encounter and maybe we can find a fix together. If ultimately it seems not feasable, we can always merge it as is, but yeah really prefer not to..
There was a problem hiding this comment.
Not as far as I know. I Can give you m'y credentials by PM if you agree
There was a problem hiding this comment.
@lsiepel : gentle ping.
There was a problem hiding this comment.
I can't really help here with a solution, but just for your information, see also openhab/openhab-distro#1643. So probably 17 MB will not be acceptable.
There was a problem hiding this comment.
Have no0t forgotten this, but have not yet had the dedicated time to look at it. Would be nice if we can either build our own or use a lite mqtt over websockets client.
bundles/org.openhab.binding.worxlandroid/src/main/resources/OH-INF/i18n/worxlandroid.properties
Show resolved
Hide resolved
There was a problem hiding this comment.
Last bits are reviewed now, covered all files. Mainly documentation and a bigger one regarding the dependencies. It also looks like a netatmo file accidently became part of this PR.
The OP has no contents, could you link a community thread and or somewhere this binding has been tests or discusssed before? I noticed many file are signed by you, and some by 'Nils' did you proceed on someone else's PR?
lsiepel
changed the title
Updated OP to the community thread. I took over this binding for the 3.x => 4 migration, despite many requests the original creator ( Nibi79 in the community) never had time to push to common repo, so I decided to proceed. We had a discussion (him and myself) on this. He can take ownership of the PR as soon as he wants. |
...id/src/main/java/org/openhab/binding/worxlandroid/internal/WorxLandroidBindingConstants.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.worxlandroid/src/main/resources/OH-INF/thing/groups.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.worxlandroid/src/main/resources/OH-INF/thing/channels.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.worxlandroid/src/main/resources/OH-INF/thing/channels.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.worxlandroid/src/main/resources/OH-INF/addon/addon.xml
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.worxlandroid/images/SC_PaperUI_Mower.png
Outdated
Show resolved
Hide resolved
| <parent> | ||
| <groupId>org.openhab.addons.bundles</groupId> | ||
| <artifactId>org.openhab.addons.reactor.bundles</artifactId> | ||
| <version>4.3.0-SNAPSHOT</version> |
There was a problem hiding this comment.
| <version>4.3.0-SNAPSHOT</version> | |
| <version>5.0.0-SNAPSHOT</version> |
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Correcting pom.xml Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Signed-off-by: Gaël L'hopital <[email protected]>
Updated to 4.3 and two SAT corrections Correcting pom.xml Signed-off-by: Gaël L'hopital <[email protected]>
clinique
force-pushed
the
worxlandroid
branch
from
581797d to
ae98150
Compare
Signed-off-by: clinique <[email protected]>
This binding is used since a while as a standalone - let's get it merged.