Forum Menu




 


Log in Problems?
New User? Sign Up!
AVR Freaks Forum Index

Post new topic   Reply to topic
View previous topic Printable version Log in to check your private messages View next topic
Author Message
ThEThInG
PostPosted: Mar 17, 2010 - 05:59 PM
Hangaround


Joined: Feb 24, 2009
Posts: 139


Does anybody see any reason I should not call a function from the ISR from a program stability standpoint? Here is what I'm doing: I have a mechanical gearbox in which I'm determining the exact position of a gear within the gearbox so that I can stop it at a certain point of rotation, for example I may want to stop it after 9 of the 16 teeth have been counted. Now I also want to keep track of how many total rotations, so 1 rotation = 16 teeth. All that works fine and dandy in my code below. Now it seems wasteful to me to keep calling the Calculate_Sector_Gear_Position function over and over when it only needs to run right after each time the ISR is called, which is trigger by a pin change of state. So my question is as stated above, if I moved the call to within the ISR, would that violate any conventions of programming? I still have a lot to learn on the software front.

Code:
#include <avr/io.h>
#include <avr/interrupt.h>
#include <stdint.h>
#include <stdbool.h>

//Definitions
#define Trigger (PINC&_BV(PINC2))
#define Gear_Sensor (PINC&_BV(PINC0))
#define Turn_On_Motor (PORTD |= _BV(PIND6))
#define Turn_Off_Motor (PORTD &= ~_BV(PIND6))
#define Turn_On_Brake (PORTB |= _BV(PINB1))
#define Turn_Off_Brake (PORTB &= ~_BV(PINB1))
#define Brake_indicator   (PINB&_BV(PINB1))
#define Set_CSxx TCCR0B |= (1<<CS01)
#define Clear_CSxx TCCR0B &= ~(1<<CS01)
#define Set_COMBits TCCR0A |= (1<<COM0A1)
#define Clear_COMBits TCCR0A &= ~(1<<COM0A1)

#define Turn_on_LED1 (PORTD |= _BV(PIND0))
#define Turn_off_LED1 (PORTD &= ~_BV(PIND0))
#define Turn_on_LED2 (PORTD |= _BV(PIND1))
#define Turn_off_LED2 (PORTD &= ~_BV(PIND1))

//Global Variables
volatile static bool sector_gear_sensor_flag, firing_completed_flag;   
volatile uint8_t sector_gear_tooth_count, gearbox_cycles;

//Fucntion Prototypes
bool Fire(bool t_flag);
bool Stop_Firing(bool t_flag);
void Calculate_Sector_Gear_Positon(bool gs_flag);

int main(){
   //Sets pin direction, PD6 Motor Drive, PB1 Motor Brake
   DDRD |= 0x40;
   DDRB |=   0x02;   

   //Sets initial state of PD6 and PB1
   PORTD &= ~0x40;   //Motor Drive off
   PORTB &= ~0x01;   //Motor Brake off

   //Enable Pull Up Resistors
   PORTC |= 0x05;   //PC0(Gbx. Pos.),PC2(Trig.)
   PORTD |= 0xA0;   //PD5,PD7   
   PORTB |= 0x05;   //PB0,PB2

   //Enable Global Interrupts and PCINT8
   sei();            
   PCICR  |= 0x02;      
   PCMSK1 |= 0x01;         

   //Sets TC0 for Phase Correct PWM on PIND6, Non-Inverted
   TCCR0A |= (1<<WGM00);   //WGM00:Mode 1
   OCR0A = 100;

   //Local Variables
   static bool trigger_flag = true;

   /* Pre-program Sensor Flag Adjustments: this section includes code which gathers sensory
   information used for determining the starting position of the gearbox. */
   sector_gear_sensor_flag = (Gear_Sensor == 0) ? true : false;    //set gearbox_sensor_flag before main loop starts

      //Main Loop
      while(1){

         trigger_flag = Fire(trigger_flag);

         trigger_flag = Stop_Firing(trigger_flag);

         Calculate_Sector_Gear_Positon(sector_gear_sensor_flag);

      }//End Main Loop      
}

//Main Interrupt
ISR(PCINT1_vect){   
   sector_gear_sensor_flag = (Gear_Sensor == 0) ? true : false;
}//End Main Interrupt

//Function Deifinitions--------------------------------------------------------------------------
bool Fire(bool t_flag){   
   if( t_flag ){
      if( Trigger == 0 )
      {
         Turn_Off_Brake;
         Set_COMBits;   
         Set_CSxx;   
         t_flag = false;
      }
   }
   return t_flag;
}//End Fire

bool Stop_Firing(bool t_flag){
   if( !t_flag ){
      if( (Trigger != 0) && (firing_completed_flag == true) ){      
         Clear_CSxx;      
         Clear_COMBits;   
         Turn_On_Brake;
         t_flag = true;
         firing_completed_flag = false;
      }
   }
   return t_flag;
}//End Stop Firing

/*Calculate_Sector_Gear_Position function is responsible for counting each tooth
of the sector gear and determining when a full cycle has been completed based on
tooth count, it will also set the firing_completed_flag so the gearbox may be stopped
when appropriate*/
void Calculate_Sector_Gear_Positon(bool gs_flag){
   static bool sgc_flag;
   if( sgc_flag != gs_flag ){
      if( gs_flag ){
         sector_gear_tooth_count = sector_gear_tooth_count + 1;
      }
   sgc_flag = gs_flag;
   }   
   if( (sector_gear_tooth_count==9) && (Trigger!=0) ){      
      firing_completed_flag = true;
   }
   if( sector_gear_tooth_count >= 16 ){
      gearbox_cycles = gearbox_cycles + 1;
      if( sector_gear_tooth_count == 16){
         sector_gear_tooth_count = 0;
      }else{
         sector_gear_tooth_count = sector_gear_tooth_count - 16;
      }
   }
}//End Calculate Sector Gear Position
 
 View user's profile Send private message  
Reply with quote Back to top
ThEThInG
PostPosted: Mar 17, 2010 - 06:06 PM
Hangaround


Joined: Feb 24, 2009
Posts: 139


I also checked with a scope the wave form produced at maximum speed by the sector gear sensor (which counts gear teeth), the minimum length of 1/2 a period is 250uS, so 250uS / 50ns = 5,000 CPU clocks (Fcpu = 20MHz). So the processor, ATMega88PA, is running through this code hundreds if not thousands of times between each period. All inputs are digital logic levels. My goal is become efficient at programming, so this is why I'm asking.
 
 View user's profile Send private message  
Reply with quote Back to top
Koshchi
PostPosted: Mar 17, 2010 - 06:08 PM
Raving lunatic


Joined: Nov 17, 2004
Posts: 9295
Location: Vancouver, BC

Quote:
I should not call a function from the ISR from a program stability standpoint?

That depends on how long the function is. You want to keep the ISR as short as possible. Also, calling a function in the ISR will force the compiler to push and pop more registers than needed since it doesn't know the context in which the function will be called.
Quote:
Now it seems wasteful to me to keep calling the Calculate_Sector_Gear_Position function over and over when it only needs to run right after each time the ISR is called

Then set a flag in the ISR and check it in main. Only call the function if the flag is set.

_________________
Regards,
Steve A.

The Board helps those that help themselves.
wylfwt?
 
 View user's profile Send private message  
Reply with quote Back to top
kk6gm
PostPosted: Mar 17, 2010 - 06:16 PM
Resident


Joined: Sep 12, 2009
Posts: 963


ThEThInG wrote:
Now it seems wasteful to me to keep calling the Calculate_Sector_Gear_Position function over and over when it only needs to run right after each time the ISR is called, which is trigger by a pin change of state.

As mentioned, you could set a flag and have main run it when the flag is set. Or you could just inline the code in the ISR and save a lot of function call overhead.
 
 View user's profile Send private message  
Reply with quote Back to top
jayjay1974
PostPosted: Mar 17, 2010 - 06:25 PM
Raving lunatic


Joined: Oct 30, 2002
Posts: 3923
Location: The Netherlands

It depends on how timing critical things are. If things are really critical, you should do the work in the ISR; otherwise there not much point in using an ISR at all. Say you need to fire something when a certain sector passes by, if you merely set a flag that you check in main(), everything that's done in main() influences the timing between the input stimulus and the output. If you have some some lengthy calculation going on that takes many milliseconds the output may come far too late; You don't want to sprinkle flag checking code everywhere to improve timing. If you handle the output in the ISR, timing is far more controllable and deterministic. That's the reason for interrupts, to handle things that must be done now. And in some cases that might mean quite a lengthy ISR too.

_________________
Free e-books
Modern Mechanix: Yesterday's Tommorow Today
 
 View user's profile Send private message  
Reply with quote Back to top
kk6gm
PostPosted: Mar 17, 2010 - 06:37 PM
Resident


Joined: Sep 12, 2009
Posts: 963


jayjay1974 wrote:
It depends on how timing critical things are. If things are really critical, you should do the work in the ISR; otherwise there not much point in using an ISR at all. Say you need to fire something when a certain sector passes by, if you merely set a flag that you check in main(), everything that's done in main() influences the timing between the input stimulus and the output. If you have some some lengthy calculation going on that takes many milliseconds the output may come far too late; You don't want to sprinkle flag checking code everywhere to improve timing. If you handle the output in the ISR, timing is far more controllable and deterministic. That's the reason for interrupts, to handle things that must be done now. And in some cases that might mean quite a lengthy ISR too.

Just wanted to add a vote of agreement. Short ISRs are best, unless you need a long ISR. Smile You have to juggle these things to get the responsiveness you need on all fronts. There are even cases where it makes sense to have all the program code in the ISRs. It's a balancing act.
 
 View user's profile Send private message  
Reply with quote Back to top
ThEThInG
PostPosted: Mar 17, 2010 - 07:00 PM
Hangaround


Joined: Feb 24, 2009
Posts: 139


Ok, thanks for the input guys, I think I'll see what the balance works best with my application and conduct tests. I was told at one point that ISR's should ALWAYS be kept short by several people when I first start programming last year. However I agree more with, it depends upon the application as to what is most critical that determines how the code is set up.
 
 View user's profile Send private message  
Reply with quote Back to top
theusch
PostPosted: Mar 17, 2010 - 07:42 PM
10k+ Postman


Joined: Feb 19, 2001
Posts: 19089
Location: Wisconsin USA

Quote:

I was told at one point that ISR's should ALWAYS be kept short by several people when I first start programming last year.

[IMO] Continue with that tenet. Once you are experienced with apps with several unrelated interrupt streams (e.g., USART + continuous A/D conversions + timers + perhaps "edges" as you have [encoders]) it will be almost obvious when you can "push" critical pieces into the ISR and still not block e.g. a fast USART link.

Glancing at your "critical" code:
1) Are both edges really necessary?
2) Can a 16-position "action table" reduce all the conditional logic to mostly straight-line?
3) If this is the only place that you use the routine (or even if it isn't), avoid function calls especially in "fast interrupts". The call/ret overhead itself can be significant in a cycle-counted ISR. You will probably find it will be quite bloated due to saving/restoring many more registers than if inline.
Quote:

Does anybody see any reason I should not call a function from the ISR from a program stability standpoint?

4) Yes. Perhaps. Library functions may or may not be re-entrant. If you use your ISR-called routine in the mainline, then you need to be re-entrantable [is that a word?] as well.
 
 View user's profile Send private message  
Reply with quote Back to top
clawson
PostPosted: Mar 17, 2010 - 07:47 PM
10k+ Postman


Joined: Jul 18, 2005
Posts: 34545
Location: (using avr-gcc in) Finchingfield, Essex, England

If you are calling functions from an ISR then put the code of that function in the same .c as the ISR() itslef. This way the compiler can possibly inline it if it looks small enough and, at any rate, see the register usage so not go over-board with the PUSH/POPs.

But it's always good practice to look at the Asm generated from the C anyway and particularly in the case of time critical ISRs. In doing this any "slow bloat" should be immediately apparent.

Cliff

_________________
 
 View user's profile Send private message  
Reply with quote Back to top
skeeve
PostPosted: Mar 17, 2010 - 08:24 PM
Posting Freak


Joined: Oct 29, 2006
Posts: 1490


clawson wrote:
If you are calling functions from an ISR then put the code of that function in the same .c as the ISR() itslef. This way the compiler can possibly inline it if it looks small enough and, at any rate, see the register usage so not go over-board with the PUSH/POPs.
I don't know about other compilers,
but my recollection is that avr-gcc won't do that from ISRs,
not even if declared static inline.
Don't know why not.

Once upon a time there was a discussion about
calling an ISR-like function from an ISR.
'Twas noted that avr-gcc still pushed and popped registers.
'Twas also noted that a standard call would restore interrupts.
A solution was two lines of inline assembly.

_________________
Michael Hennebry
"SCSI is NOT magic. There are *fundamental technical
reasons* why it is necessary to sacrifice a young
goat to your SCSI chain now and then." -- John Woods
 
 View user's profile Send private message  
Reply with quote Back to top
clawson
PostPosted: Mar 17, 2010 - 08:52 PM
10k+ Postman


Joined: Jul 18, 2005
Posts: 34545
Location: (using avr-gcc in) Finchingfield, Essex, England

Quote:

but my recollection is that avr-gcc won't do that from ISRs,
not even if declared static inline.

Observe: main.c
Code:
#include <avr/io.h>
#include <avr/interrupt.h>

void extern_fn(void);
void local_fn(void);

ISR(INT0_vect) {
   extern_fn();
}

ISR(INT1_vect) {
   local_fn();
}

int main(void) {
   while(1) {
   }
}

void local_fn(void) {
   PORTB = 0x5A;
}

extern.c:
Code:
#include <avr/io.h>

void extern_fn(void){
   PORTB = 0xA5;
}

yields the following vectors:
Code:

00000072 <__vector_2>:

  72:   1f 92          push   r1
  74:   0f 92          push   r0
  76:   0f b6          in   r0, 0x3f   ; 63
  78:   0f 92          push   r0
  7a:   11 24          eor   r1, r1
  7c:   8f 93          push   r24


void local_fn(void) {
   PORTB = 0x5A;
  7e:   8a e5          ldi   r24, 0x5A   ; 90
  80:   88 bb          out   0x18, r24   ; 24
   extern_fn();
}

  82:   8f 91          pop   r24
  84:   0f 90          pop   r0
  86:   0f be          out   0x3f, r0   ; 63
  88:   0f 90          pop   r0
  8a:   1f 90          pop   r1
  8c:   18 95          reti

Code:
00000096 <__vector_1>:
  96:   1f 92          push   r1
  98:   0f 92          push   r0
  9a:   0f b6          in   r0, 0x3f   ; 63
  9c:   0f 92          push   r0
  9e:   11 24          eor   r1, r1
  a0:   2f 93          push   r18
  a2:   3f 93          push   r19
  a4:   4f 93          push   r20
  a6:   5f 93          push   r21
  a8:   6f 93          push   r22
  aa:   7f 93          push   r23
  ac:   8f 93          push   r24
  ae:   9f 93          push   r25
  b0:   af 93          push   r26
  b2:   bf 93          push   r27
  b4:   ef 93          push   r30
  b6:   ff 93          push   r31
   extern_fn();
  b8:   0e 94 36 00    call   0x6c   ; 0x6c <extern_fn>
}
  bc:   ff 91          pop   r31
  be:   ef 91          pop   r30
  c0:   bf 91          pop   r27
  c2:   af 91          pop   r26
  c4:   9f 91          pop   r25
  c6:   8f 91          pop   r24
  c8:   7f 91          pop   r23
  ca:   6f 91          pop   r22
  cc:   5f 91          pop   r21
  ce:   4f 91          pop   r20
  d0:   3f 91          pop   r19
  d2:   2f 91          pop   r18
  d4:   0f 90          pop   r0
  d6:   0f be          out   0x3f, r0   ; 63
  d8:   0f 90          pop   r0
  da:   1f 90          pop   r1
  dc:   18 95          reti

As I say, well worth putting the called function in the same file as the ISR()s

_________________
 
 View user's profile Send private message  
Reply with quote Back to top
skeeve
PostPosted: Mar 17, 2010 - 09:31 PM
Posting Freak


Joined: Oct 29, 2006
Posts: 1490


clawson wrote:
As I say, well worth putting the called function in the same file as the ISR()s
From the example, one cannot tell.
It shows that not having them in the same file is bad.
It does not show that having them in the same file is better.
It should be better. More information is available.
Alas, available != availed.

Edit: Oops. I somehow missed the local_fn part of the example.
Has anyone noticed how hard it is to find one's eyes without them?

_________________
Michael Hennebry
"SCSI is NOT magic. There are *fundamental technical
reasons* why it is necessary to sacrifice a young
goat to your SCSI chain now and then." -- John Woods


Last edited by skeeve on Mar 17, 2010 - 10:41 PM; edited 1 time in total
 
 View user's profile Send private message  
Reply with quote Back to top
hooverphonique
PostPosted: Mar 17, 2010 - 10:04 PM
Wannabe


Joined: Aug 29, 2008
Posts: 84
Location: Denmark

why would you push/pop inside an isr around a function call? normally a function saves/restores the registers it destroys, or am i misunderstanding something?
 
 View user's profile Send private message  
Reply with quote Back to top
clawson
PostPosted: Mar 17, 2010 - 10:12 PM
10k+ Postman


Joined: Jul 18, 2005
Posts: 34545
Location: (using avr-gcc in) Finchingfield, Essex, England

Quote:

why would you push/pop inside an isr around a function call? normally a function saves/restores the registers it destroys, or am i misunderstanding something?

The point that both Michael and yourself seemed to have missed is that the compiler has no choice. If external_fn() is not in the main.c then at the time when the compiler compiles main.c and ISR(INT0_vect) it has no possible way to know how complex external_fn() is or which registers it may trash. So it has no option but to preserve the entire set of non-trashable registers on the basis that for all it knows external_fn(), when its .c file is compiled might go on to use any number of them.

Meanwhile with local_fn(), because that is visible to the compiler when it is compiling INT1_vect, it can (a) see that the only register it uses is R24 and (b) see that it is so short that to implement a CALL/RET would be an unnecessary over-head so it inlines the code within the ISR() vector code itself.

So there's a double benefit (in this case of a short inlinable function) of including the called function in the same .c as the ISR. But even if it had been more complex and the compiler could see that it would waste flash space to both generate its body and inline it the compiler would still have the advantage of knowing at the compile time of the ISR() which registers the CALLd function would be using and only need to PUSH/POP those.

Cliff

PS I guess if the linker was MUCH smarter then at the time when it comes to link main.o and external.o it could somehow determine from external.o tat only R24 were used so it could then go back to the calling code in INT1_vect and arrange that it ONLY PUSH/POP r24 but as of today the linker isn't that smart. However there is talk of making the linker far more optimising and moving the optimisation burden from compiler to linker. If this really happened I guess the above is one way it could be improved.

_________________


Last edited by clawson on Mar 17, 2010 - 10:16 PM; edited 2 times in total
 
 View user's profile Send private message  
Reply with quote Back to top
sternst
PostPosted: Mar 17, 2010 - 10:15 PM
Resident


Joined: Jul 23, 2001
Posts: 977
Location: Osnabrueck, Germany

hooverphonique wrote:
why would you push/pop inside an isr around a function call? normally a function saves/restores the registers it destroys, or am i misunderstanding something?
There is a set of registers that can be used by a function without saving/restoring them. They are called the "call-used" or "call-clobbered" registers. The caller is responsible for saving/restoring if needed (and it is needed in an ISR).

_________________
Stefan Ernst
 
 View user's profile Send private message  
Reply with quote Back to top
hooverphonique
PostPosted: Mar 17, 2010 - 10:17 PM
Wannabe


Joined: Aug 29, 2008
Posts: 84
Location: Denmark

I understand what you're saying, Cliff, but a function normally saves/restores the registers it trashes, otherwise the compiler would need to save the registers before calling an external function outside of isr's too (and restore them after the function returns)...
 
 View user's profile Send private message  
Reply with quote Back to top
clawson
PostPosted: Mar 17, 2010 - 10:20 PM
10k+ Postman


Joined: Jul 18, 2005
Posts: 34545
Location: (using avr-gcc in) Finchingfield, Essex, England

Yes but in the interrupt context the compiler cannot possibly know where in the mainline the code was when the interrupt occurs and which registers might be active so it has to preserve all of them that the compiler might possibly be using elsewhere in mainline (unless it can see the entire ISR code and know that only a subset are actually clobbered by the ISR.)

_________________
 
 View user's profile Send private message  
Reply with quote Back to top
hooverphonique
PostPosted: Mar 17, 2010 - 10:26 PM
Wannabe


Joined: Aug 29, 2008
Posts: 84
Location: Denmark

that only makes sense if there is a range of registers that a function can use without saving them (ie the compiler expects them to be possibly trahed).. and that still means that they need to be saved in non-isr context if they are in use when calling the function, and that seems like a waste of space compared to the called function just saving the registers it trashes instead of saving the whole range..
 
 View user's profile Send private message  
Reply with quote Back to top
sternst
PostPosted: Mar 17, 2010 - 10:37 PM
Resident


Joined: Jul 23, 2001
Posts: 977
Location: Osnabrueck, Germany

hooverphonique wrote:
that only makes sense if there is a range of registers that a function can use without saving them (ie the compiler expects them to be possibly trahed)
You haven't read my post, have you?

_________________
Stefan Ernst
 
 View user's profile Send private message  
Reply with quote Back to top
skeeve
PostPosted: Mar 17, 2010 - 10:38 PM
Posting Freak


Joined: Oct 29, 2006
Posts: 1490


clawson wrote:
The point that both Michael and yourself seemed to have missed is that the compiler has no choice. If external_fn() is not in the main.c then at the time when the compiler compiles main.c and ISR(INT0_vect) it has no possible way to know how complex external_fn() is or which registers it may trash. So it has no option but to preserve the entire set of non-trashable registers on the basis that for all it knows external_fn(), when its .c file is compiled might go on to use any number of them.
What I missed was local_fn. It's been a while since the aforementioned recollection.
It's possible the compiler has improved since.
Quote:
Meanwhile with local_fn(), because that is visible to the compiler when it is compiling INT1_vect, it can (a) see that the only register it uses is R24 and (b) see that it is so short that to implement a CALL/RET would be an unnecessary over-head so it inlines the code within the ISR() vector code itself.

...

PS I guess if the linker was MUCH smarter then at the time when it comes to link main.o and external.o it could somehow determine from external.o tat only R24 were used so it could then go back to the calling code in INT1_vect and arrange that it ONLY PUSH/POP r24 but as of today the linker isn't that smart. However there is talk of making the linker far more optimising and moving the optimisation burden from compiler to linker. If this really happened I guess the above is one way it could be improved.
The linker wouldn't necessarily have to think very hard.
The compiler could tag each function with the registers
it directly trashes and the functions it calls.

_________________
Michael Hennebry
"SCSI is NOT magic. There are *fundamental technical
reasons* why it is necessary to sacrifice a young
goat to your SCSI chain now and then." -- John Woods
 
 View user's profile Send private message  
Reply with quote Back to top
Display posts from previous:     
Jump to:  
All times are GMT + 1 Hour
Post new topic   Reply to topic
View previous topic Printable version Log in to check your private messages View next topic
Powered by PNphpBB2 © 2003-2006 The PNphpBB Group
Credits