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

[thekeys] Initial contribution #16037

Closed
wants to merge 15 commits into from

Conversation

JordanMartin
Copy link

@JordanMartin JordanMartin commented Dec 10, 2023

Binding for the smartlock TheKeys

  • [thekeys] Initial binding contribution

Description

This is the binding for TheKeys Smartlock.
This binding allows you to integrate, view, control and configure TheKeys Gateway and TheKeys Smartlock.

Full description of the binding capabilites in the README.md

Documentation

The logo is waiting this PR : openhab/openhab-docs#2171

Testing

The compiled jar bundle is available here : org.openhab.binding.thekeys-4.1.0-SNAPSHOT.jar

Binding for the smartlock TheKeys
Signed-off-by: Jordan Martin <jo69270@gmail.com>
@lolodomo lolodomo added the new binding If someone has started to work on a binding. For a new binding PR. label Dec 10, 2023
@lsiepel lsiepel changed the title [thekeys] Initial binding contribution [thekeys] Initial contribution Dec 15, 2023
@lolodomo
Copy link
Contributor

I will review your contribution.

Just reading the README, I have already a first comment: firmware version should be a thing property, not a channel.

@JordanMartin
Copy link
Author

I removed the version channel and fix some typo in the readme

…o fix some doc typo

Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Review part 1

@lolodomo
Copy link
Contributor

Notes for myself, items example to be reviewed.

Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
…perty

Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
@lolodomo
Copy link
Contributor

lolodomo commented Dec 18, 2023

You could have a look to compilation warnings. I see in particular several "Potential null pointer access".

I will review the remaining part (real java code) soon.

Copy link
Member

@holgerfriedrich holgerfriedrich left a comment

Choose a reason for hiding this comment

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

Thanks for you contribution!
This is not a full review, just a few comments on code style.

Javadoc runs w/o errors (warnings are ok). i18n is fine.

A lot of Null warnings.

Do you know how the gateway could be discovered (apparently there is a smartphone app which can do)? Just asking, as we try to suggest add-ons for installation if the devices are detected.

Copy link
Contributor

@clinique clinique left a comment

Choose a reason for hiding this comment

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

Some minor comments.

Signed-off-by: Jordan Martin <jo69270@gmail.com>
@JordanMartin
Copy link
Author

Thanks for you contribution! This is not a full review, just a few comments on code style.

Javadoc runs w/o errors (warnings are ok). i18n is fine.

A lot of Null warnings.

Do you know how the gateway could be discovered (apparently there is a smartphone app which can do)? Just asking, as we try to suggest add-ons for installation if the devices are detected.

As far as I know, it is not possible to discover the gateway on the network. It doesn't use mDNS nor uPNP.
It might be possible (not teseted) by using the remote api and the user account but I prefered to keep this binding only connected locally to the gateway.

@lolodomo
Copy link
Contributor

Please update your last commit to add "Signed-off-by". I hope this is the only commit without it and leading a DCO being KO.

@jlaur
Copy link
Contributor

jlaur commented Dec 28, 2023

Please update your last commit to add "Signed-off-by". I hope this is the only commit without it and leading a DCO being KO.

It is. If you click "Details" you can see that only one commit is missing the sign-off, and there are instructions how to fix it. @lolodomo, in such cases we can still merge, we just have to make sure to pick one of the correct sign-offs for the squash merge commit message.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Review of the api package.
Mainly exception handling to improve.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Last comment about files in resources folder

@lolodomo
Copy link
Contributor

lolodomo commented Dec 28, 2023

Remains to review: 3 Java classes (thing handlers and discovery class) + the test class + the example in README.
I will try to finish today.

@lolodomo
Copy link
Contributor

Mainly exception handling to improve.

The best option could be to catch different possible exceptions in your low level methods (get and post) and then rethrow only a TheKeysException.
Then you only have to consider TheKeysException in all calling methods.

bundles/org.openhab.binding.thekeys/README.md Outdated Show resolved Hide resolved
* @author Jordan Martin - Initial contribution
*/
@NonNullByDefault
@Component(service = TheKeyTranslationProvider.class)
Copy link
Member

Choose a reason for hiding this comment

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

With the latest changes in core you no longer need this.

Comment on lines +40 to +46
@NonNullByDefault
public class TheKeysDiscoveryService extends AbstractDiscoveryService implements ThingHandlerService {

private final Logger logger = LoggerFactory.getLogger(TheKeysDiscoveryService.class);

@Nullable
private TheKeysGatewayHandler gatewayHandler;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@NonNullByDefault
public class TheKeysDiscoveryService extends AbstractDiscoveryService implements ThingHandlerService {
private final Logger logger = LoggerFactory.getLogger(TheKeysDiscoveryService.class);
@Nullable
private TheKeysGatewayHandler gatewayHandler;
@Component(scope = ServiceScope.PROTOTYPE, service = TheKeysDiscoveryService.class)
@NonNullByDefault
public class TheKeysDiscoveryService extends AbstractThingHandlerDiscoveryService<TheKeysGatewayHandler> {
private final Logger logger = LoggerFactory.getLogger(TheKeysDiscoveryService.class);
private final Bundle bundle;

private TheKeysGatewayHandler gatewayHandler;

public TheKeysDiscoveryService() {
super(Set.of(THING_TYPE_SMARTLOCK), 15, false);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
super(Set.of(THING_TYPE_SMARTLOCK), 15, false);
super(TheKeysGatewayHandler.class, Set.of(THING_TYPE_SMARTLOCK), 15, false);
this.bundle = FrameworkUtil.getBundle(this.getClass());

Comment on lines +68 to +70
TheKeysGatewayHandler gatewayHandler = Objects.requireNonNull(this.gatewayHandler);
ThingUID gatewayThingUID = gatewayHandler.getThing().getUID();
String label = gatewayHandler.getTranslationProvider().getText("discovery.thekeys.smartlock.label");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TheKeysGatewayHandler gatewayHandler = Objects.requireNonNull(this.gatewayHandler);
ThingUID gatewayThingUID = gatewayHandler.getThing().getUID();
String label = gatewayHandler.getTranslationProvider().getText("discovery.thekeys.smartlock.label");
ThingUID gatewayThingUID = thingHandler.getThing().getUID();
String label = thingHandler.getTranslationProvider().getText("discovery.thekeys.smartlock.label");

The thingHandler is now an implicit non-null field. You don't need null-checks.

Comment on lines +84 to +85
TheKeysGatewayHandler gatewayHandler = Objects.requireNonNull(this.gatewayHandler);
ThingUID gatewayThingUID = gatewayHandler.getThing().getUID();
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
TheKeysGatewayHandler gatewayHandler = Objects.requireNonNull(this.gatewayHandler);
ThingUID gatewayThingUID = gatewayHandler.getThing().getUID();
ThingUID gatewayThingUID = thingHandler.getThing().getUID();

Comment on lines +95 to +111

@Override
public @Nullable ThingHandler getThingHandler() {
return gatewayHandler;
}

@Override
public void setThingHandler(@Nullable ThingHandler handler) {
if (handler instanceof TheKeysGatewayHandler theKeysGatewayHandler) {
gatewayHandler = theKeysGatewayHandler;
}
}

@Override
public void deactivate() {
super.deactivate();
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Override
public @Nullable ThingHandler getThingHandler() {
return gatewayHandler;
}
@Override
public void setThingHandler(@Nullable ThingHandler handler) {
if (handler instanceof TheKeysGatewayHandler theKeysGatewayHandler) {
gatewayHandler = theKeysGatewayHandler;
}
}
@Override
public void deactivate() {
super.deactivate();
}

Not needed anymore, this is already provided by the super class.

public TheKeysDiscoveryService() {
super(Set.of(THING_TYPE_SMARTLOCK), 15, false);
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
@Reference(unbind = "-")
public void bindTranslationProvider(TranslationProvider i18NProvider) {
this.i18NProvider = i18NProvider;
}
@Reference(unbind = "-")
public void bindLocaleProvider(LocaleProvider localeProvider) {
this.localeProvider = localeProvider;
}
public String getText(String key, @Nullable Object... arguments) {
try {
Locale locale = localeProvider.getLocale();
return i18nProvider.getText(bundle, key, getDefaultText(key), locale, arguments);
} catch (IllegalArgumentException e) {
return "Unable to load message for key " + key;
}
}
public @Nullable String getDefaultText(String key) {
return i18nProvider.getText(bundle, key, key, Locale.ENGLISH);
}

@lolodomo
Copy link
Contributor

lolodomo commented Mar 3, 2024

@JordanMartin : I thought I was very late but now I see that you have pushed nothing since we published our comments.

Please let the reviewers mark comments as resolved. This is difficult for us to follow what was really considered if you mark them as resolved even without providing any change.

I will finish the review if you are still working on this binding and you push new code.

Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
Signed-off-by: Jordan Martin <jo69270@gmail.com>
@JordanMartin
Copy link
Author

@lolodomo I've done almost all the requested changes but I haven't had time to test it yet.
I will ensure that comments are appropriately addressed in future revisions,

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Restart of the review

Comment on lines +66 to +67
| batteryLevel | Number:Power | R | Current battery level |
| lowBattery | Switch | R | Low battery warning |
Copy link
Contributor

Choose a reason for hiding this comment

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

Real channel names are "battery-level" and "low-battery".

Type for battery-level is Number, not Number:Power.

Comment on lines +70 to +71
| syncInProgress | Switch | R | Indicates the pending update of the lock state |
| lastSync | DateTime | R | Date of the last success sync with the lock |
Copy link
Contributor

Choose a reason for hiding this comment

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

Real channel names are "sync-in-progress" and "last-sync"

| status | String | R | Status of the smartlock |
| batteryLevel | Number:Power | R | Current battery level |
| lowBattery | Switch | R | Low battery warning |
| rssi | Number | R | Bluetooth signal strength with the gateway |
Copy link
Contributor

Choose a reason for hiding this comment

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

Type is Number:Power

Comment on lines +94 to +95
Number Smartlock_Battery_Level "Battery Level" <Battery> (Smartlock) ["Measurement", "Energy"] { channel="thekeys:smartlock:tk-gateway:1234:batteryLevel" }
Switch Smartlock_Low_Battery "Low Battery" <LowBattery> (Smartlock) ["Energy", "LowBattery"] { channel="thekeys:smartlock:tk-gateway:1234:lowBattery" }
Copy link
Contributor

Choose a reason for hiding this comment

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

battery-level and low-battery for the channel ids

Comment on lines +98 to +99
Switch Smartlock_Synchronization_In_Progress "Synchronization In Progress" (Smartlock) ["Point"] { channel="thekeys:smartlock:tk-gateway:1234:syncInProgress" }
DateTime Smartlock_Last_Sync "Last Sync" (Smartlock) ["Point"] { channel="thekeys:smartlock:tk-gateway:1234:lastSync" }
Copy link
Contributor

Choose a reason for hiding this comment

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

sync-in-progress and last-sync for the channel ids

Comment on lines +30 to +31
channel-type.thekeys.lock_status.label = Lock Status
channel-type.thekeys.lock_status.description = Status of the smartlock
Copy link
Contributor

Choose a reason for hiding this comment

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

Please regenerate the i18n files, they contain old names for channels.
mvn i18n:generate-default-translations

Comment on lines +1 to +2
<?xml version="1.0" encoding="UTF-8"?>
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xmlns="http://maven.apache.org/POM/4.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Look at in an existing POM file, these first 2 lines were slightly updated.

<?xml version="1.0" encoding="UTF-8" standalone="no"?>
<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
  xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 https://maven.apache.org/xsd/maven-4.0.0.xsd">

Comment on lines +35 to +36
public static final String CHANNEL_BATTERY_LEVEL = "batteryLevel";
public static final String CHANNEL_LOW_BATTERY = "lowBattery";
Copy link
Contributor

Choose a reason for hiding this comment

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

Real channel names are "battery-level" and "low-battery".

Comment on lines +40 to +41
public static final String CHANNEL_LAST_SYNC = "lastSync";
public static final String CHANNEL_SYNC_IN_PROGRESS = "syncInProgress";
Copy link
Contributor

Choose a reason for hiding this comment

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

Real channel names are "sync-in-progress" and "last-sync"

* @param url The target url
* @param timeoutMs The request timeout in ms
* @param responseType The type of the response to be parsed
* @return The parsed response or null if empty response
Copy link
Contributor

Choose a reason for hiding this comment

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

null cannot happen

* @param body The body to be posted
* @param timeout The request timeout in ms
* @param responseType The type of the response to be parsed
* @return The parsed response or null if empty response
Copy link
Contributor

Choose a reason for hiding this comment

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

null cannot happen

* @return The parsed response or null if empty response
* @throws TheKeysException If the request failed
*/
public <T> T get(String url, int timeoutMs, Class<@NonNull T> responseType) throws TheKeysException {
Copy link
Contributor

Choose a reason for hiding this comment

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

@NonNull is useless as @NonNullByDefault is applied to your full class.

* @return The parsed response or null if empty response
* @throws TheKeysException If the request failed
*/
public <T> T post(String url, String body, int timeout, Class<T> responseType) throws TheKeysException {
Copy link
Contributor

@lolodomo lolodomo May 26, 2024

Choose a reason for hiding this comment

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

Ultra minor: the parameter is timeout in that method while timeoutMs in the previous one. Suggestion (and not request for change): the naming could be consistent.

@@ -0,0 +1,101 @@
# The Keys Binding

This is the binding for [TheKeys Smartlock](https://www.the-keys.eu/en/produits/8-serrure-connectee.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like https://www.the-keys.eu/ does not respond...

@lolodomo
Copy link
Contributor

lolodomo commented May 26, 2024

@JordanMartin : I restarted the review and added few new comments (that are easy to fix). I noticed that you already considered all my previous review comments, thank you for that. I will try to finish the full review during the coming week. If you are available to handle the last comments, we can probably conclude quickly.

@lsiepel
Copy link
Contributor

lsiepel commented Jun 28, 2024

Gentle ping @JordanMartin are you able to proceed?

@lolodomo
Copy link
Contributor

I could finish the review but to not loose my free time, first I would like to have a comment from @JordanMartin to be sure he is still interested to finish this binding.

PS: I could even push changes that I recommend to speed up the process but in that case I could no more be the unique reviewer.

@lsiepel
Copy link
Contributor

lsiepel commented Jun 29, 2024

I could finish the review but to not loose my free time, first I would like to have a comment from @JordanMartin to be sure he is still interested to finish this binding.

PS: I could even push changes that I recommend to speed up the process but in that case I could no more be the unique reviewer.

I’ll be available to review. But a comment from @JordanMartin would be best as this binding hardly has a future if the contributor stops development before the initial PR is finished.

@JordanMartin
Copy link
Author

My smart lock is broken.
I will not continue this binding because I cannot test anymore, and the brand has discontinued this product (without replacement).
Sorry for the delay and the useless work.

@lsiepel
Copy link
Contributor

lsiepel commented Jul 9, 2024

That is very unfortunate. Also very good that you shared this, it saves spending any more time on this matter. I’ll close this PR for now as there seems no base to continue.

@lsiepel lsiepel closed this Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback 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.

7 participants