This is an automated email from the ASF dual-hosted git repository. sruehl pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/incubator-plc4x.git
The following commit(s) were added to refs/heads/master by this push: new f00e35d [ADS] fix wrong implementation of type bounds f00e35d is described below commit f00e35d5e3d5ed88be2e9d57a86449d7c0ac7d1f Author: Sebastian Rühl <sru...@apache.org> AuthorDate: Thu Oct 18 17:00:37 2018 +0200 [ADS] fix wrong implementation of type bounds --- .../apache/plc4x/java/ads/model/AdsDataType.java | 15 +++- .../plc4x/java/ads/model/AdsPlcFieldHandler.java | 84 ++++++++++++++-------- .../plc4x/java/ads/protocol/Plc4x2AdsProtocol.java | 3 +- .../ads/protocol/util/LittleEndianEncoder.java | 12 +--- .../java/ads/protocol/Plc4x2AdsProtocolTest.java | 8 +-- plc4j/protocols/ads/src/test/resources/logback.xml | 2 +- 6 files changed, 78 insertions(+), 46 deletions(-) diff --git a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsDataType.java b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsDataType.java index e6cf4c2..87925a8 100644 --- a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsDataType.java +++ b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsDataType.java @@ -174,7 +174,7 @@ public enum AdsDataType { * If no size specification is given, the default size of 80 characters will be used: Memory use [Bytes] = 80 + 1 Byte for string terminated Null character; * If string size specification is given: Memory use [Bytes] = String Size + 1 Byte for string terminated Null character); */ - STRING(81), + STRING(81 * 8), /** * TIME * Duration time. The most siginificant digit is one millisecond. The data type is handled internally like DWORD. @@ -510,7 +510,7 @@ public enum AdsDataType { this.upperBound = upperBound; this.typeName = name(); this.memoryUse = memoryUse; - this.targetByteSize = this.memoryUse * 8; + this.targetByteSize = this.memoryUse / 8; } public String getTypeName() { @@ -536,4 +536,15 @@ public enum AdsDataType { public boolean withinBounds(double other) { return other >= lowerBound && other <= upperBound; } + + @Override + public String toString() { + return "AdsDataType{" + + "typeName='" + typeName + '\'' + + ", lowerBound=" + lowerBound + + ", upperBound=" + upperBound + + ", memoryUse=" + memoryUse + + ", targetByteSize=" + targetByteSize + + "} " + super.toString(); + } } diff --git a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsPlcFieldHandler.java b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsPlcFieldHandler.java index bb9be24..9d1ec6a 100644 --- a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsPlcFieldHandler.java +++ b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/model/AdsPlcFieldHandler.java @@ -721,28 +721,28 @@ public class AdsPlcFieldHandler extends DefaultPlcFieldHandler { Class<? extends FieldItem> fieldType; switch (adsField.getAdsDataType()) { case BYTE: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultByteFieldItem.class; break; case WORD: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultByteArrayFieldItem.class; break; case DWORD: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultByteArrayFieldItem.class; break; case SINT: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultIntegerFieldItem.class; break; case USINT: fieldType = DefaultLongFieldItem.class; break; case INT: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultShortFieldItem.class; break; case UINT: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultIntegerFieldItem.class; break; case DINT: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultIntegerFieldItem.class; break; case UDINT: fieldType = DefaultLongFieldItem.class; @@ -754,7 +754,7 @@ public class AdsPlcFieldHandler extends DefaultPlcFieldHandler { fieldType = DefaultBigIntegerFieldItem.class; break; case INT32: - fieldType = DefaultLongFieldItem.class; + fieldType = DefaultIntegerFieldItem.class; break; case INT64: fieldType = DefaultLongFieldItem.class; @@ -820,39 +820,67 @@ public class AdsPlcFieldHandler extends DefaultPlcFieldHandler { AdsField adsField = (AdsField) field; BigDecimal minValue = BigDecimal.valueOf(adsField.getAdsDataType().getLowerBound()); BigDecimal maxValue = BigDecimal.valueOf(adsField.getAdsDataType().getUpperBound()); + Class<? extends FieldItem> fieldType; switch (adsField.getAdsDataType()) { case REAL: + fieldType = DefaultFloatFieldItem.class; break; case LREAL: + fieldType = DefaultDoubleFieldItem.class; break; default: throw new IllegalArgumentException( "Cannot assign floating point values to " + adsField.getAdsDataType().name() + " fields."); } - Double[] floatingPointValues = new Double[values.length]; - for (int i = 0; i < values.length; i++) { - if (values[i] instanceof Float) { - floatingPointValues[i] = ((Float) values[i]).doubleValue(); - } else if (values[i] instanceof Double) { - floatingPointValues[i] = (Double) values[i]; - } else { - throw new IllegalArgumentException( - "Value of type " + values[i].getClass().getName() + - " is not assignable to " + adsField.getAdsDataType().name() + " fields."); - } + if (fieldType == DefaultDoubleFieldItem.class) { + Double[] floatingPointValues = new Double[values.length]; + for (int i = 0; i < values.length; i++) { + if (values[i] instanceof Float) { + floatingPointValues[i] = ((Float) values[i]).doubleValue(); + } else if (values[i] instanceof Double) { + floatingPointValues[i] = (Double) values[i]; + } else { + throw new IllegalArgumentException( + "Value of type " + values[i].getClass().getName() + + " is not assignable to " + adsField.getAdsDataType().name() + " fields."); + } - if (minValue.compareTo(new BigDecimal(floatingPointValues[i])) > 0) { - throw new IllegalArgumentException( - "Value of " + floatingPointValues[i] + " exceeds allowed minimum for type " - + adsField.getAdsDataType().name() + " (min " + minValue.toString() + ")"); + if (minValue.compareTo(new BigDecimal(floatingPointValues[i])) > 0) { + throw new IllegalArgumentException( + "Value of " + floatingPointValues[i] + " exceeds allowed minimum for type " + + adsField.getAdsDataType().name() + " (min " + minValue.toString() + ")"); + } + if (maxValue.compareTo(new BigDecimal(floatingPointValues[i])) < 0) { + throw new IllegalArgumentException( + "Value of " + floatingPointValues[i] + " exceeds allowed maximum for type " + + adsField.getAdsDataType().name() + " (max " + maxValue.toString() + ")"); + } } - if (maxValue.compareTo(new BigDecimal(floatingPointValues[i])) < 0) { - throw new IllegalArgumentException( - "Value of " + floatingPointValues[i] + " exceeds allowed maximum for type " - + adsField.getAdsDataType().name() + " (max " + maxValue.toString() + ")"); + return new DefaultDoubleFieldItem(floatingPointValues); + } else { + Float[] floatingPointValues = new Float[values.length]; + for (int i = 0; i < values.length; i++) { + if (values[i] instanceof Float) { + floatingPointValues[i] = (Float) values[i]; + } else { + throw new IllegalArgumentException( + "Value of type " + values[i].getClass().getName() + + " is not assignable to " + adsField.getAdsDataType().name() + " fields."); + } + + if (minValue.compareTo(new BigDecimal(floatingPointValues[i])) > 0) { + throw new IllegalArgumentException( + "Value of " + floatingPointValues[i] + " exceeds allowed minimum for type " + + adsField.getAdsDataType().name() + " (min " + minValue.toString() + ")"); + } + if (maxValue.compareTo(new BigDecimal(floatingPointValues[i])) < 0) { + throw new IllegalArgumentException( + "Value of " + floatingPointValues[i] + " exceeds allowed maximum for type " + + adsField.getAdsDataType().name() + " (max " + maxValue.toString() + ")"); + } } + return new DefaultFloatFieldItem(floatingPointValues); } - return new DefaultDoubleFieldItem(floatingPointValues); } private FieldItem internalEncodeString(PlcField field, Object[] values) { diff --git a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocol.java b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocol.java index 1ba5cd9..fc67896 100644 --- a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocol.java +++ b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocol.java @@ -159,7 +159,8 @@ public class Plc4x2AdsProtocol extends MessageToMessageCodec<AmsPacket, PlcReque int bytesToBeWritten = bytes.length; int maxTheoreticalSize = directAdsField.getAdsDataType().getTargetByteSize() * directAdsField.getNumberOfElements(); if (bytesToBeWritten > maxTheoreticalSize) { - throw new PlcProtocolPayloadTooBigException("Ads", maxTheoreticalSize, bytesToBeWritten, values); + LOGGER.debug("Requested AdsDatatype {} is exceeded by number of bytes {}. Limit {}.", directAdsField.getAdsDataType(), bytesToBeWritten, maxTheoreticalSize); + throw new PlcProtocolPayloadTooBigException("ADS", maxTheoreticalSize, bytesToBeWritten, values); } Data data = Data.of(bytes); AmsPacket amsPacket = AdsWriteRequest.of(targetAmsNetId, targetAmsPort, sourceAmsNetId, sourceAmsPort, invokeId, indexGroup, indexOffset, data); diff --git a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/util/LittleEndianEncoder.java b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/util/LittleEndianEncoder.java index 44b1247..de41485 100644 --- a/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/util/LittleEndianEncoder.java +++ b/plc4j/protocols/ads/src/main/java/org/apache/plc4x/java/ads/protocol/util/LittleEndianEncoder.java @@ -191,16 +191,12 @@ public class LittleEndianEncoder { return localTimeStream .map(localTime -> ChronoUnit.MILLIS.between(LocalTime.of(0, 0), localTime)) .peek(value -> checkBound(adsDataType, value)) + .map(Long::intValue) .map(time -> new byte[]{ (byte) (time & 0x00000000_000000ffL), (byte) ((time & 0x00000000_0000ff00L) >> 8), (byte) ((time & 0x00000000_00ff0000L) >> 16), (byte) ((time & 0x00000000_ff000000L) >> 24), - - (byte) ((time & 0x000000ff_00000000L) >> 32), - (byte) ((time & 0x0000ff00_00000000L) >> 40), - (byte) ((time & 0x00ff0000_00000000L) >> 48), - (byte) ((time & 0xff000000_00000000L) >> 56), }); } @@ -210,16 +206,12 @@ public class LittleEndianEncoder { .map(localDate -> localDate.atTime(0, 0).toInstant(ZoneOffset.UTC)) .map(Instant::getEpochSecond) .peek(value -> checkBound(adsDataType, value)) + .map(Long::intValue) .map(time -> new byte[]{ (byte) (time & 0x00000000_000000ffL), (byte) ((time & 0x00000000_0000ff00L) >> 8), (byte) ((time & 0x00000000_00ff0000L) >> 16), (byte) ((time & 0x00000000_ff000000L) >> 24), - - (byte) ((time & 0x000000ff_00000000L) >> 32), - (byte) ((time & 0x0000ff00_00000000L) >> 40), - (byte) ((time & 0x00ff0000_00000000L) >> 48), - (byte) ((time & 0xff000000_00000000L) >> 56), }); } diff --git a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocolTest.java b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocolTest.java index 2cdd449..4f38bd4 100644 --- a/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocolTest.java +++ b/plc4j/protocols/ads/src/test/java/org/apache/plc4x/java/ads/protocol/Plc4x2AdsProtocolTest.java @@ -111,14 +111,14 @@ public class Plc4x2AdsProtocolTest { ImmutablePair.of( new PlcRequestContainer<>( (InternalPlcRequest) new DefaultPlcWriteRequest.Builder(null, new AdsPlcFieldHandler()) // TODO: remove null - .addItem(RandomStringUtils.randomAscii(10), "1/1:" + pair.adsDataType, pair.getValue()) + .addItem(RandomStringUtils.randomAscii(10), "1/1:" + pair.adsDataType.name(), pair.getValue()) .build(), new CompletableFuture<>()), AdsWriteResponse.of(targetAmsNetId, targetAmsPort, sourceAmsNetId, sourceAmsPort, invokeId, Result.of(0)) ), ImmutablePair.of( new PlcRequestContainer<>( (InternalPlcRequest) new DefaultPlcReadRequest.Builder(null, new AdsPlcFieldHandler()) // TODO: remove null - .addItem(RandomStringUtils.randomAscii(10), "1/1:" + pair.adsDataType) + .addItem(RandomStringUtils.randomAscii(10), "1/1:" + pair.adsDataType.name()) .build(), new CompletableFuture<>()), AdsReadResponse.of(targetAmsNetId, targetAmsPort, sourceAmsNetId, sourceAmsPort, invokeId, Result.of(0), Data.of(pair.getByteRepresentation())) ) @@ -145,7 +145,7 @@ public class Plc4x2AdsProtocolTest { dataTypeMap.put(LocalDateTime.class, AdsDataType.DATE_AND_TIME); dataTypeMap.put(byte[].class, AdsDataType.BYTE); dataTypeMap.put(Byte[].class, AdsDataType.BYTE); - return new AdsDataTypePair(dataTypePair, dataTypeMap.getOrDefault(dataTypePair.getDataTypeClass(), AdsDataType.BYTE)); + return new AdsDataTypePair(dataTypePair, dataTypeMap.get(dataTypePair.getDataTypeClass())); } private static class AdsDataTypePair extends Plc4XSupportedDataTypes.DataTypePair { @@ -154,7 +154,7 @@ public class Plc4x2AdsProtocolTest { private AdsDataTypePair(Plc4XSupportedDataTypes.DataTypePair dataTypePair, AdsDataType adsDataType) { super(dataTypePair.getDataTypePair()); - this.adsDataType = adsDataType; + this.adsDataType = Objects.requireNonNull(adsDataType); } } diff --git a/plc4j/protocols/ads/src/test/resources/logback.xml b/plc4j/protocols/ads/src/test/resources/logback.xml index dd243bd..5bfdbdc 100644 --- a/plc4j/protocols/ads/src/test/resources/logback.xml +++ b/plc4j/protocols/ads/src/test/resources/logback.xml @@ -31,7 +31,7 @@ </encoder> </appender> - <root level="info"> + <root level="debug"> <appender-ref ref="STDOUT" /> </root>