From 1218f2981e33764b438ee6b0b9bddea056f72ce7 Mon Sep 17 00:00:00 2001 From: jan iversen Date: Wed, 23 Oct 2024 12:18:59 +0200 Subject: [PATCH] Correct minor framer/pdu errors. --- examples/client_async.py | 15 ++-- examples/client_async_calls.py | 70 ++++++++++----- examples/client_calls.py | 85 ++++++++++++------- pymodbus/client/mixin.py | 9 +- pymodbus/framer/base.py | 1 - pymodbus/pdu/decoders.py | 2 +- pymodbus/pdu/file_message.py | 52 +++++------- pymodbus/transaction.py | 27 +----- test/framer/test_framer.py | 1 - test/sub_examples/test_client_server_async.py | 11 --- 10 files changed, 137 insertions(+), 136 deletions(-) diff --git a/examples/client_async.py b/examples/client_async.py index c14952dbf8..d3301e58bd 100755 --- a/examples/client_async.py +++ b/examples/client_async.py @@ -40,7 +40,6 @@ sys.exit(-1) import pymodbus.client as modbusClient -from pymodbus import ModbusException _logger = logging.getLogger(__file__) @@ -124,15 +123,11 @@ async def run_async_client(client, modbus_calls=None): async def run_a_few_calls(client): """Test connection works.""" - try: - rr = await client.read_coils(32, 1, slave=1) - assert len(rr.bits) == 8 - rr = await client.read_holding_registers(4, 2, slave=1) - assert rr.registers[0] == 17 - assert rr.registers[1] == 17 - except ModbusException: - pass - + rr = await client.read_coils(32, 1, slave=1) + assert len(rr.bits) == 8 + rr = await client.read_holding_registers(4, 2, slave=1) + assert rr.registers[0] == 17 + assert rr.registers[1] == 17 async def main(cmdline=None): """Combine setup and run.""" diff --git a/examples/client_async_calls.py b/examples/client_async_calls.py index 3f58c56734..9cfaa00b75 100755 --- a/examples/client_async_calls.py +++ b/examples/client_async_calls.py @@ -33,6 +33,8 @@ import logging import sys +from pymodbus import ModbusException + try: import client_async @@ -57,14 +59,14 @@ async def async_template_call(client): """Show complete modbus call, async version.""" try: rr = await client.read_coils(1, 1, slave=SLAVE) - except client_async.ModbusException as exc: + except ModbusException as exc: txt = f"ERROR: exception in pymodbus {exc}" _logger.error(txt) raise exc if rr.isError(): txt = "ERROR: pymodbus returned an error!" _logger.error(txt) - raise client_async.ModbusException(txt) + raise ModbusException(txt) # Validate data txt = f"### Template coils response: {rr.bits!s}" @@ -163,6 +165,12 @@ async def async_handle_input_registers(client): assert len(rr.registers) == 8 +async def async_handle_file_records(_client): + """Read/write file records.""" + _logger.info("### Read/write file records") + # NOT WORKING: + + async def async_execute_information_requests(client): """Execute extended information requests.""" _logger.info("### Running information requests.") @@ -172,21 +180,21 @@ async def async_execute_information_requests(client): rr = await client.report_slave_id(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert not rr.status + assert rr.status rr = await client.read_exception_status(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert not rr.status + assert not rr.status rr = await client.diag_get_comm_event_counter(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert rr.status - # assert not rr.count + assert rr.status + assert not rr.count rr = await client.diag_get_comm_event_log(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert rr.status - # assert not (rr.event_count + rr.message_count + len(rr.events)) + assert rr.status + assert not (rr.event_count + rr.message_count + len(rr.events)) async def async_execute_diagnostic_requests(client): @@ -197,23 +205,38 @@ async def async_execute_diagnostic_requests(client): assert not rr.isError() # test that call was OK assert rr.message == message - await client.diag_restart_communication(True, slave=SLAVE) - await client.diag_read_diagnostic_register(slave=SLAVE) - await client.diag_change_ascii_input_delimeter(slave=SLAVE) + rr = await client.diag_restart_communication(True, slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_diagnostic_register(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_change_ascii_input_delimeter(slave=SLAVE) + assert not rr.isError() # test that call was OK - # NOT WORKING: await client.diag_force_listen_only(slave=SLAVE) + # NOT WORKING: rr = await client.diag_force_listen_only(slave=SLAVE) + assert not rr.isError() # test that call was OK - await client.diag_clear_counters() - await client.diag_read_bus_comm_error_count(slave=SLAVE) - await client.diag_read_bus_exception_error_count(slave=SLAVE) - await client.diag_read_slave_message_count(slave=SLAVE) - await client.diag_read_slave_no_response_count(slave=SLAVE) - await client.diag_read_slave_nak_count(slave=SLAVE) - await client.diag_read_slave_busy_count(slave=SLAVE) - await client.diag_read_bus_char_overrun_count(slave=SLAVE) - await client.diag_read_iop_overrun_count(slave=SLAVE) - await client.diag_clear_overrun_counter(slave=SLAVE) - # NOT WORKING await client.diag_getclear_modbus_response(slave=SLAVE) + rr = await client.diag_clear_counters() + assert not rr.isError() # test that call was OK + rr = await client.diag_read_bus_comm_error_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_bus_exception_error_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_slave_message_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_slave_no_response_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_slave_nak_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_slave_busy_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_bus_char_overrun_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_read_iop_overrun_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = await client.diag_clear_overrun_counter(slave=SLAVE) + assert not rr.isError() # test that call was OK + # NOT WORKING: rr = await client.diag_getclear_modbus_response(slave=SLAVE) + assert not rr.isError() # test that call was OK # ------------------------ @@ -226,6 +249,7 @@ async def run_async_calls(client): await async_handle_discrete_input(client) await async_handle_holding_registers(client) await async_handle_input_registers(client) + await async_handle_file_records(client) await async_execute_information_requests(client) await async_execute_diagnostic_requests(client) diff --git a/examples/client_calls.py b/examples/client_calls.py index 3b3971fdf0..fe47451825 100755 --- a/examples/client_calls.py +++ b/examples/client_calls.py @@ -159,57 +159,81 @@ def handle_input_registers(client): assert len(rr.registers) == 8 +def handle_file_records(_client): + """Read/write file records.""" + _logger.info("### Read/write file records") + # NOT WORKING: + + def execute_information_requests(client): """Execute extended information requests.""" _logger.info("### Running information requests.") - rr = client.read_device_information(slave=SLAVE) - assert not rr.isError() # test that call was OK - assert rr.information[0] == b"Pymodbus" + # NOT WORKING: ONLY SYNC. + # FAILS WITH framer = RTU + # rr = client.read_device_information(slave=SLAVE) + # assert not rr.isError() # test that call was OK + # assert rr.information[0] == b"Pymodbus" rr = client.report_slave_id(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert not rr.status + assert rr.status rr = client.read_exception_status(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert not rr.status + assert not rr.status rr = client.diag_get_comm_event_counter(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert rr.status - # assert not rr.count + assert rr.status + assert not rr.count rr = client.diag_get_comm_event_log(slave=SLAVE) assert not rr.isError() # test that call was OK - # assert rr.status - # assert not (rr.event_count + rr.message_count + len(rr.events)) + assert rr.status + assert not (rr.event_count + rr.message_count + len(rr.events)) def execute_diagnostic_requests(client): """Execute extended diagnostic requests.""" _logger.info("### Running diagnostic requests.") - message = b"OK" - rr = client.diag_query_data(msg=message, slave=SLAVE) - assert not rr.isError() # test that call was OK - assert rr.message == message + # NOT WORKING: ONLY SYNC + # message = b"OK" + # rr = client.diag_query_data(msg=message, slave=SLAVE) + # assert not rr.isError() # test that call was OK + # assert rr.message == message - client.diag_restart_communication(True, slave=SLAVE) - client.diag_read_diagnostic_register(slave=SLAVE) - client.diag_change_ascii_input_delimeter(slave=SLAVE) + rr = client.diag_restart_communication(True, slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_diagnostic_register(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_change_ascii_input_delimeter(slave=SLAVE) + assert not rr.isError() # test that call was OK - # NOT WORKING: await client.diag_force_listen_only(slave=SLAVE) + # NOT WORKING: rr = client.diag_force_listen_only(slave=SLAVE) + assert not rr.isError() # test that call was OK - client.diag_clear_counters() - client.diag_read_bus_comm_error_count(slave=SLAVE) - client.diag_read_bus_exception_error_count(slave=SLAVE) - client.diag_read_slave_message_count(slave=SLAVE) - client.diag_read_slave_no_response_count(slave=SLAVE) - client.diag_read_slave_nak_count(slave=SLAVE) - client.diag_read_slave_busy_count(slave=SLAVE) - client.diag_read_bus_char_overrun_count(slave=SLAVE) - client.diag_read_iop_overrun_count(slave=SLAVE) - client.diag_clear_overrun_counter(slave=SLAVE) - # NOT WORKING client.diag_getclear_modbus_response(slave=SLAVE) + rr = client.diag_clear_counters() + assert not rr.isError() # test that call was OK + rr = client.diag_read_bus_comm_error_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_bus_exception_error_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_slave_message_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_slave_no_response_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_slave_nak_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_slave_busy_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_bus_char_overrun_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_read_iop_overrun_count(slave=SLAVE) + assert not rr.isError() # test that call was OK + rr = client.diag_clear_overrun_counter(slave=SLAVE) + assert not rr.isError() # test that call was OK + # NOT WORKING rr = client.diag_getclear_modbus_response(slave=SLAVE) + assert not rr.isError() # test that call was OK # ------------------------ @@ -222,8 +246,9 @@ def run_sync_calls(client): handle_discrete_input(client) handle_holding_registers(client) handle_input_registers(client) - # awaiting fix: execute_information_requests(client) - # awaiting fix: execute_diagnostic_requests(client) + handle_file_records(client) + execute_information_requests(client) + execute_diagnostic_requests(client) def main(cmdline=None): diff --git a/pymodbus/client/mixin.py b/pymodbus/client/mixin.py index bfc071e8a1..cf0f924125 100644 --- a/pymodbus/client/mixin.py +++ b/pymodbus/client/mixin.py @@ -367,23 +367,22 @@ def report_slave_id(self, slave: int = 1, no_response_expected: bool = False) -> """ return self.execute(no_response_expected, pdu_other_msg.ReportSlaveIdRequest(slave=slave)) - def read_file_record(self, records: list[tuple], slave: int = 1, no_response_expected: bool = False) -> T: + def read_file_record(self, records: list[pdu_file_msg.FileRecord], slave: int = 1, no_response_expected: bool = False) -> T: """Read file record (code 0x14). - :param records: List of (Reference type, File number, Record Number, Record Length) + :param records: List of FileRecord (Reference type, File number, Record Number) :param slave: device id :param no_response_expected: (optional) The client will not expect a response to the request :raises ModbusException: """ return self.execute(no_response_expected, pdu_file_msg.ReadFileRecordRequest(records, slave=slave)) - def write_file_record(self, records: list[tuple], slave: int = 1, no_response_expected: bool = False) -> T: + def write_file_record(self, records: list[pdu_file_msg.FileRecord], slave: int = 1, no_response_expected: bool = False) -> T: """Write file record (code 0x15). - :param records: List of (Reference type, File number, Record Number, Record Length) + :param records: List of File_record (Reference type, File number, Record Number, Record Length, Record Data) :param slave: (optional) Device id :param no_response_expected: (optional) The client will not expect a response to the request - :param no_response_expected: (optional) The client will not expect a response to the request :raises ModbusException: """ return self.execute(no_response_expected, pdu_file_msg.WriteFileRecordRequest(records,slave=slave)) diff --git a/pymodbus/framer/base.py b/pymodbus/framer/base.py index ffdbf52ca5..91ae01dda1 100644 --- a/pymodbus/framer/base.py +++ b/pymodbus/framer/base.py @@ -34,7 +34,6 @@ def __init__( ) -> None: """Initialize a ADU (framer) instance.""" self.decoder = decoder - self.databuffer = b"" def decode(self, _data: bytes) -> tuple[int, int, int, bytes]: """Decode ADU. diff --git a/pymodbus/pdu/decoders.py b/pymodbus/pdu/decoders.py index ca44cc535b..f50d037d33 100644 --- a/pymodbus/pdu/decoders.py +++ b/pymodbus/pdu/decoders.py @@ -112,8 +112,8 @@ def decode(self, frame: bytes) -> base.ModbusPDU | None: raise ModbusException(f"Unknown response {function_code}") pdu = pdu_type() pdu.setData(0, 0, False) - Log.debug("decode PDU for {}", function_code) pdu.decode(frame[1:]) + Log.debug("decoded PDU function_code({} sub {}) -> {} ", pdu.function_code, pdu.sub_function_code, str(pdu)) if pdu.sub_function_code >= 0: lookup = self.sub_lookup.get(pdu.function_code, {}) diff --git a/pymodbus/pdu/file_message.py b/pymodbus/pdu/file_message.py index 163375792b..1ca24c027e 100644 --- a/pymodbus/pdu/file_message.py +++ b/pymodbus/pdu/file_message.py @@ -75,7 +75,7 @@ class ReadFileRecordRequest(ModbusPDU): references within each group must be sequential. Each group is defined in a separate "sub-request" field that contains seven bytes:: - The reference type: 1 byte (must be 0x06) + The reference type: 1 byte The file number: 2 bytes The starting record number within the file: 2 bytes The length of the record to be read: 2 bytes @@ -89,14 +89,14 @@ class ReadFileRecordRequest(ModbusPDU): function_code_name = "read_file_record" _rtu_byte_count_pos = 2 - def __init__(self, records=None, slave=1, transaction=0, skip_encode=False): + def __init__(self, records: list[FileRecord] = [], slave=1, transaction=0, skip_encode=False): # pylint: disable=dangerous-default-value """Initialize a new instance. :param records: The file record requests to be read """ super().__init__() super().setData(slave, transaction, skip_encode) - self.records = records or [] + self.records = records def encode(self): """Encode the request packet. @@ -107,10 +107,10 @@ def encode(self): for record in self.records: packet += struct.pack( ">BHHH", - 0x06, + record.reference_type, record.file_number, record.record_number, - record.record_length, + 0, ) return packet @@ -128,19 +128,17 @@ def decode(self, data): record_number=decoded[2], record_length=decoded[3], ) - if decoded[0] == 0x06: # pragma: no cover - self.records.append(record) + self.records.append(record) def update_datastore(self, _context): # pragma: no cover """Run a read exception status request against the store. :returns: The populated response """ - # TODO do some new context operation here # pylint: disable=fixme - # if file number, record number, or address + length - # is too big, return an error. - files: list[FileRecord] = [] - return ReadFileRecordResponse(files) + for record in self.records: + record.record_data = b'SERVER DUMMY RECORD.' + record.record_length = len(record.record_data) / 2 + return ReadFileRecordResponse(self.records) class ReadFileRecordResponse(ModbusPDU): @@ -155,14 +153,14 @@ class ReadFileRecordResponse(ModbusPDU): function_code = 0x14 _rtu_byte_count_pos = 2 - def __init__(self, records=None, slave=1, transaction=0, skip_encode=False): + def __init__(self, records: list[FileRecord] = [], slave=1, transaction=0, skip_encode=False): # pylint: disable=dangerous-default-value """Initialize a new instance. :param records: The requested file records """ super().__init__() super().setData(slave, transaction, skip_encode) - self.records = records or [] + self.records = records def encode(self): """Encode the response. @@ -172,7 +170,7 @@ def encode(self): total = sum(record.response_length + 1 for record in self.records) packet = struct.pack("B", total) for record in self.records: - packet += struct.pack(">BB", record.response_length, 0x06) + packet += struct.pack(">BB", record.response_length, record.reference_type) packet += record.record_data return packet @@ -195,8 +193,7 @@ def decode(self, data): record_data=data[count : count + record_length], ) count += record_length - if reference_type == 0x06: # pragma: no cover - self.records.append(record) + self.records.append(record) class WriteFileRecordRequest(ModbusPDU): @@ -212,14 +209,14 @@ class WriteFileRecordRequest(ModbusPDU): function_code_name = "write_file_record" _rtu_byte_count_pos = 2 - def __init__(self, records=None, slave=1, transaction=0, skip_encode=False): + def __init__(self, records: list[FileRecord] = [], slave=1, transaction=0, skip_encode=False): # pylint: disable=dangerous-default-value """Initialize a new instance. :param records: The file record requests to be read """ super().__init__() super().setData(slave, transaction, skip_encode) - self.records = records or [] + self.records = records def encode(self): """Encode the request packet. @@ -232,7 +229,7 @@ def encode(self): for record in self.records: packet += struct.pack( ">BHHH", - 0x06, + record.reference_type, record.file_number, record.record_number, record.record_length, @@ -257,17 +254,13 @@ def decode(self, data): record_number=decoded[2], record_data=data[count - response_length : count], ) - if decoded[0] == 0x06: # pragma: no cover - self.records.append(record) + self.records.append(record) def update_datastore(self, _context): # pragma: no cover """Run the write file record request against the context. :returns: The populated response """ - # TODO do some new context operation here # pylint: disable=fixme - # if file number, record number, or address + length - # is too big, return an error. return WriteFileRecordResponse(self.records) @@ -277,14 +270,14 @@ class WriteFileRecordResponse(ModbusPDU): function_code = 0x15 _rtu_byte_count_pos = 2 - def __init__(self, records=None, slave=1, transaction=0, skip_encode=False): + def __init__(self, records: list[FileRecord] = [], slave=1, transaction=0, skip_encode=False): # pylint: disable=dangerous-default-value """Initialize a new instance. :param records: The file record requests to be read """ super().__init__() super().setData(slave, transaction, skip_encode) - self.records = records or [] + self.records = records def encode(self): """Encode the response. @@ -296,7 +289,7 @@ def encode(self): for record in self.records: packet += struct.pack( ">BHHH", - 0x06, + record.reference_type, record.file_number, record.record_number, record.record_length, @@ -321,8 +314,7 @@ def decode(self, data): record_number=decoded[2], record_data=data[count - response_length : count], ) - if decoded[0] == 0x06: # pragma: no cover - self.records.append(record) + self.records.append(record) class ReadFifoQueueRequest(ModbusPDU): diff --git a/pymodbus/transaction.py b/pymodbus/transaction.py index 2e83290ccf..dbcc0dda02 100644 --- a/pymodbus/transaction.py +++ b/pymodbus/transaction.py @@ -136,7 +136,6 @@ def __init__(self, client: ModbusBaseSyncClient, retries): self.client: ModbusBaseSyncClient = client self.retries = retries self._transaction_lock = RLock() - self._no_response_devices: list[int] = [] self.databuffer = b'' if client: self._set_adu_size() @@ -185,17 +184,16 @@ def execute(self, no_response_expected: bool, request: ModbusPDU): # noqa: C901 "Current transaction state - {}", ModbusTransactionState.to_string(self.client.state), ) - retries = self.retries if isinstance(self.client.framer, FramerSocket): request.transaction_id = self.getNextTID() else: request.transaction_id = 0 Log.debug("Running transaction {}", request.transaction_id) if _buffer := hexlify_packets( - self.client.framer.databuffer + self.databuffer ): Log.debug("Clearing current Frame: - {}", _buffer) - self.client.framer.databuffer = b'' + self.databuffer = b'' expected_response_length = None if not isinstance(self.client.framer, FramerSocket): response_pdu_size = request.get_response_pdu_size() @@ -205,12 +203,7 @@ def execute(self, no_response_expected: bool, request: ModbusPDU): # noqa: C901 expected_response_length = ( self._calculate_response_length(response_pdu_size) ) - if ( # pylint: disable=simplifiable-if-statement - request.slave_id in self._no_response_devices - ): - full = True - else: - full = False + full = False if self.client.comm_params.comm_type == CommType.UDP: full = True if not expected_response_length: @@ -223,20 +216,6 @@ def execute(self, no_response_expected: bool, request: ModbusPDU): # noqa: C901 ) if no_response_expected: return None - while retries > 0: - if self._validate_response(response): - if ( - request.slave_id in self._no_response_devices - and response - ): - self._no_response_devices.remove(request.slave_id) - Log.debug("Got response!!!") - break - if not response: - if request.slave_id not in self._no_response_devices: - self._no_response_devices.append(request.slave_id) - # No response received and retries not enabled - break self.databuffer += response used_len, pdu = self.client.framer.processIncomingFrame(self.databuffer) self.databuffer = self.databuffer[used_len:] diff --git a/test/framer/test_framer.py b/test/framer/test_framer.py index cce0d98bd9..28ab35136c 100644 --- a/test/framer/test_framer.py +++ b/test/framer/test_framer.py @@ -415,7 +415,6 @@ def test_processIncomingFrame_roundtrip(self, entry, test_framer, msg, dev_id, t assert result assert result.slave_id == dev_id assert result.transaction_id == tid - assert not test_framer.databuffer expected = test_framer.encode( result.function_code.to_bytes(1,'big') + result.encode(), dev_id, 1) diff --git a/test/sub_examples/test_client_server_async.py b/test/sub_examples/test_client_server_async.py index 92446785d3..bac88b79b0 100755 --- a/test/sub_examples/test_client_server_async.py +++ b/test/sub_examples/test_client_server_async.py @@ -8,7 +8,6 @@ These are basis for most examples and thus tested separately """ import asyncio -from unittest import mock import pytest @@ -18,7 +17,6 @@ run_async_client, setup_async_client, ) -from pymodbus.exceptions import ModbusIOException @pytest.mark.parametrize( @@ -47,15 +45,6 @@ async def test_combinations(self, mock_server, mock_clc): assert mock_server await main(cmdline=mock_clc) - async def test_client_exception(self, mock_server, mock_clc): - """Run async client and server.""" - assert mock_server - test_client = setup_async_client(cmdline=mock_clc) - test_client.read_holding_registers = mock.AsyncMock( - side_effect=ModbusIOException("test") - ) - await run_async_client(test_client, modbus_calls=run_a_few_calls) - async def test_client_no_calls(self, mock_server, mock_clc): """Run async client and server.""" assert mock_server