Function Call from ISR?

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

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.

#include 
#include 
#include 
#include 

//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
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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.

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

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.

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

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.

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

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.

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

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. :) 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.

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

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.

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

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.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

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

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

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.

Iluvatar is the better part of Valar.

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

Quote:

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

Observe: main.c

#include 
#include 

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:

#include 

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

yields the following vectors:

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
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 
}
  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

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

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?

Iluvatar is the better part of Valar.

Last Edited: Wed. Mar 17, 2010 - 10:41 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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?

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

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: Wed. Mar 17, 2010 - 10:16 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

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

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

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)...

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

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.)

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

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..

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

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

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

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.

Iluvatar is the better part of Valar.

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

hooverphonique wrote:
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..
Why "whole range"? In the non-isr context the caller knows which of these registers are in use and need to be preserved over the call. The called function doesn't know this. So always saving/restoring these registers in the called function would be a waste of space and time.

Stefan Ernst

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

sternst wrote:
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?

No, I hadn't, sorry.. I clicked directly on the "last post" icon and it was Cliff's post.. but it all makes sense now, even though I still think that having a considerable amount of regs that a function doesn't need to save seems like a cause of bloat.. anyway, that's a completely different discussion :)

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

Quote:

seems like a cause of bloat..

Yes but the whole point of my first post on this subject that with a little help you can help the compiler avoid generating such bloat and that the wise programmer will be aware of this and what's going on behind the scenes.

Too often we see threads here where someone is trying to do something very time constrained in an ISR and cannot get their brain around where 20-30 cycles have suddenly disappeared to. If they check the generated Asm the wasted cycles become obvious and if they then realise that either (a) avoid calls in ISRs or (b) at least help the compiler if calls will be made will reduce the cycle bloat they'd be much happier bunnies.

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

Yes, of course.. and this is all important info.. I've often looked at the generated assembly and wondered why the compiler did what it did, generating unnecessary code, etc...

I guess this, unfortunately, means that using an external handler function for taking care of an isr where the specific interrupt vector is unknown by the handler is a bad idea (i.e. the handler is generic in the sense that it knows how to handle e.g. a counter overflow, but it doesn't know which specific counter is used, but that can be fixed by using specific inlining).

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

Quote:

I guess this, unfortunately, means that using an external handler function for taking care of an isr where the specific interrupt vector is unknown by the handler is a bad idea

You mean calling through a variable function pointer?

In that case, yes, the compiler doesn't have a hope at compile time of know what the called routine might be so is bound to generate the littany of PUSH/POPs

(it's just vaguely possible that if everything were in the one .c file that maybe the compiler could see the complete picture and avoid some waste/bloat - but I doubt it - you'd need to run some experiments and examine the .lss)

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

no, no function pointer, but a handler function in another compilation unit.. but I guess the result would be the same..

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

hooverphonique wrote:
Yes, of course.. and this is all important info.. I've often looked at the generated assembly and wondered why the compiler did what it did, generating unnecessary code, etc...

The thing is, it's not unnecessary. It's actually VERY necessary for the program to work correctly.

I have too many hobbies.
s-conductor.com

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

mhatter wrote:
hooverphonique wrote:
Yes, of course.. and this is all important info.. I've often looked at the generated assembly and wondered why the compiler did what it did, generating unnecessary code, etc...

The thing is, it's not unnecessary. It's actually VERY necessary for the program to work correctly.

Maybe i wasn't clear on what I was talking about there.. It was not meant in the context of saving/restoring registers :)

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

clawson wrote:
Quote:

I guess this, unfortunately, means that using an external handler function for taking care of an isr where the specific interrupt vector is unknown by the handler is a bad idea

You mean calling through a variable function pointer?

In that case, yes, the compiler doesn't have a hope at compile time of know what the called routine might be so is bound to generate the littany of PUSH/POPs

The compiler could be certain if it looked at the attributes of an ISR-like function.
It wouldn't have to save any registers,
though it might want to put a CLI after the call.
Quote:
(it's just vaguely possible that if everything were in the one .c file that maybe the compiler could see the complete picture and avoid some waste/bloat - but I doubt it - you'd need to run some experiments and examine the .lss)

Iluvatar is the better part of Valar.