256dpi / arduino-mqtt

MQTT library for Arduino
MIT License
1.02k stars 236 forks source link

v2.4.5 cannot connect to broker #182

Closed joysfera closed 4 years ago

joysfera commented 4 years ago

ESP8266, Arduino core v2.6.1, fairly simple MQTT code (mostly like the provided example) written for arduino-mqtt v2.4.3. Today I have updated the library to v2.4.5 and suddenly it cannot connect to the broker. No other change has been made.

Downgrading to v2.4.3 of the library fixes the issue.

MQTT broker is Mosquitto. No TLS involved.

When the library's connect() fails, the lastError() returns -9, which is "LWMQTT_MISSING_OR_WRONG_PACKET", I suppose.

joysfera commented 4 years ago

Changing the read() back to readBytes() in MQTTClient.cpp fixes the issue. See 2c4f12c1072451cdb7b6fd19f2dfc63f4b9ea830

According to Arduino API the read() does not take any parameters and reads just a single byte so this change in your library does not make sense, anyway. How could you commit and release such thing? Are you using some other Arduino API than the official one?

https://www.arduino.cc/reference/en/language/functions/communication/stream/streamread/

256dpi commented 4 years ago

The issue was related to the blocking the nature of the read. The ESP8266 was not able to provide the data to be read. A delay(1) will yield to the wifi task to read the data. I fixed this in c3fca55 and released v2.4.6. Your code should work now as before.

Most Arduino cores and libs support the buffer based read API. I'd like to use it until there is a reason to switch to the single byte read API. The assumption is simply that the former may potentially be implemented in a more efficient way by the underlying stack.

Edit: Referenced wrong commit.

joysfera commented 4 years ago

I don't get it. What is the difference between readBytes(buffer, maxlen) (a documented API) and read(buffer, maxlen) (an undocumented call that may not be implemented at all) that you prefer to call the latter? Do you have any evidence that it is implemented in a more efficient way or was the note about efficiency related to reading single byte vs reading a block?

https://www.arduino.cc/reference/en/language/functions/communication/stream/streamreadbytes/

256dpi commented 4 years ago

The difference is subtle. readBytes is provided by the Stream class which does not know if a client is connected or disconnected. If the connection fails during a read the new implementation will able to detect this right away and return and error. For most applications this does not make a difference. However if you use LORA and deep-sleep or some other technique that can lead to connection errors the new implementation will allow you to establish a new connection more quickly.

joysfera commented 4 years ago

BTW, delay(0) might be better than delay(1) as it saves time. Or even yield(), to say that you're not waiting for anything, just letting the CPU handle the WiFi stack.

256dpi commented 4 years ago

Yes that's better. I changed it and released v2.4.7.

joysfera commented 4 years ago

Thanks, I'll give it a try. With 2.4.3 I was experiencing connection loss after few hours, hopefully the 2.4.7 will be more stable.