I'm trying to alter some quadrature encoder code I found online to directly access the digital ports as the resolution of the encoder I'm using is too high to use the digitalRead / digitalWrite functions.
I'm very new to this so I'm probably making a very obvious mistake.
This is the code I'm trying to adapt:
#define encoder_a 2 //keep this on and interrupt pin
#define encoder_b 3 //keep this on and interrupt pin
#define motor_step 4 //can be any pin
#define motor_direction 5 //can be any pin
#include <delay.h>
volatile long motor_position, encoder;
void setup () {
//set up the various outputs
pinMode(motor_step, OUTPUT);
pinMode(motor_direction, OUTPUT);
// then the encoder inputs
pinMode(encoder_a, INPUT);
pinMode(encoder_b, INPUT);
// enable pullup as we are using an open collector encoder
digitalWrite(encoder_a, HIGH);
digitalWrite(encoder_b, HIGH);
// encoder pin on interrupt 0 (pin 2)
attachInterrupt(0, encoderPinChangeA, CHANGE);
// encoder pin on interrupt 1 (pin 3)
attachInterrupt(1, encoderPinChangeB, CHANGE);
encoder = 0; //reseet the encoder position to 0
}
void loop() {
//do stuff dependent on encoder position here
//such as move a stepper motor to match encoder position
//if you want to make it 1:1 ensure the encoder res matches the motor res by dividing/multiplying
if (encoder > 0) {
digitalWrite(motor_direction, HIGH);// move stepper in reverse
digitalWrite(motor_step, HIGH);
digitalWrite(motor_step, LOW);
delayMicroseconds(200); //_delay_us(200); //modify to alter speed
motor_position++;
encoder = 0; //encoder--;
}
else if (encoder < 0) {
digitalWrite (motor_direction, LOW); //move stepper forward
digitalWrite (motor_step, HIGH);
digitalWrite (motor_step, LOW);
delayMicroseconds(200); //_delay_us(200); //modify to alter speed
motor_position--;
encoder = 0; //encoder++;
}
}
void encoderPinChangeA() {
if (digitalRead(encoder_a)==digitalRead(encoder_b)) {
encoder--;
}
else{
encoder++;
}
}
void encoderPinChangeB() {
if (digitalRead(encoder_a) != digitalRead(encoder_b)) {
encoder--;
}
else {
encoder++;
}
}
Don't do this. It isn't causing you problems in this code I don't think, but it is a dangerous practice. This sets every bit in the direction register. What if one of those other pins was set to output for another piece of code, then you just undid that.
It would be better to use |= (OR equals). By OR'ing in the bits, you leave any other bits unchaged.
Like this:
DDRD |= B00110000;
To clear a bit, use &=~ (AND equal NOT).
The other problem is here:
if (PIND0==PIND1)
This is equivalent to someone making this common mistake:
if (buttonPinNumber == HIGH)
Where they compare the pin number instead of reading the pin. To read a pin you have to use a bitmask and read the whole port. You AND in the bitmask to cut out just the bit you need.
I like to use the digitalPinToXXXX macros when I'm doing port manipulation. That way if I mae a pin change later I don't have to go find each time I used it int he code.
Here is some code that handles a rotary encoder on one of my projects.
I use pinMode here because this isn't the part where it needs to be fast.
Here is my interrupt handler. If you want to catch all 4 transitions, then you'd need to look at the other pin as well. But this gets me one increment or decrement per "click" of my knob.
void ISR_encoder_handler(){
// My encoder has a full cycle of 4 transitions per click.
// I only want one increment or decrement per click
// So we are only looking at one transition
// We're only using the falling edge on the interrupt pin.
// So we know intPin is LOW.
// If bPin is HIGH then they're different so decrement
if(*bReg & bMask){
encoderCounter--;
}
else {
encoderCounter++;
}
}
EDIT: Here's where I attach the interrupt. Just for completenes.
What I posted is exactly what you have here except I have a variable to hold the "PIND" value called bReg and I have a varable to hold the "0b00000001" part called bMask.
And I used the appropriate macros to get those values to the variables so if I ever need to change the pin I'm using I only have to change one line of code.
No, this doesn't work like you want. This will seem to work if they are both zero but it will fail when one or both are high. The reason why it works in the previous code, with only one pin being tested, is that false is represented as a zero and true is any non-zero value. It doesn't care if the answer is 1 or 2 or 20000 that is all taken as true by the implicit conversion to boolean.
There's a lot of ways to do this:
//double-negative !! always converts zero to false and non-zero to true
if (!!(PIND & 0b00000001)==!!(PIND & 0b00000010))
//since we don't actually care if it's on or off, just different, a single negative works too
if (!(PIND & 0b00000001)==!(PIND & 0b00000010))
//this is probably the best way of writing out our real intention while still using the binary magic numbers
if ((PIND & 0b00000001)&&!(PIND & 0b00000010) || !(PIND & 0b00000001)&&(PIND & 0b00000010))
/*Now let's do it without using magic numbers...*/
#define encoder_a 2 //keep this on and interrupt pin
#define encoder_b 3 //keep this on and interrupt pin
bMaskA = digitalPinToBitMask(encoder_a );
bRegA = portInputRegister(digitalPinToPort(encoder_a));
bMaskB = digitalPinToBitMask(encoder_b);
bRegB = portInputRegister(digitalPinToPort(encoder_b));
if ((bRegA & bMaskA)&&!(bRegB & bMaskB ) || !(bRegA & bMaskA)&&(bRegB & bMaskB))
/*You know, digitalRead() is not forbidden inside an interrupt routine.
This is readable, maintainable and portable:*/
if(digitalRead(encoder_a) == digitalRead(encoder_b))
I've tried to use the double negative for now as it seems the simplest of the bunch!
Huh!
Using the bitRead() function will gain most of the speed benefit of the direct port manipulation over digitalRead(). In fact, in my benchmark testing, direct port manipulation was only faster when reading both pins at once with a mask like b00000011 and then using the result to build full quadrature past and present states.
