From 53fa47f8fee11da91c2de3d83013db812cfea3fe Mon Sep 17 00:00:00 2001 From: Logan Gunthorpe Date: Mon, 28 Aug 2023 16:24:41 -0600 Subject: [PATCH 1/3] ModbusBaseClient: Fix retries in async client The code in async_execute() in ModbusBaseClient appears to be written to retry, but the send is not in the loop, so the function never retries. To fix this put the transport_send() command inside the while loop. --- pymodbus/client/base.py | 38 ++++++++++++++++++++------------------ 1 file changed, 20 insertions(+), 18 deletions(-) diff --git a/pymodbus/client/base.py b/pymodbus/client/base.py index a99d247e8..248c1e9b8 100644 --- a/pymodbus/client/base.py +++ b/pymodbus/client/base.py @@ -189,25 +189,27 @@ async def async_execute(self, request=None): """Execute requests asynchronously.""" request.transaction_id = self.transaction.getNextTID() packet = self.framer.buildPacket(request) - self.transport_send(packet) - if self.params.broadcast_enable and not request.slave_id: - resp = b"Broadcast write sent - no response expected" - else: - count = 0 - while count <= self.params.retries: - try: - req = self._build_response(request.transaction_id) - resp = await asyncio.wait_for( - req, timeout=self.comm_params.timeout_connect - ) - break - except asyncio.exceptions.TimeoutError: - count += 1 - if count > self.params.retries: - self.close(reconnect=True) - raise ModbusIOException( - f"ERROR: No response received after {self.params.retries} retries" + + count = 0 + while count <= self.params.retries: + self.transport_send(packet) + if self.params.broadcast_enable and not request.slave_id: + resp = b"Broadcast write sent - no response expected" + break + try: + req = self._build_response(request.transaction_id) + resp = await asyncio.wait_for( + req, timeout=self.comm_params.timeout_connect ) + break + except asyncio.exceptions.TimeoutError: + count += 1 + if count > self.params.retries: + self.close(reconnect=True) + raise ModbusIOException( + f"ERROR: No response received after {self.params.retries} retries" + ) + return resp def callback_data(self, data: bytes, addr: tuple = None) -> int: From 8c748554f5d070fc0579c8ede805870bf9ae151b Mon Sep 17 00:00:00 2001 From: Logan Gunthorpe Date: Mon, 28 Aug 2023 17:27:12 -0600 Subject: [PATCH 2/3] test_client: Add tests for retries and timeout with async_execute() Add tests to ensure retries send data the appropriate number of times and a timeout with ModbusIOException occurs after sufficient retries are sent. --- test/sub_client/test_client.py | 37 ++++++++++++++++++++++++++++++++-- 1 file changed, 35 insertions(+), 2 deletions(-) diff --git a/test/sub_client/test_client.py b/test/sub_client/test_client.py index 82d108c11..fadcd4fd6 100755 --- a/test/sub_client/test_client.py +++ b/test/sub_client/test_client.py @@ -17,7 +17,7 @@ from pymodbus.client.mixin import ModbusClientMixin from pymodbus.datastore import ModbusSlaveContext from pymodbus.datastore.store import ModbusSequentialDataBlock -from pymodbus.exceptions import ConnectionException +from pymodbus.exceptions import ConnectionException, ModbusIOException from pymodbus.framer.ascii_framer import ModbusAsciiFramer from pymodbus.framer.rtu_framer import ModbusRtuFramer from pymodbus.framer.socket_framer import ModbusSocketFramer @@ -365,9 +365,10 @@ async def test_client_protocol_handler(): class MockTransport: """Mock transport class which responds with an appropriate encoded packet""" - def __init__(self, base, req): + def __init__(self, base, req, retries=0): """Initialize MockTransport""" self.base = base + self.retries = retries db = ModbusSequentialDataBlock(0, [0] * 100) self.ctx = ModbusSlaveContext(di=db, co=db, hr=db, ir=db) @@ -382,6 +383,9 @@ async def delayed_resp(self): def write(self, data, addr=None): """Write data to the transport, start a task to send the response""" + if self.retries: + self.retries -= 1 + return self.delayed_resp_task = asyncio.create_task(self.delayed_resp()) def close(self): @@ -401,6 +405,35 @@ async def test_client_protocol_execute(): assert isinstance(response, pdu_bit_read.ReadCoilsResponse) +async def test_client_protocol_retry(): + """Test the client protocol execute method with retries""" + base = ModbusBaseClient(host="127.0.0.1", framer=ModbusSocketFramer, timeout=0.1) + request = pdu_bit_read.ReadCoilsRequest(1, 1) + transport = MockTransport(base, request, retries=2) + base.connection_made(transport=transport) + + response = await base.async_execute(request) + assert transport.retries == 0 + assert not response.isError() + assert isinstance(response, pdu_bit_read.ReadCoilsResponse) + + +async def test_client_protocol_timeout(): + """Test the client protocol execute method with timeout""" + base = ModbusBaseClient( + host="127.0.0.1", framer=ModbusSocketFramer, timeout=0.1, retries=2 + ) + # Avoid creating do_reconnect() task + base.connection_lost = mock.MagicMock() + request = pdu_bit_read.ReadCoilsRequest(1, 1) + transport = MockTransport(base, request, retries=4) + base.connection_made(transport=transport) + + with pytest.raises(ModbusIOException): + await base.async_execute(request) + assert transport.retries == 1 + + def test_client_udp_connect(): """Test the Udp client connection method""" with mock.patch.object(socket, "socket") as mock_method: From 52d9107ab601583a3444f995b2319f6f4d790082 Mon Sep 17 00:00:00 2001 From: jan iversen Date: Wed, 30 Aug 2023 12:32:53 +0200 Subject: [PATCH 3/3] Add no_resend_parameter. --- API_changes.rst | 1 + pymodbus/client/base.py | 9 +++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/API_changes.rst b/API_changes.rst index 02ff76934..86a008b61 100644 --- a/API_changes.rst +++ b/API_changes.rst @@ -14,6 +14,7 @@ Version 3.5.0 (future) - :code:`ModbusPlusOperation` - :code:`Endian` - :code:`ModbusStatus` +- Async clients now accepts `no_resend_on_retry=True`, to not resend the request when retrying. ------------- Version 3.4.2 diff --git a/pymodbus/client/base.py b/pymodbus/client/base.py index 248c1e9b8..9449674b3 100644 --- a/pymodbus/client/base.py +++ b/pymodbus/client/base.py @@ -32,6 +32,7 @@ class ModbusBaseClient(ModbusClientMixin, ModbusProtocol): :param reconnect_delay: (optional) Minimum delay in milliseconds before reconnecting. :param reconnect_delay_max: (optional) Maximum delay in milliseconds before reconnecting. :param on_reconnect_callback: (optional) Function that will be called just before a reconnection attempt. + :param no_resend_on_retry: (optional) Do not resend request when retrying due to missing response. :param kwargs: (optional) Experimental parameters. .. tip:: @@ -75,6 +76,7 @@ def __init__( # pylint: disable=too-many-arguments reconnect_delay: float = 0.1, reconnect_delay_max: float = 300, on_reconnect_callback: Callable[[], None] | None = None, + no_resend_on_retry: bool = False, **kwargs: Any, ) -> None: """Initialize a client instance.""" @@ -114,10 +116,8 @@ def __init__( # pylint: disable=too-many-arguments self.reconnect_delay_max = int(reconnect_delay_max) self.on_reconnect_callback = on_reconnect_callback self.retry_on_empty: int = 0 - # -> retry read on nothing - + self.no_resend_on_retry = no_resend_on_retry self.slaves: list[int] = [] - # -> list of acceptable slaves (0 for accept all) # Common variables. self.framer = framer(ClientDecoder(), self) @@ -192,7 +192,8 @@ async def async_execute(self, request=None): count = 0 while count <= self.params.retries: - self.transport_send(packet) + if not count or not self.no_resend_on_retry: + self.transport_send(packet) if self.params.broadcast_enable and not request.slave_id: resp = b"Broadcast write sent - no response expected" break