From fb974a9bc7e149ae5dc03b91c1dbcc6555fa99e5 Mon Sep 17 00:00:00 2001 From: jan iversen Date: Sun, 13 Oct 2024 18:27:15 +0200 Subject: [PATCH] Framer do not check ids (#2375) --- examples/message_parser.py | 4 ++-- pymodbus/client/base.py | 2 +- pymodbus/client/modbusclientprotocol.py | 2 +- pymodbus/framer/base.py | 19 ++++--------------- pymodbus/server/async_io.py | 7 +------ pymodbus/transaction.py | 5 +---- test/framers/conftest.py | 8 +------- test/framers/generator.py | 4 ++-- test/framers/test_framer.py | 14 +++++--------- test/framers/test_multidrop.py | 14 ++------------ .../sub_client/test_client_faulty_response.py | 4 ++-- test/sub_current/test_transaction.py | 8 ++++---- 12 files changed, 26 insertions(+), 65 deletions(-) diff --git a/examples/message_parser.py b/examples/message_parser.py index f5970a126..329edd2a8 100755 --- a/examples/message_parser.py +++ b/examples/message_parser.py @@ -73,8 +73,8 @@ def decode(self, message): print(f"Decoding Message {value}") print("=" * 80) decoders = [ - self.framer(ServerDecoder(), []), - self.framer(ClientDecoder(), []), + self.framer(ServerDecoder()), + self.framer(ClientDecoder()), ] for decoder in decoders: print(f"{decoder.decoder.__class__.__name__}") diff --git a/pymodbus/client/base.py b/pymodbus/client/base.py index 1d701b084..8ac6950ce 100644 --- a/pymodbus/client/base.py +++ b/pymodbus/client/base.py @@ -186,7 +186,7 @@ def __init__( self.slaves: list[int] = [] # Common variables. - self.framer: FramerBase = (FRAMER_NAME_TO_CLASS[framer])(ClientDecoder(), [0]) + self.framer: FramerBase = (FRAMER_NAME_TO_CLASS[framer])(ClientDecoder()) self.transaction = SyncModbusTransactionManager( self, self.retries, diff --git a/pymodbus/client/modbusclientprotocol.py b/pymodbus/client/modbusclientprotocol.py index 79f64b114..a5286bbc6 100644 --- a/pymodbus/client/modbusclientprotocol.py +++ b/pymodbus/client/modbusclientprotocol.py @@ -35,7 +35,7 @@ def __init__( self.on_connect_callback = on_connect_callback # Common variables. - self.framer: FramerBase = (FRAMER_NAME_TO_CLASS[framer])(ClientDecoder(), []) + self.framer: FramerBase = (FRAMER_NAME_TO_CLASS[framer])(ClientDecoder()) self.transaction = ModbusTransactionManager() def _handle_response(self, reply): diff --git a/pymodbus/framer/base.py b/pymodbus/framer/base.py index ab4efd8d2..9c56ca414 100644 --- a/pymodbus/framer/base.py +++ b/pymodbus/framer/base.py @@ -32,13 +32,9 @@ class FramerBase: def __init__( self, decoder: ClientDecoder | ServerDecoder, - dev_ids: list[int], ) -> None: """Initialize a ADU (framer) instance.""" self.decoder = decoder - if 0 in dev_ids: - dev_ids = [] - self.dev_ids = dev_ids self.databuffer = b"" def decode(self, _data: bytes) -> tuple[int, int, int, bytes]: @@ -52,14 +48,12 @@ def decode(self, _data: bytes) -> tuple[int, int, int, bytes]: """ return 0, 0, 0, self.EMPTY - def encode(self, data: bytes, dev_id: int, _tid: int) -> bytes: + def encode(self, data: bytes, _dev_id: int, _tid: int) -> bytes: """Encode ADU. returns: modbus ADU (bytes) """ - if dev_id and dev_id not in self.dev_ids: - self.dev_ids.append(dev_id) return data def buildFrame(self, message: ModbusPDU) -> bytes: @@ -71,7 +65,7 @@ def buildFrame(self, message: ModbusPDU) -> bytes: frame = self.encode(data, message.slave_id, message.transaction_id) return frame - def processIncomingFrame(self, data: bytes, tid=None) -> ModbusPDU | None: + def processIncomingFrame(self, data: bytes) -> ModbusPDU | None: """Process new packet pattern. This takes in a new request packet, adds it to the current @@ -82,7 +76,7 @@ def processIncomingFrame(self, data: bytes, tid=None) -> ModbusPDU | None: self.databuffer += data while True: try: - used_len, pdu = self._processIncomingFrame(self.databuffer, tid=tid) + used_len, pdu = self._processIncomingFrame(self.databuffer) if not used_len: return None if pdu: @@ -93,7 +87,7 @@ def processIncomingFrame(self, data: bytes, tid=None) -> ModbusPDU | None: raise exc self.databuffer = self.databuffer[used_len:] - def _processIncomingFrame(self, data: bytes, tid=None) -> tuple[int, ModbusPDU | None]: + def _processIncomingFrame(self, data: bytes) -> tuple[int, ModbusPDU | None]: """Process new packet pattern. This takes in a new request packet, adds it to the current @@ -108,14 +102,9 @@ def _processIncomingFrame(self, data: bytes, tid=None) -> tuple[int, ModbusPDU | used_len, dev_id, tid, frame_data = self.decode(self.databuffer) if not frame_data: return used_len, None - if self.dev_ids and dev_id not in self.dev_ids: - Log.debug("Not a valid slave id - {}, ignoring!!", dev_id) - return used_len, None if (result := self.decoder.decode(frame_data)) is None: raise ModbusIOException("Unable to decode request") result.slave_id = dev_id result.transaction_id = tid Log.debug("Frame advanced, resetting header!!") - if tid and result.transaction_id and tid != result.transaction_id: - return used_len, None return used_len, result diff --git a/pymodbus/server/async_io.py b/pymodbus/server/async_io.py index 28fafadcb..c3a42d2e9 100644 --- a/pymodbus/server/async_io.py +++ b/pymodbus/server/async_io.py @@ -68,14 +68,9 @@ def callback_connected(self) -> None: if self.server.broadcast_enable: if 0 not in slaves: slaves.append(0) - if 0 in slaves: - slaves = [] try: self.running = True - self.framer = self.server.framer( - self.server.decoder, - slaves, - ) + self.framer = self.server.framer(self.server.decoder) # schedule the connection handler on the event loop self.handler_task = asyncio.create_task(self.handle()) diff --git a/pymodbus/transaction.py b/pymodbus/transaction.py index a11ae8505..317d10318 100644 --- a/pymodbus/transaction.py +++ b/pymodbus/transaction.py @@ -232,10 +232,7 @@ def execute(self, request: ModbusPDU): # noqa: C901 self._no_response_devices.append(request.slave_id) # No response received and retries not enabled break - if (pdu := self.client.framer.processIncomingFrame( - response, - tid=request.transaction_id, - )): + if (pdu := self.client.framer.processIncomingFrame(response)): self.addTransaction(pdu) if not (response := self.getTransaction(request.transaction_id)): if len(self.transactions): diff --git a/test/framers/conftest.py b/test/framers/conftest.py index 18720c6b7..c8e80f592 100644 --- a/test/framers/conftest.py +++ b/test/framers/conftest.py @@ -17,15 +17,9 @@ def prepare_is_server(): """Return client/server.""" return False -@pytest.fixture(name="dev_ids") -def prepare_dev_ids(): - """Return list of device ids.""" - return [0, 17] - @pytest.fixture(name="test_framer") -async def prepare_test_framer(entry, is_server, dev_ids): +async def prepare_test_framer(entry, is_server): """Return framer object.""" return FRAMER_NAME_TO_CLASS[entry]( (ServerDecoder if is_server else ClientDecoder)(), - dev_ids, ) diff --git a/test/framers/generator.py b/test/framers/generator.py index 8f5775611..71518c0d5 100755 --- a/test/framers/generator.py +++ b/test/framers/generator.py @@ -23,13 +23,13 @@ def set_calls(): print(f" dev_id --> {dev_id}") for tid in (0, 3077): print(f" tid --> {tid}") - client = framer(ClientDecoder(), [0]) + client = framer(ClientDecoder()) request = ReadHoldingRegistersRequest(124, 2, dev_id) request.transaction_id = tid result = client.buildFrame(request) print(f" request --> {result}") print(f" request --> {result.hex()}") - server = framer(ServerDecoder(), [0]) + server = framer(ServerDecoder()) response = ReadHoldingRegistersResponse([141,142]) response.slave_id = dev_id response.transaction_id = tid diff --git a/test/framers/test_framer.py b/test/framers/test_framer.py index d33d6d468..3f9cd9774 100644 --- a/test/framers/test_framer.py +++ b/test/framers/test_framer.py @@ -19,27 +19,23 @@ class TestFramer: """Test module.""" - def test_setup(self, entry, is_server, dev_ids): + def test_setup(self, entry, is_server): """Test conftest.""" assert entry == FramerType.RTU assert not is_server - assert dev_ids == [0, 17] set_calls() def test_base(self): """Test FramerBase.""" - framer = FramerBase(ClientDecoder(), []) + framer = FramerBase(ClientDecoder()) framer.decode(b'') framer.encode(b'', 0, 0) - dev_id = 2 - framer.encode(b'', dev_id, 0) - assert dev_id in framer.dev_ids + framer.encode(b'', 2, 0) @pytest.mark.parametrize(("entry"), list(FramerType)) async def test_framer_init(self, test_framer): """Test framer type.""" - test_framer.incomming_dev_id = 1 - assert test_framer.incomming_dev_id + assert test_framer @pytest.mark.parametrize( ("func", "test_compare", "expect"), @@ -193,7 +189,7 @@ def test_encode_type(self, frame, frame_expected, data, dev_id, tr_id, inx1, inx """Test encode method.""" if frame == FramerTLS and dev_id + tr_id: return - frame_obj = frame(ClientDecoder(), [0]) + frame_obj = frame(ClientDecoder()) expected = frame_expected[inx1 + inx2 + inx3] encoded_data = frame_obj.encode(data, dev_id, tr_id) assert encoded_data == expected diff --git a/test/framers/test_multidrop.py b/test/framers/test_multidrop.py index 8e26971bc..a0e8f99d6 100644 --- a/test/framers/test_multidrop.py +++ b/test/framers/test_multidrop.py @@ -16,7 +16,7 @@ class TestMultidrop: @pytest.fixture(name="framer") def fixture_framer(self): """Prepare framer.""" - return FramerRTU(ServerDecoder(), [2]) + return FramerRTU(ServerDecoder()) @pytest.fixture(name="callback") def fixture_callback(self): @@ -39,11 +39,6 @@ def test_bad_crc(self, framer): serial_event = b"\x02\x03\x00\x01\x00}\xd4\x19" # Manually mangled crc assert not framer.processIncomingFrame(serial_event) - def test_wrong_id(self, framer): - """Test frame wrong id.""" - serial_event = b"\x01\x03\x00\x01\x00}\xd4+" # Frame with good CRC but other id - assert not framer.processIncomingFrame(serial_event) - def test_big_split_response_frame_from_other_id(self, framer): """Test split response.""" # This is a single *response* from device id 1 after being queried for 125 holding register values @@ -116,11 +111,6 @@ def test_frame_with_trailing_data(self, framer): # We should not respond in this case for identical reasons as test_wrapped_frame assert framer.processIncomingFrame(serial_event) - def test_wrong_dev_id(self): - """Test conincidental.""" - framer = FramerAscii(ServerDecoder(), [87]) - assert not framer.processIncomingFrame(b':0003007C00027F\r\n') - def test_wrong_class(self): """Test conincidental.""" @@ -128,7 +118,7 @@ def return_none(_data): """Return none.""" return None - framer = FramerAscii(ServerDecoder(), []) + framer = FramerAscii(ServerDecoder()) framer.decoder.decode = return_none with pytest.raises(ModbusIOException): framer.processIncomingFrame(b':1103007C00026E\r\n') diff --git a/test/sub_client/test_client_faulty_response.py b/test/sub_client/test_client_faulty_response.py index 6d6966853..93b5d5da1 100644 --- a/test/sub_client/test_client_faulty_response.py +++ b/test/sub_client/test_client_faulty_response.py @@ -15,7 +15,7 @@ class TestFaultyResponses: @pytest.fixture(name="framer") def fixture_framer(self): """Prepare framer.""" - return FramerSocket(ClientDecoder(), []) + return FramerSocket(ClientDecoder()) def test_ok_frame(self, framer): """Test ok frame.""" @@ -24,7 +24,7 @@ def test_ok_frame(self, framer): def test_1917_frame(self): """Test invalid frame in issue 1917.""" recv = b"\x01\x86\x02\x00\x01" - framer = FramerRTU(ClientDecoder(), [0]) + framer = FramerRTU(ClientDecoder()) assert not framer.processIncomingFrame(recv) def test_faulty_frame1(self, framer): diff --git a/test/sub_current/test_transaction.py b/test/sub_current/test_transaction.py index e475ceebc..8511611b7 100755 --- a/test/sub_current/test_transaction.py +++ b/test/sub_current/test_transaction.py @@ -40,10 +40,10 @@ def setup_method(self): """Set up the test environment.""" self.client = None self.decoder = ServerDecoder() - self._tcp = FramerSocket(self.decoder, []) - self._tls = FramerTLS(self.decoder, []) - self._rtu = FramerRTU(self.decoder, []) - self._ascii = FramerAscii(self.decoder, []) + self._tcp = FramerSocket(self.decoder) + self._tls = FramerTLS(self.decoder) + self._rtu = FramerRTU(self.decoder) + self._ascii = FramerAscii(self.decoder) self._manager = SyncModbusTransactionManager(self.client, 3) # ----------------------------------------------------------------------- #