What is wrong with my char to int conversion from serial input?

[jaddow]

Hello,
the code at the bottom of this post is just a part of another program i'm writing. I have built this mini version for testing and understanding the problems i'm having with it. (I'm pretty much a newbie)

So, the idea is to be able to write position data x/y (max 3 chars at once) into the serial monitor as well as single letters for further control over the program. I tried parsint before, it worked just fine. But because it's blocking the Serial.read as long as it hasn't recieved a valid int, it didn't make sense to continue with it. So I came up with the idea below... 

The problem I'm having is, that it doesn't seem to be able to inject the arrays content into the cti int. It just returns 0 and it seems like my knowledge is not enough to solve that mystery.
I hope my comments are clear and that you have a solution for this. Thank you very much for your time!


Greetings, Jaddow

Code: [Select]

  const byte neuer_Char = 32;
  char cInputArray[neuer_Char];   // an array to store the received data
  static byte cntCha = 0;
  char EndMark_1 = '\n';          //Endmarker for ending the Input
  char cInput;                    //Char input
  int cti;                        //char to int
  bool no_input = false;
  int z_3;                        //counter
  float itf;                      //Int to float

void setup() {
    Serial.begin(9600);
    Serial.println("<ready>");
}

void loop() {
  if(z_3 < 1){                    //Counter for executing a buffer clearing once
    Serial.read();               
    z_3++;
    }
  while(no_input == false) {
    cInput = Serial.read();
    if (cInput != EndMark_1) {    //As long as the chars aren't the endmarker, add them to the array
      cInputArray[cntCha] = cInput; //Serial.read into the array
      cntCha++;                   //count up
      if (cntCha >= neuer_Char) {
        cntCha = neuer_Char - 1;
        }
      }
    else{
      z_3 = 0;
      char buffer[4];             //create a buffer
      buffer[0] = cInputArray[0]; //b[0] is the first char in the array
      buffer[1] = cInputArray[1]; //...
      buffer[2] = cInputArray[2];
      buffer[3] = '\0';           //max size 4
      cti = atoi(buffer);         //buffer chars conversion to a single int
      Serial.println(cti);
      itf = (float)cti;           //conv. to float for further calculations
      cInputArray[cntCha] = '\0'; //terminate the string
      cntCha = 0;                 //char counter is back to 0
      no_input = true;            //ends and blocks the while-loop
      }
    }
  no_input = false;               //reopen the loop - only for testing
  }

[groundFungus]

The serial input basics tutorial could be of interest to you.  You are reading from the serial buffer before you know if anything is in the buffer.

[drmpf]

The SafeString library  (available from the Arduino Library manager) solves this problem easily and safely.
SafeString avoids the coding errors that are easy to make using low level c-strings and provides high level string functions similar to Arduino Strings but without the memory problems and odd errors.

SafeString has extensive error checking and error messages built in.
My tutorial Text I/O for the Real World covers this type of usage in detail

Here is a sample program that reads  x y from an input line without blocking.  None of the SafeString methods block.
If you are doing more than just reading in numbers, check out my Multi-tasking in Arduino tutorial for non-blocking coding.

Code: [Select]

// Command inputs
// using SafeString library  installed from Library Manager
// tutorial at https://www.forward.com.au/pfod/ArduinoProgramming/SafeString/index.html
// and https://www.forward.com.au/pfod/ArduinoProgramming/Serial_IO/index.html
#include <SafeStringReader.h>

createSafeStringReader(sfReader, 20, "\n\r");  // a line reader that holds 20chars terminated by newline

int x;
int y;

void setup() {
  Serial.begin(9600);
  for (int i = 10; i > 0; i--) {
    Serial.print(i); Serial.print(' ');
  }
  Serial.println();

  SafeString::setOutput(Serial); // enable error msgs
  sfReader.connect(Serial); // read from this input

  Serial.println("This demo expects x y data separated by space , or newline");
  Serial.println("  a character for direction comma and an int for speed newline");
  Serial.println("e.g. 230, 55");
  Serial.println(" OR  230 55 ");
  Serial.println(" Set the Arduino monitor to Newline endings OR CR NL or CR");
  Serial.println();
}

void loop() {
  if (sfReader.read()) {
    // got a line of data
    if (parseData()) {
      showParsedData();
    } else {
      Serial.print(" data format error:");
      Serial.println(sfReader);
    }
    sfReader.clear(); // empty for next line
  }
}


bool parseData() {        // split the data into its parts
  cSF(token, 12); // shorthand for createSafeString(token,12);
  size_t idx = 0;
  idx = sfReader.stoken(token, idx, " ,"); //  break on space or comma
  if (token.toInt(x)) {
    // got a valid int for x
  } else {
    Serial.print(token);
    Serial.println(" Invalid int for x");
    return false;
  }

  idx = sfReader.stoken(token, idx, " ,"); //  break on space or comma
  if (token.toInt(y)) {
    // got a valid int for x
  } else {
    Serial.print(token);
    Serial.println(" Invalid int for y");
    return false;
  }
  return true;
}

//============

void showParsedData() {
  Serial.print(" x:");
  Serial.print(x);
  Serial.print(" y:");
  Serial.println(y);
}



[Deva_Rishi]

solves this problem easily and safely.

I really don't see how using this library solves the issue the OP is having.
Within the sketch there are some strange things.

Code: [Select]

static byte cntCha = 0;as a global variable this doesn't need to be 'static' .  Not the cause of your issue but still it is wrong. It looks as if a lot of the variables were declared local (to loop() ) before and are now global.

Code: [Select]

if(z_3 < 1){                    //Counter for executing a buffer clearing once
    Serial.read();               
    z_3++;
    }what are you doing here ? you just throw out the first character in the buffer.

Code: [Select]

z_3 = 0;
      char buffer[4];             //create a buffer
      buffer[0] = cInputArray[0]; //b[0] is the first char in the array
      buffer[1] = cInputArray[1]; //...
      buffer[2] = cInputArray[2];
      buffer[3] = '\0';           //max size 4
      cti = atoi(buffer);         //buffer chars conversion to a single intand here you first reset the counter to throw the first character out, and then you start copying the characters into buffer[]. There is no need for that, and that not going to work if the number of chars received is not exactly '3' (ok it may work the first time since the cInputArray[] is full of 'null-terminators' ) what you should do is simply terminate cInputArray[] and extract from that.

Code: [Select]

cInputArray[cntCha] = '\0';
cti = atoi(cInputArray);

[drmpf]

My SafeString solution 'solves' the problem, as I understand, it by avoiding all the mess with char[] and counters and adding null terminators and the unreliable atoi() function.
The code I provided is a much more reliable solution then trying to fix the original code.

[Deva_Rishi]

My SafeString solution 'solves' the problem

I am not saying that your code does not work, but one of the issues here is the OP not comprehending why his code doesn't work. Your solution does nothing to clear that up.

by avoiding all the mess with char[] and counters and adding null terminators

which i suspect is to complex for some, but still a method that can be used with any data type, and therefore a useful technique to master.

the unreliable atoi() function.

It is not unreliable, it just returns '0' if it can not make any sense of the input. As long as '0' is not a valid return within the code this is not a problem.

The code I provided is a much more reliable solution then trying to fix the original code.

i doubt that it is in any way more reliable than explaining what is the error, since that will help the OP to understand what is the mistake, and how to avoid this in the future.

[J-M-L]

The code I provided is a much more reliable solution then trying to fix the original code.

fixing the original code definitely requires some rework, but you can make it as reliable as using your libraires, without the overhead of learning something new and specific. It will equip the OP with knowledge s/he can apply in coming years to many situations.

In order for you to write your library, you had to have that understanding and knowledge. It served you well in your job I'm sure, why would you deprive other from gaining the same knowledge?

Once this knowledge is acquired, people can decide to use your library to make their life easier, but they will understand the why, and the associated cost.

Knowledge helps make smart decisions.

PS: your integral transformation functions break C+C++ conventions on parsing buffers. For example

Code: [Select]

#include <SafeString.h>
createSafeString(msgStr, 50);
char msgBuf[50] = "1234,Hello";

void setup() {
  long v1=0, v2=0;
 
  Serial.begin(115200);
  SafeString::setOutput(Serial);
 
  msgStr = F("1234,Hello");

  msgStr.toLong(v1);
  Serial.print(F("v1=")); Serial.println(v1);

  v2 = strtol(msgBuf, NULL, 10);
  Serial.print(F("v2=")); Serial.println(v2);
}

void loop() {}

Serial Monitor (@ 115200 bauds) will show
v1=0
v2=1234


I did not see any warning... Calling msgStr.toLong() would fail whereas there is a perfectly formed integral at the start of the buffer. the version with strtol() will work fine and return 1234

Why did you decide to only accept the conversion if there are just trailing spaces (or similar)? why would a space be any better than a comma or other stuff in my buffer?



Also if I do this

Code: [Select]

#include <SafeString.h>
createSafeString(msgStr, 50);
char msgBuf[50] = "1234,Hello";

void setup() {
  long v=0;
 
  Serial.begin(115200);
  SafeString::setOutput(Serial);
  msgStr = F("1234,Hello");
  msgStr.toLong(v);
  Serial.print(F("v=")); Serial.println(v);
}

void loop() {}Not only the code is failing to provide the right answer and I don't get any warning and it requires 5442 bytes of Flash and 400 bytes of SRAM

If I do this

Code: [Select]

char msgBuf[50] = "1234,Hello";

void setup() {
  long v=0;
 
  Serial.begin(115200);
  v = strtol(msgBuf, NULL, 10);
  Serial.print(F("v=")); Serial.println(v);
}

void loop() {}I get the right answer and it requires 2802 bytes of Flash and 240 bytes of SRAM

In summary: Your code wastes 3 360 bytes of Flash and 160 bytes of precious SRAM and does not deliver the expected result nor warning.

So... again understanding what you do and being in control is important...

[drmpf]

Some Arduino users want to learn C/C++ programming,  Some just want to get their project running reliably.  SafeString improves reliability for string handling.  They can choose to re-write the c-string handling with the necessary char counting, null termination and index bounds checking, from stratch, OR use a pre-written pre-tested library.

I agree array handling is a necessary technique, but c-string processing is very easy to code incorrectly and takes a lot of effort to continually get correct. SafeString avoids that extra work.  Well  worth the effort to learn the SafeString library.  The detailed tutorial helps a lot.

J-M-L  I don't agree that using a library deprives the user of gaining the same knowledge. In fact the opposite, the library provides the code that does the job and can be used as learning examples.

The parsing convention you are proposing is un-reliable and does not detect data or format errors see the examples below.

Re atoi() versus SafeString.toInt()   

C/C++ programmers often ignore the function returns and that is the problem in this case.  
SafeString.toInt() returns false if the conversion 'fails'  See the code and code comments in my posting above. When SafeString.toInt() returns false the argument int is left unchanged.

If you don't check the return you won't know if the conversion failed or not.
I did not add an error msg for this error because it is not an error you can usually fix by increasing the char buffer size or changing the code.  It is most commonly an input data error that needs to be caught and handled.

The problems with atoi() mean you cannot write robust code that detects bad data.  Its deficiencies are mentioned in the SafeString tutorial and covered in detail in the SafeStringToNum.ino example sketch that comes with the library.

As per J-M-L's post to even consider using atoi(), your allowable data range must exclude 0.
So atoi() is not at all useful for general purpose coding.

Just because atoi() returns an answer in no way implies the input was that int
For example
if atoi() returns 0 you have no idea if the input was 0 or some non-int or just an empty string
if atoi() returns 5 you don't know if the input was 5 or 5a or 5somethings
if atoi() returns -7616 you don't know if the input was -7616 or 123456  (on Uno)

Code: [Select]

void setup() {
  long v=0;
  Serial.begin(115200);
  Serial.println(F(" atoi() tests"));
  v = atoi("");
  Serial.print(F(" \"\"      v=")); Serial.println(v);
  v = atoi("5a");
  Serial.print(F(" \"5a\"   v=")); Serial.println(v);
  v = atoi("123456");
  Serial.print(F(" \"123456\" v=")); Serial.println(v);
}
void loop() {}

Not many people would consider 5a or 5a, to be a number.  
SafeString toInt() returns false for all of these.

Why did you decide to only accept the conversion if there are just trailing spaces (or similar)?

atoi() ignores leading white space as does strtol() so it did not seem reasonable to complain about trailing white space.  Also in Excel it does not cause number conversions to fail.  

So as a general purpose method for the conversion of strings to ints,  atoi() fails miserably.
Just because C/C++ provides a poorly coded method does not mean we should use it.
Arduino String toInt() is no better.  
They should not be recommended to beginners as the way to do things.  It will only cause them problems later. 

It is possible to code a reliable conversion, which is what SafeString.toInt() does.  

Code: [Select]

bool SafeString::toInt(int &i) {
  cleanUp();  // does nothing if not wrapping a char[]
  if (len == 0) {
    return false; // not found
  }
  char* endPtr;
  long result = strtol(buffer, &endPtr, 10);
  if (result > INT_MAX) {
    return false;
  }
  if (result < INT_MIN) {
    return false;
  }
  // check endPtr to see if number valid 5a is invalid,  5. is valid
  if (endPtr == buffer)  { // no numbers found at all
    return false;
  } // else
  // else check for trailing white space
  while (*endPtr != '\0') {
    if (!isspace(*endPtr)) { // number not terminated by white space
      return false;
    }
    endPtr++;
  }
  // else all OK
  i = result;
  return true; // OK
}

I think is un-reasonable to expect beginners, or even experienced programmers, to have to code that when they want to do a conversion that reliably converts only valid ints.  Using a pre-code, pre-tested library instead makes more sense.

C's c-string methods have a lot of acknowledged problems for coders, which is why C++ Strings were introduced.  SafeString fixes the problems of using Strings on small micros and also fixes these problems with number conversions.

[aarg]

'atoi()' is a straw man. It's deprecated, and probably in some part for the weaknesses you've identified. In any case, it is not possible to predict what the format of an entire string is, it seems reasonable to restrict number conversion to rules that are consistent with that purpose, for example you think '5' and '5A' should return something different. That seems wrong to me. '5a' might not be an error, you should not flag it as such unless you know its purpose (such as delimiter or other symbol). It could just as easily be '5,' and the single digit really is a valid number. If you algorithmically are expecting a decimal number first, then the 'A' is either an error, or a part of the string with another purpose. So parsing '5A' should return 5 and leave the pointer at 'A' for further processing. That is exactly what strtol() does. Trailing spaces seem like a non-issue to me for similar reasons. 'strtol()' stops at the first non-numeric character, if it's a space, it either belongs there, or not. If not, your program probably must make some decisions about it before continuing the parse. If it is one or more spaces, and you use 'strtol()' again, it will throw them away cleanly.

Allowing the routines to find the effective beginning and end of the string it is converting, seems necessary or at least efficient, but for them to do anything more is not appropriate because they don't have access to any context for the number as a subset of the entire string.

A really robust validation method usually requires that the string be broken into tokens before any numeric conversion takes place. It is not the job of numeric conversion routines to parse the string. That is why they are so simple minded. It is only in beginners or the simple test sketches we see so often in Arduino, where numeric conversion is "thrown inline". In that case, it's not the routines fault if it gets fooled by bad data thrown at it.

[Deva_Rishi]

Some Arduino users want to learn C/C++ programming,  Some just want to get their project running reliably.  SafeString improves reliability for string handling.

Yeah sure but by now you've hi-jacked this topic. I'm sure your library is great, mention it once without negating the question asked

What is wrong with my char to int conversion from serial input?

That is the question.

'atoi()' is a straw man. It's deprecated, and probably in some part for the weaknesses you've identified.

deprecated ? well i don't use it, i just read the characters 1 by 1 in a function and return '-1' if anything doesn't agree with what i expect, but these are simple core choices you can make in conversion. It depends on what in your opinion, is a valid input value and how you want it processed.

[drmpf]

It is only in beginners or the simple test sketches we see so often in Arduino, where numeric conversion is "thrown inline"

My view is that this forum should educate user on how to code correctly and so should avoid misleading them into thinking atoi() is the way to do things.

In any case, it is not possible to predict what the format of an entire string is.

I disagree, if you don't know what the format of the entire string should be you have no hope of parsing it correctly.  The process must be:-  Define the acceptable input format. Parse to that format.  Check the parsed tokens are valid.
If the token is supposed to be an int then I expect "5" to pass but "5A" to fail.  But if the token is supposed to be in hex format then I expect both to pass.  You really do need to know the expected format.

A really robust validation method usually requires that the string be broken into tokens before any numeric conversion takes place. It is not the job of numeric conversion routines to parse the string.. That is why they are so simple minded.

I don't object to simple minded conversion routines (SafeString.toInt() is very simple minded).  I do object to atoi() that returns garbage, with no indication it failed.


The parsing needs to done before trying to convert.  SafeString has a number of tokenizing methods and readers that support this parsing.  
strtol() is not a parsing method it has no concept of delimiters.  Sure you can uses it to 'parse' but it takes a lot of extra code to do that.  Try coding using strol() to parse "0a,55.5,b" and then see what happens if you pass the bad data "aa,,b".


So to iterate your statement

A really robust validation method usually requires that the string be broken into tokens before any numeric conversion takes place.

To which I would add  and the conversion should return an error if the token is not numeric format expected, e.g. int, hex, long, float etc

[Deva_Rishi]

I disagree, if you don't know what the format of the entire string should be you have no hope of parsing it correctly.  The process must be:-  Define the acceptable input format. Parse to that format.  Check the parsed tokens are valid.
If the token is supposed to be an int then I expect "5" to pass but "5A" to fail.  But if the token is supposed to be in hex format then I expect both to pass.  You really do need to know the expected format.

Totally with you on this.

My view is that this forum should educate user on how to code correctly and so should avoid misleading them into thinking atoi() is the way to do things.

For sure but first the code should work if the user input is correct, and it wasn't doing that. That is not the fault of atoi() but of the cutting off of the first character. No function would have returned the proper value with the first character missing.

SafeString has a number of tokenizing methods and readers that support this parsing. 

But to be honest it is not very hard to write one which does exactly what you want, and highly educative ! And as i said before you ared hijacking and it is not the first time.

[drmpf]

@Deva_Rishi  My apologies for these digressions.  
Ignoring the SafeString solution, the original code problems still seem to be outstanding.
Perhaps @jaddow can post a text description of what he is try to achieve.

[Deva_Rishi]

My apologies for these digressions

accepted.

the original code problems still seem to be outstanding.

i think i actually fixed it, but we haven't heard back from the OP. Or at least using atoi(), which should work if the input is correct.

[J-M-L]

... there was long rang here but no point, it's not worth a debate we already had,

I just disagree with @drmpf on many fronts - including the frequent hijadking to advertise his library as the 8th wonder of the world and the cure for all diseases. It's not, there is no one size fits all and I'd be more supportive if there was not this snake oil seller attitude and misleading way too broad statements.