ISR routine - moving unnecessary code out

elcrni · May 5, 2021, 9:02am

hi all,

So i have a rotary encoder and i would like to use ISR for it. So i did a research, managed to compile a code and it works, nicely.
BUT, somehow i have a feeling this is NOT how ISR code should look like and not the place where this much of a code should be running, especially as i would need to add more variables.

so my ISR code goes like this:

//==============================================================================
// Interrupt Service Routine - ISR Timer
//==============================================================================

ISR(TIMER1_COMPA_vect) {

  if ( digitalRead(encoderPinA) != A_set ) { // debounce once more
    A_set = !A_set;
    if ( A_set && !B_set ){
      if (count2 == true)
        counter_hour_start ++; 
      else if (count3 == true)
        counter_minute_start ++;
      else if (count4 == true)
        counter_hour_end ++;
      else if (count5 == true)
        counter_minute_end ++;
      else if (count0 == true)
        counter ++;
    }
  }

    if ( digitalRead(encoderPinB) != B_set ) { // debounce once more
     B_set = !B_set;
      if ( B_set && !A_set ) {
      if (count2 == true)
        counter_hour_start --;
      else if (count3 == true)
        counter_minute_start --;
      else if (count4 == true)
        counter_hour_end --;
      else if (count5 == true)
        counter_minute_end --;
      else if (count0 == true)
        counter --;
    }  
  }
}

what i would like to do is move out the entire code with variables out, so i could use any part of the code like

 if (count2 == true)
        counter_hour_start --;

in the function where it is actually used, and not in the ISR.

I was thinking of something like (code i found online):

if(digitalRead(4) && D4_state){
   	D4_state = HIGH;
	//Pin D4 triggered the ISR
   }
   else if(digitalRead(4) && !D4_state){
   	D4_state = LOW;
   }
   
   
   if(digitalRead(5) && D5_state){
   	D5_state = HIGH;
	//Pin D5 triggered the ISR
   }
   else if(digitalRead(5) && !D5_state){
   	D5_state = LOW;
   }

so, i would basically just get notified of state change of the encoder, and then later using it inside different functions as needed.

Is this the way to go?

Also, one more question:
i have encoder connected to pins 2 and 3 on Mega2560 mini, those 2 pins are INT pins while i see that for ISR PCINT pins are used...
is this different and should i move encoder to different set of PCINT pins?

Many thanks,
Alek

DrDiettrich · May 5, 2021, 9:50am

Continue searching, there exist considerably simpler procedures.

elcrni · May 5, 2021, 9:53am

thanks DrDiettrich, sort of...
Posting this thread IS a part of me continuing the search for considerably simpler procedures

thanks,
Alek

DrDiettrich · May 5, 2021, 11:02am

Hint: 2 old signal states and 2 new signal states make up 16 combinations. Build an array that contains the position difference for each combination and increment/decrement the position counter accordingly.

elcrni · May 5, 2021, 11:14am

thanks, i am not exactly sure how to pull that out with array, but would it be much worse/slower if i use it like this:

if(digitalRead(4) && D4_state){
   	D4_state = HIGH;
	//Pin D4 triggered the ISR
   }
   else if(digitalRead(4) && !D4_state){
   	D4_state = LOW;
   }

then in a function i would just call the state and act on it, like this

if (D4_state == HIGH) {
if (count2 == true)
        counter_hour_start ++;
}

and later the same but for decrement. Is this valid use of ISR timing?

The problem is, i have another interrupt (on change) on another pin, that should capture 1Hz signal and decode it, and ive noticed when using encoder my 1Hz signal values jump up and down (it should read 100ms and 200ms pulse width) but if i am turning the encoder pulse width jumps +- 30-40ms.
so my goal is to get ISR done as fast as possible with as little effect on my signal decoding as possible,

Also, not to repeat myself, but i am using timing to read encoder and not real interrupt, still a bit confused on that... so i should be able to use ANY pins for encoder as long as they are PCINT?

Many thanks,
Alek

DrDiettrich · May 5, 2021, 1:29pm

Don't you realize that this code does exactly nothing?

PCINT is meaningful only with an ISR.

elcrni · May 5, 2021, 1:44pm

I did not realize that, havent had a time to test, the original code goes like this:

bool D4_state = LOW;
bool D5_state = LOW;

void setup() {  
  PCICR |= B00000100;			//Bit2 = 1 -> "PCIE2" enabeled (PCINT16 to PCINT23)
  PCMSK2 |= B00110000;			//D4 and D5 will trigger interrupt
}

void loop() { 
  //your code here...
}

ISR (PCINT2_vect) 
{
   if(digitalRead(4) && D4_state){
   	D4_state = HIGH;
	//Pin D4 triggered the ISR
   }
   else if(digitalRead(4) && !D4_state){
   	D4_state = LOW;
   }
   
   
   if(digitalRead(5) && D5_state){
   	D5_state = HIGH;
	//Pin D5 triggered the ISR
   }
   else if(digitalRead(5) && !D5_state){
   	D5_state = LOW;
   }
}

So i thought i will just use that in my ISR to determine the direction of encoder, then deal with counting later on, outside of ISR...

As said, i am quite new with all this so pardon my lack of knowledge
Thanks,
Alek

DrDiettrich · May 5, 2021, 1:54pm

Learn C or continue playing around with other peoples' code that you don't understand or ask somebody to write the code for you. The choice is yours.

elcrni · May 5, 2021, 2:08pm

This could be a solution for every single question and thread on this forum. If we could all just learn C there would be no forum.
Playing with bits and pieces of already available code is what makes all this a fun hobby to me, and i do learn something from time to time.
I could easily pay someone to write the code for me, i could just as easily buy the finished product without bothering to make something, but again, how would i have fun then?

I just kindly asked someone with greater knowledge than mine to help with opinions and advices.

So, if you are having a bad day, have a beer (or whatever you prefer), be happy and skip posting replies like the last one, that really doesn't help anyone

cheers,
Alek

alto777 · May 5, 2021, 2:52pm

It might cut the traffic, but this is by no means the case.

But about your original code in the ISR, there's nothing to it time-wise, really, and if you wanted you could save some (time) by using an alternative to digitalRead like fastDigitalRead or direct port manipulation.

a7

DrDiettrich · May 5, 2021, 2:55pm

Sorry for the unpleasant thoughts. I had to learn English before I could read data sheets, and had to learn C before I could understand Arduino program code. If you think that there is some easier way for you - good luck.

elcrni · May 5, 2021, 3:19pm

Thanks alto777, thats pretty much what i was after, i will connect the scope to see exactly how long do i stay in ISR, but what i wanted to know is if i could save some time. thanks

another thing that bothers me is interrupt pins i am using, i just did a test with NANO board and connected the encoder to pins 5 and 6, so non INT pins and it still works. So, what i would like to know is am i better off using INT pins (2 and 3) for my encoder on non INT pins, keeping in mind i already have interrupt use on another INT pin (attachInterrupt(INT_01, int0handler, CHANGE)

I would try to avoid these two clashing or blocking each other, so short question is, does encoder block the other interrupt if used on INT pin, even though encoder uses sampling rather than real interrupt.
Hope i explained it correctly.

If you never asked a single question or asked for help during the process then you are far greater man than i am, and i admire you! No hard feelings though, everyone is entitled to say whatever they want.

Many thanks,
Alek

system · September 2, 2021, 3:19pm

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.