OK to sei() in SIGNAL function?

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

In one of my SIGNAL functions I need to enable interrupts for around 1 second. Is that allowed? Seems like it's not working properly...

/Jakob Selbing

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

Do You have a SIGNAL function that executes for more than one second?

If so, that is probably the thing to change. Let the SIGNAL function set a flag variable that the main program can inspect, and act upon, instead. As soon as the SIGNAL function has set the flag it terminates and leaves the field free for other interrupts to be serviced.

Interrupt service routines (ISR) should terminate as soon as possible. Think millisecond or microseconds. It all depends on the application and intensity, but if You need a 1 second "window" in a ISR that is definitely too long.

Example: You write a ISR to de-bounce a switch. When the switch closes a lengthy operation should be executed. Don't code that operation into the ISR. Let the ISR simply indicate that the switch was closed. The main program runs a loop waiting for things to occur, sees the indiction and executes the lengthy operation. When no indications are present the main program can use it's time to do non-prioritized operations (eg. updating a display).

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

JohanEkdahl wrote:
When no indications are present the main program can use it's time to do non-prioritized operations (eg. updating a display).

I agree completely.... except when I don't.

I have an application where there are 2 types of "interrupts".

The first is stuff like switch change detection, real-time-clock 1/1024 second tick incrementing, and the like. They should be as fast as possible and set flags, increment variables, but no significant operations. IN this case it makes no sense to leave interrupts enabled. Get in, do the job quick and get out. If other interrupts triggered during the interim, then service them after finishing the first. If you have too many interrupts with too much stuff in them, you may never finish servicing all the interrupts, and all foreground processing may crash to a halt.

In my application, the other type of interrupt occurs at one second intervals. It is a really high priority. Higher priority than the foreground task. I need to update a display exactly at the top of a second. Or as exact as possible. Essentially I need to do time-slicing multitasking, but the slice only occurs once a second, and it needs to precisely on the second.

So, I use an interrupt to do that high priority function, and I leave interrupts enabled so that the quick stuff can still get executed (like RTC!). This works fine. It works fine because it happens so darn infrequently.... eons as far as the processor is concerned.

I have a wait-for-input function that scans the keypad, and a rotary input switch. It also checks flags which may have been set by interrupts. It then processes those flags at the same time it is waiting for input.

It might cause some latency, but compared to human activity (button press, etc) the latency is miniscule.

The whole application hangs together really well, despite my breaking the rules a little.

BTW, now is the time to convert SIGNAL and INTERRUPT to ISR. In those cases where you want to have interrupts enabled, then you may sei() as the first line of the ISR.

-Tony

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

JohanEkdahl wrote:
Do You have a SIGNAL function that executes for more than one second?

If so, that is probably the thing to change. Let the SIGNAL function set a flag variable that the main program can inspect, and act upon, instead. As soon as the SIGNAL function has set the flag it terminates and leaves the field free for other interrupts to be serviced.

You're drawing too many conclusions on your own here. This is a very exceptional interrupts, occuring when power is disconnected.

The question I'm asking is "Is it OK to enable interrupts within a SIGNAL function?".

/Jakob Selbing

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

jaksel wrote:
The question I'm asking is "Is it OK to enable interrupts within a SIGNAL function?".

The simple answer is YES, but be careful about the execution duration of the interrupt. Also consider re-entrancy, and how to avoid it, or problems from it.

-Tony

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

Spamiam wrote:
jaksel wrote:
The question I'm asking is "Is it OK to enable interrupts within a SIGNAL function?".

The simple answer is YES, but be careful about the execution duration of the interrupt. Also consider re-entrancy, and how to avoid it, or problems from it.

-Tony

Ah, you're right. Re-entrancy must be the problem...how careless of me not to think of that...
Thanks!

/Jakob Selbing

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

You don't put a SEI inside the ISR. Well, you can, but if you want the SEI placed at the very start of the routine you need to manually assign the ISR with the "Interrupt" attribute.

Using SEI inside the routine will do the same job, but the prologue code will be executed first. That can cause you to miss fast interrupts if said interrupt conditions occurs during the prologue.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

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

abcminiuser wrote:
You don't put a SEI inside the ISR. Well, you can, but if you want the SEI placed at the very start of the routine you need to manually assign the ISR with the "Interrupt" attribute.

Using SEI inside the routine will do the same job, but the prologue code will be executed first. That can cause you to miss fast interrupts if said interrupt conditions occurs during the prologue.

- Dean :twisted:

How long is that prologue? I have not looked at the disasm output of the interrupt. How many clock cycles? THis is not any sort of criticism, simply a request for info. If it is pretty long, then I can see the need for a version that has the briefest possible period of disabled interrupts, hence the need for the "INTERRUPT" type of declaration.

Bit, I think I would start not offering "INTERRUPT" as an alternative since it is now considered bad form, and the compiler will generate warnings/notices every time INTERRUPT or SIGNAL is used.

If the prologue is so long that dropped interrupts are an issue, then GCC needs to offer an alternative to the simple expedient of using ISR with a sei() as the first statement. Is this one of those steps forward that is also a step backward too? Maybe we had 2 different types of interrupts for a good reason. It seems to me that the GCC writers do not consider it to be an issue, but maybe it is!

Maybe the GCC writers expect us to do stuff in ASM if we need the lowest latency. Personally, I do not want to do stuff in ASM. I used to do it on the 8088/80286, but I would rather not do it any more.

-Tony

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

Spamiam wrote:
If the prologue is so long that dropped interrupts are an issue, then GCC needs to offer an alternative to the simple expedient of using ISR with a sei() as the first statement. Is this one of those steps forward that is also a step backward too? Maybe we had 2 different types of interrupts for a good reason. It seems to me that the GCC writers do not consider it to be an issue, but maybe it is!

Maybe the GCC writers expect us to do stuff in ASM if we need the lowest latency. Personally, I do not want to do stuff in ASM. I used to do it on the 8088/80286, but I would rather not do it any more.


But I thought that the idea was that the new ISR() was a direct replacement for SIGNAL() and if you wanted the equivalent of the old INTERRUPT() (sei at the start of the prologue) then you just need to add:

__attribute__((interrupt))

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

Quote:
If the prologue is so long that dropped interrupts are an issue, then GCC needs to offer an alternative to the simple expedient of using ISR with a sei() as the first statement. Is this one of those steps forward that is also a step backward too? Maybe we had 2 different types of interrupts for a good reason. It seems to me that the GCC writers do not consider it to be an issue, but maybe it is!

Many things are steps both forwards and backwards. As I have understood it there was massive confusion about INTERRUPT and signal. Eg. that in the vast majority of cases You'd want the SIGNAL version, ut as a newbie all You knew was that You wanted to serve an interrupt and erroneously chose the INTERRUPT version.

All things that could be done with the previous version of avr-libc are still possible to do, and the overwhelmingly most common case is supplied via the ISR macro. Other variants will have to be made "by hand" or through a macro You write Yourself or steal from the previous version of avr-libc!

The produced prologue can be examined by looking at the list file (the .LSS file).

As of January 15, 2018, Site fix-up work has begun! Now do your part and report any bugs or deficiencies here

No guarantees, but if we don't report problems they won't get much of  a chance to be fixed! Details/discussions at link given just above.

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

If the "Mcallprologues" switch is speicied to the compiler, it will generate one single prologue and one single epilogue code which will push and pop every register plus a few of the systems registers. These sile pro and epilgues will be call from every ISR. That saves space in the long run, but it takes more time to push and pop all those registers that your ISR probably isn't changing.

Without that switch, each ISR gets its own condensed version which only pushes and pops the registers used by that particular ISR. This is faster but with a lot of ISRs will waste code space. It's a tradeoff.

In the new AVRLibC, you use the ISR macro as you know. If you specify that you want it to be interruptable, it requires you to get a little messier:

void xxx_vect(void) __attribute__((interrupt));
ISR(xxx_vect)
{
//...
}

Where "xxx" is the signal name. For example, to make an ISR for the pin change 1 interrupt on the Butterfly that is itself interruptable:

void PCINT1_vect(void) __attribute__((interrupt));
ISR(PCINT1_vect)
{
//...
}

Looks a little messier but it works.

Also, you can define a common interrupt which is called if an ISR is executed with no ISR code. This means you can handle bad interrupts without the AVR mysteriously resetting to 0:

ISR(__vector_default)
{
//...
}

In my badisr routine, it sends a message to the butterfly's LCD and then enters an infinite loop. Because of that, I don't care about clobbering all the registers, and thus don't need a prologue or epilogue for it. I use the "naked" attribute to specify that no prologue or epilogue should be added:

void __vector_default(void) __attribute__((naked));
ISR(__vector_default)      // Bad ISR routine; should never be called, here for safety
{
	MAIN_ShowError(PSTR("BADISR"));
	while (1) {};
}

Hope that helps.
- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

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

Please note the definition of ISR() from avr-libc 1.4.x:

#define ISR( vector ) \
void vector (void) __attribute__ ((signal)); \
void vector (void)

And contrast this to the definitions of SIGANL() and INTERRUPT() in avr-libc 1.2.x:

#define SIGNAL( vector ) \
void vector (void) __attribute__((signal)); \
void vector (void)

#define INTERRUPT( vector ) \
void vector (void) __attribute__((interrupt)); \
void vector (void)

Now, Dean's suggestion for making an interrupt handler which is interruptible in avr-libc 1.4.x is this:

void xxx_vect (void) __attribute__((interrupt));
ISR(xxx_vect)
{
  ...
}

Which would expand to:

void xxx_vect (void) __attribute__((interrupt));
void xxx_vect (void) __attribute__((signal));
void xxx_vect (void)
{
...
}

I'd personally be wary of such a double-declaration... I guess the compiler would automagically determine that the first attribute was the "right" one, and the function xxx_vect would be declared as an interruptible interrupt. But I don't like counting on things as ambiguous as that.

According to the doc's, you shouldn't need the ISR() part following the __attribute__((interrupt)) declaration for an interruptible interrupt... just skip straight on to the void function definition:

void xxx_vect (void) __attribute__((interrupt));
void xxx_vect (void)
{
...
}
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ahh nuts. I'm making my own custom ISR implementation:

#define NOBLOCK __attribute__((interrupt))
#define BLOCK   __attribute__((signal))

#undef ISR
#define ISR(vector, blockotherisrs) \
void vector (void) blockotherisrs; \
void vector (void)

The purists will yell at at me, but it should work in Buttload. It should clean up the ambiguity and mess and will remain constant independant of AVRLibC version.

By the way, my method (using two prototypes) works - I checked it. However you're right it saying it's not the best way.

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!

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

abcminiuser wrote:
The purists will yell at at me...

I think it's a great idea.

It appears to be effective in leaving no space for ambiguity about whether an ISR is truly blocking or not, whilst maintaining total convenience for the user who really does want to have an interruptible interrupt. And it could easily be expanded to include the possibility for a naked ISR as well.

IMO, it's just too bad that it's too late to consider contributing it to the codebase for mainstream avr-libc now that the new ISR() API has already been decided.

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

I've suggested it before, and I got a cold response from users here. Not unfriendly, just uninterested. I think it's nicer in that it's clear what is it and what it's doing. I have some ideas on how to extend it a bit further, which i'll work on. I was just miffed at the deprication of the INTERRUPT macro since it has no replacement.

EDIT: Nope, my other ideas are baloney. Oh well. Here's what I added to ButtLoad:

/*
             BUTTLOAD - Butterfly ISP Programmer
				
              Copyright (C) Dean Camera, 2006.
                  dean_camera@hotmail.com
*/

/* Must be included after avr/interrupt.h. This file redefines the new
   ISR macro to extend it to allow custom attributes. When the old ISR
   macros SIGNAL and INTERRUPT were depricated, no suitable replacement
   was specifed for interruptable ISR routine (and no macro at all exists
   for naked ISRs). This file avoids the clumsiness of declaring the ISR
   routines manually with custom attributes and thus gives code uniformity. */

#ifndef ISRMACRO_H
#define ISRMACRO_H

// If present, kill the current ISR macro:
#ifdef ISR
   #undef ISR
#endif

// The three types of ISRs are specified here:
#define ISR_NOBLOCK __attribute__((interrupt))
#define ISR_BLOCK   __attribute__((signal))
#define ISR_NAKED   __attribute__((naked))

// Define the new ISR macro:
#define ISR(vector, attributes)  \
 void vector (void) attributes; \
 void vector (void)

#endif

- Dean :twisted:

Make Atmel Studio better with my free extensions. Open source and feedback welcome!