You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by sr...@apache.org on 2026/08/19 21:08:51 UTC
(plc4x) 01/04: fix(plc4j/spi): align BCD nibbles with the buffer's right-aligned partial fields
This is an automated email from the ASF dual-hosted git repository.
sruehl pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit 35aff81bec138bcba11333785d5a3020ccd94fb1
Author: Sebastian Rühl <sr...@apache.org>
AuthorDate: Wed Aug 19 23:05:59 2026 +0200
fix(plc4j/spi): align BCD nibbles with the buffer's right-aligned partial fields
The byte buffers hand partial (non byte multiple) fields to an encoding
right-aligned in the byte array: readBits starts filling at bit
(8 - (numBits % 8)) % 8 and writeBits consumes from the same offset, which
EncodingUnsignedBinary follows too - its left-align shift is deliberately
commented out. EncodingBCD instead walked nibbles from the high nibble of byte
zero, so the two disagreed for every field whose digit count is odd.
A 12 bit field carrying 123 arrives as {0x01, 0x23} and decoded to 12; a 4 bit
nibble decoded from the padding half and came out 0. The write path was broken
symmetrically, packing {0x12, 0x30} where the buffer then emitted the low 12
bits, 0x230. Widths that are a multiple of 8 were always correct, which is why
this stayed hidden: the S7 DateAndTime 12 bit millisecond and 4 bit day-of-week
fields are the only non byte multiple BCD fields in the repository.
Fixed in the encoding rather than in the buffers, since the encoding was the
outlier - indexing nibbles from the padding offset, which is bit identical for
the even digit counts that already worked. The eight duplicated pack and unpack
loops collapsed into two helpers on the way.
The Go implementation already had these semantics, so its comment describing the
divergence as live is no longer true and now records the fix instead.
---
plc4go/spi/utils/encodingBCD.go | 33 +---
.../buffers/bytebased/encoding/EncodingBCD.java | 189 ++++++++++---------
.../spi/buffers/bytebased/BcdBitAlignmentTest.java | 201 +++++++++++++++++++++
.../bytebased/encoding/EncodingBCDTest.java | 97 +++++++++-
4 files changed, 403 insertions(+), 117 deletions(-)
diff --git a/plc4go/spi/utils/encodingBCD.go b/plc4go/spi/utils/encodingBCD.go
index f7af23a6fd..d69bd8c0c8 100644
--- a/plc4go/spi/utils/encodingBCD.go
+++ b/plc4go/spi/utils/encodingBCD.go
@@ -120,29 +120,16 @@ func writerArgsSelectBCD(args []WithWriterArgs) bool {
// bitLength-wide window. So the nibble stream is simply walked from the top of
// that window downwards.
//
-// NOTE ON A DIVERGENCE FROM plc4j (plc4j is the buggy side here; verified by
-// running it, see internal/s7/DateAndTimeBcd_test.go for the wire vector):
-// plc4j routes the bytes through ReadBufferByteBased.readBits, which RIGHT
-// aligns a partial (non byte multiple) read inside the result array
-// (`int resultBitIndex = (8 - (numBits % 8)) % 8;`), while
-// EncodingBCD.decodeInt/decodeLong/decodeShort/decodeByte/decodeBigInteger then
-// read nibbles LEFT aligned from bytes[0] (digit i comes from bytes[i/2], high
-// or low nibble). The two disagree for every field whose bit length is not a
-// multiple of 8: a 12 bit read of 0x123 lands as {0x01, 0x23} and decodes to 12
-// instead of 123, and a 4 bit read of nibble 0x4 lands as {0x04} but is read
-// from the HIGH nibble and decodes to 0 instead of 4.
-//
-// The plc4j write path is broken symmetrically: EncodingBCD.encodeInt packs the
-// digits LEFT aligned into ceil(numDigits/2) bytes (123 over 12 bits becomes
-// {0x12, 0x30}) while WriteBufferByteBased.writeBits takes the value bits RIGHT
-// aligned with the same offset, so it emits the LAST 12 bits, i.e. 0x230.
-//
-// This implementation uses plain nibble stream semantics, which is what the
-// Siemens DATE_AND_TIME wire format actually is and what plc4j also produces
-// for every bitLength%8 == 0 case (hence nobody noticed: the S7 "msec" uint 12
-// and "dow" uint 4 fields are the only non byte multiple BCD fields in the whole
-// repository, at s7.mspec lines 307-316 for the DateAndTime type and again at
-// lines 851/853 for the DATE_AND_TIME data item).
+// The two languages agree here now. plc4j used to disagree for every field whose
+// bit length is not a multiple of 8: its buffers right align a partial read
+// inside the byte array while EncodingBCD walked nibbles from the high nibble of
+// byte 0, so a 12 bit 0x123 decoded to 12 and a 4 bit nibble decoded to 0, with
+// the write path broken symmetrically. That was fixed in EncodingBCD by indexing
+// nibbles from the padding offset instead, which is bit identical for the byte
+// multiple widths that were always correct. Only the S7 "msec" uint 12 and "dow"
+// uint 4 fields are non byte multiple BCD fields in this repository (s7.mspec
+// lines 307-316 for the DateAndTime type, again at 851/853 for the DATE_AND_TIME
+// data item), which is why the bug stayed invisible for so long.
func decodeBCD(raw uint64, bitLength uint8) (uint64, error) {
if bitLength%4 != 0 {
return 0, errors.Errorf("bit length %d must be a multiple of 4 for BCD encoding", bitLength)
diff --git a/plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCD.java b/plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCD.java
index 4601fdbc94..3b13f5541a 100644
--- a/plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCD.java
+++ b/plc4j/spi/buffers/byte/src/main/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCD.java
@@ -25,6 +25,25 @@ import java.math.BigInteger;
/**
* BCD = Binary Encoded Decimal (A decimal number is represented by a sequence of 4-bit hexadecimal values from 0-9)
* <a href="https://www.elektronik-kompendium.de/sites/dig/1010311.htm">...</a>
+ * <p>
+ * IMPORTANT - bit alignment: the byte-based buffers exchange partial (non byte multiple) fields with the
+ * encodings RIGHT-aligned in the returned/accepted {@code byte[]}. {@code ReadBufferByteBased.readBits}
+ * starts filling the result at bit {@code (8 - (numBits % 8)) % 8} and {@code WriteBufferByteBased.writeBits}
+ * consumes the value starting at that same bit, so a 12 bit field carrying the digits {@code 123} is the
+ * array {@code {0x01, 0x23}} - not {@code {0x12, 0x30}}. {@code EncodingUnsignedBinary} follows the same
+ * convention (its {@code shift} is deliberately 0). This class therefore skips the leading padding nibbles
+ * instead of packing/unpacking digits from the high nibble of byte 0: for a field whose digit count is odd
+ * the first digit lives in the LOW nibble of the first byte. Getting this wrong is invisible whenever
+ * {@code numBits % 8 == 0} (offset 0) and corrupts every odd-digit-count field otherwise - which is exactly
+ * how the S7 {@code DateAndTime} 12 bit millisecond and 4 bit day-of-week fields used to break.
+ * <p>
+ * CAVEAT - byte order: the buffers put {@code byteOrder.process(bytes)} between this encoding and the
+ * bit stream. Reversing a right-aligned {@code byte[]} moves the leading padding nibble into the middle
+ * of the field, so for a field whose digit count is odd the contract above only holds under a
+ * big-endian byte order (the default, and the only one any mspec currently pairs with BCD): under
+ * {@code LITTLE_ENDIAN} a 12 bit write of {@code 123} reads back as {@code 103}. Reversing whole bytes
+ * is simply not a meaningful operation on a nibble stream that does not start on a byte boundary; if a
+ * protocol ever needs byte-swapped BCD it has to swap complete BCD bytes, not the encoded field.
*/
public class EncodingBCD extends BaseEncodingDefault {
@@ -41,6 +60,68 @@ public class EncodingBCD extends BaseEncodingDefault {
return NAME;
}
+ /**
+ * Number of bytes a {@code numBits} wide field occupies in the buffer representation.
+ */
+ private static int numBytesFor(int numBits) {
+ return (numBits + 7) / 8;
+ }
+
+ /**
+ * Index of the nibble holding the most significant BCD digit. Non-zero (namely 1) exactly when the
+ * field has an odd number of digits, in which case the leading nibble of the first byte is padding.
+ */
+ private static int firstNibbleIndex(int numBits) {
+ return (numBytesFor(numBits) * 2) - (numBits / 4);
+ }
+
+ private static void requireMultipleOfFour(int numBits) {
+ if (numBits % 4 != 0) {
+ throw new IllegalArgumentException("numBits must be a multiple of 4");
+ }
+ }
+
+ /**
+ * Same guard (and same message) {@code EncodingUnsignedBinary} applies, so a short array reports
+ * what is missing instead of throwing a raw {@link ArrayIndexOutOfBoundsException} out of the
+ * nibble unpacking. A {@code null} array still fails with a {@link NullPointerException}.
+ */
+ private static void requireEnoughBytes(int numBits, byte[] bytes) {
+ int numBytes = numBytesFor(numBits);
+ if (bytes.length < numBytes) {
+ throw new IllegalArgumentException("Expected at least " + numBytes + " bytes");
+ }
+ }
+
+ /**
+ * Reads the BCD digit stored in the given nibble, validating it is in the range 0-9.
+ */
+ private static int readDigit(byte[] bytes, int nibbleIndex) {
+ int currentByte = bytes[nibbleIndex / 2];
+ int digit = ((nibbleIndex % 2) == 0) ? ((currentByte >> 4) & 0x0F) : (currentByte & 0x0F);
+ if (digit > 9) {
+ throw new IllegalArgumentException("Invalid BCD digit: " + digit);
+ }
+ return digit;
+ }
+
+ /**
+ * Packs the given digits (most significant first) right-aligned into a {@code numBits} wide field.
+ */
+ private static byte[] packDigits(int numBits, byte[] digits) {
+ byte[] result = new byte[numBytesFor(numBits)];
+ int nibbleIndex = firstNibbleIndex(numBits);
+ for (byte digit : digits) {
+ if ((nibbleIndex % 2) == 0) {
+ result[nibbleIndex / 2] |= (byte) (digit << 4);
+ } else {
+ result[nibbleIndex / 2] |= digit;
+ }
+ nibbleIndex++;
+ }
+ return result;
+ }
+
@Override
public byte[] encodeByte(int numBits, byte value) {
return encodeInt(numBits, value & 0xFF);
@@ -75,9 +156,7 @@ public class EncodingBCD extends BaseEncodingDefault {
@Override
public byte[] encodeInt(int numBits, int value) {
- if (numBits % 4 != 0) {
- throw new IllegalArgumentException("numBits must be a multiple of 4");
- }
+ requireMultipleOfFour(numBits);
if (value < 0) {
throw new IllegalArgumentException("BCD encoding only supports non-negative integers");
}
@@ -98,51 +177,27 @@ public class EncodingBCD extends BaseEncodingDefault {
value /= 10;
}
- // Pack digits into bytes (two digits per byte)
- byte[] result = new byte[(numDigits + 1) / 2];
- for (int i = 0; i < numDigits; i += 2) {
- int high = digits[i];
- int low = (i + 1 < numDigits) ? digits[i + 1] : 0;
- result[i / 2] = (byte) ((high << 4) | low);
- }
-
- return result;
+ return packDigits(numBits, digits);
}
@Override
public int decodeInt(int numBits, byte[] bytes) {
- if (numBits % 4 != 0) {
- throw new IllegalArgumentException("numBits must be a multiple of 4");
- }
+ requireMultipleOfFour(numBits);
+ requireEnoughBytes(numBits, bytes);
int numDigits = numBits / 4;
+ int firstNibble = firstNibbleIndex(numBits);
int value = 0;
for (int i = 0; i < numDigits; i++) {
- int byteIndex = i / 2;
- boolean evenDigitNumber = (i % 2 == 0);
-
- int digit;
- if (evenDigitNumber) {
- digit = (bytes[byteIndex] >> 4) & 0x0F;
- } else {
- digit = bytes[byteIndex] & 0x0F;
- }
-
- if (digit > 9) {
- throw new IllegalArgumentException("Invalid BCD digit: " + digit);
- }
-
- value = value * 10 + digit;
+ value = value * 10 + readDigit(bytes, firstNibble + i);
}
return value;
}
@Override
public byte[] encodeLong(int numBits, long value) {
- if (numBits % 4 != 0) {
- throw new IllegalArgumentException("numBits must be a multiple of 4");
- }
+ requireMultipleOfFour(numBits);
if (value < 0) {
throw new IllegalArgumentException("BCD encoding only supports non-negative integers");
}
@@ -163,42 +218,20 @@ public class EncodingBCD extends BaseEncodingDefault {
value /= 10;
}
- // Pack digits into bytes (two digits per byte)
- byte[] result = new byte[(numDigits + 1) / 2];
- for (int i = 0; i < numDigits; i += 2) {
- int high = digits[i];
- int low = (i + 1 < numDigits) ? digits[i + 1] : 0;
- result[i / 2] = (byte) ((high << 4) | low);
- }
-
- return result;
+ return packDigits(numBits, digits);
}
@Override
public long decodeLong(int numBits, byte[] bytes) {
- if (numBits % 4 != 0) {
- throw new IllegalArgumentException("numBits must be a multiple of 4");
- }
+ requireMultipleOfFour(numBits);
+ requireEnoughBytes(numBits, bytes);
int numDigits = numBits / 4;
+ int firstNibble = firstNibbleIndex(numBits);
long value = 0;
for (int i = 0; i < numDigits; i++) {
- int byteIndex = i / 2;
- boolean evenDigitNumber = (i % 2 == 0);
-
- int digit;
- if (evenDigitNumber) {
- digit = (bytes[byteIndex] >> 4) & 0x0F;
- } else {
- digit = bytes[byteIndex] & 0x0F;
- }
-
- if (digit > 9) {
- throw new IllegalArgumentException("Invalid BCD digit: " + digit);
- }
-
- value = value * 10 + digit;
+ value = value * 10 + readDigit(bytes, firstNibble + i);
}
return value;
}
@@ -206,9 +239,7 @@ public class EncodingBCD extends BaseEncodingDefault {
@Override
public byte[] encodeBigInteger(int numBits, BigInteger value) {
- if (numBits % 4 != 0) {
- throw new IllegalArgumentException("numBits must be a multiple of 4");
- }
+ requireMultipleOfFour(numBits);
if (value.compareTo(BigInteger.ZERO) < 0) {
throw new IllegalArgumentException("BCD encoding only supports non-negative integers");
}
@@ -229,42 +260,20 @@ public class EncodingBCD extends BaseEncodingDefault {
value = value.divide(BigInteger.TEN);
}
- // Pack digits into bytes (two digits per byte)
- byte[] result = new byte[(numDigits + 1) / 2];
- for (int i = 0; i < numDigits; i += 2) {
- int high = digits[i];
- int low = (i + 1 < numDigits) ? digits[i + 1] : 0;
- result[i / 2] = (byte) ((high << 4) | low);
- }
-
- return result;
+ return packDigits(numBits, digits);
}
@Override
public BigInteger decodeBigInteger(int numBits, byte[] bytes) {
- if (numBits % 4 != 0) {
- throw new IllegalArgumentException("numBits must be a multiple of 4");
- }
+ requireMultipleOfFour(numBits);
+ requireEnoughBytes(numBits, bytes);
int numDigits = numBits / 4;
+ int firstNibble = firstNibbleIndex(numBits);
BigInteger value = BigInteger.ZERO;
for (int i = 0; i < numDigits; i++) {
- int byteIndex = i / 2;
- boolean evenDigitNumber = (i % 2 == 0);
-
- int digit;
- if (evenDigitNumber) {
- digit = (bytes[byteIndex] >> 4) & 0x0F;
- } else {
- digit = bytes[byteIndex] & 0x0F;
- }
-
- if (digit > 9) {
- throw new IllegalArgumentException("Invalid BCD digit: " + digit);
- }
-
- value = value.multiply(BigInteger.TEN).add(BigInteger.valueOf(digit));
+ value = value.multiply(BigInteger.TEN).add(BigInteger.valueOf(readDigit(bytes, firstNibble + i)));
}
return value;
}
diff --git a/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/BcdBitAlignmentTest.java b/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/BcdBitAlignmentTest.java
new file mode 100644
index 0000000000..9f24638d4e
--- /dev/null
+++ b/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/BcdBitAlignmentTest.java
@@ -0,0 +1,201 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.spi.buffers.bytebased;
+
+import org.apache.plc4x.java.spi.buffers.bytebased.encoding.EncodingBCD;
+import org.junit.jupiter.api.Test;
+
+import java.math.BigInteger;
+
+import static org.junit.jupiter.api.Assertions.*;
+
+/**
+ * End-to-end regression tests for BCD fields whose width is not a multiple of 8.
+ * <p>
+ * {@code readBits} right-aligns a partial read and {@code writeBits} consumes the value from the same
+ * right-aligned bit, but {@code EncodingBCD} used to pack and unpack its digits starting at the high
+ * nibble of byte 0. The two conventions agree only when {@code numBits % 8 == 0}, so every field with
+ * an odd number of BCD digits silently lost its most significant digit on read and wrote a shifted
+ * value on serialize. The concrete victim was the S7 {@code DateAndTime} tail - a 12 bit millisecond
+ * field followed by a 4 bit day-of-week field - which is reproduced verbatim below.
+ */
+class BcdBitAlignmentTest {
+
+ /**
+ * The S7 DateAndTime field layout for 2024-08-19 14:35:07.123 with a day-of-week nibble of 4.
+ * Hand-built rather than captured - 2024-08-19 is a Monday, whose Siemens nibble is 2 - because
+ * what matters here is only that the trailing nibble differs from the millisecond digits next to
+ * it, so a misaligned read or write cannot accidentally produce the expected bytes.
+ */
+ private static final byte[] S7_DATE_AND_TIME =
+ {0x24, 0x08, 0x19, 0x14, 0x35, 0x07, 0x12, 0x34};
+
+ @Test
+ void readS7DateAndTimeTail() throws Exception {
+ ReadBufferByteBased buffer =
+ new ReadBufferByteBased(S7_DATE_AND_TIME, EncodingBCD.optionEncodingBCD());
+
+ assertEquals((short) 24, buffer.readUnsignedShort(8)); // year
+ assertEquals((short) 8, buffer.readUnsignedShort(8)); // month
+ assertEquals((short) 19, buffer.readUnsignedShort(8)); // day
+ assertEquals((short) 14, buffer.readUnsignedShort(8)); // hour
+ assertEquals((short) 35, buffer.readUnsignedShort(8)); // minutes
+ assertEquals((short) 7, buffer.readUnsignedShort(8)); // seconds
+ // The 12 bit millisecond field: 0x123 -> 123, NOT 12 (the pre-fix result, which dropped the
+ // trailing digit because the leading padding nibble was read as the first digit).
+ assertEquals((short) 123, buffer.readUnsignedShort(12));
+ // The 4 bit day-of-week nibble: 4, NOT 0 (the pre-fix result read the high nibble).
+ assertEquals((short) 4, buffer.readUnsignedShort(4));
+ assertEquals(0, buffer.getRemainingBits());
+ }
+
+ @Test
+ void writeS7DateAndTimeTail() throws Exception {
+ byte[] data = new byte[8];
+ WriteBufferByteBased buffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+
+ buffer.writeUnsignedShort(8, (short) 24);
+ buffer.writeUnsignedShort(8, (short) 8);
+ buffer.writeUnsignedShort(8, (short) 19);
+ buffer.writeUnsignedShort(8, (short) 14);
+ buffer.writeUnsignedShort(8, (short) 35);
+ buffer.writeUnsignedShort(8, (short) 7);
+ buffer.writeUnsignedShort(12, (short) 123);
+ buffer.writeUnsignedShort(4, (short) 4);
+
+ // Pre-fix this produced ... 0x23 0x00: writeBits emitted only the low 12 bits of the
+ // left-aligned {0x12, 0x30} the encoding handed it.
+ assertArrayEquals(S7_DATE_AND_TIME, data);
+ }
+
+ @Test
+ void twelveBitFieldRoundTripsEveryValue() throws Exception {
+ for (short value = 0; value <= 999; value++) {
+ byte[] data = new byte[2];
+ WriteBufferByteBased writeBuffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ writeBuffer.writeUnsignedShort(12, value);
+
+ ReadBufferByteBased readBuffer =
+ new ReadBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ assertEquals(value, readBuffer.readUnsignedShort(12), "12 bit BCD round trip of " + value);
+ }
+ }
+
+ /**
+ * Note the two distinct alignments at play: within the {@code byte[]} the encoding exchanges with
+ * the buffer the field is RIGHT-aligned (a lone digit sits in the low nibble), but the buffer then
+ * appends those bits to the STREAM at the current bit position - so a 4 bit field written at bit 0
+ * occupies the high nibble of byte 0. Both have to be right for the round trip to work.
+ */
+ @Test
+ void fourBitFieldRoundTripsEveryValue() throws Exception {
+ for (short value = 0; value <= 9; value++) {
+ byte[] data = new byte[1];
+ WriteBufferByteBased writeBuffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ writeBuffer.writeUnsignedShort(4, value);
+ assertArrayEquals(new byte[]{(byte) (value << 4)}, data,
+ "a 4 bit field written at bit 0 occupies the high nibble of the stream");
+
+ ReadBufferByteBased readBuffer =
+ new ReadBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ assertEquals(value, readBuffer.readUnsignedShort(4), "4 bit BCD round trip of " + value);
+ }
+ }
+
+ /**
+ * The same 4 bit field read/written at bit 4 - i.e. in the LOW nibble, which is where the S7
+ * day-of-week nibble actually lives. This is the case the pre-fix code got wrong.
+ */
+ @Test
+ void fourBitFieldAtOddNibbleOffsetRoundTripsEveryValue() throws Exception {
+ for (short value = 0; value <= 9; value++) {
+ byte[] data = new byte[1];
+ WriteBufferByteBased writeBuffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ writeBuffer.writeUnsignedShort(4, (short) 0);
+ writeBuffer.writeUnsignedShort(4, value);
+ assertArrayEquals(new byte[]{(byte) value}, data,
+ "a 4 bit field written at bit 4 occupies the low nibble");
+
+ ReadBufferByteBased readBuffer =
+ new ReadBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ assertEquals((short) 0, readBuffer.readUnsignedShort(4));
+ assertEquals(value, readBuffer.readUnsignedShort(4), "4 bit BCD round trip of " + value);
+ }
+ }
+
+ /** Two consecutive 4 bit BCD nibbles must land in the high then the low nibble of one byte. */
+ @Test
+ void consecutiveNibblesPackInOrder() throws Exception {
+ byte[] data = new byte[1];
+ WriteBufferByteBased writeBuffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ writeBuffer.writeUnsignedShort(4, (short) 7);
+ writeBuffer.writeUnsignedShort(4, (short) 2);
+ assertArrayEquals(new byte[]{0x72}, data);
+
+ ReadBufferByteBased readBuffer =
+ new ReadBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ assertEquals((short) 7, readBuffer.readUnsignedShort(4));
+ assertEquals((short) 2, readBuffer.readUnsignedShort(4));
+ }
+
+ /** A 20 bit / 5 digit field exercises the offset across more than two bytes. */
+ @Test
+ void twentyBitFieldRoundTrips() throws Exception {
+ byte[] data = new byte[3];
+ WriteBufferByteBased writeBuffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ writeBuffer.writeUnsignedInt(20, 12345);
+ // Written at bit 0, so the 20 bits occupy bits 0..19 of the stream (the trailing nibble of
+ // the 3 byte array stays untouched).
+ assertArrayEquals(new byte[]{0x12, 0x34, 0x50}, data);
+
+ ReadBufferByteBased readBuffer =
+ new ReadBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ assertEquals(12345, readBuffer.readUnsignedInt(20));
+ }
+
+ /** The long and BigInteger code paths share the same packing, so pin them too. */
+ @Test
+ void oddDigitLongAndBigIntegerFieldsRoundTrip() throws Exception {
+ byte[] data = new byte[5];
+ WriteBufferByteBased writeBuffer =
+ new WriteBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ writeBuffer.writeUnsignedLong(36, 123456789L);
+ assertArrayEquals(new byte[]{0x12, 0x34, 0x56, 0x78, (byte) 0x90}, data);
+
+ ReadBufferByteBased readBuffer =
+ new ReadBufferByteBased(data, EncodingBCD.optionEncodingBCD());
+ assertEquals(123456789L, readBuffer.readUnsignedLong(36));
+
+ byte[] bigData = new byte[5];
+ WriteBufferByteBased bigWriteBuffer =
+ new WriteBufferByteBased(bigData, EncodingBCD.optionEncodingBCD());
+ bigWriteBuffer.writeUnsignedBigInteger(36, BigInteger.valueOf(987654321L));
+ assertArrayEquals(new byte[]{(byte) 0x98, 0x76, 0x54, 0x32, 0x10}, bigData);
+
+ ReadBufferByteBased bigReadBuffer =
+ new ReadBufferByteBased(bigData, EncodingBCD.optionEncodingBCD());
+ assertEquals(BigInteger.valueOf(987654321L), bigReadBuffer.readUnsignedBigInteger(36));
+ }
+}
diff --git a/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCDTest.java b/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCDTest.java
index 6bdf33e659..0d7cfcfcbb 100644
--- a/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCDTest.java
+++ b/plc4j/spi/buffers/byte/src/test/java/org/apache/plc4x/java/spi/buffers/bytebased/encoding/EncodingBCDTest.java
@@ -165,14 +165,27 @@ class EncodingBCDTest extends BaseEncodingDefaultTest {
@Test
void decodeWithInsufficientBytes() {
+ // Same guard and same message as EncodingUnsignedBinary, instead of the raw
+ // ArrayIndexOutOfBoundsException the nibble unpacking would otherwise throw.
byte[] encoded1 = new byte[0];
- assertThrows(ArrayIndexOutOfBoundsException.class, () -> encoding.decodeByte(8, encoded1));
+ IllegalArgumentException e1 =
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeByte(8, encoded1));
+ assertEquals("Expected at least 1 bytes", e1.getMessage());
byte[] encoded2 = new byte[1]; // Need 2 bytes for 16 bits
- assertThrows(ArrayIndexOutOfBoundsException.class, () -> encoding.decodeShort(16, encoded2));
+ IllegalArgumentException e2 =
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeShort(16, encoded2));
+ assertEquals("Expected at least 2 bytes", e2.getMessage());
byte[] encoded3 = new byte[3]; // Need 4 bytes for 32 bits
- assertThrows(ArrayIndexOutOfBoundsException.class, () -> encoding.decodeInt(32, encoded3));
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeInt(32, encoded3));
+
+ byte[] encoded4 = new byte[1]; // Need 2 bytes for a 12 bit (odd digit count) field
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeInt(12, encoded4));
+
+ byte[] encoded5 = new byte[7];
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeLong(64, encoded5));
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeBigInteger(64, encoded5));
}
@Test
@@ -190,4 +203,80 @@ class EncodingBCDTest extends BaseEncodingDefaultTest {
// TODO: Update this list of unsupported operations.
//assertUnsupportedOperations(encoding);
}
-}
\ No newline at end of file
+
+ // ---- Bit alignment (see the class javadoc of EncodingBCD) ----------------------------------
+ // The byte-based buffers hand partial fields to the encodings RIGHT-aligned: readBits starts
+ // filling the result at bit (8 - (numBits % 8)) % 8 and writeBits consumes it from that same
+ // bit. BCD used to pack/unpack from the high nibble of byte 0 regardless, which silently
+ // corrupted every field with an odd number of digits (the S7 DateAndTime 12 bit millisecond
+ // and 4 bit day-of-week fields).
+
+ @Test
+ void encodeOddDigitCountIsRightAligned() {
+ // 3 digits in a 12 bit field -> the leading nibble of byte 0 is padding.
+ assertArrayEquals(new byte[]{0x01, 0x23}, encoding.encodeInt(12, 123));
+ assertArrayEquals(new byte[]{0x00, 0x07}, encoding.encodeInt(12, 7));
+ assertArrayEquals(new byte[]{0x09, (byte) 0x99}, encoding.encodeInt(12, 999));
+ // 1 digit in a 4 bit field -> the digit is the LOW nibble.
+ assertArrayEquals(new byte[]{0x04}, encoding.encodeInt(4, 4));
+ assertArrayEquals(new byte[]{0x00}, encoding.encodeInt(4, 0));
+ assertArrayEquals(new byte[]{0x09}, encoding.encodeInt(4, 9));
+ // 5 digits in a 20 bit field.
+ assertArrayEquals(new byte[]{0x01, 0x23, 0x45}, encoding.encodeInt(20, 12345));
+ // Same for the other widths sharing the packing code.
+ assertArrayEquals(new byte[]{0x01, 0x23}, encoding.encodeShort(12, (short) 123));
+ assertArrayEquals(new byte[]{0x04}, encoding.encodeByte(4, (byte) 4));
+ assertArrayEquals(new byte[]{0x01, 0x23}, encoding.encodeLong(12, 123L));
+ assertArrayEquals(new byte[]{0x01, 0x23}, encoding.encodeBigInteger(12, BigInteger.valueOf(123)));
+ }
+
+ @Test
+ void decodeOddDigitCountIsRightAligned() {
+ assertEquals(123, encoding.decodeInt(12, new byte[]{0x01, 0x23}));
+ assertEquals(7, encoding.decodeInt(12, new byte[]{0x00, 0x07}));
+ assertEquals(999, encoding.decodeInt(12, new byte[]{0x09, (byte) 0x99}));
+ assertEquals(4, encoding.decodeInt(4, new byte[]{0x04}));
+ assertEquals(9, encoding.decodeInt(4, new byte[]{0x09}));
+ assertEquals(12345, encoding.decodeInt(20, new byte[]{0x01, 0x23, 0x45}));
+ assertEquals((short) 123, encoding.decodeShort(12, new byte[]{0x01, 0x23}));
+ assertEquals((byte) 4, encoding.decodeByte(4, new byte[]{0x04}));
+ assertEquals(123L, encoding.decodeLong(12, new byte[]{0x01, 0x23}));
+ assertEquals(BigInteger.valueOf(123), encoding.decodeBigInteger(12, new byte[]{0x01, 0x23}));
+ }
+
+ @Test
+ void evenDigitCountIsUnchanged() {
+ // numBits % 8 == 0 means offset 0, i.e. exactly the previous behaviour - these must not move.
+ assertArrayEquals(new byte[]{0x12}, encoding.encodeInt(8, 12));
+ assertArrayEquals(new byte[]{0x12, 0x34}, encoding.encodeInt(16, 1234));
+ assertArrayEquals(new byte[]{0x12, 0x34, 0x56, 0x78}, encoding.encodeInt(32, 12345678));
+ assertEquals(12, encoding.decodeInt(8, new byte[]{0x12}));
+ assertEquals(1234, encoding.decodeInt(16, new byte[]{0x12, 0x34}));
+ assertEquals(12345678, encoding.decodeInt(32, new byte[]{0x12, 0x34, 0x56, 0x78}));
+ }
+
+ @Test
+ void oddDigitCountRoundTripsEveryValue() {
+ for (int value = 0; value <= 9; value++) {
+ assertArrayEquals(new byte[]{(byte) value}, encoding.encodeInt(4, value));
+ assertEquals(value, encoding.decodeInt(4, encoding.encodeInt(4, value)));
+ }
+ for (int value = 0; value <= 999; value++) {
+ assertEquals(value, encoding.decodeInt(12, encoding.encodeInt(12, value)));
+ }
+ }
+
+ @Test
+ void decodeRejectsInvalidNibblesInOddDigitFields() {
+ // The padding nibble is ignored, but every DIGIT nibble is still validated.
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeInt(4, new byte[]{0x0A}));
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeInt(12, new byte[]{0x0A, 0x23}));
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeInt(12, new byte[]{0x01, (byte) 0xB3}));
+ assertThrows(IllegalArgumentException.class, () -> encoding.decodeInt(12, new byte[]{0x01, 0x2C}));
+ // ... and the padding nibble genuinely is ignored (readBits leaves it zeroed, but a caller
+ // handing us a dirty buffer must not be able to change the decoded value).
+ assertEquals(4, encoding.decodeInt(4, new byte[]{(byte) 0xF4}));
+ assertEquals(123, encoding.decodeInt(12, new byte[]{(byte) 0xF1, 0x23}));
+ }
+
+}