Skip to content

Commit 2f7b422

Browse files
jlaurjlaur
authored
[danfossairunit] Fix TCP read handling and improve value parsing precision (#20533)
* Fix partial TCP reads causing invalid response data Signed-off-by: Jacob Laursen <jacob-github@vindvejr.dk>
1 parent 1e7a248 commit 2f7b422

5 files changed

Lines changed: 120 additions & 85 deletions

File tree

‎bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/CommunicationController.java‎

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,12 +23,20 @@
2323
* @author Jacob Laursen - Initial contribution
2424
*/
2525
@NonNullByDefault
26-
public interface CommunicationController {
27-
void connect() throws IOException;
28-
29-
void disconnect();
26+
public interface CommunicationController extends AutoCloseable {
27+
/**
28+
* Closes any open connection. Should be called when the controller is no longer needed.
29+
*/
30+
@Override
31+
void close();
3032

33+
/**
34+
* Sends a request to the air unit. The implementation will establish a connection on demand.
35+
*/
3136
byte[] sendRobustRequest(Parameter parameter) throws IOException;
3237

38+
/**
39+
* Sends a request to the air unit. The implementation will establish a connection on demand.
40+
*/
3341
byte[] sendRobustRequest(Parameter parameter, byte[] value) throws IOException;
3442
}

‎bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/DanfossAirUnit.java‎

Lines changed: 24 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -93,15 +93,6 @@ private int getInt(Parameter parameter) throws IOException, UnexpectedResponseVa
9393
return ((result[0] & 0xff) << 24) | ((result[1] & 0xff) << 16) | ((result[2] & 0xff) << 8) | (result[3] & 0xff);
9494
}
9595

96-
private float getTemperature(Parameter parameter) throws IOException, UnexpectedResponseValueException {
97-
short shortTemp = getShort(parameter);
98-
float temp = ((float) shortTemp) / 100;
99-
if (temp <= -274 || temp > 100) {
100-
throw new UnexpectedResponseValueException("Invalid temperature: %s".formatted(temp));
101-
}
102-
return temp;
103-
}
104-
10596
private Instant getTimestamp(Parameter parameter) throws IOException, UnexpectedResponseValueException {
10697
byte[] result = communicationController.sendRobustRequest(parameter);
10798
return asInstant(result);
@@ -131,9 +122,9 @@ private static int asUnsignedByte(byte b) {
131122
return b & 0xFF;
132123
}
133124

134-
private static float asPercentByte(byte b) {
135-
float f = asUnsignedByte(b);
136-
return f * 100 / 255;
125+
private static BigDecimal asPercentByte(byte b) {
126+
return BigDecimal.valueOf(asUnsignedByte(b)).multiply(BigDecimal.valueOf(100)).divide(BigDecimal.valueOf(255),
127+
1, RoundingMode.HALF_UP);
137128
}
138129

139130
public String getUnitName() throws IOException {
@@ -201,48 +192,56 @@ public OnOffType getBypass() throws IOException {
201192
}
202193

203194
public QuantityType<Dimensionless> getHumidity() throws IOException {
204-
BigDecimal value = BigDecimal.valueOf(asPercentByte(getByte(Parameter.HUMIDITY)));
205-
return new QuantityType<>(value.setScale(1, RoundingMode.HALF_UP), Units.PERCENT);
195+
BigDecimal value = asPercentByte(getByte(Parameter.HUMIDITY));
196+
return new QuantityType<>(value, Units.PERCENT);
206197
}
207198

208199
public QuantityType<Temperature> getRoomTemperature() throws IOException, UnexpectedResponseValueException {
209-
return getTemperatureAsDecimalType(Parameter.ROOM_TEMPERATURE);
200+
return getTemperatureAsQuantityType(Parameter.ROOM_TEMPERATURE);
210201
}
211202

212203
public QuantityType<Temperature> getRoomTemperatureCalculated()
213204
throws IOException, UnexpectedResponseValueException {
214-
return getTemperatureAsDecimalType(Parameter.ROOM_TEMPERATURE_CALCULATED);
205+
return getTemperatureAsQuantityType(Parameter.ROOM_TEMPERATURE_CALCULATED);
215206
}
216207

217208
public QuantityType<Temperature> getOutdoorTemperature() throws IOException, UnexpectedResponseValueException {
218-
return getTemperatureAsDecimalType(Parameter.OUTDOOR_TEMPERATURE);
209+
return getTemperatureAsQuantityType(Parameter.OUTDOOR_TEMPERATURE);
219210
}
220211

221212
public QuantityType<Temperature> getSupplyTemperature() throws IOException, UnexpectedResponseValueException {
222-
return getTemperatureAsDecimalType(Parameter.SUPPLY_TEMPERATURE);
213+
return getTemperatureAsQuantityType(Parameter.SUPPLY_TEMPERATURE);
223214
}
224215

225216
public QuantityType<Temperature> getExtractTemperature() throws IOException, UnexpectedResponseValueException {
226-
return getTemperatureAsDecimalType(Parameter.EXTRACT_TEMPERATURE);
217+
return getTemperatureAsQuantityType(Parameter.EXTRACT_TEMPERATURE);
227218
}
228219

229220
public QuantityType<Temperature> getExhaustTemperature() throws IOException, UnexpectedResponseValueException {
230-
return getTemperatureAsDecimalType(Parameter.EXHAUST_TEMPERATURE);
221+
return getTemperatureAsQuantityType(Parameter.EXHAUST_TEMPERATURE);
231222
}
232223

233-
private QuantityType<Temperature> getTemperatureAsDecimalType(Parameter parameter)
224+
private QuantityType<Temperature> getTemperatureAsQuantityType(Parameter parameter)
234225
throws IOException, UnexpectedResponseValueException {
235-
BigDecimal value = BigDecimal.valueOf(getTemperature(parameter));
236-
return new QuantityType<>(value.setScale(1, RoundingMode.HALF_UP), SIUnits.CELSIUS);
226+
short raw = getShort(parameter);
227+
228+
if (raw < -27315 || raw > 10000) {
229+
BigDecimal invalid = BigDecimal.valueOf(raw).movePointLeft(2);
230+
throw new UnexpectedResponseValueException("Invalid temperature: %s".formatted(invalid));
231+
}
232+
233+
BigDecimal value = BigDecimal.valueOf(raw).movePointLeft(2).setScale(1, RoundingMode.HALF_UP);
234+
235+
return new QuantityType<>(value, SIUnits.CELSIUS);
237236
}
238237

239238
public DecimalType getBatteryLife() throws IOException {
240239
return new DecimalType(BigDecimal.valueOf(asUnsignedByte(getByte(Parameter.BATTERY_LIFE))));
241240
}
242241

243242
public DecimalType getFilterLife() throws IOException {
244-
BigDecimal value = BigDecimal.valueOf(asPercentByte(getByte(Parameter.FILTER_LIFE)));
245-
return new DecimalType(value.setScale(1, RoundingMode.HALF_UP));
243+
BigDecimal value = asPercentByte(getByte(Parameter.FILTER_LIFE));
244+
return new DecimalType(value);
246245
}
247246

248247
public DecimalType getFilterPeriod() throws IOException {

‎bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/DanfossAirUnitCommunicationController.java‎

Lines changed: 80 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,11 @@
1818
import java.net.InetAddress;
1919
import java.net.InetSocketAddress;
2020
import java.net.Socket;
21-
import java.util.Arrays;
2221

2322
import org.eclipse.jdt.annotation.NonNullByDefault;
2423
import org.eclipse.jdt.annotation.Nullable;
2524
import org.openhab.binding.danfossairunit.internal.protocol.Parameter;
25+
import org.openhab.core.util.HexUtils;
2626
import org.slf4j.Logger;
2727
import org.slf4j.LoggerFactory;
2828

@@ -39,47 +39,51 @@ public class DanfossAirUnitCommunicationController implements CommunicationContr
3939
private static final int TCP_PORT = 30046;
4040
private static final int READ_TIMEOUT_MILLISECONDS = 5_000;
4141
private static final int DEFAULT_CONNECT_TIMEOUT_MILLISECONDS = 5_000;
42+
private static final int RESPONSE_LENGTH = 63;
43+
private static final byte[] EMPTY = new byte[0];
4244

4345
private final Logger logger = LoggerFactory.getLogger(DanfossAirUnitCommunicationController.class);
44-
private final InetAddress inetAddr;
46+
private final InetAddress hostAddress;
4547
private final int connectTimeoutMilliseconds;
4648

47-
private boolean connected = false;
4849
private @Nullable Socket socket;
4950
private @Nullable OutputStream outputStream;
5051
private @Nullable InputStream inputStream;
5152

52-
public DanfossAirUnitCommunicationController(InetAddress inetAddr) {
53-
this(inetAddr, DEFAULT_CONNECT_TIMEOUT_MILLISECONDS);
53+
public DanfossAirUnitCommunicationController(InetAddress hostAddress) {
54+
this(hostAddress, DEFAULT_CONNECT_TIMEOUT_MILLISECONDS);
5455
}
5556

56-
public DanfossAirUnitCommunicationController(InetAddress inetAddr, int connectTimeoutMilliseconds) {
57-
this.inetAddr = inetAddr;
57+
public DanfossAirUnitCommunicationController(InetAddress hostAddress, int connectTimeoutMilliseconds) {
58+
this.hostAddress = hostAddress;
5859
this.connectTimeoutMilliseconds = connectTimeoutMilliseconds;
5960
}
6061

61-
@Override
62-
public synchronized void connect() throws IOException {
63-
if (connected) {
62+
private synchronized void connect() throws IOException {
63+
Socket socket = this.socket;
64+
if (socket != null && !socket.isClosed()) {
65+
// Already connected
6466
return;
6567
}
66-
Socket socket = this.socket = new Socket();
67-
socket.connect(new InetSocketAddress(inetAddr, TCP_PORT), connectTimeoutMilliseconds);
68+
69+
socket = new Socket();
70+
socket.connect(new InetSocketAddress(hostAddress, TCP_PORT), connectTimeoutMilliseconds);
6871
socket.setSoTimeout(READ_TIMEOUT_MILLISECONDS);
69-
outputStream = socket.getOutputStream();
70-
inputStream = socket.getInputStream();
71-
connected = true;
72+
73+
OutputStream outputStream = socket.getOutputStream();
74+
InputStream inputStream = socket.getInputStream();
75+
76+
this.socket = socket;
77+
this.outputStream = outputStream;
78+
this.inputStream = inputStream;
7279
}
7380

7481
@Override
75-
public synchronized void disconnect() {
76-
if (!connected) {
77-
return;
78-
}
82+
public synchronized void close() {
7983
try {
80-
Socket localSocket = this.socket;
81-
if (localSocket != null) {
82-
localSocket.close();
84+
Socket socket = this.socket;
85+
if (socket != null && !socket.isClosed()) {
86+
socket.close();
8387
}
8488
} catch (IOException ioe) {
8589
logger.debug("Connection to air unit could not be closed gracefully. {}", ioe.getMessage());
@@ -88,51 +92,78 @@ public synchronized void disconnect() {
8892
this.inputStream = null;
8993
this.outputStream = null;
9094
}
91-
connected = false;
9295
}
9396

9497
@Override
9598
public byte[] sendRobustRequest(Parameter parameter) throws IOException {
96-
return sendRobustRequest(parameter, new byte[] {});
99+
return sendRobustRequest(parameter, EMPTY);
97100
}
98101

99102
@Override
100103
public synchronized byte[] sendRobustRequest(Parameter parameter, byte[] value) throws IOException {
101-
connect();
102104
byte[] request = parameter.getRequest(value);
105+
103106
try {
104-
return sendRequestInternal(request);
105-
} catch (IOException ioe) {
106-
// retry once if there was connection problem
107-
disconnect();
108107
connect();
109-
return sendRequestInternal(request);
108+
109+
byte[] response = sendRequestInternal(request);
110+
if (logger.isTraceEnabled()) {
111+
logger.trace("{} response: {}", parameter, HexUtils.bytesToHex(response));
112+
}
113+
114+
return response;
115+
} catch (IOException suppressedException) {
116+
logger.debug("{} request failed, retrying once: {}", parameter, suppressedException.getMessage());
117+
118+
close();
119+
120+
try {
121+
connect();
122+
123+
byte[] response = sendRequestInternal(request);
124+
if (logger.isTraceEnabled()) {
125+
logger.trace("{} response: {}", parameter, HexUtils.bytesToHex(response));
126+
}
127+
128+
return response;
129+
} catch (IOException e) {
130+
suppressedException.addSuppressed(e);
131+
throw suppressedException;
132+
}
110133
}
111134
}
112135

113-
private synchronized byte[] sendRequestInternal(byte[] request) throws IOException {
114-
OutputStream localOutputStream = this.outputStream;
136+
private byte[] sendRequestInternal(byte[] request) throws IOException {
137+
OutputStream outputStream = this.outputStream;
138+
InputStream inputStream = this.inputStream;
115139

116-
if (localOutputStream == null) {
117-
throw new IOException(
118-
String.format("Output stream is null while sending request: %s", Arrays.toString(request)));
119-
}
120-
localOutputStream.write(request);
121-
localOutputStream.flush();
122-
123-
byte[] result = new byte[63];
124-
InputStream localInputStream = this.inputStream;
125-
if (localInputStream == null) {
126-
throw new IOException(
127-
String.format("Input stream is null while sending request: %s", Arrays.toString(request)));
140+
if (outputStream == null || inputStream == null) {
141+
throw new IOException("Input/output streams not initialized");
128142
}
129143

130-
int bytesRead = localInputStream.read(result, 0, 63);
131-
if (bytesRead < 63) {
132-
throw new IOException(String.format(
133-
"Error reading from stream, read returned %d as number of bytes read into the buffer", bytesRead));
144+
outputStream.write(request);
145+
outputStream.flush();
146+
147+
return readExact(inputStream, RESPONSE_LENGTH);
148+
}
149+
150+
private byte[] readExact(InputStream inputStream, int length) throws IOException {
151+
byte[] response = new byte[length];
152+
int offset = 0;
153+
154+
while (offset < length) {
155+
int bytesRead = inputStream.read(response, offset, length - offset);
156+
157+
if (bytesRead == -1) {
158+
throw new IOException("Stream closed while reading response");
159+
}
160+
if (bytesRead == 0) {
161+
throw new IOException("Read returned 0 bytes, possible stream stall");
162+
}
163+
164+
offset += bytesRead;
134165
}
135166

136-
return result;
167+
return response;
137168
}
138169
}

‎bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/discovery/DanfossAirUnitDiscoveryService.java‎

Lines changed: 2 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -166,14 +166,11 @@ private void sendBroadcastToDiscoverThing(DatagramSocket socket, InetAddress bro
166166
}
167167

168168
private @Nullable String getSerialNumber(InetAddress address) {
169-
var controller = new DanfossAirUnitCommunicationController(address, SOCKET_TIMEOUT_MILLISECONDS);
170-
var unit = new DanfossAirUnit(controller, FixedTimeZoneProvider.of(ZoneId.of("UTC")));
171-
try {
169+
try (var controller = new DanfossAirUnitCommunicationController(address, SOCKET_TIMEOUT_MILLISECONDS)) {
170+
var unit = new DanfossAirUnit(controller, FixedTimeZoneProvider.of(ZoneId.of("UTC")));
172171
return unit.getUnitSerialNumber();
173172
} catch (IOException | UnexpectedResponseValueException e) {
174173
return null;
175-
} finally {
176-
controller.disconnect();
177174
}
178175
}
179176
}

‎bundles/org.openhab.binding.danfossairunit/src/main/java/org/openhab/binding/danfossairunit/internal/handler/DanfossAirUnitHandler.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ private void updateAllChannels() {
127127
return;
128128
}
129129

130-
logger.debug("Updating DanfossHRV data '{}'", getThing().getUID());
130+
logger.trace("Updating DanfossHRV data '{}'", getThing().getUID());
131131

132132
for (Channel channel : Channel.values()) {
133133
if (Thread.interrupted()) {
@@ -219,7 +219,7 @@ public void dispose() {
219219

220220
DanfossAirUnitCommunicationController communicationController = this.communicationController;
221221
if (communicationController != null) {
222-
communicationController.disconnect();
222+
communicationController.close();
223223
}
224224
this.communicationController = null;
225225
}

0 commit comments

Comments
 (0)
⚡