jgromes / RadioLib

Universal wireless communication library for embedded devices
https://jgromes.github.io/RadioLib/
MIT License
1.55k stars 387 forks source link

Running LORA.receive(str) timeout extension using loop error #10

Closed Bambofy closed 5 years ago

Bambofy commented 5 years ago

Hi,

I'm trying to extend the timeout for the receive function by putting it in a for loop. It can receive the first message fine, but in the next loops, it receives garbage?

Receiver

int state = 0;
String str = "";
for (int i = 0; i < 10; i++)
{
    state = LORA.receive(str);
    if (state == ERR_NONE)
    {
         Serial.println(str);
     }
}

Transmitter

void loop()
{
    int state = LORA.transmit("hello world!");
    if (state == ERR_NONE) Serial.println("Success");
}

Thanks, Richard

jgromes commented 5 years ago

In your for loop, you're storing the received data using a String variable that was created outside of the for loop. String class allocates memory dynamically, hence unpredictable stuff will happen when it's used in such a way.

The issue will disappear (sort of) if you move the declaration into the for loop, i.e.

int state = 0;
for (int i = 0; i < 10; i++)
{
    String str = "";
    state = LORA.receive(str);
    if (state == ERR_NONE)
    {
         Serial.println(str);
     }
}

From memory point of view however, this is even worse, since RAM will now fragment quickly - I would expect the MCU to reset every few seconds.

Why are you using String class in the first place?

jgromes commented 5 years ago

If what you're trying to achieve is longer time in receive mode, I sugggest you use interrupt-driven methods - see ReceiveInterrupt example for details.

Bambofy commented 5 years ago

Thanks, moving the String object into the loop solved the memory issue. I'm really not sure what the problem is with the dynamically allocated String object? If i do this:

String str = "";
int counter = 0;
function()
{
   str = String(counter);
   counter++;
}

doesn't the string object get deallocated? also what's the best replacement for this because i rely heavily on the string object functions?

thanks, Richard

jgromes commented 5 years ago

Where would it get deallocated? Looking at the String implementation, all the contructor does is it sets the buffer pointer to null, that doesn't mean the dynamic buffer is deallocated - it means it's now inaccessible and leaking memory. Sort of like doing this:

char* p = new char[10];
p = new char[5]; // 10 bytes are now inaccessible
delete[] p; // only 5 bytes can be deallocated

Arduino String is notorious for causing all kinds of memory issues - I try to avoid it at all costs. Especially for your application, if dynamic memory allocation is necessary (e.g. for receiving data), I would strongly suggest taking good care of proper memory management and only using C-strings.

Bambofy commented 5 years ago

Wow that is really bad memory management! i will convert my String objects now.