From c23e00cea1ee4fb43c6b50b25622bfec727f82d3 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 18 Jun 2026 07:54:01 +0200 Subject: [PATCH] fix(s7commplus): decode struct PValues per the real wire format (packed + normal) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit decode_pvalue_to_bytes treated every Struct (0x17) as a VLQ element count followed by that many nested PValues. Real S7-1500 structs use neither shape: the leading struct id is a fixed UInt32, and the body comes in two forms (per ValueStruct.Deserialize in the thomas-v2/S7CommPlusDriver reference): - Packed struct — system datatypes and optimized-DB struct reads, id in 0x90000000..0x9fffffff or 0x02000000..0x02ffffff: a UInt64 interface timestamp, VLQ transport flags, a VLQ element count (a second count follows when the Count2Present flag is set), then the members as one raw byte blob, returned verbatim for the caller to interpret using the struct's layout. - Normal struct — members as [VLQ key][PValue], terminated by a 0 key. Verified against a real S7-1500: reading a struct node now returns the packed member bytes, which decode to the expected scalar values (matching the individual leaf reads). Co-Authored-By: Claude Opus 4.8 --- s7/codec.py | 39 +++++++++++++++++++++++--- tests/test_s7_codec.py | 62 ++++++++++++++++++++++++++++++++++++++---- 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/s7/codec.py b/s7/codec.py index 74f94a2e..08fb0741 100644 --- a/s7/codec.py +++ b/s7/codec.py @@ -466,14 +466,45 @@ def decode_pvalue_to_bytes(data: bytes, offset: int) -> tuple[bytes, int]: consumed += length return bytes(raw), consumed elif datatype == DataType.STRUCT: - # Struct: read count, then nested PValues - count, c = decode_uint32_vlq(data, offset + consumed) - consumed += c + # Struct value. Mirrors ValueStruct.Deserialize in the C# reference driver + # (thomas-v2/S7CommPlusDriver, Core/PValue.cs). + # + # The leading struct id is a fixed UInt32 (not VLQ). Two transmission forms: + # + # * Packed struct — for system datatypes (DTL, optimized-DB structs, ...) whose + # members are sent as one opaque blob. Detected by the id falling in the ranges + # 0x90000000..0x9fffffff or 0x02000000..0x02ffffff. Layout: UInt64 interface + # timestamp, VLQ transport flags, VLQ element count (a second count follows when + # the Count2Present flag, bit 10, is set), then `count` raw bytes. We return those + # raw bytes verbatim — the caller interprets them using the struct's member layout. + # + # * Normal struct — members as [VLQ key][PValue], terminated by a key of 0. + struct_id = int.from_bytes(data[offset + consumed : offset + consumed + 4], "big") + consumed += 4 + + if (0x90000000 < struct_id < 0x9FFFFFFF) or (0x02000000 < struct_id < 0x02FFFFFF): + consumed += 8 # PackedStructInterfaceTimestamp (UInt64, fixed) + transport_flags, c = decode_uint32_vlq(data, offset + consumed) + consumed += c + count, c = decode_uint32_vlq(data, offset + consumed) + consumed += c + if transport_flags & 0x400: # Count2Present: a second count follows + count, c = decode_uint32_vlq(data, offset + consumed) + consumed += c + raw = data[offset + consumed : offset + consumed + count] + consumed += count + return bytes(raw), consumed + + # Normal struct: concatenate member values, stopping at the 0 key terminator. result = bytearray() - for _ in range(count): + key, c = decode_uint32_vlq(data, offset + consumed) + consumed += c + while key > 0: val_bytes, c = decode_pvalue_to_bytes(data, offset + consumed) consumed += c result += val_bytes + key, c = decode_uint32_vlq(data, offset + consumed) + consumed += c return bytes(result), consumed else: raise ValueError(f"Unsupported PValue datatype: {datatype:#04x}") diff --git a/tests/test_s7_codec.py b/tests/test_s7_codec.py index 2bce0de0..847cebe8 100644 --- a/tests/test_s7_codec.py +++ b/tests/test_s7_codec.py @@ -538,14 +538,64 @@ def test_wstring(self) -> None: result, consumed = decode_pvalue_to_bytes(data, 0) assert result == text - def test_struct_nested(self) -> None: - # Struct with 2 USINT elements - vlq_count = encode_uint32_vlq(2) - elem1 = bytes([0x00, DataType.USINT, 0x0A]) - elem2 = bytes([0x00, DataType.USINT, 0x14]) - data = bytes([0x00, DataType.STRUCT]) + vlq_count + elem1 + elem2 + def test_struct_normal(self) -> None: + # Normal struct: UInt32 id, then [VLQ key][PValue] members, 0-key terminated. + struct_id = struct.pack(">I", 0x00000001) + member1 = encode_uint32_vlq(1) + bytes([0x00, DataType.USINT, 0x0A]) + member2 = encode_uint32_vlq(2) + bytes([0x00, DataType.USINT, 0x14]) + terminator = encode_uint32_vlq(0) + data = bytes([0x00, DataType.STRUCT]) + struct_id + member1 + member2 + terminator result, consumed = decode_pvalue_to_bytes(data, 0) assert result == bytes([0x0A, 0x14]) + assert consumed == len(data) + + def test_struct_packed(self) -> None: + # Packed struct (system datatype): id in 0x90000000..0x9fffffff, then UInt64 interface + # timestamp, VLQ transport flags, VLQ element count, then raw member bytes returned + # verbatim. Mirrors a real S7-1500 read of a struct node with three Int members. + struct_id = struct.pack(">I", 0x91080009) + timestamp = bytes.fromhex("58936099d03e9bd4") + transport_flags = encode_uint32_vlq(0x03) + members = struct.pack(">3h", 100, 200, 300) # three Int members + count = encode_uint32_vlq(len(members)) + data = bytes([0x00, DataType.STRUCT]) + struct_id + timestamp + transport_flags + count + members + result, consumed = decode_pvalue_to_bytes(data, 0) + assert result == members + assert consumed == len(data) + + def test_struct_packed_count2_present(self) -> None: + # When the Count2Present transport flag (bit 10) is set, a second count follows. + struct_id = struct.pack(">I", 0x91080009) + timestamp = bytes(8) + transport_flags = encode_uint32_vlq(0x402) # AlwaysSet | Count2Present + members = bytes([0xAA, 0xBB]) + data = ( + bytes([0x00, DataType.STRUCT]) + + struct_id + + timestamp + + transport_flags + + encode_uint32_vlq(999) # first count (ignored when Count2Present) + + encode_uint32_vlq(len(members)) # second count is the real one + + members + ) + result, consumed = decode_pvalue_to_bytes(data, 0) + assert result == members + assert consumed == len(data) + + def test_struct_id_boundary_is_normal(self) -> None: + # The packed-struct ranges are *exclusive* of their bounds, matching the C# reference + # (ValueStruct.Deserialize: ``Value > 0x90000000 && Value < 0x9fffffff``). The bound + # values 0x90000000 and 0x02000000 are base sentinels — real type-ids are base + n + # (e.g. TI_BOOL = 0x02000000 + 1), so the bounds themselves are never packed structs + # and must be parsed as normal structs. Guards against a `<=` regression. + for boundary in (0x90000000, 0x9FFFFFFF, 0x02000000, 0x02FFFFFF): + struct_id = struct.pack(">I", boundary) + member = encode_uint32_vlq(1) + bytes([0x00, DataType.USINT, 0x07]) + terminator = encode_uint32_vlq(0) + data = bytes([0x00, DataType.STRUCT]) + struct_id + member + terminator + result, consumed = decode_pvalue_to_bytes(data, 0) + assert result == bytes([0x07]), f"id {boundary:#010x} must parse as a normal struct" + assert consumed == len(data) def test_unsupported_type(self) -> None: data = bytes([0x00, 0xFF])