fix(s7commplus): decode struct PValues per the real wire format (packed + normal)#747
Conversation
gijzelaerr
left a comment
There was a problem hiding this comment.
Review Summary
Clean, well-scoped PR. The codec change correctly handles both packed and normal struct formats per the C# reference driver, and the tests cover the key cases (normal struct, packed struct, packed with Count2Present).
No security concerns — this is purely wire format decoding with no I/O or external calls. No malicious patterns.
One potential correctness bug flagged inline (struct id range checks use exclusive < instead of inclusive <=).
| bytes([0x00, DataType.STRUCT]) | ||
| + struct_id | ||
| + timestamp | ||
| + transport_flags |
There was a problem hiding this comment.
Good test coverage. Consider adding an edge-case test with struct_id = 0x90000000 (boundary value) to verify it's treated as a packed struct — currently the < comparison would misclassify it.
There was a problem hiding this comment.
Added test_struct_id_boundary_is_normal, covering 0x90000000, 0x9FFFFFFF, 0x02000000, 0x02FFFFFF. It asserts they parse as normal structs (not packed): the ranges are exclusive per the C# reference (details in the other thread), so the bounds are base sentinels rather than packed-struct ids.
211e5ef to
69005e6
Compare
…ed + normal) 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 <noreply@anthropic.com>
69005e6 to
c23e00c
Compare
Summary
decode_pvalue_to_bytestreated everyStruct(0x17) as a VLQ element count followed by that many nested PValues. A real S7-1500 uses neither shape — the leading struct id is a fixedUInt32, and the body comes in two forms (matchingValueStruct.Deserializein the thomas-v2/S7CommPlusDriver reference):0x90000000..0x9fffffffor0x02000000..0x02ffffff. Layout:UInt64interface timestamp, VLQ transport flags, VLQ element count (a second count follows when theCount2Presentflag, bit 10, is set), then the members as one raw byte blob — returned verbatim for the caller to interpret with the struct's member layout.[VLQ key][PValue], terminated by a0key.Why
Reading a struct node (rather than each scalar leaf) previously returned garbage / raised. With this, a single
read_symbolicof a struct returns the correct member bytes.Test plan
Count2Present(two counts).pytest tests/test_s7_codec.py— 99 passed; fullpytest tests/green;ruffclean.Int) members — matching the individual leaf reads.🤖 Generated with Claude Code