| Author |
Message |
|
|
Posted: Mar 17, 2010 - 05:59 PM |
|

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
|
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 06:06 PM |
|

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. |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 06:08 PM |
|

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?
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 06:16 PM |
|

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. |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 06:25 PM |
|


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
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 06:37 PM |
|

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. 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. |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 07:00 PM |
|

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. |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 07:42 PM |
|


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. |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 07:47 PM |
|


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 |
_________________
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 08:24 PM |
|

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
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 08:52 PM |
|


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 |
_________________
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 09:31 PM |
|

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
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:04 PM |
|

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? |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:12 PM |
|


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
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:15 PM |
|

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
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:17 PM |
|

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)... |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:20 PM |
|


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.) |
_________________
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:26 PM |
|

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.. |
|
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:37 PM |
|

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
|
| |
|
|
|
|
|
Posted: Mar 17, 2010 - 10:38 PM |
|

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