Skip to content

Memory Leak - observed on Apollo3 / Artemis #135

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
PaulZC opened this issue Apr 14, 2022 · 9 comments · Fixed by #136
Closed

Memory Leak - observed on Apollo3 / Artemis #135

PaulZC opened this issue Apr 14, 2022 · 9 comments · Fixed by #136
Labels
bug Something isn't working

Comments

@PaulZC
Copy link
Collaborator

PaulZC commented Apr 14, 2022

Based on this forum issue:

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

@PaulZC PaulZC added the bug Something isn't working label Apr 14, 2022
@PaulZC
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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
Copy link
Collaborator Author

PaulZC commented Apr 14, 2022

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

@PaulZC PaulZC linked a pull request Apr 14, 2022 that will close this issue
PaulZC added a commit that referenced this issue Apr 14, 2022
v2.2.9 - add a destructor to resolve issue #135
@ricozinn
Copy link

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants