-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[lgthinq] Initial contribution #12149
base: main
Are you sure you want to change the base?
Conversation
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/contribution-lg-thinq-air-conditioner-addon/132444/3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your contribution.
Please check for a constant naming scheme.
Some first comments made with required changes.
...inding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqHandlerFactory.java
Outdated
Show resolved
Hide resolved
...inding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqHandlerFactory.java
Outdated
Show resolved
Hide resolved
...inding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqHandlerFactory.java
Outdated
Show resolved
Hide resolved
....openhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/api/Gateway.java
Outdated
Show resolved
Hide resolved
.../src/main/java/org/openhab/binding/lgthinq/internal/LGDeviceDynStateDescriptionProvider.java
Outdated
Show resolved
Hide resolved
...inding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/LGApiV1ClientServiceImpl.java
Outdated
Show resolved
Hide resolved
...inding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/LGApiV2ClientServiceImpl.java
Outdated
Show resolved
Hide resolved
....openhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/model/LGDevice.java
Outdated
Show resolved
Hide resolved
bundles/org.openhab.binding.lgthinq/src/main/resources/OH-INF/i18n/lgthinq.properties
Show resolved
Hide resolved
...openhab.binding.lgthinq/src/test/java/org/openhab/binding/lgthinq/handler/LGBridgeTests.java
Outdated
Show resolved
Hide resolved
...nq/src/main/java/org/openhab/binding/lgthinq/internal/discovery/LGThinqDiscoveryService.java
Outdated
Show resolved
Hide resolved
...inding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqHandlerFactory.java
Outdated
Show resolved
Hide resolved
...lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqAirConditionerHandler.java
Outdated
Show resolved
Hide resolved
...ding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqBindingConstants.java
Outdated
Show resolved
Hide resolved
...nq/src/main/java/org/openhab/binding/lgthinq/internal/discovery/LGThinqDiscoveryService.java
Outdated
Show resolved
Hide resolved
...binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/LGThinqApiClientService.java
Outdated
Show resolved
Hide resolved
...penhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/model/ACSnapShot.java
Outdated
Show resolved
Hide resolved
...nhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/model/ACSnapShotV1.java
Outdated
Show resolved
Hide resolved
....openhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgapi/model/LGDevice.java
Outdated
Show resolved
Hide resolved
...org.openhab.binding.lgthinq/src/test/java/org/openhab/binding/lgthinq/handler/JsonUtils.java
Show resolved
Hide resolved
...ab.binding.lgthinq/src/test/java/org/openhab/binding/lgthinq/handler/LGThinqBridgeTests.java
Show resolved
Hide resolved
...ding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/LGThinqBindingConstants.java
Outdated
Show resolved
Hide resolved
@nemerdaud Gave your binding a first shot and it just worked. Amazing! 😄 |
Changing the op_mode does not seem to work, it results in an error after a few seconds.
|
@tobof , please provide me $OPENHAB_USERDATA/thinq/thinq-569c7573-8e85-12aa-86e5-44cb8b809993-cap.json file.
|
aea5f82
to
ef8745b
Compare
356b6b6
to
5a1eb61
Compare
Co-authored-by: Jacob Laursen <[email protected]> Signed-off-by: Nemer Daud <[email protected]>
Co-authored-by: lsiepel <[email protected]> Signed-off-by: Nemer Daud <[email protected]>
Co-authored-by: lsiepel <[email protected]> Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: nemerdaud <[email protected]>
…ature/lgthinq-binding
Signed-off-by: nemerdaud <[email protected]>
Signed-off-by: nemerdaud <[email protected]>
This pull request has been mentioned on openHAB Community. There might be relevant details there: https://community.openhab.org/t/snapshot-binding-will-not-load/157873/10 |
Signed-off-by: nemerdaud <[email protected]>
Signed-off-by: nemerdaud <[email protected]>
Signed-off-by: nemerdaud <[email protected]>
@lsiepel , I think I resolved all the itens. Fell free to continue the review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This pass i had a better look to some dto / model / and other code files. Some comments can be applied binding wide, please check.
Also want to mention that there are 100+ compile warnings. Most of them should be fixed, the same for the SAT warnings.
bundles/org.openhab.binding.lgthinq/src/main/feature/feature.xml
Outdated
Show resolved
Hide resolved
...b.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/api/LGThinqGateway.java
Outdated
Show resolved
Hide resolved
....lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/api/OauthLgEmpAuthenticator.java
Outdated
Show resolved
Hide resolved
....lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/api/OauthLgEmpAuthenticator.java
Outdated
Show resolved
Hide resolved
...penhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/api/RestUtils.java
Outdated
Show resolved
Hide resolved
...ing.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/type/ThingModelTypeUtils.java
Outdated
Show resolved
Hide resolved
...org.openhab.binding.lgthinq/src/test/java/org/openhab/binding/lgthinq/handler/JsonUtils.java
Outdated
Show resolved
Hide resolved
...penhab.binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/internal/api/RestUtils.java
Outdated
Show resolved
Hide resolved
...rc/test/java/org/openhab/binding/lgthinq/internal/handler/LGThinQWasherDryerHandlerTest.java
Outdated
Show resolved
Hide resolved
...nq/src/main/java/org/openhab/binding/lgthinq/lgservices/LGThinQWMApiV2ClientServiceImpl.java
Outdated
Show resolved
Hide resolved
@nemerdaud sorry for my commit e473608 directly to your branch. Intended to make it in a private branch but clearly that did not happen. |
4a20df9
to
305ebfa
Compare
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Sorry for the delay, @lsiepel . It was a little bit of work to fix the warning (compiler & sat) and logs. The last review is done (I hope :-) ) . |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for maintaining this PR and working towards a merge. In general the binding is in a good shape. The review is mainly about smaller issues like naming, labels and smaller inconsistency's but the overall structure seems good.
When looking at the comments, please keep in mind that the sugggestions might also apply to other places. Especially when looking at null suppresions and thing/channel structure.
Looked at 148/154 files, only the handlers are left.
The ThinQ Bridge is necessary to work as a hub/bridge to discovery and first configure the LG ThinQ devices related with the LG's user account. | ||
Then, the first thing is to create the LG ThinQ Bridge and then, it will discover all Things you have related in your LG Account. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These two lines should be moved to the discovery paragraph (and streamlined)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the second paragraph is unnecessary since it's a basic concept of the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make sure we talk about the same, to what lines do you refer with the second paragraph?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There were some redundant and unnecessary information. I reorganized this 2 topics like this:
`
Bridge Thing
This Binding has a Bridge responsible for discovering and registering LG Things.
Thus, adding the Bridge (LGThinq GW Bridge) is the first step in configuring this Binding.
Discovery
This Binding has auto-discovery for the supported LG Thinq devices. Once LG Thinq Bridge has been added, LG Thinq devices linked to your account will be automatically discovered and displayed in the OpenHab Inbox.
`
Let me know if it's OK for you.
...a/org/openhab/binding/lgthinq/lgservices/model/devices/fridge/FridgeCapabilityFactoryV1.java
Outdated
Show resolved
Hide resolved
*/ | ||
@NonNullByDefault | ||
public interface LGThinQACApiClientService extends LGThinQApiClientService<ACCapability, ACCanonicalSnapshot> { | ||
void changeOperationMode(String bridgeName, String deviceId, int newOpMode) throws LGThinqApiException; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier comment, please provide javadoc to all public interfaces methods
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I missed it ! It will take some time...
...hinq/src/main/java/org/openhab/binding/lgthinq/lgservices/api/LGThinqCanonicalModelUtil.java
Outdated
Show resolved
Hide resolved
...binding.lgthinq/src/main/java/org/openhab/binding/lgthinq/lgservices/api/LGThinqGateway.java
Outdated
Show resolved
Hide resolved
...q/src/main/java/org/openhab/binding/lgthinq/lgservices/api/LGThinqOauthEmpAuthenticator.java
Outdated
Show resolved
Hide resolved
Regarding the review, I would propose that you comment or thumbs up a comment if it has been fixed. I can then verify and mark it resolved. This would make it easy for me to keep track. Hope it also works for you. |
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
Signed-off-by: Nemer Daud <[email protected]>
OK ! I think it's better. When I fix an item, I will push it and thumbs up in a sequence. |
Binding to integrate OpenHab to LG Thinq API to control Thinq compatible devices through OpenHab.
See README.MD documentation for supported devices.
Signed-off-by: Nemer Daud [email protected]