Is char array causing a memory leak?

I'll preface this with, I know just enough Arduino/C++ to get in over my head.
I'm interfacing with a PS2 optical mouse using the PS2 mouse handler library.
The device hangs after a period of time with no mouse movement (about 1 hour) but will hang sooner with movement.

I'm looking at the char arrays as the culprit, but I'm unsure of the mechanism behind the hang and so don't really know where to go from here. You're asking 'why on earth am I using char arrays?' The full sketch has a bunch of other functionality, and the function that acts on the mouse data (OpenWeather library) is looking for these values in that format to send to a web API.

The issue is present in the following code.

/* 
Uses PS2MouseHandler library to read a PS2 optical mouse
Hardware is Arduino Nano ESP32, HP PS\2 optical mouse with PAW3402 sensor chip, and a globe of the earth
Moving the mouse along the surface of the globe translates mouse xy movements to lat long
Contains code from various sources because spherical trigonometry is over my head
Calibration is performed by rotating the globe to the 'home' position, then reboot
*/


#include <Arduino.h>
#include <math.h>
#include <PS2MouseHandler.h>

const byte MOUSE_DATA = 8;
const byte MOUSE_CLOCK = 9;
struct Point {
    double x;
    double y;
};
unsigned long globeTimer = millis();
Point mousePoint = {0,0}; 
Point currentLatLong = {32.0,-117.0}; // home location
const Point ORIGIN = {0, 0}; // origin is always 0.0
PS2MouseHandler mouse(MOUSE_CLOCK, MOUSE_DATA, PS2_MOUSE_REMOTE);

const double GLOBE_R = 6.0; // globe raduis in inches
const double MOUSE_RES = 1000; // mouse resolution in pixels per inch.
const double GLOBE_SCALE = (MOUSE_RES * GLOBE_R); 

//Original sketch needed the values as char arrays to pass into other functions, so we're doing some conversions
char computedLat[20] = ""; 
char computedLong[20] = "";
char *Lat = "";
char *Long = "";

void setup() {
  Serial.begin(115000);
  if(mouse.initialise() != 0){
    Serial.println("mouse error");
  };
  mouse.set_resolution(MOUSE_RES); //the PAW3402 can be set to 500, 800, or 1000
  // do other setup here
}

void loop() {
  globe(); //check for updates
  if (millis() - globeTimer > 1000) { //serial output once per second
    globeTimer = millis();
    Serial.print("Lattitude : ");
    Serial.println(Lat);
    Serial.print("Longitude : ");
    Serial.println(Long);
  }
  //put code here
}

void globe() {
    mouse.get_data(); //get mouse movements
    mousePoint = {mouse.x_movement(), (mouse.y_movement() * -1.0)}; //invert y and scale the mouse movement to the size of the earth
    currentLatLong = calculateDestination(currentLatLong.x, currentLatLong.y, getVector(mousePoint).x, getVector(mousePoint).y); //compute our new lat long

    dtostrf(currentLatLong.x, 12, 6, computedLat); //convert to char array
    dtostrf(currentLatLong.y, 12, 6, computedLong);
    Lat = trimWhitespace(computedLat, sizeof(computedLat)); //and trim
    Long = trimWhitespace(computedLong, sizeof(computedLong));
}

Point getVector(Point point2) { //get bearing and distance based on mouse movement
  return {toDegrees(calculateBearing(ORIGIN, point2)), calculateDistance(ORIGIN, point2)};
}

// Calculate destination point given a starting point, bearing, and distance
Point calculateDestination(double startLat, double startLon, double bearing2, double distance2) {
    double delta = distance2 / GLOBE_SCALE; // Angular distance in radians using the scaling factor
    double lat1 = toRadians(startLat);
    double lon1 = toRadians(startLon);
    double brng = toRadians(bearing2);
    double lat2 = asin(sin(lat1) * cos(delta) + cos(lat1) * sin(delta) * cos(brng));
    double lon2 = lon1 + atan2(sin(brng) * sin(delta) * cos(lat1), cos(delta) - sin(lat1) * sin(lat2));

    // Normalize longitude to be between -180 and 180 degrees
    lon2 = fmod(lon2 + 3 * PI, 2 * PI) - PI;
    return {toDegrees(lat2), toDegrees(lon2)};
}

// Convert degrees to radians
double toRadians(double degrees) {
    return degrees * PI / 180.0;
}

// Convert radians to degrees
double toDegrees(double radians) {
    return radians * 180.0 / PI;
}
  
// Calculate bearing between two points
double calculateBearing(Point oldP, Point newP) {
    double dx = newP.x - oldP.x;
    double dy = newP.y - oldP.y;
    double angle = atan2(dy, dx);
    return fmod(toRadians(90.0) - angle, 2 * PI); // Convert to standard bearing
}

// Calculate distance between two points
double calculateDistance(Point oldP, Point newP) {
    double dx = newP.x - oldP.x;
    double dy = newP.y - oldP.y;
    return sqrt(dx*dx + dy*dy);
}

char* trimWhitespace(char* str, int length) { // this isn't needed if you don't need output in char array
  int start = 0;
  int end = length - 1;

  while (start < length && (str[start] == ' ' || str[start] == '\t' || str[start] == '\n')) {// Find the start index of non-whitespace characters
    start++;
  }
  while (end >= start && (str[end] == ' ' || str[end] == '\t' || str[end] == '\n')) { // Find the end index of non-whitespace characters
    end--;
  }
  for (int i = start; i <= end; i++) {// Move characters to the beginning of the array
    str[i - start] = str[i];
  }
  str[end - start + 1] = '\0';// Null-terminate the trimmed string
  return str;
}


on an ESP32 you don't have to use dtostrf()

you could just use snprintf() with %f format and you won't have to worry about the size of the buffer nor trimming white spaces and in your special case I don't know why you want to generate the text representation of the latitude and longitude.

if you want to print your data, don't do this

just use the point you calculated in the globe() function

    Serial.print("Lattitude : ");
    Serial.println(currentLatLong.x, 6); // print with 6 digits after the decimal point 
    Serial.print("Longitude : ");
    Serial.println(currentLatLong.y, 6); // print with 6 digits after the decimal point 

this way you get rid of the char buffer and the trimWhitespace function and you'l find out if this was your issue :slight_smile:

I don't immediately see any problem with the char arrays, but your entire trimWhitespace() function is not needed. dtostrf can directly generate the needed text without any leading or trailing whitespaces, just specify the minimum width the same as the number of decimal places and it will take however much space is needed to actually represent the number, with no leading spaces.

is there a benefit to using dtostrf vs sprintf?

dtostrf does not tell you how large the result can be so you are a bit in the dark regarding the buffer size and always need to plan for the worse case or cross fingers. We use it on small arduino like the UNO because sprintf does not support the %f format

On an ESP32 it does work and snprintf() will print in the buffer whilst taking into account the size of the buffer, so you will not overflow.

Got it.

Worst case would be 11 characters (sign, 3 digits, decimal point, 6 digits) plus the null terminator, for a char array size of 12. To maintain device portability, I feel it might be best to keep using dtostrf.

Thank you for pointing out the snprintf. Googling sprintf wasn't giving me the correct function.

I'm now running the following, which compiles and runs. I'll know in an hour if it solved the problem. In the meantime, I'm also experimenting with the snprintf function as an alternative.

char Lat[12];
char Long[12];

void globe() {
    mouse.get_data(); //get mouse movements
    mousePoint = {(double)mouse.x_movement(), ((double)mouse.y_movement() * -1.0)}; //invert y-axis
    currentLatLong = calculateDestination(currentLatLong.x, currentLatLong.y, getVector(mousePoint).x, getVector(mousePoint).y); //compute our new lat long
    dtostrf(currentLatLong.x, 6, 6, Lat); //convert to char array
    dtostrf(currentLatLong.y, 6, 6, Long);
}

until a bug somewhere makes currentLatLong.x much larger than expected

as I wrote above, for the code you have, you should neither use snprintf() nor dtostrf()... just print the number and let your arduino do the work without needing an extra buffer

The Serial.print output is secondary to the actual function that needs to work with currentLatLong, That function is building a string to send as an API call thusly:

  String url = "https://api.openweathermap.org/data/2.5/onecall?lat=" + latitude + "&lon=" + longitude + "&exclude=minutely" + exclude + "&units=" + units + "&lang=" + language + "&appid=" + api_key;

which I believe means that it must be formatted and buffered ahead of time in order to pass into the function.

I agree that a larger than expected value would cause problems. but the math behind those values in currentLatLong don't (shouldn't?) allow for it. Is it considered good practice to include out of bounds checking for values that can't be out of bounds?

You can (and should to optimize memory usage ) build the URL directly from the various elements

String url = "https://api.openweathermap.org/data/2.5/onecall?lat=";
url += String(currentLatLong.x,6);
url +=…
…

You could just as easily use snprintf() to build the url, particularly if you are going to need to convert the String to a char array when using it.

Yes - that would be the preferred way (snprintf)

I would say, avoid using char arrays, except if you have memory constraints.

Use String objects and their methods and helper functions for all the manipulations, it is much safer and usually easier.

It's very easy to hit unexpected edge cases with pointers, producing strange errors and behaviors, very hard to track down.

The same for any type of array. If you can use the Array and Vector classes. It will save you a lot of hours hunting ghosts.

So how do you know if a String concatenation failed because of memory shortage?

I said if there are no memory constrains.
With an ESP32 in many cases you will not have memory problems. And it's easier to avoid/deal with fragmentation problems than with memory corruption.

I would argue with that (an experienced programmer can easily spot potential overflow issues when playing with pointers or arrays and indexes rather than something happening at run time and dependant on everything else the code does to fill up memory - esp as the String class fails silently to do what’s being asked if memory is low and does not report the issue so you can’t handle it easily) but agree that String class issues due to not enough RAM are less frequent on ESP32 than smaller Arduinos.

The thing is that not everyone goes for the high end arduinos and there are lots of people playing around with UNO R3 or plain AVR Nanos where vector is not an option and memory is a concern. (Not to mention small attiny)

Learning how to master arrays, pointers and overflow goes a long way in a programmer’s life when it comes to developing for embedded systems. If you code for a desktop computer then it indeed matters much less as you have usually gigs of RAM and virtual memory and can use modern C++ containers or other programming languages not having this risk.

And if there are no memory problems as you say then coding with old C style buffers and Functions is not an issue - you just need to be a careful programmer as anyone needs to be anyway. (And use the functions with the n (strncpy, …) or l (strlcpy,…) that do not lead to buffer overflow and test the return value just in case - something you can’t do with the String class.)

Yes, I agree that the class String can be tricky, was not designed for MCU's I think. You have to be careful in loops and concatenations. But still if you have just a couple of arrays in your code it's better to use String and avoid the danger of a subtle mistake with a pointer.

With the Array class it's more clear. It has a very small memory and performance footprint, it's just a wrapper of a raw array. But it's safer and more convenient. Only for the option of getting size is worth to use it.

but this won't compile on a UNO

#include <array>

constexpr size_t arraySize = 5;
std::array<int, arraySize> testArray;

void setup() {
  Serial.begin(115200);
  for (auto &entry : testArray) entry = random(100);

  Serial.println("Array elements:");
  for (int i = 0; i < arraySize; i++) {
    Serial.print(F("testArray["));
    Serial.print(i);
    Serial.print(F("]="));
    Serial.println(testArray[i]);
  }
}

void loop() {}

you'll get

fatal error: array: No such file or directory
 #include <array>
          ^~~~~~~
compilation terminated.

it will work fine on an ESP32.

so again my point is that when you are on a small MCU, the old stuff is often needed - you are close to the metal (well silicon).

Yes, fully agree. But with ESP32 things start to be different. And the OP mentioned it.

You can use at least C++11 version, even more I think. I have used it extensively with the Arduino stuff and libraries without any problem.

Using the dtostrf version, this hung after about 5 hours. Oddly, it stopped mid-output.
I haven't been successful in getting a debug session going, possibly due to a driver problem, but that's a separate issue.

04:46:09.771 -> Lattitude : 32.000000
04:46:09.771 -> Longitude : -117.000000
04:46:10.766 -> Lattitude : 32.000000
04:46:10.766 -> Longitude : -117.000000
04:46:11.794 -> Lattitude :

fair indeed (and snprintf works well on that platform. :slight_smile: )