QNEthernet Library - EthernetClient misbehaving

jimmie

Well-known member
I was using the NativeEthernet library and had some intermittent issues with it and the community recommended I use the QNEthernet library instead.

I changed my code to use the QNEthernet library but I am having problems with the display of a webpage. It does not get rendered correctly.

Here is my code:

Code:
void processWebClient()
{
  EthernetClient webClient = webServer.available();
    while (webClient.connected())
    {

...

          webClient.println(F("<td>Server Port (> 1024)</td>"));
          webClient.println(F("<td>"));
          webClient.println(str[1]);
          webClient.println(F("</td>"));
          webClient.println(F("<td>"));
          if (f)
          {
            webClient.println(F("<input id=\"new_myTCPserverPort\" name=\"new_myTCPserverPort\" type=\"text\" onblur=\"check_port(this)\">"));
          }
          webClient.println(F("</td>"));
          webClient.println(F("</tr>"));
          webClient.println(F("<tr>"));
...

    }

}

Are the println statements incorrect?
 
Can you show the complete program? The println calls aren’t necessarily incorrect, but I’d have to see what else you’re doing.
 
Thank you Shawn. The program is too long but I have attached the webserver module. I hope it helps.

I have also attached how the page looks like when it is properly rendered vs. now.
 

Attachments

  • web1.jpg
    web1.jpg
    182.3 KB · Views: 61
  • web2.jpg
    web2.jpg
    124.1 KB · Views: 55
  • webServer.ino
    39 KB · Views: 49
See this post for a pointer to a server example. In the meantime, I will have a look at your module. (Won’t be immediate, necessarily.)
 
Thank you Shawn.

While the issue below may not related, it has to do with the Ethernet connection. It also uses the println command.

I have a networked relay that has 4 relays. With the Native Ethernet library, the code below turns ALL relays off IMMEDIATELY. With the QNEthernet library, it works but is very slow. It turns the first relay off, then second, then third etc.

The code I am using is here:

Code:
void allRelayOFF()
{
  if (setRelayStatus(1, 0))
  {
    relayNONE = false;
    setRelayStatus(2, 0);
    setRelayStatus(3, 0);
    setRelayStatus(4, 0);
  }
  else
  {
    relayNONE = true;
    Serial.println("<<<  Relay: failed to set state OFF for relays");
  }
}

bool setRelayStatus(int relay, int state)
{
  bool res = false;
  char request_str[100] = {0};
  char response_str[1024] = {0};
  int index = 0;

  snprintf(request_str, 100, "GET /state.xml?relay%dState=%d HTTP/1.1", relay, state);

  if (relayClient.connect(ipWebRelay, relayPort))
  {
    //Serial.print("Connected to Webrelay at: ");
    //Serial.println(relayClient.remoteIP());
    // Make a HTTP request:
    relayClient.println(request_str);
    relayClient.println("Authorization: Basic bm9uZTp3ZWJyZWxheQ==");
    relayClient.println();  // end header
    while (relayClient.connected()) {
      if (relayClient.available()) {
        // read an incoming byte from the server and print it to serial monitor:
        response_str[index] = relayClient.read();
        index += 1;
      }
    }
    response_str[index] = '\0';

    // the server's disconnected, stop the client:
    relayClient.stop();

    //Serial.println("Webrelay Response");
    //Serial.println(response_str);
    //Serial.println("Webrelay disconnected.");

    res = true;
  } else {
    // if you didn't get a connection to the server:
    Serial.println("<<<  setRelayStatus: connection failed!");
  }
  return res;
}
 
Shawn:

I have done some more checking and now I am sure there is something wrong in the QNEthernet's library handling of:

Code:
webClient.println

When I took out sections from the code I attached, the page still does not render correctly, but always stops roughly after the same number of "webClient.println" statements.
 
Try client.writeFully instead of print. There’s no guarantee that all the data gets sent otherwise. See https://github.com/ssilverman/QNEthernet#how-to-write-data-to-clients. (Note that I forgot to add to that section that the client class now provides a writeFully implementation, so you don’t need to provide your own.)

Or you could check the print/println return values to see which bytes had not been sent. That’s the same thing writeFully does. See if that helps.
 
Last edited:
Thank you Shawn.

I have tried using writeFully per your earlier recommendations but I must have the format incorrectly.

Can you please show me how to use writeFully for the statements below:

Code:
//----------------------------------------------------------------------------------
          webClient.println(F("<td>Max Hz Dist (in)</td>"));
          webClient.println(F("<td>"));
          webClient.println(str[12]);
          webClient.println(F("</td>"));
          webClient.println(F("<td>"));
          if (f)
          {
            webClient.println(F("<input id=\"new_maxDist2Veh\" name=\"new_maxDist2Veh\" type=\"text\" onblur=\"check_general(this)\">"));
          }
          webClient.println(F("</td>"));
          webClient.println(F("</tr>"));
          webClient.println(F("<tr>"));
//----------------------------------------------------------------------------------
     relayClient.println(request_str);
    relayClient.println("Authorization: Basic bm9uZTp3ZWJyZWxheQ==");
    relayClient.println();  // end header
//----------------------------------------------------------------------------------

Thank you for your time.
 
Replace print with writeFully and println with writeFully, but adding “\r\n”. Note that in HTML, you don’t really need to append those CRLF characters. I’d suggest just using writeFully without the CRLF.

I also just realized I don’t provide a flash string form of writeFully. (The reason being the `write` functions aren't meant for them.) Try removing those F() wrappers.

(Another approach would be for me to to override the print functions, however, those are not overridable in the Print API.)
 
Last edited:
The problem is also that you're sending little chunks one at a time to the stack. I would, even instead of replacing all "print" or "println" with "writeFully", drastically reduce the number of write/print calls by collapsing all the strings and by not using CRLF (i.e. println) where you don't have to. I'm fixing up your attached "webServer.ino" example to show you what I mean.

Note that you didn't attach your full example, so I don't know what any of the types are. For example, I'm assuming signed int for month(), day(), second(), etc. I'm also assuming signed int for sensorSN, numOverHs, curCPUTemp, etc., which means I'm using "%d" in all the "printf" calls. Change to "%u" (for unsigned), "%f", "%g", etc., as necessary.

Sending data over the network needs a slightly different approach than sending to a local serial port.
 
Barring any mistakes I've made, and I don't guarantee there aren't any, I've attached a revamped version of that file you attached (webServer.ino). You might want to do a diff to verify my changes.

Some notes:
1. You may choose to split the strings differently. My goal was just to reduce the number of write calls. Feel free to add more. You'll note I even split across comment lines.
2. In C++, constant string concatenation happens when two quoted strings are next to each other, including across new lines.
3. I tried to keep your formatting as similar as I could.
4. I believe constant strings (const char[] things) are put into program memory so you shouldn't need the F() calls. (I need to verify this.)
5. You could use a loop with tables instead of all the repetitive HTML.
6. Since I don't know what all the variable types are, I'm assuming signed int for month(), day(), second(), etc. I'm also assuming signed int for sensorSN, numOverHs, etc., which means I'm using "%d" in all the "printf" calls. Change to "%u" (for unsigned), "%f", "%g", etc., as necessary.
7. There's a `writeFully` call or two (eg. `webClient.writeFully(sysOutput)`) that I'm not certain will work. `writeFully` only works for uint8_t, uint8_t*, and char*. If `sysOutput` is not one of these (eg. an int or float or something), you'll need to use printf instead. Or, per the code and since we want to ensure all the bytes get written, use `snprintf` per my changes (into the new `printBuf`), and then `writeFully(printBuf)`.
8. I called the file "webSever.ino.cpp", so you might want to change the name, depending on your setup.
 

Attachments

  • webServer.ino.cpp
    37.7 KB · Views: 70
Thank you very much Shawn. You have gone beyond the call and I really appreciate it. I used your code and it works.

IMHO, this is the single most important library needed to exploit the benefits of the Ethernet interface of the Teensy 4.1.

If I may trouble you with one last small request, this time, it is only 3 lines of code that need to be changed. The code below operates a relay board that has 4 relays. With the NativeEthernet library, it was extremely fast, with all four relays coming on or off. With your library, it works, but is very slow where relay 1 works, then a short bit later relay 2 and so on.

Here is the full code:

Code:
void allRelayOFF()
{
  if (setRelayStatus(1, 0))
  {
    relayNONE = false;
    setRelayStatus(2, 0);
    setRelayStatus(3, 0);
    setRelayStatus(4, 0);
  }
  else
  {
    relayNONE = true;
    Serial.println("<<<  Relay: failed to set state OFF for relays");
  }
}

// set relay status
bool setRelayStatus(int relay, int state)
{
  bool res = false;
  char request_str[100] = {0};
  char response_str[1024] = {0};
  int index = 0;

  snprintf(request_str, 100, "GET /state.xml?relay%dState=%d HTTP/1.1", relay, state);

  if (relayClient.connect(ipWebRelay, relayPort))
  {
    //Serial.print("Connected to Webrelay at: ");
    //Serial.println(relayClient.remoteIP());
    // Make a HTTP request:
[COLOR="#FF0000"]    relayClient.println(request_str);
    relayClient.println("Authorization: Basic bm9uZTp3ZWJyZWxheQ==");
    relayClient.println();  // end header[/COLOR]
    while (relayClient.connected())
    {
      if (relayClient.available()) {
        // read an incoming byte from the server and print it to serial monitor:
        response_str[index] = relayClient.read();
        index += 1;
      }
    }
    response_str[index] = '\0';

    // the server's disconnected, stop the client:
    relayClient.stop();

    //Serial.println("Webrelay Response");
    //Serial.println(response_str);
    //Serial.println("Webrelay disconnected.");

    res = true;
  } else {
    // if you didn't get a connection to the server:
    Serial.println("<<<  setRelayStatus: connection failed!");
  }
  return res;
}

Thank you again for your time.
 
Here's an off-the cuff change:

Code:
void allRelayOFF()
{
  if (setRelayStatus(1, 0))
  {
    relayNONE = false;
    setRelayStatus(2, 0);
    setRelayStatus(3, 0);
    setRelayStatus(4, 0);
  }
  else
  {
    relayNONE = true;
    Serial.println("<<<  Relay: failed to set state OFF for relays");
  }
}

// set relay status
bool setRelayStatus(int relay, int state)
{
  bool res = false;
  char request_str[64] = {0};
  char response_str[512] = {0};
  int index = 0;

  snprintf(request_str, 100, "GET /state.xml?relay%dState=%d HTTP/1.1", relay, state);

  if (relayClient.connect(ipWebRelay, relayPort))
  {
    //Serial.print("Connected to Webrelay at: ");
    //Serial.println(relayClient.remoteIP());
    // Make a HTTP request:
    relayClient.writeFully(request_str);
    relayClient.writeFully("Authorization: Basic bm9uZTp3ZWJyZWxheQ==\r\n"
                           "Connection: close\r\n"
                           "\r\n");  // end header
    relayClient.flush();
    while (relayClient.connected())
    {
      int avail = relayClient.available();
      if (avail > 0) {
        int read = relayClient.read(response_str + index,
                                    min(sizeof(response_str) - index - 1, avail));
            // Subtract one from the max remaining size to account for the NUL
        index += read;
        if (index >= sizeof(response_str) - 1) {
          break;
        }
      }
    }
    index = min(index, sizeof(response_str) - 1);
        // Shouldn't really need this line but we don't, in theory, know what
        // read() returned
    response_str[index] = '\0';

    // the server's disconnected, stop the client:
    relayClient.stop();

    //Serial.println("Webrelay Response");
    //Serial.println(response_str);
    //Serial.println("Webrelay disconnected.");

    res = true;
  } else {
    // if you didn't get a connection to the server:
    Serial.println("<<<  setRelayStatus: connection failed!");
  }
  return res;
}

Some changes and notes:
1. Using `writeFully()` when sending data to the client. (I've mentioned in several places on these threads and in a section in the README that one should be aware that if you don't check the return value of your write calls (includes print calls), then there's no guarantee that all the data will get sent.)
2. Calling flush() after writing all the data.
3. I suspect that reading data from the client was also causing some slowdown, so I improved that too.
4. I reduced the size of `request_str` because if you count characters, 100 isn't needed.
5. The code is careful not to overflow the `response_str` buffer.
6. I'm assuming the response isn't that big so I reduced the `response_str` size to 512 (half of what it was).
7. We might not even use the received data, so you could just comment it out, gaining that speed back.
8. I added a "Connection: close" header because the client intends to close the connection right after. (HTTP/1.1)
9. Because you're sending an HTTP/1.1 request, you need a "Host" header. I didn't add that because you might have a better idea of what you need.
10. `allRelayOFF()` error handling could be improved by also checking the return values of all the other `setRelayStatus()` calls. I'd suggest OR'ing the return values, assuming they're all Boolean returns.
11. If you see that the request speed is improved, I'm not certain which of these changes helped. If so, I'm curious which it might be. You could try adding these changes one by one to see which provides the best improvement.
 
Last edited:
Thank you very much Shawn. I have learnt a lot from you.

If I may add a quick note:

The relay code above takes the same time as before (it is not faster). With the NativeEthernet library, the 4 relays switched off immediately, whereas with the QNEthernet library it takes about 1 second for the 4 relays to switch off. For me this extra time is of no practical consequence. The most important attribute is the stability of the library and avoiding getting a Static IP of 0.0.0.0 which is what I had with the nativeEthernet library.

That said, I am concerned about the latency of the library and would appreciate your input. Using println with a client (before you sent the code that fixed it), could not fully render a single html page. What would happen to latency if clients are exchanging thousands of bytes?
 
Whenever you use print/println/write, you **MUST** check the return value. If you send many small packets to the network, it will get a little backlogged and then start not sending data. You can mitigate this by:
1. Always checking the return value from write/print/println and then attempting to resend what didn't get sent. For example, if you try to send 100 bytes but it returns 90, then you'll have to retry sending those last 10 bytes. That's what the writeFully functions are for. They do this work for you.
2. QNEthernet uses an entirely different approach than NativeEthernet for polling the IP stack and for sending data. QNEthernet doesn't use a timer, but NativeEthernet uses a 1ms timer. Etc. I'd like to emphasize (and I've repeated this a bunch of times already): Whenever you use print/println/write, you **MUST** check the return value. That's the only way to guarantee **WITH ANY NETWORK STACK** that all your data gets sent. NativeEthernet will exhibit this too under some circumstances.
3. Also try sending data in larger chunks. That will help.

This has nothing to do with stability, but in how the stack is being used. In the general case the API itself requires you to check the return value of print/println/write and then retrying all data that didn't get sent.

(See: https://forum.pjrc.com/threads/6846...hernet-Library?p=291275&viewfull=1#post291275)
Do you see the "0.0.0.0" issue with QNEthernet?

More questions:
1. Can you post your complete relay example? Maybe there's something there that's relevant. For example, you pointed to only three `println` lines that needed changing, but reading the data takes time and you didn't think it was important, so it's possible there's something important that you're not recognizing is important.
2. If you try requests manually to the relay server, do you notice any delays? How do you know it's not the server?
3. Did you try not reading the response from the server, i.e. commenting it out (because you don't use it)? Does that improve the speed?
4. What happens with and without the flush()? What effect does that have? Does it improve the response time, even slightly?
 
The only two reasons I could see it being slower is either the client.connect() or client.stop(). Connect for obvious reasons could take longer depending on implementations. Stop may be spending extra time in its while loop waiting for the socket to close. Originally I did the same thing, but I did take out the while loop in mine since I didn’t see a need to block while the socket is closing since either way it will close.

May want to look in to adding some elapsedMillis timers at places in your sketch and see where hangups are potentially happening.
 
@vjmuzik you may be right. I have a while loop in stop() because the Arduino spec says the connection timeout is also for that function. I now see the wisdom of your close() function. I’ll look into adding one too.

@jimmie, as a test, can you try modifying stop() in QNEthernetClient.c to remove the while loop that waits for close, and then re-test?
 
Thank you @shawn and @vjmuzik.

I will remove the loop tomorrow and report.

Also, to answer your question, with QNEthernet, I never got an IP of 0.0.0.0 but then this only happened a few times with nativeEthernet after the system has been running a few days and the system restarts due to connectivity loss.

I also want to add that recycling power did not fix the problem of 0.0.0.0 potentially because I was powering the board using a powerful POE inverter, so it may be a capacitance issue where the board never really lost power.
 
I made the changes below but the speed is still the same:

Code:
void EthernetClient::stop() {
  if (conn_ == nullptr) {
    return;
  }

  const auto &state = conn_->state;
  if (state == nullptr) {
    // This can happen if this object was moved to another
    // or if the connection was disconnected
    conn_ = nullptr;
    return;
  }

  if (conn_->connected) {
    // First try to flush any data
    tcp_output(state->pcb);
    EthernetClass::loop();  // Maybe some TCP data gets in
    // NOTE: loop() requires a re-check of the state

    if (state != nullptr) {
      if (tcp_close(state->pcb) != ERR_OK) {
        tcp_abort(state->pcb);
      } 
	  ////else 
	  ////{
        ////elapsedMillis timer;
        ////while (conn_->connected && timer < connTimeout_) {
          // NOTE: Depends on Ethernet loop being called from yield()
          ////yield();
        ////}
      ////}
    }
  }

  conn_ = nullptr;
}

===========================

I also want to to report that I traced the IP 0.0.0.0 to a hardware (not a software) issue. The Ethernet pins are too close to the Ethernet chip on the Teensy 4.1 and because I am using a custom carrier board, I needed to solder the pins from above where they are too close to the chip so I must have shorted something. When I tried a different Teensy 4.1 with the same carrier board but using the OSH connector, an IP was immediately picked and everything worked fine. This leads me to believe that the problem is related to my hardware rather than the NativeEthernet library and it could be that the chip failed so the library was never able to get an IP. I am still not sure why both libraries did not detect a chip error though?
 
@jimmie, in addition to commenting out that while loop in EthernetClient::stop(), did you also have a call to client.flush() after writing all the data? stop() flushes too; I'm just wondering if it will have an effect.

Update: I just added `EthernetClient::close()` to the library, so use that (`client.close()`) instead of `client.stop()`. It's identical to what you removed from `stop()`, above.

Question: How much of a delay between the relay changes are we talking? You said use of NativeEthernet caused all the relays to change at nearly the same time. What is the delay between the relays when you use QNEthernet? Are we talking milliseconds or hundreds of milliseconds?
 
Last edited:
@shawn. Thanks will try it.

To answer the delay question, with QNEthernet, it took about 850 ms to turn all 4 relays off.
 
What is the delay between the relays (this was my question, not the total time)? Sounds like about 250ms?

Update: While I think about why this is (it may have something to do with the 250ms TCP timer, but I'm not certain yet), perhaps try putting all relay requests into the same connection. i.e. pipeline them without "Connection: close". Then there's no setup/teardown time for each relay.
 
Last edited:
Good Afternoon @shawn

I used the code below both for QNEThernet and NativeEthernet. The times in ms are as below:

------------------------------------------
QNEthernet
------------------------------------------
Time [1] = 365

Time [2] = 250

Time [3] = 250

Time [4] = 250
------------------------------------------
NativeEthernet
------------------------------------------
Time [1] = 4

Time [2] = 3

Time [3] = 4

Time [4] = 4
------------------------------------------


Code:
void allRelayOFF()
{

  long startTime[5];
  long endTime[5];

  startTime[1] = millis();
  if (setRelayStatus(1, 0))
  {
    relayNONE = false;
    endTime[1] = millis();
    startTime[2] = millis();
    setRelayStatus(2, 0);
    endTime[2] = millis();
    startTime[3] = millis();
    setRelayStatus(3, 0);
    endTime[3] = millis();
    startTime[4] = millis();
    setRelayStatus(4, 0);
    endTime[4] = millis();
  }
  else
  {
    relayNONE = true;
    Serial.println("<<<  Relay: failed to set state OFF for relays");
  }
  //---------------------------------------------------------
  Serial.print("Time [1] = ");
  Serial.println(endTime [1] - startTime[1]);
  Serial.print("Time [2] = ");
  Serial.println(endTime [2] - startTime[2]);
  Serial.print("Time [3] = ");
  Serial.println(endTime [3] - startTime[3]);
  Serial.print("Time [4] = ");
  Serial.println(endTime [4] - startTime[4]);
 //---------------------------------------------------------  
}
 
Thanks for that. When I have a chance, I'll show you how to pipeline everything properly (well, "mostly" properly, short of writing a complete HTTP client) into one request to avoid TCP setup/tear-down times. I have a hunch that it's lwIP's timer system that's behaving a bit slower than what NativeEthernet is doing.

While waiting for this, try the following change and see what happens for you. In QNEthernet.cpp:EthernetClass::loop(), change "125" to "20" (or "10" if you feel like it) and see what happens. Basically, polls happen much sooner. That may or may not have an effect because there's another TCP timer in there that doesn't refresh more than every 250ms. Try also varying TCP_TMR_INTERVAL by adding `#define TCP_TMR_INTERVAL something-less-than-250` to lwipopts.h (of course, keep it before the final `#endif`). (Alternatively, you can change the #define in tcp_priv.h, around line 120. Whichever you prefer.)

Change those first separately to see if one or the other makes a difference.

Update: See this thread: https://lists.gnu.org/archive/html/lwip-users/2006-08/msg00090.html (but it's really old)

Update 2: Could you add more timing code inside of setRelayStatus() to see which call is taking the longest? Is it setup? Tear-down? Writing data? Etc. This is a useful test case, and I'm hoping any observations we make here can improve the library, if possible.
 
Last edited:
This is an off-the-cuff attempt at pipelining requests. Note that `relayNONE` changes its meaning slightly. You might need a variable with a different meaning. In fact, unless you actually read the server response, you don't know the state of any of the relays.

Code:
// Set status for all relays.
bool setAllRelaysStatus(int state) {
  char buf[128];

  if (relayClient.connect(ipWebRelay, relayPort)) {
    // Make several pipelined HTTP requests
    for (int relay = 1; relay <= 4; relay++) {
      snprintf(buf, sizeof(buf),
              "GET /state.xml?relay%dState=%d HTTP/1.1\r\n"    // 37 + numbers
              "Authorization: Basic bm9uZTp3ZWJyZWxheQ==\r\n"  // 43 bytes
              "Host: %u.%u.%u.%u\r\n"                          // 15-23 bytes
              "\r\n"                                           // 2 bytes
              relay, state,
              ipWebRelay[0], ipWebRelay[1], ipWebRelay[2], ipWebRelay[3]);
      // Note: A "Host" header is required with HTTP/1.1 requests;
      // adjust as needed

      relayClient.writeFully(buf);  // snprintf adds a NUL
      relayClient.flush();

      // Read any data to clear the buffers
      // We SHOULD check the response here, but this is just a demo,
      // not a full client. If knowing the actual result is important
      // then you should use a more complete client implementation.
      while (relayClient.connected()) {
        int toRead = min(relayClient.available(),
                         static_cast<int>(sizeof(buf)));
        if (toRead <= 0) {
           break;
        }
        relayClient.read(buf, toRead);
      }
    }

    // We've sent all the data, so close the connection
    // Note: stop() waits until closed, but close() does not wait
    relayClient.close();

    return true;  // Not actually necessarily successful;
                  // depends on responses
  }

  Serial.println("<<<  setRelayStatus: connection failed!");
  return false;
}

Get the latest version fro GitHub for the new close() function.
 
Back
Top