From 232f0d41d66ef76762f6d5c5b71fc2082792ecf7 Mon Sep 17 00:00:00 2001 From: Gijs Molenaar Date: Wed, 9 Sep 2026 10:22:07 +0200 Subject: [PATCH] fix(s7commplus): use explicit target types for scalar writes --- CHANGES.md | 2 + doc/API/tags.rst | 23 +++++++- s7commplus/async_client.py | 17 ++++-- s7commplus/client.py | 45 ++++++++------ tests/test_s7_server.py | 12 ++-- tests/test_s7_unit.py | 16 ++--- tests/test_s7_write_types.py | 110 +++++++++++++++++++++++++++++++++++ 7 files changed, 187 insertions(+), 38 deletions(-) create mode 100644 tests/test_s7_write_types.py diff --git a/CHANGES.md b/CHANGES.md index fb4aaf69..12f5b996 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -9,6 +9,8 @@ Major release: new `s7commplus` package with S7CommPlus protocol support. * New `s7commplus` package for S7CommPlus protocol (S7-1200/1500) * S7CommPlus V1, V2 (TLS), and V3 support for S7-1200/1500 * S7CommPlus area read/write (M, I, Q, counters, timers) +* Require an explicit target datatype in S7CommPlus multi-write tuples; add + `datatype=` to sync/async symbolic and area writes for scalar PLC targets. * S7CommPlus PLC start/stop via INVOKE * S7CommPlus object browsing via EXPLORE * S7CommPlus live symbol browsing (`client.browse()`) and datablock listing (experimental) diff --git a/doc/API/tags.rst b/doc/API/tags.rst index 13723d76..9cbf5c7d 100644 --- a/doc/API/tags.rst +++ b/doc/API/tags.rst @@ -115,6 +115,7 @@ S7CommPlus tag API is not implemented yet; :class:`~snap7.tags.Tag` and the import struct from s7commplus import Client + from s7commplus.protocol import DataType client = Client() client.connect("192.168.1.10", use_tls=True) @@ -126,7 +127,27 @@ S7CommPlus tag API is not implemented yet; :class:`~snap7.tags.Tag` and the # Symbolic values are currently exposed as raw wire bytes. raw = client.read_symbolic(path[0], path[1:]) speed = struct.unpack(">f", raw)[0] - client.write_symbolic(path[0], path[1:], struct.pack(">f", 1500.0)) + client.write_symbolic(path[0], path[1:], struct.pack(">f", 1500.0), datatype=DataType.REAL) + +The write datatype must match the PLC variable's ``data_type`` from browsing. +For example, an INT requires ``struct.pack(">h", value)`` and +``datatype=DataType.INT``. BLOB is not a generic scalar datatype. The +``datatype=`` keyword is supported by both sync and async symbolic and area +writes; omitting it preserves the legacy BLOB encoding. + +Multi-write requires four-element tuples with an explicit datatype. The +three-element form from development versions is rejected before sending: + +.. code-block:: python + + client.db_write_multi([ + (7, 0, struct.pack(">f", 2.0), DataType.REAL), + (7, 4, struct.pack(">H", 512), DataType.WORD), + ]) + +These DB/offset addresses must be validated for the target PLC; they are not +interchangeable with browse-derived symbolic paths. Explicit BLOB remains +available for targets that accept it, including the raw-byte emulator. API reference ------------- diff --git a/s7commplus/async_client.py b/s7commplus/async_client.py index 53b474a2..cbedaf3b 100644 --- a/s7commplus/async_client.py +++ b/s7commplus/async_client.py @@ -501,7 +501,7 @@ async def db_write(self, db_number: int, start: int, data: bytes, datatype: Data await self.db_write_multi([(db_number, start, data, datatype)]) async def db_write_multi(self, items: list[DBWriteItem]) -> None: - """Write multiple regions, optionally adding a DataType as each tuple's fourth item.""" + """Write (db_number, start_offset, data, datatype) tuples matching the PLC target types.""" payload = _build_write_payload(items, self._protocol_version) response = await self._send_request(FunctionCode.SET_MULTI_VARIABLES, payload) _parse_write_response(response) @@ -526,9 +526,9 @@ async def read_area(self, area_rid: int, start: int, size: int) -> bytes: raise RuntimeError("Area read failed") return results[0] - async def write_area(self, area_rid: int, start: int, data: bytes) -> None: - """Write raw bytes to a controller memory area (M, I, Q, counters, timers).""" - payload = _build_area_write_payload(area_rid, start, data, self._protocol_version) + async def write_area(self, area_rid: int, start: int, data: bytes, *, datatype: DataType = DataType.BLOB) -> None: + """Write a controller memory area, specifying the target datatype for scalar writes.""" + payload = _build_area_write_payload(area_rid, start, data, self._protocol_version, datatype=datatype) response = await self._send_request(FunctionCode.SET_MULTI_VARIABLES, payload) _parse_write_response(response) @@ -710,12 +710,17 @@ async def read_symbolic(self, access_area: int, lids: list[int], symbol_crc: int raise RuntimeError("Symbolic read failed") return results[0] - async def write_symbolic(self, access_area: int, lids: list[int], data: bytes, symbol_crc: int = 0) -> None: + async def write_symbolic( + self, access_area: int, lids: list[int], data: bytes, symbol_crc: int = 0, *, datatype: DataType = DataType.BLOB + ) -> None: """Write a variable using S7CommPlus symbolic (LID-based) access. .. warning:: This method is **experimental** and may change. + + Set ``datatype`` to the target PLC datatype reported by browse(). + The legacy BLOB default is not a generic replacement for scalar types. """ - payload = _build_symbolic_write_payload(access_area, lids, data, symbol_crc, self._protocol_version) + payload = _build_symbolic_write_payload(access_area, lids, data, symbol_crc, self._protocol_version, datatype=datatype) response = await self._send_request(FunctionCode.SET_MULTI_VARIABLES, payload) _parse_write_response(response) diff --git a/s7commplus/client.py b/s7commplus/client.py index 4d3a786e..7abe8d80 100644 --- a/s7commplus/client.py +++ b/s7commplus/client.py @@ -26,7 +26,6 @@ decode_pvalue_to_bytes, encode_item_address, encode_object_qualifier, - encode_pvalue_blob, encode_pvalue_typed, parse_create_object_session_id, ) @@ -44,13 +43,12 @@ logger = logging.getLogger(__name__) _T = TypeVar("_T") -DBWriteItem: TypeAlias = tuple[int, int, bytes] | tuple[int, int, bytes, DataType] +DBWriteItem: TypeAlias = tuple[int, int, bytes, DataType] def _normalize_write_item(item: DBWriteItem) -> tuple[int, int, bytes, DataType]: - if len(item) == 3: - db_number, start, data = item - return db_number, start, data, DataType.BLOB + if len(item) != 4: + raise ValueError("Write items require (db_number, start_offset, data, datatype); use the target PLC datatype") db_number, start, data, datatype = item return db_number, start, data, DataType(datatype) @@ -245,15 +243,18 @@ def db_write_multi(self, items: list[DBWriteItem]) -> None: """Write multiple data block regions in a single request. Args: - items: ``(db_number, start_offset, data)`` tuples for BLOB writes, - or four-tuples adding an explicit :class:`DataType`. + items: ``(db_number, start_offset, data, datatype)`` tuples. + The datatype must match the target PLC variable. BLOB is not + a generic replacement for scalar datatypes. """ if self._connection is None: raise RuntimeError("Not connected") if self._connection.requires_substreamed: - for item in items: - db_number, start, data, datatype = _normalize_write_item(item) + normalized = [_normalize_write_item(item) for item in items] + for _, _, data, datatype in normalized: + encode_pvalue_typed(datatype, data) + for db_number, start, data, datatype in normalized: self._db_write_substreamed(db_number, start, data, datatype) return @@ -322,25 +323,27 @@ def read_area(self, area_rid: int, start: int, size: int) -> bytes: raise RuntimeError("Area read failed") return results[0] - def write_area(self, area_rid: int, start: int, data: bytes) -> None: + def write_area(self, area_rid: int, start: int, data: bytes, *, datatype: DataType = DataType.BLOB) -> None: """Write raw bytes to a controller memory area (M, I, Q, counters, timers). Args: area_rid: Native object RID for the area. start: Start byte offset. data: Bytes to write. + datatype: Target PLC datatype. BLOB preserves legacy raw-byte calls; + use an explicit scalar datatype when writing a scalar target. """ if self._connection is None: raise RuntimeError("Not connected") if self._connection.requires_substreamed: payload = _build_substreamed_write_payload( - self._connection.session_id, area_rid, Ids.CONTROLLER_AREA_VALUE_ACTUAL, [start + 1, len(data)], data + self._connection.session_id, area_rid, Ids.CONTROLLER_AREA_VALUE_ACTUAL, [start + 1, len(data)], data, datatype ) self._connection.send_request(FunctionCode.SET_VAR_SUBSTREAMED, payload) return - payload = _build_area_write_payload(area_rid, start, data, self._connection.protocol_version) + payload = _build_area_write_payload(area_rid, start, data, self._connection.protocol_version, datatype=datatype) response = self._connection.send_request(FunctionCode.SET_MULTI_VARIABLES, payload) _parse_write_response(response) @@ -389,7 +392,9 @@ def read_symbolic(self, access_area: int, lids: list[int], symbol_crc: int = 0) raise RuntimeError("Symbolic read failed") return results[0] - def write_symbolic(self, access_area: int, lids: list[int], data: bytes, symbol_crc: int = 0) -> None: + def write_symbolic( + self, access_area: int, lids: list[int], data: bytes, symbol_crc: int = 0, *, datatype: DataType = DataType.BLOB + ) -> None: """Write a variable using S7CommPlus symbolic (LID-based) access. .. warning:: This method is **experimental** and may change. @@ -407,12 +412,14 @@ def write_symbolic(self, access_area: int, lids: list[int], data: bytes, symbol_ lids: LID path through the symbol tree. data: Raw bytes to write. symbol_crc: Symbol CRC for layout validation (0 = skip check). + datatype: Target PLC datatype, as reported by browse(). BLOB preserves + legacy calls but is not a generic replacement for scalar types. """ if self._connection is None: raise RuntimeError("Not connected") payload = _build_symbolic_write_payload( - access_area, lids, data, symbol_crc, protocol_version=self._connection.protocol_version + access_area, lids, data, symbol_crc, protocol_version=self._connection.protocol_version, datatype=datatype ) response = self._connection.send_request(FunctionCode.SET_MULTI_VARIABLES, payload) _parse_write_response(response) @@ -1072,7 +1079,9 @@ def _build_area_read_payload(area_rid: int, start: int, size: int, protocol_vers return bytes(payload) -def _build_area_write_payload(area_rid: int, start: int, data: bytes, protocol_version: int = ProtocolVersion.V2) -> bytes: +def _build_area_write_payload( + area_rid: int, start: int, data: bytes, protocol_version: int = ProtocolVersion.V2, *, datatype: DataType = DataType.BLOB +) -> bytes: """Build a SetMultiVariables payload for controller memory area access.""" addr_bytes, field_count = encode_item_address( access_area=area_rid, @@ -1086,7 +1095,7 @@ def _build_area_write_payload(area_rid: int, start: int, data: bytes, protocol_v payload += encode_uint32_vlq(field_count) payload += addr_bytes payload += encode_uint32_vlq(1) # item number 1 - payload += encode_pvalue_blob(data) + payload += encode_pvalue_typed(datatype, data) payload += bytes([0x00]) payload += encode_object_qualifier(protocol_version=protocol_version) payload += struct.pack(">I", 0) @@ -1136,6 +1145,8 @@ def _build_symbolic_write_payload( data: bytes, symbol_crc: int = 0, protocol_version: int = ProtocolVersion.V2, + *, + datatype: DataType = DataType.BLOB, ) -> bytes: """Build a SetMultiVariables payload for symbolic (LID-based) access.""" if access_area >= 0x8A0E0000: @@ -1156,7 +1167,7 @@ def _build_symbolic_write_payload( payload += encode_uint32_vlq(field_count) payload += addr_bytes payload += encode_uint32_vlq(1) # item number 1 - payload += encode_pvalue_blob(data) + payload += encode_pvalue_typed(datatype, data) payload += bytes([0x00]) payload += encode_object_qualifier(protocol_version=protocol_version) payload += struct.pack(">I", 0) diff --git a/tests/test_s7_server.py b/tests/test_s7_server.py index f90fa13a..65c44764 100644 --- a/tests/test_s7_server.py +++ b/tests/test_s7_server.py @@ -218,9 +218,9 @@ def test_multi_write(self, server: S7CommPlusServer) -> None: try: client.db_write_multi( [ - (1, 0, b"first"), - (1, 10, b"second"), - (2, 20, b"third"), + (1, 0, b"first", DataType.BLOB), + (1, 10, b"second", DataType.BLOB), + (2, 20, b"third", DataType.BLOB), ] ) @@ -332,9 +332,9 @@ async def test_multi_write(self, server: S7CommPlusServer) -> None: await client.connect("127.0.0.1", port=TEST_PORT) await client.write_multi( [ - (1, 0, b"alpha"), - (1, 10, b"beta"), - (2, 20, b"gamma"), + (1, 0, b"alpha", DataType.BLOB), + (1, 10, b"beta", DataType.BLOB), + (2, 20, b"gamma", DataType.BLOB), ] ) diff --git a/tests/test_s7_unit.py b/tests/test_s7_unit.py index d060c8b8..9e5679e9 100644 --- a/tests/test_s7_unit.py +++ b/tests/test_s7_unit.py @@ -156,13 +156,13 @@ def test_error_items(self) -> None: class TestBuildWritePayload: def test_single_item(self) -> None: - payload = _build_write_payload([(1, 0, bytes([1, 2, 3, 4]))]) + payload = _build_write_payload([(1, 0, bytes([1, 2, 3, 4]), DataType.BLOB)]) assert isinstance(payload, bytes) assert len(payload) > 0 def test_data_appears_in_payload(self) -> None: data = bytes([0xDE, 0xAD, 0xBE, 0xEF]) - payload = _build_write_payload([(1, 0, data)]) + payload = _build_write_payload([(1, 0, data, DataType.BLOB)]) # The raw data should appear in the payload (inside the BLOB PValue) assert data in payload @@ -196,7 +196,7 @@ def test_read_payload_roundtrip(self) -> None: def test_write_read_consistency(self) -> None: """Build write and read payloads for same address, verify both compile.""" read_payload = _build_read_payload([(1, 0, 4)]) - write_payload = _build_write_payload([(1, 0, bytes([1, 2, 3, 4]))]) + write_payload = _build_write_payload([(1, 0, bytes([1, 2, 3, 4]), DataType.BLOB)]) assert isinstance(read_payload, bytes) assert isinstance(write_payload, bytes) @@ -216,7 +216,7 @@ def test_read_payload_has_no_static_integrity_id(self) -> None: assert self._has_only_trailing_fill(payload) def test_write_payload_has_no_static_integrity_id(self) -> None: - payload = _build_write_payload([(1, 0, bytes([1, 2, 3, 4]))]) + payload = _build_write_payload([(1, 0, bytes([1, 2, 3, 4]), DataType.BLOB)]) assert self._has_only_trailing_fill(payload) def test_area_read_payload_has_no_static_integrity_id(self) -> None: @@ -585,12 +585,12 @@ def test_db_read_multi_not_connected(self) -> None: def test_db_write_multi_not_connected(self) -> None: client = S7CommPlusClient() with pytest.raises(RuntimeError, match="Not connected"): - client.db_write_multi([(1, 0, b"data")]) + client.db_write_multi([(1, 0, b"data", DataType.BLOB)]) def test_write_multi_not_connected(self) -> None: client = S7CommPlusClient() with pytest.raises(RuntimeError, match="Not connected"): - client.write_multi([(1, 0, b"data")]) + client.write_multi([(1, 0, b"data", DataType.BLOB)]) def test_db_write_multi_uses_one_substreamed_request_per_item(self) -> None: client = S7CommPlusClient() @@ -598,7 +598,7 @@ def test_db_write_multi_uses_one_substreamed_request_per_item(self) -> None: connection.requires_substreamed = True connection.session_id = 0x70000001 client._connection = connection - items = [(1, 0, b"first"), (2, 10, b"second")] + items = [(1, 0, b"first", DataType.BLOB), (2, 10, b"second", DataType.BLOB)] client.db_write_multi(items) @@ -614,7 +614,7 @@ def test_db_write_multi_uses_one_substreamed_request_per_item(self) -> None: data, ), ) - for db_number, start, data in items + for db_number, start, data, _ in items ] ) diff --git a/tests/test_s7_write_types.py b/tests/test_s7_write_types.py new file mode 100644 index 00000000..006416bb --- /dev/null +++ b/tests/test_s7_write_types.py @@ -0,0 +1,110 @@ +"""Check scalar write types at the public API's outgoing request boundary.""" + +import struct +from unittest.mock import AsyncMock, MagicMock + +import pytest + +from s7commplus.async_client import S7CommPlusAsyncClient +from s7commplus.client import S7CommPlusClient +from s7commplus.protocol import DataType, FunctionCode, ProtocolVersion +from s7commplus.vlq import decode_uint32_vlq + + +@pytest.mark.parametrize("asynchronous", [False, True]) +@pytest.mark.parametrize("operation", ["symbolic", "area", "multi"]) +@pytest.mark.parametrize( + ("datatype", "data", "wire_value"), + [ + (DataType.INT, struct.pack(">h", 77), b"\x00\x4d"), + (DataType.WORD, struct.pack(">H", 512), b"\x02\x00"), + (DataType.DINT, struct.pack(">i", -5), b"\x7b"), + (DataType.UDINT, struct.pack(">I", 300), b"\x82\x2c"), + (DataType.REAL, struct.pack(">f", 2.0), b"\x40\x00\x00\x00"), + ], +) +async def test_scalar_write_wire_type( + asynchronous: bool, operation: str, datatype: DataType, data: bytes, wire_value: bytes +) -> None: + if asynchronous: + client = S7CommPlusAsyncClient() + send = AsyncMock(return_value=b"\x00\x00") + client._send_request = send + if operation == "symbolic": + await client.write_symbolic(0x8A0E0001, [15], data, datatype=datatype) + elif operation == "area": + await client.write_area(82, 0, data, datatype=datatype) + else: + await client.db_write_multi([(1, 0, data, datatype)]) + else: + sync_client = S7CommPlusClient() + connection = MagicMock() + connection.requires_substreamed = False + connection.protocol_version = ProtocolVersion.V2 + send = connection.send_request + send.return_value = b"\x00\x00" + sync_client._connection = connection + if operation == "symbolic": + sync_client.write_symbolic(0x8A0E0001, [15], data, datatype=datatype) + elif operation == "area": + sync_client.write_area(82, 0, data, datatype=datatype) + else: + sync_client.db_write_multi([(1, 0, data, datatype)]) + + function, payload = send.call_args.args + assert function == FunctionCode.SET_MULTI_VARIABLES + count, consumed = decode_uint32_vlq(payload, 4) + assert count == 1 + offset = 4 + consumed + fields, consumed = decode_uint32_vlq(payload, offset) + offset += consumed + for _ in range(fields): + _, consumed = decode_uint32_vlq(payload, offset) + offset += consumed + index, consumed = decode_uint32_vlq(payload, offset) + assert index == 1 + offset += consumed + assert payload[offset : offset + 2 + len(wire_value)] == bytes((0, datatype)) + wire_value + + +@pytest.mark.parametrize("substreamed", [False, True]) +def test_missing_multi_write_type_rejected_before_any_send(substreamed: bool) -> None: + client = S7CommPlusClient() + connection = MagicMock() + connection.requires_substreamed = substreamed + client._connection = connection + with pytest.raises(ValueError, match="require.*datatype"): + client.db_write_multi([(1, 0, b"\x00\x4d", DataType.INT), (1, 2, b"\x00\x01")]) # type: ignore[list-item] + connection.send_request.assert_not_called() + + +async def test_async_missing_multi_write_type_rejected_before_send() -> None: + client = S7CommPlusAsyncClient() + send = AsyncMock() + client._send_request = send + with pytest.raises(ValueError, match="require.*datatype"): + await client.db_write_multi([(1, 0, b"\x00\x4d")]) # type: ignore[list-item] + send.assert_not_called() + + +@pytest.mark.parametrize("substreamed", [False, True]) +def test_invalid_scalar_width_rejected_before_any_send(substreamed: bool) -> None: + client = S7CommPlusClient() + connection = MagicMock() + connection.requires_substreamed = substreamed + client._connection = connection + with pytest.raises(ValueError): + client.db_write_multi([(1, 0, b"\x00\x4d", DataType.INT), (1, 2, b"\x01", DataType.INT)]) + connection.send_request.assert_not_called() + + +def test_substreamed_area_write_keeps_explicit_type() -> None: + client = S7CommPlusClient() + connection = MagicMock() + connection.requires_substreamed = True + connection.session_id = 0x70000001 + client._connection = connection + client.write_area(82, 0, struct.pack(">I", 300), datatype=DataType.UDINT) + function, payload = connection.send_request.call_args.args + assert function == FunctionCode.SET_VAR_SUBSTREAMED + assert payload.endswith(bytes((0, DataType.UDINT, 0x82, 0x2C, 1, 0, 0, 0, 0)))