Use libmodbus to decode incoming Modbus TCP messages

[Abestanis]

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.

[thiagoralves]

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]

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.)

[Abestanis]

Hey @thiagoralves, no worries and sorry for the slow response, I've been quite busy the last few months.

I have had issues with libmodbus in the past, and feel it is a bit too big ...

I get what you are saying but consider this: It might seem like a big black box of code that you're not sure to trust, but it is much more likely to handle edge cases like #253, because it has been used by many people already. Implementing everything yourself is an option but you are also more likely to get things wrong and not notice it. Additionally, I think it can be problematic trying to re-implement everything. The main goal of this project is to provide an awesome PLC platform, and Modbus is just one of the underlying building blocks whose implementation should be left to those who want to focus and spend resources on providing a good Modbus implementation, which in this case is libmodbus.

I guess for the fix you need, simply splitting the received packet into separate Modbus PDUs and processing them individually might do the trick.

I completely agree with LeSpocky about this, it won't be enough. TCP is a streaming protocol and makes no guarantees about packet boundaries at all. The behavior of not splitting Modbus packets between TCP frames we're seeing right now is an implementation detail of the TCP stack of my operating system, which could change at any time and might be different on another operating system.

Now we could change our modbus parser to only read enough data for the modbus header first, parse that, then only read the number of bytes as indicated in the header, but don't fail on incomplete packages and instead return the number of bytes that need to be read for the complete packet instead, and then loop until we received enough data like libmodbus is doing here. But this would be a lot of work and also basically means we would be changing the entire Modbus implementation, which we are both trying to avoid. With the current approach we have minimal changes to our code but still benefit from the battle proven code from libmodbus.

So TLDR, I would ask you to please reconsider your opinion on using libmodbus to solve this problem. Implementing the correct packet parsing manually is going to require too much work and I am unfortunately unable to spend the time that would be required for this. I've been running a controller with this patch for the last three months now and it has completely fixed the issue described in #253.

[thiagoralves]

Okay, I'm convinced! :) I'll merge the changes, and if things go south we can always revert it back. Thanks for working on this @Abestanis

[thiagoralves]

However @Abestanis correct me if I'm wrong, I just skimmed through your code changes, and it doesn't seem to be connecting the libmodbus parsing with the OpenPLC buffers. You're still calling the processModbus function, just avoiding to write the response of it back to the requester. Then you're creating a modbus socket using libmodbus and just providing a plain response. How is that tied with the actual state of internal registers and coils?

[Abestanis]

Okay, I'm convinced! :) I'll merge the changes, and if things go south we can always revert it back.

Awesome, thanks you! 🎉

You're still calling the processModbus function, just avoiding to write the response of it back to the requester

The changes in processMessage are not related to the main problem of #253, sorry for the confusion, maybe I should have split it into two PRs. They solve this potential problem:

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.

The actual fix to #253 is as follows:

Instead of just calling listenToClient, which just calls read on the client_fd and expects to read one whole message into the buffer but actually just reads up to NET_BUFFER_SIZE, we now:

1. Construct and initialize a modbus context, which contains the parsing machinery of libmodbus:
   

   OpenPLC_v3/webserver/core/server.cpp

   Line 226 in 5bf7fea

   ctx = modbus_new_tcp(NULL, 502); // The IP and port don't matter

2. We configure the context to read from our client_fd socket.
   

   OpenPLC_v3/webserver/core/server.cpp

   Line 227 in 5bf7fea

   modbus_set_socket(ctx, client_fd);

3. Instead of listenToClient we then call modbus_receive, which reads from our client_fd until it has read one complete modbus message into the buffer.
   

   OpenPLC_v3/webserver/core/server.cpp

   Line 239 in 5bf7fea

   messageSize = modbus_receive(ctx, buffer);

modbus_receive eventually ends up here which is the main loop that handles reading the modbus packet.

How is that tied with the actual state of internal registers and coils?

It's not, that part is handled in processMessage and processModbusMessage the same way as before. The only difference is reading the message into buffer.