3
\$\begingroup\$

Although I have long experience of other languages, I've not used C until playing around with microcontrollers using the Arduino IDE. I'm a bit uncomfortable with my use of pointers in conjunction with C strings.

Since this is a microcontroller project, I want to avoid the String class and try to write code that will be reasonably efficient as well as readable by C programmers (i.e. idiomatic or following best practise)

I read data from a serial port, including a number in ASCII character form. I will display that number on a three-digit seven-segment display, so I want to right-justify numbers less than three digits and if the number is larger than 999 I just want to display the last three digits. I can indicate errors like out-of range numbers separately.

I have the following working code

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

  Serial.println(last3Digits("12345")); // 345
  Serial.println(last3Digits("789"));   // 789
  Serial.println(last3Digits("60"));    // _60
  Serial.println(last3Digits("5"));     // __5
  Serial.println(last3Digits(""));      // __0

}

char * last3Digits(char * aString) {

  char * retVal = "abc";
  byte len = strlen(aString);
  switch (len) {
    case 0:
      strncpy(retVal, "__0", 3);
      break;
    case 1:
      retVal[0] = '_';
      retVal[1] = '_';
      retVal[2] = aString[0];
      break;
    case 2:
      retVal[0] = '_';
      retVal[1] = aString[0];
      retVal[2] = aString[1];
      break;
    default: // len >= 3
      strncpy(retVal, aString+len-3, 3); // copy only last 3 digits
  }
  retVal[3] = 0;

  return retVal;
}

// ---------------------------------------------------

byte counter = 0;

void loop() {
  Serial.print(counter);
  Serial.print(" ");
  delay(1000);
  if (++counter > 9) { counter = 0; }
}

The above can be run in an online emulator at https://wokwi.com/arduino/projects/315730913481720385

My function last3Digits() seems clumsy to me. I imagine it can be made smaller without losing readability, and I imagine I must have made some other rookie mistakes.

  • overwriting a string constant (what is the better way to allocate space?)
  • redundant setting of string terminator (felt safer)
  • ?
\$\endgroup\$
2
  • 1
    \$\begingroup\$ Re "write code that will be reasonably efficient": Why do you assume that? Using C strings can become very very slow if not used correctly (Shlemiel the painter’s algorithm). A case. \$\endgroup\$ Nov 21 '21 at 16:34
  • \$\begingroup\$ @PeterMortensen Oh just concerns about potential issues described in this article and a desire to develop skills that will be transferable to smaller microcontrollers like the ATtiny85 or ATtiny402. \$\endgroup\$ Nov 21 '21 at 17:18
4
\$\begingroup\$

Well, there are some concerning points about your code:

  1. You are not using const where appropriate. It is there to help you avoid obvious bugs, like overwriting constants. Crank up the warning-level, and the compiler warns about assigning string-constants to mutable pointers.

  2. If you need a small persistent space, ask for it. Don't misuse string-constants:

     static char result[4] = "";
    

    Be aware static state breaks re-entrancy and thread-safety.

  3. strncpy() is not a string-function. It is a fixed-buffer-function, filling it with the start of the given string and zero-padding the end, thus generally wasting time or not adding a string-terminator.

  4. Decide what to do with leading '0'. I would discard them.

  5. Re-writing the same data already written is generally a poor use of your time. Still, it doesn't hurt that much, especially without any caches to invalidate and pages to dirty, like outside small microcontrollers.

  6. If you unconditionally write underscores in the first two places of the return-buffer, you can thereafter ignore them if you don't have better things for them.

  7. If you want a generic name for a string-argument, s conveys all the information you need.

Putting it all together and simplifying a bit:

char* last3Digits(const char* s) {
    static char r[4] = "";
    r[0] = r[1] = '_';
    while (*s == '0')
        ++s;
    if (!*s)
        s = "0";
    size_t n = strlen(s);
    s += n;
    if (n > 3) n = 3;
    memcpy(r + 3 - n, s - n, n);
    return r;
}
\$\endgroup\$
4
\$\begingroup\$

You can run into problems with thread safety when using a static buffer. I prefer the following form which should also work for any size display:

void lastDigits(char* buf, const size_t size, const char* num) 
{
    size_t len = strlen(num);
    if (len >= size)
    {
        memcpy(buf, num + len - size, size + 1);
    }
    else
    {
        memset(buf, '_', size - len);
        memcpy(buf + size - len, num, len + 1);
    }
}

int main()
{
    char buf[4];
    lastDigits(buf, 3, "12");
    printf("%s\n", buf);
}
\$\endgroup\$
4
  • 1
    \$\begingroup\$ Use memset() for filling underscores. \$\endgroup\$ Nov 20 '21 at 20:21
  • \$\begingroup\$ @Deduplicator thanks, updated. \$\endgroup\$
    – jdt
    Nov 20 '21 at 20:23
  • 1
    \$\begingroup\$ Rework and polish for a nice generic tool: Rename rightJustify(), argument for fill-character, ensure the input- and output-buffer may overlap (use memmove() instead of memcpy(), swap filling and moving). \$\endgroup\$ Nov 20 '21 at 20:35
  • \$\begingroup\$ @Deduplicator, sounds like a good idea for a whole new question ;-) \$\endgroup\$
    – jdt
    Nov 20 '21 at 20:38

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.