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

[mqtt] Enable discovery timeout reset #6704

Merged
merged 2 commits into from
Dec 31, 2019
Merged

[mqtt] Enable discovery timeout reset #6704

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cfde300
Enable discovery timeout reset.
jochen314 Dec 29, 2019
7c68015
remove empty lines
jochen314 Dec 30, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...ava/org/openhab/binding/mqtt/homeassistant/internal/discovery/HomeAssistantDiscovery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,8 @@ protected void unsetTypeProvider(MqttChannelTypeProvider provider) {
@Override
public void receivedMessage(ThingUID connectionBridge, MqttBrokerConnection connection, String topic,
byte[] payload) {
resetTimeout();

// For HomeAssistant we need to subscribe to a wildcard topic, because topics can either be:
// homeassistant/<component>/<node_id>/<object_id>/config OR
// homeassistant/<component>/<object_id>/config.
Expand Down
2 changes: 2 additions & 0 deletions ...ie/src/main/java/org/openhab/binding/mqtt/homie/internal/discovery/Homie300Discovery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,8 @@ public static boolean checkVersion(byte[] payload) {
@Override
public void receivedMessage(ThingUID connectionBridge, MqttBrokerConnection connection, String topic,
byte[] payload) {
resetTimeout();

if (!checkVersion(payload)) {
logger.trace("Found homie device. But version {} is out of range.",
new String(payload, StandardCharsets.UTF_8));
Expand Down
54 changes: 50 additions & 4 deletions ....binding.mqtt/src/main/java/org/openhab/binding/mqtt/discovery/AbstractMQTTDiscovery.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,17 @@
*/
package org.openhab.binding.mqtt.discovery;

import java.util.Date;
import java.util.Set;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.smarthome.config.discovery.AbstractDiscoveryService;
import org.eclipse.smarthome.core.thing.ThingTypeUID;

import java.util.Date;
import java.util.Set;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

/**
* Base MQTT discovery class. Responsible for connecting to the {@link MQTTTopicDiscoveryService}.
Expand All @@ -36,25 +40,60 @@
*/
@NonNullByDefault
public abstract class AbstractMQTTDiscovery extends AbstractDiscoveryService implements MQTTTopicDiscoveryParticipant {
private final Logger logger = LoggerFactory.getLogger(AbstractMQTTDiscovery.class);

protected final String subscribeTopic;

private int timeout;

private @Nullable ScheduledFuture<?> scheduledStop;

public AbstractMQTTDiscovery(@Nullable Set<ThingTypeUID> supportedThingTypes, int timeout,
boolean backgroundDiscoveryEnabledByDefault, String baseTopic) {
super(supportedThingTypes, timeout, backgroundDiscoveryEnabledByDefault);
super(supportedThingTypes, 0, backgroundDiscoveryEnabledByDefault);
this.subscribeTopic = baseTopic;
this.timeout = timeout;
}

/**
* Return the topic discovery service.
*/
protected abstract MQTTTopicDiscoveryService getDiscoveryService();

private synchronized void stopTimeout() {
if (scheduledStop != null) {
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
if (scheduledStop != null) {
final ScheduledFuture<?> scheduledStop = this.scheduledStop;
if (scheduledStop != null) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the point?
This is a synchronized method. If it would not be, then there is a lot more to take care of....
Actually, most of the code is taken from org.eclipse.smarthome.config.discovery.AbstractDiscoveryService. If the code is not good, then we should refactor that class and build the resetTimeout method into that.

scheduledStop.cancel(false);
scheduledStop = null;
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
scheduledStop = null;
this.scheduledStop = null;

}
}

protected synchronized void resetTimeout() {
stopTimeout();

// schedule an automatic call of stopScan when timeout is reached
if (timeout > 0) {
Runnable runnable = new Runnable() {
@Override
public void run() {
try {
stopScan();
} catch (Exception e) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it really necessary to catch Exception here? What exception do you expect`?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

see above: the code is basically taken from org.eclipse.smarthome.config.discovery.AbstractDiscoveryService. They thought it would be good tp do it, so I took their example.

Copy link
Member

Choose a reason for hiding this comment

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

Eclipse smarthome contains a lot of over engineered, pre Java-8 code that is only slowly refactored and converted into modern Java 11 code. I suggest not taking it as example code.

logger.debug("Exception occurred during execution: {}", e.getMessage(), e);
}
}
};

scheduledStop = scheduler.schedule(runnable, timeout, TimeUnit.SECONDS);
}
}

@Override
protected void startScan() {
if (isBackgroundDiscoveryEnabled()) {
super.stopScan();
return;
}
resetTimeout();
getDiscoveryService().subscribe(this, subscribeTopic);
}

Expand All @@ -64,10 +103,17 @@ protected synchronized void stopScan() {
super.stopScan();
return;
}
stopTimeout();
getDiscoveryService().unsubscribe(this);
super.stopScan();
}

@Override
public synchronized void abortScan() {
stopTimeout();
super.abortScan();
}

@Override
protected void startBackgroundDiscovery() {
// Remove results that are restored after a restart
Expand Down