[neohub] Quality improvements (#10522)

* [neohub] eliminate once in a blue moon fin-ack fin-ack issues

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* Revert "[neohub] eliminate once in a blue moon fin-ack fin-ack issues"

This reverts commit 022513ee85878f2f9cf2fd27fa3d79d251197c5a.

* [neohub] extra hub properties; hub id in logs (help for multiple hubs)

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] run spotless

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] getFirmwareVersion returns null for unknown

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] api version enum; tweaked logging

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] fix mvn warning

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] cosmetic for diff

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] device info property, and comments

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] tweaks to fin-ack sequence

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] eliminate irrelevant compiler warnings, and live test errors

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] avert merge conflict with #10525

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

* [neohub] apply changes in anticipation of reviewer approval

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
This commit is contained in:
Andrew Fiddian-Green 2021-04-19 19:39:12 +01:00 committed by GitHub
parent ada54dbcfa
commit 858ef65aef
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
7 changed files with 144 additions and 52 deletions

View File

@ -39,9 +39,9 @@ import org.slf4j.LoggerFactory;
/**
* The {@link NeoBaseHandler} is the openHAB Handler for NeoPlug devices
*
*
* @author Andrew Fiddian-Green - Initial contribution
*
*
*/
@NonNullByDefault
public class NeoBaseHandler extends BaseThingHandler {
@ -211,17 +211,17 @@ public class NeoBaseHandler extends BaseThingHandler {
break;
case ERR_COMMUNICATION:
logger.debug(MSG_HUB_COMM);
logger.debug(MSG_HUB_COMM, hub.getThing().getUID());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
break;
case ERR_INITIALIZATION:
logger.warn(MSG_HUB_CONFIG);
logger.warn(MSG_HUB_CONFIG, hub.getThing().getUID());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
break;
}
} else {
logger.debug(MSG_HUB_CONFIG);
logger.debug(MSG_HUB_CONFIG, "unknown");
}
} else {
logger.debug(MSG_FMT_COMMAND_BAD, command.toString());
@ -230,7 +230,7 @@ public class NeoBaseHandler extends BaseThingHandler {
/**
* internal getter returns the NeoHub handler
*
*
* @return the neohub handler or null
*/
protected @Nullable NeoHubHandler getNeoHub() {

View File

@ -23,7 +23,7 @@ import org.openhab.core.thing.ThingTypeUID;
* @author Sebastian Prehn - Initial contribution (NeoHub command codes)
* @author Andrew Fiddian-Green - Initial contribution (OpenHAB v2.x binding
* code)
*
*
*/
@NonNullByDefault
public class NeoHubBindingConstants {
@ -182,10 +182,17 @@ public class NeoHubBindingConstants {
* logger message strings
*/
public static final String PLEASE_REPORT_BUG = "Unexpected situation - please report a bug: ";
public static final String MSG_HUB_CONFIG = PLEASE_REPORT_BUG + "hub needs to be initialized!";
public static final String MSG_HUB_COMM = PLEASE_REPORT_BUG + "error communicating with the hub!";
public static final String MSG_FMT_DEVICE_POLL_ERR = "Device data polling error: {}";
public static final String MSG_FMT_SYSTEM_POLL_ERR = "System data polling error: {}";
public static final String MSG_FMT_ENGINEERS_POLL_ERR = "Engineers data polling error: {}";
public static final String MSG_FMT_SET_VALUE_ERR = "{} set value error: {}";
public static final String MSG_HUB_CONFIG = PLEASE_REPORT_BUG + "hub '{}' needs to be initialized!";
public static final String MSG_HUB_COMM = PLEASE_REPORT_BUG + "error communicating with hub '{}'!";
public static final String MSG_FMT_DEVICE_POLL_ERR = "hub '{}' device data polling error: {}";
public static final String MSG_FMT_SYSTEM_POLL_ERR = "hub '{}' system data polling error: {}";
public static final String MSG_FMT_ENGINEERS_POLL_ERR = "hub '{}' engineers data polling error: {}";
public static final String MSG_FMT_SET_VALUE_ERR = "hub '{}' {} set value error: {}";
/*
* hub property names
*/
public static final String PROPERTY_FIRMWARE_VERSION = "Firmware version";
public static final String PROPERTY_API_VERSION = "API version";
public static final String PROPERTY_API_DEVICEINFO = "Devices [online/total]";
}

View File

@ -71,7 +71,18 @@ public class NeoHubHandler extends BaseBridgeHandler {
private @Nullable NeoHubReadDcbResponse systemData = null;
private boolean isLegacyApiSelected = true;
private enum ApiVersion {
LEGACY("legacy"),
NEW("new");
public final String label;
private ApiVersion(String label) {
this.label = label;
}
}
private ApiVersion apiVersion = ApiVersion.LEGACY;
private boolean isApiOnline = false;
public NeoHubHandler(Bridge bridge) {
@ -88,7 +99,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
NeoHubConfiguration config = getConfigAs(NeoHubConfiguration.class);
if (logger.isDebugEnabled()) {
logger.debug("hostname={}", config.hostName);
logger.debug("hub '{}' hostname={}", getThing().getUID(), config.hostName);
}
if (!MATCHER_IP_ADDRESS.matcher(config.hostName).matches()) {
@ -97,7 +108,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
if (logger.isDebugEnabled()) {
logger.debug("port={}", config.portNumber);
logger.debug("hub '{}' port={}", getThing().getUID(), config.portNumber);
}
if (config.portNumber <= 0 || config.portNumber > 0xFFFF) {
@ -106,7 +117,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
if (logger.isDebugEnabled()) {
logger.debug("polling interval={}", config.pollingInterval);
logger.debug("hub '{}' polling interval={}", getThing().getUID(), config.pollingInterval);
}
if (config.pollingInterval < FAST_POLL_INTERVAL || config.pollingInterval > LAZY_POLL_INTERVAL) {
@ -116,7 +127,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
if (logger.isDebugEnabled()) {
logger.debug("socketTimeout={}", config.socketTimeout);
logger.debug("hub '{}' socketTimeout={}", getThing().getUID(), config.socketTimeout);
}
if (config.socketTimeout < 5 || config.socketTimeout > 20) {
@ -126,14 +137,14 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
if (logger.isDebugEnabled()) {
logger.debug("preferLegacyApi={}", config.preferLegacyApi);
logger.debug("hub '{}' preferLegacyApi={}", getThing().getUID(), config.preferLegacyApi);
}
socket = new NeoHubSocket(config.hostName, config.portNumber, config.socketTimeout);
this.config = config;
if (logger.isDebugEnabled()) {
logger.debug("start background polling..");
logger.debug("hub '{}' start background polling..", getThing().getUID());
}
// create a "lazy" polling scheduler
@ -160,7 +171,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
@Override
public void dispose() {
if (logger.isDebugEnabled()) {
logger.debug("stop background polling..");
logger.debug("hub '{}' stop background polling..", getThing().getUID());
}
// clean up the lazy polling scheduler
@ -205,7 +216,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
return NeoHubReturnResult.SUCCEEDED;
} catch (Exception e) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
logger.warn(MSG_FMT_SET_VALUE_ERR, commandStr, e.getMessage());
logger.warn(MSG_FMT_SET_VALUE_ERR, getThing().getUID(), commandStr, e.getMessage());
return NeoHubReturnResult.ERR_COMMUNICATION;
}
}
@ -219,7 +230,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
NeoHubSocket socket = this.socket;
if (socket == null || config == null) {
logger.warn(MSG_HUB_CONFIG);
logger.warn(MSG_HUB_CONFIG, getThing().getUID());
return null;
}
@ -227,7 +238,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
String responseJson;
NeoHubAbstractDeviceData deviceData;
if (isLegacyApiSelected) {
if (apiVersion == ApiVersion.LEGACY) {
responseJson = socket.sendMessage(CMD_CODE_INFO);
deviceData = NeoHubInfoResponse.createDeviceData(responseJson);
} else {
@ -236,7 +247,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
if (deviceData == null) {
logger.warn(MSG_FMT_DEVICE_POLL_ERR, "failed to create device data response");
logger.warn(MSG_FMT_DEVICE_POLL_ERR, getThing().getUID(), "failed to create device data response");
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
return null;
}
@ -244,7 +255,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
@Nullable
List<? extends AbstractRecord> devices = deviceData.getDevices();
if (devices == null || devices.isEmpty()) {
logger.warn(MSG_FMT_DEVICE_POLL_ERR, "no devices found");
logger.warn(MSG_FMT_DEVICE_POLL_ERR, getThing().getUID(), "no devices found");
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
return null;
}
@ -280,7 +291,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
return deviceData;
} catch (Exception e) {
logger.warn(MSG_FMT_DEVICE_POLL_ERR, e.getMessage());
logger.warn(MSG_FMT_DEVICE_POLL_ERR, getThing().getUID(), e.getMessage());
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR);
return null;
}
@ -302,7 +313,7 @@ public class NeoHubHandler extends BaseBridgeHandler {
String responseJson;
NeoHubReadDcbResponse systemData;
if (isLegacyApiSelected) {
if (apiVersion == ApiVersion.LEGACY) {
responseJson = socket.sendMessage(CMD_CODE_READ_DCB);
systemData = NeoHubReadDcbResponse.createSystemData(responseJson);
} else {
@ -311,13 +322,21 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
if (systemData == null) {
logger.warn(MSG_FMT_SYSTEM_POLL_ERR, "failed to create system data response");
logger.warn(MSG_FMT_SYSTEM_POLL_ERR, getThing().getUID(), "failed to create system data response");
return null;
}
String physicalFirmware = systemData.getFirmwareVersion();
if (physicalFirmware != null) {
String thingFirmware = getThing().getProperties().get(PROPERTY_FIRMWARE_VERSION);
if (!physicalFirmware.equals(thingFirmware)) {
getThing().setProperty(PROPERTY_FIRMWARE_VERSION, physicalFirmware);
}
}
return systemData;
} catch (Exception e) {
logger.warn(MSG_FMT_SYSTEM_POLL_ERR, e.getMessage());
logger.warn(MSG_FMT_SYSTEM_POLL_ERR, getThing().getUID(), e.getMessage());
return null;
}
}
@ -347,9 +366,11 @@ public class NeoHubHandler extends BaseBridgeHandler {
// evaluate and update the state of our RF mesh QoS channel
List<? extends AbstractRecord> devices = deviceData.getDevices();
State state;
String property;
if (devices == null || devices.isEmpty()) {
state = UnDefType.UNDEF;
property = "[?/?]";
} else {
int totalDeviceCount = devices.size();
int onlineDeviceCount = 0;
@ -360,17 +381,25 @@ public class NeoHubHandler extends BaseBridgeHandler {
@Nullable
Boolean onlineBefore = connectionStates.put(deviceName, online);
if (!online.equals(onlineBefore)) {
logger.info("device \"{}\" has {} the RF mesh network", deviceName,
online.booleanValue() ? "joined" : "left");
/*
* note: we use logger.info() here to log changes; reason is that the average user does really need
* to know if a device (very occasionally) drops out of the normally reliable RF mesh; however we
* only log it if 1) the state has changed, and 2) either 2a) the device has already been discovered
* by the bridge handler, or 2b) logger debug mode is set
*/
if (!online.equals(onlineBefore) && ((onlineBefore != null) || logger.isDebugEnabled())) {
logger.info("hub '{}' device \"{}\" has {} the RF mesh network", getThing().getUID(),
deviceName, online.booleanValue() ? "joined" : "left");
}
if (online.booleanValue()) {
onlineDeviceCount++;
}
}
property = String.format("[%d/%d]", onlineDeviceCount, totalDeviceCount);
state = new QuantityType<>((100.0 * onlineDeviceCount) / totalDeviceCount, Units.PERCENT);
}
getThing().setProperty(PROPERTY_API_DEVICEINFO, property);
updateState(CHAN_MESH_NETWORK_QOS, state);
}
if (fastPollingCallsToGo.get() > 0) {
@ -409,34 +438,40 @@ public class NeoHubHandler extends BaseBridgeHandler {
}
} catch (JsonSyntaxException | NeoHubException | IOException e) {
// we learned that this API is not currently supported; no big deal
logger.debug("Legacy API is not supported!");
logger.debug("hub '{}' legacy API is not supported!", getThing().getUID());
}
try {
responseJson = socket.sendMessage(CMD_CODE_GET_SYSTEM);
systemData = NeoHubReadDcbResponse.createSystemData(responseJson);
supportsFutureApi = systemData != null;
if (!supportsFutureApi) {
throw new NeoHubException("new API not supported");
throw new NeoHubException(String.format("hub '%s' new API not supported", getThing().getUID()));
}
} catch (JsonSyntaxException | NeoHubException | IOException e) {
// we learned that this API is not currently supported; no big deal
logger.debug("New API is not supported!");
logger.debug("hub '{}' new API is not supported!", getThing().getUID());
}
}
if (!supportsLegacyApi && !supportsFutureApi) {
logger.warn("Currently neither legacy nor new API are supported!");
logger.warn("hub '{}' currently neither legacy nor new API are supported!", getThing().getUID());
isApiOnline = false;
return;
}
NeoHubConfiguration config = this.config;
boolean isLegacyApiSelected = (supportsLegacyApi && config != null && config.preferLegacyApi);
if (isLegacyApiSelected != this.isLegacyApiSelected) {
logger.info("Changing API version: {}",
isLegacyApiSelected ? "\"new\" => \"legacy\"" : "\"legacy\" => \"new\"");
ApiVersion apiVersion = (supportsLegacyApi && config != null && config.preferLegacyApi) ? ApiVersion.LEGACY
: ApiVersion.NEW;
if (apiVersion != this.apiVersion) {
logger.debug("hub '{}' changing API version: '{}' => '{}'", getThing().getUID(), this.apiVersion.label,
apiVersion.label);
this.apiVersion = apiVersion;
}
this.isLegacyApiSelected = isLegacyApiSelected;
if (!apiVersion.label.equals(getThing().getProperties().get(PROPERTY_API_VERSION))) {
getThing().setProperty(PROPERTY_API_VERSION, apiVersion.label);
}
this.isApiOnline = true;
}
@ -451,14 +486,14 @@ public class NeoHubHandler extends BaseBridgeHandler {
responseJson = socket.sendMessage(CMD_CODE_GET_ENGINEERS);
return NeoHubGetEngineersData.createEngineersData(responseJson);
} catch (JsonSyntaxException | IOException | NeoHubException e) {
logger.warn(MSG_FMT_ENGINEERS_POLL_ERR, e.getMessage());
logger.warn(MSG_FMT_ENGINEERS_POLL_ERR, getThing().getUID(), e.getMessage());
}
}
return null;
}
public boolean isLegacyApiSelected() {
return isLegacyApiSelected;
return apiVersion == ApiVersion.LEGACY;
}
public Unit<?> getTemperatureUnit() {

View File

@ -116,7 +116,6 @@ public class NeoHubHandlerFactory extends BaseThingHandlerFactory {
/*
* destroy the discovery service
*/
@SuppressWarnings("null")
private synchronized void destroyDiscoveryService(NeoHubHandler handler) {
// fetch the respective thing's service registration from our list
ServiceRegistration<?> serviceReg = discoServices.remove(handler.getThing().getUID());

View File

@ -12,6 +12,7 @@
*/
package org.openhab.binding.neohub.internal;
import java.math.BigDecimal;
import java.time.Instant;
import javax.measure.Unit;
@ -39,6 +40,12 @@ public class NeoHubReadDcbResponse {
@SerializedName("CORF")
private @Nullable String degreesCorF;
@SerializedName("Firmware version")
private @Nullable BigDecimal firmwareVersionNew;
@SerializedName("HUB_VERSION")
private @Nullable BigDecimal firmwareVersionOld;
/*
* note: time-stamps are measured in seconds from 1970-01-01T00:00:00Z
*
@ -51,13 +58,23 @@ public class NeoHubReadDcbResponse {
return "F".equalsIgnoreCase(degreesCorF) ? ImperialUnits.FAHRENHEIT : SIUnits.CELSIUS;
}
public @Nullable String getFirmwareVersion() {
if (firmwareVersionNew != null) {
return firmwareVersionNew.toString();
}
if (firmwareVersionOld != null) {
return firmwareVersionOld.toString();
}
return null;
}
/**
* Create wrapper around a JSON string
*
*
* @param fromJson the JSON string
* @return a NeoHubReadDcbResponse wrapper around the JSON string
* @throws JsonSyntaxException
*
*
*/
public static @Nullable NeoHubReadDcbResponse createSystemData(String fromJson) throws JsonSyntaxException {
return GSON.fromJson(fromJson, NeoHubReadDcbResponse.class);

View File

@ -64,7 +64,7 @@ public class NeoHubSocket {
* @param requestJson the message to be sent to the NeoHub
* @return responseJson received from NeoHub
* @throws NeoHubException, IOException
*
*
*/
public String sendMessage(final String requestJson) throws IOException, NeoHubException {
IOException caughtException = null;
@ -84,15 +84,20 @@ public class NeoHubSocket {
writer.write(requestJson);
writer.write(0); // NULL terminate the command string
writer.flush();
socket.shutdownOutput();
if (logger.isTraceEnabled()) {
logger.trace("sent {} characters..", requestJson.length());
}
int inChar;
// NULL termination, end of stream (-1), or newline
while (((inChar = reader.read()) > 0) && (inChar != '\n')) {
builder.append((char) inChar);
boolean done = false;
// read until end of stream
while ((inChar = reader.read()) != -1) {
// a JSON block is terminated by a newline or NULL
if (!(done |= (inChar == '\n') || (inChar == 0))) {
builder.append((char) inChar);
}
}
}
} catch (IOException e) {

View File

@ -20,6 +20,7 @@ import java.io.FileReader;
import java.io.IOException;
import java.math.BigDecimal;
import java.time.Instant;
import java.util.regex.Pattern;
import org.eclipse.jdt.annotation.NonNullByDefault;
import org.junit.jupiter.api.Test;
@ -43,6 +44,16 @@ import org.openhab.core.library.unit.SIUnits;
@NonNullByDefault
public class NeoHubTestData {
/*
* to actually run tests on a physical device you must have a hub physically available, and its IP address must be
* correctly configured in the "hubIPAddress" string constant e.g. "192.168.1.123"
* note: only run the test if such a device is actually available
*/
private static final String hubIpAddress = "192.168.1.xxx";
private static final Pattern VALID_IP_V4_ADDRESS = Pattern
.compile("\\b((25[0-5]|2[0-4][0-9]|[01]?[0-9][0-9]?)(\\.|$)){4}\\b");
/*
* Load the test JSON payload string from a file
*/
@ -64,6 +75,7 @@ public class NeoHubTestData {
/*
* Test an INFO JSON response string as produced by older firmware versions
*/
@SuppressWarnings("null")
@Test
public void testInfoJsonOld() {
// load INFO JSON response string in old JSON format
@ -124,6 +136,7 @@ public class NeoHubTestData {
/*
* Test an INFO JSON response string as produced by newer firmware versions
*/
@SuppressWarnings("null")
@Test
public void testInfoJsonNew() {
// load INFO JSON response string in new JSON format
@ -148,12 +161,14 @@ public class NeoHubTestData {
/*
* Test for a READ_DCB JSON string that has valid CORF C response
*/
@SuppressWarnings("null")
@Test
public void testReadDcbJson() {
// load READ_DCB JSON response string with valid CORF C response
NeoHubReadDcbResponse dcbResponse = NeoHubReadDcbResponse.createSystemData(load("dcb_celsius"));
assertNotNull(dcbResponse);
assertEquals(SIUnits.CELSIUS, dcbResponse.getTemperatureUnit());
assertEquals("2134", dcbResponse.getFirmwareVersion());
// load READ_DCB JSON response string with valid CORF F response
dcbResponse = NeoHubReadDcbResponse.createSystemData(load("dcb_fahrenheit"));
@ -174,6 +189,7 @@ public class NeoHubTestData {
/*
* Test an INFO JSON string that has a door contact and a temperature sensor
*/
@SuppressWarnings("null")
@Test
public void testInfoJsonWithSensors() {
/*
@ -228,6 +244,7 @@ public class NeoHubTestData {
* From NeoHub rev2.6 onwards the READ_DCB command is "deprecated" so we can
* also test the replacement GET_SYSTEM command (valid CORF response)
*/
@SuppressWarnings("null")
@Test
public void testGetSystemJson() {
// load GET_SYSTEM JSON response string
@ -235,12 +252,14 @@ public class NeoHubTestData {
dcbResponse = NeoHubReadDcbResponse.createSystemData(load("system"));
assertNotNull(dcbResponse);
assertEquals(SIUnits.CELSIUS, dcbResponse.getTemperatureUnit());
assertEquals("2134", dcbResponse.getFirmwareVersion());
}
/*
* From NeoHub rev2.6 onwards the INFO command is "deprecated" so we must test
* the replacement GET_LIVE_DATA command
*/
@SuppressWarnings("null")
@Test
public void testGetLiveDataJson() {
// load GET_LIVE_DATA JSON response string
@ -329,6 +348,7 @@ public class NeoHubTestData {
* element is not returned in the GET_LIVE_DATA call so we must test the
* replacement GET_ENGINEERS command
*/
@SuppressWarnings("null")
@Test
public void testGetEngineersJson() {
// load GET_ENGINEERS JSON response string
@ -346,7 +366,7 @@ public class NeoHubTestData {
* send JSON request to the socket and retrieve JSON response
*/
private String testCommunicationInner(String requestJson) {
NeoHubSocket socket = new NeoHubSocket("192.168.1.109", 4242, 5);
NeoHubSocket socket = new NeoHubSocket(hubIpAddress, 4242, 5);
String responseJson = "";
try {
responseJson = socket.sendMessage(requestJson);
@ -359,8 +379,17 @@ public class NeoHubTestData {
/*
* Test the communications
*/
@SuppressWarnings("null")
@Test
public void testCommunications() {
/*
* tests the actual communication with a real physical device on 'hubIpAddress'
* note: only run the test if such a device is actually available
*/
if (!VALID_IP_V4_ADDRESS.matcher(hubIpAddress).matches()) {
return;
}
String responseJson = testCommunicationInner(CMD_CODE_INFO);
assertFalse(responseJson.isEmpty());