Works on AVR but not on SAMD21 micro [SOLVED]

Projects Programming
1

Hi,

I ve been using the following function to convert seconds to an English readable string.
The function works perfect on Mega but since moving to a SAMD21 platform the result is usually messed up with spaces in the wrong place etc. Since moving to SAM21 I had to make modification to several functions to get them working as expected and it usually was a matter of taking care of the the 32bit length of variables. However cant seem to find the problem in this case.
Anyone can help ?

void secondsToEnglishString(char str[40], uint32_t seconds)
{

  if (seconds==0) {
     snprintf(str, sizeof(str),"%ld seconds", seconds);
     return;
  }
  
  boolean hasDays = false;
  // Take care of days
  if (seconds >= 86400) {
    snprintf(str, sizeof(str),"%ld day%c", seconds/86400, seconds>=86400? 's':0);
    seconds = seconds % 86400;
    hasDays = true;
    str += strlen(str);
  }
  
  boolean hasHours = false;
  // Take care of hours
  if (seconds >= 3600) {
    if (hasDays)
      strcpy(str++, " ");
    snprintf(str, sizeof(str),"%ld hour%c", seconds/3600, seconds>=7200? 's':0);
    seconds = seconds % 3600;
    hasHours = true;
    str += strlen(str);
  }

 // Take care of minutes
 boolean hasMins = false;
 if (seconds >= 60) {
  if (hasHours || hasDays)
      strcpy(str++, " ");
  snprintf(str, sizeof(str), "%ld minutes", seconds/60); 
  seconds = seconds % 60;
  hasMins=true;
  str += strlen(str);
  
 }
  
  // Take care of minutes
  //seconds = seconds / 60;
  if (seconds) {
    if (hasHours || hasDays || hasMins)
      strcpy(str++, " ");
    snprintf(str, sizeof(str), "%ld seconds", seconds);
  }

  
}
2 
const uint32_t dayLength = 86400;
const uint32_t hourLength = 3600;
...

will this help?

3

Not really. Tried that already :slight_smile:

See output below:
Total On time:48 12 2 m 43

The above should have been:
Total On time:48d 12 h 2 m 43 s

5

No, that's not how to do an optional 's'. Try:

    snprintf(str, sizeof(str),"%ld day%s", seconds/86400, seconds>86400? "s" : "");
6

You are right MarkT.
Unfortunately though, that didnt solve the problem either.
It seems when the result has more than one digits, the optional "s" disappears

ie 9 s is ok
when its 12 , the "s" doesn't show

7

Are you sure you are not overflowing the string buffer - you use sizeof(str), but are changing str as a pointer, so you are not safe against buffer overflow.

Can you not calculate all the values then use one snprintf() call to fill in the str buffer?

8

Probably the source of your problem is that sizeof(str) is going to be 2 on an AVR and 4 on an ARM, because str is actually a pointer by the time it is passed to the function.

However, this is also a really horrible and incorrect way to use snprintf()...

9

No, its declared thus:

char str[40]

So its size is 40. The problem is that the code treats it as a pointer and increments it, yet sizeof() is compile-time and cannot know this, so it no longer acts as the true array bound.

10

Please give a full (minimal) sketch that shows the problem and that we can try on both boards.

The %ld is not connected to a uint32_t. The compiler expects a "unsigned long" and not a "uint32_t".
See the reference: https://www.cplusplus.com/reference/cstdio/printf/

MarkT writes about this:

strcpy(str++, ...
snprintf(str, sizeof(str) ...

That means the extra safety by using "snprintf()" is not safe anymore.

11

Are you sure? I wasn't. So I tested it...

void foo(char s[40]) {
  Serial.print("Sizeof check: ");
  Serial.println(sizeof(s));
}
2 Likes
12

@westfw I guess you want all of us to try it with a sketch and see for ourselves ?

A template knows the number of array elements and "by reference" will work: void foo(char (&s)[40])
But that's about it (I think).

@Watcher You should rely on what you know is right. Use a pointer as the first parameter and the size in the second parameter. Stay away from all the trouble that is mentioned. Show a new sketch, so we can have a look at it.

13

Well, only the OP, actually.

I don't see how the AVR version could possibly be "working." When I compile with Warnings turned on, I get multiple:

test_seconds2english.ino:37:29: warning: 'sizeof' on array function parameter 'str' will return size of 'char*' [-Wsizeof-array-argument]
     snprintf(str, sizeof(str), "%ld seconds", seconds);
                             ^
test_seconds2english.ino:33:40: note: declared here
 void secondsToEnglishString(char str[40], uint32_t seconds)
                                        ^

(which was my guess as to "the problem"), as well as:

test_seconds2english.ino:44:25: warning: argument to 'sizeof' in 'int snprintf(char*, size_t, const char*, ...)' call is the same expression as the destination; did you mean to provide an explicit length? [-Wsizeof-pointer-memaccess]
     snprintf(str, sizeof(str), "%ld day%c", seconds / 86400, seconds >= 86400 ? 's' : 0);
                         ^

(the snprintf() problem I and others have commented on.)
And the output on an Uno looks like:

0 seconds is 0
20 seconds is 2
200 seconds is 3 2
2000 seconds is 3 2
20000 seconds is 5 3 2
200000 seconds is 2 7 3 2

Here's a full test suite, with a fixed version. Compiled and tested on Uno (AVR) and QT Py (SAMD21)...

void setup() {
  Serial.begin(9600);
  while (!Serial)
    ;
  printseconds(0);
  printseconds(20);
  printseconds(200);
  printseconds(2000);
  printseconds(20000);
  printseconds(200000);
  Serial.println("\nWith fixes");
  fixedprintseconds(0);
  fixedprintseconds(20);
  fixedprintseconds(200);
  fixedprintseconds(2000);
  fixedprintseconds(20000);
  fixedprintseconds(200000);
  fixedprintseconds((uint32_t)2e9);  // demonstrate string overflow
}

void printseconds(uint32_t s) {
  char buffer[40];
  secondsToEnglishString(buffer, s);
  Serial.print(s); Serial.print(" seconds is ");
  Serial.println(buffer);
}
void fixedprintseconds(uint32_t s) {
  char buffer[40];
  fixedSToEnglishString(buffer, s);
  Serial.print(s); Serial.print(" seconds is ");
  Serial.println(buffer);
}

void secondsToEnglishString(char str[40], uint32_t seconds)
{

  if (seconds == 0) {
    snprintf(str, sizeof(str), "%ld seconds", seconds);
    return;
  }

  boolean hasDays = false;
  // Take care of days
  if (seconds >= 86400) {
    snprintf(str, sizeof(str), "%ld day%c", seconds / 86400, seconds >= 86400 ? 's' : 0);
    seconds = seconds % 86400;
    hasDays = true;
    str += strlen(str);
  }

  boolean hasHours = false;
  // Take care of hours
  if (seconds >= 3600) {
    if (hasDays)
      strcpy(str++, " ");
    snprintf(str, sizeof(str), "%ld hour%c", seconds / 3600, seconds >= 7200 ? 's' : 0);
    seconds = seconds % 3600;
    hasHours = true;
    str += strlen(str);
  }

  // Take care of minutes
  boolean hasMins = false;
  if (seconds >= 60) {
    if (hasHours || hasDays)
      strcpy(str++, " ");
    snprintf(str, sizeof(str), "%ld minutes", seconds / 60);
    seconds = seconds % 60;
    hasMins = true;
    str += strlen(str);

  }

  // Take care of minutes
  //seconds = seconds / 60;
  if (seconds) {
    if (hasHours || hasDays || hasMins)
      strcpy(str++, " ");
    snprintf(str, sizeof(str), "%ld seconds", seconds);
  }
}

void fixedSToEnglishString(char str[40], uint32_t seconds)
{
  uint8_t remainingLen = 40;
  uint8_t substringLen;  // length of this bit of the string.
  boolean needsSpace;

  if (seconds == 0) {
    snprintf(str, remainingLen, "%ld seconds", seconds);
    return;
  }

  // Take care of days
  if (seconds >= 86400) {
    substringLen = snprintf(str, remainingLen, "%ld day%c", seconds / 86400, seconds >= 86400 ? 's' : 0);
    seconds = seconds % 86400;
    needsSpace = true;
    remainingLen -= substringLen;
    str += substringLen;
  }

  // Take care of hours
  if (seconds >= 3600) {
    if (needsSpace) {
      strcpy(str++, " ");
      remainingLen--;
    }
    substringLen = snprintf(str, remainingLen, "%ld hour%c", seconds / 3600, seconds >= 7200 ? 's' : 0);
    seconds = seconds % 3600;
    needsSpace = true;
    remainingLen -= substringLen;
    str += substringLen;
  }

  // Take care of minutes
  if (seconds >= 60) {
    if (needsSpace) {
      strcpy(str++, " ");
      remainingLen--;
    }
    substringLen = snprintf(str, remainingLen, "%ld minutes", seconds / 60);
    seconds = seconds % 60;
    needsSpace = true;
    remainingLen -= substringLen;
    str += substringLen;
  }

  // Take care of minutes
  //seconds = seconds / 60;
  if (seconds) {
    if (needsSpace) {
      strcpy(str++, " ");
      remainingLen--;
    }
    snprintf(str, remainingLen, "%ld seconds", seconds);
  }
}

void loop() {
}

And it's output...

0 seconds is 0
20 seconds is 2
200 seconds is 3 2
2000 seconds is 3 2
20000 seconds is 5 3 2
200000 seconds is 2 7 3 2

With fixes
0 seconds is 0 seconds
20 seconds is  20 seconds
200 seconds is  3 minutes 20 seconds
2000 seconds is  33 minutes 20 seconds
20000 seconds is  5 hours 33 minutes 20 seconds
200000 seconds is 2 days 7 hours 33 minutes 20 seconds
2000000000 seconds is 23148 days 3 hours 33 minutes 20 second
1 Like
14

Thank you all for your help.

@ westfw : Thanks so much for all the effort to essentially re-develop and test this function.
Your input was very... educational for me. Could have never pinpoint the problem alone!
Thanks again!

15

This topic was automatically closed 180 days after the last reply. New replies are no longer allowed.