Skip to content

Commit

Permalink
[danfossairunit] Fix network reliability issues and setting of all ch…
Browse files Browse the repository at this point in the history
…annel values to zero (openhab#11172)

* Fix Potential null pointer accesses
* Added constants for TCP port and poll interval in seconds.
* Extract interface from DanfossAirUnitCommunicationController.
* Remove unused constants which seems to be left-overs from skeleton.
* Added constant for discovery timeout value for readability.
* Created handler subfolder for DanfossAirUnitHandler (like discovery) for consistency with other bindings.
* Handle lost connection gracefully without updating all channels to zero.
* Fix infinitly blocking network calls preventing proper error handling.
* Fix thing status being reset to ONLINE after failing to update all channels.
* Fix error handling when receiving invalid manual fan step.

Fixes openhab#11167
Fixes openhab#11188

Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
  • Loading branch information
jlaur authored and thinkingstone committed Nov 7, 2021
1 parent 20a38e8 commit 50ac6a9
Show file tree
Hide file tree
Showing 8 changed files with 299 additions and 97 deletions.
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Copyright (c) 2010-2021 Contributors to the openHAB project
*
* See the NOTICE file(s) distributed with this work for additional
* information.
*
* This program and the accompanying materials are made available under the
* terms of the Eclipse Public License 2.0 which is available at
* http://www.eclipse.org/legal/epl-2.0
*
* SPDX-License-Identifier: EPL-2.0
*/
package org.openhab.binding.danfossairunit.internal;

import java.io.IOException;

import org.eclipse.jdt.annotation.NonNullByDefault;

/**
* This interface defines a communication controller that can be used to send requests to the Danfoss Air Unit.
*
* @author Jacob Laursen - Refactoring, bugfixes and enhancements
*/
@NonNullByDefault
public interface CommunicationController {
void connect() throws IOException;

void disconnect();

byte[] sendRobustRequest(byte[] operation, byte[] register) throws IOException;

byte[] sendRobustRequest(byte[] operation, byte[] register, byte[] value) throws IOException;
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
import java.io.IOException;
import java.math.BigDecimal;
import java.math.RoundingMode;
import java.net.InetAddress;
import java.nio.charset.StandardCharsets;
import java.time.DateTimeException;
import java.time.ZoneId;
Expand All @@ -43,30 +42,22 @@
*
* @author Ralf Duckstein - Initial contribution
* @author Robert Bach - heavy refactorings
* @author Jacob Laursen - Refactoring, bugfixes and enhancements
*/

@SuppressWarnings("SameParameterValue")
@NonNullByDefault
public class DanfossAirUnit {

private final DanfossAirUnitCommunicationController communicationController;
private final CommunicationController communicationController;

public DanfossAirUnit(InetAddress inetAddr, int port) {
this.communicationController = new DanfossAirUnitCommunicationController(inetAddr, port);
}

public void cleanUp() {
this.communicationController.disconnect();
public DanfossAirUnit(CommunicationController communicationController) {
this.communicationController = communicationController;
}

private boolean getBoolean(byte[] operation, byte[] register) throws IOException {
return communicationController.sendRobustRequest(operation, register)[0] != 0;
}

private void setSetting(byte[] register, boolean value) throws IOException {
setSetting(register, value ? (byte) 1 : (byte) 0);
}

private short getWord(byte[] operation, byte[] register) throws IOException {
byte[] resultBytes = communicationController.sendRobustRequest(operation, register);
return (short) ((resultBytes[0] << 8) | (resultBytes[1] & 0xFF));
Expand All @@ -87,14 +78,6 @@ private void set(byte[] operation, byte[] register, byte value) throws IOExcepti
communicationController.sendRobustRequest(operation, register, valueArray);
}

private void set(byte[] operation, byte[] register, short value) throws IOException {
communicationController.sendRobustRequest(operation, register, shortToBytes(value));
}

private byte[] shortToBytes(short s) {
return new byte[] { (byte) ((s & 0xFF00) >> 8), (byte) (s & 0x00FF) };
}

private short getShort(byte[] operation, byte[] register) throws IOException {
byte[] result = communicationController.sendRobustRequest(operation, register);
return (short) ((result[0] << 8) + (result[1] & 0xff));
Expand Down Expand Up @@ -141,14 +124,6 @@ private static float asPercentByte(byte b) {
return f * 100 / 255;
}

private void setSetting(byte[] register, short value) throws IOException {
byte[] valueArray = new byte[2];
valueArray[0] = (byte) (value >> 8);
valueArray[1] = (byte) value;

communicationController.sendRobustRequest(REGISTER_1_WRITE, register, valueArray);
}

public String getUnitName() throws IOException {
return getString(REGISTER_1_READ, UNIT_NAME);
}
Expand All @@ -161,8 +136,12 @@ public StringType getMode() throws IOException {
return new StringType(Mode.values()[getByte(REGISTER_1_READ, MODE)].name());
}

public PercentType getManualFanStep() throws IOException {
return new PercentType(BigDecimal.valueOf(getByte(REGISTER_1_READ, MANUAL_FAN_SPEED_STEP) * 10));
public PercentType getManualFanStep() throws IOException, UnexpectedResponseValueException {
byte value = getByte(REGISTER_1_READ, MANUAL_FAN_SPEED_STEP);
if (value < 0 || value > 10) {
throw new UnexpectedResponseValueException(String.format("Invalid fan step: %d", value));
}
return new PercentType(BigDecimal.valueOf(value * 10));
}

public DecimalType getSupplyFanSpeed() throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,12 +30,6 @@ public class DanfossAirUnitBindingConstants {

public static String BINDING_ID = "danfossairunit";

// List of all Thing Type UIDs
public static ThingTypeUID THING_TYPE_SAMPLE = new ThingTypeUID(BINDING_ID, "sample");

// List of all Channel ids
public static String CHANNEL_1 = "channel1";

// The only thing type UIDs
public static ThingTypeUID THING_TYPE_AIRUNIT = new ThingTypeUID(BINDING_ID, "airunit");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,19 +30,22 @@
* The {@link DanfossAirUnitCommunicationController} class does the actual network communication with the air unit.
*
* @author Robert Bach - initial contribution
* @author Jacob Laursen - Refactoring, bugfixes and enhancements
*/

@NonNullByDefault
public class DanfossAirUnitCommunicationController {
public class DanfossAirUnitCommunicationController implements CommunicationController {

private static final int SOCKET_TIMEOUT_MILLISECONDS = 5_000;

private final Logger logger = LoggerFactory.getLogger(DanfossAirUnitCommunicationController.class);

private final InetAddress inetAddr;
private final int port;
private boolean connected = false;
private @Nullable Socket socket;
private @Nullable OutputStream oStream;
private @Nullable InputStream iStream;
private @Nullable OutputStream outputStream;
private @Nullable InputStream inputStream;

public DanfossAirUnitCommunicationController(InetAddress inetAddr, int port) {
this.inetAddr = inetAddr;
Expand All @@ -53,9 +56,11 @@ public synchronized void connect() throws IOException {
if (connected) {
return;
}
socket = new Socket(inetAddr, port);
oStream = socket.getOutputStream();
iStream = socket.getInputStream();
Socket localSocket = new Socket(inetAddr, port);
localSocket.setSoTimeout(SOCKET_TIMEOUT_MILLISECONDS);
this.outputStream = localSocket.getOutputStream();
this.inputStream = localSocket.getInputStream();
this.socket = localSocket;
connected = true;
}

Expand All @@ -64,15 +69,16 @@ public synchronized void disconnect() {
return;
}
try {
if (socket != null) {
socket.close();
Socket localSocket = this.socket;
if (localSocket != null) {
localSocket.close();
}
} catch (IOException ioe) {
logger.debug("Connection to air unit could not be closed gracefully. {}", ioe.getMessage());
} finally {
socket = null;
iStream = null;
oStream = null;
this.socket = null;
this.inputStream = null;
this.outputStream = null;
}
connected = false;
}
Expand All @@ -98,21 +104,27 @@ public synchronized byte[] sendRobustRequest(byte[] operation, byte[] register,
}

private synchronized byte[] sendRequestInternal(byte[] request) throws IOException {
OutputStream localOutputStream = this.outputStream;

if (oStream == null) {
if (localOutputStream == null) {
throw new IOException(
String.format("Output stream is null while sending request: %s", Arrays.toString(request)));
}
oStream.write(request);
oStream.flush();
localOutputStream.write(request);
localOutputStream.flush();

byte[] result = new byte[63];
if (iStream == null) {
InputStream localInputStream = this.inputStream;
if (localInputStream == null) {
throw new IOException(
String.format("Input stream is null while sending request: %s", Arrays.toString(request)));
}
// noinspection ResultOfMethodCallIgnored
iStream.read(result, 0, 63);

int bytesRead = localInputStream.read(result, 0, 63);
if (bytesRead < 63) {
throw new IOException(String.format(
"Error reading from stream, read returned %d as number of bytes read into the buffer", bytesRead));
}

return result;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ public boolean updateValue(String channelId, State newState) {
return writeToCache;
}

@NonNullByDefault
private static class StateWithTimestamp {
State state;
long timestamp;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,12 @@ public class DanfossAirUnitDiscoveryService extends AbstractDiscoveryService {
private static final int BROADCAST_PORT = 30045;
private static final byte[] DISCOVER_SEND = { 0x0c, 0x00, 0x30, 0x00, 0x11, 0x00, 0x12, 0x00, 0x13 };
private static final byte[] DISCOVER_RECEIVE = { 0x0d, 0x00, 0x07, 0x00, 0x02, 0x02, 0x00 };
private static final int TIMEOUT_IN_SECONDS = 15;

private final Logger logger = LoggerFactory.getLogger(DanfossAirUnitDiscoveryService.class);

public DanfossAirUnitDiscoveryService() {
super(SUPPORTED_THING_TYPES_UIDS, 15, true);
super(SUPPORTED_THING_TYPES_UIDS, TIMEOUT_IN_SECONDS, true);
}

@Override
Expand Down
Loading

0 comments on commit 50ac6a9

Please sign in to comment.