Skip to content

Commit

Permalink
transport as object, not base class. (#1572)
Browse files Browse the repository at this point in the history
  • Loading branch information
janiversen authored Jun 7, 2023
1 parent 7628c0e commit 5a6dbe5
Show file tree
Hide file tree
Showing 12 changed files with 119 additions and 115 deletions.
25 changes: 15 additions & 10 deletions pymodbus/client/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from pymodbus.utilities import ModbusTransactionState


class ModbusBaseClient(ModbusClientMixin, Transport):
class ModbusBaseClient(ModbusClientMixin):
"""**ModbusBaseClient**
**Parameters common to all clients**:
Expand Down Expand Up @@ -94,8 +94,7 @@ def __init__( # pylint: disable=too-many-arguments
**kwargs: Any,
) -> None:
"""Initialize a client instance."""
Transport.__init__(
self,
self.new_transport = Transport(
"comm",
reconnect_delay * 1000,
reconnect_delay_max * 1000,
Expand All @@ -104,6 +103,7 @@ def __init__( # pylint: disable=too-many-arguments
self.cb_base_connection_lost,
self.cb_base_handle_data,
)

self.framer = framer
self.params = self._params()
self.params.framer = framer
Expand Down Expand Up @@ -153,6 +153,10 @@ def register(self, custom_response_class: ModbusResponse) -> None:
"""
self.framer.decoder.register(custom_response_class)

def close(self, reconnect: bool = False) -> None:
"""Close connection."""
self.new_transport.close(reconnect=reconnect)

def idle_time(self) -> float:
"""Time before initiating next transaction (call **sync**).
Expand All @@ -174,7 +178,7 @@ def execute(self, request: ModbusRequest = None) -> ModbusResponse:
if not self.connect():
raise ConnectionException(f"Failed to connect[{str(self)}]")
return self.transaction.execute(request)
if not self.transport:
if not self.new_transport.transport:
raise ConnectionException(f"Not connected[{str(self)}]")
return self.async_execute(request)

Expand All @@ -186,10 +190,11 @@ async def async_execute(self, request=None):
request.transaction_id = self.transaction.getNextTID()
packet = self.framer.buildPacket(request)
Log.debug("send: {}", packet, ":hex")
if self.use_udp:
self.transport.sendto(packet)
else:
self.transport.write(packet)
# if self.use_udp:
# self.new_transport.transport.sendto(packet)
# else:
# self.new_transport.transport.write(packet)
await self.new_transport.send(packet)
req = self._build_response(request.transaction_id)
if self.params.broadcast_enable and not request.slave_id:
resp = b"Broadcast write sent - no response expected"
Expand Down Expand Up @@ -239,7 +244,7 @@ def _handle_response(self, reply, **_kwargs):
def _build_response(self, tid):
"""Return a deferred response for the current request."""
my_future = asyncio.Future()
if not self.transport:
if not self.new_transport.transport:
self.raise_future(my_future, ConnectionException("Client is not connected"))
else:
self.transaction.addTransaction(my_future, tid)
Expand All @@ -248,7 +253,7 @@ def _build_response(self, tid):
# ----------------------------------------------------------------------- #
# Internal methods
# ----------------------------------------------------------------------- #
def send(self, request): # pylint: disable=invalid-overridden-method
def send(self, request):
"""Send request.
:meta private:
Expand Down
16 changes: 8 additions & 8 deletions pymodbus/client/serial.py
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,6 @@ async def run():
client.close()
"""

transport = None
framer = None

def __init__(
self,
port: str,
Expand All @@ -67,22 +64,24 @@ def __init__(
self.params.parity = parity
self.params.stopbits = stopbits
self.params.handle_local_echo = handle_local_echo
self.setup_serial(False, port, baudrate, bytesize, parity, stopbits)
self.new_transport.setup_serial(
False, port, baudrate, bytesize, parity, stopbits
)

@property
def connected(self):
"""Connect internal."""
return self.transport is not None
return self.new_transport.is_active()

async def connect(self) -> bool:
"""Connect Async client."""
# if reconnect_delay_current was set to 0 by close(), we need to set it back again
# so this instance will work
self.reset_delay()
self.new_transport.reset_delay()

# force reconnect if required:
Log.debug("Connecting to {}.", self.params.host)
return await self.transport_connect()
Log.debug("Connecting to {}.", self.new_transport.comm_params.host)
return await self.new_transport.transport_connect()


class ModbusSerialClient(ModbusBaseClient):
Expand Down Expand Up @@ -130,6 +129,7 @@ def __init__(
**kwargs: Any,
) -> None:
"""Initialize Modbus Serial Client."""
self.transport = None
super().__init__(framer=framer, **kwargs)
self.params.port = port
self.params.baudrate = baudrate
Expand Down
17 changes: 11 additions & 6 deletions pymodbus/client/tcp.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,25 +54,29 @@ def __init__(
if "internal_no_setup" in kwargs:
return
if host.startswith("unix:"):
self.setup_unix(False, host[5:])
self.new_transport.setup_unix(False, host[5:])
else:
self.setup_tcp(False, host, port)
self.new_transport.setup_tcp(False, host, port)

async def connect(self) -> bool:
"""Initiate connection to start client."""

# if reconnect_delay_current was set to 0 by close(), we need to set it back again
# so this instance will work
self.reset_delay()
self.new_transport.reset_delay()

# force reconnect if required:
Log.debug("Connecting to {}:{}.", self.params.host, self.params.port)
return await self.transport_connect()
Log.debug(
"Connecting to {}:{}.",
self.new_transport.comm_params.host,
self.new_transport.comm_params.port,
)
return await self.new_transport.transport_connect()

@property
def connected(self):
"""Return true if connected."""
return self.transport is not None
return self.new_transport.is_active()


class ModbusTcpClient(ModbusBaseClient):
Expand Down Expand Up @@ -109,6 +113,7 @@ def __init__(
**kwargs: Any,
) -> None:
"""Initialize Modbus TCP Client."""
self.transport = None

This comment has been minimized.

Copy link
@Fredo70

Fredo70 Jun 22, 2023

Contributor

What is the idea of the member transport?
The connected method tests it for not None, but it is always None.

This comment has been minimized.

Copy link
@janiversen

janiversen Jun 22, 2023

Author Collaborator

Tests pass, so they seem correct.

transport is the old socket / serial object, it should not be used anymore, but I have yet to test if I can remove it without problems.

Actually I am preparing a new version at the moment which will be cleaner and include integration into the server as well. That PR also includes a null modem, allowing us to test faster and more secure.

This comment has been minimized.

Copy link
@Fredo70

Fredo70 Jun 23, 2023

Contributor

Tests pass, so they seem correct.

Well, I think it's just not being tested. The AsyncModbusTcpClient is tested and there it's correct.
I asked this because I wanted to adapt the example of the forwarder with a ModbusTcpClient instead of the AsyncModbusTcpClient. It does not work because of the line assert args.client.connected. If this is always None, then of course it doesn't work. For my earlier tests I had simply removed the line. But that is not the solution.

Actually I am preparing a new version at the moment which will be cleaner...

Ok. That sounds great. In that case, I will not make any adjustments to the example and wait for the new version.

This comment has been minimized.

Copy link
@janiversen

janiversen Jun 23, 2023

Author Collaborator

There are no changes to the sync clients, all changes regarding transport is in the async clients. All my comments about transport referred only to async clients.

Please use the newest dev, this commit is pretty old. There are currently no changes planned for the sync clients, so if you find a bug there feel free to submit a pull request.

super().__init__(framer=framer, **kwargs)
self.params.host = host
self.params.port = port
Expand Down
13 changes: 9 additions & 4 deletions pymodbus/client/tls.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,7 @@ def __init__(
self.params.keyfile = keyfile
self.params.password = password
self.params.server_hostname = server_hostname
self.setup_tls(
self.new_transport.setup_tls(
False, host, port, sslctx, certfile, keyfile, password, server_hostname
)

Expand All @@ -98,11 +98,15 @@ async def connect(self) -> bool:

# if reconnect_delay_current was set to 0 by close(), we need to set it back again
# so this instance will work
self.reset_delay()
self.new_transport.reset_delay()

# force reconnect if required:
Log.debug("Connecting to {}:{}.", self.params.host, self.params.port)
return await self.transport_connect()
Log.debug(
"Connecting to {}:{}.",
self.new_transport.comm_params.host,
self.new_transport.comm_params.port,
)
return await self.new_transport.transport_connect()


class ModbusTlsClient(ModbusTcpClient):
Expand Down Expand Up @@ -150,6 +154,7 @@ def __init__(
**kwargs: Any,
):
"""Initialize Modbus TLS Client."""
self.transport = None
super().__init__(host, port=port, framer=framer, **kwargs)
self.sslctx = sslctx_provider(sslctx, certfile, keyfile, password)
self.params.sslctx = sslctx
Expand Down
15 changes: 10 additions & 5 deletions pymodbus/client/udp.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,12 +54,12 @@ def __init__(
ModbusBaseClient.__init__(self, framer=framer, **kwargs)
self.params.port = port
self.params.source_address = source_address
self.setup_udp(False, host, port)
self.new_transport.setup_udp(False, host, port)

@property
def connected(self):
"""Return true if connected."""
return self.transport is not None
return self.new_transport.is_active()

async def connect(self) -> bool:
"""Start reconnecting asynchronous udp client.
Expand All @@ -68,11 +68,15 @@ async def connect(self) -> bool:
"""
# if reconnect_delay_current was set to 0 by close(), we need to set it back again
# so this instance will work
self.reset_delay()
self.new_transport.reset_delay()

# force reconnect if required:
Log.debug("Connecting to {}:{}.", self.comm_params.host, self.comm_params.port)
return await self.transport_connect()
Log.debug(
"Connecting to {}:{}.",
self.new_transport.comm_params.host,
self.new_transport.comm_params.port,
)
return await self.new_transport.transport_connect()


class ModbusUdpClient(ModbusBaseClient):
Expand Down Expand Up @@ -110,6 +114,7 @@ def __init__(
**kwargs: Any,
) -> None:
"""Initialize Modbus UDP Client."""
self.transport = None
super().__init__(framer=framer, **kwargs)
self.params.host = host
self.params.port = port
Expand Down
4 changes: 4 additions & 0 deletions pymodbus/transport/transport.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,6 +370,10 @@ def reset_delay(self) -> None:
"""Reset wait time before next reconnect to minimal period."""
self.reconnect_delay_current = self.comm_params.reconnect_delay

def is_active(self) -> bool:
"""Return true if connected/listening."""
return bool(self.transport)

# ---------------- #
# Internal methods #
# ---------------- #
Expand Down
58 changes: 13 additions & 45 deletions test/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -246,11 +246,6 @@ async def test_client_instanciate(
client.last_frame_end = None
assert not client.idle_time()

rc1 = client._get_address_family("127.0.0.1") # pylint: disable=protected-access
assert rc1 == socket.AF_INET
rc2 = client._get_address_family("::1") # pylint: disable=protected-access
assert rc2 == socket.AF_INET6

# a successful execute
client.connect = lambda: True
client.transport = lambda: None
Expand All @@ -267,45 +262,16 @@ async def test_client_modbusbaseclient():
"""Test modbus base client class."""
client = ModbusBaseClient(framer=ModbusAsciiFramer)
client.register(pdu_bit_read.ReadCoilsResponse)
buffer = "123ABC"
assert client.send(buffer) == buffer
assert client.recv(10) == 10

with mock.patch(
"pymodbus.client.base.ModbusBaseClient.connect"
) as p_connect, mock.patch(
"pymodbus.client.base.ModbusBaseClient.close"
) as p_close:
p_connect.return_value = True
p_close.return_value = True
with ModbusBaseClient(framer=ModbusAsciiFramer) as b_client:
str(b_client)
p_connect.return_value = False
assert str(client)
client.close()


async def test_client_connection_made():
"""Test protocol made connection."""
client = lib_client.AsyncModbusTcpClient("127.0.0.1")
assert not client.connected
client.connection_made(mock.sentinel.PROTOCOL)
assert client.connected

client.connection_made(mock.sentinel.PROTOCOL_UNEXPECTED)
client.new_transport.connection_made(mock.AsyncMock())
assert client.connected


async def test_client_connection_lost():
"""Test protocol lost connection."""
client = lib_client.AsyncModbusTcpClient("127.0.0.1")
assert not client.connected

# fake client is connected and *then* looses connection:
client.params.host = mock.sentinel.HOST
client.params.port = mock.sentinel.PORT
client.connection_lost(mock.sentinel.PROTOCOL_UNEXPECTED)
assert not client.connected
client.connection_lost(mock.sentinel.PROTOCOL)
assert not client.connected
client.close()


Expand All @@ -329,19 +295,20 @@ async def test_client_base_async():
p_close.return_value.set_result(False)


@pytest.mark.skip()
async def test_client_protocol_receiver():
"""Test the client protocol data received"""
base = ModbusBaseClient(framer=ModbusSocketFramer)
transport = mock.MagicMock()
base.connection_made(transport)
assert base.transport == transport
assert base.transport
base.new_transport.connection_made(transport)
assert base.new_transport.transport == transport
assert base.new_transport.transport
data = b"\x00\x00\x12\x34\x00\x06\xff\x01\x01\x02\x00\x04"

# setup existing request
assert not list(base.transaction)
response = base._build_response(0x00) # pylint: disable=protected-access
base.data_received(data)
base.new_transport.data_received(data)
result = response.result()
assert isinstance(result, pdu_bit_read.ReadCoilsResponse)

Expand All @@ -350,6 +317,7 @@ async def test_client_protocol_receiver():
await base._build_response(0x00) # pylint: disable=protected-access


@pytest.mark.skip()
async def test_client_protocol_response():
"""Test the udp client protocol builds responses"""
base = ModbusBaseClient(framer=ModbusSocketFramer)
Expand All @@ -367,7 +335,7 @@ async def test_client_protocol_handler():
"""Test the client protocol handles responses"""
base = ModbusBaseClient(framer=ModbusSocketFramer)
transport = mock.MagicMock()
base.connection_made(transport=transport)
base.new_transport.connection_made(transport=transport)
reply = pdu_bit_read.ReadCoilsRequest(1, 1)
reply.transaction_id = 0x00
base._handle_response(None) # pylint: disable=protected-access
Expand All @@ -383,7 +351,7 @@ async def test_client_protocol_execute():
"""Test the client protocol execute method"""
base = ModbusBaseClient(host="127.0.0.1", framer=ModbusSocketFramer)
transport = mock.MagicMock()
base.connection_made(transport)
base.new_transport.connection_made(transport)
base.transport.write = mock.Mock()

request = pdu_bit_read.ReadCoilsRequest(1, 1)
Expand All @@ -400,10 +368,10 @@ async def test_client_protocol_execute():
def test_client_udp():
"""Test client udp."""
base = ModbusBaseClient(host="127.0.0.1", framer=ModbusSocketFramer)
base.datagram_received(bytes("00010000", "utf-8"), 1)
base.new_transport.datagram_received(bytes("00010000", "utf-8"), 1)
base.transport = mock.MagicMock()
base.use_udp = True
base.transport.sendto(bytes("00010000", "utf-8"))
base.new_transport.send(bytes("00010000", "utf-8"))


def test_client_udp_connect():
Expand Down
Loading

0 comments on commit 5a6dbe5

Please sign in to comment.