Pages: [1]   Go Down
Author Topic: Code critique on overly complex utility function  (Read 552 times)
0 Members and 1 Guest are viewing this topic.
Offline Offline
Newbie
*
Karma: 0
Posts: 3
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

At one point while working on my latest and largest Arduino project, I discovered the joys of debugging with sprintf, then was disappointed when it didn't play nice with the Arduino String class. I foolishly decided that it would be a good idea to try to fix that myself in several different ways.

Before you tell me, I know every part of this is bad practice. If I ever do make it work I almost certainly won't use it, but I think I might be able to learn something about C++ and embedded systems programming through it (before C++ the lowest-level programming language I had used was Python).

This is the .cpp file:
Code:
#ifndef Ssprintf
#define Ssprintf

#include "Arduino.h"

// Change these
#define REPLACEMENT_LEN 128
#define REPLACEMENT_TRUNC_TEXT "[...]" // NOTE: change the next line whenever you change this
#define REPLACEMENT_TRUNC_LEN 5

// Don't change these
#define FORMAT_CHARS "diuoxXfFeEgGaAcspn%S"

// Helper function
#define sprintf_trunc(replacement, format, ...) \
if (snprintf(replacement, REPLACEMENT_LEN - REPLACEMENT_TRUNC_LEN, format, __VA_ARGS__) \
      > REPLACEMENT_LEN - REPLACEMENT_TRUNC_LEN) \
    strcat(replacement, REPLACEMENT_TRUNC_TEXT);

inline String va_ssprintf(String format, va_list args) {
  String result = "";
  int strpos = 0;
  int fmtStartPos;
  int fmtEndPos;

  while(true) { // Includes a break; statement   
    fmtStartPos = format.indexOf('%', strpos);
    if (fmtStartPos == -1) break; // We're past the last argument

    fmtEndPos = fmtStartPos + 1; // Adds one later so we don't find the first % again and think it's %%
    // Find the next character
    //   Logic: Increment fmtEndPos while the fmtEndPosth char is not in FORMAT_CHARS.
    //     fmtEndPos ends up being the first character AFTER the format specifier, which is good.
    while(strchr(FORMAT_CHARS, format.charAt(fmtEndPos++)) == NULL);

    // Create a buffer for the substring that is big enough for the format string and \0...
    char substr[fmtEndPos - fmtStartPos+1];
    // ..and populate it with the substring
    format.substring(fmtStartPos, fmtEndPos).toCharArray(substr, fmtEndPos - fmtStartPos + 1);

    // Allocate some memory for the replacement (fixed size)
    char replacement[REPLACEMENT_LEN];
    // Now get the arg value according to variable type and use it (sprintf must be in the scope of each case)
    switch (substr[strlen(substr) - 1]) {
      // Special treatment cases first

    case 'S':
      { // Arduino string
        // Get the string
        String* argPtr = va_arg(args, String*);
        String argStr = *argPtr;
        // Convert the string to a char*
        char arg[argStr.length()+1];
        argStr.toCharArray(arg, argStr.length()+1);
        // Replace the 'S' with an 's', that's what sprintf expects
        //   Logic: Get a pointer to the 'S' char, defererence it into the char, set the char to 's'
        *strrchr(substr, 'S') = 's';

        // Put at most REPLACEMENT_LEN - REPLACEMENT_TRUNC_LEN characters into the array,
        //   so there's room for the truncation indicator if necessary
        sprintf_trunc(replacement, substr, arg);
        break;
      }

    case '%': // This one's special because it doesn't mean there's an argument to fetch
      // The simplest way to handle this is to turn it into sprintf("%s", "%");
      sprintf_trunc(replacement, "%s", "%");
      break;

    case 'n': // Fetch an argument to maintain the place in va_args, but don't use it
      // Turn this one into sprintf("%s", "");
      va_arg(args, unsigned long);
      sprintf_trunc(replacement, "%s", "");
      break;

      // The rest of the cases are fairly straightforward (except lengths are annoying)
    case 'd':
    case 'i':
      {
        if (*strstr(substr, "hh")) {
          signed char arg = va_arg(args, int); // compiler told me to use int
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'h')) {
          short int arg = va_arg(args, int); // compiler told me to use int
          sprintf_trunc(replacement, substr, arg);
        } else if (*strstr(substr, "ll")) {
          long long int arg = va_arg(args, long long int);
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'l')) {
          long int arg = va_arg(args, long int);
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'j')) {
          intmax_t arg = va_arg(args, intmax_t);
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'z')) {
          size_t arg = va_arg(args, size_t);
        } else { // Apparently ptrdiff_t isn't supported
          int arg = va_arg(args, int);
          sprintf_trunc(replacement, substr, arg);
        }
        break;
      }

    case 'u':
    case 'o':
    case 'x':
    case 'X':
      {
        if (*strstr(substr, "hh")) {
          unsigned char arg = va_arg(args, unsigned int); // compiler told me to use int
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'h')) {
          unsigned short int arg = va_arg(args, unsigned int); // compiler told me to use int
          sprintf_trunc(replacement, substr, arg);
        } else if (*strstr(substr, "ll")) {
          unsigned long long int arg = va_arg(args, unsigned long long int);
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'l')) {
          unsigned long int arg = va_arg(args, unsigned long int);
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'j')) {
          uintmax_t arg = va_arg(args, uintmax_t);
          sprintf_trunc(replacement, substr, arg);
        } else if (*strchr(substr, 'z')) {
          size_t arg = va_arg(args, size_t);
        } else { // Apparently ptrdiff_t isn't supported
          unsigned int arg = va_arg(args, unsigned int);
          sprintf_trunc(replacement, substr, arg);
        }
        break;
      }

    case 'f':
    case 'F':
    case 'e':
    case 'E':
    case 'g':
    case 'G':
    case 'a':
    case 'A':
      {
        //! Here is an opportunity to do something about arduino sprintf()'s lack of float handling (but I won't)
        if (*strchr(substr, 'L')) {
          long double arg = va_arg(args, long double);
          sprintf_trunc(replacement, substr, arg);
        } else {
          double arg = va_arg(args, double);
          sprintf_trunc(replacement, substr, arg);
        }
        break;
      }

    case 'c':
      { // no support for wint_t
        int arg = va_arg(args, int);
        sprintf_trunc(replacement, substr, arg);
        break;
      }

    case 's':
      {
        if (strchr(substr, 'l') != NULL) {
          wchar_t* arg = va_arg(args, wchar_t*);
          sprintf_trunc(replacement, substr, arg);
        } else {
          char* arg = va_arg(args, char*);
          sprintf_trunc(replacement, substr, arg);
        }
        break;
      }

    case 'p':
      {
        void* arg = va_arg(args, void*);
        sprintf_trunc(replacement, substr, arg);
        break;
      }

    default: // Shouldn't happen, so this is a format error (but remember to pop an item off va_args)
      va_arg(args, unsigned long);
      sprintf_trunc(replacement, "%s", "__FORMAT_ERROR__");
      break;
    }

    // Add: Everything from the end of the previous format string (strpos) to the beginning of this format string,
    //      then the replacement for this format string
    result += format.substring(strpos, fmtStartPos);
    result += replacement;
    //Serial.print("result so far: ");
    //Serial.println(result);

    // Finally, update our current position in the string. This WON'T be reached on the last iteration
    //   (when fmtStartPos == -1), so it will remain the end of the last format string after the loop
    strpos = fmtEndPos;
  }

  // Add everything from the last format string to the end of the string

  //Serial.print("right-before-final result: ");
  //Serial.println(result);
  //Serial.print("last substring: ");
  //Serial.println(format.substring(strpos));
  result += format.substring(strpos);
  //Serial.print("final result: ");
  //Serial.println(result);
  Serial.flush();
  return result;
}

inline String ssprintf(String format, ...) {
  va_list args;
  va_start(args, format);

  String result = va_ssprintf(format, args);

  va_end(args);

  return result;
}

#endif

I'll post the code I'm using to test it below, since it puts the post over the character limit.

I'm looking for any critique except reasons not to do this (like I said, I know not to do this outside of a programming exercise). The behavior is so erratic that I can't offer any more helpful information, so I'm hoping that experienced C++ developers can point out the many errors I'm sure I've made and that might make it more consistent.
Logged

Offline Offline
Newbie
*
Karma: 0
Posts: 3
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Here are my test cases (which I found via Google, but comprehensive but are definitely enough to demonstrate that my code is unreliable):
Code:
#include "_utils.cpp"

void setup() {
  Serial.begin(9600);
  delay(1000);
  Serial.println("Beginning tests");

  Serial.flush();
 
  String helloWorld = "Hello world";
  strictEqual(ssprintf("%s", "Hello world"), "Hello world");
  strictEqual(ssprintf("%S", &helloWorld), "Hello world");

  Serial.println("Basic substitution: %d (decimal integer)");
  strictEqual(ssprintf("%d", 69), "69");
  strictEqual(ssprintf("%d", true), "1");

  Serial.println("Basic substitution: %c (character)");
  strictEqual(ssprintf("%c", 69), "E");

  Serial.println("Decimal integer");
  strictEqual(ssprintf("%d", 0), "0");
  strictEqual(ssprintf("%d", 1), "1");
  strictEqual(ssprintf("%d", 10), "10");
  strictEqual(ssprintf("%d", -1), "-1");

  Serial.println("Octal");
  strictEqual(ssprintf("%o", 0), "0");
  strictEqual(ssprintf("%o", 1), "1");
  strictEqual(ssprintf("%o", 10), "12");
  strictEqual(ssprintf("%o", -1), "-1");

  Serial.println("Lower Hex");
  strictEqual(ssprintf("%x", 0), "0");
  strictEqual(ssprintf("%x", 1), "1");
  strictEqual(ssprintf("%x", 10), "a");
  strictEqual(ssprintf("%x", -1), "-1");

  Serial.println("Upper Hex");
  strictEqual(ssprintf("%X", 0), "0");
  strictEqual(ssprintf("%X", 1), "1");
  strictEqual(ssprintf("%X", 10), "A");
  strictEqual(ssprintf("%X", -1), "-1");

  Serial.println("Flags (align, padding, sign)");
  strictEqual(ssprintf("%3d", 0), "  0");
  strictEqual(ssprintf("%3d", 1), "  1");
  strictEqual(ssprintf("%3d", -1), " -1");
  Serial.println("--");
  strictEqual(ssprintf("%03d", 0), "000");
  strictEqual(ssprintf("%03d", 1), "001");
  strictEqual(ssprintf("%03d", -1), "-01");
  strictEqual(ssprintf("%-3d", 0), "0  ");
  strictEqual(ssprintf("%-3d", 1), "1  ");
  strictEqual(ssprintf("%-3d", -1), "-1 ");
  strictEqual(ssprintf("%-03d", 0), "0  ");
  strictEqual(ssprintf("%-03d", 1), "1  ");
  strictEqual(ssprintf("%-03d", -1), "-1 ");
  strictEqual(ssprintf("%+3d", 0), " +0");
  strictEqual(ssprintf("%+3d", 1), " +1");
  strictEqual(ssprintf("%+3d", -1), " -1");
  strictEqual(ssprintf("%+03d", 0), "+00");
  strictEqual(ssprintf("%+03d", 1), "+01");
  strictEqual(ssprintf("%+03d", -1), "-01");
  strictEqual(ssprintf("%-+3d", 0), "+0 ");
  strictEqual(ssprintf("%-+3d", 1), "+1 ");
  strictEqual(ssprintf("%-+3d", -1), "-1 ");
  strictEqual(ssprintf("%-+03d", 0), "+0 ");
  strictEqual(ssprintf("%-+03d", 1), "+1 ");
  strictEqual(ssprintf("%-+03d", -1), "-1 ");
 
 
  Serial.println("String width (char*)");
  strictEqual(ssprintf("|%s|", "test"), "|test|");
  strictEqual(ssprintf("|%s|", "test but longer"), "|test but longer|");
  strictEqual(ssprintf("|%2s|", "test"), "|test|");
  strictEqual(ssprintf("|%2s|", "test but longer"), "|test but longer|");
  strictEqual(ssprintf("|%-2s|", "test"), "|test|");
  strictEqual(ssprintf("|%-2s|", "test but longer"), "|test but longer|");
  strictEqual(ssprintf("|%20s|", "test"), "|                test|");
  strictEqual(ssprintf("|%20s|", "test but longer"), "|     test but longer|");
  strictEqual(ssprintf("|%-20s|", "test"), "|test                |");
  strictEqual(ssprintf("|%-20s|", "test but longer"), "|test but longer     |");

  Serial.println("String width and precision (char*)");
  strictEqual(ssprintf("|%2.2s|", "test"), "|te|");
  strictEqual(ssprintf("|%2.2s|", "test but longer"), "|te|");
  strictEqual(ssprintf("|%-2.2s|", "test"), "|te|");
  strictEqual(ssprintf("|%-2.2s|", "test but longer"), "|te|");
  strictEqual(ssprintf("|%20.2s|", "test"), "|                  te|");
  strictEqual(ssprintf("|%20.2s|", "test but longer"), "|                  te|");
  strictEqual(ssprintf("|%-20.2s|", "test"), "|te                  |");
  strictEqual(ssprintf("|%-20.2s|", "test but longer"), "|te                  |");
 
  Serial.println("String width (String)");
  String test = "test";
  String testLonger = "test but longer";
  strictEqual(ssprintf("|%S|", &test), "|test|");
  strictEqual(ssprintf("|%S|", &testLonger), "|test but longer|");
  strictEqual(ssprintf("|%2S|", &test), "|test|");
  strictEqual(ssprintf("|%2S|", &testLonger), "|test but longer|");
  strictEqual(ssprintf("|%-2S|", &test), "|test|");
  strictEqual(ssprintf("|%-2S|", &testLonger), "|test but longer|");
  strictEqual(ssprintf("|%20S|", &test), "|                test|");
  strictEqual(ssprintf("|%20S|", &testLonger), "|     test but longer|");
  strictEqual(ssprintf("|%-20S|", &test), "|test                |");
  strictEqual(ssprintf("|%-20S|", &testLonger), "|test but longer     |");

  Serial.println("String width and precision (String)");
  strictEqual(ssprintf("|%2.2S|", &test), "|te|");
  strictEqual(ssprintf("|%2.2S|", &testLonger), "|te|");
  strictEqual(ssprintf("|%-2.2S|", &test), "|te|");
  strictEqual(ssprintf("|%-2.2S|", &testLonger), "|te|");
  strictEqual(ssprintf("|%20.2S|", &test), "|                  te|");
  strictEqual(ssprintf("|%20.2S|", &testLonger), "|                  te|");
  strictEqual(ssprintf("|%-20.2S|", &test), "|te                  |");
  strictEqual(ssprintf("|%-20.2S|", &testLonger), "|te                  |");

}

void strictEqual(String first, String second) {
  if (first == second) {
    Serial.println("--PASSED--");
  } else {
    Serial.println("--FAILED--");
    Serial.print("\tExpected: ");
    Serial.println(second);
    Serial.print("\tActual: ");
    Serial.println(first);
  }
  Serial.flush();
}

void loop() {
  // put your main code here, to run repeatedly:
  delay(1000);
}



In case it helps, here's the most recent output in its entirety:
Code:
Beginning tests
--FAILED--
Eyúd: Hello world
%mr$Ï
--FAILED--
Eyúd: Hello world
_,rS
Basic substitution: %d (decimal integer)
--PASSED--
Logged

Global Moderator
UK
Offline Offline
Brattain Member
*****
Karma: 310
Posts: 26631
I don't think you connected the grounds, Dave.
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I wouldn't even start to imagine the merest notion of allowing the String class to cross my mind, on its way to becoming the basis for a debug method.

It'd be like the old joke about the railway wheel-tapper who sent fourteen locos to the repair sheds before they discovered his hammer was cracked.
Logged

"Pete, it's a fool looks for logic in the chambers of the human heart." Ulysses Everett McGill.
Do not send technical questions via personal messaging - they will be ignored.

UK
Offline Offline
Shannon Member
****
Karma: 223
Posts: 12630
-
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

I'm looking for any critique except reasons not to do this (like I said, I know not to do this outside of a programming exercise). The behavior is so erratic that I can't offer any more helpful information, so I'm hoping that experienced C++ developers can point out the many errors I'm sure I've made and that might make it more consistent.

That is so complex that it would take considerable time to review the design and even longer to test the implementation. What it's trying to do does not seem to me to be sensible so I haven't bothered investing that time. Any non-trivial use of String introduces the risk of stability problems, and to use this as the basis for a debugging system is fundamentally unwise IMO. If you were to ignore that issue then it would IMO be more sensible to adopt the streams based interface entirely rather than try to mangle the vprintf interface to fit it.

What would serve you far better would be to configure the stdin and stdout streams so that you can printf() to and read() from the serial port using the standard I/O functions instead of having to use the Arduino Serial interface. (I also found it was useful to connect these standard streams up to an RF24 interface so that sketches and libraries can access the standard streams without needing to know what mechanism is being used to carry them.)
Logged

I only provide help via the forum - please do not contact me for private consultancy.

Global Moderator
Melbourne, Australia
Offline Offline
Brattain Member
*****
Karma: 511
Posts: 19350
Lua rocks!
View Profile
WWW
 Bigger Bigger  Smaller Smaller  Reset Reset

Please note that in versions of the IDE up to and including 1.0.3, the String library has bugs as discussed here and here.

In particular, the dynamic memory allocation used by the String class may fail and cause random crashes.

I recommend reworking your code to manage without String. Use C-style strings instead (strcpy, strcat, strcmp, etc.), as described here for example.

Alternatively, install the fix described here:  Fixing String Crashes

Preferably upgrade your IDE to version 1.0.4 or above at: http://arduino.cc/en/Main/Software
Logged

http://www.gammon.com.au/electronics

Please post technical questions on the forum - not to me by personal message. Thanks a lot.

Offline Offline
Newbie
*
Karma: 0
Posts: 3
View Profile
 Bigger Bigger  Smaller Smaller  Reset Reset

Thanks for the pointers, I'll look into configuring stdin and stdout. I'm not surprised nobody wanted to look over the code — I don't even want to any more. I did also upgrade to Arduino 1.0.4, which gave different results but still not correct ones. I'm filing this one under "bad ideas", which I should have done before making a post out of it.
Logged

Pages: [1]   Go Up
Jump to: