Arduino Mega 2560 + Arduino Motor Shield Rev3 : Interrupts Issue

Hello everyone,

I am working on a project with an arduino mega 2560 and an arduino motor shield rev3 in order to control a dc motor. A wheel is fixed on the motor and we have a sensor which detect an aperture on the wheel. The whole system is turning at 100 Hz and the aim of the project is to generate a pulse each time we detect this aperture.

The sensor output is connected to two different interrupts (int1 and int5). Int5 is declared on rising edge and is useful only when we want to stop the motor (there is 3 different modes which are controlled with push buttons). Int 1 is declared on falling edge and it is our reference to generate the pulse in output.

In theory I should have a pulse in output every 10 ms since the sensor detect the aperture every 10ms. It works but sometimes I also have random pulses in output. I checked the int 1 flag and indeed the interrupt is triggered but when I check the sensor signal there is no noise or unwanted detection. So I am wondering if I did set up my interrupts correctly ? If I did then in which condition the flag could set itself without seeing an interrupts on its pin ?
If someone had the time to look at my program it would be awesome and highly appreciated. Thank you and have a good friday !

I attached the main.c file

main.c (8.22 KB)

The OP’s code:

/*
   Beam_Chopper_100Hz.c
   Created: 02/20/2018
   Author : VERGER Romain
*/
//Instr_Time=62.5ns because the microcontroleur as a frequency of 16MHz

/* DEFINE */
#define bit_get(p,m) ((p) & (m))
#define bit_set(p,m) ((p) |= (m))
#define bit_clear(p,m) ((p) &= ~(m))
#define bit_flip(p,m) ((p) ^= (m))
#define bit_write(c,p,m) (c ? bit_set(p,m) : bit_clear(p,m))
#define BIT(x) (0x01 << (x))
#define LONGBIT(x) ((unsigned long)0x00000001 << (x))

/* INCLUDE */
#include <avr/io.h>
#include <util/delay.h>
#include <avr/interrupt.h>
#include <float.h>
#include <math.h>

/* VARIABLE INITIALIZATION*/
unsigned int dir = 0;   //direction variable
int Chopper_Cycle = 0;  //count the trigger cycle (in chopper mode)
int OneShot_Cycle = 0;  //count the trigger cycle (in one shot mode)
int Chopper_mode = 0;   //beam Chopper mode = Chopper
int Continue_mode = 1;    //beam Chopper mode = Continue (default mode)
int OneShot_mode = 0;   //beam Chopper mode = One Shot
int OneShot_Pushed = 0;   //send one trigger to the generation laser
unsigned int time_10us = 0; //increase in the timer 2 interruption
unsigned int time_mes = 0; //store the result of time_10us
//speed error
unsigned int PWM = 10000; //PWM value
unsigned int time_ref = 950; //reference value to correct speed [100Hz]
int kp1 = 2;        //control coefficient
//system safety
unsigned int PWM_Max = 65500; //PWM maximum value
unsigned int PWM_Min = 000; //PWM minimum value

/* INITIALIZATION */
/* Output initialization */
int init_PIN(void)
{
  //Shield
  PORTE = (1 << PE3);
  DDRB = (1 << DDB7);       //set pin PB7 as output (Motor 2 direction)
  //Generation Laser Trigger
  DDRL = (1 << DDL7);       //set pin PL7 as output (trigger pulse for the generation laser)
  //Buttons LED
  DDRG = (1 << DDG0);       //set pin PG0 as output (Chopper mode LED)
  DDRC = (1 << DDC0);       //set pin PC0 as output (Continue mode LED)
  DDRH = (1 << DDH1);       //set pin PH1 as output (One Shot mode LED)
  PORTG = (Chopper_mode << PG0);  //turn off chopper mode LED
  PORTC = (Continue_mode << PC0); //turn on continue mode LED (mode by default)
  PORTH = (OneShot_mode << PH1);  //turn off one shot mode LED
  return 0;
}
/* Input initialization */
int init_ISR(void)
{
  //Beam Chopper Wheel
  DDRE = (0 << DDE5);       //set pin PE5 as input (wheel sensor output)
  //Buttons
  DDRD = (0 << DDD0);       //set pin PD0 as input (One Shot Push button)
  DDRD = (0 << DDD1);       //set pin pD as input (wheel sensor output)
  DDRD = (0 << DDD2);       //set pin PD2 as input (Chopper button)
  DDRD = (0 << DDD3);       //set pin PD3 as input (Continue button)
  DDRE = (0 << DDE4);       //set pin PE4 as input (One Shot button)
  //External Interrupt
  EIMSK |= (1 << INT0);     //activates INT0 (PD0)
  EIMSK |= (1 << INT1);     //activates INT1 (PD1)
  EIMSK |= (1 << INT2);     //activates INT2 (PD2)
  EIMSK |= (1 << INT3);     //activates INT3 (PD3)
  EIMSK |= (1 << INT4);     //activates INT4 (PE4)
  EIMSK |= (1 << INT5);     //activates INT5 (PE5)
  EICRA |= 0B10101011;      //set INT2, INT3 & INT4 on falling edge and INT0 & INT1 on rising edge
  EICRB |= 0B00001110;      //set INT4 & INT5 on falling edge
  return 0;
}
/* Timer 2 initialization */
int init_Timer2(void)
{
  //set timer with interrupts each 10us
  TCCR2A = 0;       //set TCCR2A register to 0
  TCCR2A |= (1 << WGM21); //activate the CTC mode
  OCR2A = 160;      //set the top to 10us (OCR2A*Instr_Time*Prescale)
  TCCR2B = 0;     //set TCCR2B register to 0
  TCNT2 = 0;      //counter value =0
  GTCCR |= (1 << PSRASY); //reset prescaler
  TCCR2B |= (1 << CS20); //no prescaler and start timer2
  TIMSK2 |= (1 << OCIE2A); //enable timer2 interrupt
  sei();
  return 0;
}
/* PWM initialization */
void init_pwm1(void)
{
  DDRB |= (1 << DDB5);                  //PB5 as output
  ICR1 = 0xFFFF;                    //set maximum to 16 bits
  OCR1A = 8500;                     //duty cycle of 13% (OCR1A/ICR1)
  TCCR1A |= (1 << COM1A1);                //set non-inverting mode
  TCCR1A |= (0 << WGM11);                 //set phase and frequency corrected PWM using ICR1 as top
  TCCR1B |= (0 << WGM12) | (1 << WGM13);
  TCCR1B |= ((1 << CS10) | (0 << CS11) | (0 << CS12 )); //set prescaler to 1 and starts PWM
}

/* SUB PROGRAMS & MAIN PROGRAM */
void turn(void)
{
  /*Trigger during Chopper Mode*/
  if (Chopper_mode == 1)            //if on chopper mode
  {
    PORTG = (1 << PG0);             //Chopper button LED
    PORTH = (0 << PH1);             //1 shot button LED
    PORTC = (0 << PC0);             //Continuous button LED
    PORTL = (1 << PL7);
    while (Chopper_Cycle != 1000)       //send trigger 1 time on 20 in chopper mode
    {
      Chopper_Cycle++;            //increment the chopper cycle variable
    }
    Chopper_Cycle = 0;
  }
  if (Chopper_mode == 0)
  {
    PORTG = (0 << PG0);             //Chopper button LED
    PORTH = (1 << PH1);             //1 shot button LED
    PORTC = (0 << PC0);             //Continuous button LED
  }
  PORTL = (0 << PL7);               //no trigger for generation laser
  /*Trigger during One Shot Mode*/
  if (OneShot_mode == 1 && OneShot_Pushed == 1) //send the trigger each time the button shot is pushed
  {
    OneShot_Cycle++;              //increment the one shot cycle variable
    if (OneShot_Cycle == 5)         //delay to disable the case of two non wanted consecutive pushing
    {
      PORTL = (1 << PL7);           //generates a trigger pulse to the generation laser
      OneShot_Pushed = 0;           //reset this variable to force the user to push the button again
    }
  }
  /*Safety test to not exceed PWM limits*/
  if (PWM > PWM_Max)
    PWM = PWM_Max;
  if (PWM < PWM_Min)
    PWM = PWM_Min;

  OCR1A = PWM;                  //set the rotation speed

  time_mes = time_10us;
  time_10us = 0;                //restart counter
  if (time_mes > time_ref)            //motor too slow ? increase speed
  {
    PWM = PWM + kp1;
  }
  if (time_mes < time_ref)            //motor too fast ? reduce speed
  {
    PWM = PWM - kp1;
  }
}
void alignment(void)
{
  PORTE = (0 << PE3);
  while (Continue_mode == 1 && OneShot_mode == 0 && Chopper_mode == 0)    //if in continue mode
  {
    PORTG = (0 << PG0);             //Chopper button LED
    PORTH = (0 << PH1);             //1 shot button LED
    PORTC = (1 << PC0);             //Continuous button LED
    OCR1A = 0000;               //stop motor
    while ((PINE &= 0x20) == 0x20)      //while the trigger is not aligned [High level if the opto-sensor aperture is not in front of the sensor]
    {
      OCR1A = 8250;             //turn the motor
    }
    PORTL = (1 << PL7);
  }
}
int main(void)
{
  /*Initialization*/
  init_PIN();             //output initialization
  init_ISR();             //input initialization
  init_Timer2();            //timer 2 initialization
  init_pwm1();            //PWM initialization
  sei();                //enable Global Interrupt
  while (1)
  {
    alignment();          //align the wheel matches the trigger
  }
}

/* INTERRUPTIONS */
/*Chopper mode button interruption*/
ISR(INT2_vect)
{
  Chopper_mode = 1;   //enable the chopper mode
  Continue_mode = 0;    //disable the continue mode
  OneShot_mode = 0;   //disable the one shot mode
  Chopper_Cycle = 0;    //reset the chopper cycle variable
  PORTB = (1 << PB7); //set the rotation way
  OCR1A = PWM;      //Starting Speed
}
/*Continue mode button interruption*/
ISR(INT3_vect)
{
  Chopper_mode = 0;   //disable the chopper mode
  Continue_mode = 1;    //enable the continue mode
  OneShot_mode = 0;   //disable the one shot mode
  PORTB = (1 << PB7); //set the rotation way
  PWM = 10000;
}
/*One Shot button interruption*/
ISR(INT4_vect)
{
  Chopper_mode = 0; //disable the chopper mode
  Continue_mode = 0;  //disable the continue mode
  OneShot_mode = 1; //enable the one shot mode
  OneShot_Cycle = 0;  //reset the one shot cycle variable
  OCR1A = PWM;    //starting speed
}
/*One Shot Push button interruption*/
ISR(INT0_vect)
{
  OneShot_Pushed = 1; //allow the generation of a trigger pulse for the generation laser
  OneShot_Cycle = 0; //reset the one shot cycle variable
}
/*Wheel trigger interruption*/
ISR(INT1_vect)
{
  if (Continue_mode == 0) //if in mode chopper mode or one shot mode
  {
    turn();       //sub program to turn the motor and correct the speed
  }
}
ISR(INT5_vect)
{
  if (Continue_mode == 1)         //if in continue mode
  {
    dir = !dir;             //change the rotation way in order to brake the motor
    PORTB = (dir << PB7);         //apply the rotation way modification
  }
}
/*Timer 2 interruption*/
ISR(TIMER2_COMPA_vect)
{
  time_10us++;    //each 10us we increment the variable
}

You seem to have gone to great effort to avoid using the higher level Arduino features and have made your life unnecessarily complicated with direct register manipulation. I suppose if it is a learning exercise then that's OK, but perhaps there are more important things to learn first.

In any case, any variables that are accessed in both an interrupt handler and in normal program execution need to be marked with the volatile modifier, otherwise the compiler may optimise them and break things.

You want to keep interrupt handlers a short as possible; ideally just setting a flag that your loop() then processes. Stuff like this doesn't belong in an interrupt handler:

   while (Chopper_Cycle != 1000)       //send trigger 1 time on 20 in chopper mode
    {
      Chopper_Cycle++;            //increment the chopper cycle variable
    }
    Chopper_Cycle = 0;

Although it is reasonable to detect the aperture sensor using interrupts, it is utterly unnecessary to detect button presses using interrupts. Button presses by humans are slow and can be easily picked up by regular polling. Overuse of interrupts is only going to cause problems.

Personally I would completely restructure your program.

Take a look at Nick Gammon's tips on using interrupts before going any further.
Likewise the forum sticky links here are all worth a read.

Thank you for your answer!

You are right, I will try to do the same program using the Higher level Arduino features, and do not use interrupts for the buttons. That was a dumb idea I admit it hahaha.

Hello,

I changed the program using the Arduino features as you suggested and limited the interrupts to the sensor. But I still observe these random pulses. So I’m thinking maybe someone will see what’s wrong with my code. I want to be sure that my code is ok, if it is It would mean that these random pulses are generated from something external.

Thank you for your help !
You will find my arduino code attached

Rev_G__100_Hz__2_.ino (2.82 KB)

Wow, yes, that's definitely an improvement which makes it much easier for somebody else to understand your code. You always have the possibility of optimising critical sections of code for performance later by going lower level, but there is no point in making life a misery from the beginning. :wink:
Additional benefit: easier to move your code to a different piece of Arduino hardware.

A few minor points:

volatile int Direction = 1;

You have correctly marked the shared variables as volatile, but you have to be careful. An int is two bytes, so you cannot access its value atomically without disabling and re-enabling interrupts before and after the access. Just to be clear: when you try to read or modify the int's two bytes you might access the first byte, then an interrupt occurs, inside the ISR the int is accessed again possibly changing its value, the ISR returns and now the second byte of the read occurs - so the read or modify ends up with an inconsistent result.
In your case it is easy to fix just by using byte instead of int.

void loop() {
...
  if (ChopperMode == HIGH) {
    if (Pulse == HIGH) {
      TriggerOut();
    }
    digitalWrite(Trigger, LOW);
  }
}

//Generate Pulse with a 3ms width
void TriggerOut() {
  digitalWrite(Trigger, HIGH);
  delay(3);
  Pulse = 0;
}

This seems to be a slightly unusual structure here. TriggerOut() {} doesn't actually generate a 3ms pulse, it just sets the pin HIGH.
I would move the Pulse = 0; after the TriggerOut(); and add an extra digitalWrite(Trigger, LOW); after the delay(3);. In other words, try to make your functions perform a single self contained task wherever possible.

if (Pulse == HIGH) {
...
Pulse = 1;

Although HIGH may be defined as 1, you are mixing your metaphors here. Either set Pulse to HIGH/LOW or 1/0 but don't mix.

You want to try to avoid using delay() in your code in favour of timing things using millis(), but it may not matter for your sketch and is probably not a priority at the moment anyway.

Normally you need to debounce button presses to avoid multiple dirty transitions occurring when a button is pressed or released. Again, it may or may not be a problem here.

So, onto the real issue. I don't see anything glaring at me that could be causing the spurious interrupts. In situations where I would expect a programming error, I would usually suggest making a copy of the sketch and simplifying it to the bare minimum that demonstrates the issue.
Do the spurious interrupts occur just by letting things run and not pressing any buttons?
How are you checking the sensor signal?
What sensor is it? How is it wired?

The OP’s updated code:

#include <avr/io.h>
#include <avr/interrupt.h>

// Variables Declaration
const int ContinuousButton = 18;
const int ContinuousLED = 37;
const int ChopperButton = 19;
const int ChopperLED = 41;
const int OneShotButton = 2;
const int OneShotLED = 16;
const int MotorPWM = 11;
const int MotorDirection = 13;
const int MotorSensorStop = 3;
const int Trigger = 42;

// Variables That Will Change
int ContinuousMode = 1;
int ChopperMode = 0;
int OneShotMode = 0;
int ContAction = 0;
int ChopAction = 0;
int OneShotAction = 0;
int Starting = 1;

volatile int Direction = 1;
volatile int Pulse = 0;
volatile int OnePulse = 0;

void setup() {
  // initialize the LED pin as an output:
  pinMode(ContinuousLED, OUTPUT);
  pinMode(ChopperLED, OUTPUT);
  pinMode(OneShotLED, OUTPUT);
  pinMode(9, OUTPUT);
  pinMode(MotorPWM, OUTPUT);
  pinMode(MotorDirection, OUTPUT);
  pinMode(4, OUTPUT);
  pinMode(Trigger, OUTPUT);
  // initialize the pushbutton pin as an input:
  pinMode(ContinuousButton, INPUT);
  pinMode(ChopperButton, INPUT);
  pinMode(OneShotButton, INPUT);
  //Interrupts
  attachInterrupt(digitalPinToInterrupt(3), Int5Interrupt, RISING);
  attachInterrupt(digitalPinToInterrupt(20), Int1Interrupt, FALLING);
}

//MAIN PROGRAM
void loop() {
  ContAction = !digitalRead(ContinuousButton);
  ChopAction = !digitalRead(ChopperButton);
  OneShotAction = !digitalRead(OneShotButton);
  LED();
  if (ContinuousMode == HIGH) {
    Stop();
    digitalWrite(Trigger, HIGH);
  }
  if (ChopperMode == HIGH) {
    if (Pulse == HIGH) {
      TriggerOut();
    }
    digitalWrite(Trigger, LOW);
  }
}

//Manage LED State and Modes
void LED() {
  if (ContAction == HIGH || Starting == HIGH) {
    digitalWrite(ContinuousLED, HIGH);
    digitalWrite(ChopperLED, LOW);
    digitalWrite(OneShotLED, LOW);
    ContinuousMode = 1;
    ChopperMode = 0;
    OneShotMode = 0;
    Starting = 0;
  }
  if (ChopAction == HIGH) {
    digitalWrite(ContinuousLED, LOW);
    digitalWrite(ChopperLED, HIGH);
    digitalWrite(OneShotLED, LOW);
    ContinuousMode = 0;
    ChopperMode = 1;
    OneShotMode = 0;
    Turn();
  }
  if (OneShotAction == HIGH) {
    digitalWrite(ContinuousLED, LOW);
    digitalWrite(ChopperLED, LOW);
    digitalWrite(OneShotLED, HIGH);
    ContinuousMode = 0;
    ChopperMode = 0;
    OneShotMode = 1;
    Turn();
  }
}
//Stop the motor
void Stop () {
  if (ContinuousMode == 1) {
    digitalWrite(9, LOW);
    analogWrite(MotorPWM, 0);
    while (digitalRead(3) != LOW) {
      digitalWrite(MotorDirection, Direction);
      analogWrite(MotorPWM, 40);
    }
  }
}
//Run the motor at 100Hz
void Turn () {
  digitalWrite(MotorDirection, HIGH);
  digitalWrite(9, LOW);
  analogWrite(MotorPWM, 106);
}
//Generate Pulse with a 3ms width
void TriggerOut() {
  digitalWrite(Trigger, HIGH);
  delay(3);
  Pulse = 0;
}
//Interrupts Handler
void Int1Interrupt() {
  Pulse = 1;
}
void Int5Interrupt() {
  Direction = !Direction;
}

Thank you for your complete answer, it helps a lot !

The spurious interrupts randomly occur when the system is running and without any external action from the user and the buttons are already connected to a debounce circuit.
I checked the sensor signal with an oscilloscope and there is nothing wrong with it. I even tried to replaced the signal from the sensor by a similar signal generated by a waveform generator and I observed the same effect. So it does not seem that this sensor is the source of this issue.
I will apply the advice you gave me for my code, it can only be better using these advice !
I will focus on the external components, now it seems that the issue is coming from there.

Thanks again for your help, I appreciate it