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/24 14:48:34 UTC
(plc4x) 18/24: fix(iec-60870): acknowledge how far we have read, not what the station read
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 de0d5e14c60e9e29827b073e824e50865cca9dc8
Author: Sebastian Rühl <sr...@apache.org>
AuthorDate: Mon Aug 24 14:47:25 2026 +0200
fix(iec-60870): acknowledge how far we have read, not what the station read
An S-format acknowledgement tells the station how far we have taken its telemetry
in, so it can release what it was holding in case it needed to send it again. The
driver built that number from the received frame's receive sequence number plus
one - which is the station telling us how much of *our* traffic it had read. A
different number entirely, and one that says nothing about what we have taken in.
It also neither wrapped at fifteen bits nor shifted the value into the upper bits
of the control field where the standard puts it.
The send sequence number was not in the model to build it from. It shares the two
bytes the frame formats are told apart by, and the note left in the mspec had the
right idea and did not work: a virtual field referencing the parent's discriminator
compiles as a getter but not inside the generated parse, where that discriminator is
not in scope. Passing it in as a parser argument puts it there. Both sequence numbers
also gained a virtual for their decoded value, since on the wire each sits one bit up.
The wire format is untouched - virtual fields serialize nothing, and all 128 APDUs
in the parser-serializer testsuite still round-trip byte for byte. What changes is
what we send: the number is now one past what the station last sent us, wrapped, in
the bits the standard names. Recorded under incompatible changes, because a station
that reads our acknowledgements will see different values.
The mspec's other note - re-reading the receive sequence number as a 15-bit field -
is deliberately left alone. It would move where the serializer puts bytes, and the
low bit of a little-endian 16-bit value is not where a bit-level read of fifteen bits
would leave it. The virtual says the same thing and cannot get that wrong.
---
RELEASE_NOTES | 12 ++++
.../protocols/iec608705104/readwrite/model/APDU.go | 2 +-
.../iec608705104/readwrite/model/APDUIFormat.go | 62 ++++++++++++++++--
.../iec608705104/readwrite/model/APDUSFormat.go | 35 ++++++++++
.../plc4x/java/iec608705104/readwrite/APDU.java | 2 +-
.../java/iec608705104/readwrite/APDUIFormat.java | 33 +++++++++-
.../java/iec608705104/readwrite/APDUSFormat.java | 15 +++++
.../java/iec608705104/Iec60870Connection.java | 54 ++++++++++++++--
.../iec608705104/Iec60870SequenceNumberTest.java | 74 ++++++++++++++++++++++
.../protocols/iec608705104/iec-60870-5-104.mspec | 20 ++++--
10 files changed, 291 insertions(+), 18 deletions(-)
diff --git a/RELEASE_NOTES b/RELEASE_NOTES
index cf1e293cb6..b029d334c6 100644
--- a/RELEASE_NOTES
+++ b/RELEASE_NOTES
@@ -160,6 +160,18 @@ Incompatible changes
PlcConnectionStateChangedEvent events which have many more
state change options beyond a simple connected and disconnected
event.
+- The IEC 60870-5-104 driver now derives its S-format
+ acknowledgements from the send sequence number of the frames it
+ received, counted modulo 2^15 and encoded in the upper fifteen
+ bits of the control field, as the standard requires. It
+ previously sent the received frame's receive sequence number
+ plus one, which is the station reporting how much of our traffic
+ it had taken in - a different number, and one that said nothing
+ about how far we had read. This changes what the driver puts on
+ the wire in response to received telemetry, so a station that
+ checks acknowledgements will see different (correct) values, and
+ one that had adapted to the old behaviour may need attention.
+ The frame format itself is unchanged.
Changed Maven Coordinates
-------------------------
diff --git a/plc4go/protocols/iec608705104/readwrite/model/APDU.go b/plc4go/protocols/iec608705104/readwrite/model/APDU.go
index 9315289bd7..ae7cf6880c 100644
--- a/plc4go/protocols/iec608705104/readwrite/model/APDU.go
+++ b/plc4go/protocols/iec608705104/readwrite/model/APDU.go
@@ -446,7 +446,7 @@ func (m *_APDU) parse(ctx context.Context, readBuffer utils.ReadBuffer) (__aPDU
return nil, errors.Wrap(err, "Error parsing sub-type APDUSFormat for type-switch of APDU")
}
case 0 == 0: // APDUIFormat
- if _child, err = new(_APDUIFormat).parse(ctx, readBuffer, m); err != nil {
+ if _child, err = new(_APDUIFormat).parse(ctx, readBuffer, m, uint16(command)); err != nil {
return nil, errors.Wrap(err, "Error parsing sub-type APDUIFormat for type-switch of APDU")
}
default:
diff --git a/plc4go/protocols/iec608705104/readwrite/model/APDUIFormat.go b/plc4go/protocols/iec608705104/readwrite/model/APDUIFormat.go
index bd1edc80ff..688809a1d9 100644
--- a/plc4go/protocols/iec608705104/readwrite/model/APDUIFormat.go
+++ b/plc4go/protocols/iec608705104/readwrite/model/APDUIFormat.go
@@ -45,13 +45,18 @@ type APDUIFormat interface {
utils.Copyable
APDU
// GetReceiveSequenceNo returns ReceiveSequenceNo (property field)
- // TODO: Fix this ...
- // [virtual uint 15 sendSequenceNo 'command >> 1']
- // TODO: Shift this right by one bit to make it an uint 15
GetReceiveSequenceNo() uint16
// GetAsdu returns Asdu (property field)
// Payload
GetAsdu() ASDU
+ // GetSendSequenceNumber returns SendSequenceNumber (virtual field)
+ // The send sequence number is in the command field: an I-format frame is one whose
+ // lowest bit is clear, and the number occupies the fifteen bits above it. Virtual
+ // rather than read, because those same two bytes are what tells the formats apart and
+ // are already consumed above - passed in so it is in scope where the parse happens.
+ GetSendSequenceNumber() uint16
+ // GetReceiveSequenceNumber returns ReceiveSequenceNumber (virtual field)
+ GetReceiveSequenceNumber() uint16
// IsAPDUIFormat is a marker method to prevent unintentional type checks (interfaces of same signature)
IsAPDUIFormat()
// CreateBuilder creates a APDUIFormatBuilder
@@ -227,6 +232,27 @@ func (m *_APDUIFormat) GetAsdu() ASDU {
return m.Asdu
}
+///////////////////////
+///////////////////////
+///////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////
+/////////////////////// Accessors for virtual fields.
+///////////////////////
+
+func (m *_APDUIFormat) GetSendSequenceNumber() uint16 {
+ ctx := context.Background()
+ _ = ctx
+ return uint16(m.GetCommand() >> uint16(1))
+}
+
+func (m *_APDUIFormat) GetReceiveSequenceNumber() uint16 {
+ ctx := context.Background()
+ _ = ctx
+ return uint16(m.GetReceiveSequenceNo() >> uint16(1))
+}
+
///////////////////////
///////////////////////
///////////////////////////////////////////////////////////
@@ -250,9 +276,13 @@ func (m *_APDUIFormat) GetPlx4xTypeName() string {
func (m *_APDUIFormat) GetLengthInBits(ctx context.Context) uint64 {
lengthInBits := uint64(m.APDUContract.(*_APDU).getLengthInBits(ctx))
+ // A virtual field doesn't have any in- or output.
+
// Simple field (receiveSequenceNo)
lengthInBits += 16
+ // A virtual field doesn't have any in- or output.
+
// Simple field (asdu)
lengthInBits += m.Asdu.GetLengthInBits(ctx)
@@ -263,7 +293,7 @@ func (m *_APDUIFormat) GetLengthInBytes(ctx context.Context) uint64 {
return m.GetLengthInBits(ctx) / 8
}
-func (m *_APDUIFormat) parse(ctx context.Context, readBuffer utils.ReadBuffer, parent *_APDU) (__aPDUIFormat APDUIFormat, err error) {
+func (m *_APDUIFormat) parse(ctx context.Context, readBuffer utils.ReadBuffer, parent *_APDU, command uint16) (__aPDUIFormat APDUIFormat, err error) {
m.APDUContract = parent
parent._SubType = m
positionAware := readBuffer
@@ -274,12 +304,24 @@ func (m *_APDUIFormat) parse(ctx context.Context, readBuffer utils.ReadBuffer, p
currentPos := positionAware.GetPos()
_ = currentPos
+ sendSequenceNumber, err := ReadVirtualField[uint16](ctx, "sendSequenceNumber", (*uint16)(nil), command>>uint16(1), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian))
+ if err != nil {
+ return nil, errors.Wrap(err, fmt.Sprintf("Error parsing 'sendSequenceNumber' field"))
+ }
+ _ = sendSequenceNumber
+
receiveSequenceNo, err := ReadSimpleField(ctx, "receiveSequenceNo", ReadUnsignedShort(readBuffer, uint8(16)), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian))
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("Error parsing 'receiveSequenceNo' field"))
}
m.ReceiveSequenceNo = receiveSequenceNo
+ receiveSequenceNumber, err := ReadVirtualField[uint16](ctx, "receiveSequenceNumber", (*uint16)(nil), receiveSequenceNo>>uint16(1), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian))
+ if err != nil {
+ return nil, errors.Wrap(err, fmt.Sprintf("Error parsing 'receiveSequenceNumber' field"))
+ }
+ _ = receiveSequenceNumber
+
asdu, err := ReadSimpleField[ASDU](ctx, "asdu", ReadComplex[ASDU](ASDUParseWithBuffer, readBuffer), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian))
if err != nil {
return nil, errors.Wrap(err, fmt.Sprintf("Error parsing 'asdu' field"))
@@ -310,10 +352,22 @@ func (m *_APDUIFormat) SerializeWithWriteBuffer(ctx context.Context, writeBuffer
if pushErr := writeBuffer.PushContext("APDUIFormat"); pushErr != nil {
return errors.Wrap(pushErr, "Error pushing for APDUIFormat")
}
+ // Virtual field
+ sendSequenceNumber := m.GetSendSequenceNumber()
+ _ = sendSequenceNumber
+ if _sendSequenceNumberErr := writeBuffer.WriteVirtual(ctx, "sendSequenceNumber", m.GetSendSequenceNumber()); _sendSequenceNumberErr != nil {
+ return errors.Wrap(_sendSequenceNumberErr, "Error serializing 'sendSequenceNumber' field")
+ }
if err := WriteSimpleField[uint16](ctx, "receiveSequenceNo", m.GetReceiveSequenceNo(), WriteUnsignedShort(writeBuffer, 16), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian)); err != nil {
return errors.Wrap(err, "Error serializing 'receiveSequenceNo' field")
}
+ // Virtual field
+ receiveSequenceNumber := m.GetReceiveSequenceNumber()
+ _ = receiveSequenceNumber
+ if _receiveSequenceNumberErr := writeBuffer.WriteVirtual(ctx, "receiveSequenceNumber", m.GetReceiveSequenceNumber()); _receiveSequenceNumberErr != nil {
+ return errors.Wrap(_receiveSequenceNumberErr, "Error serializing 'receiveSequenceNumber' field")
+ }
if err := WriteSimpleField[ASDU](ctx, "asdu", m.GetAsdu(), WriteComplex[ASDU](writeBuffer), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian)); err != nil {
return errors.Wrap(err, "Error serializing 'asdu' field")
diff --git a/plc4go/protocols/iec608705104/readwrite/model/APDUSFormat.go b/plc4go/protocols/iec608705104/readwrite/model/APDUSFormat.go
index 6dadb276f9..d5bc0dd386 100644
--- a/plc4go/protocols/iec608705104/readwrite/model/APDUSFormat.go
+++ b/plc4go/protocols/iec608705104/readwrite/model/APDUSFormat.go
@@ -45,7 +45,13 @@ type APDUSFormat interface {
utils.Copyable
APDU
// GetReceiveSequenceNo returns ReceiveSequenceNo (property field)
+ // Both control fields carry their sequence number in the upper fifteen bits, with the
+ // lowest bit reserved to tell the frame formats apart. The number is therefore the
+ // field shifted down by one, and the field stays as it is on the wire so that writing
+ // one back out needs no special case.
GetReceiveSequenceNo() uint16
+ // GetReceiveSequenceNumber returns ReceiveSequenceNumber (virtual field)
+ GetReceiveSequenceNumber() uint16
// IsAPDUSFormat is a marker method to prevent unintentional type checks (interfaces of same signature)
IsAPDUSFormat()
// CreateBuilder creates a APDUSFormatBuilder
@@ -190,6 +196,21 @@ func (m *_APDUSFormat) GetReceiveSequenceNo() uint16 {
return m.ReceiveSequenceNo
}
+///////////////////////
+///////////////////////
+///////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////
+///////////////////////////////////////////////////////////
+/////////////////////// Accessors for virtual fields.
+///////////////////////
+
+func (m *_APDUSFormat) GetReceiveSequenceNumber() uint16 {
+ ctx := context.Background()
+ _ = ctx
+ return uint16(m.GetReceiveSequenceNo() >> uint16(1))
+}
+
///////////////////////
///////////////////////
///////////////////////////////////////////////////////////
@@ -216,6 +237,8 @@ func (m *_APDUSFormat) GetLengthInBits(ctx context.Context) uint64 {
// Simple field (receiveSequenceNo)
lengthInBits += 16
+ // A virtual field doesn't have any in- or output.
+
return lengthInBits
}
@@ -240,6 +263,12 @@ func (m *_APDUSFormat) parse(ctx context.Context, readBuffer utils.ReadBuffer, p
}
m.ReceiveSequenceNo = receiveSequenceNo
+ receiveSequenceNumber, err := ReadVirtualField[uint16](ctx, "receiveSequenceNumber", (*uint16)(nil), receiveSequenceNo>>uint16(1), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian))
+ if err != nil {
+ return nil, errors.Wrap(err, fmt.Sprintf("Error parsing 'receiveSequenceNumber' field"))
+ }
+ _ = receiveSequenceNumber
+
if closeErr := readBuffer.CloseContext("APDUSFormat"); closeErr != nil {
return nil, errors.Wrap(closeErr, "Error closing for APDUSFormat")
}
@@ -268,6 +297,12 @@ func (m *_APDUSFormat) SerializeWithWriteBuffer(ctx context.Context, writeBuffer
if err := WriteSimpleField[uint16](ctx, "receiveSequenceNo", m.GetReceiveSequenceNo(), WriteUnsignedShort(writeBuffer, 16), codegen.WithEncoding("UTF8"), codegen.WithByteOrder(binary.LittleEndian)); err != nil {
return errors.Wrap(err, "Error serializing 'receiveSequenceNo' field")
}
+ // Virtual field
+ receiveSequenceNumber := m.GetReceiveSequenceNumber()
+ _ = receiveSequenceNumber
+ if _receiveSequenceNumberErr := writeBuffer.WriteVirtual(ctx, "receiveSequenceNumber", m.GetReceiveSequenceNumber()); _receiveSequenceNumberErr != nil {
+ return errors.Wrap(_receiveSequenceNumberErr, "Error serializing 'receiveSequenceNumber' field")
+ }
if popErr := writeBuffer.PopContext("APDUSFormat"); popErr != nil {
return errors.Wrap(popErr, "Error popping for APDUSFormat")
diff --git a/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDU.java b/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDU.java
index ae87a00f00..a073aa2a73 100644
--- a/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDU.java
+++ b/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDU.java
@@ -87,7 +87,7 @@ public abstract class APDU implements Message {
} else if (EvaluationHelper.equals(command, (int) (0x01))) {
builder = APDUSFormat.staticParseAPDUBuilder(readBuffer);
} else if (true) {
- builder = APDUIFormat.staticParseAPDUBuilder(readBuffer);
+ builder = APDUIFormat.staticParseAPDUBuilder(readBuffer, command);
}
if (builder == null) {
throw new BufferException("Unsupported case for discriminated type parameters parameters [command]");
diff --git a/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUIFormat.java b/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUIFormat.java
index c2c597d86d..d2b74e83dd 100644
--- a/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUIFormat.java
+++ b/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUIFormat.java
@@ -56,13 +56,34 @@ public class APDUIFormat extends APDU implements Message {
return asdu;
}
- public static APDUBuilder staticParseAPDUBuilder(ReadBuffer readBuffer) throws BufferException {
+ /**
+ * Virtual field sendSequenceNumber
+ */
+ public short getSendSequenceNumber() {
+ return (short) ((command) >> (1));
+ }
+
+ /**
+ * Virtual field receiveSequenceNumber
+ */
+ public short getReceiveSequenceNumber() {
+ return (short) ((receiveSequenceNo) >> (1));
+ }
+
+ public static APDUBuilder staticParseAPDUBuilder(ReadBuffer readBuffer, int command) throws
+ BufferException {
readBuffer.pushContext(WithOption.WithName("APDUIFormat"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
int startPos = readBuffer.getPositionInBits();
boolean _lastItem = ThreadLocalHelper.lastItemThreadLocal.get();
+ // Virtual Field: sendSequenceNumber (doesn't parse anything, just makes the value available)
+ short sendSequenceNumber = FieldReaderFactory.readVirtualField(short.class, (command) >> (1), WithOption.WithName("sendSequenceNumber"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+
// Simple Field: receiveSequenceNo
int receiveSequenceNo = FieldReaderFactory.readSimpleField(DataReaderFactory.readUnsignedInt(readBuffer, 16), WithOption.WithName("receiveSequenceNo"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+ // Virtual Field: receiveSequenceNumber (doesn't parse anything, just makes the value available)
+ short receiveSequenceNumber = FieldReaderFactory.readVirtualField(short.class, (receiveSequenceNo) >> (1), WithOption.WithName("receiveSequenceNumber"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+
// Simple Field: asdu
ASDU asdu = FieldReaderFactory.readSimpleField(DataReaderFactory.readComplex(() -> DataReaderFactory.castToDeclaredType(ASDU.class, ASDU.staticParse(readBuffer)), readBuffer), WithOption.WithName("asdu"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
@@ -74,9 +95,15 @@ public class APDUIFormat extends APDU implements Message {
writeBuffer.pushContext(WithOption.WithName("APDUIFormat"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
int startPos = writeBuffer.getPositionInBits();
boolean _lastItem = ThreadLocalHelper.lastItemThreadLocal.get();
+ // Virtual Field: sendSequenceNumber (doesn't serialize anything, just makes the value available)
+ short sendSequenceNumber = (short) getSendSequenceNumber();
+
// Simple Field: receiveSequenceNo
FieldWriterFactory.writeSimpleField((int) receiveSequenceNo, DataWriterFactory.writeUnsignedInt(writeBuffer, 16), WithOption.WithName("receiveSequenceNo"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+ // Virtual Field: receiveSequenceNumber (doesn't serialize anything, just makes the value available)
+ short receiveSequenceNumber = (short) getReceiveSequenceNumber();
+
// Simple Field: asdu
FieldWriterFactory.writeSimpleField((ASDU) asdu, DataWriterFactory.writeComplex(writeBuffer), WithOption.WithName("asdu"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
@@ -93,9 +120,13 @@ public class APDUIFormat extends APDU implements Message {
int lengthInBits = super.getLengthInBits();
APDUIFormat _value = this;
boolean _lastItem = ThreadLocalHelper.lastItemThreadLocal.get();
+ // Virtual Field: sendSequenceNumber (doesn't produce any output, just makes the value available)
+
// Simple Field: receiveSequenceNo
lengthInBits += 16;
+ // Virtual Field: receiveSequenceNumber (doesn't produce any output, just makes the value available)
+
// Simple Field: asdu
lengthInBits += asdu.getLengthInBits();
diff --git a/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUSFormat.java b/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUSFormat.java
index 5a25e44a32..0224c78b68 100644
--- a/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUSFormat.java
+++ b/plc4j/drivers/iec-60870/src/main/generated/org/apache/plc4x/java/iec608705104/readwrite/APDUSFormat.java
@@ -46,6 +46,13 @@ public class APDUSFormat extends APDU implements Message {
return receiveSequenceNo;
}
+ /**
+ * Virtual field receiveSequenceNumber
+ */
+ public short getReceiveSequenceNumber() {
+ return (short) ((receiveSequenceNo) >> (1));
+ }
+
public static APDUBuilder staticParseAPDUBuilder(ReadBuffer readBuffer) throws BufferException {
readBuffer.pushContext(WithOption.WithName("APDUSFormat"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
int startPos = readBuffer.getPositionInBits();
@@ -53,6 +60,9 @@ public class APDUSFormat extends APDU implements Message {
// Simple Field: receiveSequenceNo
int receiveSequenceNo = FieldReaderFactory.readSimpleField(DataReaderFactory.readUnsignedInt(readBuffer, 16), WithOption.WithName("receiveSequenceNo"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+ // Virtual Field: receiveSequenceNumber (doesn't parse anything, just makes the value available)
+ short receiveSequenceNumber = FieldReaderFactory.readVirtualField(short.class, (receiveSequenceNo) >> (1), WithOption.WithName("receiveSequenceNumber"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+
readBuffer.popContext();
return new APDUBuilderImpl(receiveSequenceNo);
}
@@ -64,6 +74,9 @@ public class APDUSFormat extends APDU implements Message {
// Simple Field: receiveSequenceNo
FieldWriterFactory.writeSimpleField((int) receiveSequenceNo, DataWriterFactory.writeUnsignedInt(writeBuffer, 16), WithOption.WithName("receiveSequenceNo"), WithOption.WithFloatEncoding("IEEE754"), WithOption.WithSignedIntegerEncoding("twos-complement"), WithOption.WithUnsignedIntegerEncoding("unsigned-binary"), WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"), WithOption.WithStringEncoding("UTF8"));
+ // Virtual Field: receiveSequenceNumber (doesn't serialize anything, just makes the value available)
+ short receiveSequenceNumber = (short) getReceiveSequenceNumber();
+
writeBuffer.popContext();
}
@@ -80,6 +93,8 @@ public class APDUSFormat extends APDU implements Message {
// Simple Field: receiveSequenceNo
lengthInBits += 16;
+ // Virtual Field: receiveSequenceNumber (doesn't produce any output, just makes the value available)
+
return lengthInBits;
}
diff --git a/plc4j/drivers/iec-60870/src/main/java/org/apache/plc4x/java/iec608705104/Iec60870Connection.java b/plc4j/drivers/iec-60870/src/main/java/org/apache/plc4x/java/iec608705104/Iec60870Connection.java
index 275d518f9c..4b599401cf 100644
--- a/plc4j/drivers/iec-60870/src/main/java/org/apache/plc4x/java/iec608705104/Iec60870Connection.java
+++ b/plc4j/drivers/iec-60870/src/main/java/org/apache/plc4x/java/iec608705104/Iec60870Connection.java
@@ -92,6 +92,11 @@ public class Iec60870Connection extends ConnectionBase<Iec608705014Configuration
/** Send an S-format acknowledgement every N unconfirmed I-format frames. */
private static final int ACK_EVERY_N_FRAMES = 8;
+ /**
+ * Sequence numbers are fifteen bits wide and wrap, so they are counted modulo this.
+ */
+ private static final int SEQUENCE_NUMBER_MODULUS = 1 << 15;
+
private Iec60870MessageCodec messageCodec;
/** Single-flight pending response future for the handshake. */
@@ -102,6 +107,13 @@ public class Iec60870Connection extends ConnectionBase<Iec608705014Configuration
private volatile boolean handshakeComplete = false;
private int unconfirmedPackets;
+ /**
+ * V(R) - the sequence number we expect the station to send next, which is one past the last one
+ * it did send. This is what an acknowledgement carries: it tells the station how far we have
+ * read, so it can release what it was holding in case it had to send it again.
+ */
+ private int expectedSendSequenceNumber;
+
public Iec60870Connection(Iec608705014Configuration configuration,
TransportInstance<?> transportInstance,
AuditLog auditLog) {
@@ -266,13 +278,14 @@ public class Iec60870Connection extends ConnectionBase<Iec608705014Configuration
// Real telemetry: I-format ASDUs.
if (apdu instanceof APDUIFormat apduiFormat) {
+ // One past what the station just sent, wrapped - that is what we have read up to, and
+ // what an acknowledgement is for. It used to be built from the frame's *receive*
+ // sequence number, which is the station telling us how much of ours it had read: a
+ // different number entirely, and one that says nothing about what we have taken in.
+ expectedSendSequenceNumber = nextExpectedAfter(apduiFormat.getSendSequenceNumber());
unconfirmedPackets++;
if (unconfirmedPackets >= ACK_EVERY_N_FRAMES) {
- try {
- messageCodec.send(new APDUSFormat(0x01, apduiFormat.getReceiveSequenceNo() + 1));
- } catch (MessageCodecException e) {
- LOGGER.error("Failed to send S-format acknowledgement", e);
- }
+ sendAcknowledgement();
unconfirmedPackets = 0;
}
processAsdu(apduiFormat.getAsdu());
@@ -325,6 +338,37 @@ public class Iec60870Connection extends ConnectionBase<Iec608705014Configuration
// ASDU → PlcSubscriptionEvent dispatch
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
+ /**
+ * @return the sequence number we expect next after receiving {@code sendSequenceNumber}, which
+ * is one past it - fifteen bits wide, so it wraps rather than growing.
+ */
+ static int nextExpectedAfter(int sendSequenceNumber) {
+ return (sendSequenceNumber + 1) % SEQUENCE_NUMBER_MODULUS;
+ }
+
+ /**
+ * @return the control-field value carrying {@code sequenceNumber}. The number lives in the upper
+ * fifteen bits, the lowest being what tells the frame formats apart, so it goes out shifted up
+ * by one.
+ */
+ static int encodeSequenceNumber(int sequenceNumber) {
+ return (sequenceNumber % SEQUENCE_NUMBER_MODULUS) << 1;
+ }
+
+ /**
+ * Tells the station how far we have read, so it can let go of what it was keeping in case it
+ * needed to send it again.
+ */
+ private void sendAcknowledgement() {
+ try {
+ // The number sits in the upper fifteen bits of the control field, the lowest bit being
+ // what tells the frame formats apart - so it goes on the wire shifted up by one.
+ messageCodec.send(new APDUSFormat(0x01, encodeSequenceNumber(expectedSendSequenceNumber)));
+ } catch (MessageCodecException e) {
+ LOGGER.error("Failed to send S-format acknowledgement", e);
+ }
+ }
+
private void processAsdu(ASDU asdu) {
int asduAddress = asdu.getAsduAddressField();
for (InformationObject informationObject : asdu.getInformationObjects()) {
diff --git a/plc4j/drivers/iec-60870/src/test/java/org/apache/plc4x/java/iec608705104/Iec60870SequenceNumberTest.java b/plc4j/drivers/iec-60870/src/test/java/org/apache/plc4x/java/iec608705104/Iec60870SequenceNumberTest.java
new file mode 100644
index 0000000000..ae9268cd4e
--- /dev/null
+++ b/plc4j/drivers/iec-60870/src/test/java/org/apache/plc4x/java/iec608705104/Iec60870SequenceNumberTest.java
@@ -0,0 +1,74 @@
+/*
+ * 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.iec608705104;
+
+import org.junit.jupiter.api.Test;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * An acknowledgement tells the station how far we have read, so it can release what it was holding
+ * in case it had to send it again. Getting the number wrong is not a local matter: the station acts
+ * on what we send it.
+ */
+class Iec60870SequenceNumberTest {
+
+ @Test
+ void whatWeExpectNextIsOnePastWhatArrived() {
+ assertEquals(1, Iec60870Connection.nextExpectedAfter(0));
+ assertEquals(8, Iec60870Connection.nextExpectedAfter(7));
+ assertEquals(32767, Iec60870Connection.nextExpectedAfter(32766));
+ }
+
+ @Test
+ void theSequenceWrapsRatherThanGrowing() {
+ // Fifteen bits wide: past the top it starts again, it does not become a sixteen-bit number.
+ assertEquals(0, Iec60870Connection.nextExpectedAfter(32767));
+ assertEquals(1, Iec60870Connection.nextExpectedAfter(0 + 32768));
+ }
+
+ @Test
+ void theNumberGoesOnTheWireShiftedUpByOne() {
+ // The lowest bit of the control field is what marks the format, so it must stay clear.
+ assertEquals(0b10, Iec60870Connection.encodeSequenceNumber(1));
+ assertEquals(0b1110, Iec60870Connection.encodeSequenceNumber(7));
+ assertEquals(0xFFFE, Iec60870Connection.encodeSequenceNumber(32767));
+ }
+
+ @Test
+ void anEncodedSequenceNumberAlwaysFitsTheControlField() {
+ for (int sequenceNumber = 0; sequenceNumber < 32768; sequenceNumber++) {
+ int encoded = Iec60870Connection.encodeSequenceNumber(sequenceNumber);
+ assertTrue(encoded >= 0 && encoded <= 0xFFFE,
+ sequenceNumber + " encoded to " + encoded + ", which does not fit the field");
+ assertEquals(0, encoded & 1,
+ sequenceNumber + " encoded with its lowest bit set, which would change the format");
+ }
+ }
+
+ @Test
+ void theWholeCycleRoundTrips() {
+ // Every number the station can send, acknowledged and decoded back the way the peer will.
+ for (int sent = 0; sent < 32768; sent++) {
+ int encoded = Iec60870Connection.encodeSequenceNumber(Iec60870Connection.nextExpectedAfter(sent));
+ assertEquals((sent + 1) % 32768, encoded >> 1);
+ }
+ }
+}
diff --git a/protocols/iec-60870/src/main/resources/protocols/iec608705104/iec-60870-5-104.mspec b/protocols/iec-60870/src/main/resources/protocols/iec608705104/iec-60870-5-104.mspec
index 60b9cb9317..1cc50906e0 100644
--- a/protocols/iec-60870/src/main/resources/protocols/iec608705104/iec-60870-5-104.mspec
+++ b/protocols/iec-60870/src/main/resources/protocols/iec608705104/iec-60870-5-104.mspec
@@ -55,15 +55,23 @@
// S-Format Frames
['0x01' *SFormat
- [simple uint 16 receiveSequenceNo ]
+ // Both control fields carry their sequence number in the upper fifteen bits, with the
+ // lowest bit reserved to tell the frame formats apart. The number is therefore the
+ // field shifted down by one, and the field stays as it is on the wire so that writing
+ // one back out needs no special case.
+ [simple uint 16 receiveSequenceNo ]
+ [virtual uint 15 receiveSequenceNumber 'receiveSequenceNo >> 1' ]
]
// I-Format Frames (Catch-all for all other values)
- [* *IFormat
- // TODO: Fix this ...
- // [virtual uint 15 sendSequenceNo 'command >> 1']
- // TODO: Shift this right by one bit to make it an uint 15
- [simple uint 16 receiveSequenceNo ]
+ [* *IFormat (uint 16 command)
+ // The send sequence number is in the command field: an I-format frame is one whose
+ // lowest bit is clear, and the number occupies the fifteen bits above it. Virtual
+ // rather than read, because those same two bytes are what tells the formats apart and
+ // are already consumed above - passed in so it is in scope where the parse happens.
+ [virtual uint 15 sendSequenceNumber 'command >> 1' ]
+ [simple uint 16 receiveSequenceNo ]
+ [virtual uint 15 receiveSequenceNumber 'receiveSequenceNo >> 1' ]
// Payload
[simple ASDU asdu]
]