After using the String class for a few iteration, it starts malfunctioning

[joviol]

I have entered the following program to get commands through an ethernet shield. The commands seem to get to the arduino correctly as it prints it correctly in the println call placed in the loop() function. But once I call the processCommand function, it only works the first few times, after which it starts printing random strings (mostly "?"). If I try to use a reference to String instead as a parameter to processCommand, the indexOf function works only the first few times after which it will not recognise the string event if it prints the one I entered, which is one of those I expect as a command.

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

byte mac[] = {
  0x00, 0xAA, 0xBB, 0xCC, 0xDE, 0x02
};
IPAddress ip(192, 168, 1, 177);
IPAddress gateway(192, 168, 1, 1);
IPAddress subnet(255, 255, 255, 0);

// telnet defaults to port 23
EthernetServer server(23);
boolean alreadyConnected = false;

const int ledPin = 2;
String commandString = "";

void setup() {
  // put your setup code here, to run once:
  pinMode(ledPin, OUTPUT);

   // Open serial communications and wait for port to open:
  Serial.begin(9600);
  // this check is only needed on the Leonardo:
  while (!Serial) {
    ; // wait for serial port to connect. Needed for Leonardo only
  }

  // start the Ethernet connection:
  Serial.println("Trying to get an IP address using DHCP");
  if (Ethernet.begin(mac) == 0) {
    Serial.println("Failed to configure Ethernet using DHCP");
    // initialize the ethernet device not using DHCP:
    Ethernet.begin(mac, ip, gateway, subnet);
  }
  // print your local IP address:
  Serial.print("My IP address: ");
  ip = Ethernet.localIP();
  for (byte thisByte = 0; thisByte < 4; thisByte++) {
    // print the value of each byte of the IP address:
    Serial.print(ip[thisByte], DEC);
    Serial.print(".");
  }
  Serial.println();
  // start listening for clients
  server.begin();

}

void loop() {
  // wait for a new client:
  EthernetClient client = server.available();

  if(client) {
    if(!alreadyConnected) {
      // Clear out the input buffer
      client.flush();
      commandString = "";

      server.println("--> Please type your command and hit Return...");
      alreadyConnected = true;
    }

    while(client.available()) {
      // Read bytes incoming from the client:
      char newChar = client.read();
      if(newChar == 0x0D) { // 0x0D = Carriage Return ASCII code
        server.print("Received the following command: ");
        server.println(commandString);
        processCommand(commandString);
      }
      else {
        Serial.println(newChar);
        commandString += newChar;
      }
    }
  }

}

void processCommand(String cmd) {
  server.print("Processing Command: ");
  server.println(cmd);

  if(cmd.indexOf("photo") > -1) {
    server.println("Photo command Received");
    server.print("Reading from the photoresistor: ... ");
    server.println(analogRead(A0));
    commandString = "";
    return;
  }

  if(cmd.indexOf("ledon") > -1){
    server.println("LED On command received");
    digitalWrite(ledPin, HIGH);
    server.println("LED was turned on");
    commandString = "";
    return;
  }

  if(cmd.indexOf("ledoff") > -1){
    server.println("LED Off command received");
    digitalWrite(ledPin, LOW);
    server.println("LED was turned off");
    commandString = "";
    return;
  }
  commandString = "";
  instructions();
}

void instructions() {
  server.println("I don't understand");
  server.println("Please use one of the following commands:");
  server.println("> photo, to get a reading from the photoresistor");
  server.println("> ledon, to turn on the LED");
  server.println("> ledoff, to turn off the LED");
}

I am using and Arduino Uno Rev3 and an ethernet shield. The version of the IDE is 1.6.3

[matthijskooijman]

At first glance, your code looks ok to me. Having said that, it seems more likely that you're doing something wrong in your code than that the Arduino core is broken. This bug tracker is really for Arduino bugs, not for getting help with your own code (the forums are better for that).

I'll leave this report open, perhaps others have ideas. However, if you believe there is indeed a bug in the Arduino core, it would be helpful if you could narrow it down a bit further: See how much code you can remove, while still observing the breakage. Also, try using debug prints to see if you can figure out what is happening exactly.

[mschlenker]

Hi @matthijskooijman , does this http://download.arduino-hausautomation.de/Blog/ZwitschernerWeihnachtsbaum/WasserstandsTweet/WasserstandsTweet.ino look too wrong for you? The code worked on Arduino 1.5.6 and stopped working on 1.6.x. When replacing

const String TEMBOO_APP_KEY_NAME = "ZwitschernderWeihnachtsbaum";

by

define TEMBOO_APP_KEY_NAME "ZwitschernderWeihnachtsbaum"

(using macros is definitively better than using const at least on MCUs) everything works fine again. Coming from higher level languages than C I should assume that something defined as const might be optimized by the compiler in a way that it is treated like not changable program memory on a microcontroller. But there seem to be some side effects introduced between 1.5.x and 1.6.x, so further investigation might make sense.

[matthijskooijman]

I haven't looked closely at the code, no time for that. I'm also not sure how this relates to the original bug?

As for const vs define, those should in a lot of cases result in the same code being generated. However, your const-version uses the String class, which is not a native C++ type. This probably means it will have overhead, since String stores its data on the heap and I suspect it cannot be optimized away. Using const char arrays is probably better in this case static const char FOO[] = "foo";.

[mschlenker]

@matthijskooijman I do know about the overhead now. I am coming from languages like Ruby and Python where everything is an object and just didn't care about the overhead 18 months ago when the sample just worked. So we probably should compare the behaviour of the avr-gcc from 1.5.x with that from 1.6.x. Having a corrupted heap after using String seven times is not great.

[mschlenker]

@joviol Please read https://learn.adafruit.com/memories-of-an-arduino/optimizing-sram In your case the combined usage of Ethernet and String might just be too much - global variables already take up 1053 bytes. Using char command[ ] = "ledoff"; (the longest command you use) might be a starting point.

You should still compare the behaviour of your original example with different versions of the Arduino IDE (1.0.6 and 1.5.x). Maybe toolchains and libraries optimize a little bit better there?

[joviol]

After removing all the ethernet code from this sample and replacing it with Serial, the problem went away. However, if it were a problem with the memory, should I see some kind of out of memory error somewhere. It is odd that in the loop function the incoming string is printed correctly but when I try to print it from the processCommand function, it starts printing garbage.

Anyway, it could also be a problem with possible non printable characters coming from the telnet connection. This is something I still need to analyse.

I apologise for hastily opening this issue, I should have tried removing the networking code first. It is just, that after a day of troubleshooting my code and based on the behaviour I saw from the logging, it very much looked like there was a problem with the String class.

Thank You very much for your responses and understanding.

[mschlenker]

However, if it were a problem with the memory, should I see some kind of out of memory error somewhere.

No, when heap and stack collide it just behaves weird (see the link I provided). Look at the compiler output: You just have 1k left! The String class is not very memory efficient and Ethernet neither... Maybe @PaulStoffregen s F() macro already might save enough memory to avoid the collision?

[mschlenker]

@ffissore Please close or better move to Doc. The String class is an ugly memory hog, the doc should warn. In this case neither String nor Ethernet were the cause for the problem, but together they were the occasion.