-
Notifications
You must be signed in to change notification settings - Fork 949
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.
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
Allow configuring pyserial hardware RS485 settings #2460
base: dev
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a formal review, just a fast comment, but a very important one.
pymodbus/transport/transport.py
Outdated
@@ -98,6 +102,9 @@ class CommParams: | |||
parity: str = '' | |||
stopbits: int = -1 | |||
|
|||
# RS485 | |||
rs485_settings: RS485Settings | None = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This fails is serial is not installed.
Pymodbus must work without serial installed.
bda50b7
to
8cf52c6
Compare
Great idea but still has one of the issues from #2205 - this may not work properly when used as async. The way pymodbus implements the async side of the library may cause multiple calls to write in the underlying |
0936647
to
c704a8f
Compare
c704a8f
to
b7360e3
Compare
It seems problematic to support the userspace sleep techniques that is using, probably best to defer that for now.
It's seems there's some sort of windows support as well here that this should work with. |
Yes, that's true. It uses time.sleep, which is really bad in async code. I didn't mean to use that class, but why it's been added. And it's because sometimes there's a need to control the RTS pin from the software instead of leaving it to the hardware implementation. Also like I've described in my previous comment, the hardware RTS control may not be desired here anyway, as it is highly OS/hardware dependant. It may cause invalid RTS states during frame transmission. The RTS control should be implemented in the pymodbus directly, as it knows when the frame starts and where it ends (the underlying Serial instance is not aware of that, it just gets some bytes to write, which may not even be a complete frame at once). Unfortunately I haven't had time to come with my own proposal in form of the PR :(. |
Aren't there cases where settings rs485 hardware options like this that would potentially be useful though? |
They are useful, but the main issue I see with going with this implementation is uncertainty of the RTS behaviour while frame is being sent. It's because neither pymodbus, not pyserial is in full control of the write operation in async, as it's the asyncio loop that schedules writes: when and how much data to write. You don't have any guarantees that the frame is going to be written with a single Serial.write. And if it's not in a single write, then the hardware may toggle the RTS pin because from the hardware standpoint it finished writing some data it was requested to write. The hardware is not aware when the frame really ends. |
AFAIU it's really the kernel and lower level hardware/drivers that determine the actual execution of these sort of operations with userspace largely delegating to the lower level layers. I think there's also a 4096 byte write buffer in the kernel for serial ports. Additionally pymodbus should never write more than one modbus frame at a time before waiting for a response from my understanding with some limited exceptions like broadcasts(due to a transaction execution lock, and for our projects we also implement a priority queue with additional locking on top of this for scheduling high priority modbus operations ahead of low priority operations). I would expect in practice that modbus frames would be effectively written in their entirety to the kernel buffer since they are much smaller than the kernel buffer. Also the event loop in general will execute any CPU bound code paths until competition(like generating a modbus frame) unless waiting for an IO bound operation to complete(i.e. like a serial file descriptor read/write, although in practice likely the event loop will only wait on a read due to kernel write buffering). |
The async loop also cuts cpu bound methods, the only guarantee is that it does not cut a python statement due to the python global lock. |
We really need to define the use case....in theory it sounds good, but most usb rs485 converters handle the dtr, so does the rs485 piggy back I use on my rp4, so maybe the most generic solution is to document the problem and suggest to use a converter that does it automatically. The current PR, does not warn when an OS does not support setting RS485. Calling send directly, means blocking, which is bad when the app uses multiple clients. |
I think the use case is the same as for pyserial, to apply the rs485 configuration settings to the port.
In at least some cases pyserial will AFAIU, IMO best to just leave it delegated to pyserial as it's going to be a bit tricky to cover all cases. |
That sounds more like the solution, why do you need to apply rs485 settings to the port, when most usb converters and piggy backs handles it automatically ? I think you are right that it is very complicated to get right without severe side effects, hence my suggestion to update the documentation instead. |
Just for fun I tried it on one of my mac minis with a M2 Cpu....no errors until open was called, then it broke, with an illegal action on port. Seems the usb converter I used, creates/uses a pseudo port, which clearly does not like ioctl......this is all something we have to document with this PR, in order to avoid issues. |
An alternative solution, for those who need this speciality, is to adapt socat and do whatever they need done with the serial port that way pymodbus stays clean, and users have even more freedom. |
Just for any ports that actually need it configured.
AFAIU it's fairly driver/chipset specific even for native serial ports. Where should I add docs for the option?
That seems quite complicated. I mean this is just a pass-through option(like the various other pyserial params) that's forwarded to pyserial by pymodbus so it shoudn't cause much in the way of maintainability issues. |
it needs to go into the client/server documentation at least....but it is NOT about documenting the option, but about how to see if works, and if not what are probably causes. Please be aware I am far from convinced it is a complexity we want to have in pymodbus.
Why is that complicated ? I do it frequently to test specific protocol problems. It is a lot more than just a pass through the option makes pymodbus responsible for potential problems from a user pow. And that is what I want to avoid. If the pull request is turned into something where e.g, @yawor nods and says it a stable and good solution, I would be more convinced. |
Is this pull request still being worked on ? |
I'm not really sure what's still needed here, this was really only designed to pass through the rs485 params to pyserial. |
Then please read what was written! @yawor raised a serious concern that needs to be addressed, But most importantly, passing the class is not enough
Please remember this is not just a pass through, pymodbus becomes responsible (users will file issues here and not in pyserial), and before accepting that, the raised point (here and in earlier messages) must be addressed |
I thought I had responded to those already, was there something important that I hadn't?
I can add docs, I'm thinking for Linux I would mostly just link to the kernel docs and for windows link to the win32 serial docs and say this option is for configuring these device settings as needed.
It does call the ioctl's when I was testing.
As much as pyserial does AFAIU, I don't really have a windows setup for testing though.
Technically pyserial is itself also just passing the setting through to the kernel drivers using the ioctl's AFAIU, so probably most issues would be kernel/hardware related if anything. |
Well that is something that needs to be documented, test in the test harness. ioctl on windows work differently and often not work. and I still do not see the reason, since most rs485 converters this automatically. Seems you did not reread the messages, you never answered the problem about how the handling could cut messagees. |
I mean, this is just exposing the port settings for any that do.
I don't really see how it would be an issue, see this comment. |
That explains very little, if the data is still in the driver and the pin is lowered, physical sending will be broken and this PR turns it into pymodbus responsibility. I am still to see a board that need manual signal setting, . It would be a lot easier to add a documentation warning, that pymodbus expects the driver/hardware to take care of correct signal handling and we only test with usb converters. I just tested my raspberry pi with 3 different boards, to find another problem, all worked like a charm with v3.8.2 What I still do not understand, do you have a real problem or is it just a feature you want? |
Loosely based on #2205 but is designed to configure the native rs485 serial port settings via ioctl's rather than using the weird RS485 subclass that appears to try to implement some sort of software based rs485 support.