cotestatnt / AsyncTelegram2

Powerful, flexible and secure Arduino Telegram BOT library. Hardware independent, it can be used with any MCU capable of handling an SSL connection.
MIT License
83 stars 25 forks source link

Paraing Json to char* without deep copy #49

Closed joroMaser closed 2 years ago

joroMaser commented 2 years ago

*I think there is a problem about parsing char from JSON* in function AsyncTelegram2::getNewMessage

I have 3 examples :

1 )

In MessageQuery when I print out
Serial.println(msg.callbackQueryData ); I got

XXXXXXXXX/sendMessage HTTP/1.0
Host: api.telegram.org
Connection: keep-alive
Content-Type: application/jso.....

But when I add 1 more member to struct TBMessage : String callbackQueryData1; And then I add 1 more line for parsing MessageQuery

message.callbackQueryData1 = updateDoc["result"][0]["callback_query"]["data"].as<String>();

Now Serial.println(msg.callbackQueryData1 ); print good result

2 )

In MessageText : I jot that JSON

{
  "ok": true,
  "result": [
    {
      "update_id": XXXXXXXX,
      "message": {
        "message_id": 123,
        "from": {
          "id": XXXXXXXX,
          "is_bot": false,
          "first_name": "XXXXXXXXXX",
          "language_code": "en"
        },
        "chat": {
          "id": -XXXXXXXX,
          "title": "XXXXXXXX",
          "type": "supergroup"
        },
        "date": XXXXXXXX,
        "text": "Gggh"
      }
    }
  ]
}

But when I tried to print msg.sender.firstName I got empty string and strlen(msg.sender.firstName) print 1

But when I add String firstName1; to struct TBUser and then parse it message.sender.firstName1 = updateDoc["result"][0]["message"]["from"]["first_name"].as<String>();; I can print the first name as well

3 )

In MessageQuery when I tried to print callbackQueryID (char *) Serial.println(msg.callbackQueryID); I got back_data (what ???)

But if I convert callbackQueryID : String callbackQueryID; in struct TBMessage and parse as String : message.callbackQueryID = updateDoc["result"][0]["callback_query"]["id"].as<String>(); I got right result.

So maybe there is a problem with parsing to char ? can you please check it ? maybe we need to convert all char to String in all structs ?

Maybe the error is because you only copy the pointer, you should deep copy with malloc and memcpy. But is better to use String more easy and more clean

cotestatnt commented 2 years ago

@joroMaser sorry for late reply, but i'm very busy in these days.

Maybe the error is because you only copy the pointer, you should deep copy with malloc and memcpy. But is better to use String more easy and more clean

Yes could be the reason, but i'm still trying to reproduce the issue on my side (ESP32). With Serial.println(msg.callbackQueryData ) I have always expected text.

Could you share a complete test sketch?

joroMaser commented 2 years ago

@cotestatnt I will try again and copy the full code . But any way https://github.com/cotestatnt/AsyncTelegram2/blob/master/src/AsyncTelegram2.cpp#L178 updateDoc allocated in stack. So the address should not be used after AsyncTelegram2::getNewMessage is finish. Even you got good results , and you may have had a basket and the information on the stack has not yet been destroyed. The right way is to convert those variable to String CPP

joroMaser commented 2 years ago

@cotestatnt Ok that is the error Here is my full code , I used AsyncTelegram2-2.0.8 and only change the define debug to 1 in header file

#define DEBUG_ENABLE        1
#ifndef DEBUG_ENABLE
    #define DEBUG_ENABLE    1
#endif

And here is my full code:


#include <AsyncTelegram2.h>

#define MYTZ "CET-1CEST,M3.5.0,M10.5.0/3"
#include <time.h>

#include <ESP8266WiFi.h>

BearSSL::WiFiClientSecure client;
BearSSL::Session   session;
BearSSL::X509List  certificate(telegram_cert);

AsyncTelegram2 myBot(client);

const char* ssid = "XXXX";
const char* pass = "XXXX";
const char* token = "XXXX";

void initWifi()
{
  WiFi.setAutoConnect(true);
  WiFi.mode(WIFI_STA);
  WiFi.begin(ssid, pass);
  delay(500);

  while (WiFi.status() != WL_CONNECTED) {
    Serial.print('.');
    delay(100);
  }
  Serial.println(WiFi.localIP());
  WiFi.setAutoReconnect(true);
  WiFi.persistent(true);

}

void initTelegram()
{
  configTime(MYTZ, "time.google.com", "time.windows.com", "pool.ntp.org");

  client.setSession(&session);
  client.setTrustAnchors(&certificate);
  client.setBufferSizes(1024, 1024);

  myBot.setUpdateTime(2000);
  myBot.setTelegramToken(token);
  Serial.print("\nTest Telegram connection... ");
  myBot.begin() ? Serial.println("OK") : Serial.println("NOK");
}

void setup() {
  Serial.begin(9600);
  Serial.println("Starting TelegramBot...");
  initWifi();
  initTelegram();

}

void loop() {

  TBMessage msg;

  if (myBot.getNewMessage(msg)) {

    String DUMMY = "TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST TEST";
    Serial.println(DUMMY);
    if ( msg.messageType == MessageText)
    {
       Serial.println(msg.sender.firstName );
    }
  }

}

The output of msg.sender.firstName is : (junk data) But if I remove DUMMY , I can print the msg.sender.firstName.

DUMMY is allocated on stack(just placeholder) , so when there is some data on stack the pointer that allocated on stack in AsyncTelegram2::getNewMessage maybe remove so I print only junk data. So you can see that you have a problem with char* that allocated in AsyncTelegram2::getNewMessage. You must use String CPP or copy the data with malloc and memcpy but it is less good to manage memory with such a code

cotestatnt commented 2 years ago

Hi @joroMaser, I've done a lot of tests in theese days and I was able to reproduce the issue in a deterministic mode with my test setup starting from your code. Generally, if i send a long message I have msg.sender.firstName "corrupted", with a shorter not. Clearly it's related to how system handle memory, and maybe my little different behaviour depends from version of IDE or ESP8266 Arduino core. Aniway I agree with you: switching to String can be the best solution.

I had never paid attention to this aspect because they are "fixed length data" usually and for the use I imagined it was enough to read them for the first time and then store in a variable of the sketch (and not for every message received).

joroMaser commented 2 years ago

@cotestatnt yes. I agree...that point to memory problem and can cause to unrefined behavior. Simply the pointer of stack :) and move to String CPP will he best solution. Let me now when you fix this bug . And maybe update Action tab to check that issue in the next time

joroMaser commented 2 years ago

@cotestatnt I saw your last commit. Is that issue fix? Do you add any check for you Action tab? Thank you friend!!

cotestatnt commented 2 years ago

hi @joroMaser That commit was buggy. I've tried to switch back to release 2.0.7 which don't reset the esp8266 and then reintegrate latest mods. In testing since yesterday and up to know seems work

joroMaser commented 2 years ago

@cotestatnt You have a problem with your last commit I send simple text message :

{
  "ok": true,
  "result": [
    {
      "update_id": XXXXX,
      "message": {
        "message_id": XXX,
        "from": {
          "id": XXXX,
          "is_bot": false,
          "first_name": "YYY",
          "language_code": "en"
        },
        "chat": {
          "id": AAAA,
          "first_name": "YYYY",
          "type": "private"
        },
        "date": AAAAA,
        "text": "AAAA"
      }
    }
  ]
}

Then I tried to print first name if ( msg.messageType == MessageText) but Serial.println(msg.sender.id ); return 0 and Serial.println(msg.sender.firstName ); return null

cotestatnt commented 2 years ago

Thanks a lot @joroMaser, was a cut&paste error. Now should be ok.

joroMaser commented 2 years ago

@cotestatnt by the way I saw that in function bool AsyncTelegram2::getUpdates() you drop yield(); after while (telegramClient->connected()) Why? I think it's important to prevent watchdog and save async...

cotestatnt commented 2 years ago

It's not that relevant in that position (actually the statement should be if and not while) Furthermore, the headers are read at that point using the readStringUntil() method which already uses the yield() instruction inside.