Migrate from pyserial and in-tree async serial transport to serialx#2929
Migrate from pyserial and in-tree async serial transport to serialx#2929puddly wants to merge 1 commit intopymodbus-dev:devfrom
Conversation
janiversen
left a comment
janiversen
left a comment
There was a problem hiding this comment.
To be very fair and based on experience, that the tests pass is good and like sufficient for the async code.
But for the sync code, we have over time had so many issues stemming from real life devices, that we must be very careful to throw out that code. Your code removes all the wait for data as well as inter byte timeouts which scares me.
| # Buffer size for a single `read()` syscall when the caller didn't specify how many | ||
| # bytes to read. Larger than any Modbus frame, so one syscall returns whatever's | ||
| # currently in the kernel buffer without truncation. | ||
| DEFAULT_RECV_SIZE = 4096 |
There was a problem hiding this comment.
This is way too big !! this must be synchronized with the cutoff limit in transport.py. The cutoff was added to close huge frames as an attack vector.
| # except serial.SerialException as msg: | ||
| # pyserial raises undocumented exceptions like termios | ||
| except Exception as msg: # pylint: disable=broad-exception-caught | ||
| self.socket.open() |
There was a problem hiding this comment.
Where is the inter_byte_timeout ? this is used to detect broken frames.
| self._wait_for_data() | ||
| result = self.socket.read(size) | ||
| return result | ||
| return self.socket.read(size if size else DEFAULT_RECV_SIZE) |
There was a problem hiding this comment.
This seems to ignore the whole idea of waiting for data, something which is important with low BPS.
This can return 0, which meaning the wait for data must be handled at the calling level, which is not acceptable....all serial special handling must be done at this level.
| ) | ||
|
|
||
| @mock.patch("serial.Serial") | ||
| @mock.patch("serialx.serial_for_url") |
There was a problem hiding this comment.
it looks as if, this is not testing a serial line, but the socket part. If correct the tests should be duplicated to ensure it works for both.
Same goes for the next tests.
|
|
||
| self.serial_write = ( # pylint: disable=attribute-defined-outside-init | ||
| client.transport.sync_serial.write | ||
| client.transport.serial.write |
There was a problem hiding this comment.
client.transport provides to different serial implementations, a sync and an async, not sure if this change just combined them.
| "ruff>=0.15.0", | ||
| "twine>=6.2.0", | ||
| "types-Pygments", | ||
| "types-pyserial", |
There was a problem hiding this comment.
This cannot be true, does serialx not have a typed API ???
2 participants
Implementation of #2928.
This PR migrates from pyserial to serialx. As part of the change, I've dropped the in-tree asyncio adapter in favor of serialx's native serial code. serialx does not use polling on any platform or for either API so I've removed a few tests that referenced it.
Two major bugfixes:
Serial.close()within the event loop. This is a blocking operation for thesocket://URI scheme and can block almost indefinitely withos.close()on Linux, especially at low baudrates. For code running in a shared event loop (like in Home Assistant), this will cause issues. Only callconnection_lostafter the serial port is actually closed home-assistant-libs/pyserial-asyncio-fast#16 has more context.Unrelated, but I think a codepaths using
await asyncio.sleep(0.1)may be due to a race between opening a transport andconnection_madebeing called.