arduino-libraries / ArduinoModbus

250 stars 119 forks source link

TCP Server in endless loop when client doesn't close #6

Open SokoFromNZ opened 5 years ago

SokoFromNZ commented 5 years ago

Hi guys,

I'm trying to provide values from my MKRZERO + MKR ETH Shield to my home automation server (Loxone Miniserver). I ran into an issue with your example.

You are in a while-loop as long as the client is connected:

...
    while (client.connected()) {
      // poll for Modbus TCP requests, while client connected
      modbusTCPServer.poll();

      // update the LED
      updateLED();
    }
...   

My client (Loxone Miniserver) doesn't close the connection after reading it (I can't change this behaviour!). So I'm stuck here as my MKRZero wants to do different things as well in its void loop().

My idea was now to close the TCP connection from my/Arduino side once everything is done (reads are replied to, writes are written).

WiFiModbusServerLED.ino: Update 1:

...
    // WHILE is now an IF
    if (client.connected()) {
      // poll for Modbus TCP requests, while client connected
      modbusTCPServer.poll();

      // update the LED
      updateLED();

      // CLOSE the connection
      client.stop();
    }
...   

Is this a valid approach to close the connection?

The issue now is that not everything is processed the client wants to do. I reckon this is because the modbusTCPServer.poll() is not in a loop anymore. After investigating the modbusTCPServer.poll() method I think this problem can be solved by giving it a return value.

WiFiModbusServerLED.ino: Update 2:

...
    // WHILE is now an IF
    if (client.connected()) {
      // processing ALL requests until nothing is to do anymore
      while(modbusTCPServer.poll()>0);

      // update the LED
      updateLED();

      // CLOSE the connection
      client.stop();
    }
...   

This has to go of course with this (including all the virtual stuff in all the other files): ModbusTCPServer.cpp: Update 1:

int ModbusTCPServer::poll()
{
  if (_client != NULL) {
    uint8_t request[MODBUS_TCP_MAX_ADU_LENGTH];

    int requestLength = modbus_receive(_mb, request);

    if (requestLength > 0) {
      modbus_reply(_mb, request, requestLength, &_mbMapping);
    }
    return requestLength;
  }
  return -1;
}

Does this make sense to you guys and is it a valid approach?

sandeepmistry commented 5 years ago

Hi @SokoFromNZ,

Have you tried:

...
    // WHILE is now an IF
    if (client.connected()) {
      // poll for Modbus TCP requests, while client connected
     while (client.available()) {
      modbusTCPServer.poll();
    }

      // update the LED
      updateLED();

      // CLOSE the connection
      client.stop();
    }
...   
SokoFromNZ commented 5 years ago

Hi,

I've tried that and it works, thanks! So I can revert my internal changes to your library, great!

Maybe its an idea to change your example though.

Soko

sandeepmistry commented 5 years ago

Hi @SokoFromNZ,

I've tried that and it works, thanks! So I can revert my internal changes to your library, great!

Great, thanks for trying this!

Maybe its an idea to change your example though.

Let's hold off on this for now, it really depends how many Modbus TCP clients have this behaviour. I'll leave this issue open to see if we can get additional feedback on this in the future.

Brianthechemist commented 1 year ago

Not to revive this, but i’m having this same issue with the arduino h7 portenta machine control board communicating with Mach 4 (a cnc controller). Except in this case, the tcp client reports ‘tcp/ip’ connection was closed by remote peer’

is there another way to use this library in a non-blocking manner?