[tellstick] Avoid updates duplication after communication errors (#13479)

* [tellstick] Avoid updates duplication after communication errors

Fix #13453

Do not register the same device handler many times as listener in the bridge handler
Unregister the device handler from the bridge handler when disposing device handler

HTTP timeout set to 15s
Remove the retry mechanism related to the timeout
Check HTTP status code
Fix discovery service unregistration
Log statistics about request/refresh durations and number of timeouts/errors
Change logging in case of exception
Also change few logs level (remove usage of logger.error)
Execute one refresh at bridge initialization and not 2
Small enhancement of the bridge/things status management
implement discovery service unregistration
Fix few code analysis findings

Signed-off-by: Laurent Garnier <lg.hc@free.fr>

* Use a set for deviceStatusListeners to avoid duplications

Review comment: @NonNullByDefault for TellstickHandlerFactory

Review comment: use ThingStatusDetail.CONFIGURATION_ERROR if no bridge
is defined

Review comment: use 1_000_000 instead of 1000000

Review comment: use Instant instead of LocalDateTime

Review comment: Thread.currentThread().interrupt()

Signed-off-by: Laurent Garnier <lg.hc@free.fr>
This commit is contained in:
lolodomo 2022-11-05 16:11:06 +01:00 committed by GitHub
parent 51d3fc211a
commit 969fef8612
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 185 additions and 72 deletions

View File

@ -16,6 +16,8 @@ import static org.openhab.binding.tellstick.internal.TellstickBindingConstants.*
import java.util.Hashtable;
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.eclipse.jetty.client.HttpClient;
import org.openhab.binding.tellstick.internal.core.TelldusCoreBridgeHandler;
import org.openhab.binding.tellstick.internal.discovery.TellstickDiscoveryService;
@ -31,6 +33,7 @@ import org.openhab.core.thing.ThingTypeUID;
import org.openhab.core.thing.binding.BaseThingHandlerFactory;
import org.openhab.core.thing.binding.ThingHandler;
import org.openhab.core.thing.binding.ThingHandlerFactory;
import org.osgi.framework.ServiceRegistration;
import org.osgi.service.component.annotations.Activate;
import org.osgi.service.component.annotations.Component;
import org.osgi.service.component.annotations.Reference;
@ -43,12 +46,15 @@ import org.slf4j.LoggerFactory;
*
* @author Jarle Hjortland - Initial contribution
* @author Jan Gustafsson - Adding support for local API
* @author Laurent Garnier - fix discovery service registering/unregistering
*/
@Component(service = ThingHandlerFactory.class, configurationPid = "binding.tellstick")
@NonNullByDefault
public class TellstickHandlerFactory extends BaseThingHandlerFactory {
private final Logger logger = LoggerFactory.getLogger(TellstickHandlerFactory.class);
private TellstickDiscoveryService discoveryService = null;
private final HttpClient httpClient;
private @Nullable TellstickDiscoveryService discoveryService = null;
private @Nullable ServiceRegistration<DiscoveryService> discoveryServiceRegistration = null;
@Activate
public TellstickHandlerFactory(@Reference HttpClientFactory httpClientFactory) {
@ -60,18 +66,38 @@ public class TellstickHandlerFactory extends BaseThingHandlerFactory {
return SUPPORTED_THING_TYPES_UIDS.contains(thingTypeUID);
}
private void registerDeviceDiscoveryService(TelldusBridgeHandler tellstickBridgeHandler) {
if (discoveryService == null) {
discoveryService = new TellstickDiscoveryService(tellstickBridgeHandler);
discoveryService.activate();
bundleContext.registerService(DiscoveryService.class.getName(), discoveryService, new Hashtable<>());
private synchronized void registerDeviceDiscoveryService(TelldusBridgeHandler tellstickBridgeHandler) {
TellstickDiscoveryService service = discoveryService;
if (service == null) {
service = new TellstickDiscoveryService(tellstickBridgeHandler);
service.activate();
discoveryService = service;
discoveryServiceRegistration = (ServiceRegistration<DiscoveryService>) bundleContext
.registerService(DiscoveryService.class.getName(), service, new Hashtable<>());
} else {
discoveryService.addBridgeHandler(tellstickBridgeHandler);
service.addBridgeHandler(tellstickBridgeHandler);
}
}
private synchronized void unregisterDeviceDiscoveryService(TelldusBridgeHandler tellstickBridgeHandler) {
TellstickDiscoveryService service = discoveryService;
if (service != null) {
if (service.isOnlyForOneBridgeHandler()) {
service.deactivate();
discoveryService = null;
ServiceRegistration<DiscoveryService> serviceReg = discoveryServiceRegistration;
if (serviceReg != null) {
serviceReg.unregister();
discoveryServiceRegistration = null;
}
} else {
service.removeBridgeHandler(tellstickBridgeHandler);
}
}
}
@Override
protected ThingHandler createHandler(Thing thing) {
protected @Nullable ThingHandler createHandler(Thing thing) {
if (thing.getThingTypeUID().equals(TELLDUSCOREBRIDGE_THING_TYPE)) {
TelldusCoreBridgeHandler handler = new TelldusCoreBridgeHandler((Bridge) thing);
registerDeviceDiscoveryService(handler);
@ -91,4 +117,11 @@ public class TellstickHandlerFactory extends BaseThingHandlerFactory {
return null;
}
}
@Override
protected void removeHandler(ThingHandler thingHandler) {
if (thingHandler instanceof TelldusBridgeHandler) {
unregisterDeviceDiscoveryService((TelldusBridgeHandler) thingHandler);
}
}
}

View File

@ -16,8 +16,9 @@ import java.util.Calendar;
import java.util.Collections;
import java.util.Date;
import java.util.List;
import java.util.Set;
import java.util.Vector;
import java.util.concurrent.CopyOnWriteArrayList;
import java.util.concurrent.ConcurrentHashMap;
import org.openhab.binding.tellstick.internal.conf.TellstickBridgeConfiguration;
import org.openhab.binding.tellstick.internal.handler.DeviceStatusListener;
@ -66,7 +67,7 @@ public class TelldusCoreBridgeHandler extends BaseBridgeHandler
private List<TellstickSensor> sensorList = new Vector<>();
private TellstickEventHandler eventHandler;
private static boolean initialized = false;
private List<DeviceStatusListener> deviceStatusListeners = new CopyOnWriteArrayList<>();
private Set<DeviceStatusListener> deviceStatusListeners = ConcurrentHashMap.newKeySet();
@Override
public void handleCommand(ChannelUID channelUID, Command command) {
@ -160,7 +161,6 @@ public class TelldusCoreBridgeHandler extends BaseBridgeHandler
}
}
} catch (SupportedMethodsException e) {
logger.error("Failed to get devices ", e);
}

View File

@ -205,4 +205,13 @@ public class TellstickDiscoveryService extends AbstractDiscoveryService implemen
telldusBridgeHandlers.add(tellstickBridgeHandler);
tellstickBridgeHandler.registerDeviceStatusListener(this);
}
public void removeBridgeHandler(TelldusBridgeHandler tellstickBridgeHandler) {
telldusBridgeHandlers.remove(tellstickBridgeHandler);
tellstickBridgeHandler.unregisterDeviceStatusListener(this);
}
public boolean isOnlyForOneBridgeHandler() {
return telldusBridgeHandlers.size() == 1;
}
}

View File

@ -151,24 +151,26 @@ public class TelldusDevicesHandler extends BaseThingHandler implements DeviceSta
public void initialize() {
Configuration config = getConfig();
logger.debug("Initialize TelldusDeviceHandler {}. class {}", config, config.getClass());
final Object configDeviceId = config.get(TellstickBindingConstants.DEVICE_ID);
if (configDeviceId != null) {
deviceId = configDeviceId.toString();
} else {
logger.debug("Initialized TellStick device missing serialNumber configuration... troubles ahead");
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR);
}
final Boolean isADimmer = (Boolean) config.get(TellstickBindingConstants.DEVICE_ISDIMMER);
if (isADimmer != null) {
this.isDimmer = isADimmer;
isDimmer = isADimmer;
}
final BigDecimal repeatCount = (BigDecimal) config.get(TellstickBindingConstants.DEVICE_RESEND_COUNT);
if (repeatCount != null) {
resend = repeatCount.intValue();
}
Bridge bridge = getBridge();
if (bridge != null) {
bridgeStatusChanged(bridge.getStatusInfo());
final Object configDeviceId = config.get(TellstickBindingConstants.DEVICE_ID);
if (configDeviceId != null) {
deviceId = configDeviceId.toString();
Bridge bridge = getBridge();
if (bridge != null) {
bridgeStatusChanged(bridge.getStatusInfo());
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, "No bridge defined");
}
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"Missing serialNumber configuration");
}
}
@ -180,7 +182,7 @@ public class TelldusDevicesHandler extends BaseThingHandler implements DeviceSta
Bridge localBridge = getBridge();
if (localBridge != null) {
TelldusBridgeHandler telldusBridgeHandler = (TelldusBridgeHandler) localBridge.getHandler();
logger.debug("Init bridge for {}, bridge:{}", deviceId, telldusBridgeHandler);
logger.debug("Init device {}, bridge:{}", deviceId, telldusBridgeHandler);
if (telldusBridgeHandler != null) {
this.bridgeHandler = telldusBridgeHandler;
this.bridgeHandler.registerDeviceStatusListener(this);
@ -208,14 +210,23 @@ public class TelldusDevicesHandler extends BaseThingHandler implements DeviceSta
}
}
} catch (Exception e) {
logger.error("Failed to init bridge for {}", deviceId, e);
logger.warn("Failed to init device {}", deviceId, e);
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.HANDLER_INITIALIZING_ERROR);
}
} else {
updateStatus(ThingStatus.OFFLINE, bridgeStatusInfo.getStatusDetail());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}
}
@Override
public void dispose() {
TelldusBridgeHandler bridgeHandler = getTellstickBridgeHandler();
if (bridgeHandler != null) {
bridgeHandler.unregisterDeviceStatusListener(this);
}
super.dispose();
}
private Device getDevice(TelldusBridgeHandler tellHandler, String deviceId) {
Device dev = null;
if (deviceId != null) {

View File

@ -12,8 +12,11 @@
*/
package org.openhab.binding.tellstick.internal.live;
import java.time.Duration;
import java.time.Instant;
import java.util.List;
import java.util.Vector;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
@ -51,14 +54,19 @@ import org.tellstick.device.iface.Device;
*/
public class TelldusLiveBridgeHandler extends BaseBridgeHandler implements TelldusBridgeHandler {
private static final int REFRESH_DELAY = 10;
private final Logger logger = LoggerFactory.getLogger(TelldusLiveBridgeHandler.class);
private TellstickNetDevices deviceList = null;
private TellstickNetSensors sensorList = null;
private TelldusLiveDeviceController controller = new TelldusLiveDeviceController();
private List<DeviceStatusListener> deviceStatusListeners = new Vector<>();
private Set<DeviceStatusListener> deviceStatusListeners = ConcurrentHashMap.newKeySet();
private static final int REFRESH_DELAY = 10;
private int nbRefresh;
private long sumRefreshDuration;
private long minRefreshDuration = 1_000_000;
private long maxRefreshDuration;
public TelldusLiveBridgeHandler(Bridge bridge) {
super(bridge);
@ -88,9 +96,8 @@ public class TelldusLiveBridgeHandler extends BaseBridgeHandler implements Telld
this.controller = new TelldusLiveDeviceController();
this.controller.connectHttpClient(configuration.publicKey, configuration.privateKey, configuration.token,
configuration.tokenSecret);
updateStatus(ThingStatus.UNKNOWN);
startAutomaticRefresh(configuration.refreshInterval);
refreshDeviceList();
updateStatus(ThingStatus.ONLINE);
}
private synchronized void startAutomaticRefresh(long refreshInterval) {
@ -112,17 +119,36 @@ public class TelldusLiveBridgeHandler extends BaseBridgeHandler implements Telld
}
synchronized void refreshDeviceList() {
Instant start = Instant.now();
try {
updateDevices(deviceList);
updateSensors(sensorList);
updateStatus(ThingStatus.ONLINE);
} catch (TellstickException e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
logger.error("Failed to update", e);
} catch (Exception e) {
logger.warn("Failed to update", e);
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, e.getMessage());
logger.error("Failed to update", e);
}
monitorAdditionalRefresh(start, Instant.now());
}
private void monitorAdditionalRefresh(Instant start, Instant end) {
if (!logger.isDebugEnabled()) {
return;
}
long duration = Duration.between(start, end).toMillis();
sumRefreshDuration += duration;
nbRefresh++;
if (duration < minRefreshDuration) {
minRefreshDuration = duration;
}
if (duration > maxRefreshDuration) {
maxRefreshDuration = duration;
}
logger.debug(
"{} refresh avg = {} ms min = {} max = {} (request avg = {} ms min = {} max = {}) ({} timeouts {} errors)",
nbRefresh, nbRefresh == 0 ? 0 : sumRefreshDuration / nbRefresh, minRefreshDuration, maxRefreshDuration,
controller.getAverageRequestDuration(), controller.getMinRequestDuration(),
controller.getMaxRequestDuration(), controller.getNbTimeouts(), controller.getNbErrors());
}
private synchronized void updateDevices(TellstickNetDevices previouslist) throws TellstickException {

View File

@ -14,6 +14,8 @@ package org.openhab.binding.tellstick.internal.live;
import java.math.BigDecimal;
import java.math.RoundingMode;
import java.time.Duration;
import java.time.Instant;
import java.util.concurrent.ExecutionException;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
@ -35,6 +37,7 @@ import org.asynchttpclient.Response;
import org.asynchttpclient.oauth.ConsumerKey;
import org.asynchttpclient.oauth.OAuthSignatureCalculator;
import org.asynchttpclient.oauth.RequestToken;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.tellstick.internal.TelldusBindingException;
import org.openhab.binding.tellstick.internal.handler.TelldusDeviceController;
import org.openhab.binding.tellstick.internal.live.xml.TelldusLiveResponse;
@ -66,7 +69,7 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
private final Logger logger = LoggerFactory.getLogger(TelldusLiveDeviceController.class);
private long lastSend = 0;
public static final long DEFAULT_INTERVAL_BETWEEN_SEND = 250;
static final int REQUEST_TIMEOUT_MS = 5000;
private static final int REQUEST_TIMEOUT_MS = 15000;
private AsyncHttpClient client;
static final String HTTP_API_TELLDUS_COM_XML = "http://api.telldus.com/xml/";
static final String HTTP_TELLDUS_CLIENTS = HTTP_API_TELLDUS_COM_XML + "clients/list";
@ -77,7 +80,13 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
static final String HTTP_TELLDUS_DEVICE_DIM = HTTP_API_TELLDUS_COM_XML + "device/dim?id=%d&level=%d";
static final String HTTP_TELLDUS_DEVICE_TURNOFF = HTTP_API_TELLDUS_COM_XML + "device/turnOff?id=%d";
static final String HTTP_TELLDUS_DEVICE_TURNON = HTTP_API_TELLDUS_COM_XML + "device/turnOn?id=%d";
private static final int MAX_RETRIES = 3;
private int nbRequest;
private long sumRequestDuration;
private long minRequestDuration = 1_000_000;
private long maxRequestDuration;
private int nbTimeouts;
private int nbErrors;
public TelldusLiveDeviceController() {
}
@ -87,7 +96,7 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
try {
client.close();
} catch (Exception e) {
logger.error("Failed to close client", e);
logger.debug("Failed to close client", e);
}
}
@ -101,7 +110,7 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
Response response = client.prepareGet(HTTP_TELLDUS_CLIENTS).execute().get();
logger.debug("Response {} statusText {}", response.getResponseBody(), response.getStatusText());
} catch (InterruptedException | ExecutionException e) {
logger.error("Failed to connect", e);
logger.warn("Failed to connect", e);
}
}
@ -114,7 +123,7 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
@Override
public void handleSendEvent(Device device, int resendCount, boolean isdimmer, Command command)
throws TellstickException {
logger.info("Send {} to {}", command, device);
logger.debug("Send {} to {}", command, device);
if (device instanceof TellstickNetDevice) {
if (command == OnOffType.ON) {
turnOn(device);
@ -276,37 +285,71 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
}
<T> T callRestMethod(String uri, Class<T> response) throws TelldusLiveException {
Instant start = Instant.now();
T resultObj = null;
try {
for (int i = 0; i < MAX_RETRIES; i++) {
try {
resultObj = innerCallRest(uri, response);
break;
} catch (TimeoutException e) {
logger.warn("TimeoutException error in get", e);
} catch (InterruptedException e) {
logger.warn("InterruptedException error in get", e);
}
}
} catch (JAXBException e) {
logger.warn("Encoding error in get", e);
resultObj = innerCallRest(uri, response);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
logResponse(uri, e);
monitorAdditionalRequest(start, Instant.now(), e);
throw new TelldusLiveException(e);
} catch (XMLStreamException e) {
logger.warn("Communication error in get", e);
} catch (TimeoutException | ExecutionException | JAXBException | XMLStreamException e) {
logResponse(uri, e);
throw new TelldusLiveException(e);
} catch (ExecutionException e) {
logger.warn("ExecutionException error in get", e);
monitorAdditionalRequest(start, Instant.now(), e);
throw new TelldusLiveException(e);
}
monitorAdditionalRequest(start, Instant.now(), null);
return resultObj;
}
private void monitorAdditionalRequest(Instant start, Instant end, @Nullable Throwable exception) {
if (!logger.isDebugEnabled()) {
return;
}
long duration = Duration.between(start, end).toMillis();
sumRequestDuration += duration;
nbRequest++;
if (duration < minRequestDuration) {
minRequestDuration = duration;
}
if (duration > maxRequestDuration) {
maxRequestDuration = duration;
}
if (exception instanceof TimeoutException) {
nbTimeouts++;
} else if (exception != null) {
nbErrors++;
}
}
public long getAverageRequestDuration() {
return nbRequest == 0 ? 0 : sumRequestDuration / nbRequest;
}
public long getMinRequestDuration() {
return minRequestDuration;
}
public long getMaxRequestDuration() {
return maxRequestDuration;
}
public int getNbTimeouts() {
return nbTimeouts;
}
public int getNbErrors() {
return nbErrors;
}
private <T> T innerCallRest(String uri, Class<T> response) throws InterruptedException, ExecutionException,
TimeoutException, JAXBException, FactoryConfigurationError, XMLStreamException {
Future<Response> future = client.prepareGet(uri).execute();
Response resp = future.get(REQUEST_TIMEOUT_MS, TimeUnit.MILLISECONDS);
if (resp.getStatusCode() != 200) {
throw new ExecutionException("HTTP request returned status code " + resp.getStatusCode(), null);
}
// TelldusLiveHandler.logger.info("Devices" + resp.getResponseBody());
JAXBContext jc = JAXBContext.newInstance(response);
XMLInputFactory xif = XMLInputFactory.newInstance();
@ -324,8 +367,6 @@ public class TelldusLiveDeviceController implements DeviceChangeListener, Sensor
}
private void logResponse(String uri, Exception e) {
if (e != null) {
logger.warn("Request [{}] Failure:{}", uri, e.getMessage());
}
logger.warn("Request [{}] failed: {} {}", uri, e.getClass().getSimpleName(), e.getMessage());
}
}

View File

@ -151,10 +151,7 @@ public class TellstickNetDevice implements Device {
return false;
}
TellstickNetDevice other = (TellstickNetDevice) obj;
if (deviceId != other.deviceId) {
return false;
}
return true;
return deviceId == other.deviceId;
}
public int getMethods() {

View File

@ -143,10 +143,7 @@ public class TellstickNetSensor implements Device {
return false;
}
TellstickNetSensor other = (TellstickNetSensor) obj;
if (deviceId != other.deviceId) {
return false;
}
return true;
return deviceId == other.deviceId;
}
public void setUpdated(boolean b) {

View File

@ -13,9 +13,9 @@
package org.openhab.binding.tellstick.internal.local;
import java.time.Duration;
import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ScheduledFuture;
import java.util.concurrent.TimeUnit;
@ -50,7 +50,7 @@ import org.tellstick.device.iface.Device;
* to the framework. All {@link TelldusDevicesHandler}s use the
* {@link TelldusLocalDeviceController} to execute the actual commands.
*
* @author Jan Gustafsson- Initial contribution
* @author Jan Gustafsson - Initial contribution
*/
public class TelldusLocalBridgeHandler extends BaseBridgeHandler implements TelldusBridgeHandler {
@ -59,7 +59,7 @@ public class TelldusLocalBridgeHandler extends BaseBridgeHandler implements Tell
private TellstickLocalDevicesDTO deviceList = null;
private TellstickLocalSensorsDTO sensorList = null;
private TelldusLocalDeviceController controller = null;
private List<DeviceStatusListener> deviceStatusListeners = Collections.synchronizedList(new ArrayList<>());
private Set<DeviceStatusListener> deviceStatusListeners = ConcurrentHashMap.newKeySet();
private final HttpClient httpClient;
private ScheduledFuture<?> pollingJob;
/**

View File

@ -56,7 +56,6 @@ public class TelldusLocalDeviceController implements DeviceChangeListener, Senso
private final Logger logger = LoggerFactory.getLogger(TelldusLocalDeviceController.class);
private long lastSend = 0;
public static final long DEFAULT_INTERVAL_BETWEEN_SEND_SEC = 250;
static final int REQUEST_TIMEOUT_MS = 5000;
private final HttpClient httpClient;
private final Gson gson = new Gson();
private String localApiUrl;