Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Framer do not check ids #2375

Merged
merged 4 commits into from
Oct 13, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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