4 encoders - is 8mhz not fast enough?

Go To Last Post
29 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Here is my ISR.  I see the ISR getting called, I have been over everything a few times, but I'm thinking the encoder is too fast to get the pins read in time.  Code below.  It's 4 encoders driving a PWM pin and RGB LEDs.  Any help appreciated!

 

I've double checked my wiring and encoder operation.  It's driving the pins properly (using internal pullups with common to Gnd)

 

ISR(PCINT2_vect)//Vector for Encoders
{
	if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 0))
     	{
		 RedValue ++;
		}
    if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 1))
		{
         RedValue --;
		}
	
		
	if ((PIND & (1<<PIND2) == 0) && (PIND & (1<<PIND3)==0))
	{
		GreenValue ++;
	}
	if ((PIND & (1<<PIND2)==0) && (PIND & (1<<PIND3) == 1))
	{
		GreenValue --;
	}


	if ((PIND & (1<<PIND4)==0) && (PIND & (1<<PIND5)==0))
	{
		BlueValue ++;
	}
	if ((PIND & (1<<PIND4)==0) && (PIND & (1<<PIND5) == 1))
	{
		BlueValue --;
	}

	
	if ((PIND & (1<<PIND6)==0) && (PIND & (1<<PIND1)==7))
	{
		if (OCR1A < 255)
		{
		    OCR1A ++;				
		}			
	}
	if ((PIND & (1<<PIND0)==6) && (PIND & (1<<PIND7) == 1))
	{
		if (OCR1A > 2)
		{
			OCR1A --;
		}			
	}
}

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It looks like the code is the cause to me.
For example, does a code with only one encoder work properly?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kabasan wrote:

It looks like the code is the cause to me.
For example, does a code with only one encoder work properly?

 

That is the hope, that I've just missed something somewhere.

 

It does not.

Last Edited: Sat. Nov 21, 2020 - 08:57 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Is revalue, green value etc declared volatile?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Kartman wrote:
Is revalue, green value etc declared volatile?

 

Yes.

 

If I just trigger one of the colors straight off the ISR that works as well, but the comparing of the 2 pins does not appear to happen in time.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I dare say the problem might be elsewhere. You also might want to consider using a better way of decoding the encoders. Using pcints is not the best way. As well, how are you managing debounce?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It's the only real option for 4 encoders except for just checking repeatedly in the while 1 loop

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

First off, this isn't correct, but not the main problem:

if ((PIND & (1<<PIND6)==0) && (PIND & (1<<PIND1)==7))

The ISR will run faster with four sets of if/else-if. You know RedValue++ and RedValue-- won't happen at the same time.

 

I see a big problem using a shared ISR. If an encoder is resting in its zero-zero position and another encoder is turning generating interrupts the resting channel will be incrementing.

The ISR tells you there was an edge, but not which of the eight inputs changed. You need a volatile variable that stores the value on PIND from the previous pass of the ISR. Then you can calculate which edge(s) occurred.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

balisong42 wrote:

The ISR will run faster with four sets of if/else-if. You know RedValue++ and RedValue-- won't happen at the same time.

Right.

 

Also, the same test is being performed twice for each encoder.  The test can be reduced to something like this.

    if (!(PIND & (1<<PIND0))
    {
        if (!(PIND & (1<<PIND1))
        {
            RedValue ++;
        }
        else
        {
            RedValue --;
        }
    }
    ....repeat for each encoder....

 

More importantly, don't test bits for == 1 or == 0.  == 0 is safe enough, but == 1 will fail for any set bit other than D0.  e.g. PIND & (1 << 3) == 1 will fail even if bit 3 is set, because the & value will either be 0 or 0x08, but never 1.  Just test for zero or non-zero.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Lets try this....

 

 

    PCMSK2 = 0b01010101;

ISR(PCINT2_vect)//Vector for Encoders
{
    //if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 0))
    if (PORTD = 0b11111110)
         {
         RedValue ++;
        }
    //if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 1))
    else if (PORTD = 0b11111100)
        {
         RedValue --;
        }
    
        
    //if ((PIND & (1<<PIND2) == 0) && (PIND & (1<<PIND3)==0))
    else if (PORTD = 0b11111011)
    {
        GreenValue ++;
    }
    //if ((PIND & (1<<PIND2)==0) && (PIND & (1<<PIND3) == 1))
    else if (PORTD = 0b11110011)
    {
        GreenValue --;
    }


    //if ((PIND & (1<<PIND4)==0) && (PIND & (1<<PIND5)==0))
    else if (PORTD = 0b11101111)
    {
        BlueValue ++;
    }
    //if ((PIND & (1<<PIND4)==0) && (PIND & (1<<PIND5) == 1))
    else if (PORTD = 0b11001111)
    {
        BlueValue --;
    }

    
    //if ((PIND & (1<<PIND6)==0) && (PIND & (1<<PIND7)==0))
    else if (PORTD = 0b10111111)
    {
        if (OCR1A < 255)
        {
            OCR1A ++;                
        }            
    }
    //if ((PIND & (1<<PIND0)==6) && (PIND & (1<<PIND7) == 1))
    else if (PORTD = 0b00111111)
    {
        if (OCR1A > 2)
        {
            OCR1A --;
        }            
    }
}
}

 

Last Edited: Sat. Nov 21, 2020 - 10:27 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

 but I'm thinking the encoder is too fast to get the pins read in time. 

You neglect to say how fast the encoder is spinning---20 rpm, 200 rpm?  2000 rpm?  Anything slow should be quite simple.

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It's a PEC12R-3225F-S0024-ND

 

Speed isn't critical, it just needs to read properly most of the time.

Last Edited: Sat. Nov 21, 2020 - 10:45 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chriscalandro wrote:

if (PORTD = 0b11111110)
    

No x2

 

First, you're using = when I assume you mean ==

 

Second, you don't care what the other bits are for each encoder - don't assume they are all 1s.  You must use a bit mask to only view the desired bits.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

chriscalandro wrote:

if (PORTD = 0b11111110)
    

No x2

 

First, you're using = when I assume you mean ==

 

Second, you don't care what the other bits are for each encoder - don't assume they are all 1s.  You must use a bit mask to only view the desired bits.

 

U understand this, but right now I'm trying to get each encoder to work individually with as few ticks as possible.  When it gets to the point I can actually read it, I'll mask the other bits.

 

I added a buffer variable at the beginning of the ISR and that seems to work.  Then I just read the buffer.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It's a PEC12R-3225F-S0024-ND

Ok, it is just a knob, not a spinning shaft encoder...so how would it ever be too fast??

 

This just requires stepping through a standard state sequence, examples are everywhere...look up encoder state machine & you will be rolling in no time.  Take a look at some examples &  take the time to understand the steps they are taking.  You will use that knowledge again in the future--so remember it!

When in the dark remember-the future looks brighter than ever.   I look forward to being able to predict the future!

Last Edited: Sat. Nov 21, 2020 - 11:57 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I recently wrote some code for encodes here...

 

https://github.com/wrightflyer/s...

 

Code for encoders in encoder.cpp/encoder.h. As main.cpp shows you can instantiate a number. My plan is to have 8. At present the test code only reads a couple of them but that all seems to work very nicely and I've no reason to doubt that it will work for all eight. These are actually for knobs on a synthesizer. The "scanning" is done in the interrupt update function called from a timer interrupt. 

 

PS by a strange quirk of fate I was running the mega32 at 4MHz simply because I happened to have a 4MHz crystal available

 

Oh and I forgot that I'd made a YouTube video of this in action... https://youtu.be/1vMo6aEs0Ek

Last Edited: Sun. Nov 22, 2020 - 01:31 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Please start by reading just one encoder.
If you neglect the basics, you will waste time.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think you have another serious problem when you start reading multiple encoders.  If encoder #1 changes and generates an interrupt, you will adjust e.g RedValue up or down.  But then if encoder #2 changes and generates an interrupt, you may adjust RedValue again, even though encoder #1 has not changed.  So I think you need to rethink your approach for multiple encoders (if I'm understanding things correctly).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

I think you have another serious problem when you start reading multiple encoders.  If encoder #1 changes and generates an interrupt, you will adjust e.g RedValue up or down.  But then if encoder #2 changes and generates an interrupt, you may adjust RedValue again, even though encoder #1 has not changed.  So I think you need to rethink your approach for multiple encoders (if I'm understanding things correctly).

 

I'm going to eventually change it back to pin masking once I get one of them working.  With the ISR and only 1 encoder set up it's not reliable. Just trying to get one read and processed as fast as possible.

 

I'm not seeing how this would end with one encoder controlling the variable for another.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chriscalandro wrote:
I'm not seeing how this would end with one encoder controlling the variable for another.

 

Using your original code as an example,

 

ISR(PCINT2_vect)//Vector for Encoders
{
	if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 0))
     	{
		 RedValue ++;
		}
    if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 1))
		{
         RedValue --;
		}
......

Call the encoder connected to PIND0/PIND1 encoder #1.  Now suppose another encoder (2, 3 or 4) triggers an interrupt and this ISR runs.  Encoder #1 has not changed state, but RedValue will be incremented or decremented again if PIND0 is still at 0.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

kk6gm wrote:

chriscalandro wrote:
I'm not seeing how this would end with one encoder controlling the variable for another.

 

Using your original code as an example,

 

ISR(PCINT2_vect)//Vector for Encoders
{
	if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 0))
     	{
		 RedValue ++;
		}
    if ((PIND & (1<<PIND0)==0) && (PIND & (1<<PIND1) == 1))
		{
         RedValue --;
		}
......

Call the encoder connected to PIND0/PIND1 encoder #1.  Now suppose another encoder (2, 3 or 4) triggers an interrupt and this ISR runs.  Encoder #1 has not changed state, but RedValue will be incremented or decremented again if PIND0 is still at 0.

 

Oh, I see what you are saying.  These have a detent, and I have verified the detent positions are all decoupled.

 

But yeah, if they weren't and it got left in a position where it was left closed it would create an issue.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chriscalandro wrote:

Oh, I see what you are saying.  These have a detent, and I have verified the detent positions are all decoupled.

 

But yeah, if they weren't and it got left in a position where it was left closed it would create an issue.

Well, if you can guarantee that two encoders won't be manipulated at the same time...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The reality is that the encoders don't output 'perfect' signals. Firstly, if they're mechanical encoders, the contacts will 'bounce' - they will give you multiple pulses for a number of milliseconds. Your code does not take this into account. You can demonstrate this if you keep a count of the number of interrupts. Move the encoder one position and you'll see a number of interrupts.

Once you observe this, then you can begin to understand why the technique you are using is flawed.

 

The skill of an embedded systems person is understanding that signals from the real world are far from perfect and knowing how to cope with this. Another skill is understand the speed of various signals. How many pulses per second would you expect from your encoders? Microprocessors work in the realm of nanoseconds where mechanical signals are 10's of milliseconds.

 

In Cliff's code you will find a more complete and robust method of handling encoders.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

OK, back to pin masking and worked my way up to all 4.  The problem is 100% read speed and the detented encoders I have having shorter pulse widths.

 

I hooked up some lower resolution encoders and got much better results.  Looks like this project will need a external crystal...

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chriscalandro wrote:

Looks like this project will need a external crystal...

 

It's your approach not a lack of processing speed that is the problem. Consider...

 

1) You have 4 encoders but only 2 hands

2) Your encoders have 24 detents

3) It is impossible to rotate, in one action, the encoder more than 270 degrees

4) The fastest you can move 270 degrees is no less than 200ms

 

So, the maximum number of detents in one action is 16, times 2 hands, in 200ms

 

Which I make 6.25ms per detent. Or 50,000 CPU cycles.

 

So you have plenty of CPU time.

 

Looking at the datasheet I see a couple of things...

 

1) The debounce time is no more than 2ms

2) There is a recommended filter circuit - have you implemented that?

 

Where it me doing this I would sample the inputs at 1ms, run a software debounce with a vote of 3, and then decode in software with simple state machines. And I'd likely be able to do it on an AVR running at 1MHz.

#1 Hardware Problem? https://www.avrfreaks.net/forum/...

#2 Hardware Problem? Read AVR042.

#3 All grounds are not created equal

#4 Have you proved your chip is running at xxMHz?

#5 "If you think you need floating point to solve the problem then you don't understand the problem. If you really do need floating point then you have a problem you do not understand."

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

chriscalandro wrote:
Oh, I see what you are saying. 
This is part of the reason why the code I linked above is written in C++. All "state" to do with a particular encoder reading are class members so each is completely independent and does not interfere with any other.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

For very slow encoders like this I would use something like a 200Hz-2000Hz timer ISR that check the IO's for the encoders, if there is a change since last ISR an encoder have moved

Read only once for each ISR, and use that value (status) for the hole ISR, else your code aren't robust if IO's change while in the ISR.

 

For things like this don't do any debounce if your encoder logic is correct, (trust me I have seen many that aren't), you will just get an extra +1 that in the next ISR will be a -1, the end result will be correct.

 

I normally do this in ASM but here is some app. numbers for C.

1000Hz ISR that take about 30clk (if no changes) and up to perhaps 100 if all 4 change at the same time.

and AVR @16MHz will then spend about 0.2% if no encoder change and 0.6%, so speed is not a problem.  

 

Edit:

I reread the title so @8MHz the raw encoder code would take about 1% of the time

Last Edited: Sun. Nov 22, 2020 - 05:05 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0


The AVR CPU executes at millions of instructions per second.  It can handle hundreds of encoders, "at the same time".

 

I recommend putting resistors and capacitors on the output pins of each encoder.  A pain at this point if the PCB has been laid out, true.  Next time don't lay out the PCB until the prototype works.

 

The R/C components will debounce your encoder signals and extend the pulses.  This is helpful when using cheap (<$1 per unit) encoders that generate very short pulses on turning.

 

Each of the four encoders will have an pin-change interrupt and a data pin:  8 input pins total.  Changing any encoder position will cause a pin-change interrupt.  In the IRQ, check the data line for the encoder to determine which direction that it was turned.   Have two boolean flags for the main code for each encoder.  One boolean flag indicates whether the individual encoder was changed and the other flag indicates its direction.   The main code polls the flags at least 20 times a second for movements on any of the four encoders.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Clearly you havent been reading closely........