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.

Sign up for GitHub

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

Support providing framer type Enum or class #2484

Closed
wants to merge 6 commits into from
Closed

Support providing framer type Enum or class #2484

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e758230
Support providing framer type Enum or class
pjkundert Dec 2, 2024
902f3a6
Remove redundancy
pjkundert Dec 4, 2024
21b50dd
Extend support for passing custom Framer classes to client API
pjkundert Dec 4, 2024
2452d07
Be more careful with type checking for framer
pjkundert Dec 4, 2024
1efab8f
Restore some accidentally removed .gitkeep files
pjkundert Dec 4, 2024
87535cd
Don't log error when missing slave ignored
pjkundert Dec 9, 2024
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
12 changes: 8 additions & 4 deletions pymodbus/client/base.py
View file
Original file line number Diff line number Diff line change
@@ -22,7 +22,7 @@ class ModbusBaseClient(ModbusClientMixin[Awaitable[ModbusPDU]]):

def __init__(
self,
framer: FramerType,
framer: FramerType | type[FramerBase],
retries: int,
comm_params: CommParams,
trace_packet: Callable[[bool, bytes], bytes] | None,
@@ -35,9 +35,11 @@ def __init__(
"""
ModbusClientMixin.__init__(self) # type: ignore[arg-type]
self.comm_params = comm_params
if not isinstance(framer, type) or not issubclass(framer, FramerBase):
framer = FRAMER_NAME_TO_CLASS[FramerType(framer)]
self.ctx = TransactionManager(
comm_params,
(FRAMER_NAME_TO_CLASS[framer])(DecodePDU(False)),
framer(DecodePDU(False)),
retries,
False,
trace_packet,
@@ -116,7 +118,7 @@ class ModbusBaseSyncClient(ModbusClientMixin[ModbusPDU]):

def __init__(
self,
framer: FramerType,
framer: FramerType | type[FramerBase],
retries: int,
comm_params: CommParams,
trace_packet: Callable[[bool, bytes], bytes] | None,
@@ -133,7 +135,9 @@ def __init__(
self.slaves: list[int] = []

# Common variables.
self.framer: FramerBase = (FRAMER_NAME_TO_CLASS[framer])(DecodePDU(False))
if not isinstance(framer, type) or not issubclass(framer, FramerBase):
framer = FRAMER_NAME_TO_CLASS[FramerType(framer)]
self.framer: FramerBase = framer(DecodePDU(False))
self.transaction = TransactionManager(
self.comm_params,
self.framer,
1 change: 0 additions & 1 deletion pymodbus/pdu/__init__.py
View file
Original file line number Diff line number Diff line change
@@ -2,7 +2,6 @@
__all__ = [
"DecodePDU",
"ExceptionResponse",
"ExceptionResponse",
"FileRecord",
"ModbusPDU",
]
21 changes: 12 additions & 9 deletions pymodbus/server/async_io.py
View file
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@
from pymodbus.datastore import ModbusServerContext
from pymodbus.device import ModbusControlBlock, ModbusDeviceIdentification
from pymodbus.exceptions import NoSuchSlaveException
from pymodbus.framer import FRAMER_NAME_TO_CLASS, FramerType
from pymodbus.framer import FRAMER_NAME_TO_CLASS, FramerBase, FramerType
from pymodbus.logging import Log
from pymodbus.pdu import DecodePDU, ModbusPDU
from pymodbus.pdu.pdu import ExceptionResponse
@@ -166,9 +166,10 @@ async def server_async_execute(self, request, *addr):
response = await request.update_datastore(context)

except NoSuchSlaveException:
Log.error("requested slave does not exist: {}", request.dev_id)
if self.server.ignore_missing_slaves:
Log.debug("ignored slave that does not exist: {}", request.slave_id)
return # the client will simply timeout waiting for a response
Log.error("requested slave does not exist: {}", request.slave_id)
response = ExceptionResponse(0x00, ExceptionResponse.GATEWAY_NO_RESPONSE)
except Exception as exc: # pylint: disable=broad-except
Log.error(
@@ -201,7 +202,7 @@ def __init__(
ignore_missing_slaves: bool,
broadcast_enable: bool,
identity: ModbusDeviceIdentification | None,
framer: FramerType,
framer: FramerType | type[FramerBase],
trace_packet: Callable[[bool, bytes], bytes] | None,
trace_pdu: Callable[[bool, ModbusPDU], ModbusPDU] | None,
trace_connect: Callable[[bool], None] | None,
@@ -223,8 +224,10 @@ def __init__(
self.handle_local_echo = False
if isinstance(identity, ModbusDeviceIdentification):
self.control.Identity.update(identity)

self.framer = FRAMER_NAME_TO_CLASS[framer]
# Support mapping of FramerType to a Framer class, or a Framer class
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is only the server, if we are to include this feature again, at least it must also support client and sync client.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I think I've generalized this to all servers and clients. I couldn't figure out where in the tests to be able to add an "evil" framer, though...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you look at the test directory, it has a sub directory called framer, which is where framer testing goes. Please do not change existing tests, but add tests.

FramerBase is an internal class which we do not want public, because having it public makes it a lot harder to change, please see my bullet list.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did look at it, but didn't find any tests of framers in a full I/O cycle. I'll look again.

Agreed; I don't think FramerBase should be public -- users can derive "evil" framers from any of the existing public FramerXxx classes.

if not isinstance(framer, type) or not issubclass(framer, FramerBase):
framer = FRAMER_NAME_TO_CLASS[FramerType(framer)]
self.framer = framer
self.serving: asyncio.Future = asyncio.Future()

def callback_new_connection(self):
@@ -273,7 +276,7 @@ def __init__(
self,
context: ModbusServerContext,
*,
framer=FramerType.SOCKET,
framer: FramerType | type[FramerBase] = FramerType.SOCKET,
identity: ModbusDeviceIdentification | None = None,
address: tuple[str, int] = ("", 502),
ignore_missing_slaves: bool = False,
@@ -336,7 +339,7 @@ def __init__( # pylint: disable=too-many-arguments
self,
context: ModbusServerContext,
*,
framer=FramerType.TLS,
framer: FramerType | type[FramerBase] = FramerType.TLS,
identity: ModbusDeviceIdentification | None = None,
address: tuple[str, int] = ("", 502),
sslctx=None,
@@ -403,7 +406,7 @@ def __init__(
self,
context: ModbusServerContext,
*,
framer=FramerType.SOCKET,
framer: FramerType | type[FramerBase] = FramerType.SOCKET,
identity: ModbusDeviceIdentification | None = None,
address: tuple[str, int] = ("", 502),
ignore_missing_slaves: bool = False,
@@ -463,7 +466,7 @@ def __init__(
self,
context: ModbusServerContext,
*,
framer: FramerType = FramerType.RTU,
framer: FramerType | type[FramerBase] = FramerType.RTU,
ignore_missing_slaves: bool = False,
identity: ModbusDeviceIdentification | None = None,
broadcast_enable: bool = False,