Skip to content

Commit

Permalink
[neohub] Quality improvements (openhab#10522)
Browse files Browse the repository at this point in the history 
* [neohub] eliminate once in a blue moon fin-ack fin-ack issues

Signed-off-by: Andrew Fiddian-Green <[email protected]>

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

This reverts commit 022513e.

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

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] run spotless

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] getFirmwareVersion returns null for unknown

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] api version enum; tweaked logging

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] fix mvn warning

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] cosmetic for diff

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] device info property, and comments

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] tweaks to fin-ack sequence

Signed-off-by: Andrew Fiddian-Green <[email protected]>

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

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] avert merge conflict with openhab#10525

Signed-off-by: Andrew Fiddian-Green <[email protected]>

* [neohub] apply changes in anticipation of reviewer approval

Signed-off-by: Andrew Fiddian-Green <[email protected]>
  • Loading branch information
@andrewfg
andrewfg authored Apr 19, 2021
1 parent a584be6 commit 7cbc79a
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 52 deletions.
12 changes: 6 additions & 6 deletions ...nhab.binding.neohub/src/main/java/org/openhab/binding/neohub/internal/NeoBaseHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,9 @@

/**
* The {@link NeoBaseHandler} is the openHAB Handler for NeoPlug devices
*
*
* @author Andrew Fiddian-Green - Initial contribution
*
*
*/
@NonNullByDefault
public class NeoBaseHandler extends BaseThingHandler {
Expand Down Expand Up @@ -211,17 +211,17 @@ protected void toNeoHubSendCommand(String channelId, Command command) {
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());
Expand All @@ -230,7 +230,7 @@ protected void toNeoHubSendCommand(String channelId, Command command) {

/**
* internal getter returns the NeoHub handler
*
*
* @return the neohub handler or null
*/
protected @Nullable NeoHubHandler getNeoHub() {
Expand Down
21 changes: 14 additions & 7 deletions ...ding.neohub/src/main/java/org/openhab/binding/neohub/internal/NeoHubBindingConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
* @author Sebastian Prehn - Initial contribution (NeoHub command codes)
* @author Andrew Fiddian-Green - Initial contribution (OpenHAB v2.x binding
* code)
*
*
*/
@NonNullByDefault
public class NeoHubBindingConstants {
Expand Down Expand Up @@ -182,10 +182,17 @@ public static enum NeoHubReturnResult {
* 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]";
}
97 changes: 66 additions & 31 deletions ...enhab.binding.neohub/src/main/java/org/openhab/binding/neohub/internal/NeoHubHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -88,7 +99,7 @@ public void initialize() {
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()) {
Expand All @@ -97,7 +108,7 @@ public void initialize() {
}

if (logger.isDebugEnabled()) {
logger.debug("port={}", config.portNumber);
logger.debug("hub '{}' port={}", getThing().getUID(), config.portNumber);
}

if (config.portNumber <= 0 || config.portNumber > 0xFFFF) {
Expand All @@ -106,7 +117,7 @@ public void initialize() {
}

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) {
Expand All @@ -116,7 +127,7 @@ public void initialize() {
}

if (logger.isDebugEnabled()) {
logger.debug("socketTimeout={}", config.socketTimeout);
logger.debug("hub '{}' socketTimeout={}", getThing().getUID(), config.socketTimeout);
}

if (config.socketTimeout < 5 || config.socketTimeout > 20) {
Expand All @@ -126,14 +137,14 @@ public void initialize() {
}

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
Expand All @@ -160,7 +171,7 @@ public void initialize() {
@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
Expand Down Expand Up @@ -205,7 +216,7 @@ public synchronized NeoHubReturnResult toNeoHubSendChannelValue(String commandSt
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;
}
}
Expand All @@ -219,15 +230,15 @@ public synchronized NeoHubReturnResult toNeoHubSendChannelValue(String commandSt
NeoHubSocket socket = this.socket;

if (socket == null || config == null) {
logger.warn(MSG_HUB_CONFIG);
logger.warn(MSG_HUB_CONFIG, getThing().getUID());
return null;
}

try {
String responseJson;
NeoHubAbstractDeviceData deviceData;

if (isLegacyApiSelected) {
if (apiVersion == ApiVersion.LEGACY) {
responseJson = socket.sendMessage(CMD_CODE_INFO);
deviceData = NeoHubInfoResponse.createDeviceData(responseJson);
} else {
Expand All @@ -236,15 +247,15 @@ public synchronized NeoHubReturnResult toNeoHubSendChannelValue(String commandSt
}

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;
}

@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;
}
Expand Down Expand Up @@ -280,7 +291,7 @@ public synchronized NeoHubReturnResult toNeoHubSendChannelValue(String commandSt

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;
}
Expand All @@ -302,7 +313,7 @@ public synchronized NeoHubReturnResult toNeoHubSendChannelValue(String commandSt
String responseJson;
NeoHubReadDcbResponse systemData;

if (isLegacyApiSelected) {
if (apiVersion == ApiVersion.LEGACY) {
responseJson = socket.sendMessage(CMD_CODE_READ_DCB);
systemData = NeoHubReadDcbResponse.createSystemData(responseJson);
} else {
Expand All @@ -311,13 +322,21 @@ public synchronized NeoHubReturnResult toNeoHubSendChannelValue(String commandSt
}

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;
}
}
Expand Down Expand Up @@ -347,9 +366,11 @@ private synchronized void lazyPollingSchedulerExecute() {
// 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;
Expand All @@ -360,17 +381,25 @@ private synchronized void lazyPollingSchedulerExecute() {

@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) {
Expand Down Expand Up @@ -409,34 +438,40 @@ private void selectApi() {
}
} 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;
}

Expand All @@ -451,14 +486,14 @@ private void selectApi() {
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() {
Expand Down
1 change: 0 additions & 1 deletion ...inding.neohub/src/main/java/org/openhab/binding/neohub/internal/NeoHubHandlerFactory.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,6 @@ private synchronized void createDiscoveryService(NeoHubHandler handler) {
/*
* 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());
Expand Down
Loading

0 comments on commit 7cbc79a

Please sign in to comment.