Serial communication stops working with too much interrupts

Hi!

I am having an issue with my arduino Uno R3 : I have a Rotary encoder plugged on the external interrupts (pins 2 and 3). This sensor just sends alternatively HIGH and LOW value as it rotates. As a result, if it passes from LOW to HIGH or from HIGH to LOW, the interrupt routine is called. Everything works fine if I rotate it slowly but when I increase the speed, the serial communication is stopped after a certain speed. I tend to believe that it is due to too many interrupts, and maybe because an interrupt is called during another interrupt, but I don't really know if that's right.

Here is my code :

int encoder0PinA = 2;
int encoder0PinB = 3;
volatile int encoder0pos = 0;
volatile int to_send;

void setup() { 
  pinMode (encoder0PinA,INPUT);
  pinMode (encoder0PinB,INPUT);
  Serial.begin (115200);
  attachInterrupt(1, doEncoderB, CHANGE);
  attachInterrupt(0, doEncoderA, CHANGE);
} 

void loop() {
}

void doEncoderA(){
  if (digitalRead(encoder0PinA) == HIGH) {
    if (digitalRead(encoder0PinB) == LOW) {  
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  else          
  { 
    if (digitalRead(encoder0PinB) == HIGH) {   
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  to_send = encoder0pos
  Serial.write(to_send);
}

void doEncoderB(){
  if (digitalRead(encoder0PinB) == HIGH) {   
    if (digitalRead(encoder0PinA) == HIGH) {  
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  else { 
    if (digitalRead(encoder0PinA) == LOW) {   
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  to_send = encoder0pos;
  Serial.write(to_send);
}

Does anyone have an idea on that behaviour?

EDIT : my rotary encoder is of course an incremental one

There is a conflict with interrupts, but not the way you assumed.

The functions doEncoderA and doEncoderB are interrupt handlers. You may not use the Serial function in an interrupt handlers.

The result can be:

  • It is working.
  • It is not working at all.
  • It is working just once.
  • Or what you have.

In the interrupt handler, you can set or increment a global variable. That variable can be used in the loop() function.
A global variable that is used in an interrupt should be 'volatile'. And if it is more than 8-bits, the interrupts should be turned off in the loop() function if that variable is used.

Upload the new sketch, so we can check it.

Once you have that, you can set a motor to the encoder for very fast rotations, but it should not fail.

Thank you for your answer! :slight_smile:

Ok, I am trying that right now. But I have just one question : if my serial.write is in the loop, can't I loose some values if for instance 2 interrupts are called before the values are written to serial?

Ok, so here is my new code :

int encoder0PinA = 2;
int encoder0PinB = 3;
volatile int encoder0pos = 0;
volatile int to_send;
volatile bool change=false;

void setup() { 
  pinMode (encoder0PinA,INPUT);
  pinMode (encoder0PinB,INPUT);
  Serial.begin (115200);
  attachInterrupt(1, doEncoderB, CHANGE);
  attachInterrupt(0, doEncoderA, CHANGE);
} 

void loop() {
  if(change){
    Serial.write(to_send);
    change=false;
  }
}

void doEncoderA(){
  change=true;
  if (digitalRead(encoder0PinA) == HIGH) {
    if (digitalRead(encoder0PinB) == LOW) {  
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  else          
  { 
    if (digitalRead(encoder0PinB) == HIGH) {   
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  to_send = encoder0pos
  //Serial.write(to_send);
}

void doEncoderB(){
  change=true;
  if (digitalRead(encoder0PinB) == HIGH) {   
    if (digitalRead(encoder0PinA) == HIGH) {  
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  else { 
    if (digitalRead(encoder0PinA) == LOW) {   
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  to_send = encoder0pos;
  //Serial.write(to_send);
}

It works for any speed so that's cool :slight_smile:
But I think I am loosing values when I rotate too fast because there are too many interrupts during one loop. Is there any way to avoid loosing values? Because the program I am making cannot accept any loss because I need to have a precise positionning system. I thought that interrupts seemed to be the best idea for my application.

It is possible with a buffer.
But a buffer in between an interrupt and the loop() should be carefully coded.

The code as it is now, could cause a problem.
Reading the 'change' and 'to_send' is slowed down by the Serial.write() or any other code you might add. It is possible to use the buffer in the Serial library. But you have to estimate how fast it will be filled.
And the 'to_send' could be half overwritten (a byte) this way.

I have not used encoders myself, but there might be library for it. Or else a library with a circular buffer capable to be filled in an interrupt.

Your idea to use interrupts is very good ofcourse.

This is one little step better:

if( change) {
  noInterrupts();
  int to_send_copy = to_send;
  change = false;
  interrupts();

  Serial.write( to_send_copy);
}

Ok, thank you so much for helping me :open_mouth: I owe you one!
I think I finally came up with a good code even if it is not perfect... (it supports fast - but not too fast - rotations with no loss at all)

int encoder0PinA = 2;
int encoder0PinB = 3;
volatile int encoder0pos = 0;
const int MAX_LIST_SIZE=700;
volatile int to_send_list[MAX_LIST_SIZE];
volatile int to_send;
volatile int curseur=-1;

void setup() { 
  pinMode (encoder0PinA,INPUT);
  pinMode (encoder0PinB,INPUT);
  Serial.begin (115200);
  attachInterrupt(1, doEncoderB, CHANGE);
  attachInterrupt(0, doEncoderA, CHANGE);
  for(int i=0; i<MAX_LIST_SIZE; i++){
    to_send_list[i]=-1;
  }
} 

void loop() {
while(curseur>=0){
    noInterrupts();// the nointerrupt thing you gave me
    to_send=to_send_list[curseur];
    to_send_list[curseur]=-1;
    curseur--;
    interrupts();

    Serial.write(to_send);
  }
}

void doEncoderA(){
  if (digitalRead(encoder0PinA) == HIGH) {
    if (digitalRead(encoder0PinB) == LOW) {  
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  else          
  { 
    if (digitalRead(encoder0PinB) == HIGH) {   
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  curseur++;
  to_send_list[curseur] = encoder0pos;
}

void doEncoderB(){
  if (digitalRead(encoder0PinB) == HIGH) {   
    if (digitalRead(encoder0PinA) == HIGH) {  
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  else { 
    if (digitalRead(encoder0PinA) == LOW) {   
      encoder0pos = 0;
    } 
    else {
      encoder0pos = 1;
    }
  }
  curseur++;
  to_send_list[curseur] = encoder0pos;
}

For now it seems good but I will make more tests to see if I can find a problem :smiley:
However,the size of the buffer "to_send_list" seems to have great limitations (the arduino doesn't start when I try to choose 1000 for MAX_LIST_SIZE)... Maybe because of the very smal amount of memory of the arduino...

I looked at the arduino uno memory and it only has 2k for variables so it is normal that I can't store an array of 1000 int (one int is 2 bytes)... That's a shame because if I could, then I would be able to rotate my encoder faster but well... If I don't have any other choice :frowning:
Anyway, after many tests, my last code doesn't seem to raise any error so far, so that's already a big step!

You have to check if the curseur gets too high (in the interrupt handlers) or too low (in loop).
The memory size is small, and the Serial library uses some.
About 100 for the size would be more appropriate.

You use 'curseur' to point to the last stored.
However, it makes more sense to let it point to the the first free location.
That way, curseur is 0 when starting, which indicates that the first location is ready to be used, and the buffer is still empty.
You have to change the interrupts handlers and the "if" in the loop.

Since you are storing 0 or 1 in your array, a byte would do rather than an int. If you're comfortable with bit twiddling operations and a little more complexity in the code, you could use a bit instead for greater space savings.

It looks as though you're treating the array as a stack and sending the latest readings to serial rather than the oldest. That doesn't tie to what your original code was doing - looks like a bug - is it?

Sorry I should have said but actually, the code I post here is a simple version of the real buisness , that's why I send only 1 or 0. In the real code, it is the same thing except other informations (10 bits) are sent, so an int is the minimum size I can obtain when I "stick" all the informations together. Concerning the way I sent the numbers, it doesn't matter for me if the oldest arrive first or not, it will be the same thing for me afterwards.

Actually, I added a check if the curseur gets too high in the interrupt handlers since my last post so that I get to know that I am getting too fast but thank you anyway. :slight_smile: About checking if the "curseur" gets too low, it shouldn't make a problem since the "curseur" can't be less than -1 in my code (0 with your modification) because it only decreases in the while(...) wich is in the loop.

I don't think I will be adding anything else on my code so do you really think I should set the size at 100? 700 seems to work fine... (actually I would like to make it bigger). Is there any reason for me to decrease the size of my array, I mean is it dangerous to fill the SRAM to much?

No, there is no danger. But most libraries use buffers and classes. I think with 700 integers (1.4kbyte) you don't have a lot of memory for all the rest.
I hope you don't use the String class, that is one of the most memory eating classes.
You could check the memory, Arduino Playground - AvailableMemory

Oh this is so cool, I now exactly the amount of RAM I can use :smiley:
I used the function with the heap and stack pointers and with an array of 700 ints, I have still 392 bytes to use. When I increase it to 800, there is just 192 which is normal since an int is 2 bytes. So the function seem to work fine :open_mouth:
That explains why I couldn't go to 1000 ints because I can only go to an array of 896 ints with the memory available (yes I have unbelievable math skills :stuck_out_tongue: ). Thank you for pointing out this link, I only used the technical specifications of the arduino board so this is much easier now ^^
Well I think one would have to be very carefull and keep memory space in order to avoid problems if variables are created during the loop. Since I don't have that kind of stuff I should be able to almost fill the memory without any problem but I will keep a security just in case. Thank you for your help :slight_smile:

EDIT : no I don't use the String class, I am not that mad to look for more problems :stuck_out_tongue:

Hello guys,

I've faced the same problem. I want to read some messages from a CAN bus, but there are so many messages, so my Arduinos (i have Leonardo and Uno) cannot handle them all. Maximum - 3-4 messages.
Could you please help me with code optimization:

// demo: CAN-BUS Shield, receive data
#include <mcp_can.h>
#include <SPI.h>

unsigned char Flag_Recv = 0;
unsigned char len = 0;
unsigned char buf[8];
char str[20];
String message;
int errMessage;
int count=0;

void setup()
{
  Serial.begin(115200);
  delay(3000);
  pinMode (10, OUTPUT);
  digitalWrite (10, LOW);
  if(CAN.begin(CAN_500KBPS)) {
    Serial.println("CAN Init ok"); // init can bus : baudrate = 500k
  }
  else {
  //  Serial.println("CAN Init ERROR"); // init can bus : baudrate = 500k
  }  
  //CAN.init_Filt(0,0,1665);
  attachInterrupt(0, MCP2515_ISR, FALLING);     // start interrupt
  
  Serial.println("Init finished");
}

void MCP2515_ISR()
{
  Serial.println("1");
  Flag_Recv = 1;
  if (Flag_Recv == 1) { 
      message = "";
      Flag_Recv = 0;                        // clear flag
      CAN.readMsgBuf(&len, buf);       // read data,  len: data length, buf: data buf
      errMessage = 0;
      errMessage = CAN.checkError();
      if (errMessage != 0) {
        Serial.print ("Error = ");
         Serial.println(errMessage);
      }
      
      //Serial.println("t");
      /*Serial.println(CAN.getCanId());
      delay(1);*/
      count++;
      Serial.println(count);
      if (count>9) {
        count = 0;
      }
      
      //Serial.println("interrupt");
      message += CAN.getCanId(); 
      if (message == "1665") {
        message += " ";
        message += len; 
        message += + " ";
        
        //Serial.print("data len = ");
        //Serial.print(len);
        
        for(int i = 0; i<len; i++)            // print the data
        {
          //Serial.print(buf[i]);
          //Serial.print("\t");
          message += buf[i];//+ " ";
          message += " ";
        }
        //Serial.println(message.length());
        //delay(10);
        Serial.print(message);
        Serial.println();
      }
      //delay(100);
      
    }
    
}

void loop()
{
    /*if(Flag_Recv)                           // check if get data
    {
      message = "";
      Flag_Recv = 0;                        // clear flag
      CAN.readMsgBuf(&len, buf);       // read data,  len: data length, buf: data buf
      errMessage = 0;
      errMessage = CAN.checkError();
      if (errMessage != 0) {
        Serial.print ("Error = ");
         Serial.println(errMessage);
      }
      
      //Serial.println("t");
      //Serial.println(CAN.getCanId());
      //Serial.println("interrupt");
      message += CAN.getCanId(); 
      if (message == "1665") {
        message += " ";
        message += len; 
        message += + " ";
        
        //Serial.print("data len = ");
        //Serial.print(len);
        
        for(int i = 0; i<len; i++)            // print the data
        {
          //Serial.print(buf[i]);
          //Serial.print("\t");
          message += buf[i];//+ " ";
          message += " ";
        }
        //Serial.println(message.length());
        //delay(10);
        //Serial.print(message);
        //Serial.println();
      }
      delay(100);
    }
    */
    //delay(100);
}

/*********************************************************************************************************
  END FILE
*********************************************************************************************************/

This is a standard example from MCP CAN library.
I know, that I want to filter only messages came from CAN ID: 1665 (or 0x681)
Then I have a lot of non useful messages that starts from 0xF0
And I'm only interested in those who start from 0x04

So I tried to do a lot with code:

// demo: CAN-BUS Shield, receive data
#include <mcp_can.h>
#include <SPI.h>

unsigned char Flag_Recv = 0;
unsigned char len = 0;
unsigned char buf[8];
char str[20];
String message;
int errMessage;
int count=0;

void setup()
{
  Serial.begin(115200);
  delay(3000);
  pinMode (10, OUTPUT);
  digitalWrite (10, LOW);
  if(CAN.begin(CAN_500KBPS)) {
    Serial.println("CAN Init ok"); // init can bus : baudrate = 500k
  }
  else {
  //  Serial.println("CAN Init ERROR"); // init can bus : baudrate = 500k
  }  
  //CAN.init_Filt(0,0,1665);
  attachInterrupt(0, MCP2515_ISR, FALLING);     // start interrupt
  
  Serial.println("Init finished");
}

void MCP2515_ISR()
{
  Serial.println("1");
  Flag_Recv = 1;
  if (Flag_Recv == 1) { 
      message = "";
      Flag_Recv = 0;                        // clear flag
      CAN.readMsgBuf(&len, buf);       // read data,  len: data length, buf: data buf
      errMessage = 0;
      errMessage = CAN.checkError();
      if (errMessage != 0) {
        Serial.print ("Error = ");
         Serial.println(errMessage);
      }
      
      //Serial.println("t");
      /*Serial.println(CAN.getCanId());
      delay(1);*/
      count++;
      Serial.println(count);
      if (count>9) {
        count = 0;
      }
      
      //Serial.println("interrupt");
      message += CAN.getCanId(); 
      if (message == "1665") {
        message += " ";
        message += len; 
        message += + " ";
        
        //Serial.print("data len = ");
        //Serial.print(len);
        
        for(int i = 0; i<len; i++)            // print the data
        {
          //Serial.print(buf[i]);
          //Serial.print("\t");
          message += buf[i];//+ " ";
          message += " ";
        }
        //Serial.println(message.length());
        //delay(10);
        Serial.print(message);
        Serial.println();
      }
      //delay(100);
      
    }
    
}

void loop()
{
    /*if(Flag_Recv)                           // check if get data
    {
      message = "";
      Flag_Recv = 0;                        // clear flag
      CAN.readMsgBuf(&len, buf);       // read data,  len: data length, buf: data buf
      errMessage = 0;
      errMessage = CAN.checkError();
      if (errMessage != 0) {
        Serial.print ("Error = ");
         Serial.println(errMessage);
      }
      
      //Serial.println("t");
      //Serial.println(CAN.getCanId());
      //Serial.println("interrupt");
      message += CAN.getCanId(); 
      if (message == "1665") {
        message += " ";
        message += len; 
        message += + " ";
        
        //Serial.print("data len = ");
        //Serial.print(len);
        
        for(int i = 0; i<len; i++)            // print the data
        {
          //Serial.print(buf[i]);
          //Serial.print("\t");
          message += buf[i];//+ " ";
          message += " ";
        }
        //Serial.println(message.length());
        //delay(10);
        //Serial.print(message);
        //Serial.println();
      }
      delay(100);
    }
    */
    //delay(100);
}

/*********************************************************************************************************
  END FILE
*********************************************************************************************************/

sorry for lots of comments, but I'm trying to find out the fastest solution.

Here I'm using Strings. May my problem be due to this?

With best regards,
Alexander