Weirdness with serialEvent and built Strings

So here’s the situation, and it’s WEIRD …

I have some code that does a bunch of stuff … but when I added the option to send commands into my code via the serialEvent() method, I would type a string but the string that the method produced would only be the first character of the string I typed into the serial monitor.

For example, if I typed in: dobutton

The string would only contain the letter d

So here’s where it gets interesting:

If I comment out enough variables that I have defined, then suddenly I get the whole string that I typed into the serial monitor.

This code is a program I was writing that will act as a remote control for a cable box (at least I think it will, I haven’t successfully debugged it yet because I ran into this problem).

#include <IRremote.h>
#include <EnableInterrupt.h>
uint32_t last_interrupt1_time     = 0;
uint32_t last_interrupt2_time     = 0;
const int DEBOUNCE_DELAY  PROGMEM = 150;
#define button1 7
#define button2 6
boolean one             = false;
boolean two             = false;
const String commands[]  PROGMEM = {"Menu","OK","Up","Down","Left","Right","Exit","Mute","Vol+","Vol-","Ch+","Ch-","Last","Info","Menu","1","2","3","4","5","6","7","8","9","0","Stop","Pause","Play","FFWD","REW","MYDVR","PageUp","PageDown","A","B","C","D"};
const String cmdHex[]    PROGMEM = {"36C127","366133","36812F","37A10B","37810F","364137","366932","E0E0F00F","E0E0E01F","E0E0D02F","377111","36F121","36E123","36213B","373918","36113D","37111D","36912D","37910D","365135","375115","36D125","37D105","363139","373119","365934","374117","37990C","36293A","37291A","36C926","36D924","37D904","37E902","36193C","37191C","F7E10"};
String inputString="";


void sendCommand(String command) {
  String cmd = "";
  command.toLowerCase();
  Serial.println("Command: " + command);
/*
 * Code that actually does stuff would go here and 
 * would utilize the cmd variable that I defined.
 */
}

long hstol(String recv){
  return strtol(recv.c_str(), NULL, 16);
}

void setup() {
  Serial.begin(9600);
  pinMode(button1, INPUT_PULLUP);
  pinMode(button2, INPUT_PULLUP);
  enableInterrupt(button1, button1Handler, FALLING);
  enableInterrupt(button2, button2Handler, FALLING);
  Serial.println("Ready");
}

void loop() {
}

void button1Handler() {
  uint32_t interrupt1_time = millis();
  if (interrupt1_time - last_interrupt1_time > DEBOUNCE_DELAY) {one = true;}
  last_interrupt1_time = interrupt1_time;
}
void button2Handler() {
String testVariable="1111";
}

void doString() {
  Serial.println(inputString);
  sendCommand(inputString);
  inputString = "";
}

void serialEvent() {
  while (Serial.available()) {
    char inChar = (char)Serial.read();
    if (inChar == '\n') {doString();}
    else {inputString += inChar;}
  }
}

If you want to try this for yourself, you’ll need to download the EnableInterrupt library, but once you are able to compile this code, do the following:

With the code as it is, if you type in something, for example, type in: ApplesAreRed
You will get back: Ap

If you then take the variable testVariable in the button2Handler() method and delete a single 1 from the string so that it looks like this:

String testVariable="111";

Then re-upload the code and type in: ApplesAreRed
You will get back: Apples

NOW, put the string back to where it was with five 1’s, but then go and comment out the line: #include <IRremote.h> so that is looks like this:

//#include <IRremote.h>

Then re-run the test, you will get the entire string back in the results…

I’ve done this on a Nano and a standard Uno with the same results.

Does anyone know why this is happening or how to fix it?

const String commands[]  PROGMEM = {"Menu","OK","Up","Down","Left","Right","Exit","Mute","Vol+","Vol-","Ch+","Ch-","Last","Info","Menu","1","2","3","4","5","6","7","8","9","0","Stop","Pause","Play","FFWD","REW","MYDVR","PageUp","PageDown","A","B","C","D"};
const String cmdHex[]    PROGMEM = {"36C127","366133","36812F","37A10B","37810F","364137","366932","E0E0F00F","E0E0E01F","E0E0D02F","377111","36F121","36E123","36213B","373918","36113D","37111D","36912D","37910D","365135","375115","36D125","37D105","363139","373119","365934","374117","37990C","36293A","37291A","36C926","36D924","37D904","37E902","36193C","37191C","F7E10"};

PROGMEM is for basic types, NOT String instances.

  enableInterrupt(button1, button1Handler, FALLING);
  enableInterrupt(button2, button2Handler, FALLING);

Why do you think you need interrupts for switches?

Why is the number of the interrupt stored in a variable called button1 or button2?

Variables used in interrupt handlers and other functions need to be declared volatile.

Your real problem appears to be in the code you redacted. Post your REAL code.

PaulS:
Your real problem appears to be in the code you redacted. Post your REAL code.

If you compile and upload the code I posted, you will see the problem for yourself.

PaulS:
Why do you think you need interrupts for switches?

I use buttons that connect a digital pin to ground (when pressed). I then do a pinMode on that buttons pin number (which I declare in a variable) and set it to INPUT_PULLUP. Then I use the EnableInterrupt library so that when a button is pressed, it doesn't matter what else is happening in the code, a boolean gets set to true so that I can do something with that button casually as opposed to methodically checking for the buttons pin being brought to 0. I want to be able to have a button press affect a variable no matter what else is happening in the program.

PaulS:
Why is the number of the interrupt stored in a variable called button1 or button2?

It's not. button1 and button2 are PIN numbers.

PaulS:
Variables used in interrupt handlers and other functions need to be declared volatile.

I'm not declaring an interrupt variable, I'm declaring a pin number in a variable.

I’m not declaring an interrupt variable,

  last_interrupt1_time = interrupt1_time;

last_interrupt1_time is an interrupt variable, and MUST be declared volatile.

It looks like you have made a good attempt to strip this down to the smallest program which reproduces the problem.

So simply commenting out the #include will fix the problem? Then you have to work out what code that introduced and what runs during the runtime that might affect your code.

I had a quick look at the library. There is a note that a recent change to the library fixed a serious bug. Do you have the new version?

I'm guessing something goes wrong with the two libraries using interrupts but I haven't looked closely yet.

Don't use the class String - it will cause you problems sooner or later in the restricted RAM of your Arduino.
Use plain old C-style strings instead.

const int DEBOUNCE_DELAY  PROGMEM = 150;

It doesn't make sense to try to put a single int into program space.

const String commands[]  PROGMEM = {"Menu", "OK", "Up", "Down", "Left", "Right", "Exit", "Mute", "Vol+", "Vol-", "Ch+", "Ch-", "Last", "Info", "Menu", "1", "2", "3", "4", "5", "6", "7", "8", "9", "0", "Stop", "Pause", "Play", "FFWD", "REW", "MYDVR", "PageUp", "PageDown", "A", "B", "C", "D"};
const String cmdHex[]    PROGMEM = {"36C127", "366133", "36812F", "37A10B", "37810F", "364137", "366932", "E0E0F00F", "E0E0E01F", "E0E0D02F", "377111", "36F121", "36E123", "36213B", "373918", "36113D", "37111D", "36912D", "37910D", "365135", "375115", "36D125", "37D105", "363139", "373119", "365934", "374117", "37990C", "36293A", "37291A", "36C926", "36D924", "37D904", "37E902", "36193C", "37191C", "F7E10"};

As stated by PaulS, PROGMEM is for base types and not class objects.
A String object consists of a set of object internal variables, one of which is a pointer. That pointer is assigned a value when memory is allocated for the character data on the heap. In other words, there are two parts to a String: the object part stored in one area of RAM, and the character data part stored somewhere else in RAM.
Your PROGMEM attempts to put the object variables in (const!) program memory, where the pointer cannot be assigned to.

As it is, when I compile those declarations I get the following:

/home/guest/Arduino/test/test.ino:6:14: warning: uninitialized variable 'commands' put into program memory area [-Wuninitialized]
 const String commands[]  PROGMEM = {"Menu", "OK", "Up", "Down", "Left", "Right", "Exit", "Mute", "Vol+", "Vol-", "Ch+", "Ch-", "Last", "Info", "Menu", "1", "2", "3", "4", "5", "6", "7", "8", "9", "0", "Stop", "Pause", "Play", "FFWD", "REW", "MYDVR", "PageUp", "PageDown", "A", "B", "C", "D"};
              ^
/home/guest/Arduino/test/test.ino:7:14: warning: uninitialized variable 'cmdHex' put into program memory area [-Wuninitialized]
 const String cmdHex[]    PROGMEM = {"36C127", "366133", "36812F", "37A10B", "37810F", "364137", "366932", "E0E0F00F", "E0E0E01F", "E0E0D02F", "377111", "36F121", "36E123", "36213B", "373918", "36113D", "37111D", "36912D", "37910D", "365135", "375115", "36D125", "37D105", "363139", "373119", "365934", "374117", "37990C", "36293A", "37291A", "36C926", "36D924", "37D904", "37E902", "36193C", "37191C", "F7E10"};

I have no idea what the result of that could be, but it is best avoided.

void button2Handler() {
  String testVariable = "1111";
}

The String class internally uses realloc() and free(); at least in AVR libc those functions are not reentrant. That means you cannot safely use Strings inside and outside of an ISR without the risk of something exploding.

It is not a good idea to use the String (capital S) class on an Arduino as it can cause memory corruption in the small memory on an Arduino. This can happen after the program has been running perfectly for some time. Just use cstrings - char arrays terminated with '\0' (NULL).

Have a look at the examples in Serial Input Basics - simple reliable ways to receive data.

...R

jremington:

  last_interrupt1_time = interrupt1_time;

last_interrupt1_time is an interrupt variable, and MUST be declared volatile.

That is a variable used to track time for debouncing the button as can be seen in this method:

void button1Handler() {
  uint32_t interrupt1_time = millis();
  if (interrupt1_time - last_interrupt1_time > DEBOUNCE_DELAY) {
    one = true;
  }
  last_interrupt1_time = interrupt1_time;
}

MorganS:
So simply commenting out the #include will fix the problem? Then you have to work out what code that introduced and what runs during the runtime that might affect your code.

I thought the same thing Morgan, However, if you comment out, for example, the two arrays that are declared and leave in the included library, it will also fix the problem ... so it seems to be something like the serial buffer being overrun with the instantiation of other variables somehow...

arduarn:

void button2Handler() {

String testVariable = "1111";
}



The *String* class internally uses *realloc()* and *free()*; at least in AVR libc those functions are not reentrant. That means you cannot safely use *String*s inside and outside of an ISR without the risk of something exploding.

Arduarn,

My knowledge of C and C++ is about 99% ignorance and 1% "feel it out" ... I know Java fairly well, but even with Java, my understanding of datatypes on a technical level is fairly basic ...

Would you please provide me with an example of the PROPER way to declare and use strings in an Arduino sketch?

Thank you,

Mike

Robin2:
Have a look at the examples in Serial Input Basics - simple reliable ways to receive data.

...R

Robin,

Very nice post you made there ... It makes sense to me and it is both simple and concise. I do, however, have a question:

In example 2, you call the reading of the Serial buffer from the loop using a function. If that function were renamed with the built in serialEvent() function, then the newData variable monitored in the loop, would the end result be the same in terms of properly reading input from the Serial monitor? ALSO, if I were going to scrutinize the input for something specific, will I need to concern myself with the \0 that is appended at the end of the line? For example, lets say I typed in Apple ... would I need to test for it in this way:

if (receivedChars.equals("Apple\0")) {//do something}

Or would I leave the \0 out of my test?

Thank you,

Mike

if (receivedChars.equals("Apple\0")) {//do something}

That assumes that receivedChars is a big-S string.

Use strcmp() or strncmp() to compare small-s strings. The string functions with the “n” in the middle require a length to be given so they are safe from buffer-overrun errors. Or at least safer. Use the “n” version everywhere you can.

Bare string literals such as “Apple” have the ‘\0’ null character added automatically by the compiler. But you do have to provide space for that if you’re storing that in a string…

char test[6] = "Apple";

MorganS:
Use strcmp() or strncmp() to compare small-s strings.

I did find strcmp and it didn’t work … haven’t tried strnbmp … this is what I ended up doing:

void serialEvent() {
  while (Serial.available() > 0 && newData == false) {
    rc = Serial.read();
    if (rc != endMarker) {
            inputChar[ndx] = rc;
            ndx++;
            if (ndx >= numChars) {
                ndx = numChars - 1;
            }
        }
        else {
            inputChar[ndx] = '\0'; // terminate the string
            newData = true;
            doString(ndx);
            ndx = 0;
        }
  }
}


void doString(int ndx) {
  String input = String(inputChar); //Cuts off the unused indexes in the char array
  int len = sizeof(input); 
  input = input.substring(0,ndx-1);
  sndACommand(input);
  for (int f=0; f<32; f++) {inputChar[f] = "";}
  newData = false;
}

Doing it that way, the string can be compared in the sendACommand function. Works like a charm.

Mike

You’ve really got to get away from using the big-S string class. It will cause out-of-memory problems.

This is useless and a little dangerous…

for (int f=0; f<32; f++) {inputChar[f] = "";}

What if the length of the buffer changes in the future? If it gets longer then this has to be changed manually. If it gets shorter and you forget to change this, then this overwrites something else.

  String input = String(inputChar); //Cuts off the unused indexes in the char array

int len = sizeof(input);
  input = input.substring(0,ndx-1);

You create the variable len but then never use it for anything.

Since ndx is the length of the command received, then substringing it to that length makes it the length that it is. I bet it works exactly the same without this.

EasyGoing1:
In example 2, you call the reading of the Serial buffer from the loop using a function. If that function were renamed with the built in serialEvent() function, then the newData variable monitored in the loop, would the end result be the same

It might, but why on earth would you bother? Isn't it much simpler just to use my functions as I posted them?

...R

It's not. button1 and button2 are PIN numbers.

The first argument to attachInterrupt() is NOT a pin number. RTFM!

But he's using a library. I checked the library. That part is OK.

MorganS:
But he’s using a library. I checked the library. That part is OK.

I guess you see what you want to. I looked at " enableInterrupt(button1, button1Handler, FALLING);" and saw attachInterrupt…

I don’t see the purpose of adding a library to wrap the attachInterrupt function, when there is a macro to convert a pin number to an interrupt number. I don’t remember its name offhand, but if I needed to use it, I could find it faster than finding a library.

That library also works on pin-change interrupts. It makes both types of interrupts work the same for the programmer.