Not another 'appears to be a misspelled signal name' thread!

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

I suppose I should prepare myself for the onslaught of "Nested interrupts are bad!" responses. I know. I don't use them carelessly. I rarely use them, in fact. ATM I'm investigating their appropriateness for an upcoming project.

I have a number of interrupt sources I must service. All of the ISRs are pretty slim and trim. They are all triggered on external events. Most of them come relatively infrequently.

There is one in particular, however, which can come pretty fast and furious. As often as every 4 us (I'll be running at 16 or 20 MHz, so that's 64 to 80 cycles). It is also the most sensitive to latency. To ensure low latency, I'm looking at making the other ISRs non-blocking. Fortunately, this interrupt is also lowest in the vector table and has the highest priority. This would ensure that its latency is never much more than about two to three times it's own interrupt response time of 7-10 cycles.

Under normal system operation, this won't cause any problems. Each ISR is more than fast enough to handle it's specific task. Even if every non-blocking ISR were to be prempted by another ISR, they would all have enough time to complete their tasks before their next interrupt, even if the high-priority blocking ISR with the low-latency needs were to come as fast as it can.

As we all know, a serious danger of using non-blocking ISRs is the risk that an ISR might preempt itself. I could write each non-blocking ISR to account for this reentrancy, but I don't want to and even if I did it wouldn't protect against an eventual stack overflow.

Under normal conditions, this app could never experience the conditions which would lead to either reentrancy or a stack overflow. However, there are some extraordinary conditions resulting from potential hardware issues which could cause one or more interrupts (including the one with the non-blocking ISR) to fire so rapidly as to starve main. With one or more regularly occurring interrupts serviced by non-blocking ISRs in the mix, if these conditions were to continue for more than a short time a stack overflow would occur and the app would certainly crash and burn.

Both problems (reentrancy of non-blocking ISRs and stack overflow) can be addressed by properly handling the non-blocking aspect of each non-blocking ISR.

The ISR attribute 'ISR_NOBLOCK' simply inserts an sei at the very beginning of the ISR, even before the prologue. This of course re-enables interrupts globally. In principle, it would be better to re-enable all (currently enabled) interrupts except the one serviced by this ISR. This would prevent reentrancy and any possibility of stack overflow. The worst case would be that every ISR made non-blocking in this way could still be pre-empted, but never by itself.

There is no attribute or other mechanism in AVR GCC or AVR Libc to achieve this. The procedure for enabling and disabling a given interrupt may be unique to that interrupt. Different flags in different I/O registers, some reachable by SBI/CBI, some requiring a RMW and the use of a register.

My thought was to do this myself in the app. I would write a short, naked ISR which calls a function which is itself declared with the signal or interrupt attribute. A trivial example:

#include 
#include 

void __attribute__ ((__signal__)) foo(void) {
  GPIOR0 = TCNT0;
}

ISR (INT1_vect, ISR_NAKED) {
  EIMSK &= ~(1<<INT1);
  sei();
  foo();
  cli();
  EIMSK |= (1<<INT1);
  reti();
}

ISR (INT0_vect) {
  TCNT0 = 0;
}

int main(void) {
  EIMSK = (1<<INT1) | (1<<INT0);
  sei();
  while(1);
}
00000080 :
#include 
#include 

void __attribute__ ((__signal__)) foo(void) {
  80:	1f 92       	push	r1
  82:	0f 92       	push	r0
  84:	0f b6       	in	r0, 0x3f	; 63
  86:	0f 92       	push	r0
  88:	11 24       	eor	r1, r1
  8a:	8f 93       	push	r24
  GPIOR0 = TCNT0;
  8c:	86 b5       	in	r24, 0x26	; 38
  8e:	8e bb       	out	0x1e, r24	; 30
}
  90:	8f 91       	pop	r24
  92:	0f 90       	pop	r0
  94:	0f be       	out	0x3f, r0	; 63
  96:	0f 90       	pop	r0
  98:	1f 90       	pop	r1
  9a:	18 95       	reti

0000009c <__vector_2>:

ISR (INT1_vect, ISR_NAKED) {
  EIMSK &= ~(1<<INT1);
  9c:	e9 98       	cbi	0x1d, 1	; 29
  sei();
  9e:	78 94       	sei
  foo();
  a0:	0e 94 40 00 	call	0x80	; 0x80 
  cli();
  a4:	f8 94       	cli
  EIMSK |= (1<<INT1);
  a6:	e9 9a       	sbi	0x1d, 1	; 29
  reti();
  a8:	18 95       	reti

000000aa <__vector_1>:
}

ISR (INT0_vect) {
  aa:	1f 92       	push	r1
  ac:	0f 92       	push	r0
  ae:	0f b6       	in	r0, 0x3f	; 63
  b0:	0f 92       	push	r0
  b2:	11 24       	eor	r1, r1
  TCNT0 = 0;
  b4:	16 bc       	out	0x26, r1	; 38
}
  b6:	0f 90       	pop	r0
  b8:	0f be       	out	0x3f, r0	; 63
  ba:	0f 90       	pop	r0
  bc:	1f 90       	pop	r1
  be:	18 95       	reti

000000c0 
: int main(void) { EIMSK = (1<<INT1) | (1<<INT0); c0: 83 e0 ldi r24, 0x03 ; 3 c2: 8d bb out 0x1d, r24 ; 29 sei(); c4: 78 94 sei c6: ff cf rjmp .-2 ; 0xc6

This works well. It does add about 14 cycles to INT1_vect, up to 7 of which are added into the worst-case latency of any pre-empting ISR. Added to the 7-to-10 cycle interrupt response of INT1_vect to begin with, and a pre-empting ISR will see at most an additional 17 cycle latency above it's own inherent response time.

The main risk is the use of ISR_NAKED. In this case the reason it isn't a problem is because EIMSK &= ~(1<<INT1); compiles to a single cbi which has no side-effects. If there were side effects, the naked ISR would need to be written in assembler (or inline assembler). Indeed, this is the only recommended approach for any naked ISR.

Another trivial example:

#include 
#include 

void __attribute__ ((__signal__)) foo(void) {
  GPIOR0 = TCNT0;
}

ISR (PCINT1_vect, ISR_NAKED) {
  uint8_t dummy;
  __asm__ __volatile__ (
                        "push   %[reg]                    \n\t"
                        "in     %[reg], __SREG__          \n\t"
                        "push   %[reg]                    \n\t"
                        "lds    %[reg], %[pcicr]          \n\t"
                        "cbr    %[reg], %[pcie1]          \n\t"
                        "sts    %[pcicr], %[reg]          \n\t"
                        "sei                              \n\t"
                        "call   foo                       \n\t"
                        "cli                              \n\t"
                        "sbr    %[reg], %[pcie1]          \n\t"
                        "sts    %[pcicr], %[reg]          \n\t"
                        "pop    %[reg]                    \n\t"
                        "out    __SREG__, %[reg]          \n\t"
                        "pop    %[reg]                    \n\t"
                        "reti                             \n\t"
                      :
                        [reg]  "=d" (dummy)
                      :
                        [pcicr] "M" (_SFR_MEM_ADDR(PCICR)),
                        [pcie1] "I" (PCIE1)
                       );
                        
}

ISR (PCINT0_vect) {
  TCNT0 = 0;
}

int main(void) {
  PCMSK0 = (1<<PCINT0);
  PCMSK1 = (1<<PCINT8);
  PCICR = (1<<PCIE1) | (1<<PCIE0);
  sei();
  while(1);
}
00000080 :
#include 
#include 

void __attribute__ ((__signal__)) foo(void) {
  80:	1f 92       	push	r1
  82:	0f 92       	push	r0
  84:	0f b6       	in	r0, 0x3f	; 63
  86:	0f 92       	push	r0
  88:	11 24       	eor	r1, r1
  8a:	8f 93       	push	r24
  GPIOR0 = TCNT0;
  8c:	86 b5       	in	r24, 0x26	; 38
  8e:	8e bb       	out	0x1e, r24	; 30
}
  90:	8f 91       	pop	r24
  92:	0f 90       	pop	r0
  94:	0f be       	out	0x3f, r0	; 63
  96:	0f 90       	pop	r0
  98:	1f 90       	pop	r1
  9a:	18 95       	reti

0000009c <__vector_4>:

ISR (PCINT1_vect, ISR_NAKED) {
  uint8_t dummy;
  __asm__ __volatile__ (
  9c:	8f 93       	push	r24
  9e:	8f b7       	in	r24, 0x3f	; 63
  a0:	8f 93       	push	r24
  a2:	80 91 68 00 	lds	r24, 0x0068
  a6:	8e 7f       	andi	r24, 0xFE	; 254
  a8:	80 93 68 00 	sts	0x0068, r24
  ac:	78 94       	sei
  ae:	0e 94 40 00 	call	0x80	; 0x80 
  b2:	f8 94       	cli
  b4:	81 60       	ori	r24, 0x01	; 1
  b6:	80 93 68 00 	sts	0x0068, r24
  ba:	8f 91       	pop	r24
  bc:	8f bf       	out	0x3f, r24	; 63
  be:	8f 91       	pop	r24
  c0:	18 95       	reti

000000c2 <__vector_3>:
                        [pcie1] "I" (PCIE1)
                       );
                        
}

ISR (PCINT0_vect) {
  c2:	1f 92       	push	r1
  c4:	0f 92       	push	r0
  c6:	0f b6       	in	r0, 0x3f	; 63
  c8:	0f 92       	push	r0
  ca:	11 24       	eor	r1, r1
  TCNT0 = 0;
  cc:	16 bc       	out	0x26, r1	; 38
}
  ce:	0f 90       	pop	r0
  d0:	0f be       	out	0x3f, r0	; 63
  d2:	0f 90       	pop	r0
  d4:	1f 90       	pop	r1
  d6:	18 95       	reti

000000d8 
: int main(void) { PCMSK0 = (1<<PCINT0); d8: 81 e0 ldi r24, 0x01 ; 1 da: 80 93 6b 00 sts 0x006B, r24 PCMSK1 = (1<<PCINT8); de: 80 93 6c 00 sts 0x006C, r24 PCICR = (1<<PCIE1) | (1<<PCIE0); e2: 83 e0 ldi r24, 0x03 ; 3 e4: 80 93 68 00 sts 0x0068, r24 sei(); e8: 78 94 sei ea: ff cf rjmp .-2 ; 0xea

This adds about 28 cycles to PCINT1_vect, up to 15 of which are added into the worst-case latency of any pre-empting ISR. This gives a worst-case added latency to the pre-empting ISR of 22-25 cycles.

This is not an insignificant added latency, but the alternative is to use normal blocking ISRs everywhere. In my case, one of these ISRs has a worst-case turn-around time of about 168 cycles. Were the time critical interrupt to occur shortly after this ISR began, that would be the added latency. That's much worse than 25 cycles.

There is some redundant code as a result of the signal attribute applied against foo(). Registers are push/popped in foo() which are already handled by the naked ISR. A 100% assembler ISR could address this.

I also realise that I could implement these 'candidates' for non-blocking interrupts as a FSM within a tightly polled loop in main, rather than as interrupts at all. For the moment, I'd like to continue investigating implementing them as interrupts. So putting aside the notion of whether or not this is the best approach, or even a good idea, I have a few questions.

One minor problem with the above 'safer' non-blocking ISR is a warning issued during compilation:

nonblocking_isr_test.c: In function 'foo':
nonblocking_isr_test.c:4:35: warning: 'foo' appears to be a misspelled signal handler [enabled by default]
 void __attribute__ ((__signal__)) foo(void) {
                                   ^

When building with -Werror (as I generally do) this becomes a hard error:

nonblocking_isr_test.c: In function 'foo':
nonblocking_isr_test.c:4:35: error: 'foo' appears to be a misspelled signal handler [-Werror]
 void __attribute__ ((__signal__)) foo(void) {
                                   ^
cc1: all warnings being treated as errors

A grep of the AVR GCC source located the code responsible for issuing the error in avr.c:

/* Implement `TARGET_SET_CURRENT_FUNCTION'.  */
/* Sanity cheching for above function attributes.  */

static void
avr_set_current_function (tree decl)
{
.
.
.
      /* If the function has the 'signal' or 'interrupt' attribute, ensure
         that the name of the function is "__vector_NN" so as to catch
         when the user misspells the vector name.  */

      if (!STR_PREFIX_P (name, "__vector"))
        warning_at (loc, 0, "%qs appears to be a misspelled %s handler",
                    name, isr);
.
.
.
}

So if a function is declared with attribute signal (or interrupt) it must have a name that begins with __vector otherwise a warning is issued. This seems hard-coded, and there appears not to be a -Wno-foo=bar option to defeat it in the presence of -Werror.

A workaround is to prefix the name of the called function which implements the body of the ISR with __vector, but that intrudes into the reserved namespace. That doesn't give me a warm fuzzy feeling.

So the questions are:

  1. Is there a way to avoid this warning without intruding into the reserved namespace?
  2. Is there another approach to achieve the goal of a 'safer' non-blocking ISR in a manner similar to this one i.e. selectively disabling the interrupt source associated with the ISR in question?
  3. Is there an entirely different approach? Please don't respond with "Nested interrupts are bad!". There will be no love between us ;)
  4. Please exclude from your responses any suggestions that the non-blocking ISRs be replaced with a polling loop. I already know ;)
Thanks for your time!

Cheers,
JJ

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Thu. Jun 12, 2014 - 12:35 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You need __vect* to avoid the warning.
The ISRs are named __vect*[0-9] .
Select a name from the difference, possibly __vect_foo .

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles

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

no clue on 1

on 2 why not disable the specific interrupt that you are handling, at the beginning of the ISR and re-enable it at the end? then after disabling all you need to do is add a sei() yourself. only attention is to be taken when re-enabling the interrupts as at that moment it can fire and still cause problems.

on 3:
what you could do is add a counter. First thing you do is check if the counter is not beyond a certain level. if it has, then just exit the ISR. if not then increment the counter re-enable interrupts and do the things you need to do. last thing before exiting the interrupt then is disable interrupt decrement the counter and return from the interrupt.

4 might be an alternative

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

I would agree with meslomp. i.e. in the 'slow' ISR(), read current state of interrupt masks. Disable all but your critical ISR(). SEI(). At the end of the 'slow' ISR() restore the masks.

In practice, most ISR()s complete in < 100 cycles. So they can work normally. This will only cost you extra cycles in the 'slow' ISR(). Adding latency to your critical ISR().

Your regular latency is (preamble + body + postamble) of the slowest ISR().
With approach(2), the latency is (preamble + disable_other_isrs).

Or you could change to an Xmega.

David.

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

Sorry can't help with the software but one has to wonder, why not just use XMega or something other with prioritized interrupts? Xmega has 3 levels so your problem is "fixed" in pretty much hardware already.

Rain

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

What would be the proper response to the hardware issues leading to stack overflow? You can maybe examine the SP in one or more of the interrupts and when it gets too deep do whatever is necessary to unwind.

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

I had a similar problem, one fast and high priority interrupt and all others with low priority.

Since the AVR support no priority level, only the high priority interrupt was handled as real interrupt.
To handle all other interrupts, I use a timer interrupt, which can be defined as NOBLOCK without interrupting itself.
The timer interval must only be longer, than the execution time of the fast interrupt.

And this timer interrupt poll all the low priority interrupts:


ISR( TIMER0_OVF_vect, ISR_NOBLOCK ) // interruptable by higher interrupts
{
TCCR0 = 0; // stop T0

if( TIFR & 1<<OCF2 )
handle_T2(); // T2 compare interrupt

if( !(ADCSRA & 1<<ADSC) ) // ADC interrupt
handle_adc();

if( TWCR & 1<<TWINT )
handle_i2c(); // I2C interrupt

TCCR0 = 1<<CS00; // restart T0
}

Peter

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

My, but you're all up and 'freaking' early this morning!

skeeve wrote:
You need __vect* to avoid the warning.
Yes, but it's the intrusion into the reserved namespace which bothers me. I suppose that's better than a warning or error...

meslomp wrote:
on 2 why not disable the specific interrupt that you are handling, at the beginning of the ISR and re-enable it at the end? then after disabling all you need to do is add a sei() yourself. only attention is to be taken when re-enabling the interrupts as at that moment it can fire and still cause problems.
Like this?:
ISR(INT1_vect) {
  EIMSK &= ~(1<<INT1);
  sei();
  ...
  cli();
  EIMSK &= ~(1<<INT1);
}

The trouble there is the compiler generates a prologue for the entire ISR and places it ahead of the first coded line. A larger ISR will have a larger prologue. This means that interrupts are disabled for much longer, adding to the worst-case latency of the other critical ISR. My thought was to use a naked ISR to perform the above as early in the ISR as possible, before the compiler-generated prologue. That's what the code in my OP does.

In my project, one of the candidates for ISR_NOBLOCK is an ISR with a fairly lengthy prologue. The main reason is a 32=16x16 unsigned multiply. In AVR GCC that's not a built-in operation. It requires a call to __umulhisi3. This forces the compiler to push the entire call-used set of registers. In truth __umulhisi3 uses 8 of those registers, and the ISR uses 2 more, so there are actually only 2 registers pushed which need not be, but the end result is a prologue which is 34 cycles long. Plus the interrupt response time, plus the time to disable the interrupt source and re-enable global interrupts, plus the one guaranteed instruction, and total worst-case latency for the critical ISR grows by 42 cycles. With a naked ISR and a 'signalised' function, I can get that down to 13.

meslomp wrote:
on 3:
what you could do is add a counter. First thing you do is check if the counter is not beyond a certain level. if it has, then just exit the ISR. if not then increment the counter re-enable interrupts and do the things you need to do. last thing before exiting the interrupt then is disable interrupt decrement the counter and return from the interrupt.
A good means of combating stack-overflow, but it doesn't address the root problem of reducing time spent with interrupts disabled, since these checks won't occur until after a possibly lengthy prologue. I could do it in a naked ISR as above, but the check would cost as much as the RMW version of the naked ISR in my OP. It also presumes the ISR is reentrant, unless the count is limited to '1'.

meslom wrote:
4 might be an alternative
Yes ;)

david.prentice wrote:
I would agree with meslomp. i.e. in the 'slow' ISR(), read current state of interrupt masks. Disable all but your critical ISR(). SEI(). At the end of the 'slow' ISR() restore the masks.

In practice, most ISR()s complete in < 100 cycles. So they can work normally. This will only cost you extra cycles in the 'slow' ISR(). Adding latency to your critical ISR().

I don't think he was saying that, but it is an interesting idea. As you say though it will add latency for the critical ISR, more so than just disabling the one interrupt source.

Other problems with this approach are maintainability. Should the design change and add an interrupt source, every ISR employing this technique would need to be changed as well. A macro (or inlined function) could make this more mantainable, but even so the more interrupt source that need disabling the greater the added latency to the critical ISR.

None of the other ISRs would suffer by this approach, it's just faster for a single ISR to disable 1 interrupt source (its own) than several. That speed translates into a lower latency for the one critical ISR. It's just a 'bonus' that the other ISRs have an opportunity to pre-empt as well. Since the critical interrupt has the highest priority (lowest vector) of all used interrupts, there is no negative consequence to this.

david.prentice wrote:
Your regular latency is (preamble + body + postamble) of the slowest ISR().
That's the added latency above and beyond any given ISR's own latency. In my case, the critcal ISR could experience 168 cycles of latency before its own.
david.prentic wrote:
With approach(2), the latency is (preamble + disable_other_isrs).
With my approach the latency is reduced somewhat further since the 'preamble' in this case is only the interrupt response time (7 to 10 cycles), and doesn't include the ISR's prologue.

david.prentice wrote:
Or you could change to an Xmega.
Yes, I'd meant to put that in the list. It would be my first XMEGA project. Looking forward to it when it happens, but it likely won't be this one.

dak664 wrote:
What would be the proper response to the hardware issues leading to stack overflow? You can maybe examine the SP in one or more of the interrupts and when it gets too deep do whatever is necessary to unwind.
I had considered, and dismissed that. But now you've got me thinking about it some more.

There are two interrupt sources which could flood in under extraordinary hardware conditions. One is the critcal interrupt. The other, coincidentally, is the slowest ISR. I will likely write the critical ISR in assembly in order to better control it's own latency, and could pretty easily add SP check code. If a stack overflow is imminent, a corrective action could be undertaken.

The trouble I see is with the longer non-blocking ISR. Any SP checking code would come after the lengthy prologue. Since ISR_NOBLOCK enables global interrupts before the prologue, it would be possible for the ISR to be pre-empted before any of the SP checking code runs. If the interrupts flood in fast enough for long enough, a stack overflow could nevertheless occur.

I could either rewrite the entire lengthy ISR in assembly, or write the SP check as assembly and place it in a naked ISR wrapped around a 'signalised' function. Since any SP check code is likely to be longer than the code to disable a single interrupt source, I'm not sure this is an improvement.

I will continue to think about it... there may be something I'm overlooking.

danni wrote:
I had a similar problem, one fast and high priority interrupt and all others with low priority.

Since the AVR support no priority level, only the high priority interrupt was handled as real interrupt.
To handle all other interrupts, I use a timer interrupt, which can be defined as NOBLOCK without interrupting itself.
The timer interval must only be longer, than the execution time of the fast interrupt.

And this timer interrupt poll all the low priority interrupts

An elegant hybrid of interrupts and polling.

Under normal conditions, this could work quite well. The problem in my case is that in abnormal conditions, the critical interrupt could flood in faster than it's ISR can handle. Under these conditions, the rest of the system is starved for CPU cycles. Even a long-period timer interrupt, if non-blocking, would eventually nest. If this nesting were to continue, a stack overflow would be assured.

Thank you all for your ideas and your time.
Still thinking...

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:
skeeve wrote:
You need __vect* to avoid the warning.
Yes, but it's the intrusion into the reserved namespace which bothers me. I suppose that's better than a warning or error...
If you mean the __* namespace, workarounds are a lot of work.

You might invoke avr-gcc without -Werror through a wrapper .
If the only warning or error was about your misspelled signal handler,
the wrapper would return success.
Otherwise the wrapper would remove the .o file and return failure.

You might compile with -S -ffunction-sections -o tmp.s and edit the assembly.

// use awk or sed to insert .subsection 1 just before label
ISR(...)
{
asm (" .subsection 0\n"
     " CBI #0, #1\n"
     " .subsection 1\n" :: "I" _SFR_IO_ADDR(...), "I" (BIT_NUMBER):);
....
asm (" SBI #0, #1\n":: "I" _SFR_IO_ADDR(...), "I" (BIT_NUMBER):);  // to parallel CBI, could be C
}

If you are really brave, you might edit the compiler:

      if (!STR_PREFIX_P (name+1, "_vector"))
        warning_at (loc, 0, "#qs appears to be a misspelled #s handler",
                    name, isr);

You are then allowed names of the form ?_vector* without intruding on the _* namespace.

I know how to use entities, but it's a pain.

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles

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

Exposing his deep knowledge, skeeve wrote:
If you mean the __* namespace, workarounds are a lot of work.
You speak the truth.

Quote:
You might invoke avr-gcc without -Werror through a wrapper .
If the only warning or error was about your misspelled signal handler,
the wrapper would return success.
Otherwise the wrapper would remove the .o file and return failure.
I had considered that kind of approach. It's a generic technique which can be applied to any number of other pesky but otherwise benign warnings. My preference is to intrude as little as possible into the 'standard' build process. A build which requires special handling of this kind (although arguably common) is to me somewhat less elegant. Avoiding a warning (even a benign one) by means of proper technique within the source code seems better than trapping and suppressing it as part of the build process. Nevertheless, it's a good idea to keep in the tool kit.

Quote:
You might compile with -S -ffunction-sections -o tmp.s and edit the assembly.
// use awk or sed to insert .subsection 1 just before label

Although firmly in the 'less elegant' category described above, I do like it. Feeds my need to use awk/sed wherever possible ;)

Isn't your script missing a cli before the sbi in the second asm? It also assumes that the ISR was declared with ISR_NOBLOCK, otherwise you'd also need an sei within the first asm. The second asm would need to be inserted right before the reti, of course.

I'd considered writing a C macro to generate the 'safer' non-blocking wrapper. Your sed example pushed me over the edge:

#define ISR_SAFER_NOBLOCK(vect, ioreg, bit)                         \
void __attribute__ ((__signal__)) __vector_body_of_ ## vect(void);  \
ISR(vect, ISR_NAKED) {                                              \
  if (_SFR_MEM_ADDR(ioreg) <= 0x3F) {                               \
  __asm__ __volatile__ (                                            \
                        "sei            \n\t"                       \
                        "cbi %[i], % \n\t"                       \
                        "call %x[f]     \n\t"                       \
                        "cli            \n\t"                       \
                        "sbi %[i], %[b] \n\t"                       \
                        "reti           \n\t"                       \
                      :                                             \
                      :                                             \
                        [i] "I" (_SFR_IO_ADDR(ioreg)),              \
                        [b] "I" (bit),                              \
                        [f] "s" (__vector_body_of_ ## vect)         \
                       );                                           \
  }                                                                 \
  else {                                                            \
  uint8_t reg;                                                      \
  __asm__ __volatile__ (                                            \
                        "push   %[r]            \n\t"               \
                        "in     %[r], __SREG__  \n\t"               \
                        "push   %[r]            \n\t"               \
                        "lds    %[r], %[i]      \n\t"               \
                        "cbr    %[r], %[b]      \n\t"               \
                        "sei                    \n\t"               \
                        "sts    %[i], %[r]      \n\t"               \
                        "call   %x[f]           \n\t"               \
                        "cli                    \n\t"               \
                        "sbr    %[r], %[b]      \n\t"               \
                        "sts    %[i], %[r]      \n\t"               \
                        "pop    %[r]            \n\t"               \
                        "out    __SREG__, %[r]  \n\t"               \
                        "pop    %[r]            \n\t"               \
                        "reti                   \n\t"               \
                      :                                             \
                        [r]  "=d" (reg)                             \
                      :                                             \
                        [i] "M" (_SFR_MEM_ADDR(ioreg)),             \
                        [b] "I" (bit),                              \
                        [f] "s" (__vector_body_of_ ## vect)         \
                       );                                           \
  }                                                                 \
}                                                                   \
void __vector_body_of_ ## vect(void)

Then you declare the ISRs:

ISR_SAFER_NOBLOCK(PCINT0_vect, PCICR, PCIE0) {
  ...
}

ISR_SAFER_NOBLOCK(INT0_vect, EIMSK, INT0) {
  ...
}

It is the responsibility of the programmer to provide a matching vector name, I/O register, and bit for the intended interrupt, but the macro correctly handles the choice between [b]cbi/sbi and RWM.

It works at all optimisation levels (even -O0!), and -Wl,--relax is able to change the call into an rcall when possible.

It differs from your post-compilation assembly-editing script in that there is the call/rcall and the associated second reti, but these do not contribute to the latency experienced by any pending interrupts, since note that I've moved the sei to before the cbi/sts which disables the single interrupt source. Hardware guarantees that the cbi/sts will execute before any pending interrupts are serviced.

I'm uncertain whether the servicing of a new pending interrupt from the same source will be suppressed in this case, i.e. whether:

sei
cbi EIMSK, INT0

... and:

ldi r24, (1<<PCIE0)
sei
sts PCICR, r24

... are subject to the same guarantees as:

sei
cli

I shall have to contrive a test to satisfy myself that they are.

skeeve wrote:
If you are really brave, you might edit the compiler:
Yes I mentioned that snippet of code in my OP. I'm not that 'brave', though ;) ... no need to start hacking the compiler for such little return.

I might consider a feature request for a couple of switches like -Wno-signal-warning= and -Wno-signal-error=.

skeeve wrote:
I know how to use entities, but it's a pain.
I'm afraid I don't understand the meaning of 'entities' in this context. Perhaps a gaping hole in my knowledge of C?

///////////////////////////////////////////////////////////////////////////////

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

SprinterSB wrote:
Yes, you can use an assembler name for the function. The feature has been added to avr-gcc as PR57631.
PR57631 didn't make it into 3.4.3.

I just download 3.4.4 and can see from the source that PR57631 is in.

However, I can't seem to figure out how to apply your advice. Whereas I (intruding into reserved namespace) have declared a macro beginning with:

#define ISR_SAFER_NOBLOCK(vect, ioreg, bit)                         \
void __attribute__ ((__signal__)) __vector_body_of_ ## vect(void);  \

... what are you suggesting I do? I'm uncertain how to 'use an assembler name for the function'.

I understand this, but I'm not certain how to construct an assembler name for the function which doesn't begin with __ but which will pass the tests added by PR57631, nor am I certain that this is at all what you're suggesting.

Quote:
What's nice: You know the rules for code that might go into naked funktions, yet you are breaking it ...
Eh? What rules would those be?:
In the documentation, someone wrote:
Another solution is to still implement the ISR in C language but take over the compiler's job of generating the prologue and epilogue. This can be done using the ISR_NAKED attribute to the ISR() macro. Note that the compiler does not generate anything as prologue or epilogue, so the final reti() must be provided by the actual implementation. SREG must be manually saved if the ISR code modifies it, and the compiler-implied assumption of __zero_reg__ always being 0 could be wrong (e. g. when interrupting right after of a MUL instruction).
.
.
.

#define ISR_NAKED

ISR is created with no prologue or epilogue code. The user code is responsible for preservation of the machine state including the SREG register, as well as placing a reti() at the end of the interrupt routine.

Use this attribute in the attributes parameter of the ISR macro.

Nevertheless:
joeymorin wrote:
The main risk is the use of ISR_NAKED. In this case the reason it isn't a problem is because EIMSK &= ~(1<<INT1); compiles to a single cbi which has no side-effects. If there were side effects, the naked ISR would need to be written in assembler (or inline assembler). Indeed, this is the only recommended approach for any naked ISR.
The naked ISRs I have proffered save and restore all of the state that they modify, and they call a single function declared with attribute signal, ensuring that it saves and restores all of the state which it modifies. The single naked ISR written in C in the OP modifies no state at all.

So what am I missing?

Quote:
Is the architecture as you outlined above, i.e.
Not quite:
  • The high-prio ISR is short and (can be) in (inline) asm
  • Of the low-prio ISRs, some are short, some are longer (but not 'fat'), some are in (inline) asm, and some are in C.
Quote:
Why do you need to call foo? May it help to make the call to foo transparent (will only work if foo stands in asm or naked C)?
That was just a trivial example. For the (inline) asm low-prio ISRs, I would just write the 'safer, non-blockifying' code into the existing (inline) asm. The function (foo in the trivial example) would contain the C code normally contained within the ISR, and would be declared with attribute signal.

The net effect is merely to inject 'pre'-prologue and 'post'-epilogue (but 'pre'-reti) code fragments into the generated ISR which handles the desired 'safer' non-blocking behaviour.

Quote:
How many low-prio ISRs are there?
At the moment at least two, possibly four or more.

Quote:
If a tight main loop + poll can replace your low-prio ISRs, I'd use that alternative. No more bother about latencies or switching on/off IRQ sources.
[sigh]:
joeymorin wrote:
Please exclude from your responses any suggestions that the non-blocking ISRs be replaced with a polling loop. I already know ;)

Quote:
From my experience, NOBLOCK is not really useful. Better use BLOCK ISR, disable the IRQ source, sei, worker-code, cli, enable IRQ source and then return.
Gosh, I wish I'd said that! ... It's more or less the thesis of my OP ;)

Quote:
This will add latency, and without counting how much lateny is actually risen, I'd expect it's too much for your timing constraints.
The added latency is, I believe, minimised by my proffered approach, is easily quantifiable, and for the purposes of this project is likely to be tolerable. If it turns out not to be, polling will be my most likely recourse.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

skeeve wrote:
I was trying to avoid calls because I thought you did not have time for them.
They don't contribute to the overall latency, so it's not a big deal.
Quote:
I'm sure they are more legible.
Yes, but it's also easier to handle in a macro as above. I can't think of a way to do this (within the source) without a function and the associated call/reti. The compiler doesn't seem willing to inline a function declared with attribute signal, which isn't surprising.

Quote:
The bodies still have to be declared signal handlers.
Just giving them signal handler names does not do the trick.
Of course, which is why I did ;)

Quote:
Entities are what html allows in place of individual characters.
html does not require entities for percent signs,
but this web site often chokes on explicit percent signs.
You browser seems to put them in automatically.
Oh I see ;) ... and when I googled for C entities I nevertheless received a lot of references to HTML, which I dismissed it as irrelevant. I understand now you're referring of course to the forum software's difficulty with accepting the '%' sign within posts. Obvious now that I see you substituted a '#'.

My browser has no '%' magic (although I'm told Greasemonkey does). I compose my lengthier posts in a text editor and copy/paste into the browser before submitting. I use the text editor to 'replace-all' '%' with '%' to avoid the 'Bad Request' error from the forum software.

EDIT: Ah, you seem to have edited your post since I started composing my response:

Quote:
I'd have hard-coded a register instead of using reg,
but that is the only potential problem I see.
What advantage is there to hard-coding vs. allowing the compiler to select one for me? Letting it do so, and employing the proper constraint and modifiers at least ensures that the programmer won't try to ldi into a lower register, or something else equally silly. Reviewing my code I realise that I could have simply used "=r" instead of "=d" in this case. In any event since this code constitutes, effectively, a separate thread within its own context, I appreciate there is no real advantage in doing so, but I always feel it is a good habit to let the compiler make choices wherever possible. This will help in the even a code fragment is transplanted elsewhere later. I certainly see no advantage in hard-coding it.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:
SprinterSB wrote:
Yes, you can use an assembler name for the function. The feature has been added to avr-gcc as PR57631.
PR57631 didn't make it into 3.4.3.
Quote:
However, I can't seem to figure out how to apply your advice. Whereas I (intruding into reserved namespace) have declared a macro beginning with:
#define ISR_SAFER_NOBLOCK(vect, ioreg, bit)                         \
void __attribute__ ((__signal__)) __vector_body_of_ ## vect(void);  \

... what are you suggesting I do? I'm uncertain how to 'use an assembler name for the function'.

I understand this, but I'm not certain how to construct an assembler name for the function which doesn't begin with __ but which will pass the tests added by PR57631, nor am I certain that this is at all what you're suggesting.

You can't.
The implicit claim is that ISR(foo) asm("__vector_foo")
does not violate the reserved namespace.
__vector_foo is not a token never seen as a symbol by the C compiler.
If you disagree and are unwilling to violate the reserved namespace,
we are back to editing a standard ISR
ISR(...)
{
// use awk or sed to insert .subsection 1 just before label
// use awk or sed to remove marked ret or reti ending the function
// use awk or sed to remove marked nop
// if there is more than one ret or reti, more editing is needed
// be sure to use -ffunction-sections .
asm (" .subsection 0\n"
     " CBI #0, #1\n"
     " .subsection 2\n"
     " CLI\n"
     " SBI #0, #1\n"
     " RETI\n"
     " .subsection 1\n"
     " NOP ; MARKED for removal\n"::"I" _SFR_IO_ADDR(...), "I" (BIT_NUMBER) );
// editing does not change length of preceding code
...
asm (" ; use awk or sed to delete the ret or reti following this MARK\n");
}

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles

Last Edited: Fri. Jun 13, 2014 - 02:44 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

With my emphasis, skeeve wrote:
You can't.
The implicit claim is that ISR(foo) asm("__vector_foo")
does not violate the reserved namespace.
__vector_foo is not a token ever seen as a symbol by the C compiler.
I perceive.

Quote:
If you disagree and are unwilling to violate the reserved namespace,
we are back to editing a standard ISR
I didn't and don't disagree. I just didn't get it. All better now :shock:

Here's a somewhat more thorough macro:

#define prestringify(s) #s
#define stringify(s) prestringify(s)
#define ISR_ONESHOT 1 // For ISRs which need to remain disabled
#define ISR_SAFER_NOBLOCK(vect, ioreg, bit, ...)                    \
void __attribute__ ((__signal__)) body_of_ ## vect(void)            \
            __asm__ ("__vector_body_of" stringify(vect));           \
ISR(vect, ISR_NAKED) {                                              \
  if ((__VA_ARGS__ + 0) == ISR_ONESHOT) {                           \
    if (_SFR_MEM_ADDR(ioreg) < (0x20 + __SFR_OFFSET)) {             \
      __asm__ __volatile__ (                                        \
                            "cbi %[i], %[b] \n\t"                   \
                            "sei            \n\t"                   \
                            "nop            \n\t"                   \
                            "call %x[f]     \n\t"                   \
                            "reti           \n\t"                   \
                          :                                         \
                          :                                         \
                            [i] "I" (_SFR_IO_ADDR(ioreg)),          \
                            [b] "I" (bit),                          \
                            [f] "s" (body_of_ ## vect)              \
                           );                                       \
    }                                                               \
    else if (_SFR_MEM_ADDR(ioreg) < (0x40 + __SFR_OFFSET)) {        \
      uint8_t reg;                                                  \
      __asm__ __volatile__ (                                        \
                            "push   %[r]            \n\t"           \
                            "in     %[r], __SREG__  \n\t"           \
                            "push   %[r]            \n\t"           \
                            "in     %[r], %[i]      \n\t"           \
                            "cbr    %[r], %[b]      \n\t"           \
                            "out    %[i], %[r]      \n\t"           \
                            "sei                    \n\t"           \
                            "nop                    \n\t"           \
                            "call   %x[f]           \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "out    __SREG__, %[r]  \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "reti                   \n\t"           \
                          :                                         \
                            [r] "=d" (reg)                          \
                          :                                         \
                            [i]  "M" (_SFR_IO_ADDR(ioreg)),         \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
    }                                                               \
    else {                                                          \
      uint8_t reg;                                                  \
      __asm__ __volatile__ (                                        \
                            "push   %[r]            \n\t"           \
                            "in     %[r], __SREG__  \n\t"           \
                            "push   %[r]            \n\t"           \
                            "lds    %[r], %[i]      \n\t"           \
                            "cbr    %[r], %[b]      \n\t"           \
                            "sts    %[i], %[r]      \n\t"           \
                            "sei                    \n\t"           \
                            "nop                    \n\t"           \
                            "call   %x[f]           \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "out    __SREG__, %[r]  \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "reti                   \n\t"           \
                          :                                         \
                            [r] "=d" (reg)                          \
                          :                                         \
                            [i]  "M" (_SFR_MEM_ADDR(ioreg)),        \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
    }                                                               \
  }                                                                 \
  else {                                                            \
    if (_SFR_MEM_ADDR(ioreg) < (0x20 + __SFR_OFFSET)) {             \
      __asm__ __volatile__ (                                        \
                            "cbi %[i], %[b] \n\t"                   \
                            "sei            \n\t"                   \
                            "nop            \n\t"                   \
                            "call %x[f]     \n\t"                   \
                            "cli            \n\t"                   \
                            "sbi %[i], %[b] \n\t"                   \
                            "reti           \n\t"                   \
                          :                                         \
                          :                                         \
                            [i] "I" (_SFR_IO_ADDR(ioreg)),          \
                            [b] "I" (bit),                          \
                            [f] "s" (body_of_ ## vect)              \
                           );                                       \
    }                                                               \
    else if (_SFR_MEM_ADDR(ioreg) < (0x40 + __SFR_OFFSET)) {        \
      uint8_t reg;                                                  \
      __asm__ __volatile__ (                                        \
                            "push   %[r]            \n\t"           \
                            "in     %[r], __SREG__  \n\t"           \
                            "push   %[r]            \n\t"           \
                            "in     %[r], %[i]      \n\t"           \
                            "cbr    %[r], %[b]      \n\t"           \
                            "out    %[i], %[r]      \n\t"           \
                            "sei                    \n\t"           \
                            "nop                    \n\t"           \
                            "call   %x[f]           \n\t"           \
                            "cli                    \n\t"           \
                            "sbr    %[r], %[b]      \n\t"           \
                            "out    %[i], %[r]      \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "out    __SREG__, %[r]  \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "reti                   \n\t"           \
                          :                                         \
                            [r] "=d" (reg)                          \
                          :                                         \
                            [i]  "M" (_SFR_IO_ADDR(ioreg)),         \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
    }                                                               \
    else {                                                          \
      uint8_t reg;                                                  \
      __asm__ __volatile__ (                                        \
                            "push   %[r]            \n\t"           \
                            "in     %[r], __SREG__  \n\t"           \
                            "push   %[r]            \n\t"           \
                            "lds    %[r], %[i]      \n\t"           \
                            "cbr    %[r], %[b]      \n\t"           \
                            "sts    %[i], %[r]      \n\t"           \
                            "sei                    \n\t"           \
                            "nop                    \n\t"           \
                            "call   %x[f]           \n\t"           \
                            "cli                    \n\t"           \
                            "sbr    %[r], %[b]      \n\t"           \
                            "sts    %[i], %[r]      \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "out    __SREG__, %[r]  \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "reti                   \n\t"           \
                          :                                         \
                            [r] "=d" (reg)                          \
                          :                                         \
                            [i]  "M" (_SFR_MEM_ADDR(ioreg)),        \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
    }                                                               \
  }                                                                 \
}                                                                   \
void body_of_ ## vect(void)

Invoking:

ISR_SAFER_NOBLOCK(PCINT0_vect, PCICR, PCIE0) {
  ...
}

ISR_SAFER_NOBLOCK(ANALOG_COMP_vect, ACSR, ACIE, ) {
  ...
}

ISR_SAFER_NOBLOCK(INT1_vect, EIMSK, INT1, ISR_ONESHOT) {
  ...
}
///////////////////////////////////////////////////////////////////////////////

EDIT: Updated macro with bug-fixed code. Will post an explanation soon.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Fri. Jun 13, 2014 - 04:12 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Other alternative (possibly)

why not write the Interrupt service routines completely in assembler?
this would give you control over the prologue and epilogue and you only have to store the registers that you actually use.

if you make a "foo.S" file with:


INT1_vect:
push sreg
push r0
tst r0
pop r0
pop sreg
reti
// INT1_vect

then you should have the shortest possible overhead.

the only thing I am not sure of is if the compiler at this point omits the pro-epilogue. but my guess is that if it is not someone will know.
I know that I have used this in the past on a time critical ISR and that seemed to function correct (one off project) where if the compiler would have added code there was a big chance that the function would not work correctly.

I could not find the entire code snipped on my PC and part of our network is down for maintenance I could not go to the location were my projects are stored.
So I could not check the compilers output.
regards

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

joeymorin wrote:
Under normal conditions, this could work quite well. The problem in my case is that in abnormal conditions, the critical interrupt could flood in faster than it's ISR can handle. Under these conditions, the rest of the system is starved for CPU cycles. Even a long-period timer interrupt, if non-blocking, would eventually nest. If this nesting were to continue, a stack overflow would be assured.

No big problem. You need a counter, which was preset by the timer interrupt and count down inside the external interupt.
If the external interrupt was fired to often, the counter expired and you enter an error handler, e.g. disable the interrupt.
On normal operation the timer interrupt was able to preset this counter.

Also you can shorten the timer inside the interrupt handler.
E.g. with prescaler 256 and preload to 255, the next overflow occur after 256 cycle.
But on high external rate, the timer can interrupt itself not prior 65536 cycle.

Peter

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

joeymorin wrote:
Quote:
I'd have hard-coded a register instead of using reg,
but that is the only potential problem I see.
What advantage is there to hard-coding vs. allowing the compiler to select one for me?
You did what I usually do for a scratch register.
It avoids interfering with the compiler too much.
In this case, we do want to interfere.
In principle, the compiler might have been silly enough to decide reg is an SRAM variable.
In a naked function, I expect that would be messy even if it weren't an ISR.
Also, removing reg would result in smaller less complex code.

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles

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

danni wrote:
No big problem. You need a counter, which was preset by the timer interrupt and count down inside the external interupt.
If the external interrupt was fired to often, the counter expired and you enter an error handler, e.g. disable the interrupt.
On normal operation the timer interrupt was able to preset this counter.
Interesting idea. However, it doesn't immediately preclude reentrancy for the timer interrupt, although that could be managed by a flag tested and set within the timer ISR before going non-blocking, but then that adds to the latency.

Absent such a flag, it would seem a careful analysis of the timing relationship between the two interrupts, both under normal and abnormal conditions, would be required in order to arrive at an appropriate threshold for the counter. If the specs for timing change, or even the operating conditions change, that threshold may no longer be appropriate.

I think it could be a useful technique, but I wonder at the appropriateness of the whole catch-all-hybrid-timer-interrupt-and-polling-loop technique for this particular project.

Of the low-priority tasks, there is one which nevertheless needs to respond within 500 us of it's external trigger. This despite the fact that it may come only every 8 ms. This means the catch-all timer interrupt you suggest would need to fire at least every 500 us, faster for a safety margin, say 400 us.

So this catch-all timer ISR which then polls the interrupt sources for the low-priority tasks will threaten its additional latency to the high-priority ISR much more frequently than if each of those tasks were serviced by their own 'safer' non-blocking ISRs. In this case, at least 20 times more often. And if that timer ISR employs a flag test-and-set before going non-blocking (to preclude reentrancy), that latency will be more than just its 7-10 cycle interrupt response time.

It's unclear to me at this point whether or not my app can withstand the extra latency this often. I'll have to think about it.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

skeeve wrote:
In principle, the compiler might have been silly enough to decide reg is an SRAM variable.
Are you sure?

Automatic variables are placed either in a register, or on the stack. The stack is only ever used for automatics when the register allocator has run out of registers, or if the object is too large to fit in the available registers.

Since reg is the only automatic, is uint8_t, and there are no other variables or other object which need to be loaded into registers (arguments, globals, etc.), it's pretty much guaranteed that the stack (i.e. SRAM) won't be involved. A serious design change in the compiler would be needed to elicit that kind of behaviour.

Quote:
Also, removing reg would result in smaller less complex code.
That depends upon your definition of 'less complex'.

Me:

  if ((__VA_ARGS__ + 0) == ISR_ONESHOT) {                           \
      uint8_t reg;                                                  \
      __asm__ __volatile__ (                                        \
                            "push   %[r]            \n\t"           \
                            "in     %[r], __SREG__  \n\t"           \
                            "push   %[r]            \n\t"           \
                            "lds    %[r], %[i]      \n\t"           \
                            "cbr    %[r], %      \n\t"           \
                            "sts    %[i], %[r]      \n\t"           \
                            "sei                    \n\t"           \
                            "call   %x[f]           \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "out    __SREG__, %[r]  \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "reti                   \n\t"           \
                          :                                         \
                            [r] "=d" (reg)                          \
                          :                                         \
                            [i]  "M" (_SFR_MEM_ADDR(ioreg)),        \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
  }                                                                 \
  else {                                                            \
      uint8_t reg;                                                  \
      __asm__ __volatile__ (                                        \
                            "push   %[r]            \n\t"           \
                            "in     %[r], __SREG__  \n\t"           \
                            "push   %[r]            \n\t"           \
                            "lds    %[r], %[i]      \n\t"           \
                            "cbr    %[r], %[b]      \n\t"           \
                            "sts    %[i], %[r]      \n\t"           \
                            "sei                    \n\t"           \
                            "call   %x[f]           \n\t"           \
                            "cli                    \n\t"           \
                            "sbr    %[r], %[b]      \n\t"           \
                            "sts    %[i], %[r]      \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "out    __SREG__, %[r]  \n\t"           \
                            "pop    %[r]            \n\t"           \
                            "reti                   \n\t"           \
                          :                                         \
                            [r] "=d" (reg)                          \
                          :                                         \
                            [i]  "M" (_SFR_MEM_ADDR(ioreg)),        \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
  }                                                                 \
}                                                                   \

You:

      __asm__ __volatile__ (                                        \
                            "push   r16            \n\t"            \
                            "in     r16, __SREG__  \n\t"            \
                            "push   r16            \n\t"            \
                            "lds    r16, %[i]      \n\t"            \
                            "cbr    r16, %[b]      \n\t"            \
                            "sts    %[i], r16      \n\t"            \
                            "sei                   \n\t"            \
                            "call   %x[f]          \n\t"            \
                          :                                         \
                          :                                         \
                            [i]  "M" (_SFR_MEM_ADDR(ioreg)),        \
                            [b]  "M" (1<<bit),                      \
                            [f]  "s" (body_of_ ## vect)             \
                           );                                       \
  if ((__VA_ARGS__ + 0) == ISR_ONESHOT) {                           \
      __asm__ __volatile__ (                                        \
                            "cli                   \n\t"            \
                            "sbr    r16, %[b]      \n\t"            \
                            "sts    %[i], r16      \n\t"            \
                          :                                         \
                          :                                         \
                            [i]  "M" (_SFR_MEM_ADDR(ioreg)),        \
                            [b]  "M" (1<<bit)                       \
                           );                                       \
  }                                                                 \
      __asm__ __volatile__ (                                        \
                            "pop    r16            \n\t"            \
                            "out    __SREG__, r16  \n\t"            \
                            "pop    r16            \n\t"            \
                            "reti                  \n\t"            \
                          :                                         \
                          :                                         \
                           );                                       \

When hard-coding the register, it permits you to split the asm into 3 parts without fear that the compiler will choose different registers each time, so that you can place the test for [b]ISR_ONESHOT inside instead of outside, but I fail to see how that makes the code 'smaller', 'less complex', or more readable.

I think this is down to a matter of taste, but I may be missing something.

///////////////////////////////////////////////////////////////////////////////

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:
skeeve wrote:
In principle, the compiler might have been silly enough to decide reg is an SRAM variable.
Are you sure?
In principle, it could, but I would not expect it to do so.
One of the hazards of reg is that it attracts cri-tics.
Another is unnecessary source code complexity.
Quote:
Quote:
Also, removing reg would result in smaller less complex code.
That depends upon your definition of 'less complex'.
Removing reg would reduce the number of variables in your C source code.
Removing reg would reduce the number of tokens in your C source code.
Removing reg would reduce the number of tokens in your asm statement.

"Demons after money.
Whatever happened to the still beating heart of a virgin?
No one has any standards anymore." -- Giles