Interrupt puzzle while using hall effect sensor with ATtiny85 to read RPM.

Hello!

I am trying to create a hall effect sensor driven RPM detector using an ATtiny85.

The problem I am having is that the interrupt is being triggered on any change (rising or falling) and is causing the RPM reading to be wildly inaccurate and unpredictable. Instead of just triggering an interrupt any time the magnetic field changes, I want to be able to detect a rising edge and then a falling edge to determine that a full rotation has occurred instead of detecting a possible dozen triggers during one single rotation of a two-pole rotor due to magnetic field strength variations between each trigger sample period.

This is the code I am using:

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

//Initialize variables
unsigned int timer[2] = {millis(), millis()};
unsigned int rpm[5] = {0, 0, 0, 0, 0};
unsigned int record = 0;
unsigned int idle[3] = {millis(), millis(), 0};
const int poles = 2;

void setup()
{
    pinMode(0, OUTPUT);
    pinMode(1, OUTPUT);
    pinMode(2, OUTPUT);
    pinMode(3, OUTPUT);
    pinMode(4, INPUT);
    GIMSK = 0b00100000;    // turns on pin change interrupts
    PCMSK = 0b00010000;    // turn on interrupts on pins PB4
    sei();                 // enables interrupts
    digitalWrite(0, LOW);
    digitalWrite(1, LOW);
    digitalWrite(2, LOW);
    digitalWrite(3, LOW);
}

void loop()
{
  // display values using LEDs
  if(rpm[0] > 0 && rpm[0] < 6500)
  {
    digitalWrite(0, HIGH);
    digitalWrite(1, LOW);
    digitalWrite(2, LOW);
    digitalWrite(3, LOW);
  }
  // ...
  // more multiplexing of LEDS in here based on RPM reading
  // ...
  else if(rpm[0] >= 9000)
  {
    digitalWrite(0, HIGH);
    digitalWrite(1, LOW);
    digitalWrite(2, LOW);
    digitalWrite(3, HIGH);
  }
  // keep track of max RPM record this session
  if(rpm[0] > record)
  {
    record = rpm[0];
  }
  // save current active time
  idle[0] = millis();
  // if inactive for 5 seconds, show record RPM
  if((idle[0] - idle[1]) > 5000 && idle[2] == 0)
  {
    // reset RPM readings for next run
    rpm[0] = 0;
    rpm[1] = 0;
    rpm[2] = 0;
    rpm[3] = 0;
    rpm[4] = 0;
    // keep track of idle status
    idle[1] = millis();
    idle[2] = 1;
    // display values using LEDs
    if(record > 0 && record < 6500)
    {
      digitalWrite(0, HIGH);
      digitalWrite(1, LOW);
      digitalWrite(2, LOW);
      digitalWrite(3, LOW);
    }
    // ...
    // more multiplexing of LEDS in here based on RPM stored record
    // ...
    else if(record >= 9000)
    {
      digitalWrite(0, HIGH);
      digitalWrite(1, LOW);
      digitalWrite(2, LOW);
      digitalWrite(3, HIGH);
    }
  }
}
 
ISR(PCINT0_vect)
{
    // store the old RPM readings
    rpm[4] = rpm[3];
    rpm[3] = rpm[2];
    rpm[2] = rpm[1];
    // get the current time
    timer[0] = millis();
    // compare the times to determine the RPM taking into consider the number of poles per rotation
    rpm[1] = (60000 / (timer[0] - timer[1])) / poles;
    // save the current time for the next loop to compare against
    timer[1] = timer[0];
    // average the last 4 RPM readings
    rpm[0] = (rpm[1] + rpm[2] + rpm[3] + rpm[4]) / 4;
    // save current active time and state
    idle[1] = millis();
    idle[2] = 0;
}

Attached is a circuit schematic drawing and an oscilloscope image of the hall effect output signal from one rotation of the rotor.


The input to Pin3 (PB4) of ATtiny85 comes from a DRV5053 hall effect sensor with an at-rest voltage output of approximately 1.0 volts which rises up to a 1.8 volt peak and falls down to a 0.2 volt dip.

I have had some success by instead using an analogRead in the void loop and manually keeping track of when a value is rising, has peaked in the positive direction, is falling, has peaked in the falling direction, etc., but that seems like a pretty inelegant solution and I would rather use an interrupt.

Thank you for the help!

Let me know if you need more details!

...
unsigned int timer[2] = {millis(), millis()};
...
unsigned int idle[3] = {millis(), millis(), 0};
...

Why?

There needs to be two timer values stored. Let's say one trigger occurs at 0.0020 seconds and the next trigger occurs at 0.0031 seconds. After such an event occurred, timer[1] would be 0.0031 and timer[0] would be 0.0020. Subtracting the two would show 0.0011 seconds passed and then RPM could be calculated based on how many revolutions per minute would occur at that pace.

The idle array is similar except it has an additional third member to keep track of whether or not a recent reading has occurred or whether the rotor is idle. I added the third array member for storing status since it would then be faster to check that third array member over and over again each loop iteration instead of manually checking the times of the last valid trigger against the current time every loop iteration.

Why have a separate timer array and an idle array instead of both using the same timer? The idle array has a value which is adjusted during the void loop and a value which is adjusted during the trigger event. The timer array has both values adjusted only during the trigger event. The two have different purposes and different values and thus are two separate arrays. I guess I could have just made the timer array have more members and add the idle array values into the timer array as three additional members.

Why arrays instead of individual variables? I guess it seems cleaner to me to use timer[1] and timer[0] instead of nowTime and oldTime. Same thing with the rpm array. rpm[0] through rpm[4] seems easier to digest than rpmOld1, rpmOld2, rpmOld3, rpmOld4, and rpmAverage.

Is it a bad thing to use arrays? Is it better to use a bunch of different variables instead for some reason? Does it have an effect on the operation of the device?

Why is millis used to initialize the arrays? The array initialization occurs before init is called. Which means the value returned from millis is guaranteed to always be zero. The following is functionally equivalent (and significantly more efficient)...

...
unsigned int timer[2] = {0,0};
...
unsigned int idle[3] = {0,0,0};
...

Copy that. The more you know...

I will change that. Any other ideas for separately detecting trigger rising and trigger falling on the ATtiny85?

This is how I handle such things…
https://storage.googleapis.com/google-code-archive-downloads/v2/code.google.com/arduino-tiny/PinChangeInterrupt-0001.zip

(Huh. Looks like I forgot to move it to github.)

Those variables you are changing inside an ISR should be declared volatile. See my page about Interrupts

Variables inside an ISR should be volatile. Copy that!

After taking what has been said into consideration, after simplifying out the stuff which is not important at the moment, and after only adjusting what I understood above, this is where I am at:

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

//Initialize variables
volatile unsigned int timer[2] = {0, 0};
volatile unsigned int rpm[5] = {0, 0, 0, 0, 0};
volatile const int poles = 2;

void setup()
{
	// set up io
	pinMode(0, OUTPUT);    // PB0 pin 5
	pinMode(1, OUTPUT);    // PB1 pin 6
	pinMode(2, OUTPUT);    // PB2 pin 7
	pinMode(3, OUTPUT);    // PB3 pin 2
	pinMode(4, INPUT);     // PB4 pin 3

	// set up interrupt
	GIMSK = 0b00100000;    // turns on pin change interrupts
	PCMSK = 0b00010000;    // turn on interrupts on pin PB4
	sei();                 // enables interrupts

	// turn LEDs off
	digitalWrite(0, LOW);
	digitalWrite(1, LOW);
	digitalWrite(2, LOW);
	digitalWrite(3, LOW);
}

void loop()
{
	// display values using multiplexed LEDs
	if(rpm[0] > 0 && rpm[0] < 2000)
	{
		digitalWrite(0, HIGH);
		digitalWrite(1, LOW);
		digitalWrite(2, LOW);
		digitalWrite(3, LOW);
	}
	// ...
	// more multiplexing of LEDS in here based on RPM reading
	// ...
	else if(rpm[0] >= 9000)
	{
		digitalWrite(0, LOW);
		digitalWrite(1, LOW);
		digitalWrite(2, LOW);
		digitalWrite(3, HIGH);
	}
}
 
ISR(PCINT0_vect)
{
	// store the old RPM readings
	rpm[4] = rpm[3];
	rpm[3] = rpm[2];
	rpm[2] = rpm[1];

	// get the current time
	timer[0] = millis();

	// compare the times to determine the RPM taking into consider the number of poles per rotation
	rpm[1] = (60000 / (timer[0] - timer[1])) / poles;

	// save the current time for the next event to compare against
	timer[1] = timer[0];

	// average the last 4 RPM readings
	rpm[0] = (rpm[1] + rpm[2] + rpm[3] + rpm[4]) / 4;
}

It is still just a regular pinChange type of an event with no detection for rising or falling edge and no way of knowing when each revolution completes.

From what I have read, it is much easier to do rising edge, falling edge, or regular pinchange with the Atmega328 compared to the ATtiny85. It seems like the Atmega328 already has the commands built in. The example files given by Coding Badly were way over my head and I did not understand it. The information given by Nick Gammon didn’t seem to provide an answer for how it should be performed on an ATtiny85.

Can I simply perform an analogRead inside the ISR to determine for myself the value and determine for myself whether the value is rising or falling compared to its previous value? Is there no method which is built in which makes this functionality exist through a simple command?

Is there a certain pin I need to use? Or a certain command? I am getting more confused. What if GIMSK = 0b01000000; instead of GIMSK = 0b00100000;? What is the difference between PCIE (pin change interrupt) and INT0? Is INT0 only used for waking from sleep?

I did this and it seems to work except for maybe having a slight bug in my logic. Since it is doing an analogRead and manually detecting if the signal was rising or falling, it makes me think that I should just stop using an interrupt for such a simple task and just make a tight loop instead.

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

//Initialize variables
volatile unsigned int timer[2] = {0, 0};
volatile unsigned int rpm[5] = {0, 0, 0, 0, 0};
volatile unsigned int phase[3] = {0, 0, 0};
volatile const int poles = 1;

void setup()
{
  // set up io
  pinMode(0, OUTPUT);    // PB0 pin 5
  pinMode(1, OUTPUT);    // PB1 pin 6
  pinMode(2, OUTPUT);    // PB2 pin 7
  pinMode(3, OUTPUT);    // PB3 pin 2
  pinMode(4, INPUT);     // PB4 pin 3

  // set up interrupt
  GIMSK = 0b00100000;    // turns on pin change interrupts
  PCMSK = 0b00010000;    // turn on interrupts on pin PB4
  sei();                 // enables interrupts

  // turn LEDs off
  digitalWrite(0, LOW);
  digitalWrite(1, LOW);
  digitalWrite(2, LOW);
  digitalWrite(3, LOW);
}

void loop()
{
  // display values using multiplexed LEDs
  if(rpm[0] > 0 && rpm[0] < 6500)
  {
    digitalWrite(0, HIGH);
    digitalWrite(1, LOW);
    digitalWrite(2, LOW);
    digitalWrite(3, LOW);
  }
  else if(rpm[0] >= 6500 && rpm[0] < 7000)
  {
    digitalWrite(0, LOW);
    digitalWrite(1, HIGH);
    digitalWrite(2, HIGH);
    digitalWrite(3, HIGH);
  }
  else if(rpm[0] >= 7000 && rpm[0] < 7500)
  {
    digitalWrite(0, HIGH);
    digitalWrite(1, HIGH);
    digitalWrite(2, LOW);
    digitalWrite(3, LOW);
  }
  else if(rpm[0] >= 7500 && rpm[0] < 8000)
  {
    digitalWrite(0, LOW);
    digitalWrite(1, LOW);
    digitalWrite(2, HIGH);
    digitalWrite(3, HIGH);
  }
  else if(rpm[0] >= 8000 && rpm[0] < 8500)
  {
    digitalWrite(0, HIGH);
    digitalWrite(1, HIGH);
    digitalWrite(2, HIGH);
    digitalWrite(3, LOW);
  }
  else if(rpm[0] >= 8500 && rpm[0] < 9000)
  {
    digitalWrite(0, LOW);
    digitalWrite(1, LOW);
    digitalWrite(2, LOW);
    digitalWrite(3, HIGH);
  }
  else if(rpm[0] >= 9000)
  {
    digitalWrite(0, HIGH);
    digitalWrite(1, LOW);
    digitalWrite(2, LOW);
    digitalWrite(3, HIGH);
  }
}
 
ISR(PCINT0_vect)
{
  phase[1] = phase[0];
  phase[0] = analogRead(A2);
  if(phase[0] > phase[1])
  {
    phase[3] = 1;
  }
  else if(phase[0] < phase[1])
  {
    phase[3] = 0;
  }
  if(phase[3] == 1)
  {
    // store the old RPM readings
    rpm[4] = rpm[3];
    rpm[3] = rpm[2];
    rpm[2] = rpm[1];
  
    // get the current time
    timer[0] = millis();
  
    // compare the times to determine the RPM taking into consider the number of poles per rotation
    rpm[1] = (60000 / (timer[0] - timer[1])) / poles;
  
    // save the current time for the next event to compare against
    timer[1] = timer[0];
  
    // average the last 4 RPM readings
    rpm[0] = (rpm[1] + rpm[2] + rpm[3] + rpm[4]) / 4;
  }
}

ty_ger07:
From what I have read, it is much easier to do rising edge, falling edge, or regular pinchange with the Atmega328 compared to the ATtiny85.

Reference.

Can I simply perform an analogRead inside the ISR to determine for myself the value and determine for myself whether the value is rising or falling compared to its previous value?

digitalRead is a better choice. Or port read (PIN).

Is INT0 only used for waking from sleep?

External Interrupt Request (INT0) supports change, edge, and level. It is available on one pin with the ATtiny85 processor.

Well, I guess I don't know any more. I have read so many different things and so many different approaches that it is making my head spin. Maybe it depends on the core and library used.

digitalRead is a better choice. Or port read (PIN).

I will take that into consideration.

External Interrupt Request (INT0) supports change, edge, and level. It is available on one pin with the ATtiny85 processor.

So, I should be reading from PB2 pin 7 if I want what I was attempting to work properly? I have been trying to read from PB4 pin 3 this whole time.

So, if I do this:

void setup()
{
   GIMSK = 0b01000000;    // turns on INT0 interrupts
   PCMSK = 0b00000100;    // turn on interrupts on pin PB2
   sei();                 // enables interrupts
}

I will have an INT0 interrupt on pin PB2, correct? Is there anything wrong with that?

Then, how do I specify a rising edge? Currently, I am just looking for pin change by using:

ISR(PCINT0_vect)
{
...

ty_ger07:
So, I should be reading from PB2 pin 7 if I want what I was attempting to work properly?

If you want to use External Interrupt Request then you will have to use PB2. That is a fine choice.

Or, you can use Pin Change Interrupts and do the edge detection yourself. Either will work.

Ok, thank you!

How does this look? Do you see any errors or poor optimization?

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

// variables
volatile unsigned int counted = 0;
volatile unsigned int timer[2] = {0, 0};
volatile unsigned int rpm[5] = {0, 0, 0, 0, 0};

void setup()
{
  // set up pins IO
  pinMode(0, OUTPUT);   // PB0 pin 5 - OUTPUT
  pinMode(1, OUTPUT);   // PB1 pin 6 - OUTPUT
  pinMode(2, INPUT);    // PB2 pin 7 - INPUT
  pinMode(3, OUTPUT);   // PB3 pin 2 - OUTPUT
  pinMode(4, OUTPUT);   // PB4 pin 3 - OUTPUT
  pinMode(5, OUTPUT);   // PB5 pin 1 - OUTPUT

  // set up interrupt
  GIMSK = 0b01000000;    // turns on rising edge INT0 interrupt
  PCMSK = 0b00000100;    // turn on interrupt on PB2 pin 7
  sei();                 // enables interrupts
}

void loop()
{
  counted = 0;
}
 
ISR(INT0_vect)
{
  // if this is a new interupt which has not already been counted...
  if(counted == 0)
  {
    // save the previous time
    timer[1] = timer[0];
    
    // get the current time
    timer[0] = millis();
    
    // store the old RPM readings cascading backwards in time
    // this will be used later for averaging
    rpm[4] = rpm[3];
    rpm[3] = rpm[2];
    rpm[2] = rpm[1];
    
    // compare the times to determine the RPM
    rpm[1] = 60000 / (timer[0] - timer[1]);
    
    // average the last 4 RPM readings
    rpm[0] = (rpm[1] + rpm[2] + rpm[3] + rpm[4]) / 4;

    // display values using  LEDs labeled respective to the values below
    if(rpm[0] > 0 && rpm[0] < 3000)
    {
      digitalWrite(0, HIGH);
      digitalWrite(1, LOW);
      digitalWrite(3, LOW);
      digitalWrite(4, LOW);
      digitalWrite(5, LOW);
    }
    else if(rpm[0] >= 3000 && rpm[0] < 4000)
    {
      digitalWrite(0, LOW);
      digitalWrite(1, HIGH);
      digitalWrite(3, HIGH);
      digitalWrite(4, HIGH);
      digitalWrite(5, HIGH);
    }
    else if(rpm[0] >= 4000 && rpm[0] < 5000)
    {
      digitalWrite(0, HIGH);
      digitalWrite(1, HIGH);
      digitalWrite(3, LOW);
      digitalWrite(4, LOW);
      digitalWrite(5, LOW);
    }
    else if(rpm[0] >= 5000 && rpm[0] < 6000)
    {
      digitalWrite(0, LOW);
      digitalWrite(1, LOW);
      digitalWrite(3, HIGH);
      digitalWrite(4, HIGH);
      digitalWrite(5, HIGH);
    }
    else if(rpm[0] >= 6000 && rpm[0] < 7000)
    {
      digitalWrite(0, HIGH);
      digitalWrite(1, HIGH);
      digitalWrite(3, HIGH);
      digitalWrite(4, LOW);
      digitalWrite(5, LOW);
    }
    else if(rpm[0] >= 7000 && rpm[0] < 8000)
    {
      digitalWrite(0, LOW);
      digitalWrite(1, LOW);
      digitalWrite(3, LOW);
      digitalWrite(4, HIGH);
      digitalWrite(5, HIGH);
    }
    else if(rpm[0] >= 8000 && rpm[0] < 9000)
    {
      digitalWrite(0, HIGH);
      digitalWrite(1, HIGH);
      digitalWrite(3, HIGH);
      digitalWrite(4, HIGH);
      digitalWrite(5, LOW);
    }
    else if(rpm[0] >= 9000)
    {
      digitalWrite(0, LOW);
      digitalWrite(1, LOW);
      digitalWrite(3, LOW);
      digitalWrite(4, LOW);
      digitalWrite(5, HIGH);
    }
  }
  
  // store that this interrupt has been counted
  counted = 1;
}

Yes, I am using a high voltage programmer and have disabled the reset pin fuse bit in order to use that pin as an output.

EDIT:

The LEDs flicker as if they forget their state during interrupt changes and it sort of seems to work, but the RPM indication fluctuates sporadically and is not very accurate. It seems like my timer isn’t very accurate or I have a bug / lack of understanding.

I guess the problem is that I just am not very good at C and don't know what I am doing. I need to either buckle down and do a lot more learning or need someone to hold my hand. The idea in my mind seems so simple but for some reason I can't make it work.