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.

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

Use libmodbus to decode incoming Modbus TCP messages #254

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Abestanis
Copy link
Contributor

This allows us to better handle multiple Modbus ADUs in a single TCP frame (fixes #253).

libmodbus is better at parsing ADUs and we already use it when we are the Modbus client.

I tested this by sending many commands in a short amount of time and verifying via Wireshark that some of them were sent in one TCP frame. I made sure that OpenPLC was sending one response per request, even for those in one TCP packet.

In addition to the receiving problem, I also noticed that the result of write was ignored, which returns the number of bytes written. If we generate a lot of response messages, we could fill up the kernel buffer and drop some bytes. The new approach calls write in a loop until all bytes have been written or write returns an error.

This allows us to better handle
multiple Modbus ADUs in a single TCP frame.
@thiagoralves
Copy link
Owner

Hi @Abestanis
Sorry for taking so long to look into this. I appreciate your PR, but I would rather avoid changing the entire Modbus implementation at this point. The included libmodbus on OpenPLC is quite old, and I plan to either update that or ditch libmodbus entirely. I have had issues with libmodbus in the past, and feel it is a bit too big (cumbersome might be a better word) for what we actually need in OpenPLC. I guess for the fix you need, simply splitting the received packet into separate Modbus PDUs and processing them individually might do the trick. You can use the frame size field to calculate the message size and split it there.

@LeSpocky
Copy link

I guess for the fix you need, simply splitting the received packet into separate Modbus PDUs and processing them individually might do the trick. You can use the frame size field to calculate the message size and split it there.

This is usually not enough when handling fragmentation in a TCP stream. Packets can be split anywhere, not necessarily at higher level protocol frame borders. In other words: TCP fragmentation might happen inside of a Modbus PDU, so you would get first part of the PDU in one read from the TCP socket, and the second half of the PDU on the next read from the TCP socket. A robust implementation must consider this. (Been there, done that, for other protocols though.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Modbus should accept multiple ADUs per TCP frame
3 participants