Issues with atoi conversion

As part of my MQTT project, I send 999 for a status report. However, when I send it the second time, the value gets changed to 0. I have narrowed the causing code to this snippet:

static void callback(char* topic, byte* payload, unsigned int length) {
  char PublishData[100];
Serial.print(F("Payload: "));
Serial.write(payload,length); // shows 999 both times
Serial.print(F(" - "));

  int readVar = (unsigned int)atoi((char*) payload);

Serial.println(readVar);      // first time shows 999, second time shows 0

and here is the output from the Serial log:

Payload: 999 - 999
Payload: 999 - 0
Payload: 999 - 999
Payload: 999 - 0
Payload: 999 - 999
Payload: 999 - 0
Payload: 999 - 999
Payload: 999 - 0

I need the byte* payload to get converted to an int but it appears the atoi line is wrong, somehow. What have I done wrong?

atoi can deal with negative numbers, why the unsigned cast?

also byte is unsigned, char is signed.

Where is the code which is calling this function ? I suspect the defect is there.

In the first case, you output a specific number of ascii values and do not rely on the payload being null-terminated.

In the second case you use atoi which does rely on the sequence of ascii values being null-terminated (and also assumes the characters are all decimal digits).

The fact that these behave differently suggests that the array is in fact not null terminated.

If that variable is only ever going to receive ascii characters, they would be better passed as a null-terminated c-string (the length argument is then redundant).

If you don't know that you have a null-terminated array of numeric ascii characters, you need to do some sanity checking and convert the unknown input to a c-string before you call atoi to try to parse it, and be aware that if it can't be parsed into a number then atoi() is still going to return a result - zero in this case.

I have narrowed the causing code to this snippet:

http://snippers-r-us.com might be able to help you. Here, we need to see ALL of your code. There are any number of reasons for the output you get that are explained by code you didn't post.

Thanks for the replies, everyone - I pulled an all-nighter and found that payload wasn't null-temrinated, just like PeterH said. I can't change the way it's sent (as it's coming from a third party program (MQTT)) but I converted it to a String then sorted it out from there.

Sorry for omitting the entirety of my code - I had managed to narrow it down to the code that I posted (when removing the atoi line the value didn't change every second time) but yes, I should have put it all in. For those still interested, here it is below:

#include <SPI.h>
#include <Ethernet.h>
#include <PubSubClient.h>

#define datapin 2
#define clockpin 3
#define latchpin 4
int current = 0;		// 00000000
const int relayValues[]={
  1,2,4,8,16,32,64,128,256,512,1024,2048,4096,8192,16384,32768};
int relayCheck[] = {
  0,0,0,0,0,0,0,0,0,0,0,0,0,0,0,0};
int changeVal = 0;
//char OnOrOff[] = {"off","on"};
//char DataResult[100]="Publishing data - relay "; //24
String OnOrOff;
String DataMash;
String DataResult;
byte mac[] = {  
  0xDE, 0xC0, 0xDE, 0xC0, 0xFF, 0xEE };
byte mqttserver[] = { 
  192, 168, 2, 7 };
byte ip[] = { 
  192, 168, 2, 101 };

//char DataMash[30]="                             ";

EthernetClient ethClient;
PubSubClient clientr(mqttserver, 1883, callback);

/////////////////// calculateRelayVal ///////////////////
int calculateRelayVal(int newVal) {

  // if newVal==0 then return value as 11111111
  // newVal has relay we need to toggle
  // referenced in relayCheck as newVal-1
  int intRelayToWorkWith = relayValues[(newVal-1)];

  if(relayCheck[(newVal-1)]==0) {
    // relay is currently off, turn on
    changeVal = current | intRelayToWorkWith;
    relayCheck[(newVal-1)]=1;
  } 
  else {
    // relay is currently on, turn off
    changeVal = current ^ intRelayToWorkWith;
    relayCheck[(newVal-1)]=0;
  }

  current = changeVal;
  return current;
}

/////////////////// doWork ///////////////////
void doWork(int val) {

  // as relays need a '0' to turn on and a '1' to turn off, need to NOT this value
  unsigned int realVal = ~val;

  uint8_t rVlow = realVal & 0xff;
  uint8_t rVhigh = (realVal >> 8);

  digitalWrite(latchpin,LOW);
  shiftOut(datapin,clockpin,MSBFIRST,rVhigh);
  shiftOut(datapin,clockpin,MSBFIRST,rVlow);
  digitalWrite(latchpin,HIGH);

}

/////////////////// callback ///////////////////
static void callback(char* topic, byte* payload, unsigned int length) {
  char PublishData[24];
  char tina[4];
  String tp = (char*)payload;
  (tp.substring(0,length)).toCharArray(tina,4);
  unsigned int readVar = (unsigned int)atoi(tina);

  if(readVar<0 || (readVar>16 && readVar!=999)) {
    // if input is outside of specification (greater than 16 and not 999 (status check) or less than zero then make -1
    // -1 is used in the switch below to indicate invalid input
    readVar=-1; 
  }
  if(readVar==999){
    String strGetStatus="S:";
    for(int i=0;i<16;i++){
      strGetStatus+=relayCheck[i];
    }
    strGetStatus.toCharArray(PublishData,100);
    //    clientr.publish("RelayControlReport",PublishData);
    clientr.publish("WebControl",PublishData);
  } 
  else {
    if(readVar>=0 && readVar<=16) {
      int registerValue=0;
      if(readVar > 0) { 
        registerValue = calculateRelayVal(readVar); 
      } 
      else {
        current=0;
        for(int i=0;i<16;i++){
          relayCheck[i]=0;
        }
      }
      doWork(registerValue);

      if(relayCheck[(readVar-1)]==1){
        OnOrOff=" on";
      } 
      else {
        OnOrOff=" off"; 
      }

      if(readVar!=0){
        DataMash="Relay: ";
        DataResult = DataMash + readVar + OnOrOff;
        DataResult.toCharArray(PublishData,100); 
      } 
      else {
        DataResult="All relays off";
        DataResult.toCharArray(PublishData,100);
      }
      clientr.publish("RelayControlReport",PublishData);
    }
  }
}

/////////////////// setup ///////////////////
void setup()
{
  pinMode(datapin,OUTPUT);
  pinMode(clockpin,OUTPUT);
  pinMode(latchpin,OUTPUT);

  digitalWrite(latchpin,LOW);
  shiftOut(datapin,clockpin,MSBFIRST,-1);
  shiftOut(datapin,clockpin,MSBFIRST,-1);
  digitalWrite(latchpin,HIGH);

  delay(500);
  Serial.begin(9600);
  Serial.println(F("Scouris - MQTT Control, booting up."));
  Serial.print(F("Eth start .."));
  Ethernet.begin(mac,ip);

  // below is for when running with DHCP
  Serial.print(F("up, IP: "));
  for (byte thisByte = 0; thisByte < 4; thisByte++) {
    // print the value of each byte of the IP address:
    Serial.print(Ethernet.localIP()[thisByte], DEC);
    Serial.print(F(".")); 
  }
  Serial.println("");

  if(clientr.connect("arduinoClient")) {
    clientr.publish("checkInTopic","Relay Controller Online");
    clientr.publish("RelayControlReport","Relay Controller Online");
    clientr.subscribe("RelayControl"); 
  }
  doWork(0);
  //  delay(500);
}

/////////////////// loop ///////////////////
void loop()
{
  clientr.loop();
}

scouris:
Thanks for the replies, everyone - I pulled an all-nighter and found that payload wasn't null-temrinated, just like PeterH said. I can't change the way it's sent (as it's coming from a third party program (MQTT)) but I converted it to a String then sorted it out from there.

I wouldn't recommend using Strings at all, and particularly not just to terminate a char array. A better (IMO) solution was declare a local char array long enough to include the received byte array plus null terminator, and do a strncpy or memcpy to copy the incoming message into that and then null-terminate it.