Serial Issues after adding lines of code

Hi all,

I’m having issues with my project where in my main loop, I parse 2 character commands which in turn will run a function that outputs IR to a specific device.

So far most of my devices function correctly, but when I add additional functions to the parsing statement, everything stops working. I am not able to print to serial correctly, and when I do, the received strings are null.

I changed the order of the functions being called to make sure those functions were running correctly, and they were, so I am stumped on this issue.

Below is my code:

#include <JenkinsRemote.h>
#include <IRremote.h>

boolean commandComplete = false;
String stringRead = "";

JenkinsRemote jenkinsRemote;

void setup()
{
  Serial.begin(115200);
  stringRead.reserve(1);
}

void loop()
{
  if (commandComplete)
  {
    CMD_IR command = CMD_INVALID; 
    DEVICE_ID device;
    char temp = stringRead[0];

    device = (DEVICE_ID)atoi(&temp);
    command = (CMD_IR)(atoi(&stringRead[1]));
  
    if (device == DEV_TV)
      jenkinsRemote.ControlSamsungTV(command);
    else if (device == DEV_SOUND_SYSTEM)
      jenkinsRemote.ControlLogitechSound(command);
    else if (device == DEV_FAN)
      jenkinsRemote.ControlFans(command);
    else if (device == DEV_LIGHT)
      jenkinsRemote.ControlLights(command);
    /*else if (device == DEV_COMPUTER)
     jenkinsRemote.ControlComputer(command);
    else if (device == DEV_VERIZON_FIOS)
     jenkinsRemote.ControlFios(command);*/
    else
      Serial.println("Invalid Command");

    stringRead = "";
    commandComplete = false;
  }
}

void serialEvent()
{
  while (Serial.available()) {
    // get the new byte:
    char inChar = (char)Serial.read();  

    if (inChar == '!')
    {
      commandComplete = true;
    }
    else
    {
      stringRead += inChar;
    }
  }
}

The commented out parts in the main loop break my code when I put them back in.

Edit: I have an Uno and I am using the hardware serial.

Any ideas?

Thanks.

    char temp = stringRead[0];
      : 
   device = (DEVICE_ID)atoi(&temp);
    command = (CMD_IR)(atoi(&stringRead[1]));

atoi() wants a pointer to a null-terminated string, and won't behave well if you give it pointers to single characters. (depending on stack contents, it may keep reading memory and causing all sorts of unpredictable results.)
Since you only have a single character, you shouldn't need atoi() at all, just do:

device = (DEVICE_ID)(stringRead[0] - '0');
command = (CMD_IR)(stringRead[1] - '0');

My suspicion is the use of serialEvent(). That is an interrupt service routine (IRS) and there is little or no documentation for it. I would use a good old fashioned function with if (Serial.available > 0) and scrap serialEvent.

...R

My suspicion is the use of serialEvent(). That is an interrupt service routine (IRS)

No, it isn’t.

main.cpp

#include <Arduino.h>

int main(void)
{
	init();

#if defined(USBCON)
	USBDevice.attach();
#endif
	
	setup();
    
	for (;;) {
		loop();
		if (serialEventRun) serialEventRun();
	}
        
	return 0;
}

I was looking at HardwareSerial.cpp earlier today (in another context) and it looks very like an ISR.

In any case, my problem with it is that it is called unpredictably by circumstances outside my control and it could be called 2 or 3 times before my first walk through my serialEvent() function completes.

With Serial.available() I get similar functionality that is entirely under my control.

...R

I was looking at HardwareSerial.cpp earlier today (in another context) and it looks very like an ISR.

It IS.

In any case, my problem with it is that it is called unpredictably by circumstances outside my control and it could be called 2 or 3 times before my first walk through my serialEvent() function completes.

Or more, if you write blocking code in the serialEvent() function. But, so what? That is no different from what happens if you have all the serial processing code in loop() instead.

With Serial.available() I get similar functionality that is entirely under my control.

The serialEvent() function gets called, at the end of each pass through loop(), if there is serial data to be handled. You can elect to read one byte, or you can call Serial.available() and read multiple bytes, if there are more than one, or you can call Serial.available() multiple times to get changing values - all exactly like you can do in loop().

Robin2:
I was looking at HardwareSerial.cpp earlier today (in another context) and it looks very like an ISR.

Should have gone to Spec Savers :wink:

AWOL:

Robin2:
I was looking at HardwareSerial.cpp earlier today (in another context) and it looks very like an ISR.

Should have gone to Spec Savers :wink:

Together with @PaulS ???

I was looking at HardwareSerial.cpp earlier today (in another context) and it looks very like an ISR.

It IS.

...R

What I meant was that serial data is received and sent using interrupts. The HardwareSerial class defines the interrupts to be used, and implements the interrupt handlers for those interrupts. Not sure what AWOL meant. You've got to remember that he reads the forum on a tablet, so some things look a little strange that way.

serialEvent is not an ISR.
It is called from serialEventRun.
It does not run in interrupt context.
What on Earth makes it an ISR (or an "IRS")?
(It doesn't look any different on a phone, tablet or 24 inch monitor)

Robin2:

AWOL:

Robin2:
I was looking at HardwareSerial.cpp earlier today (in another context) and it looks very like an ISR.

Should have gone to Spec Savers :wink:

Together with @PaulS ???

Why not?
You may get a group discount.

serialEvent is not an ISR.
It is called from serialEventRun.
It does not run in interrupt context.
What on Earth makes it an ISR (or an "IRS")?

I think that Robin2 was referring to HardwareSerial in general, not serialEvent specifically. But, I could be wrong. I was once. 8)

I was referring to serialEvent() as I understood it from a brief read of HardwareSerial.cpp together with the “extensive” documentation if the Reference section

Called when data is available. Use Serial.read() to capture this data.

I’m not trying to argue the fine points about what exactly constitutes an ISR (that is probably above my pay grade). But as far as I can see something that I don’t have direct control over calls serialEvent() whenever it feels like and (presumably) (as with other function calls) serialEvent() returns to the point immediately after it was called. That has many of the characteristics of an ISR. Perhaps (and this is just a guess in the absence of documentation) the principal disinction between it and a real ISR is that interrupts are enabled while serialEvent() is working. If so that is probably an additional disadvantage.

…R

But as far as I can see something that I don't have direct control over calls serialEvent() whenever it feels like and (presumably) (as with other function calls) serialEvent() returns to the point immediately after it was called.

No, if "serialEvent" exists, it is called by "serialEventRun" immediately after "loop()" is called by "main()".
There is nothing arbitrary or unpredictable about it.

This is nothing at all like an ISR which can happen (on an AVR) at any time interrupts are enabled, even between storing the two halves of a sixteen bit variable.

Edit: minor spelling

Update: I fixed the issue (I still don't actually know what the issue was exactly).

I changed from using a string type to a char array and that seemed to work.

In the serialevent loop, I printed out each char that serial received when the code didn't work, and they came out perfect. When I printed out the string concatenation, the entire string was NULL. Not sure why that broke by adding more code, but having a char array fixed it...

Update: I fixed the issue

Please, don't stand on ceremony - just post the code.

#include <JenkinsRemote.h>
#include <IRremote.h>

boolean commandComplete = false;
char stringRead[4];
int readCount = 0;

JenkinsRemote jenkinsRemote;

void setup()
{
  Serial.begin(115200);
  digitalWrite(13,LOW);
}

void loop()
{
  if (commandComplete)
  {
    CMD_IR command = CMD_INVALID; 
    DEVICE_ID device;
    
    device = (DEVICE_ID)(stringRead[0] - '0');
    command = (CMD_IR)(atoi(&stringRead[1]));
  
    Serial.print(device);
    Serial.println(command);
    
    if (device == DEV_TV)
      jenkinsRemote.ControlSamsungTV(command);
    else if (device == DEV_SOUND_SYSTEM)
      jenkinsRemote.ControlLogitechSound(command);
    else if (device == DEV_FAN)
      jenkinsRemote.ControlFans(command);
    else if (device == DEV_LIGHT)
      jenkinsRemote.ControlLights(command);
    else if (device == DEV_COMPUTER)
      jenkinsRemote.ControlComputer(command);
    else if (device == DEV_VERIZON_FIOS)
      jenkinsRemote.ControlFios(command);
    else
      Serial.println("Invalid Command");

    commandComplete = false;
  }
}

void serialEvent()
{
  while (Serial.available()) {
    // get the new byte:
    char inChar = (char)Serial.read();  
    
    if (inChar == '!')
    {
      commandComplete = true;
      stringRead[readCount] = 0;
      readCount = 0;
    }
    else
    {
      stringRead[readCount] = inChar;
      readCount++;
    }
  }
}

OK I lied, it looks adding code still breaks everything...it's not as broken as before. I can still control some devices. but now when I add more function calls, other functions just don't get called at all...it's very random.

For ex.
The power/direct'/optical function don't get called at all...but the vol up an down do...

bool JenkinsRemote::ControlLogitechSound (CMD_IR command)
{
	switch (command)
	{
		case CMD_POWER_ON_OFF:
			irsend.sendRaw(LogitechSoundPower,LOGITECH_CODE_SIZE,IR_FREQ);
			return true;
		break;
		case CMD_VOL_UP:
			irsend.sendRaw(LogitechSoundVolumeUp,LOGITECH_CODE_SIZE,IR_FREQ);
			return true;
		break;
		case CMD_VOL_DN:
			irsend.sendRaw(LogitechSoundVolumeDown,LOGITECH_CODE_SIZE,IR_FREQ);
			return true;
		break;
		case CMD_INPUT_DIRECT:
			irsend.sendRaw(LogitechSoundInputDirect,LOGITECH_CODE_SIZE,IR_FREQ);
			return true;
		break;
		case CMD_INPUT_OPTICAL:
			irsend.sendRaw(LogitechSoundInputOptical,LOGITECH_CODE_SIZE,IR_FREQ);
			return true;
		break;
		default:
			Serial.println("Invalid command");
		break;
	}
	
	return false;
}

In my sketch, when I comment out a function call (doesn't matter which), everything works perfectly...

void loop()
{
  if (commandComplete)
  {
    CMD_IR command = CMD_INVALID; 
    DEVICE_ID device;
    
    device = (DEVICE_ID)(stringRead[0] - '0');
    command = (CMD_IR)(atoi(&stringRead[1]));
    
    if (device == DEV_TV)
      jenkinsRemote.ControlSamsungTV(command);
    else if (device == DEV_SOUND_SYSTEM)
      jenkinsRemote.ControlLogitechSound(command);
    else if (device == DEV_FAN)
      jenkinsRemote.ControlFans(command);
    else if (device == DEV_LIGHT)
      jenkinsRemote.ControlLights(command);
    else if (device == DEV_COMPUTER)
      //jenkinsRemote.ControlComputer(command);
    else if (device == DEV_VERIZON_FIOS)
      jenkinsRemote.ControlFios(command);
    else
      Serial.println("Invalid Command");

    commandComplete = false;
  }
}

This makes no sense to me, hopefully someone out there knows wtf is going on.

Thanks.

This makes no sense to me, hopefully someone out there knows wtf is going on.

You have a limited amount of memory. You use more than you have. You get unexpected results.

There is nothing unexpected about this sequence of events. I can't imagine why it makes no sense to you.

But my sketch size says it's only 8006bytes/32526bytes

That's how much program memory you're using, not how much RAM