From bd67b1766eeea5d23450fb083160160a596d8bf2 Mon Sep 17 00:00:00 2001 From: Gijs Molenaar Date: Wed, 11 Nov 2026 15:00:58 +0200 Subject: [PATCH] Let read_tags and read_multi_vars read more than 20 items read_multi_vars raised ValueError when given more than MAX_VARS (20) items, before the read optimizer ran, so read_tags failed above 20 tags although the optimizer already spreads a read over several requests. - The dict form of read_multi_vars and read_tags accept any number of items, in both clients. The ctypes form of read_multi_vars and write_multi_vars keep the MAX_VARS limit. - optimizer.packetize takes a max_items argument (default 20) and starts a new packet when the current one holds that many blocks, next to the request and reply byte budgets. With a PDU of 480 bytes or more, over 20 small blocks used to fit the byte budgets of one request. The client passes MAX_VARS. - read_tags raises NotImplementedError for a symbolic tag, as read_tag does, in both clients. MAX_VARS stays a public name with the value 20. --- CHANGES.md | 5 ++++ snap7/async_client.py | 9 ++++--- snap7/client.py | 24 ++++++++++-------- snap7/optimizer.py | 19 +++++++++++--- tests/test_async_typed_access.py | 8 ++++++ tests/test_client.py | 23 ++++++++++++++--- tests/test_optimizer.py | 43 ++++++++++++++++++++++++++++++++ 7 files changed, 110 insertions(+), 21 deletions(-) diff --git a/CHANGES.md b/CHANGES.md index 11945ee4..2c652f91 100644 --- a/CHANGES.md +++ b/CHANGES.md @@ -12,6 +12,11 @@ Unreleased the heartbeat of the synchronous client counts such a refusal as a live connection. Not yet tested on hardware. * The server answers SZL 0x0424 from its CPU state, and refuses a read request that has no address specification. Previously it answered such a request with the first four bytes of DB1. +* `read_tags` and `read_multi_vars` with a list of dicts now accept more than 20 items, in both clients. Previously + they raised `ValueError` over `MAX_VARS`, although the read optimizer already splits a read over several requests. + The optimizer now also puts at most 20 items in one request; `optimizer.packetize` takes a `max_items` argument + for this. The ctypes form of `read_multi_vars` and `write_multi_vars` keep the limit. +* `read_tags` now raises `NotImplementedError` for a symbolic tag, as `read_tag` does, in both clients. 3.4.0 ----- diff --git a/snap7/async_client.py b/snap7/async_client.py index df2fac73..1e38b6ea 100644 --- a/snap7/async_client.py +++ b/snap7/async_client.py @@ -811,8 +811,10 @@ async def write_tag(self, tag: "Union[Tag, str]", value: Any, encoding: str = "l return await self.write_area(Area(resolved.area), resolved.db_number, resolved.byte_offset, buf) async def read_tags(self, tags: "Sequence[Union[Tag, str]]", encoding: str = "latin-1") -> list[Any]: - """Read multiple tags in a single optimized request, in input order.""" + """Read any number of tags, returning the values in input order.""" resolved = [Tag.from_string(t) if isinstance(t, str) else t for t in tags] + if any(t.is_symbolic for t in resolved): + raise NotImplementedError("Symbolic (LID-based) tag access is not supported by the classic S7 client.") items = [{"area": Area(t.area), "db_number": t.db_number, "start": t.byte_offset, "size": t.size} for t in resolved] _code, data_list = await self.read_multi_vars(items) return [decode_tag(t, bytearray(d), encoding=encoding) for t, d in zip(resolved, data_list)] @@ -951,15 +953,14 @@ async def read_multi_vars(self, items: List[dict[str, Any]]) -> Tuple[int, list[ """Read multiple variables (sequentially, one read_area per item). Args: - items: List of item dicts with keys: area, db_number, start, size + items: List of item dicts with keys: area, db_number, start, size. + Any number of items is accepted. Returns: Tuple of (result_code, list_of_bytearrays) """ if not items: return (0, []) - if len(items) > self.MAX_VARS: - raise ValueError(f"Too many items: {len(items)} exceeds MAX_VARS ({self.MAX_VARS})") results: list[bytearray] = [] for item in items: diff --git a/snap7/client.py b/snap7/client.py index 057aaa00..aa1c4793 100644 --- a/snap7/client.py +++ b/snap7/client.py @@ -107,7 +107,7 @@ class Client(ClientMixin): >>> client.disconnect() """ - MAX_VARS = 20 # Max variables per multi-read/multi-write request + MAX_VARS = 20 # Max variables per multi-read/multi-write PDU def __init__( self, @@ -728,10 +728,11 @@ def write_tag(self, tag: "Union[Tag, str]", value: Any, encoding: str = "latin-1 return self.write_area(Area(resolved.area), resolved.db_number, resolved.byte_offset, buf) def read_tags(self, tags: "Sequence[Union[Tag, str]]", encoding: str = "latin-1") -> list[Any]: - """Read multiple tags in a single optimized request. + """Read multiple tags in as few requests as possible. Uses the multi-variable read optimizer when available to batch - reads into minimal PDU exchanges. + reads into minimal PDU exchanges. Any number of tags is accepted; + tags that do not fit in one request go into further requests. Args: tags: Sequence of :class:`~snap7.tags.Tag` instances or address strings. @@ -741,6 +742,8 @@ def read_tags(self, tags: "Sequence[Union[Tag, str]]", encoding: str = "latin-1" List of decoded values in the same order as input. """ resolved = [Tag.from_string(t) if isinstance(t, str) else t for t in tags] + if any(t.is_symbolic for t in resolved): + raise NotImplementedError("Symbolic (LID-based) tag access is not supported by the classic S7 client.") items = [{"area": Area(t.area), "db_number": t.db_number, "start": t.byte_offset, "size": t.size} for t in resolved] _code, data_list = self.read_multi_vars(items) return [decode_tag(t, bytearray(d), encoding=encoding) for t, d in zip(resolved, data_list)] @@ -976,12 +979,14 @@ def build_chunk_request(o: int = chunk_offset, cd: bytes = bytes(chunk_data)) -> @_instrumented("read_multi_vars") def read_multi_vars(self, items: Union[List[dict[str, Any]], "Array[S7DataItem]"]) -> Tuple[int, Any]: - """Read multiple variables in a single request. + """Read multiple variables in as few requests as possible. When given a list of dicts with two or more items, uses the multi-variable read optimizer to merge adjacent reads and pack them into minimal PDU exchanges. This significantly reduces the number of round-trips compared - to reading each variable individually. + to reading each variable individually. A list of dicts may hold any + number of items: each request carries at most ``MAX_VARS`` address + specifications and the rest go into further requests. .. warning:: @@ -999,16 +1004,15 @@ def read_multi_vars(self, items: Union[List[dict[str, Any]], "Array[S7DataItem]" ctypes array or a list of bytearrays in the original item order. Raises: - ValueError: If more than MAX_VARS items are requested. + ValueError: If a ctypes array holds more than MAX_VARS items. """ if not items: return (0, items) - if len(items) > self.MAX_VARS: - raise ValueError(f"Too many items: {len(items)} exceeds MAX_VARS ({self.MAX_VARS})") - # Handle S7DataItem array (ctypes) -- unchanged legacy path if hasattr(items, "_type_") and hasattr(items[0], "Area"): + if len(items) > self.MAX_VARS: + raise ValueError(f"Too many items: {len(items)} exceeds MAX_VARS ({self.MAX_VARS})") s7_items = cast("Array[S7DataItem]", items) for s7_item in s7_items: area = Area(s7_item.Area) @@ -1152,7 +1156,7 @@ def _read_multi_vars_optimized(self, dict_items: List[dict[str, Any]]) -> Tuple[ sorted_ri = sort_items(read_items) max_block = self._max_read_size() blocks = merge_items(sorted_ri, max_gap=self.multi_read_max_gap, max_block_size=max_block) - packets = packetize(blocks, self.pdu_length) + packets = packetize(blocks, self.pdu_length, max_items=self.MAX_VARS) self._opt_plan = _OptimizationPlan(cache_key, packets, read_items) # Deep-copy blocks from cached packets so we don't mutate cached state diff --git a/snap7/optimizer.py b/snap7/optimizer.py index 56e79f1c..3f041d19 100644 --- a/snap7/optimizer.py +++ b/snap7/optimizer.py @@ -14,6 +14,10 @@ logger = logging.getLogger(__name__) +# Most address specifications a CPU accepts in one read request. This is the +# limit native Snap7 enforces as ``MaxVars``. +MAX_ITEMS_PER_PDU = 20 + # Areas that support contiguous-block merging. Counter (0x1C) and Timer # (0x1D) use element-based addressing, not byte-based, so merging them # as contiguous byte ranges would produce incorrect reads. @@ -206,12 +210,13 @@ def _split_block(block: ReadBlock, max_block_size: int) -> list[ReadBlock]: return sub_blocks -def packetize(blocks: list[ReadBlock], pdu_size: int) -> list[ReadPacket]: +def packetize(blocks: list[ReadBlock], pdu_size: int, max_items: int = MAX_ITEMS_PER_PDU) -> list[ReadPacket]: """Pack blocks into PDU-sized packets. - Two budgets are enforced per packet: + Three limits are enforced per packet: - **Request budget**: ``12 (header) + 2 (func+count) + 12*N (address specs) <= pdu_size`` - **Reply budget**: ``12 (header) + 2 (func+count) + sum(4 + ceil_even(length)) <= pdu_size`` + - **Item count**: at most *max_items* blocks (address specs) Oversized blocks are first split at item boundaries, then blocks are greedily packed into packets. @@ -219,10 +224,17 @@ def packetize(blocks: list[ReadBlock], pdu_size: int) -> list[ReadPacket]: Args: blocks: Merged read blocks. pdu_size: Negotiated PDU size in bytes. + max_items: Maximum number of blocks per packet (default 20). Returns: List of ReadPackets. + + Raises: + ValueError: If *max_items* is less than 1. """ + if max_items < 1: + raise ValueError(f"max_items must be at least 1, got {max_items}") + # First split any oversized blocks # Max data payload per block in a single-block packet max_single_block = pdu_size - 12 - 2 - 4 # header + param + data item header @@ -248,8 +260,9 @@ def packetize(blocks: list[ReadBlock], pdu_size: int) -> list[ReadPacket]: fits_request = current_req_used + req_cost <= pdu_size fits_reply = current_reply_used + reply_cost <= pdu_size + fits_count = len(current_packet.blocks) < max_items - if current_packet.blocks and (not fits_request or not fits_reply): + if current_packet.blocks and (not fits_request or not fits_reply or not fits_count): # Start a new packet packets.append(current_packet) current_packet = ReadPacket() diff --git a/tests/test_async_typed_access.py b/tests/test_async_typed_access.py index dffc4872..c5e94ea2 100644 --- a/tests/test_async_typed_access.py +++ b/tests/test_async_typed_access.py @@ -119,6 +119,14 @@ async def test_symbolic_tag_not_supported(client: AsyncClient) -> None: await client.read_tag(tag) with pytest.raises(NotImplementedError): await client.write_tag(tag, 1) + with pytest.raises(NotImplementedError): + await client.read_tags(["DB1:400:INT", tag]) + + +async def test_read_tags_above_20(client: AsyncClient) -> None: + await client.db_write(1, 500, bytearray(range(25))) + tags = [f"DB1.DBB{500 + i}:BYTE" for i in range(25)] + assert await client.read_tags(tags) == list(range(25)) async def test_is_alive(server: Server) -> None: diff --git a/tests/test_client.py b/tests/test_client.py index 967123f4..043046df 100755 --- a/tests/test_client.py +++ b/tests/test_client.py @@ -1239,16 +1239,31 @@ class TestMaxVars: def test_max_vars_constant(self) -> None: assert Client.MAX_VARS == 20 - def test_read_multi_vars_rejects_too_many(self) -> None: + def test_read_multi_vars_ctypes_rejects_too_many(self) -> None: + """The ctypes form keeps the MAX_VARS limit of the native API.""" client = Client() client.connected = True - mock_conn = MagicMock() - client.connection = mock_conn + client.connection = MagicMock() - items = [{"area": Area.DB, "db_number": 1, "start": i, "size": 1} for i in range(21)] + items = (S7DataItem * 21)() + for item in items: + item.Area = Area.DB.value with pytest.raises(ValueError, match="Too many items.*21.*MAX_VARS.*20"): client.read_multi_vars(items) + def test_read_multi_vars_dicts_above_max(self) -> None: + """The dict form has no item limit when the optimizer is off either.""" + client = Client() + client.connected = True + client.connection = MagicMock() + client.use_optimizer = False + client.read_area = MagicMock(return_value=bytearray(1)) + + items = [{"area": Area.DB, "db_number": 1, "start": i, "size": 1} for i in range(21)] + result_code, results = client.read_multi_vars(items) + assert result_code == 0 + assert len(results) == 21 + def test_write_multi_vars_rejects_too_many(self) -> None: client = Client() client.connected = True diff --git a/tests/test_optimizer.py b/tests/test_optimizer.py index c86167ef..7ae98ad8 100644 --- a/tests/test_optimizer.py +++ b/tests/test_optimizer.py @@ -7,6 +7,7 @@ from .conftest import get_free_tcp_port from ctypes import c_char from typing import TYPE_CHECKING +from unittest.mock import patch import pytest @@ -23,6 +24,7 @@ packetize, extract_results, ) +from snap7.tags import Tag from snap7.type import Area, SrvArea @@ -185,6 +187,18 @@ def test_reply_budget_limit(self) -> None: packets = packetize(blocks, pdu_size=240) assert len(packets) == 2 + def test_item_count_limit(self) -> None: + # 45 two-byte blocks fit the byte budgets of a 960-byte PDU, but not the 20-item limit + blocks = [ReadBlock(area=0x84, db_number=i, start_offset=0, byte_length=2, items=[]) for i in range(45)] + packets = packetize(blocks, pdu_size=960) + assert [len(p.blocks) for p in packets] == [20, 20, 5] + + def test_item_count_limit_is_configurable(self) -> None: + blocks = [ReadBlock(area=0x84, db_number=i, start_offset=0, byte_length=2, items=[]) for i in range(7)] + assert [len(p.blocks) for p in packetize(blocks, pdu_size=960, max_items=3)] == [3, 3, 1] + with pytest.raises(ValueError): + packetize(blocks, pdu_size=960, max_items=0) + def test_oversized_block_split(self) -> None: # A block larger than pdu - overhead should be split at item boundaries items = [ @@ -284,6 +298,35 @@ def test_multi_read_basic(self) -> None: assert results[0] == bytearray(self.db1_data[0:4]) assert results[1] == bytearray(self.db1_data[10:14]) + def test_multi_read_above_20_blocks(self) -> None: + """26 reads that cannot be merged are spread over requests of at most 20 items.""" + offsets = range(0, 100, 8) # gaps of 7 bytes, wider than multi_read_max_gap + items = [{"area": Area.DB, "db_number": db, "start": o, "size": 1} for db in (1, 2) for o in offsets] + assert len(items) == 26 + assert self.client.multi_read_max_gap < 7 + + build = self.client.protocol.build_multi_read_request + with patch.object(self.client.protocol, "build_multi_read_request", side_effect=build) as spy: + result_code, results = self.client.read_multi_vars(items) + + assert result_code == 0 + expected = [bytearray([o]) for o in offsets] + [bytearray([100 + o]) for o in offsets] + assert results == expected + spec_counts = [len(call.args[0]) for call in spy.call_args_list] + assert spec_counts and max(spec_counts) <= 20 + assert sum(spec_counts) >= 25 # a lone block may go out as a plain read + + def test_read_tags_above_20(self) -> None: + """read_tags accepts more than 20 tags and keeps their order.""" + tags = [f"DB{db}.DBB{o}:BYTE" for db in (1, 2) for o in range(0, 100, 8)] + values = self.client.read_tags(tags) + assert values == list(range(0, 100, 8)) + [100 + o for o in range(0, 100, 8)] + + def test_read_tags_refuses_symbolic(self) -> None: + tag = Tag(Area.DB, 1, 0, "INT", access_sequence=[1, 2]) + with pytest.raises(NotImplementedError): + self.client.read_tags(["DB1.DBB0:BYTE", tag]) + def test_multi_read_different_dbs(self) -> None: """Read items from DB1 and DB2.""" items = [