sparkfun / SparkFun_u-blox_GNSS_Arduino_Library

An Arduino library which allows you to communicate seamlessly with the full range of u-blox GNSS modules
Other
225 stars 99 forks source link

Memory Leak - observed on Apollo3 / Artemis #135

Closed PaulZC closed 2 years ago

PaulZC commented 2 years ago

Based on this forum issue:

Richard (@ricozinn) is observing what looks like a memory leak on Apollo3 (Artemis) v2.2.1

PaulZC commented 2 years ago

Starting a diagnosis with v2.2.8 of this library: have we got a memory leak - depending on how the SFE_UBLOX_GNSS class is created and used?

This code should run forever, and it seems to (on Apollo3 v2.2.1):

#include <Wire.h> //Needed for I2C to GNSS

#include <SparkFun_u-blox_GNSS_Arduino_Library.h> //http://librarymanager/All#SparkFun_u-blox_GNSS

SFE_UBLOX_GNSS myGNSS;

void setup() {
  Serial.begin(115200);

  Wire.begin();

  //myGNSS.enableDebugging();

  delay(10000); // Delay to let the user open the Serial Plotter
}

void loop() {

  static int success = 0, fail = 0;

  if (myGNSS.begin())
    success++;
  else
    fail++;

  Serial.printf("%d\t%d\r\n", success, fail);
}

Here are the first 600 cycles. The blue line is the success count; the red line is the fail count (zero):

image

Leave it running for a while; all is well:

image

PaulZC commented 2 years ago

Change the code, creating and re-using myGNSS in the main loop:

#include <Wire.h> //Needed for I2C to GNSS

#include <SparkFun_u-blox_GNSS_Arduino_Library.h> //http://librarymanager/All#SparkFun_u-blox_GNSS

void setup() {
  Serial.begin(115200);

  Wire.begin();

  delay(10000); // Delay to let the user open the Serial Plotter
}

void loop() {

  SFE_UBLOX_GNSS myGNSS;

  static int success = 0, fail = 0;

  if (myGNSS.begin())
    success++;
  else
    fail++;

  Serial.printf("%d\t%d\r\n", success, fail);
}

and it goes BOOM after a 1323 cycles ("out of memory"):

image

image

PaulZC commented 2 years ago

OK. SFE_UBLOX_GNSS doesn't (yet) have a destructor. If we add one, will it cure the issue?

What to add...? I did write an end function which frees the memory. Will that do the trick?

Change the loop to:

void loop() {

  SFE_UBLOX_GNSS myGNSS;

  static int success = 0, fail = 0;

  if (myGNSS.begin())
    success++;
  else
    fail++;

  Serial.printf("%d\t%d\r\n", success, fail);

  myGNSS.end();

}

and it still goes boom after 1334 cycles:

image

Time for some head-scratching... Watch this space!

PaulZC commented 2 years ago

Does this work?

void loop() {

  SFE_UBLOX_GNSS *myGNSS = new SFE_UBLOX_GNSS;

  static int success = 0, fail = 0;

  if (myGNSS->begin())
    success++;
  else
    fail++;

  Serial.printf("%d\t%d\r\n", success, fail);

  delete myGNSS;

}

Nope...

image

PaulZC commented 2 years ago

This?

void loop() {

  SFE_UBLOX_GNSS *myGNSS = new SFE_UBLOX_GNSS;

  static int success = 0, fail = 0;

  if (myGNSS->begin())
    success++;
  else
    fail++;

  Serial.printf("%d\t%d\r\n", success, fail);

  myGNSS->end();

  delete myGNSS;

}

Nope...

image

PaulZC commented 2 years ago

Is this specific to the Apollo3?

What do we see on the SparkFun RedBoard ATmega328P with its tiny amount of RAM?

The ATmega328P does not support Serial.printf, so we need to change the loop:

void loop() {

  SFE_UBLOX_GNSS myGNSS;

  static int success = 0, fail = 0;

  if (myGNSS.begin())
    success++;
  else
    fail++;

  Serial.print(success);
  Serial.print("\t");
  Serial.println(fail);

}

I see three successful begins, then it's all fails...:

image

OK. That's different. But at least the code keeps on running...

Any clues in the debug messages? Ah, yes, there we go... Out of memory on setPacketCfgPayloadSize !

image

PaulZC commented 2 years ago

OK. Time to add that destructor!

SFE_UBLOX_GNSS::~SFE_UBLOX_GNSS(void)
{
  // Destructor

  end(); // Delete all allocated memory - excluding payloadCfg, payloadAuto and spiBuffer

  if (payloadCfg != NULL)
  {
    delete[] payloadCfg; // Created with new[]
    payloadCfg = NULL;   // Redundant?
  }

  if (payloadAuto != NULL)
  {
    delete[] payloadAuto; // Created with new[]
    payloadAuto = NULL;   // Redundant?
  }

  if (spiBuffer != NULL)
  {
    delete[] spiBuffer; // Created with new[]
    spiBuffer = NULL;   // Redundant?
  }
}

Ta da! That does the trick! (On ATmega328P):

image

PaulZC commented 2 years ago

Where are we on Apollo3?

image

Ah, the sweet smell of success!!

Apologies @ricozinn for all the head-scratching caused. And thanks again for reporting.

v2.2.9 will go live shortly.

Best wishes, Paul

ricozinn commented 2 years ago

Hah, you thank me for reporting a bug but I thought I did a terrible job reporting on the issue (and I really did). Thanks for doing all the work! Mine was specifically a "stack overflow" error though and I only saw it when I also included the ArduinoBLE library and was connected to a central.

Anyway, I'll try out the release when it comes out. Thanks again!