Unexpected implementation for ModbusSerialClient.connected property

[farzadpanahi]

As we can see here the connected property under ModbusSerialClient implementation is like this:

    @property
    def connected(self):
        """Connect internal."""
        return self.connect()

and here is the connect() method:

    def connect(self):  # pylint: disable=invalid-overridden-method
        """Connect to the modbus serial server."""
        if self.socket:
            return True
        try:
            self.socket = serial.serial_for_url(
                self.comm_params.host,
                timeout=self.comm_params.timeout_connect,
                bytesize=self.comm_params.bytesize,
                stopbits=self.comm_params.stopbits,
                baudrate=self.comm_params.baudrate,
                parity=self.comm_params.parity,
            )
            if self.params.strict:
                self.socket.interCharTimeout = self.inter_char_timeout
            self.last_frame_end = None
        except serial.SerialException as msg:
            Log.error("{}", msg)
            self.close()
        return self.socket is not None

It is so unintuitive that this property rather than returning whether the serial is connected or not, it will actually try to connect if serial is not connected.

I am just really curious to know what what is the reasoning behind this, since for other protocols we do not see the same behavior.

Here is the connected property for tcp :

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

Simple, intuitive and expected. Why not have the same implementation for serial?

[janiversen]

pull requests are welcome.

Be aware that connect() work differently for tcp where it connects to the device, as to the serial where it only opens the serial port. Hence the connect as it is implemented are par5 of the retry mechanism

[farzadpanahi]

I understand that in serial, the notion of connection does not make sense the way that we have it in tcp or other transports.

I want to send a PR but I need to figure out how it is working right now.

That's what I am trying to understand. Was there a specific reason that the property connected was implemented as part of the retry mechanism?

if we change connected simply to this:

    @property
    def connected(self):
        """Check if serial is open."""
        return self.is_socket_open()

and then whoever needs to do retry, can have something like:

while not serial.connected:
    serial.connect()

is this reasonable?

[janiversen]

Retry is within the library, not outside the library! connected() was implemented that way, because that enables automatic retry.

[farzadpanahi]

I searched the whole repo for connected keyword and I do not see it used anywhere other than tests and examples. what am I missing? could you point me in the right direction? could you send me the link to the file where it is used? thanks.

[janiversen]

"git grep" is your friend, I have a lot to do and cannot use a lot of time in teaching you how the code works, sorry.

[farzadpanahi]

Sure.

This is for future reference in case someone sees this ticket and can explain how connected property is referenced and used in this lib.

$ git grep connected
API_changes.rst:- Modbus<x>Client .connect() returns True/False (connected or not)
CHANGELOG.rst:* Transfer parms. to connected client. (#1161)
README.rst:houndreds of devices connected to the application). The advantage of the server is
doc/source/client.rst:A variant of **TCP** that uses encryption and certificates. **TLS** is mostly used when the devices are connected to the internet.
doc/source/client.rst:the timeout it throws an exception. If connected successfully reconnecting later is handled automatically
examples/client_async.py:    assert client.connected
examples/client_custom_msg.py:# using the with context, the client will automatically be connected
examples/client_performance.py:    assert client.connected
examples/client_performance.py:    assert client.connected
examples/contrib/solar.py:    _logger.info("### Client connected")
examples/modbus_forwarder.py:    assert args.client.connected
examples/simple_async_client.py:    # test client is connected
examples/simple_async_client.py:    assert client.connected
examples/simulator.py:    assert client.connected
pymodbus/client/base.py:    def connected(self) -> bool:
pymodbus/client/base.py:            raise ConnectionException(f"Not connected[{self!s}]")
pymodbus/client/base.py:    def callback_disconnected(self, _reason: Exception | None) -> None:
pymodbus/client/base.py:            self.raise_future(my_future, ConnectionException("Client is not connected"))
pymodbus/client/serial.py:    def connected(self):
pymodbus/client/tcp.py:    def connected(self):
pymodbus/client/tls.py:    def connected(self):
pymodbus/client/udp.py:    def connected(self):
pymodbus/client/udp.py:        """Return true if connected."""
pymodbus/client/udp.py:    def connected(self):
pymodbus/server/async_io.py:    def callback_connected(self) -> None:
pymodbus/server/async_io.py:                "Server callback_connected exception: {}; {}",
API_changes.rst:- Modbus<x>Client .connect() returns True/False (connected or not)
CHANGELOG.rst:* Transfer parms. to connected client. (#1161)
README.rst:houndreds of devices connected to the application). The advantage of the server is
doc/source/client.rst:A variant of **TCP** that uses encryption and certificates. **TLS** is mostly used when the devices are connected to the internet.
doc/source/client.rst:the timeout it throws an exception. If connected successfully reconnecting later is handled automatically
examples/client_async.py:    assert client.connected
examples/client_custom_msg.py:# using the with context, the client will automatically be connected
examples/client_performance.py:    assert client.connected
examples/client_performance.py:    assert client.connected
examples/contrib/solar.py:    _logger.info("### Client connected")
examples/modbus_forwarder.py:    assert args.client.connected
examples/simple_async_client.py:    # test client is connected
examples/simple_async_client.py:    assert client.connected
examples/simulator.py:    assert client.connected
pymodbus/client/base.py:    def connected(self) -> bool:
pymodbus/client/base.py:            raise ConnectionException(f"Not connected[{self!s}]")
pymodbus/client/base.py:    def callback_disconnected(self, _reason: Exception | None) -> None:
pymodbus/client/base.py:            self.raise_future(my_future, ConnectionException("Client is not connected"))
pymodbus/client/serial.py:    def connected(self):
pymodbus/client/tcp.py:    def connected(self):
pymodbus/client/tls.py:    def connected(self):
pymodbus/client/udp.py:    def connected(self):
pymodbus/client/udp.py:        """Return true if connected."""
pymodbus/client/udp.py:    def connected(self):
pymodbus/server/async_io.py:    def callback_connected(self) -> None:
pymodbus/server/async_io.py:                "Server callback_connected exception: {}; {}",
pymodbus/server/async_io.py:    def callback_disconnected(self, call_exc: Exception | None) -> None:
pymodbus/server/async_io.py:                    self.callback_disconnected(exc)
pymodbus/transport/transport.py:        self.callback_connected()
pymodbus/transport/transport.py:        self.callback_disconnected(reason)
pymodbus/transport/transport.py:    def callback_connected(self) -> None:
pymodbus/transport/transport.py:        Log.debug("callback_connected called")
pymodbus/transport/transport.py:    def callback_disconnected(self, exc: Exception | None) -> None:
pymodbus/transport/transport.py:        Log.debug("callback_disconnected called: {}", exc)
pymodbus/transport/transport.py:                value.callback_disconnected(None)
pymodbus/transport/transport.py:        """Return true if connected/listening."""
test/sub_client/test_client.py:    assert not client.connected
test/sub_client/test_client.py:    assert client.connected
test/sub_server/test_server_asyncio.py:    connected = None
test/sub_server/test_server_asyncio.py:        _logger.debug("TEST Client connected")
test/sub_server/test_server_asyncio.py:        if BasicClient.connected is not None:
test/sub_server/test_server_asyncio.py:            BasicClient.connected.set_result(True)
test/sub_server/test_server_asyncio.py:        BasicClient.connected = None
test/sub_server/test_server_asyncio.py:        BasicClient.connected = asyncio.Future()
test/sub_server/test_server_asyncio.py:        await asyncio.wait_for(BasicClient.connected, timeout=0.1)
test/sub_server/test_server_asyncio.py:        BasicClient.connected = asyncio.Future()
test/sub_server/test_server_asyncio.py:            await asyncio.wait_for(BasicClient.connected, timeout=0.1)
test/sub_transport/conftest.py:    transport.callback_connected = mock.Mock()
test/sub_transport/conftest.py:    transport.callback_disconnected = mock.Mock()
test/sub_transport/conftest.py:    transport.callback_connected = mock.Mock()

excluding all tests and docs:

pymodbus/client/base.py:    def connected(self) -> bool:
pymodbus/client/base.py:            raise ConnectionException(f"Not connected[{self!s}]")
pymodbus/client/base.py:    def callback_disconnected(self, _reason: Exception | None) -> None:
pymodbus/client/base.py:            self.raise_future(my_future, ConnectionException("Client is not connected"))
pymodbus/client/serial.py:    def connected(self):
pymodbus/client/tcp.py:    def connected(self):
pymodbus/client/tls.py:    def connected(self):
pymodbus/client/udp.py:    def connected(self):
pymodbus/client/udp.py:        """Return true if connected."""
pymodbus/client/udp.py:    def connected(self):
pymodbus/server/async_io.py:    def callback_connected(self) -> None:
pymodbus/server/async_io.py:                "Server callback_connected exception: {}; {}",
pymodbus/client/base.py:    def connected(self) -> bool:
pymodbus/client/base.py:            raise ConnectionException(f"Not connected[{self!s}]")
pymodbus/client/base.py:    def callback_disconnected(self, _reason: Exception | None) -> None:
pymodbus/client/base.py:            self.raise_future(my_future, ConnectionException("Client is not connected"))
pymodbus/client/serial.py:    def connected(self):
pymodbus/client/tcp.py:    def connected(self):
pymodbus/client/tls.py:    def connected(self):
pymodbus/client/udp.py:    def connected(self):
pymodbus/client/udp.py:        """Return true if connected."""
pymodbus/client/udp.py:    def connected(self):
pymodbus/server/async_io.py:    def callback_connected(self) -> None:
pymodbus/server/async_io.py:                "Server callback_connected exception: {}; {}",
pymodbus/server/async_io.py:    def callback_disconnected(self, call_exc: Exception | None) -> None:
pymodbus/server/async_io.py:                    self.callback_disconnected(exc)
pymodbus/transport/transport.py:        self.callback_connected()
pymodbus/transport/transport.py:        self.callback_disconnected(reason)
pymodbus/transport/transport.py:    def callback_connected(self) -> None:
pymodbus/transport/transport.py:        Log.debug("callback_connected called")
pymodbus/transport/transport.py:    def callback_disconnected(self, exc: Exception | None) -> None:
pymodbus/transport/transport.py:        Log.debug("callback_disconnected called: {}", exc)
pymodbus/transport/transport.py:                value.callback_disconnected(None)
pymodbus/transport/transport.py:        """Return true if connected/listening."""

I do not see connected referenced, passed or used anywhere.

And here are references by pycharm. connected is only used in examples and tests:

[janiversen]

You already got an explanation, if you want changes then please submit a pull request.

The code works as it is, it might work differently and most surely especially the sync part might work better. But please do not expect others to make changes on your behalf.

[farzadpanahi]

I am not sure which part of my comments are suggesting that I am expecting others to make changes on my behalf?
I just opened a discussion to see if we can improve the code. My intention is to discuss the issue with the code maintainers to make sure I am not missing something and I am on the right track before sending a PR blindly. This is a very common practice in open-source community. I will send a PR when I get a chance!

[janiversen]

Looking forward to see your PR.

Well you at least expected others to make explanations, after I had tried to explain the issue:
"This is for future reference in case someone sees this ticket and can explain how connected property is referenced and used in this lib."

[farzadpanahi]

That's ok. Your repo your rules.