Fix issue #308

[jorgecis]

This fix the issue #308 in esp8266 not sure for other boards, we need test it more.

[knolleary]

Hi @jorgecis

the code you've removed existed for a reason. It handles the case where there is a slow connection between the board and the broker, and the response from the broker don't arrive instantly. If you're only testing this on a local broker you won't hit the problem.

What could be happening here is that the tight loop in readByte is hammering the hardware, not giving it a chance to do what it needs to do - we have seen a similar thing it client.loop() is called too quickly.

Instead of your proposed fix, can you add a delay(50) inside the while loop? If that doesn't work, try increasing the delay. We wouldn't want to add too much of an artificial delay to the loop, but it would help understand the problem if it were to fix it.

[jorgecis]

Hi @knolleary

My MQTT server is in the cloud, but is true the connection is via wifi and is fast. I didn't see any issue removing the code, and actually now is more fast. By the way I using TLS.

I don't want add more delay, in the contrary I want remove the delay, f the problem right now is that in the first loop, the function  _client->available() return 5, and the code reads the 5 bytes, then the problem is when the function return 0, at this point they enter in the loop causing the 15 seconds delay ( until timeout), But this is not really necessary, we already read all bytes available.

I saw your point. I think that we need found a better solution.

[knolleary]

Please can you try adding a delay(50) inside the while loop - that will tell us if the underlying problem is because we're calling available too quickly in that loop. I'm not suggesting that will be the final fix, but it will at least allow us to understand the problem better.

A 50ms delay to fix a 15 second delay doesn't feel unreasonable to me.

[stefanbode]

Hi, Knolleary. I have tried the change and also played around with the "fix". A delay before and/or after the .loop() request did not make any change to the behavior to let him run in the timeout.
The change is doing the JOB very well, but I must admit that my MQQT server response quite fast. And I can confirm, that the timeout does not happen ALL the time. Quite often but not always. In my example, I send a 2 mqqt_publich and 3 mqqt_subscribe and then call the .loop() function during startup. Sub and pub go to different locations.

[knolleary]

@stefanbode I'm not clear which change/fix you've tried. There's the change proposed in the original PR that I'm not in favour of as it assumes a good network.

There is then my suggested alternative to simply put a delay in the while loop this pr otherwise removes. (This is not the same as putting a delay in your own code around your calls to client.loop)

Can you clarify exactly what you've tested?

[stefanbode]

I also tried the delay(50) in the while-loop just after the start. It looks like also this solves the problem. This would hint to your "hammering" idea as a root cause. I finally just inserted a yield(); and wow this is doing the same without a large time penalty. You're right. I first did it wrong. Now with the yield() in YOUR code just after the while it works great. I'm using TLS.

// reads a byte into result
boolean PubSubClient::readByte(uint8_t * result) {
   uint32_t previousMillis = millis();
   while(!_client->available()) {
	 yield();
     uint32_t currentMillis = millis();
     if(currentMillis - previousMillis >= ((int32_t) MQTT_SOCKET_TIMEOUT * 1000)){
       return false;
     }
   }
   *result = _client->read();
   return true;
}