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

Fix writing to serial (rs485) on windows os. #2191

Merged
merged 4 commits into from
May 31, 2024
Merged

Fix writing to serial (rs485) on windows os. #2191

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
c71423e
Fix writing to serial (rs485) on windows os.
andrew-harness May 8, 2024
6bcb1de
Merge branch 'dev' into dev
janiversen May 31, 2024
241a7ff
update PR.
janiversen May 31, 2024
4aa44fc
real windows problem.
janiversen May 31, 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
11 changes: 10 additions & 1 deletion pymodbus/transport/serialtransport.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,17 @@ def close(self, exc: Exception | None = None) -> None:
def write(self, data) -> None:
"""Write some data to the transport."""
self.intern_write_buffer.append(data)
if not self.poll_task:
if not self.poll_task and os.name != "nt":
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@andrew-harness andrew-harness May 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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 !

Copy link
Contributor Author

@andrew-harness andrew-harness May 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds correct, please update the PR

self.async_loop.add_writer(self.sync_serial.fileno(), self.intern_write_ready)
elif not self.poll_task and os.name == "nt":
self.poll_task = self.async_loop.create_task(self.write_task())

async def write_task(self):
"""Write data to the serial port."""
while self.intern_write_buffer:
data = self.intern_write_buffer.pop(0)
self.sync_serial.write(data)
await asyncio.sleep(0) # Yield control to the event loop

def flush(self) -> None:
"""Clear output buffer and stops any more data being written."""
Expand Down