Can this function be improved

Projects Programming
1

Hi,

I have written this Date/Time function to provide some date handling and I am looking to see if any knowledgable person can give me any advice on improving this to improve the memory footprint or coding.

The date is collected from a RTC (DS3231) and then formatted to several types depending on the supplied Datatype variable.

Thank you

tDate = getDateTime("SDDate");


String getDateTime(String DateType) {
  Serial.println(DateType);
  String TSYear;
  String TSMonth;
  String TSDayName;
  String TSDay;
  String TSHour;
  String TSMinute;
  String TSSecond;
  String TSDate;
  byte zero = 0x00;
  //delay(1000);
  Wire.beginTransmission(DS3231_ADDRESS);
  Wire.write(zero);
  Wire.endTransmission();
  Wire.requestFrom(DS3231_ADDRESS, 7);
  // TSDayName 1-7 -> Monday to Sunday
  TSSecond = ChangeDate(bcdToDec(Wire.read() & 0x7f));
  TSMinute = String( ChangeDate(bcdToDec(Wire.read())));
  TSHour = String( ChangeDate(bcdToDec(Wire.read() & 0x3f)));
  TSDayName = String( ChangeDate(bcdToDec(Wire.read())));
  TSDay = String( ChangeDate(bcdToDec(Wire.read())));
  TSMonth = String( ChangeDate(bcdToDec(Wire.read())));
  TSYear = "20" + String( ChangeDate(bcdToDec(Wire.read())));
  TSDate = TSYear + TSMonth + TSDay + TSHour + TSMinute + TSSecond;
  if (DateType == "Date") {
    return TSYear + TSMonth + TSDay;
  } else if (DateType == "Time") {
    return TSHour + TSMinute + TSSecond;
  } else if (DateType == "Year") {
    return TSYear;
  } else if (DateType == "Month") {
    return TSMonth;
  } else if (DateType == "Day") {
    return TSDay;
  } else if (DateType == "SDHeader") {
    return TSYear + "_" +  TSMonth  + "_" + TSDay  + " @ " + TSHour + ":" + TSMinute + ":" + TSSecond;
  } else if (DateType == "SDFilename") {
    return TSYear + "_" +  TSMonth  + "_" + TSDay  + "-" + TSHour + "_" + TSMinute + "_" + TSSecond;
  } else if (DateType == "SDDate") {
    return TSYear +  TSMonth +  TSDay;
  } else {
    // DateTimestamp
    return TSYear + TSMonth + TSDay + TSHour + TSMinute + TSSecond;
  }
}
2

The first thing I would do would be to learn to use c-strings and get rid of the String class. It's memory hungry and using the concatenation operator tends to lead to memory fragmentation. That's the first most obvious improvement that could be made.

3

Define an enum with the names of the types of output that you want. Pass an instance of the enum to the function, not a string or a String.

4

using the concatenation operator tends to lead to memory fragmentation.

NO IT DOES NOT!!!!

As the Strings are local they only use the stack and therefore there is no chance of fragmentation which only occurs with the heap.

The idea that LOCAL Strings use the stack and not the heap can be found in many of the threads on this site.

Mark

5

Mark - come on don't shout (using capitals) esp when you might be wrong..

My understanding is that What is on the stack for a local variable will be the pointer to the instance of the String, the memory for the characters data of the String object itself is allocated from the heap, dynamically.

Concatenating Strings does allocate even more intermediary Strings that are fragmenting the heap.

Can you point at source information for what you claim?

My view: Source code shows things like

unsigned char String::reserve(unsigned int size)
{
	if (buffer && capacity >= size) return 1;
	if (changeBuffer(size)) {
		if (len == 0) buffer[0] = 0;
		return 1;
	}
	return 0;
}

unsigned char String::changeBuffer(unsigned int maxStrLen)
{
	char *newbuffer = (char *)realloc(buffer, maxStrLen + 1);
	if (newbuffer) {
		buffer = newbuffer;
		capacity = maxStrLen;
		return 1;
	}
	return 0;
}

It is my understanding that The c++ realloc() function will get memory from the heap (even if some other instance variables are on the stack)

6

holmes4:
NO IT DOES NOT!!!!

As the Strings are local they only use the stack and therefore there is no chance of fragmentation which only occurs with the heap.

The idea that LOCAL Strings use the stack and not the heap can be found in many of the threads on this site.

Mark

For stack-based String objects, the String object itself may be allocated on the stack, but the buffer where the character array is actually stored is always allocated on the heap. There is no mechanism for creating that buffer on the stack. And heavy use of concatenation CAN fragment the heap, regardless of where the String object itself resides.

Regards,
Ray L.

7

Good point Ray - Here on the Arduino forums we are told that a LOCAL String is total on the Stack and that at the end of the function when the String goes out of scope all its memory is freed and thus there is no fragmentation.

Note that two consecutive free areas in the heap are combined in to one (that's a 1960's oldie)

Mark

8

Well that's wrong with objects - as it depends what you do between being initialized and modified and deleted

If you take a local char * ptr; then the memory for that pointer is on the stack, but if in your next line you do ptr = (char *) malloc(100); then the 100 bytes are allocated on the heap and it's up to you to free that memory before exiting your local function or you'll have a memory leak.

Same things happen with objects, the destructor function is called automatically for you though when leaving the local scope and that's why you will find in the code of the String class

String::~String()
{
	free(buffer);
}

which provides the necessary cleanup, poking a hole in the heap at the same time - which, depending on other memory allocation between creation and destruction, might become an issue especially if it is a small block (say you have been playing with global Strings at the same time for example) - so your mileage may vary...

Makes sense ?

9

Thanks for the replies.

So been reading up about c strings and the use of strcpy etc however I have a little probelm of trying to put the date/time in a c string. I am getting variuous errors

incompatible types in assignment of 'String' to 'char [3
invalid operands of types 'char [3]' and 'char [3]' to binary 'operator+'

So I have a char TSSecond[3]. This is 2 character for the seconds and then the null terminator

Now trying to get the seconds in the variable TSSecond

  TSSecond = String(ChangeDate(bcdToDec(Wire.read() & 0x7f)));

I have also tried

TSSecond = ChangeDate(bcdToDec(Wire.read() & 0x7f));
TSSecond = strcpy(ChangeDate(bcdToDec(Wire.read() & 0x7f)));
TSSecond = strcpy(String (ChangeDate(bcdToDec(Wire.read() & 0x7f))));

but everything just fails.

How can I get the seconds from a DS3231 and into a C string char variable TSSecond?

10 
invalid operands of types 'char [3]' and 'char [3]' to binary 'operator+'

Yeah, you don't add c-strings with a +. Use strcat. And make sure there's enough room where you put the result.

11

you can have a look at those standard C functions

12

Thanks. I spent quite a few days changing things and what not but I am stuck and would appreciate a little more help.

I have read many pages and threads and got quite a bit of confliction in the results on using sprintf, strcat,strcpy,struct,static,dynamic,global that I am really confused.

So I have written some of the code to replace String objects with c strings (ie character arrays) to store the variables etc until I hit a little issue at trying to return a c string from a function.

So for trial and error I wrote this little function to try and sort it, could someone give me a little help. This routine just sends a random number to the dowork function. The function looks at the random number and the returns the number as a c string with or without a leading zero and places the c string in the whatistheanswer variable.

The core reason for this is so I can query the DS3231 using wire, the DS3231 returns the result as a bcd. I then check the bcd and convert to int, check if under 10 and then provide the result with a leading zero or not into a variable for use elsewhere in my program for creating filenames, timestamps in log and other things.

char whatistheanswer[3];
int randNumber;

void setup() {
  Serial.begin(9600);
}

char* dowork(int val) {

  int InVal;
  char OutVal[3];
  char tOutVal[3];
  char result[3];
  InVal = val;
  if (InVal <= 9) {
    strcpy(OutVal, "0");
    itoa (InVal, tOutVal, 10);
    strcat(OutVal, tOutVal);
  } else {
    itoa (InVal, OutVal, 10);
  }
  strcpy(result, OutVal);
  return result;
}
void loop()
{
  
  randNumber = random(1, 20);
  whatistheanswer = dowork(randNumber);
  Serial.print("whatistheanswer: ");
  Serial.println(whatistheanswer);
  delay(1000);
}

So if I run
randNumber = random(1, 20); ie 4
whatistheanswer = dowork(randNumber);

I would expect the variable whatistheanswer to be a c string of "04"

13

It's always risky returning a pointer to a variable that will shortly go out of scope.

14

You don't need any string functions to do this.

char whatistheanswer[3];
int randNumber;

void setup() {
  Serial.begin(9600);
}

void dowork(int val, char* dest)
{
  if (val <= 9) {
    dest[0] = '0';
    itoa (val, dest + 1, 10);
  } else {
    itoa (val, dest, 10);
  }
}

void loop()
{
  randNumber = random(1, 20);
  dowork(randNumber, whatistheanswer);
  Serial.print("whatistheanswer: ");
  Serial.println(whatistheanswer);
  delay(1000);
}
15

aarg:
You don't need any string functions to do this.

I would number itoa() among string functions.

16

Whandall:
I would number itoa() among string functions.

Sure. I meant purely string manipulation functions. Also, itoa is deprecated.

17

Hi aarg, thanks but dare I ask what has replaced itoa to convert ints to cstring

18

The POSIX name for this is deprecated - if you want to stay current, use the ISO C++ conformant name: _itoa.

19

SniffTheGlove:
Hi aarg, thanks but dare I ask what has replaced itoa to convert ints to cstring

ltoa()

20

aarg:
ltoa()

Which is deprecated just like itoa.

This POSIX function is deprecated. Use the ISO C++ conformant _ltoa or security-enhanced _ltoa_s instead.