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

[nikohomecontrol] Fix things staying offline #12819

Merged
merged 5 commits into from
Jun 4, 2022
Merged

[nikohomecontrol] Fix things staying offline #12819

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
626ba4d
Do not set thing status before end of initialization
mherwege May 26, 2022
247302b
Change ThingStatusDetail when no communication object
mherwege May 27, 2022
496fb26
Change bridge config error messages
mherwege May 28, 2022
a53fe3d
Bridge status changed handling
mherwege Dec 14, 2021
b183a1e
Check bridge status in handler init
mherwege Jun 4, 2022
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
114 changes: 73 additions & 41 deletions ...va/org/openhab/binding/nikohomecontrol/internal/handler/NikoHomeControlActionHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import org.openhab.core.thing.Thing;
import org.openhab.core.thing.ThingStatus;
import org.openhab.core.thing.ThingStatusDetail;
import org.openhab.core.thing.ThingStatusInfo;
import org.openhab.core.thing.binding.BaseThingHandler;
import org.openhab.core.types.Command;
import org.slf4j.Logger;
Expand All @@ -54,6 +55,8 @@ public class NikoHomeControlActionHandler extends BaseThingHandler implements Nh

private volatile @Nullable NhcAction nhcAction;

private volatile boolean initialized = false;

private String actionId = "";
private int stepValue;
private boolean invert;
Expand All @@ -64,10 +67,9 @@ public NikoHomeControlActionHandler(Thing thing) {

@Override
public void handleCommand(ChannelUID channelUID, Command command) {
NikoHomeControlCommunication nhcComm = getCommunication();
NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());
if (nhcComm == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_UNINITIALIZED,
"@text/offline.bridge-unitialized");
logger.debug("communication not up yet, cannot handle command {} for {}", command, channelUID);
return;
}

Expand Down Expand Up @@ -200,6 +202,8 @@ private void handleRollershutterCommand(Command command) {

@Override
public void initialize() {
initialized = false;

NikoHomeControlActionConfig config;
if (thing.getThingTypeUID().equals(THING_TYPE_DIMMABLE_LIGHT)) {
config = getConfig().as(NikoHomeControlActionDimmerConfig.class);
Expand All @@ -212,52 +216,66 @@ public void initialize() {
}
actionId = config.actionId;

NikoHomeControlCommunication nhcComm = getCommunication();
NikoHomeControlBridgeHandler bridgeHandler = getBridgeHandler();
if (bridgeHandler == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.invalid-bridge-handler");
return;
}

updateStatus(ThingStatus.UNKNOWN);

Bridge bridge = getBridge();
if ((bridge != null) && ThingStatus.ONLINE.equals(bridge.getStatus())) {
// We need to do this in a separate thread because we may have to wait for the
// communication to become active
scheduler.submit(this::startCommunication);
}
}

private synchronized void startCommunication() {
NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());

if (nhcComm == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_UNINITIALIZED,
"@text/offline.bridge-unitialized");
return;
} else {
updateStatus(ThingStatus.UNKNOWN);
Comment on lines -220 to -221
Copy link
Contributor

@lolodomo lolodomo Jun 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

The recommended practice is to have initialize() setting a valid status (ONLINE / OFFLINE / UNKNOWN) before returning and so before running additional code in a separate thread.
Look at the template: https://github.com/openhab/openhab-core/blob/main/tools/archetype/binding/src/main/resources/archetype-resources/src/main/java/internal/__bindingIdCamelCase__Handler.java#L80

Same remark for everywhere where you removed the set of status to UNKNOWN.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Initialize does set the status, and the removed lines make sure it is only set when all of the initialization logic has executed. However, the bridge status can change if communication is lost. I do agree that the UNKNOWN status could not happen, so the last else would not be needed, but I added it for completeness. Should I remove it and replace by some debug log?

}

// We need to do this in a separate thread because we may have to wait for the communication to become active
scheduler.submit(() -> {
if (!nhcComm.communicationActive()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"@text/offline.communication-error");
return;
}
if (!nhcComm.communicationActive()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"@text/offline.communication-error");
return;
}

NhcAction nhcAction = nhcComm.getActions().get(actionId);
if (nhcAction == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.actionId");
return;
}
NhcAction nhcAction = nhcComm.getActions().get(actionId);
if (nhcAction == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.actionId");
return;
}

nhcAction.setEventHandler(this);
nhcAction.setEventHandler(this);

updateProperties(nhcAction);
updateProperties(nhcAction);

String actionLocation = nhcAction.getLocation();
if (thing.getLocation() == null) {
thing.setLocation(actionLocation);
}
String actionLocation = nhcAction.getLocation();
if (thing.getLocation() == null) {
thing.setLocation(actionLocation);
}

actionEvent(nhcAction.getState());
this.nhcAction = nhcAction;

this.nhcAction = nhcAction;
logger.debug("action initialized {}", actionId);

logger.debug("action initialized {}", actionId);
Bridge bridge = getBridge();
if ((bridge != null) && (bridge.getStatus() == ThingStatus.ONLINE)) {
updateStatus(ThingStatus.ONLINE);
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}

Bridge bridge = getBridge();
if ((bridge != null) && (bridge.getStatus() == ThingStatus.ONLINE)) {
updateStatus(ThingStatus.ONLINE);
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}
});
actionEvent(nhcAction.getState());

initialized = true;
}

private void updateProperties(NhcAction nhcAction) {
Expand Down Expand Up @@ -341,18 +359,32 @@ private void restartCommunication(NikoHomeControlCommunication nhcComm) {
if (nhcBridgeHandler != null) {
nhcBridgeHandler.bridgeOnline();
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_UNINITIALIZED,
"@text/offline.bridge-unitialized");
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.invalid-bridge-handler");
}
}

private @Nullable NikoHomeControlCommunication getCommunication() {
NikoHomeControlBridgeHandler nhcBridgeHandler = getBridgeHandler();
private @Nullable NikoHomeControlCommunication getCommunication(
@Nullable NikoHomeControlBridgeHandler nhcBridgeHandler) {
return nhcBridgeHandler != null ? nhcBridgeHandler.getCommunication() : null;
}

private @Nullable NikoHomeControlBridgeHandler getBridgeHandler() {
Bridge nhcBridge = getBridge();
return nhcBridge != null ? (NikoHomeControlBridgeHandler) nhcBridge.getHandler() : null;
}

@Override
public void bridgeStatusChanged(ThingStatusInfo bridgeStatusInfo) {
ThingStatus bridgeStatus = bridgeStatusInfo.getStatus();
if (ThingStatus.ONLINE.equals(bridgeStatus)) {
if (!initialized) {
scheduler.submit(this::startCommunication);
} else {
updateStatus(ThingStatus.ONLINE);
}
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}
}
}
124 changes: 78 additions & 46 deletions ...g/openhab/binding/nikohomecontrol/internal/handler/NikoHomeControlEnergyMeterHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import org.openhab.core.thing.Thing;
import org.openhab.core.thing.ThingStatus;
import org.openhab.core.thing.ThingStatusDetail;
import org.openhab.core.thing.ThingStatusInfo;
import org.openhab.core.thing.binding.BaseThingHandler;
import org.openhab.core.types.Command;
import org.openhab.core.types.UnDefType;
Expand All @@ -50,6 +51,8 @@ public class NikoHomeControlEnergyMeterHandler extends BaseThingHandler implemen

private volatile @Nullable NhcEnergyMeter nhcEnergyMeter;

private volatile boolean initialized = false;

private String energyMeterId = "";

public NikoHomeControlEnergyMeterHandler(Thing thing) {
Expand All @@ -71,66 +74,81 @@ public void handleCommand(ChannelUID channelUID, Command command) {

@Override
public void initialize() {
initialized = false;

NikoHomeControlEnergyMeterConfig config = getConfig().as(NikoHomeControlEnergyMeterConfig.class);

energyMeterId = config.energyMeterId;

NikoHomeControlCommunication nhcComm = getCommunication();
NikoHomeControlBridgeHandler bridgeHandler = getBridgeHandler();
if (bridgeHandler == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.invalid-bridge-handler");
return;
}

updateStatus(ThingStatus.UNKNOWN);

Bridge bridge = getBridge();
if ((bridge != null) && ThingStatus.ONLINE.equals(bridge.getStatus())) {
// We need to do this in a separate thread because we may have to wait for the
// communication to become active
scheduler.submit(this::startCommunication);
}
}

private synchronized void startCommunication() {
NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());

if (nhcComm == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_UNINITIALIZED,
"@text/offline.bridge-unitialized");
return;
} else {
updateStatus(ThingStatus.UNKNOWN);
}

// We need to do this in a separate thread because we may have to wait for the
// communication to become active
scheduler.submit(() -> {
if (!nhcComm.communicationActive()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"@text/offline.communication-error");
return;
}
if (!nhcComm.communicationActive()) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR,
"@text/offline.communication-error");
return;
}

NhcEnergyMeter nhcEnergyMeter = nhcComm.getEnergyMeters().get(energyMeterId);
if (nhcEnergyMeter == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.energyMeterId");
return;
}
NhcEnergyMeter nhcEnergyMeter = nhcComm.getEnergyMeters().get(energyMeterId);
if (nhcEnergyMeter == null) {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.energyMeterId");
return;
}

nhcEnergyMeter.setEventHandler(this);
nhcEnergyMeter.setEventHandler(this);

updateProperties(nhcEnergyMeter);
updateProperties(nhcEnergyMeter);

String location = nhcEnergyMeter.getLocation();
if (thing.getLocation() == null) {
thing.setLocation(location);
}
String location = nhcEnergyMeter.getLocation();
if (thing.getLocation() == null) {
thing.setLocation(location);
}

// Subscribing to power readings starts an intensive data flow, therefore only do it when there is an item
// linked to the channel
if (isLinked(CHANNEL_POWER)) {
nhcComm.startEnergyMeter(energyMeterId);
}
this.nhcEnergyMeter = nhcEnergyMeter;

this.nhcEnergyMeter = nhcEnergyMeter;
logger.debug("energy meter intialized {}", energyMeterId);

logger.debug("energy meter intialized {}", energyMeterId);
Bridge bridge = getBridge();
if ((bridge != null) && (bridge.getStatus() == ThingStatus.ONLINE)) {
updateStatus(ThingStatus.ONLINE);
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}

Bridge bridge = getBridge();
if ((bridge != null) && (bridge.getStatus() == ThingStatus.ONLINE)) {
updateStatus(ThingStatus.ONLINE);
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}
});
// Subscribing to power readings starts an intensive data flow, therefore only do it when there is an item
// linked to the channel
if (isLinked(CHANNEL_POWER)) {
nhcComm.startEnergyMeter(energyMeterId);
}

initialized = true;
}

@Override
public void dispose() {
NikoHomeControlCommunication nhcComm = getCommunication();
NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());

if (nhcComm != null) {
nhcComm.stopEnergyMeter(energyMeterId);
Expand Down Expand Up @@ -178,7 +196,7 @@ public void energyMeterRemoved() {
// Subscribing to power readings starts an intensive data flow, therefore only do it when there is an item linked to
// the channel
public void channelLinked(ChannelUID channelUID) {
NikoHomeControlCommunication nhcComm = getCommunication();
NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());
if (nhcComm != null) {
// This can be expensive, therefore do it in a job.
scheduler.submit(() -> {
Expand All @@ -196,7 +214,7 @@ public void channelLinked(ChannelUID channelUID) {

@Override
public void channelUnlinked(ChannelUID channelUID) {
NikoHomeControlCommunication nhcComm = getCommunication();
NikoHomeControlCommunication nhcComm = getCommunication(getBridgeHandler());
if (nhcComm != null) {
// This can be expensive, therefore do it in a job.
scheduler.submit(() -> {
Expand Down Expand Up @@ -230,18 +248,32 @@ private void restartCommunication(NikoHomeControlCommunication nhcComm) {
if (nhcBridgeHandler != null) {
nhcBridgeHandler.bridgeOnline();
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_UNINITIALIZED,
"@text/offline.bridge-unitialized");
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR,
"@text/offline.configuration-error.invalid-bridge-handler");
}
}

private @Nullable NikoHomeControlCommunication getCommunication() {
NikoHomeControlBridgeHandler nhcBridgeHandler = getBridgeHandler();
private @Nullable NikoHomeControlCommunication getCommunication(
@Nullable NikoHomeControlBridgeHandler nhcBridgeHandler) {
return nhcBridgeHandler != null ? nhcBridgeHandler.getCommunication() : null;
}

private @Nullable NikoHomeControlBridgeHandler getBridgeHandler() {
Bridge nhcBridge = getBridge();
return nhcBridge != null ? (NikoHomeControlBridgeHandler) nhcBridge.getHandler() : null;
}

@Override
public void bridgeStatusChanged(ThingStatusInfo bridgeStatusInfo) {
ThingStatus bridgeStatus = bridgeStatusInfo.getStatus();
if (ThingStatus.ONLINE.equals(bridgeStatus)) {
if (!initialized) {
scheduler.submit(this::startCommunication);
} else {
updateStatus(ThingStatus.ONLINE);
}
} else {
updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE);
}
}
}
Loading