Skip to content

Commit

Permalink
Framer do not check ids (#2375)
Browse files Browse the repository at this point in the history
  • Loading branch information
janiversen authored Oct 13, 2024
1 parent 0b692d3 commit fb974a9
Show file tree
Hide file tree
Showing 12 changed files with 26 additions and 65 deletions.
4 changes: 2 additions & 2 deletions examples/message_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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__}")
Expand Down
2 changes: 1 addition & 1 deletion pymodbus/client/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pymodbus/client/modbusclientprotocol.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
19 changes: 4 additions & 15 deletions pymodbus/framer/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]:
Expand All @@ -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:
Expand All @@ -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
Expand All @@ -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:
Expand All @@ -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
Expand All @@ -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
7 changes: 1 addition & 6 deletions pymodbus/server/async_io.py
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Expand Down
5 changes: 1 addition & 4 deletions pymodbus/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down
8 changes: 1 addition & 7 deletions test/framers/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)
4 changes: 2 additions & 2 deletions test/framers/generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
14 changes: 5 additions & 9 deletions test/framers/test_framer.py
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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
Expand Down
14 changes: 2 additions & 12 deletions test/framers/test_multidrop.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand All @@ -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
Expand Down Expand Up @@ -116,19 +111,14 @@ 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."""

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')
Expand Down
4 changes: 2 additions & 2 deletions test/sub_client/test_client_faulty_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -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."""
Expand All @@ -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):
Expand Down
8 changes: 4 additions & 4 deletions test/sub_current/test_transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

# ----------------------------------------------------------------------- #
Expand Down

0 comments on commit fb974a9

Please sign in to comment.