Skip to content

Commit 0f3790c

Browse files
gijzelaerrclaude
andcommitted
Fix multi-read optimizer: cache mutation, dead code, and usability issues
- Fix cache mutation bug: deep-copy cached ReadPackets before assigning buffers so repeated calls don't corrupt cached state - Remove dead _map_area_int method - Replace per-call set comprehension with module-level _VALID_AREA_VALUES frozenset for Area validation - Fix O(n^2) byte concatenation in build_multi_read_request using list and b"".join() - Clear optimizer cache on disconnect - Add use_optimizer attribute (default True) to allow opting out Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent d0d8467 commit 0f3790c

File tree

2 files changed

+18
-13
lines changed

2 files changed

+18
-13
lines changed

snap7/client.py

Lines changed: 13 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
Drop-in replacement for the ctypes-based client with native Python implementation.
55
"""
66

7+
import copy
78
import logging
89
import struct
910
import time
@@ -39,6 +40,8 @@
3940
CDataArrayType,
4041
)
4142

43+
_VALID_AREA_VALUES: frozenset[int] = frozenset(a.value for a in Area)
44+
4245
logger = logging.getLogger(__name__)
4346

4447

@@ -113,6 +116,7 @@ def __init__(self, lib_location: Optional[str] = None, **kwargs: Any):
113116
# Multi-read optimizer state
114117
self._opt_plan: Optional[_OptimizationPlan] = None
115118
self.multi_read_max_gap: int = 5
119+
self.use_optimizer: bool = True
116120

117121
# Async operation state
118122
self._async_pending = False
@@ -224,6 +228,7 @@ def disconnect(self) -> int:
224228
self.connection = None
225229

226230
self.connected = False
231+
self._opt_plan = None
227232
logger.info(f"Disconnected from {self.host}:{self.port}")
228233
return 0
229234

@@ -503,8 +508,8 @@ def read_multi_vars(self, items: Union[List[dict[str, Any]], "Array[S7DataItem]"
503508
# Dict list path -- use optimizer for 2+ items
504509
dict_items = cast(List[dict[str, Any]], items)
505510

506-
if len(dict_items) <= 1:
507-
# Single item: no optimization needed
511+
if len(dict_items) <= 1 or not self.use_optimizer:
512+
# Single item or optimizer disabled: no optimization needed
508513
results: list[bytearray] = []
509514
for dict_item in dict_items:
510515
area = dict_item["area"]
@@ -555,15 +560,18 @@ def _read_multi_vars_optimized(self, dict_items: List[dict[str, Any]]) -> Tuple[
555560
packets = packetize(blocks, self.pdu_length)
556561
self._opt_plan = _OptimizationPlan(cache_key, packets, read_items)
557562

563+
# Deep-copy blocks from cached packets so we don't mutate cached state
564+
working_packets = copy.deepcopy(packets)
565+
558566
# Execute each packet
559-
for packet in packets:
567+
for packet in working_packets:
560568
block_specs = [(blk.area, blk.db_number, blk.start_offset, blk.byte_length) for blk in packet.blocks]
561569

562570
if len(block_specs) == 1:
563571
# Single block: use regular read to avoid multi-read overhead
564572
blk = packet.blocks[0]
565573
data = self.read_area(
566-
Area(blk.area) if blk.area in {a.value for a in Area} else Area.DB,
574+
Area(blk.area) if blk.area in _VALID_AREA_VALUES else Area.DB,
567575
blk.db_number,
568576
blk.start_offset,
569577
blk.byte_length,
@@ -578,13 +586,9 @@ def _read_multi_vars_optimized(self, dict_items: List[dict[str, Any]]) -> Tuple[
578586
blk.buffer = buf
579587

580588
# Extract per-item results in original order
581-
results = extract_results(packets, len(dict_items))
589+
results = extract_results(working_packets, len(dict_items))
582590
return (0, results)
583591

584-
def _map_area_int(self, area_int: int) -> S7Area:
585-
"""Map integer area value to S7Area enum."""
586-
return S7Area(area_int)
587-
588592
def write_multi_vars(self, items: Union[List[dict[str, Any]], List[S7DataItem]]) -> int:
589593
"""
590594
Write multiple variables in a single request.

snap7/s7protocol.py

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -188,13 +188,14 @@ def build_multi_read_request(self, items: List[Tuple[int, int, int, int]]) -> by
188188
item_count = len(items)
189189

190190
# Build N * 12-byte address specifications
191-
addr_specs = b""
191+
addr_spec_parts: list[bytes] = []
192192
for area_code, db_number, start_offset, byte_length in items:
193-
addr_spec = S7DataTypes.encode_address(S7Area(area_code), db_number, start_offset, S7WordLen.BYTE, byte_length)
194-
addr_specs += addr_spec
193+
addr_spec_parts.append(
194+
S7DataTypes.encode_address(S7Area(area_code), db_number, start_offset, S7WordLen.BYTE, byte_length)
195+
)
195196

196197
# Parameter: function_code(1) + item_count(1) + N * address_spec(12)
197-
param_data = struct.pack(">BB", S7Function.READ_AREA, item_count) + addr_specs
198+
param_data = struct.pack(">BB", S7Function.READ_AREA, item_count) + b"".join(addr_spec_parts)
198199
param_len = len(param_data)
199200

200201
# S7 Header (12 bytes)

0 commit comments

Comments
 (0)