shurillu / CTBot

A simple (and easy to use) Arduino Telegram BOT Library for ESP8266/ESP32
MIT License
147 stars 34 forks source link

Migration to arduino json6 #62

Closed Di-Strix closed 3 years ago

Di-Strix commented 3 years ago

Hello! Arduino Json 6 released about year ago. but CTBot still works on ArduinoJson 5. I know: working - don't touch. But more and more libraries are moving to v6. For example, ESPUI uses v6, CTBot uses v5 and it's complicated to use both libraries at once. I rewrote library to use v6 and it's working (includes reply, inline keyboards) but testing needed

shurillu commented 3 years ago

Hello Di-Strix, thank you for the very good work: I much appreciate your contribute, but your changes are only for ArduinoJson 6 compatibility. So, I modified your code for supporting ArduinoJson 5 and ArduinoJson 6 (for backward compatibility). I found some issues while moving strings to flash, I'll do this work in a future release. Thanks again man! P.S.: I put your name in the credits sections ;-)

Di-Strix commented 3 years ago

Hello shurillu! Thank's for your reply. I reviewed code published to master branch and I have some suggestions:

  1. In src/CTBotDefines.h I used

    #if CTBOT_BUFFER_SIZE == 0
    #if defined(ARDUINO_ARCH_ESP8266)
        #undef CTBOT_BUFFER_SIZE
        #define CTBOT_BUFFER_SIZE ESP.getMaxFreeBlockSize() - 10
    #elif defined(ARDUINO_ARCH_ESP32)
        #undef CTBOT_BUFFER_SIZE
        #define CTBOT_BUFFER_SIZE ESP.getMaxAllocHeap() - 10
    #endif
    #endif

    because in ArduinoJson 6 no actual difference between DynamicJsonDocument and StaticJsonDocument as it is in v5. ArduinoJson 6 documentation page And author recommends to use DynamicJsonDocument for documents which size is greater than 1kb Let's go back to the code: If user left 0 (Dynamic allocation) then it redefines CTBOT_BUFFER_SIZE to use the largest block in memory at runtime depends on chip. Then in the code after writing data to the document I'm using doc.shrinkToFit() (docs) command to reduce used memory of the document to its actual size. That let to fake DynamicJsonBuffer and you don't capp document max size to a specific value and if there are less free memory than prompted in case of hardcoded value it simply wont allocate memory, but in case ESP.getMaxFreeBlockSize() or ESP.getMaxAllocHeap() it will.

  2. In src/CTBot.cpp as I said before there is no difference between StaticJsonDocument and DynamicJsonDocument so I think it doesn't matter to bother with this

  3. Reply/Inline keyboards again with Documents :) To let user store more data in RAM at runtime, on my opinion, it has meaning to reduce size of the document after writing data. But there is problem. After shrinkToFit there no possibility to write more data. But it's easy to fix if use this algorithm:

    
    // At first you need to create a new document
    DynamicJsonDocument t_doc(CTBOT_BUFFER_SIZE);
    // Then copy all contents from old document to new one
    t_doc = m_jsonDocument; // See "Copying" paragraph at https://arduinojson.org/v6/api/dynamicjsondocument/

/ Only then I thought that I can clear old document at this point, because contents are already stored in new one, I didn't tested it, but I suppose that it might work /

// Here write new data to the document, for example (from my fork): getRows(t_doc).createNestedArray(); // Then clear old document by calling m_jsonDocument.clear(); // Actually garbageCollect doesn't needed here, but just in case I putted him here m_jsonDocument.garbageCollect(); // Reduce memory usage of the new document t_doc.shrinkToFit(); // And then replacing existing document with new one m_jsonDocument = newDoc;



That's only suggestions. I really like this cool and powerful library! Thanks for your work!
shurillu commented 3 years ago

Hello Di-Strix, I fixed some memory optimization suggested by ElVasquito (#37) and I updated the master branch. I fixed also the use of StaticJsonDocument: now the library use only DynamicJsonDocument with ArduinoJson v6 as you suggested (thanks!).

Meanwhile I did some test with moving strings in flash and the use of the suggested improvement no. 1 The results are some random resets on ESP32 (and sometimes on ESP8266) - now are all in memory. With my tests, moving all strings saves around 1Kbyte of heap, but as I said sometimes causes resets.

So, if you want give me an hand you are welcome!

Remember that the code must run on ESP8266, on ESP32 and with ArduinoJson 5 / 6 library. I know, it's an hard work testing the changes with all combination!

Cheers,

Stefano

Di-Strix commented 3 years ago

Of course! :) I have an idea to save RAM. Debug logs are using memory even if debug is disabled, but strings still use memory. Maybe it's matter to move serialLog to #define function which will check is debug enabled or not. And if it enabled - insert it into code, otherwise simply insert nothing I will test it as soon as possible and open a pull request if it makes sense.

UPD: Seems no. If debug enabled logs are using memory, but only 144 bytes, if disabled using nothing. Not suprising, because functions are inline, I forgot...