witnessmenow / Universal-Arduino-Telegram-Bot

Use Telegram on your Arduino (ESP8266 or Wifi-101 boards)
MIT License
1.11k stars 306 forks source link

ESP32 will send duplicate messages. #74

Open JacoFourie opened 6 years ago

JacoFourie commented 6 years ago

Hi all.

So I have been testing the port of the ESP32. It works well except I will get duplicate messages at random. I switched on debugging to get the message info. I also logged a support call with Telegram. I gave them the message id when I receive duplicates and they told me that the messages is sent more than one.

I will send the message only once on my end as I can see that in the serial feed.

with the debug switched on I will see 2 outputs for every message sent.

Like this.

SEND Post Message
[BOT Client]Connecting to server

{"ok":true,"result":{"message_id":2750,"from":{"id":yyyyyy,"is_bot":true,"first_name":"Open Automation Alerts","username":"OpenAutomationBot"},"chat":{"id":xxxxxxxx,"first_name":"Jaco","last_name":"Fourie","type":"private"},"date":1519390520,"text":"Web Test"}}

{"ok":true,"result":{"message_id":2750,"from":{"id":yyyyyyy,"is_bot":true,"first_name":"Open Automation Alerts","username":"OpenAutomationBot"},"chat":{"id":xxxxxxx,"first_name":"Jaco","last_name":"Fourie","type":"private"},"date":1519390520,"text":"Web Test"}}
Closing client

But yet I will get duplicates from time to time like this one .

photo_2018-02-23_14-57-43

Telegram checked on their end with the message ID and they say it is being sent more than once.

"Our devs have been checking it, there is nothing wrong on our side, somehow your code is sending the message several times :("

Do anybody else see this ? I can see I only fire the code once as the debug info only shows once.

JacoFourie commented 6 years ago

OK so I seem to have found where it is coming from.

Why is there a while loop here ?

if (payload.containsKey("text")) {
    while (millis() < sttime+8000) { // loop for a while to send the message
      String command = "bot"+_token+"/sendMessage";

          //Added this to test
      Serial.print("The Command is : ");
      Serial.println(command);

      String response = sendPostToTelegram(command, payload);
      if (_debug) Serial.println(response);
      sent = checkForOkResponse(response);
      if (sent) {
        break;
      }
    }
  }

Sometimes this loop will cause the command to fire more than once as can be seen here.

SEND Post Message
The Command is : botxxxxxxxxxxxx/sendMessage
[BOT Client]Connecting to server

The Command is : botxxxxxxxx/sendMessage

The Command is : botxxxxxxxx/sendMessage

{"ok":true,"result":{"message_id":2791,"from":{"id":xxxxxx,"is_bot":true,"first_name":"Open Automation Alerts","username":"OpenAutomationBot"},"chat":{"id":xxxxxxxx,"first_name":"Jaco","last_name":"Fourie","type":"private"},"date":1519413626,"text":"Lets test the Bot !!"}}

{"ok":true,"result":{"message_id":2791,"from":{"id":xxxxxxxx,"is_bot":true,"first_name":"Open Automation Alerts","username":"OpenAutomationBot"},"chat":{"id":xxxxxxxxxxx,"first_name":"Jaco","last_name":"Fourie","type":"private"},"date":1519413626,"text":"Lets test the Bot !!"}}
Closing client
JacoFourie commented 6 years ago

OK it seems that if the network or something is busy that code will fire in a loop and create duplicate messages. I managed to get it firing 3 times now. I assume that if you do not get a response of good you will sit in the loop until you do get one.

JacoFourie commented 6 years ago

I have commented out the while loop and will let it run now to see if I still get the duplicates. I guess the loop is there to make sure the message is sent. But it seems that the response is showing it did not send yet it did and then the loop will fire again.

witnessmenow commented 6 years ago

Thanks for the investigation, I will take a look this evening

On 23 Feb 2018 19:41, "JacoFourie" notifications@github.com wrote:

I have commented out the while loop and will let it run now to see if I still get the duplicates. I guess the loop is there to make sure the message is sent. But it seems that the response is showing it did not sent yet it did and then the loop will fire again.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/witnessmenow/Universal-Arduino-Telegram-Bot/issues/74#issuecomment-368117858, or mute the thread https://github.com/notifications/unsubscribe-auth/ABfXwgboDuTaKrW4ASw_Myt3ALxUTW4jks5tXxRpgaJpZM4SQ2Wr .

JacoFourie commented 6 years ago

OK so the code has been running 9.5 hours. It will send a message every 2 minutes to test. I did not get any duplicates. But I did loose one message. So I will see if I can find a way to make sure the message was delivered but not to send duplicates.

missing

JacoFourie commented 6 years ago

OK I see what is going on here. The response while loop will return sometimes with no response and then the send loop will send again.

So this loop needs to wait longer for a response.

    while (millis()-now < 1500) {
            while (client->available()) {
                char c = client->read();
                responseReceived=true;

        if(!finishedHeaders){
          if (currentLineIsBlank && c == '\n') {
            finishedHeaders = true;
          }
          else {
            headers = headers + c;
          }
        } else {
          if (ch_count < maxMessageLength) {
            body=body+c;
            ch_count++;
                }
        }

        if (c == '\n') {
          currentLineIsBlank = true;
        }else if (c != '\r') {
          currentLineIsBlank = false;
        }

            }
JacoFourie commented 6 years ago

I changed the code to look like this. Will let it run and check if I see duplicates or missing messages.

while (millis()-now < 4500 && !responseReceived) {

Maybe that value in the while loop should be a variable so you can set the time to wait longer for networks that are slower around the world.

while (millis()-now < timeout && !responseReceived) {
witnessmenow commented 6 years ago

Great to hear you fixed the issue!

That's a really good test it seems very stable!

Just off the top of my head the it should return a false if it fails, that wait time is just to check the response from telegram to see if it sent OK, it wouldn't have any impact on the message actually sending as you could technically not wait for a response at all, you just wouldn't know if it sent.

On 24 Feb 2018 06:32, "JacoFourie" notifications@github.com wrote:

I changed the code to look like this. Will let it run and check if I see duplicates or missing messages.

while (millis()-now < 4500 && !responseReceived) {

Maybe that value in the while loop should be a variable so you can set the time to wait longer for networks that are slower around the world.

while (millis()-now < timeout && !responseReceived) {

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/witnessmenow/Universal-Arduino-Telegram-Bot/issues/74#issuecomment-368204803, or mute the thread https://github.com/notifications/unsubscribe-auth/ABfXwhMAYxtfzaXQx7fNnTZORg5M0V0qks5tX6zogaJpZM4SQ2Wr .

JacoFourie commented 6 years ago

So it seems my timeout was too low. The server sometimes takes its time to send a response and then the send code will fire again as it did not get the OK resonse back in time. I now made the timeout value 9000 millis and all seems fine now.

I think the code should be made that you give it a timeout value in the constructor. It can have a default value of 1500 but it should be configurable. On both the send and receive side.

JacoFourie commented 6 years ago

I let it run for 190 minutes and all is OK now. I also did random tests where I would fluid the messages. I would count how many was sent and the correct number arrived on the other end.

correct

JacoFourie commented 6 years ago

So the code has been running for more than a day and I did not get any duplicates or loose any messages. ok

what I did was to add a timeout parameter to the constructor

and declared a var to hold the value. I also modified the code of the while loop to use the timeout value

//In the header file
UniversalTelegramBot (String token, Client &client , int timeout);

int send_timeout = 1500;

//In the cpp file
UniversalTelegramBot::UniversalTelegramBot(String token, Client &client , int timeout = 1500)   {
  _token = token;
  //Set the timeout value
  send_timeout = timeout;
  this->client = &client;
}

           //Wile loop of the send
        while (millis()-now < send_timeout && !responseReceived) {
            while (client->available()) {
                char c = client->read();
                responseReceived=true;

        if(!finishedHeaders){
          if (currentLineIsBlank && c == '\n') {
            finishedHeaders = true;
          }
          else {
            headers = headers + c;
          }
        } else {
          if (ch_count < maxMessageLength) {
            body=body+c;
            ch_count++;
                }
        }

        if (c == '\n') {
          currentLineIsBlank = true;
        }else if (c != '\r') {
          currentLineIsBlank = false;
        }

            }

      //if (responseReceived) {
        //if (_debug) {
        //  Serial.println();
        //  Serial.println(body);
        //  Serial.println();
        //}
        //  break;
        //}
        }

the

usab commented 5 years ago

Is this solution part of the latest release? I happen to have the same problem and cant follow your code examples. I know which part of the cpp file you are reffering to at https://github.com/witnessmenow/Universal-Arduino-Telegram-Bot/issues/74#issuecomment-368113549. But I dont understand what you did at https://github.com/witnessmenow/Universal-Arduino-Telegram-Bot/issues/74#issuecomment-368428472 where you posted the final solution. Thank you

JacoFourie commented 5 years ago

Not sure if my fix was made part of the current version. I have been using it for months now and it works 100% with the fix I put in.

kingsumos commented 5 years ago

Check the waitForResponse variable, the default value is 1500. Use a higher value to fix the issue, no code changes in the library is needed. E.g.: bot.waitForResponse = 9000; (configuration done like the longPoll option)

fritsjan commented 4 years ago

I found out that using an external antenna on the esp32 when wifi reception is low, that helps also with getting more stable responses...