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 3 commits 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.
There was a problem hiding this comment.
The old implementation effectively called .read(None), which returned everything in the kernel buffer, so the old code technically had no limit at all. I've reduced it to 1024.
| # 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.
There was a problem hiding this comment.
It moved into the constructor, just a few lines up:
parity=self.comm_params.parity,
exclusive=True,
+ inter_byte_timeout=self.inter_byte_timeout,
)
- self.socket.inter_byte_timeout = self.inter_byte_timeoutThere was a problem hiding this comment.
I also think this may be worth noting: inter_byte_timeout in POSIX has a resolution of 0.1s so I think this feature always effectively sets inter_byte_timeout = 0, since t0 would be less than 0.1s for any baudrate higher than 66 baud.
On Windows, it has a higher resolution and work as intended.
| 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.
There was a problem hiding this comment.
I don't think behavior has changed.
The old function read all of the buffered data, if there is any. If there was none, it busy polled for up to self.comm_params.timeout_connect seconds until some data arrived. If nothing arrived, it returned an empty string. This is exactly how serial.read(max_size) works if you pass read_timeout=self.comm_params.timeout_connect, to the serial class constructor.
There was a problem hiding this comment.
If it's simpler or if I'm not understanding how read() is supposed to work, I can just restore the old behavior exactly, all of the functions are the same.
| ) | ||
|
|
||
| @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.
There was a problem hiding this comment.
serial_for_url is the dispatcher for all serial backends. Directly creating Serial objects isn't recommended because it is the platform-native serial constructor and will not work for other types of serial ports (e.g. RFC2217, ESPHome, TCP).
|
|
||
| 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.
There was a problem hiding this comment.
sync_serial what the serialtransport.py called the Serial object attached to the transport. In serialx, we expose it as just serial, for backwards compatibility. Ideally, the test should not touch the sync serial API on the async transport. We only expose it for backwards compatibility.
| "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 ???
There was a problem hiding this comment.
Complete types are provided in the serialx package, there's no need to install a second one.
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.