knolleary / pubsubclient

A client library for the Arduino Ethernet Shield that provides support for MQTT.
http://pubsubclient.knolleary.net/
MIT License
3.83k stars 1.47k forks source link

publish() needs an idle period to succeed? #435

Open ohjohnsen opened 6 years ago

ohjohnsen commented 6 years ago

On my ESP8266, I've made an application both for publishing temperature/humidity, and for publishing power usage. Norwegian fusebox power monitors blink 1000/2000 times per 1kWh consumed, and by measuring time between each blink, I calculate the current power usage. The blink is measured using a GA1A1S202WP light sensor on pin A0. The measured temperature and humidity and calculated power usage is sent to a Mosquitto MQTT broker on a fixed interval using the SimpleTimer library.

However, I see that if I don't have a delay([some hundreds of milliseconds]); statement right after the publish() call, the messages are not received successfully by Mosquitto, even though publish() returns "1".

Here's my main loop:

void loop() {
    if (!client.connected()) {
        Serial.print("WiFi state: ");
        Serial.println(WiFi.status());
        Serial.print("MQTT connection lost: ");
        Serial.println(client.state());
        reconnect();
    }
    client.loop();
    timer.run();
    raw_lux_sum_value += analogRead(A0);
        // Code for measuring averaged light value, determining fuse box power monitor LED state, and calculating power usage.
}

And here's the publish function being called in the timer callback:

void publishData(float p_temperature, float p_humidity, unsigned int p_power_consumption) {
    // create a JSON object
    // doc : https://github.com/bblanchon/ArduinoJson/wiki/API%20Reference
    StaticJsonBuffer<200> jsonBuffer;
    JsonObject& root = jsonBuffer.createObject();
    // INFO: the data must be converted into a string; a problem occurs when using floats...
    root["temperature"] = (String)p_temperature;
    root["humidity"] = (String)p_humidity;
    root["powerconsumption"] = (String)p_power_consumption;
    root.prettyPrintTo(Serial);
    Serial.println("");
    /*
    {
    "temperature": "23.20",
    "humidity": "43.70",
    "powerconsumption": "500"
    }
    */
    char data[200];
    root.printTo(data, root.measureLength() + 1);
    int success = client.publish(MQTT_SENSOR_TOPIC, data);
    Serial.print("MQTT publish result: ");
    Serial.println(success);
    delay(500);
}

If I remove the delay(500); at the end of the publishData() function, the messages are not received successfully 99.9% of the time by Mosquitto.

giddyhup commented 6 years ago

Can confirm. I ran into the same issue.

ebivan commented 6 years ago

I can confrim that too. But in my case 100ms are enough, maybe even less. I will tray to go further down as mine is running on batteries and goes to deepsleep after publishing the sensor data.

jonasvdd commented 5 years ago

I can confirm this too.

Within my code I just Establish a MQTT connection and publish multiple consecutive messages to various topics. At first i just established a connection to the topic prefix and published them without delay in a loop. I saw that only the first published one arrived at my broker.

As stated above, I introduced a delay of 100ms. By doing so, ~ 90% of the packages arrived correctly. But occasionally, the last packages wouldn't be sent. The succeeding method makes sure all packages will be sent.

The succeeding snippet makes sure that all packages will be sent, without introducing a hard coded delay. This is achieved by dis- and reconnecting each iteration to the broker. It took for sending 10 messages of 31 bytes ~ 4-5 seconds. Which is quite much. But all packages will arrive. So if you want to go low power, I would propose to take the delay instead of this method.

while (processed_size < total_recv_size) {
    ...
    // try to connect to the topic
    while (!mqtt.connect(mqtthelper.topic_str)) {
        SerialMon.println(" === MQTT NOT CONNECTED === ");
        delay(100);
    }
    mqtt.publish(mqtthelper.topic_str, payload, payload_size + TIMESTAMP_BYTES);
    mqtt.disconnect();     // disconnect explicitly
    ...
}

(sorry for bad English, not a native speaker)

ghmartin77 commented 5 years ago

Seeing the same here, especially if going to deep sleep without a delay. This is probably due to the fact that the TCP stack implementation lwIP decides on its own, when it's time to send out data in its buffers. Fixed the problem for me by introducing some code into WiFiClient.cpp/.h and ClientContext.h

WiFiClient.h (in the "public:" section):

void waitUntilAllDataSent();  

WiFiClient.cpp:

void WiFiClient::waitUntilAllDataSent() {
    return _client->waitUntilAllDataSent();
}

ClientContext.h ("public:" section):

inline void waitUntilAllDataSent() {
    while ((_datasource && _datasource->available()) || tcp_sndbuf(_pcb) < _sndbufsize) {
            yield();
        }
    }

ClientContext.h ("private:" section):

size_t _sndbufsize;

ClientContext.h (last line of c'tor):

_sndbufsize = tcp_sndbuf(pcb);

In my code I do the following after publish():

        mqttWiFiClient.flush();
        mqtt.loop();
        mqttWiFiClient.waitUntilAllDataSent();
jonasvdd commented 5 years ago

Since I work with the TinyGSM library, which uses pubsubclient for MQTT messages, I just had to add the client.flush() line after publishing the message.

This is a cleaner way to solve this than my initial proposal. . It also solves the delay overhead from dis and reconnecting each time. My setup ran with this code for 2 weeks. None of the packages were lost (I published to 2-3 different topics each hour).

Thanks for your input @ghmartin77 :smile:

while (processed_size < total_recv_size) {
    ...
    // try to connect to the topic, just left it in to be sure
    while (!mqtt.connect(mqtthelper.topic_str));
    mqtt.publish(mqtthelper.topic_str, payload, payload_size + TIMESTAMP_BYTES);
    // Flushes TCP buffers, client is an instance of TinyGSMClient class
    client.flush(); 
    ...
ghmartin77 commented 5 years ago

Additional note: ESP8266 core for Arduino introduced WiFiClient.setSync() with this commit: https://github.com/esp8266/Arduino/pull/5089. It's available in 2.5.0 (currently in beta) and allows for synchronous sending without tampering around with the code.

And one more thing: Just saw this one made it into 2.4.0: https://github.com/esp8266/Arduino/pull/3967 Explains why @jonasvdd 's approach works. I was poking around with 2.3.x still where I saw this issue.