Skip to content

Commit ea259b9

Browse files
gijzelaerrclaude
andcommitted
Add s7 unified client/server tests, fix EXPLORE parser and server
32 tests for s7.Client and s7.Server using the built-in emulators: - Legacy: connect, db_read, db_write, db_read_multi, list_datablocks, context manager, delegated methods, diagnostic buffer - S7CommPlus: connect, db_read, db_write, db_read_multi, explore, list_datablocks, browse, browse-to-SymbolTable, auto-detection - Server: context manager, register_db/raw_db, get_db, properties - Guard tests: browse/explore/subscription require S7CommPlus Bug fixes: - s7/client.py: legacy connection optional when S7CommPlus explicit - s7/_s7commplus_client.py: EXPLORE parser handles WSTRING (0x15), UDINT (0x04) datatypes, skips return code VLQ, tracks nesting depth to avoid child objects overwriting parent DB name/number - s7/_s7commplus_server.py: EXPLORE response uses standard S7CommPlus IDs (Ids.OBJECT_VARIABLE_TYPE_NAME, Ids.BLOCK_BLOCK_NUMBER) with proper flags+datatype encoding Coverage: s7/client.py 21%→88%, s7/_s7commplus_client.py 40%→67%, total 78%→81%. mypy clean (0 errors across 79 files). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
1 parent 701cdd6 commit ea259b9

5 files changed

Lines changed: 462 additions & 39 deletions

File tree

s7/_s7commplus_client.py

Lines changed: 30 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -592,31 +592,39 @@ def _parse_explore_datablocks(response: bytes) -> list[dict[str, Any]]:
592592
current_name = ""
593593
current_number = 0
594594
current_rid = 0
595+
depth = 0
596+
597+
# Skip return code VLQ at start of response
598+
if offset < len(response):
599+
_, consumed = _vlq32(response, offset)
600+
offset += consumed
595601

596602
while offset < len(response):
597-
if offset >= len(response):
598-
break
599603

600604
tag = response[offset]
601605
offset += 1
602606

603607
if tag == 0xA1: # START_OF_OBJECT
608+
depth += 1
604609
if offset + 4 > len(response):
605610
break
606-
current_rid = struct.unpack(">I", response[offset : offset + 4])[0]
611+
rid = struct.unpack(">I", response[offset : offset + 4])[0]
607612
offset += 4
608613
# Skip classId, reserved, reserved (3 VLQ values)
609614
for _ in range(3):
610615
if offset >= len(response):
611616
break
612617
_, consumed = _vlq32(response, offset)
613618
offset += consumed
614-
current_name = ""
615-
current_number = 0
619+
if depth == 1:
620+
current_rid = rid
621+
current_name = ""
622+
current_number = 0
616623

617624
elif tag == 0xA2: # TERMINATING_OBJECT
618-
if current_name and current_number > 0:
625+
if depth == 1 and current_name and current_number > 0:
619626
datablocks.append({"name": current_name, "number": current_number, "rid": current_rid})
627+
depth = max(0, depth - 1)
620628

621629
elif tag == 0xA3: # ATTRIBUTE
622630
if offset >= len(response):
@@ -629,24 +637,27 @@ def _parse_explore_datablocks(response: bytes) -> list[dict[str, Any]]:
629637
datatype = response[offset + 1]
630638
offset += 2
631639

632-
if attr_id == Ids.OBJECT_VARIABLE_TYPE_NAME and datatype == 0x13: # WSTRING
640+
if attr_id == Ids.OBJECT_VARIABLE_TYPE_NAME and datatype in (0x13, 0x15): # S7STRING or WSTRING
633641
if offset >= len(response):
634642
break
635643
str_len, consumed = _vlq32(response, offset)
636644
offset += consumed
637645
if offset + str_len <= len(response):
638-
try:
639-
current_name = response[offset : offset + str_len].decode("utf-16-be", errors="replace")
640-
except Exception:
641-
current_name = ""
646+
if depth == 1:
647+
try:
648+
current_name = response[offset : offset + str_len].decode("utf-16-be", errors="replace")
649+
except Exception:
650+
current_name = ""
642651
offset += str_len
643652
continue
644653

645-
if attr_id == Ids.BLOCK_BLOCK_NUMBER and datatype in (0x07, 0x08): # UDINT/DWORD
654+
if attr_id == Ids.BLOCK_BLOCK_NUMBER and datatype in (0x03, 0x04, 0x0C): # UINT/UDINT/DWORD
646655
if offset >= len(response):
647656
break
648-
current_number, consumed = _vlq32(response, offset)
657+
val, consumed = _vlq32(response, offset)
649658
offset += consumed
659+
if depth == 1:
660+
current_number = val
650661
continue
651662

652663
# Skip unknown attribute value
@@ -680,11 +691,17 @@ def _parse_explore_fields(response: bytes, db_number: int, db_name: str) -> list
680691
"""
681692
from .vlq import decode_uint32_vlq as _vlq32
682693

694+
683695
fields: list[dict[str, Any]] = []
684696
offset = 0
685697
field_name = ""
686698
byte_offset = 0
687699

700+
# Skip return code VLQ at start of response
701+
if offset < len(response):
702+
_, consumed = _vlq32(response, offset)
703+
offset += consumed
704+
688705
while offset < len(response):
689706
tag = response[offset]
690707
offset += 1

s7/_s7commplus_server.py

Lines changed: 33 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,7 @@
3535
DataType,
3636
ElementID,
3737
FunctionCode,
38+
Ids,
3839
Opcode,
3940
ProtocolVersion,
4041
READ_FUNCTION_CODES,
@@ -716,34 +717,51 @@ def _handle_explore(self, seq_num: int, session_id: int, request_data: bytes) ->
716717
)
717718
response += encode_uint32_vlq(0) # Return code: success
718719

719-
# Return list of data blocks as objects
720+
# Return list of data blocks as objects using standard S7CommPlus IDs
720721
for db_num, db in sorted(self._data_blocks.items()):
721722
response += bytes([ElementID.START_OF_OBJECT])
722723
response += struct.pack(">I", db.object_id) # Relation ID
723724
response += encode_uint32_vlq(0x00000100) # Class: DataBlock
724725
response += encode_uint32_vlq(0x00000000) # Class flags
725726
response += encode_uint32_vlq(0x00000000) # Attribute ID
726727

727-
# DB number attribute
728+
# ObjectVariableTypeName (233) -- DB name as WSTRING
728729
response += bytes([ElementID.ATTRIBUTE])
729-
response += encode_uint32_vlq(0x0001) # DB number attribute ID
730-
response += encode_typed_value(DataType.UINT, db_num)
730+
response += encode_uint32_vlq(Ids.OBJECT_VARIABLE_TYPE_NAME)
731+
name_bytes = f"DB{db_num}".encode("utf-16-be")
732+
response += bytes([0x00, DataType.WSTRING])
733+
response += encode_uint32_vlq(len(name_bytes))
734+
response += name_bytes
731735

732-
# DB size attribute
736+
# Block_BlockNumber (2521) -- DB number as UDINT
733737
response += bytes([ElementID.ATTRIBUTE])
734-
response += encode_uint32_vlq(0x0002) # DB size attribute ID
735-
response += encode_typed_value(DataType.UDINT, len(db.data))
738+
response += encode_uint32_vlq(Ids.BLOCK_BLOCK_NUMBER)
739+
response += bytes([0x00, DataType.UDINT])
740+
response += encode_uint32_vlq(db_num)
736741

737-
# Variable list
742+
# DB size attribute (non-standard, for backward compat)
743+
response += bytes([ElementID.ATTRIBUTE])
744+
response += encode_uint32_vlq(0x0002)
745+
response += bytes([0x00, DataType.UDINT])
746+
response += encode_uint32_vlq(len(db.data))
747+
748+
# Variable list -- used by browse to resolve field names
738749
if db.variables:
739-
response += bytes([ElementID.VARNAME_LIST])
740-
response += encode_uint32_vlq(len(db.variables))
741750
for var_name, var in db.variables.items():
742-
name_bytes = var_name.encode("utf-8")
743-
response += encode_uint32_vlq(len(name_bytes))
744-
response += name_bytes
745-
response += encode_uint32_vlq(var.soft_datatype)
746-
response += encode_uint32_vlq(var.byte_offset)
751+
response += bytes([ElementID.START_OF_OBJECT])
752+
response += struct.pack(">I", 0) # child RID
753+
response += encode_uint32_vlq(0)
754+
response += encode_uint32_vlq(0)
755+
response += encode_uint32_vlq(0)
756+
757+
response += bytes([ElementID.ATTRIBUTE])
758+
response += encode_uint32_vlq(Ids.OBJECT_VARIABLE_TYPE_NAME)
759+
vname_bytes = var_name.encode("utf-16-be")
760+
response += bytes([0x00, DataType.WSTRING])
761+
response += encode_uint32_vlq(len(vname_bytes))
762+
response += vname_bytes
763+
764+
response += bytes([ElementID.TERMINATING_OBJECT])
747765

748766
response += bytes([ElementID.TERMINATING_OBJECT])
749767

s7/client.py

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -127,10 +127,23 @@ def connect(
127127
else:
128128
self._protocol = Protocol.LEGACY
129129

130-
# Always connect legacy client (needed for block ops, PLC control, etc.)
131-
self._legacy = LegacyClient()
132-
self._legacy.connect(address, rack, slot, tcp_port)
133-
logger.info(f"Legacy S7 connected to {address}:{tcp_port}")
130+
# Connect legacy client for block ops, PLC control, etc.
131+
# Skip when S7CommPlus was explicitly requested — the target may not
132+
# support legacy S7 (e.g. PUT/GET disabled) or use a different port
133+
# (e.g. test emulators).
134+
if self._protocol != Protocol.S7COMMPLUS:
135+
self._legacy = LegacyClient()
136+
self._legacy.connect(address, rack, slot, tcp_port)
137+
logger.info(f"Legacy S7 connected to {address}:{tcp_port}")
138+
elif protocol == Protocol.AUTO:
139+
# AUTO mode with S7CommPlus: also try legacy for block ops
140+
try:
141+
self._legacy = LegacyClient()
142+
self._legacy.connect(address, rack, slot, tcp_port)
143+
logger.info(f"Legacy S7 connected to {address}:{tcp_port}")
144+
except Exception as e:
145+
logger.debug(f"Legacy S7 connection failed (S7CommPlus available): {e}")
146+
self._legacy = None
134147

135148
return self
136149

tests/test_logging.py

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,18 +3,20 @@
33
import json
44
import logging
55

6+
import pytest
7+
68
from snap7.log import PLCLoggerAdapter, OperationLogger, JSONFormatter
79

810

911
class TestPLCLoggerAdapter:
10-
def test_prefix_added(self, caplog: logging.LogRecord) -> None: # type: ignore[type-arg]
12+
def test_prefix_added(self, caplog: pytest.LogCaptureFixture) -> None:
1113
base = logging.getLogger("test.adapter")
1214
adapter = PLCLoggerAdapter(base, plc_host="10.0.0.1", rack=0, slot=1)
1315
with caplog.at_level(logging.INFO, logger="test.adapter"):
1416
adapter.info("Connected")
1517
assert "[10.0.0.1 R0/S1] Connected" in caplog.text
1618

17-
def test_no_prefix_without_host(self, caplog: logging.LogRecord) -> None: # type: ignore[type-arg]
19+
def test_no_prefix_without_host(self, caplog: pytest.LogCaptureFixture) -> None:
1820
base = logging.getLogger("test.nohost")
1921
adapter = PLCLoggerAdapter(base)
2022
with caplog.at_level(logging.INFO, logger="test.nohost"):
@@ -40,7 +42,7 @@ def test_update_context_partial(self) -> None:
4042

4143

4244
class TestOperationLogger:
43-
def test_logs_timing(self, caplog: logging.LogRecord) -> None: # type: ignore[type-arg]
45+
def test_logs_timing(self, caplog: pytest.LogCaptureFixture) -> None:
4446
base = logging.getLogger("test.oplog")
4547
with caplog.at_level(logging.DEBUG, logger="test.oplog"):
4648
with OperationLogger(base, "db_read", db=1, start=0, size=4):
@@ -49,7 +51,7 @@ def test_logs_timing(self, caplog: logging.LogRecord) -> None: # type: ignore[t
4951
assert "db=1" in caplog.text
5052
assert "ms)" in caplog.text
5153

52-
def test_works_with_adapter(self, caplog: logging.LogRecord) -> None: # type: ignore[type-arg]
54+
def test_works_with_adapter(self, caplog: pytest.LogCaptureFixture) -> None:
5355
base = logging.getLogger("test.oplog_adapter")
5456
adapter = PLCLoggerAdapter(base, plc_host="10.0.0.1", rack=0, slot=1)
5557
with caplog.at_level(logging.DEBUG, logger="test.oplog_adapter"):
@@ -90,9 +92,9 @@ def test_plc_context_included(self) -> None:
9092
args=None,
9193
exc_info=None,
9294
)
93-
record.plc_host = "192.168.1.10" # type: ignore[attr-defined]
94-
record.plc_rack = 0 # type: ignore[attr-defined]
95-
record.plc_slot = 1 # type: ignore[attr-defined]
95+
record.plc_host = "192.168.1.10"
96+
record.plc_rack = 0
97+
record.plc_slot = 1
9698
output = formatter.format(record)
9799
data = json.loads(output)
98100
assert data["plc_host"] == "192.168.1.10"

0 commit comments

Comments
 (0)