volatile question in ISR routin

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

HI!

I have a question. In my Timer ISR routine I have a function. I give a parameter to this function, and it also returns one parameter. Do I need to declare these parameters as volatile?

volatile uint8_t bt_state;

ISR (TIMER1_COMPA_vect)
{
  bt_state = Read_Button(BUTTON1);
}

uint8_t Read_Button(uint8_t ButtonNO)
{
  if (PINE & ButtonNO)
  {
    return 0;	// The button is in RELEASED state
  }
  else 
  {
    return 1;	// The button is in PRESSED state
  }
}

So do I need to write this:
uint8_t Read_Button(uint8_t ButtonNO)

or this:
volatile uint8_t Read_Button(volatile uint8_t ButtonNO)

I know that the global variables used in ISR like bt_state must be declared volatile.

Thanks for the answer.

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

Quote:

Do I need to declare these parameters as volatile?

No. "volatile" means "this thing could change at any time (because of two side by side paths of execution) so make no assumption about what may be in it and always generate code to read/write it however inefficient that may be". That is not the case of variables "private" to the ISR() alone.

BTW you do realise that it is HUGELY inefficient to call a function from an ISR? At the very least make that function "static inline".

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

Quote:
Do I need to declare these parameters as volatile?
No. Volatile is for variables that are shared both inside and outside an ISR. With your example the function is called within the ISR, and therefore anything that happens inside the function is still in the ISR. Furthermore, the variable ButtonNO only exists in the function, not the ISR itself. You are only passing in a value to that parameter.

However, you should be careful of calling a function within an ISR. This can end up being very inefficient since it may force the compiler to push/pop numerous registers.

Regards,
Steve A.

The Board helps those that help themselves.

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

Your original code looks fine to me.

However I would change the names. e.g. ButtonMask

If you want to use bit-positions then you would have:

  if (PINE & (1 << ButtonNO)) 

Note that you normally have 0 for PRESSED. i.e. your buttons are active-low

David.

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

Quote:

Your original code looks fine to me.

If "fine" means "will operate as written" then I cannot disagree.

However, I have reservations:

-- Calling a function from an ISR probably doesn't leave the ISR "skinny". There could well be much register saving and restoring. An ISR doing a few microsecond job could end up many times that.

-- The implication is a button-reading job. It is indeed hooked to a timer interrupt, so I hope that OP is using this as a periodic read of input state that will be entered into a debounce routine.

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

clawson wrote:

No. "volatile" means "this thing could change at any time (because of two side by side paths of execution) so make no assumption about what may be in it and always generate code to read/write it however inefficient that may be". That is not the case of variables "private" to the ISR() alone.

BTW you do realise that it is HUGELY inefficient to call a function from an ISR? At the very least make that function "static inline".

Why would it create more inefficient code, than a function call from elsewhere?
How do I make it static inline? Just by writing:

static inline uint8_t Read_Button(uint8_t ButtonNO)
{
  if (PINE & ButtonNO)
  {
    return 0;   // The button is in RELEASED state
  }
  else
  {
    return 1;   // The button is in PRESSED state
  }
} 

And then the compiler will copy the inline function code to every location where it is called?

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

Quote:

Why would it create more inefficient code, than a function call from elsewhere?

Because ISR()s "hit" right in the middle of something else. So they cannot possibly corrupt anything that may be in use in the main() function that has been interrupted and everything must be put back as it was when the interrupt handler ends. If you just put that PINE reading code into the ISR() directly the chances are it probably won't use anything but R24 so the ISR will have an additional PUSH R24/POP R24 added to the start/end. If, however the ISR() calls Read_button() it could be a function in another file that you compiled last Thursday so the compiler cannot necessarily know how complex the function is or what registers it may be using. So it has no option but to assume that it will be the worst case and therefore saves/restores everything that might be "damaged" while away handling the CALL. This adds something like 12 PUSHs and 12 POPS to the start/end of the ISR handler.

In my book that is "inefficient code". Study the .lss to see what I mean.

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

Quote:

if (PINE & ButtonNO)

Which pin are you looking at, and how have you defined "ButtonNO"? (as mentioned earlier)

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

I suspect that avr-gcc would inline the 'function' whether you ask it or not.
After all, the ISR() and function are both in the same compilation unit.

However, I quite agree with all of you. Why make things complex in the first place?

Avoid function calls in an ISR().

David.

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

Quote:

I suspect that avr-gcc would inline the 'function' whether you ask it or not.

I tend to agree but that's just because it's a "small function". It's quite difficult to predict where GCC will set the boundary as to whether it inlines or not. Interesting to read the manual too:
http://gcc.gnu.org/onlinedocs/gnat_ugn_unw/Switches-for-gcc.html wrote:
-fno-inline
Suppresses all inlining, even if other optimization or inlining switches are set. This includes suppression of inlining that results from the use of the pragma Inline_Always. Any occurrences of pragma Inline or Inline_Always are ignored, and -gnatn and -gnatN have no effect if this switch is present.

-fno-inline-functions
Suppresses automatic inlining of subprograms, which is enabled if -O3 is used.

-fno-inline-small-functions
Suppresses automatic inlining of small subprograms, which is enabled if -O2 is used.

-fno-inline-functions-called-once
Suppresses inlining of subprograms local to the unit and called once from within it, which is enabled if -O1 is used.


So it's also going to depend on the optimisation level and sadly that manual does not seem too forthcoming on what "small subprograms" are anyway.

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

GCC is open source, so a quick grep on the sources should reveal what that sub program limit is.

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

Thank you very much for the advices, I will move my code from ISR to main(), and I only set a flag variable (volatile) in ISR, and then check it in the main.
I think this is what most people do. Or is there a better idea?

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

Just setting a flag, and just that, is IMO a bit pointless. Just check the interrupt flag in the main() code directly.

At least do something in the ISR that cannot wait, like grabbing a character from the UART, or a timestamp from the input capture unit, set an output or whatever.

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

jayjay1974 wrote:
GCC is open source, so a quick grep on the sources should reveal what that sub program limit is.
The question is not where the limit is; you can control it by, say
    -finline-limit=value --param max-inline-insns-single=value
    --param max-inline-insns-auto=value
    --param large-function-insns=value
    --param large-function-growth=value
    --param inline-unit-growth=value
    --param max-inline-insns-recursive=value
    --param max-inline-insns-recursive-auto=value
    --param max-inline-recursive-depth=value
    --param max-inline-recursive-depth-auto=value
    --param min-inline-recursive-probability=value
    --param early-inlining-insns=value
    --param max-early-inliner-iterations=value
The question: What the unit of that limit?

Inlining happens early. In makes no sense to perform inlining late in the compilation precess.

The inlines code shall benefit from constant propagation, redundancy elimination, dead code elimination, common regsiter allocation, scheduling, bransh shortening, etc.

You'll know the costs of the code only after these optimizations are performed. GCC does not wirk speculative; that would increase compilation times in an unacceptable way.

The farther you are away from writing assembler, the hard it will be to estimate the costs of the final code. There's only some intermediate representation (IR) like SSA trees or RTL. The "insns" mentioned above are of the latter type.

However, inlining runs even earier, namely at tree-SSA or prior to that at gimple stage and will have to guess into how much insns the code will transform to in order to estimate the costs — and insn costs are already very fuzzy.

If you are interested in GCC's code transformations, you can let the compiler dump the trees with -fdump-tree-all.

That generates easy to grasp C-like dump files where you can track how GCC crunches your code.

avrfreaks does not support Opera. Profile inactive.