VirtualWire or String causes arduino to hang

I've written two pieces of codes that uses VirtualWire to transport a 0-1023 integer, VirtualWire_Interger_TX and RX. This code works as far as I can tell without any problems.
But when I tried to combine the RX part with another piece of code my Arduino just started hanging after a few serial messages. From googling around a bit I've seen that there is some sort of problem with the string library at the moment and basically I wonder if I should bother with this problem or just wait to have the issue fixed in an update?

From looking at the Serial Monitor output and comparing to the working example it seems that the hangs are caused when calculating the Sensor1Data/potVal to a color.

Wireless_RGB_LED_Controller (causes Arduino to hang after 5-20 messages)

/* 
 RGB LED controller
 4 modes: off, color select, color pulse and random cycle/pulse
 By Markus Ulfberg 2009-05-19
 
 Updated to Version 2 - 2010-01-13 (Not publicly released)
 Updated to Version 3 - 2011-12-14
 
 Thanks to: Ladyada, Tom Igoe and 
 everyone at the Arduino forum for excellent 
 tutorials and everyday help. 
 
 TODO: 
 1. Use millis for debounce instead of delay. 
 
 */

#include <VirtualWire.h>

// LED's
int ledPin = 13;

// Define and set sensors to zero   
int Sensor1Data = 0;
int potVal = 0;

// RF Transmission container
char Sensor1CharMsg[4]; 

// set the ledPins
int ledRed = 10;
int ledGreen = 9;
int ledBlue = 6;

// LED Power variables
byte redPwr = 0;
byte greenPwr = 0;
byte bluePwr = 0;

void setup()
{

  // sets the digital pin as output
  pinMode(ledPin, OUTPUT);      

  // VirtualWire 
  // Initialise the IO and ISR
  // Required for DR3100
  vw_set_ptt_inverted(true); 
  // Bits per sec
  vw_setup(2000);	 

  // Start the receiver PLL running
  vw_rx_start();       

  pinMode(ledRed, OUTPUT);
  pinMode(ledGreen, OUTPUT);
  pinMode(ledBlue, OUTPUT);

  // serial for debugging purposes only
  Serial.begin(9600);
}

void loop()
{  
  dataRX();
  colorControl();


  // DEBUG 
  Serial.print("Sensor 1: ");
  Serial.print(Sensor1Data);

  Serial.print(" redPwr: ");
  Serial.print(redPwr, DEC);
  Serial.print(" greenPwr: ");
  Serial.print(greenPwr, DEC);
  Serial.print(" bluePwr: ");
  Serial.println(bluePwr, DEC);

  // End debug     
  // END DEBUG

} // END loop()

void dataRX()
{
  uint8_t buf[VW_MAX_MESSAGE_LEN];
  uint8_t buflen = VW_MAX_MESSAGE_LEN;

  // Non-blocking
  if (vw_get_message(buf, &buflen)) 
  {
    int i;
    // Turn on a light to show received good message 
    digitalWrite(13, true); 

    // Message with a good checksum received, dump it. 
    for (i = 0; i < buflen; i++)
    {            
      // Fill Sensor1CharMsg Char array with corresponding 
      // chars from buffer.   
      Sensor1CharMsg[i] = char(buf[i]);
    }

    // Null terminate the char array
    // This needs to be done otherwise problems will occur
    // when the incoming messages has less digits than the
    // one before. 
    Sensor1CharMsg[buflen] = '\0';

    // Convert Sensor1CharMsg Char array to integer
    Sensor1Data = atoi(Sensor1CharMsg);
    
    // Turn off light to and await next message 
    digitalWrite(13, false);
  }
  delay(500);
}

// lightMode 1 
void colorControl() {

  // read the potentiometer position 
  potVal = Sensor1Data;

  // RED > ORANGE > YELLOW
  if (potVal > 0 && potVal < 170) {
    redPwr = 255;
    bluePwr = 0;
    greenPwr = map(potVal, 0, 170, 0, 255);
  }

  // YELLOW > LIME?? > GREEN 
  if (potVal > 170 && potVal < 341) {
    greenPwr = 255;
    bluePwr = 0;
    redPwr = map(potVal, 341, 170, 0, 255);
  }

  // GREEN > TURQOUISE
  if (potVal > 341 && potVal < 511) {
    greenPwr = 255;
    redPwr = 0;
    bluePwr = map(potVal, 341, 511, 0, 255);
  }

  // TURQOUISE > BLUE  
  if (potVal > 511 && potVal < 682) {
    bluePwr = 255;
    redPwr = 0;
    greenPwr = map(potVal, 682, 511, 0, 255);
  }

  // BLUE > PURPLE 
  if (potVal > 682 && potVal < 852) {
    bluePwr = 255;
    greenPwr = 0;
    redPwr = map(potVal, 682, 852, 0, 255);
  }

  // PURPLE > RED
  if (Sensor1Data > 852 && potVal < 1023) {
    redPwr = 255;
    greenPwr = 0;
    bluePwr = map(potVal, 1023, 852, 0, 255);
  } 

  // Display colors 
  colorDisplay();
}        

// Displays the colors when called from other functions
void colorDisplay() {
  analogWrite(ledRed, redPwr);
  analogWrite(ledGreen, greenPwr);
  analogWrite(ledBlue, bluePwr);
}

VirtualWire_Integer_RX (works)

/* 

Sensor Receiver 
By Markus Ulfberg 2012-07-06

Gets a sensor reading 0-1023 in a char array
from RF Transmitter unit via VirtualWire 
converts char array back to integer

*/

#include <VirtualWire.h>

// LED's
int ledPin = 13;

// Sensors 
int Sensor1Data;

// RF Transmission container
char Sensor1CharMsg[4]; 

void setup() {
  Serial.begin(9600);
  
  // sets the digital pin as output
  pinMode(ledPin, OUTPUT);      
    
    // VirtualWire 
    // Initialise the IO and ISR
    // Required for DR3100
    vw_set_ptt_inverted(true); 
    // Bits per sec
    vw_setup(2000);	 
    
    // Start the receiver PLL running
    vw_rx_start();       

} // END void setup

void loop(){
    uint8_t buf[VW_MAX_MESSAGE_LEN];
    uint8_t buflen = VW_MAX_MESSAGE_LEN;
    
    // Non-blocking
    if (vw_get_message(buf, &buflen)) 
    {
	int i;
        // Turn on a light to show received good message 
        digitalWrite(13, true); 
	
        // Message with a good checksum received, dump it. 
        for (i = 0; i < buflen; i++)
	{            
          // Fill Sensor1CharMsg Char array with corresponding 
          // chars from buffer.   
          Sensor1CharMsg[i] = char(buf[i]);
	}
        
        // Null terminate the char array
        // This needs to be done otherwise problems will occur
        // when the incoming messages has less digits than the
        // one before. 
        Sensor1CharMsg[buflen] = '\0';
        
        // Convert Sensor1CharMsg Char array to integer
        Sensor1Data = atoi(Sensor1CharMsg);
        
        
        // DEBUG 
        Serial.print("Sensor 1: ");
        Serial.println(Sensor1Data);
        
        // END DEBUG
                
        // Turn off light to and await next message 
        digitalWrite(13, false);
    }
}

VirtualWire_Interger_TX (works)

/* 

Sensor Transmitter 
By Markus Ulfberg 2012-07-06

Takes a sensor reading 0-1023
converts it to a char array and sends 
to RF receiver unit via VirtualWire  

*/

#include <VirtualWire.h>

// LED's
const int ledPin = 13;

// Sensors 
const int Sensor1Pin = A2;
// const int Sensor2Pin = 3; 

int Sensor1Data;
//int Sensor2Data;
char Sensor1CharMsg[4]; 

void setup() {

 // PinModes 
 // LED 
 pinMode(ledPin,OUTPUT);
 // Sensor(s)
 pinMode(Sensor1Pin,INPUT);
 
 // for debugging
 Serial.begin(9600); 
 
 // VirtualWire setup
 vw_setup(2000);	 // Bits per sec


}

void loop() {
  
  // Read and store Sensor 1 data
  Sensor1Data = analogRead(Sensor1Pin);
  
  // Convert integer data to Char array directly 
  itoa(Sensor1Data,Sensor1CharMsg,10);
  
  /*
  // DEBUG
  Serial.print("Sensor1 Integer: ");
  Serial.print(Sensor1Data);
  Serial.print(" Sensor1 CharMsg: ");
  Serial.print(Sensor1CharMsg);
  Serial.println(" ");
  delay(1000);

  // END DEBUG
  */
 
 digitalWrite(13, true); // Turn on a light to show transmitting
 vw_send((uint8_t *)Sensor1CharMsg, strlen(Sensor1CharMsg));
 vw_wait_tx(); // Wait until the whole message is gone
 digitalWrite(13, false); // Turn off a light after transmission
 delay(200); 
 
} // END void loop...

From googling around a bit I've seen that there is some sort of problem with the string library at the moment

No. The string library is fine. The String library is a different story. It's use of dynamic memory allocation and deallocation is what causes the problem.

I wonder if I should bother with this problem or just wait to have the issue fixed in an update?

The Arduino team is in no hurry to address the problem. It is outside their control, and requires a newer version of the whole compile/link/upload tool chain.

You'll be waiting a while.

Anyway, I don't see you using the String class, so you have some other problem that fixing the String class/tool chain is not going to correct.

  // Read and store Sensor 1 data
  Sensor1Data = analogRead(Sensor1Pin);
  
  // Convert integer data to Char array directly 
  itoa(Sensor1Data,Sensor1CharMsg,10);

How are you going to fit '1', '0', '2', '3' and the trailing NULL in a 4 element array?

Your array on the receiver is too small, too.

PaulS:
How are you going to fit '1', '0', '2', '3' and the trailing NULL in a 4 element array?

Your array on the receiver is too small, too.

char Sensor1CharMsg[4];

Isn't the above a 5 element array (0 1 2 3 4), or have I got it wrong?

PaulS:
Anyway, I don't see you using the String class, so you have some other problem that fixing the String class/tool chain is not going to correct.

Ok thank you for helping me with that. Any thoughts on what might actually cause the hangs?

Isn't the above a 5 element array (0 1 2 3 4), or have I got it wrong?

No, it is not. The value in the brackets is the number of elements, not the upper index.

Any thoughts on what might actually cause the hangs?

What I like to do is fix the bugs as a become aware of them, and see what effect that has on the program. You have a known bug. Fix it, and let us know what impact that has on your problem.

Thanks for the tip!
So now I've changed both the transitter and receiver code as below and uploaded to both boards. However the change in behaviour seems minimal. If anything I do get a few more lines to run by in Serial Monitor before things hang again.

char Sensor1CharMsg[5];

All parts of the code work if I run them separately so I can't really tell where the problem lies. Is there something I can do to get more info on what causes the crash?

What about the Serial.print lines? If you remove all those, does the program work? I had a problem recently when I started using Serial.print for debugging and it ended up restarting the arduino over and over again while it was trying to run the code. I think it was something to do with a memory issue.

Zachary

char Sensor1CharMsg[4]; 
...

void dataRX()
{
  ...
  uint8_t buflen = VW_MAX_MESSAGE_LEN;

  // Non-blocking
  if (vw_get_message(buf, &buflen)) 
  {
    ...
    // Message with a good checksum received, dump it. 
    for (i = 0; i < buflen; i++)
    {            
      // Fill Sensor1CharMsg Char array with corresponding 
      // chars from buffer.   
      Sensor1CharMsg[i] = char(buf[i]);
    }

Classic buffer overflow bug. Even if you have increased Sensor1CharMsg to 5 elements, the version of VirtualWire.h I just looked at (1.9) defines VW_MAX_MESSAGE_LEN as 30. So the above code will write beyond the end of the buffer, if you receive a message more than 4 or 5 bytes long. Never trust input to be in the expected form, even if it is (in theory) under your control.

Where did you find 1.9? I found 1.5 on this page:

http://www.pjrc.com/teensy/td_libs_VirtualWire.html

On my hard disk the version that I have claims to be 1.6 (!) and in that, the vw_get_message function appears to only return the requested number of bytes, at most.

http://www.open.com.au/mikem/arduino/

Yes, but he's asking for VW_MAX_MESSAGE_LEN bytes, instead of 4 (or 5).

  for (i = 0; i < buflen; i++)
    {            
      Sensor1CharMsg[i] = char(buf[i]);
    }

Oh yes I see. That loop is the issue. Maybe a memcpy would be simpler. And choose the length as the destination length, not the source length.

...or convert the data directly from 'buf' instead of copying it to Sensor1CharMsg.

Thanks for all the suggestions but I'm afraid I'm still going nowhere here.

ZacharyBruno:
What about the Serial.print lines? If you remove all those, does the program work?

Nope that didn't work.

This I have to research more if I should go ahead and do. So I'll put this on hold if the next one is simpler.

dc42:
...or convert the data directly from 'buf' instead of copying it to Sensor1CharMsg.

I tried this but that didn't work. Allthough I doubt I did it right. Am I looking for a char to integer converter or something else?

Sensor1Data = atoi(buf);

Error Message

conversion from 'uint8_t*' to 'const char*'
rgb_mixer_RF_controlled_pulse_makkan_v4_ino:115: error: initializing argument 1 of 'int atoi(const char*)'

Makkan:
Thanks for all the suggestions but I'm afraid I'm still going nowhere here.

Please post your amended code.

/* 
  RGB LED controller
  4 modes: off, color select, color pulse and random cycle/pulse
  By Markus Ulfberg 2009-05-19
  
  Updated to Version 2 - 2010-01-13 (Not publicly released)
  Updated to Version 3 - 2011-12-14

  Thanks to: Ladyada, Tom Igoe and 
  everyone at the Arduino forum for excellent 
  tutorials and everyday help. 

  TODO: 
  1. Use millis for debounce instead of delay. 

*/

#include <VirtualWire.h>

// LED's
int ledPin = 13;

// Sensors 
int Sensor1Data;
int potVal;

// RF Transmission container
char Sensor1CharMsg[5]; 

// set the ledPins
int ledRed = 10;
int ledGreen = 9;
int ledBlue = 6;

// LED Power variables
byte redPwr = 0;
byte greenPwr = 0;
byte bluePwr = 0;

void setup()
{
  
  // sets the digital pin as output
  pinMode(ledPin, OUTPUT);      
    
    // VirtualWire 
    // Initialise the IO and ISR
    // Required for DR3100
    vw_set_ptt_inverted(true); 
    // Bits per sec
    vw_setup(2000);	 
    
    // Start the receiver PLL running
    vw_rx_start();       

  pinMode(ledRed, OUTPUT);
  pinMode(ledGreen, OUTPUT);
  pinMode(ledBlue, OUTPUT);
    
  // serial for debugging purposes only
  Serial.begin(9600);
}

void loop()
{  
  dataRX();
  colorControl();
  
  // DEBUG 
  Serial.print("Sensor 1: ");
  Serial.print(Sensor1Data);
  Serial.print(" potVal: ");
  Serial.print(potVal);
  
  Serial.print(" redPwr: ");
  Serial.print(redPwr, DEC);
  Serial.print(" greenPwr: ");
  Serial.print(greenPwr, DEC);
  Serial.print(" bluePwr: ");
  Serial.println(bluePwr, DEC);
  
  // End debug     
  // END DEBUG

} // END loop()

void dataRX()
 {
   uint8_t buf[VW_MAX_MESSAGE_LEN];
   uint8_t buflen = VW_MAX_MESSAGE_LEN;
    
    // Non-blocking
    if (vw_get_message(buf, &buflen)) 
    {
	int i;
        // Turn on a light to show received good message 
        digitalWrite(13, true); 
	
        /* Commented out for buffer to integer directly 
        
        // Message with a good checksum received, dump it. 
        for (i = 0; i < buflen; i++)
	{            
          // Fill Sensor1CharMsg Char array with corresponding 
          // chars from buffer.   
          Sensor1CharMsg[i] = char(buf[i]);
	}
        
        // Null terminate the char array
        // This needs to be done otherwise problems will occur
        // when the incoming messages has less digits than the
        // one before. 
        Sensor1CharMsg[buflen] = '\0';
        
        // Convert Sensor1CharMsg Char array to integer
        Sensor1Data = atoi(Sensor1CharMsg);
        */
        
        // Convert buffer directly to Integer String 
        Sensor1Data = atoi(buf);
        
        // Turn off light to and await next message 
        digitalWrite(13, false);
  }
  // delay(500);
}

// lightMode 1 
void colorControl() {
  
  // read the potentiometer position 
  potVal = Sensor1Data;
  
  // RED > ORANGE > YELLOW
   if (potVal > 0 && potVal < 170) {
     redPwr = 255;
     bluePwr = 0;
     greenPwr = map(potVal, 0, 170, 0, 255);
   }
 
   // YELLOW > LIME?? > GREEN 
   if (potVal > 170 && potVal < 341) {
     greenPwr = 255;
     bluePwr = 0;
     redPwr = map(potVal, 341, 170, 0, 255);
   }

    // GREEN > TURQOUISE
    if (potVal > 341 && potVal < 511) {
      greenPwr = 255;
      redPwr = 0;
      bluePwr = map(potVal, 341, 511, 0, 255);
    }
 
   // TURQOUISE > BLUE  
   if (potVal > 511 && potVal < 682) {
     bluePwr = 255;
     redPwr = 0;
     greenPwr = map(potVal, 682, 511, 0, 255);
   }
 
   // BLUE > PURPLE 
   if (potVal > 682 && potVal < 852) {
     bluePwr = 255;
     greenPwr = 0;
     redPwr = map(potVal, 682, 852, 0, 255);
   }
 
   // PURPLE > RED
   if (Sensor1Data > 852 && potVal < 1023) {
     redPwr = 255;
     greenPwr = 0;
     bluePwr = map(potVal, 1023, 852, 0, 255);
   } 
   
  // Display colors 
  colorDisplay();
}        

// Displays the colors when called from other functions
void colorDisplay() {
  analogWrite(ledRed, redPwr);
  analogWrite(ledGreen, greenPwr);
  analogWrite(ledBlue, bluePwr);
}

Makkan:
I tried this but that didn't work. Allthough I doubt I did it right. Am I looking for a char to integer converter or something else?

Sensor1Data = atoi(buf);

Error Message

conversion from 'uint8_t*' to 'const char*'

rgb_mixer_RF_controlled_pulse_makkan_v4_ino:115: error: initializing argument 1 of 'int atoi(const char*)'

Try:

Sensor1Data = atoi((const char*)buf);

dc42:
Try:

Sensor1Data = atoi((const char*)buf);

That compiled, as I suspect you already knew. Unfortunately I will have to wait a couple of hours before I can try it live. However as I do like to know what I'm coding could you please tell me or point me in a good direction to find reading material as to what actually is done in that line. Am I just stating that buf is a const char* or does (const char*)buf perform some sort of change?

Am I just stating that buf is a const char* or does (const char*)buf perform some sort of change?

You're just telling the compiler that you want the pointer to "buf" to be treated as though it were a "const char*", even though you know it is a pointer to a different type.
Because it is a pointer to a datatype with the same size, this is normally perfectly OK, particularly as in this case, where the buffer simply contains ASCII data.

dc42:
Try:

Sensor1Data = atoi((const char*)buf);

Sorry to say I got the same problem as before. A couple of lines fly by in the Serial Monitor before it hangs. Commenting out the Serial.print lines does not help either.

Again if I don't include the colorControl(); function everything seems to work just fine.

Makkan:
Again if I don't include the colorControl(); function everything seems to work just fine.

Ah, I missed that bit. The version of VirtualWire I am looking at (1.9) uses timer 1. That means that you can't use PWM on pins 9 and 10.