Fix writing to serial (rs485) on windows os.

[andrew-harness]

The issue with the write method on Windows is that it's trying to use self.sync_serial.fileno(), which doesn't work on Windows because the pySerial library doesn't provide a valid file descriptor.

To fix this, you can modify the write method to directly write to the serial port without using asyncio's add_writer method.

[janiversen]

Tests are missing ! the transport part have a 100% coverage and we would like to keep that.

[janiversen]

The if/elif is not very logical it seems you could simplify it a lot.

Secondly I am still not convinced this is the right way, at least please explain why CI that also runs on windows works.

[andrew-harness]

The if/elif is checking for the platform type. This could also be done with the platform library to be a bit more specific and check if it is Windows. Agreed could be simplified.

There also may be a more cross-platform way. I didn't spend much time on it as I just wanted to get it working for a few tests. This could be closed and just submitted as an issue for further investigation.

I'm not exactly sure why the CI is passing for windows but not working on real hardware. I would assume it is because the CI doesn't test real hardware but a NULLMODEM_HOST which may be behaving differently than hardware device. I did run the same test code (except port name) on Linux with no issues.

Here is the Exception that I get when running on Windows, Python 3.12.0:

Exception has occurred: UnsupportedOperation       (note: full exception trace is shown but execution is paused at: _run_module_as_main)
fileno
  File "C:\path-to-source\.venv\Lib\site-packages\pymodbus\transport\serialtransport.py", line 59, in write
    self.async_loop.add_writer(self.sync_serial.fileno(), self.intern_write_ready)
                               ^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\path-to-source\.venv\Lib\site-packages\pymodbus\transport\transport.py", line 393, in send
    self.transport.write(data)  # type: ignore[attr-defined]
    ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\path-to-source\.venv\Lib\site-packages\pymodbus\client\base.py", line 170, in async_execute
    self.send(packet)
  File "C:\path-to-source\icm.py", line 209, in read_register
    rr = await self.client.read_input_registers(register.value, 1, slave=self.slave_id)
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\path-to-source\icm.py", line 268, in read_current_date
    return await self.read_register(self.InputRegisters.CURRENT_DATE)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\path-to-source\icm.py", line 347, in main
    icm450.current_date = await icm450.read_current_date()
                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\base_events.py", line 664, in run_until_complete
    return future.result()
           ^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\runners.py", line 118, in run
    return self._loop.run_until_complete(task)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "C:\Python312\Lib\asyncio\runners.py", line 194, in run
    return runner.run(main)
           ^^^^^^^^^^^^^^^^
  File "C:\path-to-source\icm.py", line 371, in <module>
    asyncio.run(main())
  File "C:\Python312\Lib\runpy.py", line 88, in _run_code
    exec(code, run_globals)
  File "C:\Python312\Lib\runpy.py", line 198, in _run_module_as_main (Current frame)
    return _run_code(code, main_globals, None,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
io.UnsupportedOperation: fileno

[janiversen]

The high level tests uses null modem, which basically bypasses pyserial, but there are low level tests that uses pyserial (transport itself), however they do NOT use the serial interface but the socket interface, and that might be the difference.

Apologies if I come across as a bit negative, but the problem is that I have to maintain the code long term so while I look positive at (nearly) any PR, I give a feedback that ensures it is not making maintenance harder.

Precise comments:

• if / elif, as you admitted it can be simplified, this is important because in a year from now I (or someone else) will start thinking what is the reason that it is complex, what have I overlooked...

• Cross platform: this PR should not be closed, it solves a problem, we just need to get it right. A simplification would be to only use the create_task you suggest, and remove the add_writer totally....I am all in a favor of that.

So to sum up:

• please simplify the if / elif
• I suggest to remove add_writer in total and only use your create task.
• Please add tests to ensure the coverage is 100% (pytest --cov or just check the CI run)

I look forward to review a new version from you...I hope you can see that I am positive and your PR have value !

[andrew-harness]

I looked into this a bit more. It appears if it is windows it should set the poll_task which in the intern_write_ready does not use the fileno() to write.

In setup it is specifically setting the poll_task on windows.

    def setup(self) -> None:
        """Prepare to read/write."""
        if os.name == "nt" or self.force_poll:
            self.poll_task = asyncio.create_task(self.polling_task())
            self.poll_task.set_name("SerialTransport poll")
        else:
            self.async_loop.add_reader(self.sync_serial.fileno(), self.intern_read_ready)
        self.async_loop.call_soon(self.intern_protocol.connection_made, self)

It looks like what was happening was the write function was being called before the setup function is ran.

I put a break point in the write function at the add_writer line and noticed that it was being hit. Which according to the setup function it shouldn't be since it is Windows. I then added a break point to the setup function and noticed that the write function is called once before the setup function is ran.

Adding a timeout after the connect and before a read_input_registers (or other modbus operation) allowed the setup function to finish before the write operation. I then did not receive any exceptions.

Looking at the create_serial_connection you can see that the transport.setup is scheduled to run once the event loop becomes idle. This means that read and write operations can happen before the setup function is run.

Calling the setup function to run immediately mitigates this issue.

async def create_serial_connection(
    loop, protocol_factory, *args, **kwargs
) -> tuple[asyncio.Transport, asyncio.BaseProtocol]:
    """Create a connection to a new serial port instance."""
    protocol = protocol_factory()
    transport = SerialTransport(loop, protocol, *args, **kwargs)
    transport.setup()
    # loop.call_soon(transport.setup)
    return transport, protocol

I think my original changes should be rejected and this change be implemented instead or the connected function
(that is typically awaited after client creation) wait for the connection_made handler to finish, which is called from the setup function.

Thoughts?

[janiversen]

Sounds correct, please update the PR